fix(storyboard): align gradeRequestSigning/gradeOneVector transport defaults to 'mcp' - #2559
Conversation
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
There was a problem hiding this comment.
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 undersrc/lib/**. Per the repo's mandatory changeset-vs-wire-impact rule, a missing.changeset/*.mdon asrc/lib/**change (behavior-affecting default flip) is a high finding and must ship with the PR.
Medium findings
bin/adcp-grade.js:481— CLI--helptext 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
…ort default; add changeset
|
Both findings addressed in 556668d:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
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.mdis now present. - [medium] Stale CLI --help / transport-mismatch hint — resolved: help text and
detectTransportMismatchdocstring updated for the newmcpdefault.
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.
There was a problem hiding this comment.
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 redundanttransport: '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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 (despitereview_decision: REVIEW_REQUIRED).high_risk: truebut only reason isbin/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.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
What I checked
- Delta since the prior approve (head d276fae → 2e73523) is test-only: four calls in
test/request-signing-runner-integration.test.jsnow passrequest_signing: { transport: 'raw' }to opt into raw REST replay against the reference verifier, matching the PR's default-transport flip fromrawtomcp. 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 thesrc/lib/**andbin/**changes — satisfies changeset-vs-wire-impact. high_risk: truefrombin/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 despitereview_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.
There was a problem hiding this comment.
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.
Follow-up to #2540: @bokelley's commit aligning the standalone grader's
buildOptstransport 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 hasresolveVectorTransportdefaulting to'mcp'(storyboard path, correct) butgradeRequestSigning/gradeOneVectorstill default'raw'. This is his commit (55e16c8) cherry-picked onto current main, unchanged.Direct
gradeRequestSigningcallers against MCP agents currently get the raw REST replay (every vector 404s) unless they passtransport: 'mcp'explicitly — we hit exactly this running the vector batch against our production seller.🤖 Generated with Claude Code