plan: TypeScript SDK MCP authorization lifecycle - #1687
Conversation
667078e to
deb4716
Compare
deb4716 to
9741773
Compare
9741773 to
6ef30bc
Compare
jhrozek
left a comment
There was a problem hiding this comment.
I thought this was fine. The machine insisted on bringing these up:
1. Medium — attachment recovery does not handle the new parked terminal shape
Plan: docs/acceptance/sdk-mcp-authorization-lifecycle.md:234 ADR: docs/adr/0348-typescript-sdk-mcp-authorization-lifecycle.md:184
The plan recommends explicit Session.attach(continuationRunId) as a recovery option. However, the existing attachment:
stops iteration only on result ( sdk/typescript/src/watch.ts:591 );
marks the run no longer live only on result ( sdk/typescript/src/watch.ts:671 ).
A continuation can instead finish by parking on another authorization.required , with no result . Attachment can yield that event but remain live === true and keep watching rather than finish.
Requested change: Explicitly choose between extending attachment to recognize authorization parking, or documenting that applications must detect that event and close the attachment themselves. Add a recovery test for disconnect → attach → replay chained authorization park. “Where those existing APIs support it”
leaves this important case unresolved.
2. Medium — flow termination and subsequent control behavior are underspecified
Plan: docs/acceptance/sdk-mcp-authorization-lifecycle.md:230
AC5.2 groups caller abort, deadline, iterator return, transport loss, and client close under “reject or end according to the normalized contract.” It does not specify their observable differences or what happens to outstanding permission responders and later control calls.
For example: while next() waits for an event and an automatic permission responder is pending, the caller returns the iterator. Must the pending read finish normally or reject? Is the responder’s signal aborted? Can its late verdict still dispatch? Can resolveAsk() or cancelContinuation() be called after the
flow ends?
These are public lifecycle decisions, not implementation details. Existing SDK behavior is not a sufficient default: Run currently has a no-op iterator return() ( sdk/typescript/src/run.ts:276 ), whereas attachment closes its resources in finally ( sdk/typescript/src/watch.ts:595 ).
Requested change: Add a small termination matrix specifying:
pending next() / result() outcome;
responder cancellation and late-verdict suppression;
post-terminal manual-control behavior;
registration and transport cleanup.
Pin the race cases with executable tests.
3. Medium — correlation acceptance promises more than the event carries
Plan: docs/acceptance/sdk-mcp-authorization-lifecycle.md:228
AC5.1 says an event from another session, authorization, call, or continuation cannot be accepted as this flow’s correlation. But:
EventCommon has no session ID ( sdk/typescript/src/events.ts:342 );
authorization payloads contain authorization and call IDs, but no session ID ( sdk/typescript/src/events.ts:153 );
the handle accepts only an authorization ID, so it has no independently known call ID against which to validate the first authoritative frame.
The SDK can enforce session affinity on the request, isolate iterators, compare the authorization ID, and pin the call/run IDs learned from the response. It cannot independently verify every claimed origin from these frames.
Requested change: Separate the guarantees explicitly: session ownership is server-enforced through the session-affined request; authorization ID is checked against the handle; call and continuation IDs are pinned from authoritative frames and checked thereafter. Make the tests assert those actual boundaries
without widening the wire.
Smaller corrections
AC2.1 should specify local rejection of an empty authorization ID, matching the ADR’s “non-empty” requirement ( docs/adr/0348-typescript-sdk-mcp-authorization-lifecycle.md:54 ).
Correct the Converse proto documentation, which still promises closure after terminal result ( contracts/proto/mecatl/v1/harness.proto:100 ). This requires no wire-field change, but the plan should distinguish that from “protobuf: None.”
The empty-run-ID control response, repeated resolution event, and gRPC-versus-HTTP disconnect distinction match the existing server implementation. I would keep those decisions, along with lazy dispatch, separate permission request options, and the explicit no-retry policy.
No files changed; this was a plan and supporting-code review, not an implementation test run.
|
Addressed the review in
The acceptance checker still reports 27/27 ACs and |
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
09f76b0 to
28868b2
Compare
Stage
Plan / Interface
Summary
Runcompletion a normal typed handoffConverseterminal-shape documentation without changing protobuf fieldsHuman decisions
The directing user approved the public object model, initial Run handoff, lazy dispatch,
automatic-control options, bounded recovery contract, attachment terminal behavior,
termination matrix, and correlation boundary recorded in the plan.
Verification
bash .claude/skills/to-acceptance-plan/scripts/check-acceptance-plan.sh docs/acceptance/sdk-mcp-authorization-lifecycle.md— 27 ACs, passedbash .claude/skills/to-acceptance-plan/scripts/check-acceptance-plan-test.sh— passedCGO_ENABLED=0 task docs— passedRelates to #1469