From 3ade0ed035f159ce6cd612d0ba75c16cdfc00654 Mon Sep 17 00:00:00 2001 From: Joncallim <64296013+Joncallim@users.noreply.github.com> Date: Fri, 10 Jul 2026 12:13:38 +0800 Subject: [PATCH 1/3] docs: MCP admission contract architecture for Epic #172 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 --- ...-filesystem-mcp-bounded-context-grants.md} | 9 +- docs/adr/0009-mcp-admission-contract.md | 358 ++++++++++++++++++ docs/developer-guide.md | 17 + docs/operator-guide.md | 20 +- docs/roadmap.md | 28 +- docs/wiki.md | 7 + 6 files changed, 435 insertions(+), 4 deletions(-) rename docs/adr/{0007-filesystem-mcp-bounded-context-grants.md => 0008-filesystem-mcp-bounded-context-grants.md} (88%) create mode 100644 docs/adr/0009-mcp-admission-contract.md diff --git a/docs/adr/0007-filesystem-mcp-bounded-context-grants.md b/docs/adr/0008-filesystem-mcp-bounded-context-grants.md similarity index 88% rename from docs/adr/0007-filesystem-mcp-bounded-context-grants.md rename to docs/adr/0008-filesystem-mcp-bounded-context-grants.md index fc53c7e..7a1571f 100644 --- a/docs/adr/0007-filesystem-mcp-bounded-context-grants.md +++ b/docs/adr/0008-filesystem-mcp-bounded-context-grants.md @@ -1,8 +1,13 @@ -# ADR 0007: Filesystem MCP Bounded Context Grants +# ADR 0008: Filesystem MCP Bounded Context Grants ## Status -Accepted +Accepted. The bounded read-only filesystem grant defined here becomes one branch +of the unified MCP admission contract in ADR +[0009](0009-mcp-admission-contract.md) (EPIC #172): filesystem +read/list/search map to the `bounded_read_only` capability class and the +`bounded_context_required` / `bounded_context_approved` admission modes, and +`filesystem.project.write` is classified `planning_only` (warn, never block). ## Context diff --git a/docs/adr/0009-mcp-admission-contract.md b/docs/adr/0009-mcp-admission-contract.md new file mode 100644 index 0000000..a046a4a --- /dev/null +++ b/docs/adr/0009-mcp-admission-contract.md @@ -0,0 +1,358 @@ +# ADR 0009: MCP Admission Contract and Bounded Context Grants + +## Status + +Accepted for EPIC #172 (MCP Execution Readiness and Bounded Context Grants). +Establishes the single admission contract; the implementation slices (#172 → S1–S6 +child issues) build on it. Supersedes the code-level policy scattered across the +four admission paths described below. Related: ADR +[0006](0006-executable-workforce-beta-boundary.md) (beta boundary), +[0008](0008-filesystem-mcp-bounded-context-grants.md) (filesystem bounded context +grants), and #43 (Architect-driven MCP assignment and prompt overlays). + +## Context + +Forge distinguishes three kinds of MCP involvement, and the whole epic exists +because the code does not distinguish them in one place: + +1. **Planning-only MCP context** — the Architect says an MCP would help. Forge + records it as run-scoped prompt instructions (`promptOverlay`, + `mcpAwareSubtasks`). It grants nothing. +2. **Bounded read-only context grants** — Forge assembles a project-scoped, + inspectable read-only context packet (file names, selected excerpts) after an + explicit operator grant. Capabilities: `filesystem.project.read`, + `filesystem.project.list`, `filesystem.project.search`. +3. **Live MCP tool handles** — a worker calling an MCP directly at runtime. This + is **deferred**. It is not implemented in this beta and must read as a product + boundary, not a broken install. + +Today the same beta capability policy is re-derived by **four overlapping code +paths** that do not agree: + +| # | Path | Location | Consumes | Purpose | +|---|------|----------|----------|---------| +| 1 | `validateMcpExecutionDesign` | `web/worker/mcp-execution-design.ts:326-454` | parsed `McpExecutionDesign` fence | approval-time validation | +| 2 | `deriveMcpGrantDecisions` / `decisionStatus` | `web/worker/mcp-execution-design.ts:753-867` | same design | grant-decision preview | +| 3 | `evaluateWorkPackageMcpBroker` | `web/worker/mcp-execution-design.ts:599-739` | persisted `workPackages.mcpRequirements` + `metadata.mcpGrants` | handoff-time broker | +| 4 | `requiresFilesystemGrantApproval` / `summarizeFilesystemCapabilities` | `web/lib/mcps/filesystem-grants.ts:90-339` | `mcpRequirements` + `metadata` | filesystem grant gate | + +A **fifth** re-implementation lives client-side in +`web/app/dashboard/tasks/[id]/page.tsx:348-444` +(`canonicalFilesystemCapability`, `filesystemPackageCapabilitySummary`, +`unresolvedRequiredFilesystemGrants`), reading yet another requirement field set. + +Concrete divergences observed (all are #172's named failure mode — *preview and +handoff disagree*): + +- **Approval never runs the handoff broker.** `web/app/api/tasks/[id]/approve/route.ts` + gates only on `requiresFilesystemGrantApproval`; it never calls + `evaluateWorkPackageMcpBroker`. A package the broker will block at handoff for a + non-filesystem reason (required GitHub MCP unhealthy, an unsafe GitHub write + capability, an MCP-aware subtask capability not covered by an approved grant) + passes approval and only blocks later at handoff. **This is the root defect.** +- **`filesystem.project.write` has three verdicts.** Planning/broker treat it as a + planning-only *warning* (`isPlanningOnlyFilesystemWrite`, + `mcp-execution-design.ts:318-320`); `canonicalFilesystemProjectCapability` + (`filesystem-grants.ts:38-45`) maps it to `null` so + `summarizeFilesystemCapabilities` silently *drops* it, while + `hasUnsafeFilesystemCapability` flags it *unsafe*. +- **Capability normalization / aliasing is duplicated** with different alias + models. `filesystem-grants.ts` collapses `filesystem.read` ↔ + `filesystem.project.read`; `mcp-execution-design.ts` keeps a one-way widening + (`approvedCoverageCapabilityKeys:551-559`, `filesystemUnqualifiedAlias:561-565`). +- **Requirement field sets differ.** The grant gate reads + `permissions+capabilities+requiredCapabilities+mcpCapabilities` + (`filesystem-grants.ts:77-84`); the broker reads only `capabilities+permissions` + (`capabilityArray:498-516`); the UI reads + `capabilities+permissions+mcpCapabilities`. A requirement expressed only via + `requiredCapabilities` is blocked by the server but shown as "no grant needed". +- **The safe allow-list is encoded three times** and never sources the catalog: + `SAFE_BETA_CAPABILITY_PATTERNS` (`mcp-execution-design.ts:7-18`), the regex in + `canonicalFilesystemProjectCapability` (`filesystem-grants.ts:41`), and + `MCP_CATALOG[id].runtime.capabilities` (`catalog.ts:18,37`, the ostensible source + of truth, never read by admission). +- **Denying a required filesystem grant burns an execution attempt.** + `requiresFilesystemGrantApproval` returns `{blocked:false}` for any `denied` + effective phase regardless of blocking capabilities + (`filesystem-grants.ts:315-322`), so the package is claimed, then the executor + throws `Filesystem MCP context blocked` (`work-package-executor.ts:~1515`), + consuming an attempt for a guaranteed failure. +- **Recovery is not deterministic.** The per-task `always_allow` route only + reconciles siblings in `pending/ready/blocked/needs_rework` + (`tasks/[id]/filesystem-grants/route.ts:418-421`); the project-level route also + reconciles `failed` siblings (`projects/[id]/filesystem-grant/route.ts:166-243`). + The same grant state yields different outcomes depending on which endpoint issued it. +- **UI conflates a product boundary with a broken install.** Grant-preview copy is + a 3-way ternary on `decision.status` only + (`tasks/[id]/page.tsx:3171-3175`); a deferred GitHub-write capability renders the + identical destructive "resolve this MCP issue" copy as a genuinely unhealthy MCP. + There is no neutral badge bucket for deferred/planning + (`statusBadgeClass:1203-1245`), and `RetryHandoffControls` offers "Re-run stalled + handoff" even for non-retryable blocks. + +## Decision + +Introduce **one normalized admission decision** and route every surface through it. +The exact same decision object drives preview badges, approval blocking, handoff +blocking, filesystem recovery, and operator copy. Live MCP tool handles remain +deferred; no runtime tool is ever issued to a package run. + +### The contract: `McpAdmissionDecision` + +New module `web/lib/mcps/admission.ts` (types may live here or re-export from +`web/lib/mcps/types.ts`): + +```ts +export type McpCapabilityClass = + | 'planning_only' // filesystem.project.write, prose-only hints + | 'bounded_read_only' // filesystem.project.read|list|search (+ catalog read/list/search) + | 'deferred_live_mcp' // github write/branch/pr/merge/settings/secret, filesystem write/delete/admin, any live tool handle + | 'unknown' // capability names no known MCP + +export type McpAdmissionMode = + | 'planning_only' + | 'bounded_context_required' + | 'bounded_context_approved' + | 'blocked' + | 'deferred_live_mcp' + +export type McpAdmissionStatus = 'allowed' | 'warning' | 'blocked' + +export type McpRecoveryAction = + | 'continue_as_prompt_context' + | 'approve_project_filesystem_context' + | 'install_or_fix_mcp' + | 'revise_plan' + | 'defer_live_mcp_feature' + +export type McpAdmissionDecision = { + schemaVersion: 1 + mcpId: 'filesystem' | 'github' | string + agent: string + requirement: 'required' | 'optional' + requestedCapabilities: string[] + normalizedCapabilities: string[] + mode: McpAdmissionMode + status: McpAdmissionStatus + reason: string + recoveryAction?: McpRecoveryAction + evidenceRefs: string[] +} +``` + +### Shared primitives (single copy each) + +`web/lib/mcps/capability-normalization.ts` (new) is the ONLY home for: + +- `normalizeCapability(cap)` — `trim().toLowerCase().replace(/\s+/g,'_')`. +- `classifyCapability(mcpId, cap): McpCapabilityClass` — reads the + `bounded_read_only` set from `MCP_CATALOG[mcpId].runtime.capabilities` (plus + filesystem project-alias spellings), folds in the `planning_only` set + (`filesystem.project.write`) and the explicit `deferred_live_mcp` set. This makes + the catalog the single declarative allow-list and gives `deferred_live_mcp` an + explicit name instead of the generic "outside the allowed beta scope" string. +- `coverageKeysForGrant(cap)` / `coverageKeysForProhibition(cap)` — one alias model + for `filesystem.read` ↔ `filesystem.project.read` (decide and document the + direction once; see ADR 0008). Replaces `approvedCoverageCapabilityKeys`, + `filesystemProjectAlias`, `filesystemUnqualifiedAlias`, + `prohibitedCoverageCapabilityKeys`. +- `mergeCapabilityFields(entry)` — the one union of requirement fields. Define + `REQUIREMENT_CAPABILITY_FIELDS = ['permissions','capabilities','requiredCapabilities','mcpCapabilities']` + and read exactly those so the broker, grant gate, UI, and executor never diverge. +- `isMcpHealthy(status)` / `mcpHealthReason(mcpId, status)` — one health predicate + and one message source. +- `canProceedWithoutMcp(requirement, fallback)` — `optional` + + `continue_without_mcp`. + +### The core producer + +```ts +export function admitMcpRequirement(input: { + mcpId: string + agent: string + requirement: 'required' | 'optional' + requestedCapabilities: string[] + prohibitedCapabilities: string[] + status: ProjectMcpStatus | null + hasPromptOnlyContext: boolean + fallback: { action: McpFallbackAction } + projectGrantCovers?: boolean // filesystem bounded-context already approved + evidenceRefs?: string[] +}): McpAdmissionDecision +``` + +Decision rules (preserving every current safety behavior): + +- Unknown MCP → `mode:'blocked'`, `recoveryAction:'revise_plan'`, + `class:'unknown'`. +- All requested capabilities are `planning_only` (or none actionable) → + `mode:'planning_only'`, `status:'warning'`, + `recoveryAction:'continue_as_prompt_context'`. +- Any `deferred_live_mcp` capability → `mode:'deferred_live_mcp'`, + `status:'blocked'`, `recoveryAction:'defer_live_mcp_feature'`. +- Bounded read-only + already covered by an approved/project grant → + `mode:'bounded_context_approved'`, `status:'allowed'`. +- Bounded read-only + not yet covered → `mode:'bounded_context_required'`, + `status:'blocked'` (unless `optional`+`continue_without_mcp` → `warning`), + `recoveryAction:'approve_project_filesystem_context'`. +- MCP unhealthy/missing/disabled and required with no prompt-only fallback → + `status:'blocked'`, `recoveryAction:'install_or_fix_mcp'` (retryable); + otherwise `warning`. +- `required` with no capabilities but prompt-only context present → `warning` + (planning-only), not blocked. + +`recoveryAction` replaces the fragile `isRetryableMcpBrokerBlock` string-matching: +retryable ⇔ `recoveryAction === 'install_or_fix_mcp'`. + +### Adapters (shape-preserving, so persisted JSON and readers do not change) + +Co-located in `admission.ts`: + +- `decisionsToValidation(decisions): McpExecutionValidation` +- `decisionsToGrantPreview(decisions): McpGrantDecisions` — maps + `mode`/`status` → `proposed|warning|blocked`, keeps `decisionId`, + `sourceRequirementIndex`, `promptOverlayPresent`, **and adds** `mode`, + `recoveryAction`, `normalizedCapabilities`, `evidenceRefs`. +- `decisionsToBrokerCheck(decisions, label): WorkPackageMcpBrokerCheck` + +`web/lib/mcps/execution-design-metadata.ts:99-232` keeps reading the same +`grantDecisions`/`validation` JSON, extended (non-breaking, back-derive `mode` for +old artifacts). + +## Consolidation map (four paths → one) + +| Current function | Becomes | +|---|---| +| `validateMcpExecutionDesign` (`mcp-execution-design.ts:326-454`) | builds `admitMcpRequirement` per (requirement, agent); returns `decisionsToValidation` | +| `decisionStatus` + `deriveMcpGrantDecisions` (`:753-867`) | `deriveMcpGrantDecisions` calls `admitMcpRequirement`; `decisionStatus` deleted | +| `evaluateWorkPackageMcpBroker` (`:599-739`) + helpers `:488-591` | builds entries via `brokerEntries`, calls `admitMcpRequirement` with `mergeCapabilityFields`; returns `decisionsToBrokerCheck`; local `normalizeCapability/coverageCapabilityKey/unsafeCapability/capabilityArray/healthyStatus/canProceedWithoutMcp/isPlanningOnlyFilesystemWrite/SAFE_BETA_CAPABILITY_PATTERNS` deleted, imported from shared modules | +| `requiresFilesystemGrantApproval` / `summarizeFilesystemCapabilities` (`filesystem-grants.ts`) | filesystem-specific persistence over the shared classifier; imports normalization; keeps `FilesystemProjectCapability` nominal type + `ProjectFilesystemGrant` persistence | +| `mcpCapabilityList` (`work-package-executor.ts:1252-1260`) | imports `mergeCapabilityFields` | +| client helpers (`tasks/[id]/page.tsx:348-444`) | import shared helpers or consume a server-computed grant-state payload | + +## Implementation slices + +### S1 — Contract and terminology (child issue → S1) + +- Create `web/lib/mcps/capability-normalization.ts` and `web/lib/mcps/admission.ts` + with the types, primitives, `admitMcpRequirement`, and the three adapters above. +- Make `classifyCapability` read `MCP_CATALOG[mcpId].runtime.capabilities` and + encode the explicit `deferred_live_mcp` list. Delete `SAFE_BETA_CAPABILITY_PATTERNS`. +- Write this ADR (done) and align roadmap/task-detail copy on the three-way + terminology (planning-only / bounded read-only / deferred live MCP). + +### S2 — Broker consolidation (child issue → S2) + +- Migrate the four functions per the consolidation map. Delete duplicated helpers. +- **Enforce admission at approval.** In `web/app/api/tasks/[id]/approve/route.ts`, + after the existing `requiresFilesystemGrantApproval` check (~`:189-209`), run the + admission decision over every package and refuse approval (mirroring the existing + `missingFilesystemGrant` early return) when any decision is `status:'blocked'`, + returning the normalized `reason` + `recoveryAction`. Approval then enforces + exactly what handoff enforces. +- **Invariant:** for any fixed package, `deriveMcpGrantDecisions` (preview), + `evaluateWorkPackageMcpBroker` (handoff), and `requiresFilesystemGrantApproval` + produce the same `mode`/`status`. New tests + (`web/__tests__/mcp-admission-invariant.test.ts`) assert agreement for: required + no-capability grants, prompt-only context, filesystem read/list/search, + `filesystem.project.write`, an unsafe/deferred GitHub write, an unknown MCP, and a + requirement expressed via each of the four capability fields. + +### S3 — Filesystem grant recovery (child issue → S3) + +- `requiresFilesystemGrantApproval` (`filesystem-grants.ts:315-322`) must NOT return + `{blocked:false}` for a `denied` effective phase when blocking capabilities are + non-empty; return a distinct terminal outcome (e.g. `deniedRequired:true`) so + handoff holds the package pre-claim (zero attempts). Optional/`continue_without_mcp` + denials stay non-blocking. +- `filesystemGrantHandoffBlock` (`work-package-handoff.ts:876-906`): branch on the + denied-required outcome to `failWorkPackageForFilesystemGrant` with a message that + names the operator denial and is recoverable via the existing + `FILESYSTEM_GRANT_BLOCK_METADATA_KEY`. Pass `project?.mcpConfig` into + `requiresFilesystemGrantApproval` in BOTH branches (the default branch at + `:896-899` currently omits it), closing the approval-vs-handoff project-coverage divergence. +- Make recovery deterministic: the per-task `always_allow` route + (`tasks/[id]/filesystem-grants/route.ts:418-421`) must reconcile `failed` + grant-blocked siblings too, or delegate to the shared reconciliation used by + `projects/[id]/filesystem-grant/route.ts:166-243`. Both endpoints must recover the + identical package set for identical grant state. +- Preserve: filesystem blocks are `terminalBlock:true` and never auto-retried + (`blocked-handoff-retry.ts:67-79`). + +### S4 — Prompt/context assembly and bounded-context packet evidence (child issue → S4; builds on #43) + +- Specialist prompts receive `promptOverlay` + `mcpAwareSubtasks` + + `mcpRequirements` as **instructions**, never as tool grants + (`work-package-executor.ts` prompt assembly). No live MCP handle is ever issued. +- The bounded read-only context packet (root path, selected file names/excerpts, + included/omitted counts, redaction summary — per ADR 0008) is attached to the run + as an **inspectable artifact**, referenced from `McpAdmissionDecision.evidenceRefs`. +- Sandbox-generated files (`.forge/task-runs/...`) stay clearly separated from + host-repository writes in artifacts. +- `mcpCapabilityList` imports `mergeCapabilityFields`; executor filesystem gating uses + shared `coverageKeysForGrant`/`classifyCapability` rather than re-deriving + `filesystem.project.read` membership. + +### S5 — UI and copy hardening (child issue → S5) + +- New `web/lib/mcps/admission-copy.ts`: pure map from `mode`+`recoveryAction`+`status` + → `{ statusKey, badgeText, headline, body, cta? }`. Every surface reads it, so copy + cannot drift. Mapping: + - `planning_only` → neutral "Planning context", body "Recorded as run-scoped prompt + instructions; writes go through the Forge sandbox/host-apply path, no live MCP + tools attached", no CTA. + - `bounded_context_required` → amber "Needs project context", CTA scroll to the + package's filesystem grant control. + - `bounded_context_approved` → green "Context approved". + - `blocked` + `install_or_fix_mcp` → red, CTA deep-link + `/dashboard/projects/{projectId}#project-mcps-heading`. + - `blocked` + `revise_plan` → red, CTA open Request-changes flow. + - `deferred_live_mcp` → **neutral slate** "Deferred — beta boundary", body "Live MCP + tool handles are not part of this beta. This is a product boundary, not a broken + install", no CTA. +- Add `deferred`/`planning` neutral buckets to `statusBadgeClass` + (`tasks/[id]/page.tsx:1203-1245`). +- Extend `execution-design-metadata.ts` decision type/normalizer to carry `mode`, + `recoveryAction`, `normalizedCapabilities`, `evidenceRefs` (back-derive `mode` for + old artifacts). +- Replace the status-only ternary (`:3171-3175`), split planning-only warnings from + degradation warnings (`:3146-3155`), stop rendering deferred capabilities as + destructive alerts (`:3135-3144`), gate `RetryHandoffControls` on retryability, + surface the bounded-context packet inline, and add remediation CTAs + + bounded-context note on the projects page (`projects/[id]/page.tsx:1035-1052`) and + the MCPs catalog page (`mcps/page.tsx`). +- The worker must persist `mode`+`recoveryAction` on BOTH the `grantDecisions` + preview decisions AND the per-package block metadata (`blockedReason`) so + `RetryHandoffControls` routes correctly. + +### S6 — End-to-end regression (child issue → S6) + +- `web/__tests__` (or `web/e2e`) regression for a local-only tiny task-tracker + project: Architect creates frontend/QA/docs/review packages; the MCP plan includes + prompt-only context and no live tool handles; approval succeeds; handoff advances + ready packages; a required `filesystem.project.read|search` grant still blocks until + approved or denied through the intended path; a deferred GitHub-write capability is + reported as `deferred_live_mcp`, not an install error. +- Plus the S2 preview==handoff invariant suite. + +## Consequences + +- Preview, approval, handoff, filesystem recovery, and UI copy are structurally + incapable of disagreeing because they consume one decision object built by one + producer over one set of primitives sourced from one catalog. +- `deferred_live_mcp` is a first-class, named mode, so blocks say "deferred live MCP + feature" instead of a generic beta-scope error, and the UI shows a product + boundary instead of a broken install. +- Denying a required filesystem grant no longer burns an execution attempt; recovery + is deterministic across both endpoints. +- Live MCP runtime handles remain out of scope. Issuing them is a later, + security-reviewed epic that depends on #40 (adversarial mode) and #60 (security + review workforce). The eventual DB-backed MCP capability catalogue belongs to #121; + this ADR keeps the allow-list catalog-sourced in code as the precursor. + +## Out of scope + +Live MCP tool handles for package runs; arbitrary filesystem write grants through +MCP; GitHub write/merge automation (#69); user-edited arbitrary grant scopes beyond +the bounded flows here; parallel specialist execution; replacing the sandbox +execution JSON path. diff --git a/docs/developer-guide.md b/docs/developer-guide.md index 8e85f86..d1f885f 100644 --- a/docs/developer-guide.md +++ b/docs/developer-guide.md @@ -29,6 +29,23 @@ is `not_implemented`) -- specialists run sandboxed with no real MCP tools -- so Use precise grant terms: Architect-proposed grants, Forge broker decisions, operator-approved grant snapshots, and effective run-scoped instructions. +Admission is being consolidated onto **one contract** (EPIC #172, ADR +[0009](adr/0009-mcp-admission-contract.md)). The same normalized +`McpAdmissionDecision` object -- with a capability `mode` of `planning_only`, +`bounded_context_required`, `bounded_context_approved`, `blocked`, or +`deferred_live_mcp`, plus a `recoveryAction` -- drives grant preview, plan +approval, handoff blocking, and operator recovery copy, so those surfaces cannot +disagree. Capabilities fall into three classes: **planning-only** (prompt +overlays, MCP-aware subtasks, `filesystem.project.write`; warn, never block), +**bounded read-only** (`filesystem.project.read|list|search`; may need an +explicit operator grant), and **deferred live MCP** (live tool handles, GitHub +write/branch/PR/merge/settings/secret, filesystem write/delete/admin; a product +boundary, not a broken install). The classifier is sourced from +`MCP_CATALOG.runtime.capabilities`. Historically the same policy was re-derived +by four divergent paths (`validateMcpExecutionDesign`, `deriveMcpGrantDecisions`, +`evaluateWorkPackageMcpBroker`, and `requiresFilesystemGrantApproval`); they are +becoming thin adapters over the shared core in `web/lib/mcps/admission.ts`. + ACP providers are local command-line-agent providers. Forge starts the configured ACP adapter on demand, speaks JSON-RPC over stdio, and receives text back through the same provider interface used by the worker. The currently wired diff --git a/docs/operator-guide.md b/docs/operator-guide.md index 4f2426b..3797198 100644 --- a/docs/operator-guide.md +++ b/docs/operator-guide.md @@ -225,7 +225,10 @@ FORGE_HOST_REPOSITORY_WRITES=0 With the default execution path: -1. The operator approves the Architect plan. +1. The operator approves the Architect plan. Approval runs the same MCP admission + check as handoff, so a plan that approves will not silently stall later on MCP + grant semantics (EPIC #172, ADR + [0009](adr/0009-mcp-admission-contract.md)). 2. Forge releases ready work packages and runs the MCP/capability broker. 3. Required blocked MCP/tool grants stop the package before execution. Optional grants can continue only when the approved fallback is non-blocking. @@ -239,6 +242,21 @@ With the default execution path: 8. QA, Reviewer, and Security gates appear when required. In this beta, those are manual operator decisions, not proof that separate reviewer agents ran. +On the task detail page, each MCP request resolves to one of four states so you +can act without reading logs (EPIC #172): + +- **Planning context** -- the Architect only suggested an MCP; it is recorded as + prompt instructions and never blocks handoff. Generated file writes go through + the Forge sandbox/host-apply path, not a live MCP write tool. +- **Needs project context** -- the package needs bounded read-only filesystem + context (`filesystem.project.read|list|search`). Approve or deny the exact + grant shown; denial is recorded and the package is held, not crashed. +- **MCP needs setup** -- an MCP is missing/unhealthy/unauthenticated. Use the + linked setup/retry action on the project MCP panel. +- **Deferred -- beta boundary** -- the request needs a live MCP tool handle (or a + write/merge/admin capability). This is a product boundary, not a broken + install; it is deferred to a later security-reviewed slice. + Operators can review: - package status, assigned role, dependencies, acceptance criteria, and blocked diff --git a/docs/roadmap.md b/docs/roadmap.md index e80f430..0b1bab7 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -1,6 +1,6 @@ # Forge Roadmap -Last updated: 2026-07-01 +Last updated: 2026-07-10 ## Plain-English Summary @@ -156,6 +156,32 @@ Still out of scope: - Default-on package execution before the release gate, documentation, QA, Reviewer, Adversarial review, and manual/operator smoke path all pass. +### MCP Execution Readiness and Bounded Context Grants (#172) + +The next architecture epic after the #119 beta. MCP ambiguity is now the main +reason an approved task can still stall at handoff, because the same beta +capability policy is re-derived by four divergent code paths (planning +validation, grant preview, handoff broker, and the filesystem grant gate) that do +not agree -- most importantly, plan approval never runs the handoff broker, so a +package can pass approval and then block at handoff. + +The epic makes planning, approval, UI state, filesystem grants, and handoff follow +**one admission contract**: a normalized `McpAdmissionDecision` (ADR +[0009](adr/0009-mcp-admission-contract.md)) with a capability `mode` +(`planning_only`, `bounded_context_required`, `bounded_context_approved`, +`blocked`, `deferred_live_mcp`) and a `recoveryAction`, sourced from +`MCP_CATALOG.runtime.capabilities`. Capabilities resolve to three classes: +planning-only (warn, never block, including `filesystem.project.write`), bounded +read-only (`filesystem.project.read|list|search`, may need an operator grant), and +deferred live MCP (live tool handles and write/merge/admin -- a product boundary, +not a broken install). Live MCP tool handles remain out of scope. + +Delivered as six sweeping child slices plus #43: #176 (S1 contract/taxonomy), +#177 (S2 consolidation + approval enforcement), #178 (S3 deterministic filesystem +grant/denial recovery), #179 (S4 prompt/context packet evidence), #180 (S5 unified +operator UI copy/recovery-action contract), #181 (S6 end-to-end regression + +preview==handoff invariant), and #43 (Architect-driven MCP assignment/overlays). + ## Technical Details ### P1 Product Hardening diff --git a/docs/wiki.md b/docs/wiki.md index 8b0b618..95d11cc 100644 --- a/docs/wiki.md +++ b/docs/wiki.md @@ -52,6 +52,13 @@ Architect plan saved -> manual QA/Reviewer/Security gates where required ``` +The MCP admission check is one shared contract across grant preview, plan +approval, and handoff (EPIC #172, ADR +[0009](adr/0009-mcp-admission-contract.md)). Each MCP request resolves to a +single mode -- planning-only context, bounded read-only context (approve/deny), +an MCP that needs setup, or a deferred live-MCP feature -- so a plan that approves +will not silently stall at handoff. + Task detail controls now cover the common operator interventions: - Stop cancels a non-terminal task and any active package/run state. From 7a8ddca8d34c73ccc7b4bfcb0948c9aab0af20d8 Mon Sep 17 00:00:00 2001 From: Joncallim <64296013+Joncallim@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:02:55 +0800 Subject: [PATCH 2/3] =?UTF-8?q?docs:=20address=20PR=20#182=20review=20?= =?UTF-8?q?=E2=80=94=20harden=20MCP=20admission=20ADR=20and=20release-trut?= =?UTF-8?q?h?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/adr/0009-mcp-admission-contract.md | 705 ++++++++++++++++-------- docs/developer-guide.md | 8 +- docs/operator-guide.md | 24 +- docs/roadmap.md | 14 +- docs/wiki.md | 16 +- 5 files changed, 501 insertions(+), 266 deletions(-) diff --git a/docs/adr/0009-mcp-admission-contract.md b/docs/adr/0009-mcp-admission-contract.md index a046a4a..671e5f9 100644 --- a/docs/adr/0009-mcp-admission-contract.md +++ b/docs/adr/0009-mcp-admission-contract.md @@ -5,10 +5,16 @@ Accepted for EPIC #172 (MCP Execution Readiness and Bounded Context Grants). Establishes the single admission contract; the implementation slices (#172 → S1–S6 child issues) build on it. Supersedes the code-level policy scattered across the -four admission paths described below. Related: ADR +admission paths described below. Related: ADR [0006](0006-executable-workforce-beta-boundary.md) (beta boundary), [0008](0008-filesystem-mcp-bounded-context-grants.md) (filesystem bounded context -grants), and #43 (Architect-driven MCP assignment and prompt overlays). +grants), and #43 (Architect-driven MCP assignment and prompt overlays — see +[#43 re-scope](#43-re-scope) below). + +> **Anchor freshness.** Line anchors in this ADR were re-derived against `main` +> after #175 ("Harden work-package execution repair") landed. Before editing, +> re-`grep` the named symbol — anchors drift as `main` moves. Every anchor cites a +> *symbol name* first and a line second; trust the symbol. ## Context @@ -19,111 +25,192 @@ because the code does not distinguish them in one place: records it as run-scoped prompt instructions (`promptOverlay`, `mcpAwareSubtasks`). It grants nothing. 2. **Bounded read-only context grants** — Forge assembles a project-scoped, - inspectable read-only context packet (file names, selected excerpts) after an - explicit operator grant. Capabilities: `filesystem.project.read`, - `filesystem.project.list`, `filesystem.project.search`. + inspectable read-only context packet after an explicit operator grant. + Capabilities: `filesystem.project.read`, `filesystem.project.list`, + `filesystem.project.search`. **A bounded grant is only "deliverable" when a + context producer exists for that MCP.** Today that producer exists for + `filesystem` only (`MCP_CATALOG.filesystem.runtime.mode === + 'bounded_context_packet'`). `github` reads are safe but have no producer + (`runtime.mode === 'external_service'`, `liveTools:false`); they are delivered + as planning/prompt context, not as an approvable bounded packet. 3. **Live MCP tool handles** — a worker calling an MCP directly at runtime. This is **deferred**. It is not implemented in this beta and must read as a product boundary, not a broken install. -Today the same beta capability policy is re-derived by **four overlapping code -paths** that do not agree: +### The multiple-path problem -| # | Path | Location | Consumes | Purpose | -|---|------|----------|----------|---------| -| 1 | `validateMcpExecutionDesign` | `web/worker/mcp-execution-design.ts:326-454` | parsed `McpExecutionDesign` fence | approval-time validation | -| 2 | `deriveMcpGrantDecisions` / `decisionStatus` | `web/worker/mcp-execution-design.ts:753-867` | same design | grant-decision preview | -| 3 | `evaluateWorkPackageMcpBroker` | `web/worker/mcp-execution-design.ts:599-739` | persisted `workPackages.mcpRequirements` + `metadata.mcpGrants` | handoff-time broker | -| 4 | `requiresFilesystemGrantApproval` / `summarizeFilesystemCapabilities` | `web/lib/mcps/filesystem-grants.ts:90-339` | `mcpRequirements` + `metadata` | filesystem grant gate | +The same beta capability policy is re-derived by overlapping code paths that do +not share one producer: -A **fifth** re-implementation lives client-side in -`web/app/dashboard/tasks/[id]/page.tsx:348-444` -(`canonicalFilesystemCapability`, `filesystemPackageCapabilitySummary`, -`unresolvedRequiredFilesystemGrants`), reading yet another requirement field set. +| # | Path | Location (symbol · line on `main`) | Consumes | Purpose | +|---|------|-----------------------------------|----------|---------| +| 1 | `validateMcpExecutionDesign` | `web/worker/mcp-execution-design.ts` · 326 | parsed `McpExecutionDesign` fence | approval-time validation of the Architect design | +| 2 | `deriveMcpGrantDecisions` / `decisionStatus` | `mcp-execution-design.ts` · 803 / 753 | same design | grant-decision preview | +| 3 | `evaluateWorkPackageMcpBroker` | `mcp-execution-design.ts` · 599 | persisted `workPackages.mcpRequirements` + `metadata.mcpGrants` + `mcpAwareSubtasks` | handoff-time broker | +| 4 | `requiresFilesystemGrantApproval` / `summarizeFilesystemCapabilities` | `web/lib/mcps/filesystem-grants.ts` · 303 / 90 | `mcpRequirements` + `metadata` | filesystem grant gate (a *filesystem-only projection*) | +| 5 | client re-implementation | `web/app/dashboard/tasks/[id]/page.tsx` · `filesystemPackageCapabilitySummary` 369, `canonicalFilesystemCapability` 354, `unresolvedRequiredFilesystemGrants` 420 | yet another requirement field set | UI badges/blocking | -Concrete divergences observed (all are #172's named failure mode — *preview and -handoff disagree*): +Concrete, currently-true divergences (all are #172's named failure mode — +*preview and handoff disagree*): - **Approval never runs the handoff broker.** `web/app/api/tasks/[id]/approve/route.ts` - gates only on `requiresFilesystemGrantApproval`; it never calls - `evaluateWorkPackageMcpBroker`. A package the broker will block at handoff for a - non-filesystem reason (required GitHub MCP unhealthy, an unsafe GitHub write - capability, an MCP-aware subtask capability not covered by an approved grant) - passes approval and only blocks later at handoff. **This is the root defect.** + gates only on `requiresFilesystemGrantApproval` (inside the status-flip + `db.transaction`, `:192`); it never calls `evaluateWorkPackageMcpBroker`. A + package the broker will block at handoff for a non-filesystem reason (required + GitHub MCP unhealthy, an unsafe GitHub write capability, an MCP-aware subtask + capability not covered by an approved grant) passes approval and only blocks + later at handoff. **This is the root defect.** The approve route loads + `projects.mcpConfig` (`:161-165`), *not* the `ProjectMcpStatus` health snapshot + the broker needs, so fixing this is not a one-line call move (see S2). - **`filesystem.project.write` has three verdicts.** Planning/broker treat it as a planning-only *warning* (`isPlanningOnlyFilesystemWrite`, - `mcp-execution-design.ts:318-320`); `canonicalFilesystemProjectCapability` - (`filesystem-grants.ts:38-45`) maps it to `null` so - `summarizeFilesystemCapabilities` silently *drops* it, while - `hasUnsafeFilesystemCapability` flags it *unsafe*. + `mcp-execution-design.ts:318`); `canonicalFilesystemProjectCapability` + (`filesystem-grants.ts:38`) maps it to `null` so `summarizeFilesystemCapabilities` + silently *drops* it, while `hasUnsafeFilesystemCapability` flags it *unsafe*. - **Capability normalization / aliasing is duplicated** with different alias models. `filesystem-grants.ts` collapses `filesystem.read` ↔ `filesystem.project.read`; `mcp-execution-design.ts` keeps a one-way widening - (`approvedCoverageCapabilityKeys:551-559`, `filesystemUnqualifiedAlias:561-565`). + (`approvedCoverageCapabilityKeys:551`). - **Requirement field sets differ.** The grant gate reads `permissions+capabilities+requiredCapabilities+mcpCapabilities` (`filesystem-grants.ts:77-84`); the broker reads only `capabilities+permissions` - (`capabilityArray:498-516`); the UI reads + (`capabilityArray:498`); the executor's `mcpCapabilityList` + (`work-package-executor.ts:1527`) reads `capabilities+permissions`; the UI reads `capabilities+permissions+mcpCapabilities`. A requirement expressed only via - `requiredCapabilities` is blocked by the server but shown as "no grant needed". + `requiredCapabilities` is blocked by the filesystem gate but shown by the broker + as "no grant needed". - **The safe allow-list is encoded three times** and never sources the catalog: - `SAFE_BETA_CAPABILITY_PATTERNS` (`mcp-execution-design.ts:7-18`), the regex in - `canonicalFilesystemProjectCapability` (`filesystem-grants.ts:41`), and - `MCP_CATALOG[id].runtime.capabilities` (`catalog.ts:18,37`, the ostensible source - of truth, never read by admission). -- **Denying a required filesystem grant burns an execution attempt.** - `requiresFilesystemGrantApproval` returns `{blocked:false}` for any `denied` - effective phase regardless of blocking capabilities - (`filesystem-grants.ts:315-322`), so the package is claimed, then the executor - throws `Filesystem MCP context blocked` (`work-package-executor.ts:~1515`), - consuming an attempt for a guaranteed failure. -- **Recovery is not deterministic.** The per-task `always_allow` route only - reconciles siblings in `pending/ready/blocked/needs_rework` - (`tasks/[id]/filesystem-grants/route.ts:418-421`); the project-level route also - reconciles `failed` siblings (`projects/[id]/filesystem-grant/route.ts:166-243`). - The same grant state yields different outcomes depending on which endpoint issued it. + `SAFE_BETA_CAPABILITY_PATTERNS` (`mcp-execution-design.ts:7-18`, broader than the + catalog — it also allows `github.actions.read`, `github.repository.list|search`, + `github.contents.list|search`), the regex in `canonicalFilesystemProjectCapability` + (`filesystem-grants.ts:41`), and `MCP_CATALOG[id].runtime.capabilities` + (`catalog.ts:18,37`, the ostensible source of truth, never read by admission). +- **Denying a *required* filesystem grant burns an execution attempt AND dead-ends.** + `requiresFilesystemGrantApproval` returns `{blocked:false}` for a `denied` + effective phase regardless of blocking capabilities (`filesystem-grants.ts:315-322`). + So handoff does *not* hold the package; it is claimed and the executor throws + `Filesystem MCP context blocked` (`work-package-executor.ts:1790`), consuming an + attempt. Worse, that executor failure never writes the `mcpGrantBlock` marker + (only the handoff gate `failWorkPackageForFilesystemGrant` does, + `work-package-handoff.ts:807`), so `canEditPackageGrant` + (`tasks/[id]/filesystem-grants/route.ts:123-143`) refuses grant edits on the + resulting `failed` package — the operator cannot recover by approving the grant. +- **Recovery scope differs between the two grant endpoints.** The per-task route + recovers `failed`/`blocked` grant-blocked packages + (`FAILED_GRANT_RECOVERY_PACKAGE_STATUSES`, `tasks/[id]/filesystem-grants/route.ts:121`) + but its `always_allow` sibling propagation only touches + `STANDARD_EDITABLE_PACKAGE_STATUSES` (no `failed`) and only *within the current + task* (`:411-446`). The project route reconciles across *every task in the + project* including `failed` (`RECONCILABLE_PACKAGE_STATUSES`, + `projects/[id]/filesystem-grant/route.ts:166-244`). The same `always_allow` grant + recovers a different package set depending on which endpoint issued it. - **UI conflates a product boundary with a broken install.** Grant-preview copy is - a 3-way ternary on `decision.status` only - (`tasks/[id]/page.tsx:3171-3175`); a deferred GitHub-write capability renders the - identical destructive "resolve this MCP issue" copy as a genuinely unhealthy MCP. - There is no neutral badge bucket for deferred/planning - (`statusBadgeClass:1203-1245`), and `RetryHandoffControls` offers "Re-run stalled - handoff" even for non-retryable blocks. + a ternary on `decision.status` only; a deferred GitHub-write capability renders + the same destructive "resolve this MCP issue" copy as a genuinely unhealthy MCP. + `statusBadgeClass` (`tasks/[id]/page.tsx:1203`) has no neutral bucket for + deferred/planning, and `RetryHandoffControls` offers "Re-run stalled handoff" + even for non-retryable blocks. ## Decision -Introduce **one normalized admission decision** and route every surface through it. -The exact same decision object drives preview badges, approval blocking, handoff -blocking, filesystem recovery, and operator copy. Live MCP tool handles remain -deferred; no runtime tool is ever issued to a package run. +Introduce **one normalized, package-level admission evaluation** and route every +surface through it. Preview, approval, and handoff all consume the output of a +single producer; the filesystem grant gate becomes a *projection* of that same +evaluation. Live MCP tool handles remain deferred; no runtime tool is ever issued +to a package run. -### The contract: `McpAdmissionDecision` +### Layer 0 — capability classification and delivery kind -New module `web/lib/mcps/admission.ts` (types may live here or re-export from -`web/lib/mcps/types.ts`): +`web/lib/mcps/capability-normalization.ts` (new) is the ONLY home for +normalization and classification: ```ts export type McpCapabilityClass = - | 'planning_only' // filesystem.project.write, prose-only hints - | 'bounded_read_only' // filesystem.project.read|list|search (+ catalog read/list/search) + | 'planning_only' // filesystem.project.write, prose-only hints, and safe reads with no producer + | 'bounded_read_only' // safe reads (catalog + supplement) — deliverability depends on deliveryKind | 'deferred_live_mcp' // github write/branch/pr/merge/settings/secret, filesystem write/delete/admin, any live tool handle - | 'unknown' // capability names no known MCP + | 'unknown' // capability names no known MCP, or a known MCP + unrecognized capability -export type McpAdmissionMode = - | 'planning_only' - | 'bounded_context_required' - | 'bounded_context_approved' - | 'blocked' - | 'deferred_live_mcp' +export type McpDeliveryKind = + | 'bounded_context_packet' // Forge assembles a read-only packet (filesystem) + | 'planning_context_only' // safe reads with no producer yet (github); passed as prompt context, health-gated -export type McpAdmissionStatus = 'allowed' | 'warning' | 'blocked' +// deliveryKind is sourced from the catalog, not re-declared: +// MCP_CATALOG[id].runtime.mode === 'bounded_context_packet' -> 'bounded_context_packet' +// otherwise ('external_service') -> 'planning_context_only' +export function mcpDeliveryKind(mcpId: string): McpDeliveryKind +export function normalizeCapability(cap: string): string // trim().toLowerCase().replace(/\s+/g,'_') +export function classifyCapability(mcpId: string, cap: string): McpCapabilityClass +``` + +`classifyCapability`'s safe-read set is `MCP_CATALOG[mcpId].runtime.capabilities` +**∪ `SAFE_READ_SUPPLEMENT`**, where `SAFE_READ_SUPPLEMENT` is migrated *verbatim* +from `SAFE_BETA_CAPABILITY_PATTERNS` so no capability that is allowed today +becomes newly `unknown` (specifically `github.actions.read`, +`github.repository.list|search`, `github.contents.list|search`). Deleting +`SAFE_BETA_CAPABILITY_PATTERNS` therefore moves the exact same patterns into a +single documented data set — **not a behavior change**. `filesystem.project.write` +classifies as `planning_only`. Anything not in the safe-read set and not +planning-only classifies as `deferred_live_mcp` (known MCP) or `unknown`. + +**Risk ≠ delivery.** A `bounded_read_only` github read is *safe* but not +*deliverable as a bounded packet* — `mcpDeliveryKind('github') === +'planning_context_only'`, so it is admitted as planning/prompt context (recovery +`continue_as_prompt_context`) and stays health-gated exactly as today; it is never +offered `approve_project_filesystem_context` and never marked +`bounded_context_approved`. Only `filesystem` bounded reads flow through the +approve/deny bounded-context path. + +Shared primitives (single copy each, all in `capability-normalization.ts`): +`coverageKeysForGrant(cap)` / `coverageKeysForProhibition(cap)` (one documented +alias direction for `filesystem.read` ↔ `filesystem.project.read`, per ADR 0008; +replaces `approvedCoverageCapabilityKeys`, `filesystemProjectAlias`, +`filesystemUnqualifiedAlias`, `prohibitedCoverageCapabilityKeys`); +`mergeCapabilityFields(entry)` reading exactly +`REQUIREMENT_CAPABILITY_FIELDS = ['permissions','capabilities','requiredCapabilities','mcpCapabilities']`; +`isMcpHealthy(status)` / `mcpHealthReason(mcpId,status)`; +`canProceedWithoutMcp(requirement,fallback)` = `optional` + `continue_without_mcp` +(note: `ask_user` is **blocking**, not a continue fallback). + +### Layer 1 — the effective-grant state (no coverage boolean) + +A single normalized input models the full filesystem grant lifecycle, so denial +and recovery live in the shared decision instead of a second filesystem-only +policy: + +```ts +export type EffectiveGrantState = { + phase: 'none' | 'proposed' | 'approved' | 'denied' | 'not_issued' + source: 'none' | 'package-local' | 'project-level' + status: 'not_issued' | 'approved' | 'denied' + grantMode?: 'allow_once' | 'always_allow' + consumed?: boolean // allow_once already issued -> treat as none for a retry + coveredCapabilities: string[] // canonical filesystem.project.* + grantApprovalId?: string +} +export function readEffectiveGrantState(pkg: { metadata: unknown }, project: { mcpConfig: unknown }): EffectiveGrantState +``` + +`readEffectiveGrantState` distinguishes never-approved (`phase:'none'|'proposed'`), +explicitly denied (`status:'denied'`), consumed allow-once (`consumed:true`), +revoked project grant (was `project-level` but coverage no longer holds → +collapses to `none`), insufficient coverage (`coveredCapabilities` lacks a +required capability), package-local approval (`source:'package-local'`), and +project-level approval (`source:'project-level'`). It reads the same package +`metadata.mcpGrantPhases.effective` and `project.mcpConfig.grants.filesystem` the +current routes write. + +### Layer 2 — the per-requirement producer + +```ts +export type McpAdmissionMode = + | 'planning_only' | 'bounded_context_required' | 'bounded_context_approved' + | 'blocked' | 'deferred_live_mcp' | 'unknown_legacy' +export type McpAdmissionStatus = 'allowed' | 'warning' | 'blocked' export type McpRecoveryAction = - | 'continue_as_prompt_context' - | 'approve_project_filesystem_context' - | 'install_or_fix_mcp' - | 'revise_plan' - | 'defer_live_mcp_feature' + | 'continue_as_prompt_context' | 'approve_project_filesystem_context' + | 'install_or_fix_mcp' | 'revise_plan' | 'defer_live_mcp_feature' export type McpAdmissionDecision = { schemaVersion: 1 @@ -132,223 +219,351 @@ export type McpAdmissionDecision = { requirement: 'required' | 'optional' requestedCapabilities: string[] normalizedCapabilities: string[] + capabilityClasses: Array<{ capability: string; class: McpCapabilityClass; deliveryKind: McpDeliveryKind | null }> mode: McpAdmissionMode status: McpAdmissionStatus reason: string recoveryAction?: McpRecoveryAction - evidenceRefs: string[] + evidenceRefs: string[] // PLANNED scope only pre-run (root + capability set); run evidence added later, see S4 } -``` -### Shared primitives (single copy each) - -`web/lib/mcps/capability-normalization.ts` (new) is the ONLY home for: - -- `normalizeCapability(cap)` — `trim().toLowerCase().replace(/\s+/g,'_')`. -- `classifyCapability(mcpId, cap): McpCapabilityClass` — reads the - `bounded_read_only` set from `MCP_CATALOG[mcpId].runtime.capabilities` (plus - filesystem project-alias spellings), folds in the `planning_only` set - (`filesystem.project.write`) and the explicit `deferred_live_mcp` set. This makes - the catalog the single declarative allow-list and gives `deferred_live_mcp` an - explicit name instead of the generic "outside the allowed beta scope" string. -- `coverageKeysForGrant(cap)` / `coverageKeysForProhibition(cap)` — one alias model - for `filesystem.read` ↔ `filesystem.project.read` (decide and document the - direction once; see ADR 0008). Replaces `approvedCoverageCapabilityKeys`, - `filesystemProjectAlias`, `filesystemUnqualifiedAlias`, - `prohibitedCoverageCapabilityKeys`. -- `mergeCapabilityFields(entry)` — the one union of requirement fields. Define - `REQUIREMENT_CAPABILITY_FIELDS = ['permissions','capabilities','requiredCapabilities','mcpCapabilities']` - and read exactly those so the broker, grant gate, UI, and executor never diverge. -- `isMcpHealthy(status)` / `mcpHealthReason(mcpId, status)` — one health predicate - and one message source. -- `canProceedWithoutMcp(requirement, fallback)` — `optional` + - `continue_without_mcp`. - -### The core producer - -```ts export function admitMcpRequirement(input: { mcpId: string agent: string requirement: 'required' | 'optional' requestedCapabilities: string[] - prohibitedCapabilities: string[] + packageProhibitedKeys: ReadonlySet // deny-wins set unioned across the whole package (Layer 3) status: ProjectMcpStatus | null hasPromptOnlyContext: boolean + effectiveGrant: EffectiveGrantState fallback: { action: McpFallbackAction } - projectGrantCovers?: boolean // filesystem bounded-context already approved evidenceRefs?: string[] }): McpAdmissionDecision ``` -Decision rules (preserving every current safety behavior): - -- Unknown MCP → `mode:'blocked'`, `recoveryAction:'revise_plan'`, - `class:'unknown'`. -- All requested capabilities are `planning_only` (or none actionable) → - `mode:'planning_only'`, `status:'warning'`, - `recoveryAction:'continue_as_prompt_context'`. -- Any `deferred_live_mcp` capability → `mode:'deferred_live_mcp'`, - `status:'blocked'`, `recoveryAction:'defer_live_mcp_feature'`. -- Bounded read-only + already covered by an approved/project grant → - `mode:'bounded_context_approved'`, `status:'allowed'`. -- Bounded read-only + not yet covered → `mode:'bounded_context_required'`, - `status:'blocked'` (unless `optional`+`continue_without_mcp` → `warning`), - `recoveryAction:'approve_project_filesystem_context'`. -- MCP unhealthy/missing/disabled and required with no prompt-only fallback → - `status:'blocked'`, `recoveryAction:'install_or_fix_mcp'` (retryable); - otherwise `warning`. -- `required` with no capabilities but prompt-only context present → `warning` - (planning-only), not blocked. +`capabilityClasses` records the per-capability result so mixed requirements do not +lose information and a `class:'unknown'` capability is representable. The aggregate +`mode`/`status` is chosen by a **total, precedence-ordered decision table** (first +match wins; every branch is defined): + +1. **Unknown MCP** → `mode:'blocked'`, `status:'blocked'`, `recoveryAction:'revise_plan'`. +2. **Any capability class `unknown`** (known MCP, unrecognized/typo capability) → + `mode:'blocked'`, `status:'blocked'`, `recoveryAction:'revise_plan'`. +3. **Any capability prohibited package-wide** (`normalizeCapability(cap)` ∈ + `packageProhibitedKeys`) → treated as `deferred_live_mcp`; go to (4). +4. **Any capability class `deferred_live_mcp`** → `mode:'deferred_live_mcp'`. If + `requirement==='required'` → `status:'blocked'`, `recoveryAction:'revise_plan'`. + If `optional` → `status:'warning'`, `recoveryAction:'defer_live_mcp_feature'` + (approvable, non-blocking). "Product boundary, not broken install" copy applies. +5. **Any capability class `bounded_read_only` with `deliveryKind === + 'bounded_context_packet'`** (filesystem): + - covered by `effectiveGrant` (`status:'approved'` & not `consumed` & covers the + required capabilities) → `mode:'bounded_context_approved'`, `status:'allowed'`. + - `effectiveGrant.status === 'denied'` & required → `mode:'bounded_context_required'`, + `status:'blocked'`, `recoveryAction:'approve_project_filesystem_context'` + (**deniedRequired** — the handoff gate must HOLD, see S3). + - otherwise not covered → `mode:'bounded_context_required'`; `status:'blocked'` + unless `optional`+`continue_without_mcp` (`status:'warning'`); + `recoveryAction:'approve_project_filesystem_context'`. +6. **Any capability class `bounded_read_only` with `deliveryKind === + 'planning_context_only'`** (github reads): delivered as planning context. + Health overlay (7) may block; otherwise `mode:'planning_only'`, + `status:'allowed'`, `recoveryAction:'continue_as_prompt_context'`. +7. **All capabilities `planning_only`, OR zero actionable capabilities:** + - `required` with zero capabilities and **no** prompt-only context → + `mode:'blocked'`, `status:'blocked'`, `recoveryAction:'revise_plan'` + (the plan under-specified a required MCP). + - otherwise → `mode:'planning_only'`, `status:'warning'`, + `recoveryAction:'continue_as_prompt_context'`. +8. **Health overlay** (applied to the mode chosen above when it is `allowed`): + if `!isMcpHealthy(status)` and `required` and not `canProceedWithoutMcp` → + `status:'blocked'`, `recoveryAction:'install_or_fix_mcp'` (retryable). If + `optional`+`continue_without_mcp` → `status:'warning'`. `recoveryAction` replaces the fragile `isRetryableMcpBrokerBlock` string-matching: -retryable ⇔ `recoveryAction === 'install_or_fix_mcp'`. +a block is **retryable iff its `recoveryAction === 'install_or_fix_mcp'`**. -### Adapters (shape-preserving, so persisted JSON and readers do not change) +### Layer 3 — the package-level canonical evaluation (deny-wins + subtasks) -Co-located in `admission.ts`: +The single evaluation every surface consumes. It unions prohibitions across the +whole package (deny-wins) *before* admitting any requirement, and validates +MCP-aware subtasks against the normalized approved coverage — preserving the +current broker's package-wide prohibition removal and subtask checks +(`evaluateWorkPackageMcpBroker`, `mcp-execution-design.ts:617-631,697-721`). -- `decisionsToValidation(decisions): McpExecutionValidation` -- `decisionsToGrantPreview(decisions): McpGrantDecisions` — maps - `mode`/`status` → `proposed|warning|blocked`, keeps `decisionId`, - `sourceRequirementIndex`, `promptOverlayPresent`, **and adds** `mode`, - `recoveryAction`, `normalizedCapabilities`, `evidenceRefs`. -- `decisionsToBrokerCheck(decisions, label): WorkPackageMcpBrokerCheck` +```ts +export type McpAdmissionEvaluation = { + decision: McpAdmissionDecision + source: { // retained so adapters are shape-preserving + decisionId: string + sourceRequirementIndex: number + assignment: { type: McpAssignmentType; targetId: string | null } + fallback: { action: McpFallbackAction; message: string } + promptOverlayPresent: boolean + } + health: { installState: string; status: string; enabled: boolean; error: string | null } +} -`web/lib/mcps/execution-design-metadata.ts:99-232` keeps reading the same -`grantDecisions`/`validation` JSON, extended (non-breaking, back-derive `mode` for -old artifacts). +export type McpWorkPackageAdmission = { + schemaVersion: 2 + evaluations: McpAdmissionEvaluation[] + subtaskDecisions: Array<{ subtaskId: string; agent: string; capability: string; + class: McpCapabilityClass; status: McpAdmissionStatus; reason: string; recoveryAction?: McpRecoveryAction }> + referencedHealth: McpExecutionValidation['health'] // aggregate health array validation needs + aggregate: { + status: 'allowed' | 'warning' | 'blocked' + blocked: string[] + warnings: string[] + blockedReason: string | null + retryable: boolean // true iff every blocking decision is install_or_fix_mcp + primaryRecoveryAction?: McpRecoveryAction // precedence: revise_plan > approve_project_filesystem_context > install_or_fix_mcp > defer_live_mcp_feature + } +} -## Consolidation map (four paths → one) +export function admitWorkPackageMcp(input: { + entries: Array> // brokerEntries(): mcpRequirements ∪ metadata.mcpGrants + subtasks: Array> // metadata.mcpAwareSubtasks + label: string + statusFor: (mcpId: string) => ProjectMcpStatus | null + effectiveGrantFor: (mcpId: string) => EffectiveGrantState + hasPromptOnlyContextFor: (mcpId: string) => boolean +}): McpWorkPackageAdmission +``` -| Current function | Becomes | -|---|---| -| `validateMcpExecutionDesign` (`mcp-execution-design.ts:326-454`) | builds `admitMcpRequirement` per (requirement, agent); returns `decisionsToValidation` | -| `decisionStatus` + `deriveMcpGrantDecisions` (`:753-867`) | `deriveMcpGrantDecisions` calls `admitMcpRequirement`; `decisionStatus` deleted | -| `evaluateWorkPackageMcpBroker` (`:599-739`) + helpers `:488-591` | builds entries via `brokerEntries`, calls `admitMcpRequirement` with `mergeCapabilityFields`; returns `decisionsToBrokerCheck`; local `normalizeCapability/coverageCapabilityKey/unsafeCapability/capabilityArray/healthyStatus/canProceedWithoutMcp/isPlanningOnlyFilesystemWrite/SAFE_BETA_CAPABILITY_PATTERNS` deleted, imported from shared modules | -| `requiresFilesystemGrantApproval` / `summarizeFilesystemCapabilities` (`filesystem-grants.ts`) | filesystem-specific persistence over the shared classifier; imports normalization; keeps `FilesystemProjectCapability` nominal type + `ProjectFilesystemGrant` persistence | -| `mcpCapabilityList` (`work-package-executor.ts:1252-1260`) | imports `mergeCapabilityFields` | -| client helpers (`tasks/[id]/page.tsx:348-444`) | import shared helpers or consume a server-computed grant-state payload | +`admitWorkPackageMcp`: (1) build `packageProhibitedKeys` = union of +`coverageKeysForProhibition` over every entry; (2) `admitMcpRequirement` per entry +with that set (a capability prohibited anywhere is `deferred_live_mcp`, never +approved); (3) accumulate approved coverage keys minus the prohibition set; +(4) classify each subtask capability and check known MCP + safe scope + covered by +approved coverage, matching the current subtask loop; (5) fold everything into +`aggregate`. This is the canonical evaluation consumed by preview, approval, and +handoff. + +### Adapters (shape-preserving, over the evaluation, not bare decisions) + +Co-located in `web/lib/mcps/admission.ts`. They take the whole +`McpWorkPackageAdmission` (which retains `source` + `health`), so persisted JSON +and existing readers do not change shape — they are only *extended*: + +- `admissionToValidation(admission): McpExecutionValidation` — uses + `referencedHealth` for the aggregate health array. +- `admissionToGrantPreview(admission): McpGrantDecisions` — keeps `decisionId`, + `sourceRequirementIndex`, assignment, fallback, raw `health`, + `promptOverlayPresent`; **adds** `mode`, `recoveryAction`, + `normalizedCapabilities`, `capabilityClasses`, `evidenceRefs`. +- `admissionToBrokerCheck(admission): WorkPackageMcpBrokerCheck` — plus + `retryable`, `primaryRecoveryAction`. + +**Legacy artifacts.** `web/lib/mcps/execution-design-metadata.ts` (reader, ~99-232) +keeps reading the same `grantDecisions`/`validation` JSON. When a decision lacks +`mode` (pre-#172 artifact), the reader sets `mode:'unknown_legacy'` and **must not +invent** `bounded_context_approved` vs `bounded_context_required` — that +distinction cannot be recovered from status/capabilities/health alone. The UI +(S5) renders `unknown_legacy` as a neutral "Re-open plan to recompute" state and +derives live grant state from the package's current `metadata.mcpGrantPhases`, not +from the stale artifact. + +## Consolidation map (paths → one) + +Each edit is owned by exactly one slice. Symbols that other callers still import +keep a **transitional re-export** until their callers migrate in the owning slice. + +| Current symbol (line on `main`) | Becomes | Owner | +|---|---|---| +| `validateMcpExecutionDesign` (`mcp-execution-design.ts:326`) | builds `admitWorkPackageMcp` over design requirements; returns `admissionToValidation` | S2 | +| `decisionStatus` (`:753`) + `deriveMcpGrantDecisions` (`:803`) | `deriveMcpGrantDecisions` calls `admitWorkPackageMcp`; returns `admissionToGrantPreview`; `decisionStatus` deleted | S2 | +| `evaluateWorkPackageMcpBroker` (`:599`) + helpers `capabilityArray:498`, `approvedCoverageCapabilityKeys:551`, `isPlanningOnlyFilesystemWrite:318` | calls `admitWorkPackageMcp` with `mergeCapabilityFields`; returns `admissionToBrokerCheck`; local normalization/allowlist deleted, imported from shared modules | S2 | +| `SAFE_BETA_CAPABILITY_PATTERNS` (`:7-18`) | migrated verbatim into `SAFE_READ_SUPPLEMENT`; **kept as a transitional re-export in S1**, deleted in S2 after callers move | S1 create / S2 delete | +| `requiresFilesystemGrantApproval` / `summarizeFilesystemCapabilities` (`filesystem-grants.ts:303/90`) | filesystem-specific **projection** of `admitWorkPackageMcp` (filesystem decisions only); imports normalization; keeps `FilesystemProjectCapability` nominal type + `ProjectFilesystemGrant` persistence + `EffectiveGrantState` reader | S3 | +| `isRetryableMcpBrokerBlock` (`:90`) + `buildMcpBrokerBlockMetadata` (`blocked-handoff-retry.ts:32`) | consume `aggregate.retryable`/`primaryRecoveryAction`; persist `mode`+`recoveryAction` under `metadata.mcpBroker` | S2 (broker), S5 (persist for UI) | +| `mcpGrantsForAgent` (`workforce-materializer.ts:157`) | persists `mode`, `recoveryAction`, `normalizedCapabilities`, `evidenceRefs` on each grant (schema bump), not just id/mcp/caps/requirement/status/reason/fallback/health | S2 | +| `mcpCapabilityList` (`work-package-executor.ts:1527`) | imports `mergeCapabilityFields`; executor filesystem gating uses shared `coverageKeysForGrant`/`classifyCapability` | S4 | +| client helpers (`tasks/[id]/page.tsx:348-444`) | import shared helpers OR consume a server-computed grant-state payload; render `mode`+`recoveryAction` via `admission-copy.ts` | S5 | ## Implementation slices -### S1 — Contract and terminology (child issue → S1) +Dependency order: **S1 → S2 → {S3, S4} → S5 → S6.** S3 and S4 both depend on S2's +canonical evaluation + persisted shape. **S5 depends on S4** (it renders the +run-evidence schema S4 defines) and on S2. S6 depends on S2–S5. -- Create `web/lib/mcps/capability-normalization.ts` and `web/lib/mcps/admission.ts` - with the types, primitives, `admitMcpRequirement`, and the three adapters above. -- Make `classifyCapability` read `MCP_CATALOG[mcpId].runtime.capabilities` and - encode the explicit `deferred_live_mcp` list. Delete `SAFE_BETA_CAPABILITY_PATTERNS`. +### S1 — Contract and terminology + +- Create `web/lib/mcps/capability-normalization.ts` (Layer 0 primitives, + `classifyCapability`, `mcpDeliveryKind`, `SAFE_READ_SUPPLEMENT`) and + `web/lib/mcps/admission.ts` (Layers 1–3 + adapters). +- Keep `SAFE_BETA_CAPABILITY_PATTERNS` as a transitional re-export of + `SAFE_READ_SUPPLEMENT` so S1 compiles without touching S2's callers. - Write this ADR (done) and align roadmap/task-detail copy on the three-way - terminology (planning-only / bounded read-only / deferred live MCP). - -### S2 — Broker consolidation (child issue → S2) - -- Migrate the four functions per the consolidation map. Delete duplicated helpers. -- **Enforce admission at approval.** In `web/app/api/tasks/[id]/approve/route.ts`, - after the existing `requiresFilesystemGrantApproval` check (~`:189-209`), run the - admission decision over every package and refuse approval (mirroring the existing - `missingFilesystemGrant` early return) when any decision is `status:'blocked'`, - returning the normalized `reason` + `recoveryAction`. Approval then enforces - exactly what handoff enforces. -- **Invariant:** for any fixed package, `deriveMcpGrantDecisions` (preview), - `evaluateWorkPackageMcpBroker` (handoff), and `requiresFilesystemGrantApproval` - produce the same `mode`/`status`. New tests - (`web/__tests__/mcp-admission-invariant.test.ts`) assert agreement for: required - no-capability grants, prompt-only context, filesystem read/list/search, - `filesystem.project.write`, an unsafe/deferred GitHub write, an unknown MCP, and a - requirement expressed via each of the four capability fields. - -### S3 — Filesystem grant recovery (child issue → S3) - -- `requiresFilesystemGrantApproval` (`filesystem-grants.ts:315-322`) must NOT return - `{blocked:false}` for a `denied` effective phase when blocking capabilities are - non-empty; return a distinct terminal outcome (e.g. `deniedRequired:true`) so - handoff holds the package pre-claim (zero attempts). Optional/`continue_without_mcp` - denials stay non-blocking. -- `filesystemGrantHandoffBlock` (`work-package-handoff.ts:876-906`): branch on the - denied-required outcome to `failWorkPackageForFilesystemGrant` with a message that - names the operator denial and is recoverable via the existing - `FILESYSTEM_GRANT_BLOCK_METADATA_KEY`. Pass `project?.mcpConfig` into - `requiresFilesystemGrantApproval` in BOTH branches (the default branch at - `:896-899` currently omits it), closing the approval-vs-handoff project-coverage divergence. -- Make recovery deterministic: the per-task `always_allow` route - (`tasks/[id]/filesystem-grants/route.ts:418-421`) must reconcile `failed` - grant-blocked siblings too, or delegate to the shared reconciliation used by - `projects/[id]/filesystem-grant/route.ts:166-243`. Both endpoints must recover the - identical package set for identical grant state. -- Preserve: filesystem blocks are `terminalBlock:true` and never auto-retried - (`blocked-handoff-retry.ts:67-79`). - -### S4 — Prompt/context assembly and bounded-context packet evidence (child issue → S4; builds on #43) + terminology. +- **No deletions of live-referenced symbols in S1.** + +### S2 — Broker consolidation and approval enforcement + +- Migrate the paths per the consolidation map. Delete duplicated helpers only + after their callers import the shared modules. +- **Enforce admission at approval.** In `web/app/api/tasks/[id]/approve/route.ts`: + acquire the MCP health snapshot via `getProjectMcpOverview(project)` **before** + and **outside** the status-flip `db.transaction` (it performs live checks and + writes cached `ProjectMcpStatus` rows — it must not run inside the transaction + that flips the task to `approved`). Run `admitWorkPackageMcp` over every package + using that snapshot; if any `aggregate.status === 'blocked'`, return the same + 409 shape as the existing `missingFilesystemGrant` early return (`:199-209,280`) + with the normalized `reason` + `primaryRecoveryAction`. **Persist the + `checkedAt` health snapshot** the approval decision used on the plan-approval + gate metadata (next to the existing `approvedGrantSnapshot`, `:221-263`). +- **Parity guarantee (narrowed).** For a fixed package and a *fixed health + snapshot*, `deriveMcpGrantDecisions` (preview), the approval check, and + `evaluateWorkPackageMcpBroker` (handoff) return the same `mode`/`status` because + they call one producer. MCP health/config can change between approval and + handoff, so the promise is: **a block already visible in the approval-time + snapshot is surfaced at approval, not missed until handoff** — not that an + approved task can never block later. +- **Invariant tests** (`web/__tests__/mcp-admission-invariant.test.ts`): preview, + approval, and handoff agree for required no-capability grants, prompt-only + context, filesystem read/list/search, `filesystem.project.write`, an + unsafe/deferred GitHub write, a healthy GitHub read (planning-context, not + bounded), an unknown MCP, a package-wide prohibition that must beat a + per-entry approval, an MCP-aware subtask capability, and a requirement expressed + via each of the four capability fields. `requiresFilesystemGrantApproval` is + tested only as the *filesystem projection* of the same evaluation. + +### S3 — Filesystem grant recovery (deterministic, recoverable) + +- `requiresFilesystemGrantApproval` (`filesystem-grants.ts:315-322`) must stop + returning `{blocked:false}` for a `denied` effective phase when blocking + capabilities are non-empty and no `continue_without_mcp` fallback applies. + Return a distinct `deniedRequired:true` so handoff HOLDS the package pre-claim + (zero attempts) instead of letting the executor throw at + `work-package-executor.ts:1790`. +- **Recoverable held state, not a crash.** `failWorkPackageForFilesystemGrant` + (`work-package-handoff.ts:807`) sets the package to **`blocked`** (in the + recovery set) with the `mcpGrantBlock` marker and does **not** drive the task to + `failed`; `progressWorkforce` (`:1165-1193`) must treat a filesystem-grant + terminal block as *held* (task stays operator-actionable), not as task failure. + Both never-approved-required and denied-required take this held path. +- `filesystemGrantHandoffBlock` (`work-package-handoff.ts:876-906`): pass + `project?.mcpConfig` into `requiresFilesystemGrantApproval` in **both** branches + (the default branch at `:896-899` currently omits it), closing the + approval-vs-handoff project-coverage divergence. +- **One project-wide reconciliation routine.** Extract + `reconcileFilesystemGrantsForProject(projectId, tx)` and call it from **both** + the per-task `always_allow` path (`tasks/[id]/filesystem-grants/route.ts:411-446`, + which currently reconciles only current-task standard-status siblings) and the + project route (`projects/[id]/filesystem-grant/route.ts:166-244`). Both endpoints + recover the identical package set for identical grant state. +- **Precedence.** A later project-level `always_allow` grant covering a capability + supersedes an earlier package-local `denied` effective phase for that capability + (operator's later, broader decision wins). Document and test this precedence in + `readEffectiveGrantState`. +- **Acceptance tests assert exact transitions** (package `pending/ready → + blocked`, task not `failed`, zero new `agentRuns`; then grant approval → + package `ready`, task re-driven), not just attempt counts. +- Preserve: a filesystem block is never auto-retried + (`shouldAutoRetryBlockedHandoff`, `blocked-handoff-retry.ts:67-79`). + +### S4 — Prompt/context assembly and bounded-context packet evidence (builds on #43) - Specialist prompts receive `promptOverlay` + `mcpAwareSubtasks` + - `mcpRequirements` as **instructions**, never as tool grants - (`work-package-executor.ts` prompt assembly). No live MCP handle is ever issued. -- The bounded read-only context packet (root path, selected file names/excerpts, - included/omitted counts, redaction summary — per ADR 0008) is attached to the run - as an **inspectable artifact**, referenced from `McpAdmissionDecision.evidenceRefs`. + `mcpRequirements` as **instructions**, never as tool grants (executor prompt + assembly around `work-package-executor.ts:1527-1583`). No live MCP handle is + ever issued. +- **Evidence lifecycle — planned scope vs issued evidence.** Pre-run, + `McpAdmissionDecision.evidenceRefs` carries only *planned scope* (root path + + capability set), never file contents. The bounded read-only context packet is + assembled during execution and requires an `agentRunId`; after the run (including + **failed** runs) a run-level record is updated with stable artifact refs to the + packet **metadata** artifact (root, selected file names, included/omitted counts, + redaction summary — per ADR 0008, which forbids persisting raw file contents). + **File contents stay prompt-only and are not persisted**; "selected excerpts" are + not written to an inspectable artifact. `mcpCapabilityList` imports + `mergeCapabilityFields`; executor filesystem gating uses shared + `coverageKeysForGrant`/`classifyCapability`. - Sandbox-generated files (`.forge/task-runs/...`) stay clearly separated from host-repository writes in artifacts. -- `mcpCapabilityList` imports `mergeCapabilityFields`; executor filesystem gating uses - shared `coverageKeysForGrant`/`classifyCapability` rather than re-deriving - `filesystem.project.read` membership. - -### S5 — UI and copy hardening (child issue → S5) - -- New `web/lib/mcps/admission-copy.ts`: pure map from `mode`+`recoveryAction`+`status` - → `{ statusKey, badgeText, headline, body, cta? }`. Every surface reads it, so copy - cannot drift. Mapping: - - `planning_only` → neutral "Planning context", body "Recorded as run-scoped prompt - instructions; writes go through the Forge sandbox/host-apply path, no live MCP - tools attached", no CTA. - - `bounded_context_required` → amber "Needs project context", CTA scroll to the + +### S5 — UI and copy hardening + +- New `web/lib/mcps/admission-copy.ts`: pure map from `mode`+`recoveryAction`+ + `status` → `{ statusKey, badgeText, headline, body, cta? }`. Every surface reads + it. Mapping: + - `planning_only` → neutral "Planning context", no CTA. + - `bounded_context_required` → amber "Needs project context", CTA to the package's filesystem grant control. - `bounded_context_approved` → green "Context approved". - - `blocked` + `install_or_fix_mcp` → red, CTA deep-link + - `blocked`+`install_or_fix_mcp` → red, CTA deep-link `/dashboard/projects/{projectId}#project-mcps-heading`. - - `blocked` + `revise_plan` → red, CTA open Request-changes flow. - - `deferred_live_mcp` → **neutral slate** "Deferred — beta boundary", body "Live MCP - tool handles are not part of this beta. This is a product boundary, not a broken - install", no CTA. -- Add `deferred`/`planning` neutral buckets to `statusBadgeClass` - (`tasks/[id]/page.tsx:1203-1245`). + - `blocked`+`revise_plan` → red, CTA "Request changes / regenerate plan". + - `deferred_live_mcp` → **neutral slate** "Deferred — beta boundary", body "Live + MCP tool handles are not part of this beta. This is a product boundary, not a + broken install." **When it is a *required* (blocking) deferred requirement, it + still carries the `revise_plan` CTA** so the operator can remove/regenerate the + offending requirement instead of being stuck with an unapprovable plan and no + action; an *optional* deferred requirement is warning-only and approvable. + - `unknown_legacy` → neutral "Re-open plan to recompute"; grant state read live + from package metadata, never invented. +- Add `deferred`/`planning`/`legacy` neutral buckets to `statusBadgeClass` + (`tasks/[id]/page.tsx:1203`). - Extend `execution-design-metadata.ts` decision type/normalizer to carry `mode`, - `recoveryAction`, `normalizedCapabilities`, `evidenceRefs` (back-derive `mode` for - old artifacts). -- Replace the status-only ternary (`:3171-3175`), split planning-only warnings from - degradation warnings (`:3146-3155`), stop rendering deferred capabilities as - destructive alerts (`:3135-3144`), gate `RetryHandoffControls` on retryability, - surface the bounded-context packet inline, and add remediation CTAs + - bounded-context note on the projects page (`projects/[id]/page.tsx:1035-1052`) and - the MCPs catalog page (`mcps/page.tsx`). -- The worker must persist `mode`+`recoveryAction` on BOTH the `grantDecisions` - preview decisions AND the per-package block metadata (`blockedReason`) so - `RetryHandoffControls` routes correctly. - -### S6 — End-to-end regression (child issue → S6) + `recoveryAction`, `normalizedCapabilities`, `capabilityClasses`, `evidenceRefs` + (`unknown_legacy` for old artifacts). +- Replace the status-only ternary, split planning-only warnings from degradation + warnings, stop rendering deferred capabilities as destructive alerts, gate + `RetryHandoffControls` on `aggregate.retryable`, surface the bounded-context + packet metadata inline (from S4's schema), and add remediation CTAs + + bounded-context note on the projects page and the MCPs catalog page + (`mcps/page.tsx`). +- **Persisted schema is versioned.** The worker persists `mode`+`recoveryAction` + on BOTH the `grantDecisions` preview decisions AND the per-package block metadata + (`metadata.mcpBroker`, a JSON object — `work_packages.blocked_reason` stays the + human text). When multiple decisions block, `aggregate.retryable` is true iff + every blocking decision is `install_or_fix_mcp`, and + `primaryRecoveryAction` is the highest-precedence block + (`revise_plan > approve_project_filesystem_context > install_or_fix_mcp > + defer_live_mcp_feature`), which drives the single UI CTA. + +### S6 — End-to-end regression - `web/__tests__` (or `web/e2e`) regression for a local-only tiny task-tracker - project: Architect creates frontend/QA/docs/review packages; the MCP plan includes - prompt-only context and no live tool handles; approval succeeds; handoff advances - ready packages; a required `filesystem.project.read|search` grant still blocks until - approved or denied through the intended path; a deferred GitHub-write capability is - reported as `deferred_live_mcp`, not an install error. -- Plus the S2 preview==handoff invariant suite. + project: Architect creates frontend/QA/docs/review packages; the MCP plan + includes prompt-only context and no live tool handles; approval succeeds; + handoff advances ready packages; a required `filesystem.project.read|search` + grant holds the package (recoverable, zero attempts) until approved or denied + through the intended path; a deferred GitHub-write capability is reported as + `deferred_live_mcp`, not an install error; a healthy GitHub read is planning + context, not an approvable bounded packet. +- Plus the S2 preview==approval==handoff invariant suite. + +## #43 re-scope + +#43 predates this boundary and still asks the broker to *issue scoped MCP tools*, +agents to *receive approved tools*, and MCP tool calls to be *audited* — which +conflicts with this ADR's no-live-handle rule — and its examples use unqualified +capability names (`repository.read`, `branches.create`). Under #172, #43 is +re-scoped to its **planning/overlay output only** (Architect MCP assignment, +prompt overlays, MCP-aware subtasks feeding S4), with capability vocabulary +standardized to the `github.*` / `filesystem.project.*` namespace the classifier +and catalog use. Its runtime tool-issuance and tool-call-audit acceptance criteria +move to the later security-reviewed live-MCP epic (depends on #40 adversarial mode +and #60 security-review workforce). See #43 for the updated scope. ## Consequences - Preview, approval, handoff, filesystem recovery, and UI copy are structurally - incapable of disagreeing because they consume one decision object built by one - producer over one set of primitives sourced from one catalog. -- `deferred_live_mcp` is a first-class, named mode, so blocks say "deferred live MCP - feature" instead of a generic beta-scope error, and the UI shows a product - boundary instead of a broken install. -- Denying a required filesystem grant no longer burns an execution attempt; recovery - is deterministic across both endpoints. + incapable of disagreeing for a fixed health snapshot, because they consume one + evaluation built by one producer over one set of primitives sourced from one + catalog. The filesystem gate is a tested projection of that evaluation. +- `deferred_live_mcp` is a first-class, named mode with an operator action, so + blocks say "deferred live MCP feature" and the UI shows a product boundary with a + path forward instead of a broken install. +- Denying or withholding a required filesystem grant no longer burns an execution + attempt or dead-ends; it holds the package in a recoverable state, and recovery + is deterministic across both endpoints via one project-wide reconciliation. +- Bounded context is delivered only where a producer exists (filesystem); + safe-but-undeliverable reads (github) are honest planning context. - Live MCP runtime handles remain out of scope. Issuing them is a later, - security-reviewed epic that depends on #40 (adversarial mode) and #60 (security - review workforce). The eventual DB-backed MCP capability catalogue belongs to #121; - this ADR keeps the allow-list catalog-sourced in code as the precursor. + security-reviewed epic that depends on #40 and #60. The eventual DB-backed MCP + capability catalogue belongs to #121; this ADR keeps the allow-list + catalog-sourced in code as the precursor. ## Out of scope diff --git a/docs/developer-guide.md b/docs/developer-guide.md index d1f885f..3ceebf3 100644 --- a/docs/developer-guide.md +++ b/docs/developer-guide.md @@ -40,8 +40,12 @@ overlays, MCP-aware subtasks, `filesystem.project.write`; warn, never block), **bounded read-only** (`filesystem.project.read|list|search`; may need an explicit operator grant), and **deferred live MCP** (live tool handles, GitHub write/branch/PR/merge/settings/secret, filesystem write/delete/admin; a product -boundary, not a broken install). The classifier is sourced from -`MCP_CATALOG.runtime.capabilities`. Historically the same policy was re-derived +boundary, not a broken install). The classifier is catalog-sourced +(`MCP_CATALOG.runtime.capabilities` plus a documented safe-read supplement +carrying today's allowlist), and a safe read is delivered as a bounded packet +only where a context producer exists for that MCP -- filesystem today; GitHub +reads have no producer yet, so they are honest planning context, not an +approvable bounded grant. Historically the same policy was re-derived by four divergent paths (`validateMcpExecutionDesign`, `deriveMcpGrantDecisions`, `evaluateWorkPackageMcpBroker`, and `requiresFilesystemGrantApproval`); they are becoming thin adapters over the shared core in `web/lib/mcps/admission.ts`. diff --git a/docs/operator-guide.md b/docs/operator-guide.md index 3797198..0fb1ac0 100644 --- a/docs/operator-guide.md +++ b/docs/operator-guide.md @@ -225,10 +225,16 @@ FORGE_HOST_REPOSITORY_WRITES=0 With the default execution path: -1. The operator approves the Architect plan. Approval runs the same MCP admission - check as handoff, so a plan that approves will not silently stall later on MCP - grant semantics (EPIC #172, ADR - [0009](adr/0009-mcp-admission-contract.md)). +1. The operator approves the Architect plan. Today, approval enforces only the + required *filesystem* context grants; it does not yet run the full + MCP/capability broker, so a plan can still block later at handoff for a + non-filesystem MCP reason. EPIC #172 slice S2 (ADR + [0009](adr/0009-mcp-admission-contract.md)) makes approval run the same + admission check as handoff over a captured MCP health snapshot. Note that even + after S2, MCP health and configuration can change between approval and handoff; + the guarantee is only that a block already visible in the approval-time + snapshot is surfaced at approval instead of being missed until handoff — not + that an approved task can never block later. 2. Forge releases ready work packages and runs the MCP/capability broker. 3. Required blocked MCP/tool grants stop the package before execution. Optional grants can continue only when the approved fallback is non-blocking. @@ -242,15 +248,19 @@ With the default execution path: 8. QA, Reviewer, and Security gates appear when required. In this beta, those are manual operator decisions, not proof that separate reviewer agents ran. -On the task detail page, each MCP request resolves to one of four states so you -can act without reading logs (EPIC #172): +EPIC #172 slice S5 hardens the task detail page so each MCP request resolves to +one of four states you can act on without reading logs (target-state UI; the +copy/badge contract lands with S5): - **Planning context** -- the Architect only suggested an MCP; it is recorded as prompt instructions and never blocks handoff. Generated file writes go through the Forge sandbox/host-apply path, not a live MCP write tool. - **Needs project context** -- the package needs bounded read-only filesystem context (`filesystem.project.read|list|search`). Approve or deny the exact - grant shown; denial is recorded and the package is held, not crashed. + grant shown. Approval today holds a never-approved required grant before the + package runs; slice S3 extends the same held, zero-attempt treatment to an + explicit denial so it is recorded and held rather than burning an execution + attempt. - **MCP needs setup** -- an MCP is missing/unhealthy/unauthenticated. Use the linked setup/retry action on the project MCP panel. - **Deferred -- beta boundary** -- the request needs a live MCP tool handle (or a diff --git a/docs/roadmap.md b/docs/roadmap.md index 0b1bab7..c1e10f4 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -169,12 +169,14 @@ The epic makes planning, approval, UI state, filesystem grants, and handoff foll **one admission contract**: a normalized `McpAdmissionDecision` (ADR [0009](adr/0009-mcp-admission-contract.md)) with a capability `mode` (`planning_only`, `bounded_context_required`, `bounded_context_approved`, -`blocked`, `deferred_live_mcp`) and a `recoveryAction`, sourced from -`MCP_CATALOG.runtime.capabilities`. Capabilities resolve to three classes: -planning-only (warn, never block, including `filesystem.project.write`), bounded -read-only (`filesystem.project.read|list|search`, may need an operator grant), and -deferred live MCP (live tool handles and write/merge/admin -- a product boundary, -not a broken install). Live MCP tool handles remain out of scope. +`blocked`, `deferred_live_mcp`) and a `recoveryAction`, classified from +`MCP_CATALOG` reads plus a documented safe-read supplement. Capabilities resolve +to three classes: planning-only (warn, never block, including +`filesystem.project.write`), bounded read-only +(`filesystem.project.read|list|search`, may need an operator grant -- and only +delivered as a bounded packet where a context producer exists, filesystem today), +and deferred live MCP (live tool handles and write/merge/admin -- a product +boundary, not a broken install). Live MCP tool handles remain out of scope. Delivered as six sweeping child slices plus #43: #176 (S1 contract/taxonomy), #177 (S2 consolidation + approval enforcement), #178 (S3 deterministic filesystem diff --git a/docs/wiki.md b/docs/wiki.md index 95d11cc..3a46d18 100644 --- a/docs/wiki.md +++ b/docs/wiki.md @@ -52,12 +52,16 @@ Architect plan saved -> manual QA/Reviewer/Security gates where required ``` -The MCP admission check is one shared contract across grant preview, plan -approval, and handoff (EPIC #172, ADR -[0009](adr/0009-mcp-admission-contract.md)). Each MCP request resolves to a -single mode -- planning-only context, bounded read-only context (approve/deny), -an MCP that needs setup, or a deferred live-MCP feature -- so a plan that approves -will not silently stall at handoff. +EPIC #172 (ADR [0009](adr/0009-mcp-admission-contract.md)) introduces one shared +MCP admission contract across grant preview, plan approval, and handoff. This is +the target architecture, not yet the shipped behavior: today plan approval +enforces only required filesystem context grants, and the shared broker check at +approval time lands with slice S2. Under the contract each MCP request resolves +to a single mode -- planning-only context, bounded read-only context +(approve/deny), an MCP that needs setup, or a deferred live-MCP feature -- so a +block visible in the approval-time MCP snapshot is surfaced at approval rather +than only at handoff. Health or configuration changes between approval and +handoff can still introduce a new block. Task detail controls now cover the common operator interventions: From 04f27ba380f678ba66dea7e3f620bfd3852e2e0d Mon Sep 17 00:00:00 2001 From: Joncallim <64296013+Joncallim@users.noreply.github.com> Date: Sat, 11 Jul 2026 13:04:36 +0800 Subject: [PATCH 3/3] =?UTF-8?q?docs:=20address=20GPT-5.6=20review=20?= =?UTF-8?q?=E2=80=94=20harden=20MCP=20admission=20decision=20table=20(ADR?= =?UTF-8?q?=200009)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve 10 findings on the admission contract and sync the ready-for-agent issue bodies (#172, #176, #177, #178, #180, #181, #43) to the current ADR. Decision table / classifier: - Deny-wins: a package-wide prohibited capability blocks unconditionally and can never be downgraded to a warning by optional/continue_without_mcp (C1). - Enumerate DEFERRED_CAPABILITY_FAMILIES; a known-MCP unrecognized/typo capability is `unknown` and blocks revise_plan, not silently deferred (C2). - Total fallback matrix keyed on canProceedWithoutMcp, so ask_user/block never become non-blocking across deferred/unhealthy/missing cases (C3). - Required planning-context (github) reads require materialized prompt context or block revise_plan; planning-context/planning-only modes are not health-gated (C4). Contract / evaluation: - Two subtask coverage sets (bounded vs planning-context) so a safe github.*.read subtask is admitted when its requirement was admitted as planning context (C5). - Layer 3a canonical entry join: one admitMcpRequirement per (sourceRequirementIndex, agent, mcpId); preview partitions per-agent before deny-wins so its scope matches handoff (C6). - Preview keeps legacy status via allowed->proposed and adds canonical admissionStatus; parity asserted on (mode, admissionStatus) (C7). - Versioned McpHealthSnapshot with checkedAt persisted at approval (C8). - EffectiveGrantState gains phase:'revoked' + revocationReason; distinct recovery copy from never-approved (C9). - Broker-block persistence (metadata.mcpBroker) owned entirely by S2; S5 reads only, so retry/recovery ships before the UI slice (C10). Co-Authored-By: Claude Opus 4.8 --- docs/adr/0009-mcp-admission-contract.md | 308 +++++++++++++++++++----- 1 file changed, 242 insertions(+), 66 deletions(-) diff --git a/docs/adr/0009-mcp-admission-contract.md b/docs/adr/0009-mcp-admission-contract.md index 671e5f9..960edcf 100644 --- a/docs/adr/0009-mcp-admission-contract.md +++ b/docs/adr/0009-mcp-admission-contract.md @@ -141,26 +141,64 @@ export type McpDeliveryKind = export function mcpDeliveryKind(mcpId: string): McpDeliveryKind export function normalizeCapability(cap: string): string // trim().toLowerCase().replace(/\s+/g,'_') + +// Enumerated, recognized live-tool families. A capability that matches one of +// these on a known MCP is a *known but deferred* capability. A capability that +// matches NOTHING (safe-read, planning-only, or a deferred family) on a known MCP +// is a typo / unrecognized value and classifies as `unknown` (blocks revise_plan). +export const DEFERRED_CAPABILITY_FAMILIES: RegExp[] = [ + /^github\.[a-z_]+\.(write|create|update|delete|merge)$/, + /^github\.(branches|pull_requests|settings|secrets|workflows)\./, + /^filesystem\.[a-z._]*\.(write|delete|admin|move|create)$/, +] + export function classifyCapability(mcpId: string, cap: string): McpCapabilityClass ``` -`classifyCapability`'s safe-read set is `MCP_CATALOG[mcpId].runtime.capabilities` -**∪ `SAFE_READ_SUPPLEMENT`**, where `SAFE_READ_SUPPLEMENT` is migrated *verbatim* -from `SAFE_BETA_CAPABILITY_PATTERNS` so no capability that is allowed today -becomes newly `unknown` (specifically `github.actions.read`, -`github.repository.list|search`, `github.contents.list|search`). Deleting -`SAFE_BETA_CAPABILITY_PATTERNS` therefore moves the exact same patterns into a -single documented data set — **not a behavior change**. `filesystem.project.write` -classifies as `planning_only`. Anything not in the safe-read set and not -planning-only classifies as `deferred_live_mcp` (known MCP) or `unknown`. +`classifyCapability` applies a **total, ordered** rule set (first match wins), so +every capability lands in exactly one class and a typo can never be silently +admitted: + +1. `mcpId` is not a known MCP → `unknown`. +2. `normalizeCapability(cap) === 'filesystem.project.write'` (Forge writes it via + the sandbox JSON path, never a live tool) → `planning_only`. +3. `cap` ∈ safe-read set = `MCP_CATALOG[mcpId].runtime.capabilities` **∪ + `SAFE_READ_SUPPLEMENT`** → `bounded_read_only`. +4. `cap` matches a `DEFERRED_CAPABILITY_FAMILIES` pattern → `deferred_live_mcp`. +5. known MCP, matched nothing above (unrecognized / typo) → `unknown`. + +`SAFE_READ_SUPPLEMENT` is migrated *verbatim* from `SAFE_BETA_CAPABILITY_PATTERNS` +so no capability allowed today becomes newly `unknown` (specifically +`github.actions.read`, `github.repository.list|search`, +`github.contents.list|search`). Deleting `SAFE_BETA_CAPABILITY_PATTERNS` therefore +moves the exact same patterns into a single documented data set — **not a behavior +change**. + +**Classifier matrix (illustrative — every row is a required test case):** + +| mcpId | capability | class | why | +|---|---|---|---| +| `filesystem` | `filesystem.project.read` | `bounded_read_only` | catalog safe-read (rule 3) | +| `filesystem` | `filesystem.project.write` | `planning_only` | rule 2 (sandbox JSON path) | +| `filesystem` | `filesystem.project.delete` | `deferred_live_mcp` | deferred family (rule 4) | +| `github` | `github.issues.read` | `bounded_read_only` | catalog safe-read | +| `github` | `github.actions.read` | `bounded_read_only` | `SAFE_READ_SUPPLEMENT` | +| `github` | `github.pull_requests.write` | `deferred_live_mcp` | deferred family | +| `github` | `github.branches.create` | `deferred_live_mcp` | deferred family | +| `github` | `github.issues.reed` (typo) | `unknown` | rule 5 → block `revise_plan` | +| `slack` | `slack.messages.read` | `unknown` | rule 1 (unknown MCP) | **Risk ≠ delivery.** A `bounded_read_only` github read is *safe* but not *deliverable as a bounded packet* — `mcpDeliveryKind('github') === 'planning_context_only'`, so it is admitted as planning/prompt context (recovery -`continue_as_prompt_context`) and stays health-gated exactly as today; it is never -offered `approve_project_filesystem_context` and never marked -`bounded_context_approved`. Only `filesystem` bounded reads flow through the -approve/deny bounded-context path. +`continue_as_prompt_context`); it is never offered +`approve_project_filesystem_context` and never marked `bounded_context_approved`. +Because that path consumes **no** MCP runtime (the specialist receives prose +instructions, not a live tool), it is **not health-gated** — it is instead gated on +whether prompt context was actually materialized for that MCP +(`hasPromptOnlyContext`; see decision-table step 6). A *required* github read with +no materialized prompt context is a plan defect and blocks with `revise_plan`. Only +`filesystem` bounded reads flow through the approve/deny bounded-context path. Shared primitives (single copy each, all in `capability-normalization.ts`): `coverageKeysForGrant(cap)` / `coverageKeysForProhibition(cap)` (one documented @@ -181,25 +219,34 @@ policy: ```ts export type EffectiveGrantState = { - phase: 'none' | 'proposed' | 'approved' | 'denied' | 'not_issued' + phase: 'none' | 'proposed' | 'approved' | 'denied' | 'revoked' | 'not_issued' source: 'none' | 'package-local' | 'project-level' status: 'not_issued' | 'approved' | 'denied' grantMode?: 'allow_once' | 'always_allow' consumed?: boolean // allow_once already issued -> treat as none for a retry coveredCapabilities: string[] // canonical filesystem.project.* grantApprovalId?: string + revocationReason?: string // set only when phase === 'revoked' } export function readEffectiveGrantState(pkg: { metadata: unknown }, project: { mcpConfig: unknown }): EffectiveGrantState ``` `readEffectiveGrantState` distinguishes never-approved (`phase:'none'|'proposed'`), -explicitly denied (`status:'denied'`), consumed allow-once (`consumed:true`), -revoked project grant (was `project-level` but coverage no longer holds → -collapses to `none`), insufficient coverage (`coveredCapabilities` lacks a +explicitly denied (`phase:'denied'`), consumed allow-once (`consumed:true`), +**revoked project grant** and insufficient coverage (`coveredCapabilities` lacks a required capability), package-local approval (`source:'package-local'`), and -project-level approval (`source:'project-level'`). It reads the same package +project-level approval (`source:'project-level'`). A **revoked** grant is a +`project-level` grant that previously covered the package but whose coverage was +later removed or narrowed: it returns `phase:'revoked'`, `source:'project-level'`, +with `revocationReason` set — it is **not** collapsed to `none`, so the producer +and the UI can say "project filesystem context was removed, approve it again" and +keep it distinct from a first-time request. It reads the same package `metadata.mcpGrantPhases.effective` and `project.mcpConfig.grants.filesystem` the -current routes write. +current routes write; the revoked case is exactly the effective phase whose +`source === 'project-filesystem-approval'` while the current +`project.mcpConfig.grants.filesystem` no longer covers the required capabilities +(the case `packageProjectFilesystemEffectivePhase` + `requiresFilesystemGrantApproval` +handle today at `work-package-handoff.ts:869-893`). ### Layer 2 — the per-requirement producer @@ -246,39 +293,65 @@ lose information and a `class:'unknown'` capability is representable. The aggreg `mode`/`status` is chosen by a **total, precedence-ordered decision table** (first match wins; every branch is defined): +Let `canProceed = canProceedWithoutMcp(requirement, fallback)` — i.e. `optional` +**and** `fallback.action === 'continue_without_mcp'`. `ask_user` and `block` are +**blocking** fallbacks, so `canProceed` is `false` for them. Every "non-blocking" +branch below keys off `canProceed`, never off `requirement === 'optional'` alone, +so the fallback matrix is total across all three fallback actions. + 1. **Unknown MCP** → `mode:'blocked'`, `status:'blocked'`, `recoveryAction:'revise_plan'`. 2. **Any capability class `unknown`** (known MCP, unrecognized/typo capability) → `mode:'blocked'`, `status:'blocked'`, `recoveryAction:'revise_plan'`. 3. **Any capability prohibited package-wide** (`normalizeCapability(cap)` ∈ - `packageProhibitedKeys`) → treated as `deferred_live_mcp`; go to (4). -4. **Any capability class `deferred_live_mcp`** → `mode:'deferred_live_mcp'`. If - `requirement==='required'` → `status:'blocked'`, `recoveryAction:'revise_plan'`. - If `optional` → `status:'warning'`, `recoveryAction:'defer_live_mcp_feature'` - (approvable, non-blocking). "Product boundary, not broken install" copy applies. + `packageProhibitedKeys`) → `mode:'blocked'`, `status:'blocked'`, + `recoveryAction:'revise_plan'`, **unconditionally**. This is its own terminal + outcome evaluated *before* any fallback handling: an explicit package prohibition + is deny-wins and can **never** be downgraded to a warning by `optional` / + `continue_without_mcp`. (Matches the current broker, where an unsafe/prohibited + capability blocks regardless of `optional`.) +4. **Any capability class `deferred_live_mcp`** → `mode:'deferred_live_mcp'`. + `status:'warning'` **iff `canProceed`**, else `status:'blocked'`. `recoveryAction` + is `defer_live_mcp_feature` when warning (approvable, non-blocking) and + `revise_plan` when blocking (so the operator can remove/regenerate the offending + requirement rather than be stuck). "Product boundary, not broken install" copy + applies to the `mode` either way. 5. **Any capability class `bounded_read_only` with `deliveryKind === 'bounded_context_packet'`** (filesystem): - - covered by `effectiveGrant` (`status:'approved'` & not `consumed` & covers the - required capabilities) → `mode:'bounded_context_approved'`, `status:'allowed'`. - - `effectiveGrant.status === 'denied'` & required → `mode:'bounded_context_required'`, - `status:'blocked'`, `recoveryAction:'approve_project_filesystem_context'` - (**deniedRequired** — the handoff gate must HOLD, see S3). - - otherwise not covered → `mode:'bounded_context_required'`; `status:'blocked'` - unless `optional`+`continue_without_mcp` (`status:'warning'`); + - covered by `effectiveGrant` (`phase:'approved'` & not `consumed` & covers the + required capabilities) → `mode:'bounded_context_approved'`, `status:'allowed'` + (subject to the health overlay, step 8). + - `effectiveGrant.phase === 'denied'` & blocking (`!canProceed`) → + `mode:'bounded_context_required'`, `status:'blocked'`, + `recoveryAction:'approve_project_filesystem_context'` (**deniedRequired** — the + handoff gate must HOLD, see S3). + - `effectiveGrant.phase === 'revoked'` & blocking → `mode:'bounded_context_required'`, + `status:'blocked'`, `recoveryAction:'approve_project_filesystem_context'`, and + the `reason` carries `revocationReason` (distinct "context removed" copy, S5). + - otherwise not covered → `mode:'bounded_context_required'`; `status:'warning'` + iff `canProceed`, else `status:'blocked'`; `recoveryAction:'approve_project_filesystem_context'`. 6. **Any capability class `bounded_read_only` with `deliveryKind === - 'planning_context_only'`** (github reads): delivered as planning context. - Health overlay (7) may block; otherwise `mode:'planning_only'`, - `status:'allowed'`, `recoveryAction:'continue_as_prompt_context'`. + 'planning_context_only'`** (github reads): delivered as planning context; it + consumes no MCP runtime, so it is **not** health-gated. Gate on materialization: + - `hasPromptOnlyContext` → `mode:'planning_only'`, `status:'allowed'`, + `recoveryAction:'continue_as_prompt_context'`. + - not materialized & blocking (`!canProceed`) → `mode:'blocked'`, + `status:'blocked'`, `recoveryAction:'revise_plan'` (a required read with neither + a producer nor prompt context is a plan defect — do not admit it). + - not materialized & `canProceed` → `mode:'planning_only'`, `status:'warning'`, + `recoveryAction:'continue_as_prompt_context'`. 7. **All capabilities `planning_only`, OR zero actionable capabilities:** - `required` with zero capabilities and **no** prompt-only context → `mode:'blocked'`, `status:'blocked'`, `recoveryAction:'revise_plan'` (the plan under-specified a required MCP). - otherwise → `mode:'planning_only'`, `status:'warning'`, `recoveryAction:'continue_as_prompt_context'`. -8. **Health overlay** (applied to the mode chosen above when it is `allowed`): - if `!isMcpHealthy(status)` and `required` and not `canProceedWithoutMcp` → +8. **Health overlay** (applies only to modes whose delivery consumes MCP runtime — + `bounded_context_approved` and any future live path — and only when the mode + chosen above is `allowed`; **planning-context and planning-only modes are never + health-gated**): if `!isMcpHealthy(status)` and `!canProceed` → `status:'blocked'`, `recoveryAction:'install_or_fix_mcp'` (retryable). If - `optional`+`continue_without_mcp` → `status:'warning'`. + `canProceed` → `status:'warning'`. `recoveryAction` replaces the fragile `isRetryableMcpBrokerBlock` string-matching: a block is **retryable iff its `recoveryAction === 'install_or_fix_mcp'`**. @@ -291,7 +364,55 @@ MCP-aware subtasks against the normalized approved coverage — preserving the current broker's package-wide prohibition removal and subtask checks (`evaluateWorkPackageMcpBroker`, `mcp-execution-design.ts:617-631,697-721`). +#### Layer 3a — canonical entry join (one evaluation per logical requirement) + +`brokerEntries()` concatenates two representations of the *same* logical +requirement: raw `work_packages.mcpRequirements` (Architect-requested policy: +`requirement`, capability fields, `fallback`, `prohibitedCapabilities`) and derived +`metadata.mcpGrants` (the persisted preview decision: `decisionId`, +`sourceRequirementIndex`, `assignment`, `health`, `promptOverlayPresent`, status). +Admitting both independently would double-count decisions, warnings, and summary +counts, and could not populate the `source` envelope reliably. + +Before admission, `admitWorkPackageMcp` **joins** entries on the stable key +`(sourceRequirementIndex, agent, mcpId)` (falling back to `(agent, mcpId)` when a +raw requirement predates `sourceRequirementIndex`) and merges with fixed +precedence: + +- **requested policy** (`requirement`, merged capability fields via + `mergeCapabilityFields`, `fallback`, `prohibitedCapabilities`) comes from + `mcpRequirements`; +- **source envelope + persisted health** (`decisionId`, `sourceRequirementIndex`, + `assignment`, `promptOverlayPresent`, `health`) comes from `metadata.mcpGrants`; +- if only one side is present, its own fields are used and the missing envelope is + synthesized deterministically (`decisionId = 'req-{index}:{agent}:{mcpId}'`). + +The result is **exactly one `admitMcpRequirement` call per logical +`(sourceRequirementIndex, agent, mcpId)`**, asserted by test. + +**Preview parity of deny-wins scope.** The design-stage adapter +(`deriveMcpGrantDecisions` / `validateMcpExecutionDesign`) must partition the +`McpExecutionDesign` requirements into the **same per-agent package units** that +materialization later persists (`agentsForRequirement`, one package per agent) +*before* computing `packageProhibitedKeys`. Otherwise preview would union +prohibitions across all agents while handoff unions them per package, and the two +would disagree. Each agent-package is admitted independently in both preview and +handoff. + ```ts +// Versioned per-MCP health snapshot. checkedAt is REQUIRED: persisting only an +// ambiguous approval timestamp cannot replay a decision after cached +// ProjectMcpStatus rows change. Sourced verbatim from ProjectMcpStatus. +export type McpHealthSnapshot = { + schemaVersion: 1 + mcpId: string + installState: string + status: string + enabled: boolean + error: string | null + checkedAt: string +} + export type McpAdmissionEvaluation = { decision: McpAdmissionDecision source: { // retained so adapters are shape-preserving @@ -301,7 +422,7 @@ export type McpAdmissionEvaluation = { fallback: { action: McpFallbackAction; message: string } promptOverlayPresent: boolean } - health: { installState: string; status: string; enabled: boolean; error: string | null } + health: McpHealthSnapshot } export type McpWorkPackageAdmission = { @@ -330,12 +451,24 @@ export function admitWorkPackageMcp(input: { }): McpWorkPackageAdmission ``` -`admitWorkPackageMcp`: (1) build `packageProhibitedKeys` = union of -`coverageKeysForProhibition` over every entry; (2) `admitMcpRequirement` per entry -with that set (a capability prohibited anywhere is `deferred_live_mcp`, never -approved); (3) accumulate approved coverage keys minus the prohibition set; -(4) classify each subtask capability and check known MCP + safe scope + covered by -approved coverage, matching the current subtask loop; (5) fold everything into +`admitWorkPackageMcp`: (1) **join** raw entries per Layer 3a into one canonical +entry per `(sourceRequirementIndex, agent, mcpId)`; (2) build +`packageProhibitedKeys` = union of `coverageKeysForProhibition` over every joined +entry; (3) `admitMcpRequirement` per joined entry with that set — a capability +prohibited anywhere is a package-wide policy denial and blocks unconditionally +(deny-wins, decision-table step 3), never approved; (4) accumulate **two** coverage +sets, both minus the prohibition set: + - `boundedCoverageKeys` — canonical capability keys from decisions admitted + `bounded_context_approved` (filesystem bounded packet); + - `planningContextCoverageKeys` — canonical `(agent, capability)` keys from + decisions admitted as planning context (`planning_only` / + `planning_context_only`, e.g. a materialized `github.*.read` requirement); +(5) classify each subtask capability and admit it as covered when: unknown MCP or +class `unknown`/`deferred_live_mcp` → blocked; a `bounded_context_packet` read → +must be in `boundedCoverageKeys`; a `planning_context_only` read → must be in +`planningContextCoverageKeys` **for the same agent** (so a safe `github.*.read` +subtask is accepted when its matching requirement was admitted as planning context, +instead of being rejected for lacking a bounded grant); (6) fold everything into `aggregate`. This is the canonical evaluation consumed by preview, approval, and handoff. @@ -350,7 +483,15 @@ and existing readers do not change shape — they are only *extended*: - `admissionToGrantPreview(admission): McpGrantDecisions` — keeps `decisionId`, `sourceRequirementIndex`, assignment, fallback, raw `health`, `promptOverlayPresent`; **adds** `mode`, `recoveryAction`, - `normalizedCapabilities`, `capabilityClasses`, `evidenceRefs`. + `normalizedCapabilities`, `capabilityClasses`, `evidenceRefs`, and a canonical + `admissionStatus: McpAdmissionStatus` (`allowed | warning | blocked`). + **Status compatibility.** The legacy `McpGrantDecisions.status` + (`proposed | warning | blocked`) is preserved by the total map + `allowed → proposed`, `warning → warning`, `blocked → blocked`, so existing + readers and persisted JSON keep their shape. Literal cross-surface parity is + asserted on the canonical pair (`mode`, `admissionStatus`) — **not** on the legacy + `status` string, which stays `proposed`-spelled for the preview surface only. + `summary` counts are computed from the legacy `status` exactly as today. - `admissionToBrokerCheck(admission): WorkPackageMcpBrokerCheck` — plus `retryable`, `primaryRecoveryAction`. @@ -375,7 +516,7 @@ keep a **transitional re-export** until their callers migrate in the owning slic | `evaluateWorkPackageMcpBroker` (`:599`) + helpers `capabilityArray:498`, `approvedCoverageCapabilityKeys:551`, `isPlanningOnlyFilesystemWrite:318` | calls `admitWorkPackageMcp` with `mergeCapabilityFields`; returns `admissionToBrokerCheck`; local normalization/allowlist deleted, imported from shared modules | S2 | | `SAFE_BETA_CAPABILITY_PATTERNS` (`:7-18`) | migrated verbatim into `SAFE_READ_SUPPLEMENT`; **kept as a transitional re-export in S1**, deleted in S2 after callers move | S1 create / S2 delete | | `requiresFilesystemGrantApproval` / `summarizeFilesystemCapabilities` (`filesystem-grants.ts:303/90`) | filesystem-specific **projection** of `admitWorkPackageMcp` (filesystem decisions only); imports normalization; keeps `FilesystemProjectCapability` nominal type + `ProjectFilesystemGrant` persistence + `EffectiveGrantState` reader | S3 | -| `isRetryableMcpBrokerBlock` (`:90`) + `buildMcpBrokerBlockMetadata` (`blocked-handoff-retry.ts:32`) | consume `aggregate.retryable`/`primaryRecoveryAction`; persist `mode`+`recoveryAction` under `metadata.mcpBroker` | S2 (broker), S5 (persist for UI) | +| `isRetryableMcpBrokerBlock` (`:90`) + `buildMcpBrokerBlockMetadata` (`blocked-handoff-retry.ts:32`) | consume `aggregate.retryable`/`primaryRecoveryAction`; **persist the full versioned `metadata.mcpBroker` here**; S5 reads it only | S2 | | `mcpGrantsForAgent` (`workforce-materializer.ts:157`) | persists `mode`, `recoveryAction`, `normalizedCapabilities`, `evidenceRefs` on each grant (schema bump), not just id/mcp/caps/requirement/status/reason/fallback/health | S2 | | `mcpCapabilityList` (`work-package-executor.ts:1527`) | imports `mergeCapabilityFields`; executor filesystem gating uses shared `coverageKeysForGrant`/`classifyCapability` | S4 | | client helpers (`tasks/[id]/page.tsx:348-444`) | import shared helpers OR consume a server-computed grant-state payload; render `mode`+`recoveryAction` via `admission-copy.ts` | S5 | @@ -408,24 +549,48 @@ run-evidence schema S4 defines) and on S2. S6 depends on S2–S5. that flips the task to `approved`). Run `admitWorkPackageMcp` over every package using that snapshot; if any `aggregate.status === 'blocked'`, return the same 409 shape as the existing `missingFilesystemGrant` early return (`:199-209,280`) - with the normalized `reason` + `primaryRecoveryAction`. **Persist the - `checkedAt` health snapshot** the approval decision used on the plan-approval - gate metadata (next to the existing `approvedGrantSnapshot`, `:221-263`). + with the normalized `reason` + `primaryRecoveryAction`. **Persist the exact + health snapshot** the approval decision consumed — a versioned + `approvalHealthSnapshot: McpHealthSnapshot[]` (each `{schemaVersion, mcpId, + installState, status, enabled, error, checkedAt}`) on the plan-approval gate + metadata, next to the existing `approvedGrantSnapshot` (`:221-263`). A single + ambiguous timestamp cannot replay the decision after cached `ProjectMcpStatus` + rows change; the per-MCP `checkedAt` is required. Add an assertion that the + persisted snapshot equals the health inputs passed to `admitWorkPackageMcp`. +- **Broker-block persistence is owned entirely by S2.** The complete, versioned + `metadata.mcpBroker` producer lives here: `buildMcpBrokerBlockMetadata` + (`blocked-handoff-retry.ts:32`) persists `{schemaVersion, status:'blocked', + blocked, warnings, blockedReason, mode, recoveryAction, retryable, + primaryRecoveryAction, autoRetryAttempts, nextAutoRetryAt}` from `aggregate` + (`retryable = aggregate.retryable`, `recoveryAction = + aggregate.primaryRecoveryAction`). This makes retry/recovery + (`shouldAutoRetryBlockedHandoff` and the sweep) fully deployable **before** the UI + slice; S5 only *reads* `metadata.mcpBroker`. - **Parity guarantee (narrowed).** For a fixed package and a *fixed health snapshot*, `deriveMcpGrantDecisions` (preview), the approval check, and - `evaluateWorkPackageMcpBroker` (handoff) return the same `mode`/`status` because - they call one producer. MCP health/config can change between approval and + `evaluateWorkPackageMcpBroker` (handoff) return the same `mode`/`admissionStatus` + (the canonical pair; the preview's legacy `status` maps `allowed → proposed`) + because they call one producer. MCP health/config can change between approval and handoff, so the promise is: **a block already visible in the approval-time snapshot is surfaced at approval, not missed until handoff** — not that an approved task can never block later. - **Invariant tests** (`web/__tests__/mcp-admission-invariant.test.ts`): preview, - approval, and handoff agree for required no-capability grants, prompt-only - context, filesystem read/list/search, `filesystem.project.write`, an - unsafe/deferred GitHub write, a healthy GitHub read (planning-context, not - bounded), an unknown MCP, a package-wide prohibition that must beat a - per-entry approval, an MCP-aware subtask capability, and a requirement expressed - via each of the four capability fields. `requiresFilesystemGrantApproval` is - tested only as the *filesystem projection* of the same evaluation. + approval, and handoff agree on the canonical (`mode`, `admissionStatus`) pair for + required no-capability grants, prompt-only context, filesystem read/list/search, + `filesystem.project.write`, an unsafe/deferred GitHub write, a **package-wide + prohibition on an `optional`+`continue_without_mcp` requirement (must stay + `blocked`, never downgraded to a warning)**, a healthy GitHub read + (planning-context, not bounded), a **required GitHub read with no materialized + prompt context (blocks `revise_plan`)**, an unknown MCP, a **known-MCP typo + capability (blocks `revise_plan`)**, a package-wide prohibition that must beat a + per-entry approval, MCP-aware subtasks (both a bounded filesystem subtask and a + planning-context `github.*.read` subtask), a requirement expressed via each of the + four capability fields, and **all three fallback actions (`continue_without_mcp`, + `ask_user`, `block`) across deferred, unhealthy bounded-context, and missing + bounded-context cases** (asserting `ask_user`/`block` never become non-blocking). + Also assert **one `admitMcpRequirement` call per `(sourceRequirementIndex, agent, + mcpId)`** after the Layer 3a join. `requiresFilesystemGrantApproval` is tested + only as the *filesystem projection* of the same evaluation. ### S3 — Filesystem grant recovery (deterministic, recoverable) @@ -455,6 +620,13 @@ run-evidence schema S4 defines) and on S2. S6 depends on S2–S5. supersedes an earlier package-local `denied` effective phase for that capability (operator's later, broader decision wins). Document and test this precedence in `readEffectiveGrantState`. +- **Revoked project grant is distinct from never-approved.** When + `readEffectiveGrantState` returns `phase:'revoked'` (a project grant that + previously covered the package no longer does), the held-block `reason` and the S5 + copy say "project filesystem context was removed — approve it again", carrying + `revocationReason`, separate from the first-time "needs project context" copy. + Test the two recovery messages separately; a revoked grant must not read as a + brand-new request. - **Acceptance tests assert exact transitions** (package `pending/ready → blocked`, task not `failed`, zero new `agentRuns`; then grant approval → package `ready`, task re-driven), not just attempt counts. @@ -512,14 +684,18 @@ run-evidence schema S4 defines) and on S2. S6 depends on S2–S5. packet metadata inline (from S4's schema), and add remediation CTAs + bounded-context note on the projects page and the MCPs catalog page (`mcps/page.tsx`). -- **Persisted schema is versioned.** The worker persists `mode`+`recoveryAction` - on BOTH the `grantDecisions` preview decisions AND the per-package block metadata - (`metadata.mcpBroker`, a JSON object — `work_packages.blocked_reason` stays the - human text). When multiple decisions block, `aggregate.retryable` is true iff - every blocking decision is `install_or_fix_mcp`, and - `primaryRecoveryAction` is the highest-precedence block - (`revise_plan > approve_project_filesystem_context > install_or_fix_mcp > - defer_live_mcp_feature`), which drives the single UI CTA. +- **Reads the versioned persisted schema (produced by S2) — S5 persists nothing.** + S5 does not compute or persist admission state. It reads the `grantDecisions` + preview (`mode`, `recoveryAction`, `admissionStatus`, `normalizedCapabilities`, + `capabilityClasses`, `evidenceRefs`) and the per-package `metadata.mcpBroker` + block (`{mode, recoveryAction, retryable, primaryRecoveryAction, blocked, + warnings}`; `work_packages.blocked_reason` stays the human text) that S2 wrote, + and renders them. `RetryHandoffControls` is gated on the persisted + `metadata.mcpBroker.retryable`; the single CTA is driven by + `primaryRecoveryAction` (precedence `revise_plan > + approve_project_filesystem_context > install_or_fix_mcp > defer_live_mcp_feature`). + Because the producer/persistence contract lives in S2, retry/recovery is + deployable before this UI slice ships. ### S6 — End-to-end regression