Skip to content

fix(storyboard): align gradeRequestSigning/gradeOneVector transport defaults to 'mcp' - #2559

Merged
bokelley merged 8 commits into
adcontextprotocol:mainfrom
fgranata:fix/grader-standalone-transport-default
Aug 18, 2026
Merged

fix(storyboard): align gradeRequestSigning/gradeOneVector transport defaults to 'mcp'#2559
bokelley merged 8 commits into
adcontextprotocol:mainfrom
fgranata:fix/grader-standalone-transport-default

Conversation

@fgranata

Copy link
Copy Markdown
Contributor

Follow-up to #2540: @bokelley's commit aligning the standalone grader's buildOpts transport defaults (?? 'raw'?? 'mcp') was cherry-picked onto the PR branch after the PR had already merged, so it never landed on main — the published 13.0.0 has resolveVectorTransport defaulting to 'mcp' (storyboard path, correct) but gradeRequestSigning/gradeOneVector still default 'raw'. This is his commit (55e16c8) cherry-picked onto current main, unchanged.

Direct gradeRequestSigning callers against MCP agents currently get the raw REST replay (every vector 404s) unless they pass transport: 'mcp' explicitly — we hit exactly this running the vector batch against our production seller.

🤖 Generated with Claude Code

Both `gradeRequestSigning` and `gradeOneVector` constructed `buildOpts`
with `transport: options.transport ?? 'raw'`, bypassing the `'mcp'`
default already established in `probe-dispatch.ts` via
`resolveVectorTransport`. A caller using the standalone grader directly
against an MCP agent without an explicit `transport` option would get raw
REST-target replay, routing 404 before any verifier could run.

