Conversation
fa78d28 to
05dda8b
Compare
05dda8b to
559479c
Compare
|
Fully or partially written by an AI agent (Claude Code Panel review — PR #1692 (feat(sdk): add MCP authorization lifecycle)Fixed point: Spec — does this implement what was asked?Source: Traced all 27 ACs against the actual implementation (
Standards — does this follow project conventions?Sources read: AGENTS.md, ADR 0348, ADR 0304, ADR 0347
No Test adequacy — do the tests independently prove the contract?Traced all 27 ACs to their named tests independently of the implementation. Most are solid: real discriminated-outcome assertions, real transport call-count/ordering checks, genuine planted malformed-frame cases (7 distinct malformed-authorization cases for AC3.3, 6 grammar-violation cases for AC4.1), and a genuinely real-wire e2e suite for AC5.5/AC6.1 (spawns the real
No blockers found; AC3.5 (single-consumption, both directions) was specifically checked and is fine. Domain — what do the specialist reviewers say?Panel: secure-code-reviewer, software-architect, code-duplication-reviewer, library-reuse-reviewer Ship-blockers
Cross-confirmed (1)
Mechanical fixes (3)
Judgement calls (1)
Polish (2)
Gaps
Summary
Most important single issue: the Standards finding — ADR 0348 asserts "no server transition" while Each axis is orthogonal — none of these mask each other. The implementation itself is faithful to the acceptance plan and the security posture is sound; the open items are documentation accuracy, two test-proof gaps in Scenario 5 (concurrency/recovery), and mechanical duplication cleanup in a PR that adds the 4th copy of an existing pattern. PANEL: ship_blockers=0 important=7 advisory=5 reviewer_failures=0 |
kantord
left a comment
There was a problem hiding this comment.
Fully or partially written by an AI agent (Claude Code /panel-review).
Full 4-axis panel review posted above: ship_blockers=0, important=7, advisory=5, reviewer_failures=0.
Requesting changes — not because of a functional defect (the implementation is faithful to all 27 ACs and the security posture is sound), but for one accuracy issue and two test-proof gaps worth closing before merge:
- Standards (important): ADR 0348 §8 states "no ... server transition" changes, but
internal/adapter/server/grpc.go'srelayMCPAuthorizationControlgained new cancel-on-EOF-while-parked-on-ask behavior that didn't exist before. The behavior itself is correct and tested — the ADR text (or the PR's "Interfaces match approved contract: Yes" framing) should acknowledge it. - Test adequacy (important): AC5.1's "concurrent flows" test drives two flows sequentially, not concurrently — it doesn't prove the independence claim it names.
- Test adequacy (important): AC1.3 never actually calls
session.mcpAuthorization(...)after a park to prove "the same Session can immediately create its lifecycle handle."
The remaining 4 important / 5 advisory Domain findings (duplicated runResult(), a 4th copy of the #claim single-consumption guard, a hand-rolled timer duplicating AbortSignal.timeout(), and a test file reinventing a fixture this same PR introduced) are mechanical cleanup, not correctness risks — worth doing while this code is fresh, not necessarily blocking if the team prefers a fast-follow.
559479c to
be4d38c
Compare
|
Addressed the three changes-requested review points on the current stacked head:
The follow-up fixed-point review also found and repaired two related relay races: EOF is remembered when it arrives before a later ordinary ask, and plan-originated asks are excluded from automatic disconnect cancellation. Automatic permission controls now use ask-scoped abort lifetimes so a retired ask cannot dispatch a late verdict. Local focused validation is green, including 329 SDK tests, SDK lint/typecheck, server adapter tests, and repository lint. The stack is synced at |
be4d38c to
727d761
Compare
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Samuele Verzi <samu@stacklok.com>
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Samuele Verzi <samu@stacklok.com>
Co-authored-by: Codex <noreply@openai.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>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Samuele Verzi <samu@stacklok.com>
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Co-Authored-By: Codex <noreply@openai.com> Signed-off-by: Samuele Verzi <samu@stacklok.com>
Co-Authored-By: Codex <noreply@openai.com> Signed-off-by: Samuele Verzi <samu@stacklok.com>
727d761 to
56ddb9a
Compare
Summary
Runas a normal typed handoff, including chained authorization and exact-run continuation controlsDevelopment stage
Implementation, stacked on Plan / Interface PR #1687.
Contract linkage
09f76b009691dc9cbb73b75ff7e12649b36e64f9be4d38c5721fb3c61a01261b0d62fa1bbae95e56Interface conformance
Interfaces match approved contract: Yes. The #1687 amendment makes exact-run attachment parking terminal, specifies the termination matrix and correlation boundaries, rejects empty authorization IDs locally, and documents the existing relay behavior without changing protobuf fields.
Review response
The changes-requested review on this PR is addressed:
session.mcpAuthorization(...)on the same session.Verification
task sdk:lint,task sdk:typecheck,task lint,task site:build, and the server adapter package passedPANEL: ship_blockers=0 important=10 advisory=3 reviewer_failures=0; all correctness and documentation-contract findings were repaired before submitPlain
task testandtask docsremain host-limited on this macOS 27 machine because its installed TAPI/linker cannot parse the SDK.tbdarchitecture entries. The equivalent focused and CGO-disabled paths passed during development.Issue relationship
Closes #1469
Stack
#1686 (merged) → #1687 → #1692