docs: MCP admission contract architecture for Epic #172#182
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a9c09f3df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Joncallim
left a comment
There was a problem hiding this comment.
Architecture-stage review for Epic #172.
I reviewed the proposed contract against the current planner, workforce materializer, approval route, handoff broker, filesystem grant lifecycle, executor, persistence schema, UI readers, child issues, and current origin/main after #175. The inline comments below focus on implementation blockers and contract inconsistencies. The existing unresolved operator-guide comment is valid and is not duplicated here.
Add the canonical, handoff-ready architecture for EPIC #172 (MCP Execution Readiness and Bounded Context Grants) and align existing docs. - New ADR 0009: the unified McpAdmissionDecision contract, capability taxonomy (planning-only / bounded read-only / deferred live MCP), the four-paths-to-one consolidation map, and per-slice file-level implementation specs (S1-S6). - Fix ADR numbering collision: two ADRs were both numbered 0007. Rename the filesystem bounded-context-grants ADR to 0008 (zero inbound refs) and cross-link it to 0009. The forge-agent-workforce-model ADR keeps 0007. - developer-guide: describe the single admission contract, capability classes, and the four-to-one consolidation. - operator-guide: note approval runs the same admission check as handoff; add the four MCP access states operators see on the task page. - roadmap: add the #172 section and its six child slices (#176-#181, #43). - wiki: point the admission flow at the shared contract. Docs only; no code changes. Architecture also lives in issues #176-#181. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ruth
Respond to the 17 inline review comments on ADR 0009 and the guides.
ADR 0009:
- Add per-MCP delivery kind: catalog membership != deliverable. GitHub reads
have no bounded-context producer, so they classify as planning/health-gated
context, never an approvable bounded grant; only filesystem has a producer.
Preserve the current safe-read breadth via a documented SAFE_READ_SUPPLEMENT
migrated verbatim from SAFE_BETA_CAPABILITY_PATTERNS (no behavior change).
- Add a package-normalization phase (admitWorkPackageMcp): union prohibitions
package-wide (deny-wins on every decision) and evaluate MCP-aware subtasks
against normalized coverage (preserves the current broker semantics). [P0]
- Replace the projectGrantCovers boolean with a normalized EffectiveGrantState
(phase/status/source/mode/consumed/coveredCapabilities) so denial and recovery
live in the shared decision.
- Make the decision table total and precedence-ordered; store per-capability
classifications; define required-empty, unknown-capability, mixed-class, and
ask_user outcomes; represent class 'unknown' and 'unknown_legacy'.
- Introduce the McpAdmissionEvaluation envelope ({decision, source, health}) so
the grant-preview/validation/broker adapters stay shape-preserving.
- Do not back-derive grant modes for legacy artifacts (unknown_legacy; UI reads
live package metadata instead of inventing approved state).
- Specify approval's health snapshot: getProjectMcpOverview runs outside the
status-flip transaction and its checkedAt is persisted; narrow the parity
guarantee to "no missed approval-time block", not "never blocks later".
- Reframe the invariant around the canonical package evaluation; the filesystem
helper is a tested projection, not a co-equal source.
- Denied/withheld required filesystem grant holds the package in a recoverable
state (zero attempts, task not failed); one project-wide reconciliation routine
shared by both grant endpoints; later project always-allow supersedes an
earlier package-local denial.
- Evidence lifecycle: planned scope (pre-run) vs issued packet METADATA (post-run,
incl. failed runs); file contents stay prompt-only, never persisted.
- deferred_live_mcp is actionable: required -> revise_plan CTA; optional ->
warning. Persisted broker-block schema versioned (metadata.mcpBroker), with
retryability + primary recovery action aggregation over multiple blocks.
- Assign each edit to one slice with transitional exports; S4 depends on S2,
S5 depends on S4. Refresh executor anchors against main after #175
(mcpCapabilityList 1252 -> 1527, filesystem throw ~1515 -> 1790).
- Note the #43 re-scope (planning/overlay only; runtime-tool AC deferred).
Guides:
- operator-guide, wiki: mark approval/handoff MCP parity as target-state (lands
with S2), not current behavior; narrow the guarantee.
- developer-guide, roadmap: classifier is catalog + safe-read supplement, and a
bounded packet is delivered only where a producer exists.
Docs only; no code changes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
9a9c09f to
6664ec8
Compare
…ruth
Respond to the 17 inline review comments on ADR 0009 and the guides.
ADR 0009:
- Add per-MCP delivery kind: catalog membership != deliverable. GitHub reads
have no bounded-context producer, so they classify as planning/health-gated
context, never an approvable bounded grant; only filesystem has a producer.
Preserve the current safe-read breadth via a documented SAFE_READ_SUPPLEMENT
migrated verbatim from SAFE_BETA_CAPABILITY_PATTERNS (no behavior change).
- Add a package-normalization phase (admitWorkPackageMcp): union prohibitions
package-wide (deny-wins on every decision) and evaluate MCP-aware subtasks
against normalized coverage (preserves the current broker semantics). [P0]
- Replace the projectGrantCovers boolean with a normalized EffectiveGrantState
(phase/status/source/mode/consumed/coveredCapabilities) so denial and recovery
live in the shared decision.
- Make the decision table total and precedence-ordered; store per-capability
classifications; define required-empty, unknown-capability, mixed-class, and
ask_user outcomes; represent class 'unknown' and 'unknown_legacy'.
- Introduce the McpAdmissionEvaluation envelope ({decision, source, health}) so
the grant-preview/validation/broker adapters stay shape-preserving.
- Do not back-derive grant modes for legacy artifacts (unknown_legacy; UI reads
live package metadata instead of inventing approved state).
- Specify approval's health snapshot: getProjectMcpOverview runs outside the
status-flip transaction and its checkedAt is persisted; narrow the parity
guarantee to "no missed approval-time block", not "never blocks later".
- Reframe the invariant around the canonical package evaluation; the filesystem
helper is a tested projection, not a co-equal source.
- Denied/withheld required filesystem grant holds the package in a recoverable
state (zero attempts, task not failed); one project-wide reconciliation routine
shared by both grant endpoints; later project always-allow supersedes an
earlier package-local denial.
- Evidence lifecycle: planned scope (pre-run) vs issued packet METADATA (post-run,
incl. failed runs); file contents stay prompt-only, never persisted.
- deferred_live_mcp is actionable: required -> revise_plan CTA; optional ->
warning. Persisted broker-block schema versioned (metadata.mcpBroker), with
retryability + primary recovery action aggregation over multiple blocks.
- Assign each edit to one slice with transitional exports; S4 depends on S2,
S5 depends on S4. Refresh executor anchors against main after #175
(mcpCapabilityList 1252 -> 1527, filesystem throw ~1515 -> 1790).
- Note the #43 re-scope (planning/overlay only; runtime-tool AC deferred).
Guides:
- operator-guide, wiki: mark approval/handoff MCP parity as target-state (lands
with S2), not current behavior; narrow the guarantee.
- developer-guide, roadmap: classifier is catalog + safe-read supplement, and a
bounded packet is delivered only where a producer exists.
Docs only; no code changes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6664ec8 to
7a8ddca
Compare
Review round 2 — all 18 inline comments addressedThanks for the thorough pass. Every thread is replied-to and resolved. Because the ADR is meant to be handed to a coding agent with no reasoning left to do, I treated the P0/P1s as design gaps and rebuilt the affected parts of ADR 0009 rather than patching prose. The PR stays docs-only; Branch hygiene
ADR 0009 hardening
Release-truth (guides)
Issue graph
Second-pass amendments (beyond the comments)
|
Summary
Documentation for the architecture pass on EPIC #172 — MCP Execution Readiness and Bounded Context Grants. Docs only; no code changes.
docs/adr/0009-mcp-admission-contract.md— the canonical, handoff-ready architecture: the unifiedMcpAdmissionDecisioncontract, the capability taxonomy (planning-only / bounded read-only / deferred live MCP), the four-paths-to-one consolidation map (withfile:lineanchors), and per-slice implementation specs S1–S6.0007. Renamed the filesystem bounded-context-grants ADR to0008(it had zero inbound references) and cross-linked it to0009; the forge-agent-workforce-model ADR keeps0007.Context
The architecture pass also produced GitHub issues that carry the same file-level specs:
ready-for-agent)The root defect: the same MCP capability policy is re-derived by four divergent code paths, and plan approval never runs the handoff broker — so an approved package can stall at handoff. ADR 0009 defines the one contract that makes preview, approval, handoff, and recovery incapable of disagreeing.
Review notes
web/worker/work-package-executor.tswere deliberately left out of this branch.🤖 Generated with Claude Code