Align both call sites to `?? 'mcp'` and update the `GradeOptions.transport`
JSDoc to document `'mcp'` as the default (adcp#6548 follow-up to PR adcontextprotocol#2540).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G4B1ycLynivGzCS8faZ2QS

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Request changes

Request changes — 1 blocking finding.

The transport-default flip from 'raw' to 'mcp' in gradeRequestSigning/gradeOneVector is correct and well-motivated, aligning direct API callers with resolveVectorTransport's existing 'mcp' storyboard default.

Blocking findings

  • .changeset — Missing changeset for a published-API default flip under src/lib/**. Per the repo's mandatory changeset-vs-wire-impact rule, a missing .changeset/*.md on a src/lib/** change (behavior-affecting default flip) is a high finding and must ship with the PR.

Medium findings

  • bin/adcp-grade.js:481 — CLI --help text and transport-mismatch hint still assume the old 'raw' default and now misdirect operators grading REST agents. Should be updated to match the new default.

Add the changeset (and fix the stale CLI help text) to unblock.

Blocking findings

  • [
  • "
  • \
  • .
  • c
  • h
  • a
  • n
  • g
  • e
  • s
  • e
  • t
  • M
  • i
  • s
  • s
  • i
  • n
  • g
  • c
  • h
  • a
  • n
  • g
  • e
  • s
  • e
  • t
  • f
  • o
  • r
  • p
  • u
  • b
  • l
  • i
  • s
  • h
  • e
  • d
  • A
  • P
  • I
  • d
  • e
  • f
  • a
  • u
  • l
  • t
  • f
  • l
  • i
  • p
  • u
  • n
  • d
  • e
  • r
  • s
  • r
  • c
  • /
  • l
  • i
  • b
  • /
  • (
  • r
  • e
  • p
  • o
  • m
  • a
  • n
  • d
  • a
  • t
  • o
  • r
  • y
  • h
  • i
  • g
  • h
  • )
  • "
  • ]

Medium findings

  • bin/adcp-grade.js:481 — CLI --help text and transport-mismatch hint stale after default flip

@fgranata

Copy link
Copy Markdown
Contributor Author

Both findings addressed in 556668d:

  • Changeset added (.changeset/grader-standalone-transport-default.md, patch) covering the default flip and the CLI text.
  • CLI updated for the new default: --help now documents mcp as the default with raw as the REST-binding opt-in, and the transport-mismatch hint fires only on explicit --transport raw runs — with mcp as the resolved default, an unset transport already grades over MCP, so the old raw→mcp retry hint would have misdirected exactly the operators it was written for.

🤖 Generated with Claude Code

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Aug 17, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — subsequent pass, both prior blocking findings resolved.

This is a follow-up to a prior request-changes. Both previously flagged findings are now addressed in the delta:

  • [high] Missing changeset — resolved: .changeset/grader-standalone-transport-default.md is now present.
  • [medium] Stale CLI --help / transport-mismatch hint — resolved: help text and detectTransportMismatch docstring updated for the new mcp default.

Current-run findings: none. The transport-default flip (transport !== 'mcp'transport === 'raw') is consistently applied across the CLI parse path and grader defaults (options.transport ?? 'mcp' at grader.ts:73,82). The patch changeset type is defensible: the affected grade functions are not on any package.json export subpath, and a default-value flip is a behavior change, not a shape break — no changeset understatement.

high_risk is true (bin/adcp-grade.js modified), but per the decision rules this is a heuristic; the reviewer found no medium-or-higher concerns on the modified file, so the modification is presumed safe. No no-auto-approve team match; gated_paths is false so row 2 does not apply. With zero findings and no escalation trigger, this falls through to row 9.

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Aug 18, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve.

Checked:

  • Refactor consolidates transport resolution into a single transport = options.transport ?? 'mcp' local in gradeRequestSigning/gradeOneVector, aligning the preflight MCP-flatten skip to the same default. Behavior-preserving against the default-flip already approved in the prior pass.
  • Moving vector-lookup + preflightSkip ahead of MCP session init in gradeOneVector avoids spending a session/round-trip on skipped/unknown vectors — sound ordering change, no wire-shape re-shaping or fabrication.
  • Tests updated appropriately: raw-agent/noop-server cases now pass transport: 'raw' explicitly, MCP suite drops redundant transport: 'mcp' to assert the new default, plus a new gradeOneVector default-MCP case covering the added branch.
  • Changeset present as patch; impact is behavior-preserving so patch is appropriate. No removed/renamed exports, no required-param flips, no response-shape changes.

High-risk flag is true only because bin/adcp-grade.js was modified; the reviewer found no medium-or-higher concern on that file, so the modification is presumed safe. No deletions. gated_paths is false and no no-auto-approve team match. No blocking or medium findings — falls through to row 9.

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Aug 18, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve.

Delta since the prior approve is a single-line change: pinning transport: 'raw' on the gradeOneVector call in the #668 capability-profile-mismatch test (adcp-3-0-blockers.test.js:738). Because this PR flips the gradeOneVector default from 'raw' to 'mcp', an unset transport would attempt initializeMcpSession against the unreachable 127.0.0.1:1 probe before the raw failure the test asserts on. The pin preserves the original short-circuit and matches the same pin applied across the e2e and replay-window suites — correct and necessary.

Changeset present (.changeset/grader-standalone-transport-default.md) covering the bin/** and src/lib/** transport-default change, satisfying the changeset-vs-wire-impact rule. No witness-not-translator or transport-client concerns.

Decision table: no critical/high/medium findings; gated_paths is false; high_risk is true but the only reason is bin/adcp-grade.js (modified) with no medium-or-higher finding on it (row 5 requires a medium finding); no deletions; no author team gate. Rows 1–8 do not fire → row 9 approve.

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Aug 18, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve.

This is a subsequent pass over the delta since a prior clean approve. The changes swap the hand-rolled JSON-RPC initialize for the official @modelcontextprotocol/sdk Client + StreamableHTTPClientTransport (satisfying the official-transport mandate), add caller-cancellation via combineAbortSignals with balanced listener cleanup, keep MCP initialize auth out of process argv, carry the server-negotiated MCP-Protocol-Version into signed probes, skip MCP session allocation when all vectors are skipped/local, and extend MCP_FLATTENED_VECTORS to 009-012. All paths are tested. A changeset (.changeset/grader-standalone-transport-default.md) is present, covering the bin/** and src/lib/** changes.

Decision trace:

  • No critical/high findings → row 1 not triggered.
  • gated_paths: false → row 2 not triggered (despite review_decision: REVIEW_REQUIRED).
  • high_risk: true but only reason is bin/adcp-grade.js (modified), no (deleted) → row 3 not triggered.
  • No medium findings at all → rows 4, 5, 6, 8 not triggered.
  • No no-auto-approve team match → row 7 not triggered.
  • Falls through to row 9 → approve.

The single high-risk reason is a modification with no medium-or-higher concerns, so it is presumed safe per the change-kind guidance.

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Aug 18, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve.

What I checked

  • Delta since the prior approve (head d276fae2e73523) is test-only: four calls in test/request-signing-runner-integration.test.js now pass request_signing: { transport: 'raw' } to opt into raw REST replay against the reference verifier, matching the PR's default-transport flip from raw to mcp. The opt-in is applied precisely to tests that dispatch and assert on HTTP status/headers; skip-path tests that short-circuit before dispatch are correctly left untouched.
  • Changeset present (.changeset/grader-standalone-transport-default.md) covering the src/lib/** and bin/** changes — satisfies changeset-vs-wire-impact.
  • high_risk: true from bin/adcp-grade.js (modified), but per the high-risk-flag rules a modification with no medium-or-higher findings is presumed safe; no destructive/deleted change.
  • gated_paths: false, so row 2 does not apply despite review_decision: REVIEW_REQUIRED.
  • No no-auto-approve team match; no prior escalation.

Decision path: No critical/high/medium findings → rows 1–8 do not fire → row 9 approve.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve.

Focused 15-file bugfix aligning gradeRequestSigning/gradeOneVector transport defaults from raw to mcp, restoring intended 13.0 behavior lost to a cherry-pick (storyboard runner already defaulted mcp). initializeMcpSession is rewritten onto the official @modelcontextprotocol/sdk Client + StreamableHTTPClientTransport (satisfies the official-transport-clients rule and completes the initialize lifecycle), mcpProtocolVersion is propagated, a hasRunnableNetworkVector guard avoids allocating an MCP session when all vectors skip/run locally, abort-signal composition includes a verified listener-dispose path, and the initialize bearer is kept out of argv via ADCP_GRADE_INITIALIZE_AUTHORIZATION. REST-binding tests correctly updated to transport:'raw'. Changeset present (.changeset/grader-standalone-transport-default.md) covering the src/lib/** and bin/** changes.

Decision path: no critical/high/medium findings, so rows 1 falls through. gated_paths is false (row 2 N/A). high_risk is true only via bin/adcp-grade.js (modified) — no (deleted) reason (rows 3/5 require a medium finding, none exists). No data-loss/schema/infra medium (row 4 N/A). Prior decision was approve, not escalate (row 6 N/A). No no-auto-approve team match (row 7 N/A). Zero medium findings (<3, row 8 N/A). Falls through to row 9 → approve. review_decision: REVIEW_REQUIRED does not block approval since gated_paths is false.

Blocking findings: none.
Medium findings: none.

@bokelley
bokelley merged commit 8c01d10 into adcontextprotocol:main Aug 18, 2026
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants