diff --git a/.changeset/patient-tunas-howl.md b/.changeset/patient-tunas-howl.md new file mode 100644 index 000000000..46a26f0f4 --- /dev/null +++ b/.changeset/patient-tunas-howl.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4693 +--- +**Executor dispatches are no longer refused when a phase correctly degrades to sequential execution.** The isolation guards identified a dispatch by regex-scraping model-authored prose, which returned identifiers in a different namespace from the ones the run-scoped sentinel records — so a fresh decision was discarded on every executor dispatch and every legitimate `ISOLATION=none` degrade was denied, leaving the work unrun. Dispatch identity now has one owner for both the emitted format and the parser that reads it back. (#4594) diff --git a/docs/adr/4630-dispatch-identity-and-isolation-decision-seam.md b/docs/adr/4630-dispatch-identity-and-isolation-decision-seam.md new file mode 100644 index 000000000..0a5295f8f --- /dev/null +++ b/docs/adr/4630-dispatch-identity-and-isolation-decision-seam.md @@ -0,0 +1,211 @@ +# ADR-4630: One Canonical Dispatch-Identity Owner and a Recorded Isolation Decision + +- **Status:** Accepted +- **Date:** 2026-09-13 +- **Issue:** [#4630](https://github.com/open-gsd/gsd-core/issues/4630) — epic (`type: chore`, `area: agents`) +- **Absorbs:** [#4222](https://github.com/open-gsd/gsd-core/issues/4222), [#4561](https://github.com/open-gsd/gsd-core/issues/4561), [#4594](https://github.com/open-gsd/gsd-core/issues/4594) — three `confirmed-bug` issues that are the same missing owner reported at two ends of one wire +- **Framed by:** [ADR-1239](1239-gsd-embeddable-orchestration-engine.md) (host-integration interface — agent dispatch is interface point 2) +- **Pattern copied from:** [ADR-2121](2121-phase-identifier-parsing-consolidation.md) (phase-identifier consolidation: seam + migration + anti-divergence guard). This ADR reuses its phase-token grammar and its drift-lint shape deliberately. +- **Scope note:** unlike ADR-2121, this ADR does **not** land as a code-free Phase 0. It ships alongside Phase 1's implementation because the epic's phase sub-issues could not be created in the session that executed it; the decisions below were nonetheless fixed before that implementation was written, and Phases 2 and 3 execute against this file. + +## Context + +A GSD phase execution decides, per dispatch, whether the executor runs isolated in a git +worktree or sequentially in the primary checkout. That decision is: + +1. **produced** in workflow shell (`execute-phase/steps/executor-isolation-dispatch.md`, `per-plan-worktree-gate.md`, `references/dispatch-isolation-gate.md`, `execute-plan.md`), +2. **transported** through a run-scoped sentinel at `.gsd/dispatch-isolation-sentinel.json`, +3. **consumed** by two guard hooks (`hooks/gsd-agent-isolation-guard.js` for Claude-shaped hosts, `hooks/gsd-cursor-subagent-start.js` for Cursor). + +At every hop, both sides hand-roll their own copy of the format. #4561 states the census +directly: the dispatch-isolation membership is **hand-written at eight uncross-checked +sites**. The consequence is not a style complaint — it is three confirmed bugs: + +| Issue | Defect | +|---|---| +| #4222 | A plain `dispatch-isolation` re-query re-resolves the host *capability* and re-persists it, clobbering every shell-computed degrade but the #3737 opt-out. The guard then denies the mandated sequential dispatch with `exit 2`. | +| #4561 | #4232's re-derivation fix reaches only producers whose degrade is re-derivable from a file the resolver reads. Three producers compute their degrade from call-site context that is **not on disk** and are structurally unreachable that way. | +| #4594 | `extractDispatchIdentifiers` cannot parse the canonical prompt-body dispatch text, so the sentinel match never succeeds. | + +### What #4594 actually is, measured + +The reported symptom is a greedy capture: `/execute\s+plan\s+(\S+)\s+of\s+phase\s+(\S+)/i` +swallows the `-{phase_name}` suffix and the sentence-terminating period, so +`Execute plan 02 of phase 03-auth.` yields phase `03-auth.` and can never equal the +sentinel's `03`. + +That is true and incomplete. Running `phase-plan-index` against a real fixture shows +`plans[].id` is `03-02-hardening` — phase-prefixed, plan-numbered **and slugged** — and +`per-plan-worktree-gate.md:17` records exactly that value as the sentinel's `plan`. The +prose, meanwhile, carries a bare in-phase plan number (`02`). The two fields therefore live +in **different namespaces**, and the plan comparison mismatches on *every* dispatch, on +*both* hosts. The issue calls the Claude path "latent rather than dead"; it is dead too, for +a second reason the issue does not name. Fixing only the greedy capture would have shipped +as a green, fully-tested no-op. + +### Why this recurs rather than staying fixed + +Repairing each symptom where it was reported leaves the missing owner missing, and the next +occurrence is already being written elsewhere in the tree. The recurrence has a mechanical +root, and it is the same one ADR-2121 identified for phase identifiers: **a format with no +single owner is re-derived at each site, and two copies that agree today are the same defect +as two that disagree.** + +## Decision + +### Decision 1 — one module owns the emitted format *and* the parser that reads it back + +`hooks/lib/dispatch-identity.js` owns both halves together: + +- `renderDispatchIdentityMarker({phase, plan})` — the format producers emit. +- `parseDispatchIdentity(...texts)` — the pattern consumers read back. + +Producers emit a structured marker carrying the **same shell values the sentinel records**: + +```text +[gsd:dispatch phase="03" plan="03-02-hardening"] +``` + +`$PHASE_NUMBER` and `$plan_id` are already in scope at every producer site. Producer and +consumer then agree *by construction*, independently of how the surrounding prose reads or +whether a model paraphrases it. + +The prose frame remains as a fallback, with the greedy `(\S+)` replaced by the phase-token +grammar ADR-2121 already owns, and with `plan` deliberately **not** reported — the prose +plan token is a different namespace from `plan_id`, and reporting it is the bug. The +fallback is therefore **correct-or-absent** in both fields: an absent identifier means +"cannot compare" and is safe by the existing contract; a wrong one is a false mismatch and +is not. + +**The owner lives in `hooks/lib/`, not `src/`.** The guard hooks must load on a raw +plugin-marketplace install where the compiled `gsd-core/bin/lib/` is absent and the +self-healing build seam has not run — a hook that dies at module load is worse than one +carrying a mirror. A `src/*.cts` owner would force either a top-level require of the +compiled lib (breaking that install) or a second mirrored copy (the exact defect this epic +removes). Nothing in `src/**` needs this format: the producers are workflow *markdown* and +the consumers are *hooks*. + +### Decision 2 — the isolation decision is a recorded fact, not a re-derived one + +A producer writes an `IsolationDecision` (the value, which producer decided it, and its run +scope); consumers **read** it. A plain re-query must not overwrite a recorded decision. + +Re-derivation is the wrong primitive for the three producers #4561 names, because each +computes its degrade from call-site context that does not exist on disk: + +| Producer | Why re-derivation cannot reach it | +|---|---| +| `gsd-core/references/dispatch-isolation-gate.md` | the `orchestrator-worktree` fallback fires only at a subagent-tool-only dispatch site; the resolver cannot see which tool the caller has | +| `gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md` | the #2474 per-plan submodule intersection is a property of the plan set, not the repo | +| `gsd-core/workflows/execute-plan.md` | Pattern B's unisolated segments are by design; which segment is executing is not on disk | + +**A decision that cannot be recomputed must be recorded and honoured, not recomputed and +overwritten.** #4232's base-check re-derivation stays — it is correct for the producers it +covers, and this decision builds on it rather than replacing it. + +### Decision 3 — a copy that must exist is pinned by a parity test, never merely tolerated + +Decision 1's cold-load constraint means `hooks/lib/` carries deliberate mirrors of two +things it cannot import: the phase-token grammar (from `src/phase-id.cts`) and the isolation +vocabulary (from the Phase 2 owner). Each mirror is **pinned by a parity assertion that reds +when the owner changes**. + +This matters more than it looks. A drifted grammar mirror fails as a **non-match** — nothing +throws, no test that only feeds the old shape notices, and the symptom is a guard that +quietly stops recognizing valid input. That is the failure mode that produced #4594 and it +is invisible without a pin. + +A mirror's justification is part of the contract: **do not "clean up" a pinned mirror +without replacing the reason it exists.** + +### Decision 4 — the anti-divergence lint is part of the deliverable, not a follow-up + +A drift lint asserts that (a) every dispatch-identifier template in +`gsd-core/workflows/**/*.md` and every parsing regex in `hooks/**` derives from the single +exported token source, and (b) every dispatch-isolation producer site appears in the +canonical membership list. It is modelled on `scripts/lint-phase-id-drift.cjs`. + +Two lessons from #2121 are binding: + +- Compare the regex derived from the **source string** (`.source` / `.flags` against the + canonical token), not the rendered literal. +- A sanctioned exemption is a **dedicated comment line** that states which half is + deliberate — never a line-level substring escape. Partial ownership is drift, and a + line-level escape waves through exactly the case under review. + +Decisions 1 and 2 without Decision 4 un-consolidate quietly as soon as someone who has not +read this ADR touches the area. **The lint must be demonstrated red before it is +demonstrated green** — a drift guard that has never failed is not evidence. + +### Decision 5 — an inapplicable sentinel is reported, never silently discarded + +When a fresh sentinel exists but does not apply to a dispatch, the guard states so in its +deny reason and names both sides' identifiers. + +This is Postel's Law's "be liberal but *visible*" half, and its absence is why the defect +survived three producers and two consumers unnoticed: the guard discarded a fresh sentinel +and then denied with a message about registry resolution that never mentioned the sentinel +existed. Values interpolated into that message come from a sentinel file and from +model-authored prompt text — both untrusted — so each is length-bounded and stripped of +control characters first, matching the escaping discipline `phase-plan-index` already +applies to a `depends_on` token. + +## Alternatives rejected + +1. **Repair the three absorbed issues at their existing call sites.** This is the pattern + that produced them. It leaves the epic open with every symptom gone and CI green — the + specific outcome the epic's scoping exists to prevent. +2. **Normalize the plan comparison** — strip the phase prefix and slug and compare the + middle segment. Invents a cross-namespace equivalence no producer guarantees, and + silently restores wrong-value-instead-of-no-value the moment plan-id naming changes. + Returning `null` is honest; a heuristic match is not. +3. **Carry a structured per-dispatch kwarg through the Agent/Task protocol.** This is the + correct end state and `hooks/lib/isolation-sentinel.js` has named it as such since #3045. + Rejected *for now* under Gall's Law: it is a 19-runtime descriptor change and an + ADR-1239-level negotiation change. The marker is the simple version that works, and + growing one into the other is the sanctioned path. +4. **Own the format in `src/dispatch-identity.cts`.** Breaks the cold-tree hook load + contract, or forces the second mirror this epic exists to delete. +5. **Delete `sentinelAppliesToDispatch`'s plan/phase narrowing**, on the grounds that it has + never worked so removing it changes nothing observable. It is #3045 SECURITY F2's defence + against a stale sentinel from an earlier phase authorizing a later dispatch. The marker + makes it work for the first time; deleting it would trade one silent hole for another. + +## Consequences + +**Accepted cost.** `[gsd:dispatch k="v"]` is a homegrown key/value mini-syntax, and +Greenspun's Tenth Rule is the standing warning against letting one grow. It is therefore +deliberately inert: exactly two recognized keys, flat, no nesting, no conditionals, no +variables, no escaping rules beyond quoted values, and unrecognized keys ignored so a later +addition cannot break a deployed parser. **Revisit condition, stated so it is actionable: +the moment this marker needs a third semantic key or any structure, stop and implement +alternative 3 instead.** + +**Accepted limit.** The marker is emitted by a model following a template on BOTH paths — +the harness path AND the orchestrator-worktree path. Per +`executor-isolation-dispatch.md:131`, the orchestrator-worktree path's `{plan_number}` and +`{phase_number}` "are template placeholders, not shell variables," and `{plan_id}` is +model-substituted there exactly as on the harness path — there is no shell-built prompt on +either path, so the marker is NOT guaranteed on either. A model that drops it degrades to +the prose fallback everywhere. That fallback is now correct-or-absent, which is sufficient +to resolve #4594 on the phase field alone, but plan-level scoping is exact only when the +marker survives. The prose fallback is therefore the real floor on both paths. + +**Hyrum's Law constraint.** The prose sentence `Execute plan X of phase Y` is read by the +executor agent itself, not only by the guards. It stays **byte-identical**; the marker is +purely additive. Any future change to that sentence is a behavior change to every executor +dispatch, not a formatting edit. + +**Out of scope.** The sentinel remains a `cwd`-derived, gitignored file; #3045 SECURITY F3's +accepted forgery risk is unchanged. This ADR changes how the decision is transported, not +when a degrade should occur. + +## Phase mapping + +| Phase | Delivers | Decisions | +|---|---|---| +| 1 | `hooks/lib/dispatch-identity.js`; producers emit the marker; both guards parse through the owner; #4594 regression | 1, 3 (grammar mirror), 5 | +| 2 | Isolation-vocabulary owner; recorded `IsolationDecision` that a re-query holds; #4222 / #4561 regressions | 2, 3 (vocabulary mirror) | +| 3 | `scripts/lint-dispatch-identity-drift.cjs`, demonstrated red then green | 4 | + +Each deliverable is claimed by exactly one phase. The epic stays open until Phase 3 lands. diff --git a/docs/adr/README.md b/docs/adr/README.md index d47772a62..82073d32e 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -269,6 +269,7 @@ These govern the system as it stands. Cite these. | [ADR-3806](3806-review-dispositions-ledger.md) | Review Dispositions Ledger canonizes where and how reviews-mode records incorporate/defer decisions in PLAN.md | Accepted | — | | [ADR-4139](4139-compact-content-seam.md) | The compact-content seam — shrink the eager window, never the guarantee | Accepted | — | | [ADR-4593](4593-macos-conformance-tier-architecture.md) | A macOS-specific conformance-tier classifier, separate from the Windows-oriented one | Accepted | — | +| [ADR-4630](4630-dispatch-identity-and-isolation-decision-seam.md) | One Canonical Dispatch-Identity Owner and a Recorded Isolation Decision | Accepted | — | | [ADR-4641](4641-windows-selector-consolidation.md) | One Windows test selector, and a proportional ceiling on the conformance tier | Accepted | — | ### Proposed diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index 5cd28d9fa..eb9464bd8 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -591,6 +591,8 @@ increases monotonically across waves. `{status}` is `complete` (success), Pass paths only — executors read files themselves. + **Substitute `{plan_id}` in the prompt below with this plan's `id` field** from the `phase-plan-index` JSON loaded in step 1 (the same field referred to elsewhere in this workflow as `plan.id`) — unmodified and un-truncated, never a paraphrase. The guard hooks compare this value verbatim against the sentinel the per-plan gate wrote (`per-plan-worktree-gate.md`'s `plan_id`); a paraphrase or an omission costs the dispatch its recorded isolation decision. + **Executor routing (#1689/#3370).** Per plan, run `gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md` to set `EXECUTOR_TYPE` for `subagent_type="{EXECUTOR_TYPE}"` below. **TDD-applicability resolution (#4266/#4272).** Run `gsd-core/workflows/execute-phase/steps/tdd-applicability-resolution.md`. @@ -641,6 +643,7 @@ increases monotonically across waves. `{status}` is `complete` (success), prompt=" Execute plan {plan_number} of phase {phase_number}-{phase_name}. + [gsd:dispatch phase="{phase_number}" plan="{plan_id}"] Commit each task atomically. Create SUMMARY.md. Do NOT update STATE.md or ROADMAP.md — the orchestrator owns those writes after all worktree agents in the wave complete. diff --git a/gsd-core/workflows/execute-phase/steps/executor-isolation-dispatch.md b/gsd-core/workflows/execute-phase/steps/executor-isolation-dispatch.md index 8d8315fb2..2c7eaa8c5 100644 --- a/gsd-core/workflows/execute-phase/steps/executor-isolation-dispatch.md +++ b/gsd-core/workflows/execute-phase/steps/executor-isolation-dispatch.md @@ -149,8 +149,12 @@ Assign the composed prompt to a shell variable so it can be passed as one argume # An unreadable source file is a halt condition (#3637 fail-closed), # never a skip — a child without these texts is not a gsd-executor. # 2. Substitute this plan's {plan_number}, {phase_number}, {phase_name}, -# {phase_dir}, and {plan_file} placeholders (same values the harness -# path substitutes into its Agent() prompt). +# {phase_dir}, {plan_file}, and {plan_id} placeholders (same values the +# harness path substitutes into its Agent() prompt). {plan_id} is this +# plan's `id` field from the phase-plan-index JSON — the guard hooks +# compare it verbatim against the sentinel the per-plan gate wrote, so a +# paraphrase or omission costs the dispatch its recorded isolation +# decision. # 3. Inline the gsd-executor ROLE DEFINITION: read `agents/gsd-executor.md` # (resolved against the install root the same way the harness runtime # resolves subagent types) and inline it verbatim at the provenance @@ -175,6 +179,7 @@ TDD_APPLICABLE="$_TDD_APPLICABLE_RAW" EXECUTOR_PROMPT=' Execute plan {plan_number} of phase {phase_number}-{phase_name}. +[gsd:dispatch phase="{phase_number}" plan="{plan_id}"] Commit each task atomically. Create SUMMARY.md. Do NOT update STATE.md or ROADMAP.md — the orchestrator owns those writes after all worktree agents in the wave complete. diff --git a/hooks/gsd-agent-isolation-guard.js b/hooks/gsd-agent-isolation-guard.js index 9a9ef31dd..92d4659ff 100644 --- a/hooks/gsd-agent-isolation-guard.js +++ b/hooks/gsd-agent-isolation-guard.js @@ -63,8 +63,8 @@ const fs = require('fs'); const path = require('path'); const os = require('os'); -const { readSentinel, VALID_ISOLATION, extractDispatchIdentifiers, sentinelAppliesToDispatch } = require('./lib/isolation-sentinel.js'); -const { REASON_CODE } = require('./lib/isolation-deny-reason.js'); +const { readSentinel, VALID_ISOLATION, extractDispatchIdentifiers, sentinelAppliesToDispatch, buildSentinelDiscard } = require('./lib/isolation-sentinel.js'); +const { REASON_CODE, describeSentinelDiscard } = require('./lib/isolation-deny-reason.js'); const { HOOK_ON_CRASH, allow, deny, crash } = require('./lib/hook-exit.js'); // Required at module top, alongside the other ./lib requires — NOT behind @@ -385,16 +385,20 @@ function resolveIsolationState(cwd, { clock = Date, dispatchIds = null } = {}) { projectExists = false; } if (!projectExists) { - return { gsdProject: false, isolation: null, harnessFlag: null, error: null }; + return { gsdProject: false, isolation: null, harnessFlag: null, error: null, sentinelDiscarded: null }; } const sentinel = readSentinel(cwd, { clock }); - if (sentinel.present && !sentinel.stale && sentinelAppliesToDispatch(sentinel, dispatchIds)) { + // Hoisted so the "sentinel was present/fresh but did not apply" case below + // (#4594 row 15 — Postel's-Law finding) can distinguish itself from + // "absent"/"stale" without re-deriving applicability. + const applies = sentinelAppliesToDispatch(sentinel, dispatchIds); + if (sentinel.present && !sentinel.stale && applies) { if (sentinel.isolation !== 'harness-worktree') { - return { gsdProject: true, isolation: sentinel.isolation, harnessFlag: null, error: null }; + return { gsdProject: true, isolation: sentinel.isolation, harnessFlag: null, error: null, sentinelDiscarded: null }; } if (sentinel.harnessFlag) { - return { gsdProject: true, isolation: 'harness-worktree', harnessFlag: sentinel.harnessFlag, error: null }; + return { gsdProject: true, isolation: 'harness-worktree', harnessFlag: sentinel.harnessFlag, error: null, sentinelDiscarded: null }; } // #3045 BLOCKER 2 fix: the sentinel already PROVED this dispatch requires // isolation (it resolved harness-worktree) but carries no usable flag — @@ -423,6 +427,7 @@ function resolveIsolationState(cwd, { clock = Date, dispatchIds = null } = {}) { 'dispatch-isolation sentinel resolved "harness-worktree" but recorded no harness_flag — ' + 'cannot verify what parameter the dispatch must carry.' ), + sentinelDiscarded: null, }; } @@ -430,11 +435,19 @@ function resolveIsolationState(cwd, { clock = Date, dispatchIds = null } = {}) { // conservative fallback (#3045 finding — must still cover fail-closed case // (a): a project that opted out of worktrees entirely via // workflow.use_worktrees). + // + // #4594 row 15: a PRESENT, FRESH sentinel that simply does not apply to + // THIS dispatch (identifiers disagree) is a distinct case from "absent" or + // "stale" — record what was discarded so evaluateDispatch can name it in a + // block reason instead of silently falling through to a registry-resolution + // message that never mentions the sentinel existed. + const sentinelDiscarded = buildSentinelDiscard(sentinel, dispatchIds); + try { const { isolation, harnessFlag } = resolveRegistryIsolation(cwd, configPath); - return { gsdProject: true, isolation, harnessFlag, error: null }; + return { gsdProject: true, isolation, harnessFlag, error: null, sentinelDiscarded }; } catch (err) { - return { gsdProject: true, isolation: null, harnessFlag: null, error: err }; + return { gsdProject: true, isolation: null, harnessFlag: null, error: err, sentinelDiscarded }; } } @@ -464,10 +477,16 @@ function evaluateDispatch(data, { clock = Date } = {}) { } const cwd = data.cwd || process.cwd(); - // #3045 SECURITY F2: best-effort plan/phase extraction from this - // dispatch's own description text, so a fresh sentinel that disagrees - // with THIS dispatch is treated as inapplicable rather than trusted. - const dispatchIds = extractDispatchIdentifiers(toolInput.description); + // #3045 SECURITY F2 / #4594: best-effort plan/phase extraction from this + // dispatch's own text, so a fresh sentinel that disagrees with THIS + // dispatch is treated as inapplicable rather than trusted. PROMPT FIRST: + // `description` is short, model-authored free text that only carries usable + // identity when the model happens to reproduce the dispatch template + // verbatim, while the canonical `[gsd:dispatch phase="…" plan="…"]` marker + // (or, failing that, the prose fallback) lives in the prompt body itself — + // `description` is kept only as a fallback for a marker/prose match that + // exists solely in it. + const dispatchIds = extractDispatchIdentifiers(toolInput.prompt, toolInput.description); const state = resolveIsolationState(cwd, { clock, dispatchIds }); if (!state.gsdProject) return { action: 'allow' }; @@ -493,7 +512,8 @@ function evaluateDispatch(data, { clock = Date } = {}) { `required — a guard that cannot verify must not answer "safe" (#3050). Retry once the ` + `project configuration is readable.`; const reasonCode = isBuildFailure ? REASON_CODE.RUNTIME_BUILD_FAILED : REASON_CODE.CONFIG_UNREADABLE; - return { action: 'block', reason, reasonCode }; + const fullReason = state.sentinelDiscarded ? reason + describeSentinelDiscard(state.sentinelDiscarded) : reason; + return { action: 'block', reason: fullReason, reasonCode, sentinelDiscarded: state.sentinelDiscarded }; } if (state.isolation !== 'harness-worktree') return { action: 'allow' }; @@ -503,13 +523,14 @@ function evaluateDispatch(data, { clock = Date } = {}) { if (toolInput[parsed.param] === parsed.value) return { action: 'allow' }; - const reason = + let reason = `Agent isolation guard: this project's dispatch isolation resolves to "harness-worktree", ` + `but the Agent() dispatch for subagent_type="${subagentType}" is missing ` + `${parsed.param}="${parsed.value}". Add ${parsed.param}="${parsed.value}" to the Agent() ` + `call so the executor runs in an isolated worktree instead of the primary checkout ` + `(gsd-core/workflows/execute-phase/steps/executor-isolation-dispatch.md).`; - return { action: 'block', reason, reasonCode: REASON_CODE.HARNESS_FLAG_MISSING }; + if (state.sentinelDiscarded) reason += describeSentinelDiscard(state.sentinelDiscarded); + return { action: 'block', reason, reasonCode: REASON_CODE.HARNESS_FLAG_MISSING, sentinelDiscarded: state.sentinelDiscarded }; } /* istanbul ignore next -- stdin adapter, exercised via spawnSync in tests */ @@ -524,7 +545,12 @@ function main() { const data = JSON.parse(input); const decision = evaluateDispatch(data); if (decision.action === 'block') { - const out = { decision: 'block', reason: decision.reason, reason_code: decision.reasonCode }; + const out = { + decision: 'block', + reason: decision.reason, + reason_code: decision.reasonCode, + sentinel_discarded: decision.sentinelDiscarded ?? null, + }; // Kimi feeds stderr (not stdout) back to the model on exit 2. deny(out, decision.reason); } diff --git a/hooks/gsd-cursor-subagent-start.js b/hooks/gsd-cursor-subagent-start.js index 846486276..1ac255002 100644 --- a/hooks/gsd-cursor-subagent-start.js +++ b/hooks/gsd-cursor-subagent-start.js @@ -62,8 +62,8 @@ const { allow } = require('./lib/hook-exit.js'); // hooks/lib/cursor-workspace.js. Staged next to these scripts by // writeCursorHooksJson so the require always resolves post-install. const { resolveStatePath } = require('./lib/cursor-workspace.js'); -const { readSentinel, VALID_ISOLATION, extractDispatchIdentifiers, sentinelAppliesToDispatch } = require('./lib/isolation-sentinel.js'); -const { REASON_CODE } = require('./lib/isolation-deny-reason.js'); +const { readSentinel, VALID_ISOLATION, extractDispatchIdentifiers, sentinelAppliesToDispatch, buildSentinelDiscard } = require('./lib/isolation-sentinel.js'); +const { REASON_CODE, describeSentinelDiscard } = require('./lib/isolation-deny-reason.js'); // #3582: gsd-core/bin/lib/*.cjs (runtime-homes.cjs, worktree-safety.cjs, // runtime-name-policy.cjs, capability-registry.cjs — required below, inside // resolveIsolationEvidence and resolveFallbackIsolation) are tsc build @@ -491,18 +491,25 @@ function evaluateRootIsolation(root, subagentType, { clock = Date, dispatchIds = `Refusing to allow this subagent to spawn until the runtime library is built — a guard ` + `that cannot verify must not answer "safe" (#3050).`, reasonCode: REASON_CODE.RUNTIME_BUILD_FAILED, + sentinelDiscarded: null, }; } + // #3045 BLOCKER fix: a fresh sentinel is authoritative for THIS dispatch's + // actual resolved isolation — see the doc comment above. + // #3045 SECURITY F2: a fresh sentinel that names a DIFFERENT plan/phase + // than this dispatch is not applicable to it — fall through to the + // conservative fallback exactly as a stale sentinel would. + // Hoisted (readSentinel never throws) so the "present, fresh, but did not + // apply" case (#4594 row 15) can be reported on every deny path below + // instead of silently discarded. + const sentinel = readSentinel(root, { clock }); + const applies = sentinelAppliesToDispatch(sentinel, dispatchIds); + const sentinelDiscarded = buildSentinelDiscard(sentinel, dispatchIds); + let declaredIsolation; try { - // #3045 BLOCKER fix: a fresh sentinel is authoritative for THIS - // dispatch's actual resolved isolation — see the doc comment above. - // #3045 SECURITY F2: a fresh sentinel that names a DIFFERENT - // plan/phase than this dispatch is not applicable to it — fall through - // to the conservative fallback exactly as a stale sentinel would. - const sentinel = readSentinel(root, { clock }); - declaredIsolation = (sentinel.present && !sentinel.stale && sentinelAppliesToDispatch(sentinel, dispatchIds)) + declaredIsolation = (sentinel.present && !sentinel.stale && applies) ? sentinel.isolation : resolveFallbackIsolation(root, configPath); } catch { @@ -513,8 +520,10 @@ function evaluateRootIsolation(root, subagentType, { clock = Date, dispatchIds = `dispatch-isolation configuration ('.planning/config.json' exists under "${root}"). ` + `Refusing to allow this subagent to spawn without being able to verify whether ` + `isolation is required — a guard that cannot verify must not answer "safe" (#3050). ` + - `Retry once the project configuration is readable.`, + `Retry once the project configuration is readable.` + + (sentinelDiscarded ? describeSentinelDiscard(sentinelDiscarded) : ''), reasonCode: REASON_CODE.CONFIG_UNREADABLE, + sentinelDiscarded, }; } @@ -530,8 +539,10 @@ function evaluateRootIsolation(root, subagentType, { clock = Date, dispatchIds = `GSD subagent isolation guard: this project's dispatch isolation resolves to ` + `"harness-worktree", but the subagentStart payload for this dispatch carries no usable ` + `subagent_type. Refusing to allow it to spawn without being able to confirm whether it ` + - `is a GSD executor — a guard that cannot verify must not answer "safe" (#3050).`, + `is a GSD executor — a guard that cannot verify must not answer "safe" (#3050).` + + (sentinelDiscarded ? describeSentinelDiscard(sentinelDiscarded) : ''), reasonCode: REASON_CODE.NO_SUBAGENT_TYPE, + sentinelDiscarded, }; } @@ -547,8 +558,10 @@ function evaluateRootIsolation(root, subagentType, { clock = Date, dispatchIds = `"harness-worktree", but whether "${root}" is running in an isolated Cursor worktree ` + `could not be determined (git did not respond). Refusing to allow subagent_type=` + `"${subagentType}" to spawn without being able to verify isolation — a guard that ` + - `cannot verify must not answer "safe" (#3050). Retry once git is responsive.`, + `cannot verify must not answer "safe" (#3050). Retry once git is responsive.` + + (sentinelDiscarded ? describeSentinelDiscard(sentinelDiscarded) : ''), reasonCode: REASON_CODE.CANNOT_DETERMINE_ISOLATION, + sentinelDiscarded, }; } @@ -560,8 +573,10 @@ function evaluateRootIsolation(root, subagentType, { clock = Date, dispatchIds = `which is not an isolated Cursor worktree — it would edit the user's primary checkout ` + `directly, with no consent and no warning. Start an isolated session first (the ` + `"--worktree" CLI flag or the "/worktree" chat command; Cursor manages these worktrees ` + - `under "~/.cursor/worktrees/") and retry.`, + `under "~/.cursor/worktrees/") and retry.` + + (sentinelDiscarded ? describeSentinelDiscard(sentinelDiscarded) : ''), reasonCode: REASON_CODE.NOT_ISOLATED_WORKTREE, + sentinelDiscarded, }; } @@ -611,7 +626,12 @@ function main() { decision = { action: 'allow' }; } if (decision.action === 'deny') { - const out = { permission: 'deny', user_message: decision.reason, reason_code: decision.reasonCode }; + const out = { + permission: 'deny', + user_message: decision.reason, + reason_code: decision.reasonCode, + sentinel_discarded: decision.sentinelDiscarded ?? null, + }; if (additionalContext !== null) out.additional_context = additionalContext; process.stdout.write(JSON.stringify(out)); return; diff --git a/hooks/lib/dispatch-identity.js b/hooks/lib/dispatch-identity.js new file mode 100644 index 000000000..a370b9b6d --- /dev/null +++ b/hooks/lib/dispatch-identity.js @@ -0,0 +1,187 @@ +'use strict'; +// hooks/lib/dispatch-identity.js — the ONE canonical owner of the +// `[gsd:dispatch phase="…" plan="…"]` marker format and its prose fallback +// (#4594, epic #4630 Phase 1). See +// `.gsd/phase/fix-4594-dispatch-identity-seam/40-design.md` for the full +// rationale (behavior table, negative space, rejected alternatives). +// +// COLD-LOAD CONSTRAINT (load-bearing, not a style choice): this module MUST +// require NOTHING — no `fs`, no `path`, and above all nothing under +// `gsd-core/bin/lib/` or `ensure-runtime-build`. The guard hooks that consume +// this module must load on a raw plugin-marketplace install where the +// compiled lib is absent and the self-healing build seam has not run — a +// hook that dies at module load is worse than one carrying a mirror. See +// `tests/dispatch-identity.test.cjs`'s "cold tree load" test, which asserts +// this by monkeypatching `Module._load` to throw on exactly those paths. + +// DISPATCH_PHASE_TOKEN_SOURCE is a DELIBERATE MIRROR of +// `CASE_FLEXIBLE_PHASE_NUMBER_TOKEN_SOURCE` in `src/phase-id.cts` +// (ADR-2121 owns the phase-token grammar; `gsd-core/bin/lib/phase-id.cjs` is +// its compiled form). It cannot be an `require()`-based import: importing the +// compiled lib here would violate the cold-load constraint above (either a +// direct dependency on `gsd-core/bin/lib/` or a forced self-heal via +// `ensure-runtime-build`), which is exactly the failure mode this module +// exists to avoid for the guard hooks that consume it. +// +// Because this is a hand-copied mirror and not a shared reference, a +// hand-edited grammar change on one side that is not mirrored on the other +// does NOT fail loudly — both sides remain independently valid regex +// sources, so the failure mode is a silent, invisible non-match (a dispatch +// whose phase token the two owners now parse differently), never a thrown +// error. `tests/dispatch-identity.test.cjs`'s "templates: prose token source +// matches the case-flexible phase-id grammar" test pins this string equal to +// the compiled source at test time specifically to turn that silent drift +// into a loud, in-CI failure. +const DISPATCH_PHASE_TOKEN_SOURCE = '\\d+[A-Za-z]?(?:\\.\\d+)*'; + +// Bounded marker grammar: `[gsd:dispatch key="value" key2="value2"]`. +// - Key names bounded to {1,31} — no key we emit or expect is anywhere near +// that long; this is purely a backstop against pathological input. +// - Values bounded to {0,200} and forbidden from containing `"`, `]`, or any +// control character that could either close the marker early or forge a +// sibling key — enforced structurally by the negated character class, not +// by a separate validation pass. +// Global flag so `findMarker` can scan forward through a large prompt. +const MARKER_RE = /\[gsd:dispatch((?:\s+[A-Za-z][A-Za-z0-9_-]{0,31}="[^"\]\r\n]{0,200}")*)\s*\]/g; +const MARKER_KV_RE = /([A-Za-z][A-Za-z0-9_-]{0,31})="([^"\]\r\n]{0,200})"/g; + +// Prose fallback frame: `execute plan of phase `. +// - Case-insensitive, `\s+` throughout so CRLF (and any run of whitespace) +// parses identically to LF. +// - The plan token is bounded (`\S{1,80}`) but its VALUE is deliberately +// discarded by the caller (see `parseDispatchIdentity` below) — captured +// only so the phase token can be anchored correctly after it. +// - The phase token is bounded by `DISPATCH_PHASE_TOKEN_SOURCE`, replacing +// the old greedy `(\S+)`, so a directory-name suffix (`-auth`) or a +// sentence-terminating period is never swept into the token. +const PROSE_RE = new RegExp( + `execute\\s+plan\\s+(\\S{1,80})\\s+of\\s+phase\\s+(${DISPATCH_PHASE_TOKEN_SOURCE})`, + 'i', +); + +/** + * Render the `[gsd:dispatch phase="…" plan="…"]` marker for a producer to + * embed verbatim in a dispatch description/prompt. Never throws. + * + * Emits only keys whose value is a non-empty string not containing `"`, `]`, + * `\r`, or `\n` — such a value is UNUSABLE and that key is omitted entirely, + * because embedding it could close the marker early and forge a second, + * attacker-controlled field. Key order is always `phase` then `plan`. + * Returns `''` when neither key is usable (nothing worth emitting). + */ +function renderDispatchIdentityMarker(input) { + const value = input && typeof input === 'object' ? input : {}; + const parts = []; + for (const key of ['phase', 'plan']) { + const v = value[key]; + if (typeof v === 'string' && v.length > 0 && !/["\]\r\n]/.test(v)) { + parts.push(`${key}="${v}"`); + } + } + if (parts.length === 0) return ''; + return `[gsd:dispatch ${parts.join(' ')}]`; +} + +function emptyResult() { + return { phase: null, plan: null, source: null }; +} + +/** + * Scan `texts` in order for the first well-formed marker that yields at + * least one recognized key (`phase` and/or `plan`). Returns `{ phase, plan }` + * (one of the two possibly null, never both) or `null` if no QUALIFYING + * marker was found in any text. Unrecognized keys inside a marker are + * ignored (forward compatibility — a later `run=`/`wave=` key must not break + * a deployed parser). + * + * #4594 F1 fix: a marker that matches `MARKER_RE` but carries neither + * `phase=` nor `plan=` (e.g. only unrecognized keys, or an empty kv block) + * is NOT treated as "found" — it is skipped and scanning continues (later + * markers in the same text, then subsequent texts), falling through to the + * prose fallback if nothing qualifying turns up. Without this, prompt text + * that merely CONTAINS the literal marker syntax with no usable identifiers + * silently suppressed the prose fallback entirely, since the old + * implementation returned on the first syntactic match regardless of + * content. + */ +function findMarker(texts) { + for (const text of texts) { + if (typeof text !== 'string' || text.length === 0) continue; + MARKER_RE.lastIndex = 0; + let match; + while ((match = MARKER_RE.exec(text)) !== null) { + const kvBlock = match[1] || ''; + let phase = null; + let plan = null; + MARKER_KV_RE.lastIndex = 0; + let kv; + while ((kv = MARKER_KV_RE.exec(kvBlock)) !== null) { + const [, key, val] = kv; + if (key === 'phase' && phase === null) phase = val; + else if (key === 'plan' && plan === null) plan = val; + } + if (phase === null && plan === null) continue; + return { phase, plan }; + } + } + return null; +} + +/** + * Scan `texts` in order for the first `execute plan of phase + * ` frame. Returns `{ phase }` or `null`. + * + * The prose fallback returns `plan: null` DELIBERATELY (enforced by the + * caller, not here): the prose plan token (e.g. `02`) is a bare in-phase + * plan number, a different namespace from the sentinel's `plan_id` (e.g. + * `03-02-hardening`, which is phase-prefixed AND slugged). Reporting the + * prose plan token as the parsed `plan` is exactly the false-mismatch bug + * this module exists to fix — an absent value is honestly "cannot compare", + * while a wrong value silently forges a mismatch on every dispatch. Do not + * "fix" this by threading the plan token through — that was the bug. + */ +function findProse(texts) { + for (const text of texts) { + if (typeof text !== 'string' || text.length === 0) continue; + const match = PROSE_RE.exec(text); + if (match) { + return { phase: match[2] }; + } + } + return null; +} + +/** + * Parse dispatch identity out of one or more texts (a short description, a + * full prompt body, etc). Never throws for any input type; non-string + * entries are skipped. + * + * Pass 1: scan all texts in order for a well-formed marker; the first one + * wins outright (marker beats prose even if prose appears earlier in the + * same text — see the design doc's row 8). + * Pass 2 (only if no marker was found anywhere): scan all texts in order for + * the prose frame; the first match wins. `plan` is always `null` from this + * path (see `findProse`'s doc comment). + * + * Returns `{ phase, plan, source }` where `source` is `'marker' | 'prose' | + * null`. + */ +function parseDispatchIdentity(...texts) { + const marker = findMarker(texts); + if (marker) { + return { phase: marker.phase, plan: marker.plan, source: 'marker' }; + } + + const prose = findProse(texts); + if (prose) { + return { phase: prose.phase, plan: null, source: 'prose' }; + } + + return emptyResult(); +} + +module.exports = { + DISPATCH_PHASE_TOKEN_SOURCE, + renderDispatchIdentityMarker, + parseDispatchIdentity, +}; diff --git a/hooks/lib/isolation-deny-reason.js b/hooks/lib/isolation-deny-reason.js index 64972b8e1..8f9d99e37 100644 --- a/hooks/lib/isolation-deny-reason.js +++ b/hooks/lib/isolation-deny-reason.js @@ -36,4 +36,56 @@ const REASON_CODE = Object.freeze({ NOT_ISOLATED_WORKTREE: 'not_isolated_worktree', }); -module.exports = { REASON_CODE }; +// #4594 row 15 / F2: values interpolated into a deny reason (sentinel/dispatch +// phase and plan) come from a sentinel file on disk and, transitively, from +// model-authored prompt text, neither of which is trusted — bound length and +// strip control characters/newlines so a crafted value cannot forge extra +// lines or otherwise inject content into the guard's stdout/stderr message +// (same discipline as escaping an untrusted token before embedding it in a +// message, e.g. phase-plan-index's `depends_on` warning). +// +// Originally duplicated byte-for-byte in hooks/gsd-agent-isolation-guard.js +// and hooks/gsd-cursor-subagent-start.js (#4594 F2 review finding) — both +// hooks already require this dependency-free module, so there is no +// cold-load justification for the duplication the way there is for +// hooks/lib/dispatch-identity.js's grammar mirror. Moved here as the single +// owner; both hooks now import it. +const REASON_INTERPOLATION_MAX_LEN = 64; + +// F6: also strip Unicode line/paragraph separators (U+2028/U+2029) and the +// bidi-override/isolate control characters (U+202A-U+202E, U+2066-U+2069) — +// none of these are in `[\x00-\x1f\x7f]`, so a crafted sentinel or dispatch +// value carrying them could still visually reflow the deny message onto a +// new line or reverse/hide part of it despite the ASCII control-char strip. +const REASON_UNSAFE_CHARS_RE = new RegExp( + // eslint-disable-next-line no-control-regex -- deliberately stripping control chars/newlines + '[\\x00-\\x1f\\x7f\\u2028\\u2029\\u202a-\\u202e\\u2066-\\u2069]', + 'g', +); + +function sanitizeForReason(value) { + if (typeof value !== 'string' || value.length === 0) return '(none)'; + const stripped = value.replace(REASON_UNSAFE_CHARS_RE, ''); + return stripped.length > REASON_INTERPOLATION_MAX_LEN + ? `${stripped.slice(0, REASON_INTERPOLATION_MAX_LEN)}…` + : stripped; +} + +function describeSentinelDiscard(sentinelDiscarded) { + const sentinelPhase = sanitizeForReason(sentinelDiscarded.sentinel.phase); + const sentinelPlan = sanitizeForReason(sentinelDiscarded.sentinel.plan); + const dispatchPhase = sanitizeForReason(sentinelDiscarded.dispatch.phase); + const dispatchPlan = sanitizeForReason(sentinelDiscarded.dispatch.plan); + return ( + ` A fresh dispatch-isolation sentinel was present but did not apply to this dispatch ` + + `(sentinel phase="${sentinelPhase}" plan="${sentinelPlan}"; dispatch phase="${dispatchPhase}" ` + + `plan="${dispatchPlan}"), so it was not consulted.` + ); +} + +module.exports = { + REASON_CODE, + REASON_INTERPOLATION_MAX_LEN, + sanitizeForReason, + describeSentinelDiscard, +}; diff --git a/hooks/lib/isolation-sentinel.js b/hooks/lib/isolation-sentinel.js index d8c15341b..de4b6c46f 100644 --- a/hooks/lib/isolation-sentinel.js +++ b/hooks/lib/isolation-sentinel.js @@ -56,6 +56,7 @@ const fs = require('fs'); const path = require('path'); +const { parseDispatchIdentity } = require('./dispatch-identity.js'); // Isolation modes ADR-1239 declares (mirrors gsd-tools.cjs // routeDispatchIsolation / routeRecordDispatchIsolation). @@ -223,26 +224,40 @@ function readSentinel(cwd, { clock = Date } = {}) { } /** - * #3045 SECURITY F2: extract the `{plan, phase}` a specific Agent()/Task() - * dispatch is FOR, from the one place that data is reliably embedded today — - * the dispatch prompt/description text (`execute-phase.md`'s Agent() block - * uses the literal shape `description="Execute plan {plan_number} of phase - * {phase_number}"`, and the prompt body's `` repeats "Execute plan - * {plan_number} of phase {phase_number}-{phase_name}." verbatim — the SAME - * text the orchestrator-worktree EXECUTOR_PROMPT template and Cursor's `task` - * field carry, since Cursor dispatches the same prompt content). There is no - * structured per-dispatch kwarg carrying plan/phase identifiers today (#3045 - * would need a larger dispatch-protocol change to add one) — this is - * therefore a best-effort, NOT a guaranteed, extraction: a dispatch whose - * text doesn't match the expected shape returns `{ plan: null, phase: null }` - * and the caller must NOT treat that as a mismatch (see - * `sentinelAppliesToDispatch`). + * #3045 SECURITY F2 / #4594: extract the `{plan, phase}` a specific + * Agent()/Task() dispatch is FOR. Variadic — accepts any number of text + * sources (short description, full prompt body, etc) and delegates to + * `hooks/lib/dispatch-identity.js::parseDispatchIdentity`, the one canonical + * owner of both the `[gsd:dispatch phase="…" plan="…"]` marker format and its + * prose fallback (see `.gsd/phase/fix-4594-dispatch-identity-seam/40-design.md`). + * + * MARKER-FIRST CONTRACT: producers embed a structured marker carrying the + * exact shell values the sentinel itself records (`$PHASE_NUMBER`, + * `$plan_id`), so producer and consumer agree by construction, independent + * of how the prose reads or whether a model paraphrases the dispatch + * sentence. Only when no marker is found anywhere in the supplied texts does + * this fall back to scanning for the prose frame "execute plan of + * phase ". + * + * The prose fallback is now CORRECT-OR-ABSENT rather than possibly-wrong: + * the phase token is bounded by the same grammar `src/phase-id.cts` owns + * (ADR-2121), so a directory-name slug or trailing punctuation can no longer + * leak into the phase value, and the prose plan token is never reported at + * all (it lives in a different namespace than the sentinel's phase-prefixed, + * slugged `plan_id` — reporting it was the #4594 false-mismatch bug). + * + * This is still a best-effort, NOT a guaranteed, extraction: a dispatch that + * carries neither a marker nor a matching prose frame in ANY supplied text + * returns `{ plan: null, phase: null }`, and the caller MUST NEVER treat + * that as a mismatch — see `sentinelAppliesToDispatch`, whose whole + * contract depends on "missing" and "wrong" being distinguishable. + * + * Returns only the two-field `{ plan, phase }` shape existing callers + * depend on — `parseDispatchIdentity`'s `source` field is discarded here. */ -function extractDispatchIdentifiers(text) { - if (typeof text !== 'string' || text.length === 0) return { plan: null, phase: null }; - const m = /execute\s+plan\s+(\S+)\s+of\s+phase\s+(\S+)/i.exec(text); - if (!m) return { plan: null, phase: null }; - return { plan: m[1], phase: m[2] }; +function extractDispatchIdentifiers(...texts) { + const { phase, plan } = parseDispatchIdentity(...texts); + return { plan, phase }; } /** @@ -265,6 +280,29 @@ function sentinelAppliesToDispatch(sentinel, dispatchIds) { return true; } +/** + * #4594 F3: build the structured "a fresh sentinel was present but did not + * apply to this dispatch" descriptor, mirroring the exact comparison + * `sentinelAppliesToDispatch` performs. Returns `null` when the sentinel is + * absent, stale, malformed, or DOES apply — i.e. exactly when there is + * nothing to report as discarded. Otherwise returns the nested + * `{ sentinel: {phase, plan}, dispatch: {phase, plan} }` shape, reusing the + * `{phase, plan}` pair already flowing through this module end to end rather + * than renaming its fields into an ad hoc `sentinelPhase`/`dispatchPlan` bag + * (previously rebuilt identically at two call sites in the guard hooks). + */ +function buildSentinelDiscard(sentinel, dispatchIds) { + if (!sentinel || !sentinel.present || sentinel.stale) return null; + if (sentinelAppliesToDispatch(sentinel, dispatchIds)) return null; + return { + sentinel: { phase: sentinel.phase ?? null, plan: sentinel.plan ?? null }, + dispatch: { + phase: dispatchIds ? (dispatchIds.phase ?? null) : null, + plan: dispatchIds ? (dispatchIds.plan ?? null) : null, + }, + }; +} + module.exports = { VALID_ISOLATION, SENTINEL_RELATIVE_PATH, @@ -274,4 +312,5 @@ module.exports = { readSentinel, extractDispatchIdentifiers, sentinelAppliesToDispatch, + buildSentinelDiscard, }; diff --git a/scripts/lib/platform-conformance-tier.generated.cjs b/scripts/lib/platform-conformance-tier.generated.cjs index 831bfc718..b7f94c8df 100644 --- a/scripts/lib/platform-conformance-tier.generated.cjs +++ b/scripts/lib/platform-conformance-tier.generated.cjs @@ -68,6 +68,7 @@ module.exports = { "tests/cursor-hook-workspace-roots.test.cjs", "tests/cursor-hooks.test.cjs", "tests/cursor-subagent-isolation.test.cjs", + "tests/dispatch-identity.test.cjs", "tests/dispatcher.test.cjs", "tests/drift-detection.test.cjs", "tests/effort-surface-axis.test.cjs", diff --git a/tests/cursor-subagent-isolation.test.cjs b/tests/cursor-subagent-isolation.test.cjs index de04beede..da6c01cc0 100644 --- a/tests/cursor-subagent-isolation.test.cjs +++ b/tests/cursor-subagent-isolation.test.cjs @@ -854,6 +854,31 @@ describe('gsd-cursor-subagent-start.js: #3045 SECURITY F2 — sentinel bound to }); }); +describe('gsd-cursor-subagent-start.js: #4594 row 30 — sentinel-discard reporting parity with the Claude guard', () => { + let harnessProject; + + before(() => { + harnessProject = makeGitProject('gsd-cs-4594-', JSON.stringify({ runtime: 'cursor' })); + }); + + after(() => { + cleanup(harnessProject); + }); + + test('row 30: `data.task` in prose form + a fresh matching {isolation:"none"} sentinel -> ALLOW (was: discarded and denied)', (t) => { + // Measured production shape: sentinel plan is phase-prefixed-and-slugged + // (phase-plan-index's `plans[].id`); the prose fallback never reports a + // plan (see hooks/lib/dispatch-identity.js's findProse doc comment), so + // only phase is compared here — exactly the real-world match path. + writeSentinel(harnessProject, { isolation: 'none', phase: '03', plan: '03-02-hardening' }); + t.after(() => cleanup(path.join(harnessProject, '.gsd'))); + const r = runHook(subagentPayload([harnessProject], { task: 'Execute plan 02 of phase 03-auth.' })); + assert.equal(r.status, 0, `stdout: ${r.stdout} stderr: ${r.stderr}`); + const out = JSON.parse(r.stdout); + assert.equal(out.permission, undefined, `expected allow, got: ${r.stdout}`); + }); +}); + describe('gsd-cursor-subagent-start.js: #3045 MAJOR — clock seam boundary coverage (in-process, no subprocess wall-clock race)', () => { const cursorHookModule = require('../hooks/gsd-cursor-subagent-start.js'); diff --git a/tests/dispatch-identity.test.cjs b/tests/dispatch-identity.test.cjs new file mode 100644 index 000000000..7d75beae0 --- /dev/null +++ b/tests/dispatch-identity.test.cjs @@ -0,0 +1,445 @@ +'use strict'; + +/** + * Tests for the (not-yet-created) `hooks/lib/dispatch-identity.js` — the one + * canonical owner of the `[gsd:dispatch phase="…" plan="…"]` marker format + * and its prose fallback (#4594, epic #4630 Phase 1). + * + * See `.gsd/phase/fix-4594-dispatch-identity-seam/40-design.md` (behavior + * table + negative space) and `50-test-matrix.md` (row -> test-name mapping) + * for the full rationale. This file intentionally covers matrix rows 1-27 + * and 33-34 only; rows 28-32 are guard end-to-end rows that belong in + * `tests/gsd-agent-isolation-guard.test.cjs` / `tests/cursor-subagent-isolation.test.cjs`. + * + * RED BY DESIGN: `hooks/lib/dispatch-identity.js` does not exist yet. This + * whole file fails to load with MODULE_NOT_FOUND until that module is + * written — that failure is the expected, correct state of this commit. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { execFileSync } = require('node:child_process'); +const fc = require('fast-check'); +const { GENERATOR_SCRIPT_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); + +const { + DISPATCH_PHASE_TOKEN_SOURCE, + renderDispatchIdentityMarker, + parseDispatchIdentity, +} = require('../hooks/lib/dispatch-identity.js'); + +const { + extractDispatchIdentifiers, + sentinelAppliesToDispatch, +} = require('../hooks/lib/isolation-sentinel.js'); + +const REPO_ROOT = path.resolve(__dirname, '..'); + +// Measured production shapes (40-design.md): the sentinel's own {phase, plan} +// as recorded by a real `phase-plan-index` run in this worktree, and the two +// verbatim dispatch-prose templates that embed the phase/plan numbers. +const MEASURED_SENTINEL = { phase: '03', plan: '03-02-hardening' }; +const DESCRIPTION_FORM = 'Execute plan 02 of phase 03'; +const PROMPT_BODY_FORM = 'Execute plan 02 of phase 03-auth.'; + +/** + * Row 33's cold-tree probe: a single `node -e` script, spawned directly (no + * fan-out), doing an in-process require plus two synchronous calls against a + * small fixture path. Reuses `GENERATOR_SCRIPT_TIMEOUT_MS` (30000ms) rather + * than declaring a new literal — that constant's own doc comment describes + * "a single ... script, spawned directly ... against a small temp fixture + * repo -- no fan-out", which is this call shape as well, even though the + * spawned script here is an inline `-e` probe rather than a `scripts/*.cjs` + * file. + */ +const COLD_TREE_PROBE_TIMEOUT_MS = GENERATOR_SCRIPT_TIMEOUT_MS; + +describe('hooks/lib/dispatch-identity.js', () => { + describe('marker format grammar', () => { + test('marker: parses both identifiers', () => { + const result = parseDispatchIdentity('[gsd:dispatch phase="03" plan="03-02-hardening"]'); + assert.deepEqual(result, { phase: '03', plan: '03-02-hardening', source: 'marker' }); + }); + + test('marker: found anywhere in a large prompt', () => { + const text = `some preamble\n\nmore lines here\n[gsd:dispatch phase="03" plan="03-02-hardening"]\n\ntrailing text`; + const result = parseDispatchIdentity(text); + assert.equal(result.phase, '03'); + assert.equal(result.plan, '03-02-hardening'); + assert.equal(result.source, 'marker'); + }); + + test('render → parse round-trips', () => { + const marker = renderDispatchIdentityMarker({ phase: '03', plan: '03-02-hardening' }); + assert.equal(marker, '[gsd:dispatch phase="03" plan="03-02-hardening"]'); + const parsed = parseDispatchIdentity(marker); + assert.deepEqual(parsed, { phase: '03', plan: '03-02-hardening', source: 'marker' }); + }); + + test('property: render/parse is a bijection on valid tokens', () => { + // Only values a real producer could emit: filesystem-derived phase/plan + // identifiers, so no quotes, brackets, or newlines (those are exactly + // the hostile-value case covered separately by row 21). + const safeToken = fc + .stringMatching(/^[A-Za-z0-9._-]+$/) + .filter((s) => s.length > 0 && s.length <= 64); + + fc.assert( + fc.property(safeToken, safeToken, (phase, plan) => { + const marker = renderDispatchIdentityMarker({ phase, plan }); + const parsed = parseDispatchIdentity(marker); + assert.equal(parsed.phase, phase); + assert.equal(parsed.plan, plan); + assert.equal(parsed.source, 'marker'); + }), + { seed: 4594, numRuns: 300 }, + ); + }); + }); + + describe('prose fallback', () => { + test('prose: bare phase number, plan deliberately null', () => { + const result = parseDispatchIdentity(DESCRIPTION_FORM); + assert.deepEqual(result, { phase: '03', plan: null, source: 'prose' }); + }); + + test('prose: slug suffix and trailing period are not part of the token', () => { + const result = parseDispatchIdentity(PROMPT_BODY_FORM); + assert.deepEqual(result, { phase: '03', plan: null, source: 'prose' }); + }); + + test('prose: decimal sub-phase', () => { + const result = parseDispatchIdentity('Execute plan 02 of phase 3.2-thing.'); + assert.equal(result.phase, '3.2'); + assert.equal(result.plan, null); + }); + + test('prose: variant-letter phase', () => { + const result = parseDispatchIdentity('Execute plan 02 of phase 12A-thing.'); + assert.equal(result.phase, '12A'); + assert.equal(result.plan, null); + }); + + test('prose: multi-segment sub-phase', () => { + const result = parseDispatchIdentity('Execute plan 02 of phase 3.2.1'); + assert.equal(result.phase, '3.2.1'); + assert.equal(result.plan, null); + }); + + test('prose: token at end of input', () => { + const result = parseDispatchIdentity('Execute plan 02 of phase 03'); + assert.equal(result.phase, '03'); + }); + + test('prose: requires the full execute-plan-of-phase frame', () => { + const lookalikes = [ + 'run phase-plan-index for this project', + 'we are still in this phase 03 of the rollout', + 'the phase_dir helper resolves the directory', + ]; + for (const text of lookalikes) { + assert.deepEqual( + parseDispatchIdentity(text), + { phase: null, plan: null, source: null }, + `unexpected match for lookalike: ${text}`, + ); + } + }); + + test('prose: anchors on the first frame only', () => { + const text = 'Execute plan 02 of phase 03-auth. Note: wait until after plan 03-01 completes.'; + const result = parseDispatchIdentity(text); + assert.equal(result.phase, '03'); + assert.equal(result.plan, null); + }); + }); + + describe('precedence and malformed markers', () => { + test('marker takes precedence over prose', () => { + const text = 'Execute plan 02 of phase 03-auth. [gsd:dispatch phase="04" plan="04-01-setup"]'; + const result = parseDispatchIdentity(text); + assert.equal(result.phase, '04'); + assert.equal(result.plan, '04-01-setup'); + assert.equal(result.source, 'marker'); + }); + + test('marker: unquoted value is not accepted', () => { + const result = parseDispatchIdentity('[gsd:dispatch phase=03]'); + // Never a partial wrong value: either falls back to null/null (no + // prose present here) or, if this text also had prose, to the prose + // result — but never a phase of '03' read out of the malformed marker. + assert.notEqual(result.source, 'marker'); + assert.deepEqual(result, { phase: null, plan: null, source: null }); + }); + + test('marker: unknown keys are ignored', () => { + const result = parseDispatchIdentity('[gsd:dispatch phase="03" plan="03-02-hardening" run="abc"]'); + assert.equal(result.phase, '03'); + assert.equal(result.plan, '03-02-hardening'); + assert.equal(result.source, 'marker'); + }); + + test('marker: key order does not matter', () => { + const result = parseDispatchIdentity('[gsd:dispatch plan="03-02-hardening" phase="03"]'); + assert.equal(result.phase, '03'); + assert.equal(result.plan, '03-02-hardening'); + }); + + test('marker: phase-only is valid', () => { + const result = parseDispatchIdentity('[gsd:dispatch phase="03"]'); + assert.deepEqual(result, { phase: '03', plan: null, source: 'marker' }); + }); + + // #4594 F1: a syntactically well-formed marker that carries NEITHER + // `phase=` nor `plan=` must not suppress the prose fallback — it is not + // a marker at all for this parser's purposes. + test('F1: keyless marker (no recognized keys) falls through to prose', () => { + const result = parseDispatchIdentity('Execute plan 02 of phase 03-auth.\n[gsd:dispatch]'); + assert.deepEqual(result, { phase: '03', plan: null, source: 'prose' }); + }); + + test('F1: marker with only an unrecognized key falls through to prose', () => { + const result = parseDispatchIdentity('Execute plan 02 of phase 03-auth.\n[gsd:dispatch run="x"]'); + assert.deepEqual(result, { phase: '03', plan: null, source: 'prose' }); + }); + + test('F1: keyless marker with no prose anywhere yields the empty result', () => { + const result = parseDispatchIdentity('[gsd:dispatch]'); + assert.deepEqual(result, { phase: null, plan: null, source: null }); + }); + + test('F1: mixed case — a keyless marker precedes a later QUALIFYING marker, which wins', () => { + const result = parseDispatchIdentity( + '[gsd:dispatch] some text [gsd:dispatch phase="03" plan="03-02-hardening"]', + ); + assert.deepEqual(result, { phase: '03', plan: '03-02-hardening', source: 'marker' }); + }); + + test('F1: unknown-key tolerance is preserved for a marker that ALSO carries a recognized key', () => { + const result = parseDispatchIdentity('[gsd:dispatch phase="03" run="x"]'); + assert.deepEqual(result, { phase: '03', plan: null, source: 'marker' }); + }); + }); + + describe('hostile and edge inputs', () => { + test('parse: non-string and empty inputs', () => { + const hostileInputs = [undefined, null, '', 0, 42, true, false, [], {}, NaN]; + for (const input of hostileInputs) { + assert.doesNotThrow(() => parseDispatchIdentity(input)); + assert.deepEqual( + parseDispatchIdentity(input), + { phase: null, plan: null, source: null }, + `unexpected result for hostile input: ${String(input)}`, + ); + } + // Zero-argument call must also never throw and yield the same empty shape. + assert.doesNotThrow(() => parseDispatchIdentity()); + assert.deepEqual(parseDispatchIdentity(), { phase: null, plan: null, source: null }); + }); + + test('parse: unrelated prose', () => { + const result = parseDispatchIdentity('this text has nothing to do with any dispatch'); + assert.deepEqual(result, { phase: null, plan: null, source: null }); + }); + + test('CRLF variants parse identically', () => { + const lf = parseDispatchIdentity('preamble\n[gsd:dispatch phase="03" plan="03-02-hardening"]\ntrailer'); + const crlf = parseDispatchIdentity('preamble\r\n[gsd:dispatch phase="03" plan="03-02-hardening"]\r\ntrailer'); + assert.deepEqual(crlf, lf); + + const lfProse = parseDispatchIdentity('Execute plan 02 of phase 03-auth.\nmore text'); + const crlfProse = parseDispatchIdentity('Execute plan 02 of phase 03-auth.\r\nmore text'); + assert.deepEqual(crlfProse, lfProse); + }); + + test('marker: quote/bracket in a value cannot forge a field', () => { + // A value containing '"' or ']' is unusable per the render contract; a + // hand-crafted malicious marker string attempting the same must not + // let the parser manufacture a second, forged identifier either. + const hostileMarker = '[gsd:dispatch phase="03" plan="03-02"]" phase="99"]'; + const result = parseDispatchIdentity(hostileMarker); + assert.notEqual(result.phase, '99'); + }); + + test('parse: large prompt terminates', (t) => { + const filler = 'x'.repeat(100 * 1024); + const text = `${filler}[gsd:dispatch phase="03" plan="03-02-hardening"]`; + const result = parseDispatchIdentity(text); + assert.equal(result.phase, '03'); + assert.equal(result.plan, '03-02-hardening'); + // Backstop against catastrophic backtracking, not a timing assertion: + // this test's own node:test timeout is the enforcement mechanism. + t.diagnostic('parse completed within the test timeout on a 100KB input'); + }); + + test('parse: scans every supplied text in order', () => { + const result = parseDispatchIdentity('', '[gsd:dispatch phase="03" plan="03-02-hardening"]'); + assert.equal(result.phase, '03'); + assert.equal(result.plan, '03-02-hardening'); + assert.equal(result.source, 'marker'); + }); + }); + + describe('render: omission rules', () => { + test('render: unusable values (quote or bracket) are omitted', () => { + assert.equal(renderDispatchIdentityMarker({ phase: '03', plan: 'has"quote' }), '[gsd:dispatch phase="03"]'); + assert.equal(renderDispatchIdentityMarker({ phase: '03', plan: 'has]bracket' }), '[gsd:dispatch phase="03"]'); + assert.equal(renderDispatchIdentityMarker({ phase: 'ba"d', plan: '03-02' }), '[gsd:dispatch plan="03-02"]'); + }); + + test('render: absent or empty values are omitted, and both-absent renders empty string', () => { + assert.equal(renderDispatchIdentityMarker({ phase: '03' }), '[gsd:dispatch phase="03"]'); + assert.equal(renderDispatchIdentityMarker({ plan: '03-02-hardening' }), '[gsd:dispatch plan="03-02-hardening"]'); + assert.equal(renderDispatchIdentityMarker({ phase: '', plan: '' }), ''); + assert.equal(renderDispatchIdentityMarker({}), ''); + }); + }); + + describe('sentinelAppliesToDispatch end-state behavior (hooks/lib/isolation-sentinel.js)', () => { + test('applies: prose dispatch no longer false-mismatches on plan', () => { + // THE regression, #4594 rows 2/4: a fresh sentinel carrying the + // phase-prefixed, slugged plan id must still apply to a dispatch whose + // only identity source is the prose prompt-body form. + const dispatchIds = extractDispatchIdentifiers(PROMPT_BODY_FORM); + assert.equal(sentinelAppliesToDispatch(MEASURED_SENTINEL, dispatchIds), true); + }); + + test('applies: marker for another plan is rejected', () => { + const dispatchIds = parseDispatchIdentity('[gsd:dispatch phase="03" plan="03-01-setup"]'); + assert.equal(sentinelAppliesToDispatch(MEASURED_SENTINEL, dispatchIds), false); + }); + + test('applies: marker for another phase is rejected', () => { + const dispatchIds = parseDispatchIdentity('[gsd:dispatch phase="04" plan="03-02-hardening"]'); + assert.equal(sentinelAppliesToDispatch(MEASURED_SENTINEL, dispatchIds), false); + }); + + test('applies: a missing value is never a mismatch', () => { + assert.equal(sentinelAppliesToDispatch(MEASURED_SENTINEL, { phase: null, plan: null }), true); + assert.equal(sentinelAppliesToDispatch({ phase: '03', plan: null }, { phase: '03', plan: '03-02-hardening' }), true); + assert.equal(sentinelAppliesToDispatch({ phase: null, plan: '03-02-hardening' }, { phase: '99', plan: null }), true); + }); + + test('extractDispatchIdentifiers still returns exactly {plan, phase}', () => { + const result = extractDispatchIdentifiers(DESCRIPTION_FORM); + assert.deepEqual(Object.keys(result).sort(), ['phase', 'plan']); + }); + }); + + describe('cold-tree load (#3582-style standing constraint)', () => { + test('cold tree: the owner module has no compiled-lib dependency', () => { + const probe = ` + const Module = require('module'); + const originalLoad = Module._load; + Module._load = function patchedLoad(request, parent, isMain) { + if (typeof request === 'string' && ( + request.includes('gsd-core/bin/lib') || + request.includes('ensure-runtime-build') + )) { + throw new Error('FORBIDDEN cold-tree require: ' + request); + } + return originalLoad.call(this, request, parent, isMain); + }; + const owner = require(${JSON.stringify(path.join(REPO_ROOT, 'hooks', 'lib', 'dispatch-identity.js'))}); + const marker = owner.renderDispatchIdentityMarker({ phase: '03', plan: '03-02-hardening' }); + const parsed = owner.parseDispatchIdentity(marker); + if (parsed.phase !== '03' || parsed.plan !== '03-02-hardening') { + throw new Error('cold-tree round-trip failed: ' + JSON.stringify(parsed)); + } + process.stdout.write('OK'); + `; + + const output = execFileSync(process.execPath, ['-e', probe], { + cwd: REPO_ROOT, + encoding: 'utf8', + timeout: COLD_TREE_PROBE_TIMEOUT_MS, + }); + assert.equal(output, 'OK'); + }); + }); + + describe('producer/consumer parity', () => { + // #4594 F7: this reads the REAL workflow templates and runs their + // marker literal through the owner's real parser after substituting the + // measured placeholders — a template that loses its `[gsd:dispatch …]` + // line, or whose grammar the owner's parser can no longer read, reds + // this test. The prior version only called `renderDispatchIdentityMarker` + // and re-parsed its own output (a duplicate of the round-trip test + // above) and would have passed even if both templates below were + // deleted. There are 3 prose "execute plan ... of phase ..." sites + // across these 2 files; only 2 of them carry the `[gsd:dispatch …]` + // marker (execute-phase.md's `description=` field is prose-only) — see + // `.gsd/phase/fix-4594-dispatch-identity-seam/40-design.md`'s "Known + // limits" section. + const TEMPLATE_FILES = [ + path.join(REPO_ROOT, 'gsd-core', 'workflows', 'execute-phase.md'), + path.join(REPO_ROOT, 'gsd-core', 'workflows', 'execute-phase', 'steps', 'executor-isolation-dispatch.md'), + ]; + + /** + * Extract every `[gsd:dispatch ...]` marker LINE from a template's raw + * text, verbatim (still containing its `{phase_number}`/`{plan_id}` + * placeholders) — not a string match on content, a line-oriented + * extraction feeding the real parser below. + */ + function extractMarkerLines(text) { + return text.split(/\r?\n/).filter((line) => line.includes('[gsd:dispatch')); + } + + function substitutePlaceholders(line) { + return line + .replace(/\{phase_number\}/g, MEASURED_SENTINEL.phase) + .replace(/\{plan_id\}/g, MEASURED_SENTINEL.plan); + } + + test('templates: every producer\'s marker parses', () => { + let totalMarkerLines = 0; + for (const file of TEMPLATE_FILES) { + assert.equal(fs.existsSync(file), true, `template file not found: ${file}`); + const text = fs.readFileSync(file, 'utf-8'); + const markerLines = extractMarkerLines(text); + assert.equal( + markerLines.length, + 1, + `expected exactly one [gsd:dispatch ...] marker line in ${file}, found ${markerLines.length}`, + ); + totalMarkerLines += markerLines.length; + + const substituted = substitutePlaceholders(markerLines[0]); + const parsed = parseDispatchIdentity(substituted); + assert.deepEqual( + parsed, + { phase: MEASURED_SENTINEL.phase, plan: MEASURED_SENTINEL.plan, source: 'marker' }, + `template marker in ${file} did not parse to the expected identifiers (got ${JSON.stringify(parsed)})`, + ); + } + // 3 prose sites exist across these 2 files; exactly 2 carry the marker. + assert.equal(totalMarkerLines, 2); + }); + + test('templates: prose token source matches the case-flexible phase-id grammar', () => { + // Parity guard, matrix row 34: DISPATCH_PHASE_TOKEN_SOURCE is a hand + // mirror of gsd-core/bin/lib/phase-id.cjs's + // CASE_FLEXIBLE_PHASE_NUMBER_TOKEN_SOURCE. The mirror exists because + // hooks/lib/dispatch-identity.js must load on a cold tree with no + // compiled `gsd-core/bin/lib/` present (see the cold-tree test above), + // so it cannot `require()` the compiled module directly — it restates + // the same regex source string instead. A hand-edited grammar change + // on one side that is not mirrored on the other does NOT fail loudly: + // both sides remain independently valid regex sources, so the failure + // mode is a silent, invisible non-match (a dispatch whose phase token + // the two owners now parse differently), not a thrown error. Pinning + // this equality is the only thing that turns that drift into a loud, + // in-CI failure. Loaded only here, not at module scope, so this + // file's own MODULE_NOT_FOUND failure mode (before + // hooks/lib/dispatch-identity.js exists) is not masked by a require + // ordering accident. + const { CASE_FLEXIBLE_PHASE_NUMBER_TOKEN_SOURCE } = require( + path.join(REPO_ROOT, 'gsd-core', 'bin', 'lib', 'phase-id.cjs'), + ); + assert.equal(DISPATCH_PHASE_TOKEN_SOURCE, CASE_FLEXIBLE_PHASE_NUMBER_TOKEN_SOURCE); + }); + }); +}); diff --git a/tests/fixtures/compact-content-benchmark-baseline.json b/tests/fixtures/compact-content-benchmark-baseline.json index 15e6db86a..4ac34364e 100644 --- a/tests/fixtures/compact-content-benchmark-baseline.json +++ b/tests/fixtures/compact-content-benchmark-baseline.json @@ -18,9 +18,9 @@ "reductionPct": 16.51 }, "execute-phase": { - "offTokens": 25827, - "onTokens": 23576, - "reductionPct": 8.72 + "offTokens": 25952, + "onTokens": 23701, + "reductionPct": 8.67 }, "new-project": { "offTokens": 14279, @@ -39,8 +39,8 @@ } }, "aggregate": { - "offTokens": 107286, - "onTokens": 90638, - "reductionPct": 15.52 + "offTokens": 107411, + "onTokens": 90763, + "reductionPct": 15.5 } } diff --git a/tests/fixtures/install-tree/antigravity.json b/tests/fixtures/install-tree/antigravity.json index e1c2ad18f..8fab0f6aa 100644 --- a/tests/fixtures/install-tree/antigravity.json +++ b/tests/fixtures/install-tree/antigravity.json @@ -594,6 +594,7 @@ "hooks/gsd-write-guard.js", "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/dispatch-identity.js", "hooks/lib/exit-code-registry.js", "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", diff --git a/tests/fixtures/install-tree/augment.json b/tests/fixtures/install-tree/augment.json index e00213de6..9a7826e7c 100644 --- a/tests/fixtures/install-tree/augment.json +++ b/tests/fixtures/install-tree/augment.json @@ -666,6 +666,7 @@ "hooks/gsd-write-guard.js", "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/dispatch-identity.js", "hooks/lib/exit-code-registry.js", "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", diff --git a/tests/fixtures/install-tree/claude-local.json b/tests/fixtures/install-tree/claude-local.json index 38b7516b6..255318f7a 100644 --- a/tests/fixtures/install-tree/claude-local.json +++ b/tests/fixtures/install-tree/claude-local.json @@ -530,6 +530,7 @@ "hooks/gsd-write-guard.js", "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/dispatch-identity.js", "hooks/lib/exit-code-registry.js", "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", diff --git a/tests/fixtures/install-tree/claude.json b/tests/fixtures/install-tree/claude.json index 544c1247d..fdcdeea90 100644 --- a/tests/fixtures/install-tree/claude.json +++ b/tests/fixtures/install-tree/claude.json @@ -594,6 +594,7 @@ "hooks/gsd-write-guard.js", "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/dispatch-identity.js", "hooks/lib/exit-code-registry.js", "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", diff --git a/tests/fixtures/install-tree/codebuddy.json b/tests/fixtures/install-tree/codebuddy.json index 2292bb9a3..e2c3a5953 100644 --- a/tests/fixtures/install-tree/codebuddy.json +++ b/tests/fixtures/install-tree/codebuddy.json @@ -666,6 +666,7 @@ "hooks/gsd-write-guard.js", "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/dispatch-identity.js", "hooks/lib/exit-code-registry.js", "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", diff --git a/tests/fixtures/install-tree/cursor.json b/tests/fixtures/install-tree/cursor.json index 3e4c1ee1b..ad852c0dc 100644 --- a/tests/fixtures/install-tree/cursor.json +++ b/tests/fixtures/install-tree/cursor.json @@ -572,6 +572,7 @@ "hooks/gsd-cursor-subagent-stop.js", "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/dispatch-identity.js", "hooks/lib/exit-code-registry.js", "hooks/lib/hook-exit.js", "hooks/lib/isolation-deny-reason.js", diff --git a/tests/fixtures/install-tree/hermes.json b/tests/fixtures/install-tree/hermes.json index ef90f0107..7f1925fe6 100644 --- a/tests/fixtures/install-tree/hermes.json +++ b/tests/fixtures/install-tree/hermes.json @@ -594,6 +594,7 @@ "hooks/gsd-write-guard.js", "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/dispatch-identity.js", "hooks/lib/exit-code-registry.js", "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", diff --git a/tests/fixtures/install-tree/kilo.json b/tests/fixtures/install-tree/kilo.json index 40d0afd3b..1b74815f5 100644 --- a/tests/fixtures/install-tree/kilo.json +++ b/tests/fixtures/install-tree/kilo.json @@ -666,6 +666,7 @@ "hooks/gsd-write-guard.js", "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/dispatch-identity.js", "hooks/lib/exit-code-registry.js", "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", diff --git a/tests/fixtures/install-tree/kimi-code.json b/tests/fixtures/install-tree/kimi-code.json index 2191a96df..a57222359 100644 --- a/tests/fixtures/install-tree/kimi-code.json +++ b/tests/fixtures/install-tree/kimi-code.json @@ -595,6 +595,7 @@ "hooks/gsd-write-guard.js", "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/dispatch-identity.js", "hooks/lib/exit-code-registry.js", "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", diff --git a/tests/fixtures/install-tree/opencode.json b/tests/fixtures/install-tree/opencode.json index 8aaa4c28c..d19d9419b 100644 --- a/tests/fixtures/install-tree/opencode.json +++ b/tests/fixtures/install-tree/opencode.json @@ -666,6 +666,7 @@ "hooks/gsd-write-guard.js", "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/dispatch-identity.js", "hooks/lib/exit-code-registry.js", "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", diff --git a/tests/fixtures/install-tree/pi.json b/tests/fixtures/install-tree/pi.json index c49b00b7e..1eaa3ff04 100644 --- a/tests/fixtures/install-tree/pi.json +++ b/tests/fixtures/install-tree/pi.json @@ -396,6 +396,7 @@ "gsd-hooks/gsd-write-guard.js", "gsd-hooks/lib/cli-exit.js", "gsd-hooks/lib/cursor-workspace.js", + "gsd-hooks/lib/dispatch-identity.js", "gsd-hooks/lib/exit-code-registry.js", "gsd-hooks/lib/filename-classification.js", "gsd-hooks/lib/git-cmd.js", diff --git a/tests/fixtures/install-tree/qwen.json b/tests/fixtures/install-tree/qwen.json index fe9df2925..5343f36b4 100644 --- a/tests/fixtures/install-tree/qwen.json +++ b/tests/fixtures/install-tree/qwen.json @@ -594,6 +594,7 @@ "hooks/gsd-write-guard.js", "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/dispatch-identity.js", "hooks/lib/exit-code-registry.js", "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", diff --git a/tests/gsd-agent-isolation-guard.test.cjs b/tests/gsd-agent-isolation-guard.test.cjs index a85b44007..782d7580b 100644 --- a/tests/gsd-agent-isolation-guard.test.cjs +++ b/tests/gsd-agent-isolation-guard.test.cjs @@ -55,7 +55,7 @@ const { toLegacyResult, gitOrThrow } = require('./helpers/git-fixture.cjs'); const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const { createTempDir, createTempProject, runGsdTools, cleanup } = require('./helpers.cjs'); const { SENTINEL_RELATIVE_PATH, SENTINEL_STALE_MS, readSentinel } = require('../hooks/lib/isolation-sentinel.js'); -const { REASON_CODE } = require('../hooks/lib/isolation-deny-reason.js'); +const { REASON_CODE, REASON_INTERPOLATION_MAX_LEN, sanitizeForReason, describeSentinelDiscard } = require('../hooks/lib/isolation-deny-reason.js'); const { runtimes } = require('../gsd-core/bin/lib/capability-registry.cjs'); const HOOK_PATH = path.join(__dirname, '..', 'hooks', 'gsd-agent-isolation-guard.js'); @@ -568,6 +568,93 @@ describe('gsd-agent-isolation-guard.js: #3045 SECURITY F2 — sentinel bound to }); }); +describe('gsd-agent-isolation-guard.js: #4594 rows 15/28-32 — prompt-first extraction + sentinel-discard reporting', () => { + let harnessProject; + + before(() => { + harnessProject = mkProject('gsd-aig-4594-'); + writeConfig(harnessProject, JSON.stringify({ runtime: 'claude' })); + }); + + after(() => { + cleanup(harnessProject); + }); + + test('row 29 (THE REGRESSION): fresh sentinel matches a real prose dispatch carried only in tool_input.prompt -> ALLOW', (t) => { + // Measured production shapes (not simplified): sentinel plan is + // phase-prefixed-and-slugged (`03-02-hardening`, phase-plan-index's + // `plans[].id`), the dispatch prose is the verbatim frame the workflow + // actually emits. `description` is deliberately OMITTED so this only + // passes when evaluateDispatch scans `prompt` — before this change the + // guard read only `description` and never saw this text at all. + writeSentinel(harnessProject, { isolation: 'none', phase: '03', plan: '03-02-hardening' }); + t.after(() => cleanup(path.join(harnessProject, '.gsd'))); + const r = runHook( + agentPayload({ tool_input: { subagent_type: 'gsd-executor', prompt: 'Execute plan 02 of phase 03-auth.' } }), + harnessProject, + ); + assert.equal(r.status, 0, `stdout: ${r.stdout} stderr: ${r.stderr}`); + assert.equal(r.stdout, ''); + }); + + test('row 28: unusable description + marker-bearing prompt, sentinel matches -> ALLOW', (t) => { + writeSentinel(harnessProject, { isolation: 'none', phase: '03', plan: '03-02-hardening' }); + t.after(() => cleanup(path.join(harnessProject, '.gsd'))); + const r = runHook( + agentPayload({ + tool_input: { + subagent_type: 'gsd-executor', + description: 'Run the executor', + prompt: '[gsd:dispatch phase="03" plan="03-02-hardening"] Execute the plan.', + }, + }), + harnessProject, + ); + assert.equal(r.status, 0, `stdout: ${r.stdout} stderr: ${r.stderr}`); + assert.equal(r.stdout, ''); + }); + + test('row 31: fresh sentinel for a DIFFERENT plan, isolation harness-worktree, kwarg missing -> DENY naming the discarded sentinel and both identifiers', (t) => { + writeSentinel(harnessProject, { + isolation: 'harness-worktree', + harnessFlag: 'isolation="worktree"', + phase: '03', + plan: '03-02-hardening', + }); + t.after(() => cleanup(path.join(harnessProject, '.gsd'))); + const r = runHook( + agentPayload({ + tool_input: { + subagent_type: 'gsd-executor', + prompt: '[gsd:dispatch phase="03" plan="07-01-x"] Execute the plan.', + }, + }), + harnessProject, + ); + assert.equal(r.status, 2, `stdout: ${r.stdout} stderr: ${r.stderr}`); + const out = JSON.parse(r.stdout); + assert.equal(out.decision, 'block'); + assert.match(out.reason, /sentinel/i); + // #4594 F4: the reason PROSE is not the contract (CONTRIBUTING.md + // "Prohibited: Raw Text Matching on Test Outputs") — assert the + // STRUCTURED `sentinel_discarded` field instead. + assert.deepEqual(out.sentinel_discarded, { + sentinel: { phase: '03', plan: '03-02-hardening' }, + dispatch: { phase: '03', plan: '07-01-x' }, + }); + }); + + test('row 32: no sentinel at all -> unchanged conservative fallback (DENY, registry resolves harness-worktree)', () => { + const r = runHook(agentPayload(), harnessProject); + assert.equal(r.status, 2, `stdout: ${r.stdout} stderr: ${r.stderr}`); + const out = JSON.parse(r.stdout); + assert.equal(out.decision, 'block'); + // No sentinel existed, so there is nothing to discard/report. + assert.doesNotMatch(out.reason, /was not consulted/); + assert.equal(out.sentinel_discarded, null); + }); +}); + describe('gsd-agent-isolation-guard.js: #3045 MAJOR — clock seam boundary coverage (in-process, no subprocess wall-clock race)', () => { const guardModule = require('../hooks/gsd-agent-isolation-guard.js'); @@ -1630,3 +1717,51 @@ describe('worktreesOptedOut — ladder unit semantics (#3972)', () => { assert.equal(worktreesOptedOut(dir), true, 'unreadable scoped config falls to the root view under the gate'); }); }); + +describe('hooks/lib/isolation-deny-reason.js — sanitizeForReason (#4594 F2/F5/F6)', () => { + test('boundary: 63 chars is not truncated', () => { + const value = 'a'.repeat(63); + assert.equal(sanitizeForReason(value), value); + assert.equal(REASON_INTERPOLATION_MAX_LEN, 64); + }); + + test('boundary: exactly 64 chars (the limit) is NOT truncated', () => { + const value = 'a'.repeat(64); + assert.equal(sanitizeForReason(value), value); + assert.equal(sanitizeForReason(value).includes('…'), false); + }); + + test('boundary: 65 chars is truncated to 64 chars plus an ellipsis', () => { + const value = 'a'.repeat(65); + const result = sanitizeForReason(value); + assert.equal(result, `${'a'.repeat(64)}…`); + assert.equal(result.length, 65); + }); + + test('F6: strips Unicode line/paragraph separators (U+2028/U+2029)', () => { + assert.equal(sanitizeForReason('before
after'), 'beforeafter'); + assert.equal(sanitizeForReason('before
after'), 'beforeafter'); + }); + + test('F6: strips bidi override/isolate control characters (U+202A-U+202E, U+2066-U+2069)', () => { + assert.equal(sanitizeForReason('‮evil‬'), 'evil'); + assert.equal(sanitizeForReason('‪evil‫‭'), 'evil'); + assert.equal(sanitizeForReason('⁦evil⁧⁨⁩'), 'evil'); + }); + + test('empty/non-string values render "(none)"', () => { + assert.equal(sanitizeForReason(''), '(none)'); + assert.equal(sanitizeForReason(null), '(none)'); + assert.equal(sanitizeForReason(undefined), '(none)'); + }); + + test('describeSentinelDiscard consumes the {sentinel, dispatch} shape from buildSentinelDiscard (#4594 F3)', () => { + const discard = { + sentinel: { phase: '03', plan: '03-02-hardening' }, + dispatch: { phase: '03', plan: '07-01-x' }, + }; + const message = describeSentinelDiscard(discard); + assert.match(message, /sentinel phase="03" plan="03-02-hardening"/); + assert.match(message, /dispatch phase="03" plan="07-01-x"/); + }); +});