From f0abdb1b89d73facb085b0e3ed64aa6ebc153fba Mon Sep 17 00:00:00 2001 From: Behruz Nassre Esfahani Date: Wed, 12 Aug 2026 17:45:08 -0700 Subject: [PATCH] fix(#2486): do not recommend or persist Claude-only worktree isolation on non-Claude runtimes (#2531) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2486): runtime-branch the settings worktrees question + W020 health diagnostic On non-Claude runtimes /gsd:settings offered "Yes (Recommended)" for worktree isolation and persisted workflow.use_worktrees: true — the exact value the execution workflows fail closed on (#1521 guards). Branch the question on the same stamped config-get runtime read the guards use: Claude keeps the unchanged question; non-Claude offers only "No (Recommended)" / "Leave unchanged", never persists true, and warns when the config carries an inherited explicit true. /gsd:health gains W020, surfacing such a config with the guards' own predicate before execution-time failure. Docs state the runtime-conditional default. Fixes #2486 Co-Authored-By: Claude Fable 5 * chore(#2486): add changeset for PR #2531 Co-Authored-By: Claude Fable 5 * fix(#2486): reassign the health worktrees check W020 -> W024 (verify.cts namespace collision) The workflow-level check collided with the live W020 (git-worktree-list health) emitted by cmdValidateHealth in src/verify.cts — invisible from health.md's error_codes table, which stops at W019 and under-represents the real namespace (W010-W017, W020-W023 all live). W024 verified free. Adds a regression test pinning the chosen code against src/verify.cts so a future assignment cannot silently collide, a table note naming the namespace owner, and the changeset body reworded to house style. Co-Authored-By: Claude Fable 5 * fix(#2486): pre-select the recommended repair in the broken-inheritance case Review round 2: at settings.md:142 the pre-selection rule left "Leave unchanged" as the default when the config carried an explicit non-false use_worktrees — the exact broken state the adjacent notice warns about, so accepting the default kept a config that fails closed at execution time. "Leave unchanged" is now the default only when the key is absent (nothing to repair); explicit false AND explicit non-false both pre-select "No (Recommended)", aligning the default, the label, and the notice. Pinned by two source-contract assertions in the #2486 regression block. Goldens (settings.md hash x19) + size baseline regenerated. Co-Authored-By: Claude Fable 5 * fix(#2486): gate the worktrees question on dispatch.isolation, not the runtime name Review round 2: #2584 Phase 3 replaced the runtime-name test with a declared `dispatch.isolation` capability, invalidating this PR's premise. cursor declares harness-worktree and codex/opencode/kimi/ kimi-code declare orchestrator-worktree, so a `RUNTIME != claude` gate blocked a supported configuration on five runtimes and false-warned in health. - settings.md + health.md read `query dispatch-isolation` and branch on `ISOLATION = none`; the runtime-name read is gone from both, and the capability read needs no per-runtime stamping (it fail-closes unknown/ undocumented internally) - all "Claude Code-only primitive" prose rewritten, including the two gates the shell-syntax check missed (config-key list, JSON schema comment) - W024 reconciled across health.md + CONFIGURATION.md + planning-config.md (docs still said W020, which collides with a verify.cts code) - health.md error-codes table fixed: the namespace note no longer sits between rows orphaning I001 - the asymmetry note for the two workflows #2584 has not migrated (quick.md, diagnose-issues.md) is enforced by a set-equality test with a self-check table, so it cannot go stale in either direction Co-Authored-By: Claude Fable 5 * docs(#2486): restore the Executor isolation section clobbered by #2661 `46ba02ac` (feat(#2630), the current next tip) reverted docs/CONFIGURATION.md to a pre-#2584 state: it restored the old "Non-Claude note" wording on the workflow.use_worktrees row and deleted the whole "Executor isolation per runtime" section. The change is unrelated to that PR's phase-estimation feature and looks like a stale-copy edit. This PR's use_worktrees row links to #executor-isolation-per-runtime, so the deletion leaves a dangling anchor. Restored byte-for-byte from a40ee8a5 (the text #2584 Phase 3 originally shipped). No link/anchor checker exists in scripts/ or tests/, so CI would not have caught the dead link. The reverted row wording is outside this PR's scope and is still wrong on next; reported upstream as #2668. Co-Authored-By: Claude Opus 5 (1M context) * test(#2486): acknowledge the emitted size growth for settings.md and health.md The #2723 differential attribution check (epic #2719 Phase 3) landed on next after this branch opened and gates emitted-file growth behind a committed acknowledgment. Both grown files are attributable to source this PR changes; the ack names them and says why, per ADR-2719 §3. Verified load-bearing: removing tests/emitted-drift-ack.json reproduces the same failure; restoring it passes 59/59. Co-Authored-By: Claude Opus 5 (1M context) * fix(#2486): reword the W024 remediation so it names no bare gsd-tools call The W024 warning told the user to run `gsd-tools query config-set`, a command-position bare invocation that fails with "command not found" on a shim-only install (#2751) — the diagnostic sent the user into a second, more confusing error than the one it reported. The remediation now names only `/gsd:settings` and the config key itself, both of which work on every install layout, and the #2751 command-position gate goes green. Fixes #2486 * fix(#2486): drop the Known-asymmetry note and its guard test — #2728 migrates both workflows Review Major 2: once #2728 lands, the note describes a gap that no longer exists and instructs maintainers not to do the thing that was just done — with the set-equality guard test pinning the stale prose green. This PR now depends on #2728 (declared in the PR body), so the note and its guard go. Co-Authored-By: Claude Fable 5 * test(#2486): make the #2728 merge order enforceable instead of advisory settings.md recommends and persists `workflow.use_worktrees: true` for every runtime whose declared dispatch.isolation is not `none` — cursor (harness-worktree) and codex/opencode/kimi/kimi-code (orchestrator-worktree). quick.md and diagnose-issues.md consume that value at dispatch time and, while they still gate on the runtime NAME, FATAL for all five. Merged first, this PR reintroduces #2486's own shape for the exact runtimes it exists to help — on the path it now labels "Recommended". The dependency was stated only as prose in the PR body. A merge-order note is not a gate: an automated batch merge never reads it. This adds the check that makes the ordering structural — red while either sibling is still name-gated, green the moment #2728 lands. The predicate matches a RUNTIME-vs-"claude" comparison, not the legitimate runtime-identity read, and accepts either the inline canonical dispatch-isolation read or a reference to dispatch-isolation-gate.md, which is the shape #2728 gives quick.md. Verified both directions against real sources: red against the current workflows, green against #2728's. This also closes W024's coverage window. W024 fires only when ISOLATION is `none`, so it is structurally blind to these five runtimes — /gsd:health would report healthy right up until quick.md FATALs. W024 cannot see the hazard, so the hazard is prevented by making the unsafe ordering unmergeable rather than by warning after the fact. Separately, the W-code namespace-collision test now declares its source read instead of passing lint silently: verify.cts emits its codes as inline string literals across ~25 addIssue() calls and exports no enumerable registry, so there is nothing to require() and assert against. The gap, and what it costs, are stated in the test. * docs(#2486): use the hyphen command form for /gsd-health in CONFIGURATION.md docs/ is never passed through the install-time slash-form converters, so the colon form names a command no runtime registers. Clears the sole lint-docs-command-form violation attributable to this PR. Co-Authored-By: Claude Opus 5 (1M context) * fix(#2486): resolve isolation via new side-effect-free inspect-dispatch-isolation query; scope the settings change to isolation-none runtimes Review round 4: - B1: /gsd:health and /gsd:settings no longer call the recording dispatch-isolation query — on current next it persists the resolved decision to the executor-isolation sentinel as an unconditional #3045 side effect, letting a read-only diagnostic hard-block executor dispatch for the sentinel's lifetime across sessions. Both surfaces now use inspect-dispatch-isolation, a new read-only verb sharing the exact resolution implementation (extracted resolveDispatchIsolationDecision) with zero writes. Behavioral tests pin: no sentinel write, per-runtime parity with the recording verb, recording knobs ignored, --json shape. - B2: the #2728 merge-order interlock test is deleted — a repo test cannot sequence merges; it only made this PR unmergeable on its own schedule. The settings behavior change is scoped entirely to the ISOLATION=none branch, which needs nothing from #2728; the != none path is base behavior unchanged. - M1: the W024-vs-verify.cts namespace test (an admitted source-grep) is deleted per RULESET.TESTS.delete-bad-tests, without a standing exemption; the namespace claim lives as guidance in health.md. - M2: remaining allow-test-rule exemptions re-derived into documented categories (source-text-is-the-product, integration-test-input) with issue refs per ADR-456. - Minor: settings.md's current-value read drops the stampable --default/fallback so key-absence stays distinguishable from an explicit false on non-Claude emits (the pre-selection rule depends on the tri-state); docs now present inspect-dispatch-isolation as the inspection command and name dispatch-isolation as the recording resolver. Co-Authored-By: Claude Fable 5 * fix(#2486): put the install-marker rung in the canonical runtime resolver Round-7 review (independent cross-AI pass over the whole PR). BLOCKER — the gate did not fire in the default case. `resolveRuntime` stopped at GSD_RUNTIME > config.runtime > 'claude', and `config-new-project` writes NO `runtime` key — so on a real non-Claude install every consumer believed it was on Claude: isolation reported `harness-worktree`, /gsd:settings still offered "Yes (Recommended)" and W024 stayed silent. That is #2486's own symptom, surviving the fix meant to remove it. Verified end-to-end on a real `--qwen` install with a runtime-neutral config and GSD_RUNTIME unset: pre-fix `harness-worktree`, fixed `none`. The rung lives in `resolveRuntime` (src/runtime-slash.cts), the ONE canonical resolver, not in the isolation call site. A per-consumer fix forks precedence: `inspect-dispatch-isolation` would answer `cursor` while `dispatch-should-flatten` and `resolve-dispatch-type` still answered `claude` for the same install. All three now agree. `readInstallRuntimeMarker`, its cache and its test seams MOVED from model-resolver.cts to runtime-slash.cts, with model-resolver re-exporting the seams — one marker read and one cache, not two that drift. Deliberately NOT `resolveActiveRuntime`/`loadConfig`: an intermediate revision of this fix routed through `loadConfig`, which normalizes and rewrites legacy keys back to disk. That gave `inspect-dispatch-isolation` — the verb whose entire purpose is being side-effect-free — a write side effect, which is the defect the verb exists to avoid. `resolveRuntime` reads .planning/config.json directly. Re-verified: the inspect query against a real install creates no `.gsd/`. Tests, both of which were too weak in the first attempt and are now fail-first proven: - The W024 behavioral stub returned success-with-empty-output for an absent key. Real `config-get` EXITS NON-ZERO, and the `|| echo "true"` fallback only triggers on failure — so reintroducing the fallback would have passed. The stub now returns 1 for the absent case. - The marker regression asserted on the exported helper, so reverting gsd-tools.cjs to a marker-blind resolver still passed. It now drives a real `--qwen` install through the shipped `inspect-dispatch-isolation` query. (GSD_TEST_MODE must be cleared for that child, or install.js no-ops while still exiting 0 — a green test over an install that wrote nothing.) Spawned through `installSpawnEnv()` so ambient GSD_HOME cannot leak in. Also: the three PR-added `try/finally` test bodies converted to `t.after()` per CONTRIBUTING; CONTEXT.md's `worktree create` UNCONSUMED claim corrected (Phase 3 calls it in executor-isolation-dispatch.md); docs/CONFIGURATION.md now states the current non-Claude `use_worktrees` default rather than describing capability-scoped stamping that lands with #2652; resolver precedence comments updated to name the marker rung. Refs #2486 Refs #2668 * fix(#2486): split the marker rung out; make the use_worktrees doc row order-independent Round-8 review. trek-e's Blocker was procedural — commit 10fba7b8 moved the per-install .gsd-runtime marker rung into the canonical resolver, a large blast-radius change that arrived undisclosed and unreviewed. They offered two remedies; taking the second: SPLIT IT OUT. src/runtime-slash.cts and src/model-resolver.cts are reverted to their next state and the marker regression test is removed. What remains is what this PR was filed for: the settings.md / health.md / gsd-tools.cjs isolation query, W024, and the #2668 docs restoration. COST, stated plainly rather than buried: without that rung this PR's gate resolves 'claude' on a non-Claude install whose project config carries no `runtime` key — which is every config config-new-project writes. On those installs W024 stays quiet and /gsd-settings still offers Worktrees. The gate is correct whenever the runtime IS resolvable (GSD_RUNTIME set, or an explicit config.runtime). That is why `Fixes #2486` is already downgraded to `Refs` — #2486 must not close until the residual lands. A KNOWN LIMITATION comment at the resolver call site names #2395 so this does not read as an oversight. The rung itself belongs to #2395, which reports this exact defect and was closed by #2446 — a PR that touched only bin/install.js and fixtures and never runtime-slash.cts, so it persisted the identity into ~/.gsd/defaults.json, a tier resolveRuntime does not read. Evidence and a reopen request are posted there. That same tier is #2566's B1. DOCS — the reviewer flagged that this row and #2728's are order-dependent: whichever merges second falsifies the other. Removed the dependency instead of picking an order. The "Current default … until #2652" paragraph is now a plain troubleshooting note, true before and after #2728 lands. CONFLICT — one hunk in CONTEXT.md: next added the #2596 scope-conformance interface to the same Worktree-Safety paragraph where this PR corrected the `worktree create` UNCONSUMED claim. Resolved keeping both. Verified: lint:ci green. Full suite clean apart from the pre-existing #1160 installed-runtime capability surface. (emitted-attribution also failed until the fork's stale next was fast-forwarded — it defaults to origin/next, which was 131 commits behind; 175/175 against the current base.) Refs #2486 Refs #2668 * fix(#2486): address round-9 review — distinguish "cannot resolve" from "declares none", reject recording-only args on the read verb Codex review of the whole PR, five findings, all verified against source first. Major 3 (the one real defect). Both surfaces read isolation as `ISOLATION=$(… || echo "none")`, so a resolver failure became indistinguishable from a genuine `dispatch.isolation: none` declaration — and W024's text then asserts the latter, telling a user their runtime declares no executor-isolation primitive when GSD simply could not find out. health.md and settings.md now capture the raw value and track ISOLATION_RESOLVED, the same shape references/dispatch-isolation-gate.md already uses (#2652 review). W024 gained a second message for the unresolved case; settings still fails closed there — it must never persist a `true` it cannot justify — but reports what happened rather than a verdict it never reached. Major 4. `dispatch-isolation` applies --force-isolation AFTER the shared resolver returns; inspection accepted the flag and silently ignored it, so the same argv yielded 'none' from one verb and the declared capability from the other. inspect-dispatch-isolation now rejects --force-isolation/--phase/--plan as usage errors. Verified by mutation: disabling the guard reds exactly the three new rejection tests. Major 1. settings.md claimed this flow and the execution guards "always reach the same verdict". False while quick.md still gates on the runtime name: an orchestrator-worktree host can be offered a `true` that /gsd:quick rejects as fatal, W024 silent because isolation is not none. Scoped the claim to the capability gate, named #2728 as the conversion, and added the same caveat to the CONFIGURATION.md use_worktrees row. Not a behavior change — the `!= none` branch is untouched base behavior. Major 2. "side-effect-free" overstated it: every gsd-tools invocation runs the shared bootstrap, and getActiveWorkstream unlinks a stale workstream pointer. That is pre-existing, verb-independent and cannot block a dispatch; writing the sentinel can. Renamed the claim to "sentinel-free" everywhere and documented precisely what is and is not asserted. Minor 5. The JSON parity test asserted against a handwritten key list and never invoked the recording verb, so the two could diverge and stay green. It now runs `dispatch-isolation --json` in a separate project dir and deep-compares, with a control asserting that verb DID write a sentinel. Added the orchestrator exec branch (--cwd-target) the registry parity test never covered. runtime-converters.test.cjs pinned the old `|| echo "none"` line as canonical — that literal was the Major 3 defect. Repinned as two invariants (the raw read is present; the collapsing fallback is absent) plus an ISOLATION_RESOLVED requirement, so reformatting does not fail the test but a semantic regression does. settings.md sits at 40777 bytes against the 40960 DEFAULT hard cap. The round-9 prose was compressed to fit rather than raising the cap. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next @ 33fca50d (#1160 _resolveManifest, and the #3053 quick_id tests, which compare a local-time expectation against a TZ=UTC child and so only pass on a UTC host). * fix(#2486): second review pass — drop a dangling reference, correct the whole unresolved branch, pin the branch behaviorally Codex re-review of the full PR after the first round-9 pass. It confirmed Majors 1, 2 and 4 and Minor 5 fixed, and found seven more. All verified against source. Minor 5 was mine and the worst of them: settings.md and health.md pointed at `gsd-core/references/dispatch-isolation-gate.md`, which exists only on the #2728 branch — not in this PR and not on the merge-base. Merging this alone would have shipped three dangling canonical-source references. Removed; the surrounding text now stands on its own. Major 2. The unresolved branch corrected only the two option descriptions. The pre-selection rationale still claimed an absent key already resolves to false on this runtime, and the explicit-true notice still stated the runtime declares no primitive — both capability verdicts that were never reached. The substitution rule now covers every place in that branch that asserts one. Major 3. The repinned invariants could not catch the mutation they existed to catch: flipping the shipped block's ISOLATION_RESOLVED=true to false left all three green, since they only assert the raw read is present, the token appears, and the collapsing fallback is gone. Added a behavioral test that drives the shipped W024 block twice — resolver answering vs resolver exiting non-zero — and asserts the two emit different text, that the unresolved branch says "could not resolve", and that it does NOT claim the runtime has no primitive. Verified: the true->false mutation now reds it. Major 1. The verb fail-closes an unknown or undeclared runtime to `none` and exits 0, so ISOLATION_RESOLVED=true means "the query answered", not "the runtime published a declaration" — the shell cannot see an internal fallback. Exposing provenance is an API change and out of scope here, so W024's resolved-case text is instead written to be true of every path that reaches it ("no usable executor-isolation primitive — declared or fail-closed from an unknown value"), and both workflows state the limit of the signal explicitly. Minor 4. The rejection tests regex-matched human prose, so swapping ERROR_REASON.USAGE for UNKNOWN would have kept them green while breaking machine consumers. They now pass --json-errors and assert reason === 'usage' on the parsed envelope. Minor 6. CONTEXT.md still described the verb as side-effect-free. Now sentinel-free, consistent with the router comment and the other two surfaces. Minor 7. 183 bytes of headroom under the 40960 hard cap was called unacceptable, and it was — a routine 184-byte edit would have failed CI. Compressed this PR's own settings.md prose (the #2486 block was carrying ~7.1KB of rationale) to 40499 bytes, 458 free. Two phrases other tests pin verbatim were restored after the first compression pass reworded them. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next @ 33fca50d. * fix(#2486): third review pass — placeholder table replaces the fragile substitution rule Codex round 3 blocked the push on two Majors. Both verified and real. Major 2 was the substantive one and my error. The round-2 fix told the model to find and replace the string "this runtime declares no executor-isolation primitive (dispatch.isolation: none)" — text that does not appear anywhere in the branch. The first option description has no parenthetical, the second asserts "absent, it already resolves to false on this runtime" (not covered at all), and the pre-selection rationale and notice carried three more unsupported assertions. A model following the instruction literally would have found no match and changed nothing. Replaced with three named placeholders — {FINDING}, {CONSEQUENCE}, {ABSENCE} — and a two-column table giving each one's resolved and unresolved wording. Every assertion in the branch now flows from the table, so none of them can outrun what was actually established, and there is no string-matching to get wrong. It is also shorter than the prose it replaced. Major 1: the resolver catches thrown errors and returns none successfully, a path the round-2 wording ("declared, or unknown/undeclared") did not cover. Both surfaces now say "declared as none, or fail-closed because the capability could not be determined", which is true of the thrown path too, and both state that an internal resolution error is among the things the verb fail-closes. Still not provenance — the verb cannot distinguish these for the caller — but no longer a claim the code contradicts. Minor 3: the behavioral test asserted only that the two branches differ and that the resolved one omits "could not resolve", so arbitrary resolved text stayed green. Now pins what it must positively say: the capability finding, the fail-closed consequence, the offending key, and (both branches) the repair command. Minor 4: two stale artifacts the round-2 sweep missed — a test comment still citing the #2728-only references/dispatch-isolation-gate.md, and the emitted drift ack still describing inspection as side-effect-free. Also aligned the two remaining router comments to sentinel-free. settings.md 40420 bytes, 540 free under the cap. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next @ 33fca50d. * fix(#2486): fourth review pass — stop recommending a repair whose effect depends on the emit Codex round 4, one Major and three Minors. The Major is a real hole and worth the round. W024 advised "remove the key from .planning/config.json so the runtime default (false) applies". That default is not false everywhere: execute-phase.md:102, quick.md:155 and diagnose-issues.md:62 all read the key as `|| echo "true"`, and only `_stampNonClaudeRuntimeDefaults` rewrites them to `--default false` on a non-Claude emit. So on an un-stamped emit the advised repair leaves use_worktrees resolving to TRUE against isolation none — exactly the state W024 exists to flag. Reachable in the round-3 gap: a caught internal resolver error returns none with rc=0, so ISOLATION_RESOLVED is true and the confident branch fires on a host whose emit was never stamped. Fixed by removing the dependence rather than the symptom: both surfaces now say to set workflow.use_worktrees false explicitly, and say why deleting the key is not equivalent. The {ABSENCE} placeholder no longer claims absence resolves to false unconditionally — it is scoped to an emit that stamped that default. Minor: the notice and W024 hardcoded .planning/config.json, which is the wrong file in an active workstream (settings itself resolves $GSD_CONFIG_PATH correctly). Both now say "the project config", naming the workstream case. Minor: the new repair pin required the literal /gsd:settings, a form documented as no longer routable. Relaxed to accept the canonical slash and $-prefixed forms so a correct rewording cannot fail the test. Nit: two test descriptions still said side-effect-free. Not fixed, deliberately: the verb still cannot tell a caught resolver error from a declared none, so ISOLATION_RESOLVED remains a signal about the CALL, not the declaration. Both workflows now state that limit outright. Exposing provenance is a change to the query contract that belongs with the runtime-resolution work in #2395, not here. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next @ 33fca50d. The only change after that suite run was removing a redundant regex escape flagged by eslint; that test was re-run individually and eslint is clean. * fix(#2486): fifth review pass — stop recommending "leave it absent" as the safe default Codex round 5, one Major: the round-4 fix corrected the {ABSENCE} wording but left the pre-selection rule built on the assumption it had just falsified. An absent key still pre-selected "Leave unchanged" on the rationale that there was nothing to repair. Absence resolves to false only where the emit stamped that default; on an un-stamped emit it reads as true, so accepting the recommended default could preserve the exact isolation-none-plus-true state this branch exists to prevent. The isolation-none branch now pre-selects "No (Recommended)" in every case, including an absent key. Writing an explicit false is correct under either emit; "Leave unchanged" stays available for the deliberate shared-config case but is never the default. The test that pinned the old rule pinned a falsified assumption, so it now pins the new one and asserts the old sentence is gone. Also tightened the round-4 repair regex, which had been relaxed far enough to accept `$gsd:settings` and `/gsd:settings-bogus`. It now matches only the canonical `/gsd-settings`, `/gsd:settings` and `$gsd-settings` forms. settings.md 40685 bytes, 275 free. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next @ 33fca50d. * fix(#2486): make the inspection exemption block-scoped, and drop the last pre-#2728 claim Codex review of the conflict resolution: one Major, two Minors, all real. Major. The inspection-surface exemption I added to #2728's "every dispatch-site degrade block re-records" guard was FILE-wide. Codex probed it by adding an unrecorded executor block to health.md: the exemption assertions still passed, the whole file was skipped, and the offender went unreported. That is the same hole the hand-listed revision of this guard had — a promise of "every dispatch site" with a silent carve-out. Now scoped per block. A block earns the exemption only by resolving through `inspect-dispatch-isolation` AND carrying no dispatch primitive (`Agent(`, `harnessFlag`/`HARNESS_FLAG`, `isolation="worktree"`, or the recording verb). The file-level assertions remain as a precondition on top. Verified with Codex's own probe: the appended dispatch block is now reported at health.md:285, while the legitimate inspection block still passes. Minor. settings.md still carried the caveat saying `quick.md` gates on the runtime name and "#2728 converts that surface". #2728 has landed. Same obsolete premise already removed from docs/CONFIGURATION.md in the merge commit; this was the copy I missed. Removing it also returns 413 bytes, so settings.md now has 688 free under the 40960 cap rather than 275. Minor. Four scratch-directory prefixes still read `w024` after the renumber. Non-functional, but the whole point of the rename was that W024 now means someone else's warning. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next. * fix(#2486): name the diagnostics' state INSPECTED_ISOLATION and delete the exemption Codex probed the per-block exemption and it was still escapable: DISPATCH_PRIMITIVE matched only `Agent(`, two harness-flag spellings, `isolation="worktree"` and the recording verb, so `Task(`, `spawn_agent(`, a `codex exec` process dispatch, and — unfixably by any regex — the repo's normal split shape (isolation resolved in one bash fence, dispatch in a later one) all still qualified as exempt. Took Codex's suggested fix, which is better than the one it replaces. The diagnostics now name their state `INSPECTED_ISOLATION` / `INSPECTED_RESOLVED` / `_INSPECTED_RAW` instead of reusing the dispatch-site names. #2728's guard scans for a literal `ISOLATION=none`, so health.md and settings.md fall outside it BY CONSTRUCTION and the exemption is deleted outright — no carve-out to escape, and a diagnostic that ever writes a real `ISOLATION=none` is caught like any other site. The name is also just more accurate: an inspection result is not a dispatch decision. Pinned so the reasoning cannot be lost: the #2486 suite now asserts neither diagnostic assigns the bare `ISOLATION` name, with the rationale in the comment. Verified by mutation — renaming back makes #2728's guard flag health.md:107 AND fails the new naming pin. Net effect on the guard's coverage is positive: before this PR it scanned two fewer files by exemption; now it scans everything. settings.md 40352 bytes, 608 free. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next. --------- Co-authored-by: Claude Fable 5 --- .changeset/sturdy-birds-chatter.md | 5 + CONTEXT.md | 4 +- docs/CONFIGURATION.md | 22 +- gsd-core/bin/gsd-tools.cjs | 158 ++++-- gsd-core/references/planning-config.md | 2 +- gsd-core/workflows/health.md | 52 ++ gsd-core/workflows/settings.md | 68 ++- .../2486-settings-worktrees-runtime.json | 6 + .../2573-state-head-freshness.json | 2 +- tests/gsd-agent-isolation-guard.test.cjs | 198 ++++++++ tests/host-integration.test.cjs | 9 + tests/runtime-converters.test.cjs | 459 ++++++++++++++++++ 12 files changed, 940 insertions(+), 45 deletions(-) create mode 100644 .changeset/sturdy-birds-chatter.md create mode 100644 tests/emitted-drift-acks/2486-settings-worktrees-runtime.json diff --git a/.changeset/sturdy-birds-chatter.md b/.changeset/sturdy-birds-chatter.md new file mode 100644 index 000000000..6f1ffe8f6 --- /dev/null +++ b/.changeset/sturdy-birds-chatter.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2531 +--- +**Settings no longer offer worktree isolation on runtimes that cannot honor it, and health warns before execution fails closed** — previously `/gsd-settings` recommended "Yes" and persisted `workflow.use_worktrees: true` on every runtime, handing installs whose declared `dispatch.isolation` capability is `none` the exact value `/gsd-execute-phase` and `/gsd-quick` fail closed on. On those runtimes the Worktrees question now offers "No (Recommended)" / "Leave unchanged" (never an enabling option), warns when the config carries an inherited explicit `true`, and `/gsd-health` surfaces such a config as new warning W025 with a DEGRADED status before execution-time failure. Runtimes that declare `harness-worktree` or `orchestrator-worktree` are unaffected — the gate is the declared capability, never the runtime name. Both surfaces resolve isolation through the new `inspect-dispatch-isolation` query, a sentinel-free sibling of `dispatch-isolation`: the dispatch verb records its decision to the executor-isolation sentinel by design, which a read-only diagnostic must never trigger. The inspection verb rejects `--force-isolation`, `--phase` and `--plan` as usage errors rather than accepting and ignoring them — the recording verb applies `--force-isolation` after resolution, so silently ignoring it would hand the same argv two different answers. Both surfaces also distinguish "this runtime declares no isolation primitive" from "the capability could not be resolved", and say which one happened instead of reporting a resolver failure as a capability verdict. (#2486) diff --git a/CONTEXT.md b/CONTEXT.md index aba850889..f96415a51 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -138,7 +138,7 @@ Diagnostic-output convention for the Resolution Provenance principle (ADR-1411 P Leaf module owning the **out-of-band** half of ADR-1411's "corrupt is not absent" amendment (epic #1879). Where a read already returns a provenance envelope the cause is named in-band (`ConfigResolution.reason`, #1880); where a read returns a bare sentinel or a plausible default it cannot extend, the return value is preserved exactly and the cause is surfaced here instead. Interface: `UNUSABLE_REASON` (frozen reason enum — one entry per condition that has an emitting call site; adding a reason is three coordinated changes: enum + call site + the test locking `Object.keys(...).sort()`), `warnUnusableInput({reason, source?, content?}) → boolean` (returns whether this call actually wrote, so tests assert emission *counts* on a typed surface rather than scraping stderr), plus the `_resetUnusableInputWarningsForTests` / `_unusableInputWarningCountForTests` seams. Dedup key is `\0` — **both halves are load-bearing**: keying on the path alone would let a second, different fault on the same file go unreported, and keying on message prose would couple the guard to wording (ADR-1411 dedup clause). Path separators are deliberately **not** normalized: an earlier revision folded backslashes to `/` so two spellings of one Windows path would not double-report, but a backslash is a legal filename character on Linux and macOS, so that folding collapsed two genuinely distinct POSIX files onto one key and swallowed the second file's diagnostic. The trade is now one-directional — two spellings of one Windows path may report twice (noise), but two distinct files can never silence each other (lost signal), and ADR-1411 ranks the swallow the worse failure; ASCII control characters are stripped from the source before it is keyed or written, because the key separator is NUL (a crafted path could otherwise forge a collision) and because a path carrying ANSI escapes would replay into the operator's terminal. Callers with no path (in-memory content) fall back to a short content digest so *different* bad inputs still key differently. The diagnostic is **unconditional** — a deliberate divergence from ADR-227's never-implemented `GSD_DEBUG` opt-in, since "an opt-in nobody sets is indistinguishable from the silence #1879 is about" — and **never throws**: a failed stderr write is swallowed so a degraded read is never escalated into a crash. Adopted by `extractFrontmatter` (#1882, `frontmatter_unterminated`) and by `getRoadmapPhaseInternal`/`getMilestoneInfo` (#1881, `roadmap_unreadable`); `planning-workspace`/`verify` (#1883) follow. #1881 detects on the errno alone: `platformReadSync` returns `null` for ENOENT and its callers convert that to an errno-less Error, so reporting unconditionally in those catches would flag every project without a ROADMAP.md as corrupt. Exists as a shared seam rather than a per-site copy because four sites need identical behavior and four hand-rolled copies is `RULESET.GENERATIVE-FIX` by construction. Source of truth: `gsd-core/bin/lib/unusable-input.cjs` (generated from `src/unusable-input.cts`). Test anchor: `tests/unusable-input.test.cjs`. See Resolution Provenance, Config Loader Module. ### Worktree Safety Policy Module -CJS Module owning worktree lifecycle safety policy for the GSD orchestration layer. Interface: `resolveWorktreeContext(cwd, deps) → WorktreeContext` (linked-worktree root mapping), `parseWorktreePorcelain(output) → WorktreeEntry[]` (porcelain parser, skips detached HEAD), `planWorktreePrune(repoRoot, opts, deps) → PrunePlan` (metadata-prune plan, never destructive by default), `executeWorktreePrunePlan(plan, deps) → PruneResult` (executes prune; degrades gracefully on git timeout), `listLinkedWorktreePaths(repoRoot, deps) → LinkedPathsResult`, `inspectWorktreeHealth(repoRoot, opts, deps) → HealthResult` (orphan + stale detection), `snapshotWorktreeInventory(repoRoot, opts, deps) → InventoryResult`, `planWorktreeWaveCleanup(repoRoot, manifest) → CleanupPlan` (manifest-scoped, fail-closed), `executeWorktreeWaveCleanupPlan(plan, deps) → CleanupResult` (per-entry gauntlet: branch → base → deletions → **advisory scope conformance (#2596)** → SUMMARY-rescue → clean-worktree → merge → remove; the scope check compares the branch's committed diff against the entry's declared `files_modified` and appends `WAVE_CLEANUP_WARNING`-coded entries to a `warnings` channel WITHOUT touching `ok` — an advisory, not a gate, and skipped entirely with no git call when no scope was declared), `planWaveScopeConformance(changedPaths, declaredFiles, branch) → WaveCleanupWarning[]` (pure; literal-prefix path coverage deliberately mirroring the submodule-intersection gate's glob-prefix rule rather than introducing a second matcher; over-accepts by design because a false alarm costs an advisory more than a miss), `isSummaryArtifactRelPath(relPath) → boolean` (the single definition of "executor-written SUMMARY artifact", shared with `defaultFindSummaryFiles` so the rescue walker and the scope exemption cannot drift), `WAVE_CLEANUP_WARNING` (frozen advisory-code enum: `scope_out_of_declared`, `scope_check_unavailable`), `planWorktreeRecordAgent(manifestRaw, fields) → RecordAgentPlan` (write-strict per-agent manifest append; validates each field at write time via the same `normalizeCleanupManifestEntry` rules the reader enforces; fail-closed on a missing/garbled field or a duplicate `(worktree_path, branch)` the reader would dedup away), `cmdWorktreeRecordAgent(cwd, args, deps) → RecordAgentCmdResult` (thin deps-injectable IO wrapper for the `worktree record-agent` verb), `planWorktreeCreate(fields) → WorktreeCreatePlan` (write-strict `worktree create` planner — same missing-field-hint and `normalizeCleanupManifestEntry` validation as `planWorktreeRecordAgent`, pure/no-git), `executeWorktreeCreatePlan(plan, repoRoot, deps) → WorktreeCreateResult` (bounded `git rev-parse --verify` base check THEN `git worktree add -b `; fail-closed `base_unresolved`/`git_timeout`/`worktree_add_failed`; returns `cwd` — the working directory an executor spawn would use), `cmdWorktreeCreate(cwd, args, deps) → WorktreeCreateCmdResult` (CLI verb: requires `--root` — confinement is mandatory, not opt-in; omitting it fails closed with `reason:'root_required'` before any git side effect, rather than silently creating an unconfined worktree (#3050); plans, creates the worktree, then appends the manifest entry so it is immediately manageable by cleanup-wave/reap-orphans; dedupes by `(worktree_path, branch)`). #2584 ADR-1239 Codex-binding amendment, Phase 2: `worktree create` is the git-worktree-creation primitive for `dispatch.isolation: orchestrator-worktree` hosts — declared and testable but UNCONSUMED (no scheduler calls it yet; Phase 3 wires it). `worktree record-agent` / `worktree create` accept an optional `--files` recording the plan's declared scope, consumed by the advisory scope-conformance check above; a blank or omitted value leaves the 4-field on-disk entry shape unchanged. Source of truth: `gsd-core/bin/lib/worktree-safety.cjs`. Timeout path: all git subprocess calls are bounded; callers receive `ok:false, reason:'git_timed_out'` rather than a thrown exception. Test anchor: `tests/worktree-safety.test.cjs`. The `core.cjs` re-export spine was retired in epic #1267: this module absorbed the two thin compositional wrappers that squatted in Core — `resolveWorktreeRoot(cwd, deps) → {root, reason}` (a projection over `resolveWorktreeContext`; returns the `reason` alongside `root` — a `git_timed_out` reason means `root` is a best-effort cwd fallback, not a confirmed resolution, and callers must surface that risk rather than trust it silently, #3050) and `pruneOrphanedWorktrees(...)` (sequences `planWorktreePrune` + `executeWorktreePrunePlan` with a timeout warning) — so callers reach this single worktree-lifecycle seam directly. `gitWorktreeInfoInternal` did NOT move here — worktree-info detection belongs to the Git Query Module. +CJS Module owning worktree lifecycle safety policy for the GSD orchestration layer. Interface: `resolveWorktreeContext(cwd, deps) → WorktreeContext` (linked-worktree root mapping), `parseWorktreePorcelain(output) → WorktreeEntry[]` (porcelain parser, skips detached HEAD), `planWorktreePrune(repoRoot, opts, deps) → PrunePlan` (metadata-prune plan, never destructive by default), `executeWorktreePrunePlan(plan, deps) → PruneResult` (executes prune; degrades gracefully on git timeout), `listLinkedWorktreePaths(repoRoot, deps) → LinkedPathsResult`, `inspectWorktreeHealth(repoRoot, opts, deps) → HealthResult` (orphan + stale detection), `snapshotWorktreeInventory(repoRoot, opts, deps) → InventoryResult`, `planWorktreeWaveCleanup(repoRoot, manifest) → CleanupPlan` (manifest-scoped, fail-closed), `executeWorktreeWaveCleanupPlan(plan, deps) → CleanupResult` (per-entry gauntlet: branch → base → deletions → **advisory scope conformance (#2596)** → SUMMARY-rescue → clean-worktree → merge → remove; the scope check compares the branch's committed diff against the entry's declared `files_modified` and appends `WAVE_CLEANUP_WARNING`-coded entries to a `warnings` channel WITHOUT touching `ok` — an advisory, not a gate, and skipped entirely with no git call when no scope was declared), `planWaveScopeConformance(changedPaths, declaredFiles, branch) → WaveCleanupWarning[]` (pure; literal-prefix path coverage deliberately mirroring the submodule-intersection gate's glob-prefix rule rather than introducing a second matcher; over-accepts by design because a false alarm costs an advisory more than a miss), `isSummaryArtifactRelPath(relPath) → boolean` (the single definition of "executor-written SUMMARY artifact", shared with `defaultFindSummaryFiles` so the rescue walker and the scope exemption cannot drift), `WAVE_CLEANUP_WARNING` (frozen advisory-code enum: `scope_out_of_declared`, `scope_check_unavailable`), `planWorktreeRecordAgent(manifestRaw, fields) → RecordAgentPlan` (write-strict per-agent manifest append; validates each field at write time via the same `normalizeCleanupManifestEntry` rules the reader enforces; fail-closed on a missing/garbled field or a duplicate `(worktree_path, branch)` the reader would dedup away), `cmdWorktreeRecordAgent(cwd, args, deps) → RecordAgentCmdResult` (thin deps-injectable IO wrapper for the `worktree record-agent` verb), `planWorktreeCreate(fields) → WorktreeCreatePlan` (write-strict `worktree create` planner — same missing-field-hint and `normalizeCleanupManifestEntry` validation as `planWorktreeRecordAgent`, pure/no-git), `executeWorktreeCreatePlan(plan, repoRoot, deps) → WorktreeCreateResult` (bounded `git rev-parse --verify` base check THEN `git worktree add -b `; fail-closed `base_unresolved`/`git_timeout`/`worktree_add_failed`; returns `cwd` — the working directory an executor spawn would use), `cmdWorktreeCreate(cwd, args, deps) → WorktreeCreateCmdResult` (CLI verb: requires `--root` — confinement is mandatory, not opt-in; omitting it fails closed with `reason:'root_required'` before any git side effect, rather than silently creating an unconfined worktree (#3050); plans, creates the worktree, then appends the manifest entry so it is immediately manageable by cleanup-wave/reap-orphans; dedupes by `(worktree_path, branch)`). #2584 ADR-1239 Codex-binding amendment, Phase 2: `worktree create` is the git-worktree-creation primitive for `dispatch.isolation: orchestrator-worktree` hosts — consumed since #2584 Phase 3 — `executor-isolation-dispatch.md` calls it to create the worktree an `orchestrator-worktree` host is then process-spawned into. `worktree record-agent` / `worktree create` accept an optional `--files` recording the plan's declared scope, consumed by the advisory scope-conformance check above; a blank or omitted value leaves the 4-field on-disk entry shape unchanged. Source of truth: `gsd-core/bin/lib/worktree-safety.cjs`. Timeout path: all git subprocess calls are bounded; callers receive `ok:false, reason:'git_timed_out'` rather than a thrown exception. Test anchor: `tests/worktree-safety.test.cjs`. The `core.cjs` re-export spine was retired in epic #1267: this module absorbed the two thin compositional wrappers that squatted in Core — `resolveWorktreeRoot(cwd, deps) → {root, reason}` (a projection over `resolveWorktreeContext`; returns the `reason` alongside `root` — a `git_timed_out` reason means `root` is a best-effort cwd fallback, not a confirmed resolution, and callers must surface that risk rather than trust it silently, #3050) and `pruneOrphanedWorktrees(...)` (sequences `planWorktreePrune` + `executeWorktreePrunePlan` with a timeout warning) — so callers reach this single worktree-lifecycle seam directly. `gitWorktreeInfoInternal` did NOT move here — worktree-info detection belongs to the Git Query Module. ### Worktree Lifecycle Module Workflow contract seam covering agent worktree lifecycle orchestration rules. The `worktree_branch_check` block lives in one canonical fragment (`gsd-core/references/worktree-branch-check.md`) that `execute-phase.md`, `quick.md`, `diagnose-issues.md`, and `execute-plan.md` embed at dispatch. Key invariants: `worktree_branch_check` is **verify-only and fail-closed** — the orchestrator owns worktree lifecycle and base recovery, so the sub-agent holds no state-correction primitives; HEAD attachment verified via `git symbolic-ref`; positive allow-list `^worktree-agent-*` enforced; `git update-ref` on protected refs is prohibited; on base mismatch the sub-agent halts with `exit 42` and surfaces to the orchestrator (#48); the orchestrator runs a cwd-drift guard at `execute_waves` entry that resolves the worktree root and refuses drift into an agent worktree (#48); #1856: that refusal now also reports what the agent worktree holds — commits ahead of the resolved base and uncommitted files, both with true counts plus a truncation notice — and the commit/switch/merge-or-cherry-pick sequence to integrate them, because `re-run from the orchestrator worktree` alone silently meant abandoning work that lives only on the agent branch. The refusal condition and exit code are unchanged, and every added command is diagnostic and `|| true`-guarded so a failure degrades to the plain refusal; cleanup is manifest-scoped (`WAVE_WORKTREE_MANIFEST`) not global-discovery-based; worktree spawning is sequential (one `run_in_background` at a time to avoid `config.lock` contention). Test anchor: `tests/worktree.test.cjs`. @@ -159,7 +159,7 @@ Module owning runtime identity normalization at runtime-selection seams. Canonic Pure, no-write Module owning the **detection rung** of runtime identity — ADR-2313 Phase 5 (#3245, folded from #2320). The Runtime Name Policy Module normalizes the two *explicit* signals (`GSD_RUNTIME`, `.planning/config.json:runtime`); this module answers the different question those two cannot: *which host is this process actually running inside* when neither is set. Before it, `init` reported `agent_runtime: claude` inside a Codex session, because the ladder ended at a hardcoded default. `detectHostRuntime(deps?) → {runtime, source, signal}` is the typed surface tests assert against (`source` ∈ `session-env|config-home|none`); it probes, in order, the frozen `CODEX_SESSION_ENV_SIGNALS` table (`CODEX_SANDBOX`, `CODEX_SANDBOX_NETWORK_DISABLED` — injected by Codex into shell-tool children per openai/codex `AGENTS.md`; absent under `sandbox_mode = "danger-full-access"`, so best-effort), then an explicitly-exported `CODEX_HOME` whose `config.toml` exists — the marker FILENAME is single-sourced from the Update-Context Module's `inferPreferredRuntime` (`CODEX_CONFIG_MARKER`, re-exported here), while the TRUTHINESS RULE deliberately differs: `inferPreferredRuntime` accepts a bare, unchecked `CODEX_HOME` as sufficient to resolve an update context, whereas this module additionally requires the marker file to exist, because it asserts session identity rather than resolving an update context and needs the stronger signal — a difference pinned by a test in `tests/host-runtime-detection.test.cjs` rather than left implicit. `resolveReportedRuntime(projectDir, deps?)` composes the whole ladder: `GSD_RUNTIME` > config `runtime` > detection > `'claude'`. Three invariants are load-bearing and each has a test: it **never writes** (#2297 shared-defaults poisoning — no `~/.gsd/defaults.json`, no config mutation); it **never shells out**, so there is no subprocess to time-bound and no degraded-on-timeout path to design; and the **default `~/.codex/config.toml` is never probed**, because every machine that has run Codex carries that file and probing it would misreport Claude Code sessions as codex. _Avoid_: calling this "runtime resolution" — `resolveRuntime` (Runtime Slash Module) keeps its own frozen `GSD_RUNTIME > config > 'claude'` contract and its 71 dependents, including `formatGsdSlash`'s command-style decision, are deliberately untouched; only `withProjectRoot`'s reported `agent_runtime` consumes the detection rung. Sources: `src/host-runtime-detection.cts` → `gsd-core/bin/lib/host-runtime-detection.cjs`; the `resolveExplicitRuntime` seam it composes lives in `src/runtime-slash.cts`. See ADR-2313. ### Host-Integration Interface -Pure, additive, no-I/O Module owning the versioned, negotiated contract over the six host-integration interface points (command, dispatch, model, hooks, state, artifact) — ADR-1239 Phase A. Extends the ADR-1016 runtime descriptor with nine closed-vocabulary axes carried under `capability.json` `runtime.hostIntegration`: `embeddingMode` (`imperative|declarative`), `commandSurface` (`slash-file|slash-programmatic|slash-toml|palette|prose-only`), `dispatch` (`{namedDispatch,nested,maxDepth,background,backgroundDispatch,subagentToolkit,isolation}`), `modelMode` (`active|passive`), `hookBus` (`host|engine|none`), `stateIO` (`filesystem|sandboxed-storage|session-log-append`), `transport` (`mcp|native-extension`), `runtime` (`node|bun|sandboxed-web|python|go|rust|electron|other`), `effortSurface` (`argv|none` — how reasoning effort reaches the host; ADR-1239 amendment #2481, the first axis whose consumer is an invocation-time argument rather than an install-time artifact). `dispatch.isolation` (`harness-worktree|orchestrator-worktree|none` — how a host isolates concurrent same-wave executors; ADR-1239 Codex-binding amendment #2584; consumed by the phase scheduler since #2584 Phase 3 and, since #2652, by every single-agent dispatch site — `quick.md`, `diagnose-issues.md`, `execute-plan.md` — which resolve it through the canonical `gsd-core/references/dispatch-isolation-gate.md` rather than branching on a runtime id). `resolveOrchestratorExec(orchestratorExec, cwd) → { ok:true, command, args, cwd } | { ok:false, reason }` (#2584 Phase 2, pure, no I/O — resolves the `runtime.orchestratorExec` descriptor field, a sibling of `runtime.hostBehaviors` in `capability.json` carrying `{command, args?, cwdFlag?}`, into the concrete argv/cwd a process-spawn primitive would use for a `dispatch.isolation: orchestrator-worktree` host; appends `[cwdFlag, cwd]` to `args` when `cwdFlag` is a non-empty string, e.g. codex `exec --cd `, opencode `run --dir `, kimi `--work-dir `; when `cwdFlag` is `null`/absent — kimi-code's process-cwd case — no flag is appended and `cwd` alone is returned for the caller to bind via the subprocess's own working-directory option; fail-closed `missing_command`/`invalid_cwd`/`invalid_args`/`invalid_cwd_flag`; CONSUMED since #2584 Phase 3 — `routeDispatchIsolation` resolves it into the `exec` field of `gsd_run query dispatch-isolation --json`, and `gsd-core/workflows/execute-phase/steps/executor-isolation-dispatch.md` process-spawns that `command`/`args`/`cwd`; #2652 adds a second consumer, `_negotiatedDispatchIsolation`, which probes it at install time against a placeholder target to decide whether an `orchestrator-worktree` declaration actually resolves). Interface: `negotiateHostCapabilities(host, engine?) → { protocolVersion, effective, points, warnings }` enforcing the trust-boundary invariant `effective ⊆ host-declared ∩ engine-known` (never augment with an undeclared or unknown/future-`protocolVersion` value — fail-closed via the most-restrictive-known `SAFE_DEFAULTS`); `degradationFor(point, axes) → { level, fallback }` (a pure Full/Degraded/Absent ladder table, never throws); `profileOf(axes) → 'programmatic-cli'|'declarative-cli'|'ide'|null`; plus `PROTOCOL_VERSION` (integer, starts at 1 — distinct from the package `version`/`engines.gsd` semver), `HOST_INTEGRATION_AXES` (the frozen closed vocabulary, single source of truth), `PROFILE_BASELINES`, and `shouldFlattenDispatch(dispatch) → boolean` (ADR-1239 Phase B / #1708 — graduates the #853 rule: returns `true` = run the orchestrator inline UNLESS the host is documented to background a nesting-capable orchestrator (`background === true && backgroundDispatch === true`); fail-closed to inline; exposed to the plan/execute workflows via the `gsd_run query dispatch-should-flatten --raw` CLI, which replaced the former scattered `RUNTIME === 'codex'` prose check). The runtime-descriptor validator (`gsd-core/bin/lib/capability-validator.cjs` `validateRuntimeBody`) mirrors the closed vocabulary inline (exported as `_HOST_INTEGRATION_VOCAB`) and is kept in lock-step by the parity guard `tests/host-integration-validator-parity.test.cjs`. Orthogonal axes (resolved explicitly per ADR-1239 Phase A): `commandStyle` (GSD emission style, retained) vs `commandSurface` (host surface type); `hookEvents` dialect vs `hookBus` ownership (a host with `hooksSurface:none` may still be `hookBus:host` — e.g. opencode); `runtimeCompat` (feature→host) vs these negotiated runtime→engine axes. Phase A defined the interface; Phase B (#1679) wires it incrementally — `destSubpath` write-confinement (#1704) and the typed documentation-sourced #853 dispatch-flatten (#1708, the first consumer of a negotiated `dispatch` axis); adapters/MCP/host-bindings remain Phases C–E. Source of truth: `gsd-core/bin/lib/host-integration.cjs` (generated from `src/host-integration.cts`). See ADR-1239 and ADR-1016. +Pure, additive, no-I/O Module owning the versioned, negotiated contract over the six host-integration interface points (command, dispatch, model, hooks, state, artifact) — ADR-1239 Phase A. Extends the ADR-1016 runtime descriptor with nine closed-vocabulary axes carried under `capability.json` `runtime.hostIntegration`: `embeddingMode` (`imperative|declarative`), `commandSurface` (`slash-file|slash-programmatic|slash-toml|palette|prose-only`), `dispatch` (`{namedDispatch,nested,maxDepth,background,backgroundDispatch,subagentToolkit,isolation}`), `modelMode` (`active|passive`), `hookBus` (`host|engine|none`), `stateIO` (`filesystem|sandboxed-storage|session-log-append`), `transport` (`mcp|native-extension`), `runtime` (`node|bun|sandboxed-web|python|go|rust|electron|other`), `effortSurface` (`argv|none` — how reasoning effort reaches the host; ADR-1239 amendment #2481, the first axis whose consumer is an invocation-time argument rather than an install-time artifact). `dispatch.isolation` (`harness-worktree|orchestrator-worktree|none` — how a host isolates concurrent same-wave executors; ADR-1239 Codex-binding amendment #2584; consumed by the phase scheduler since #2584 Phase 3 and, since #2652, by every single-agent dispatch site — `quick.md`, `diagnose-issues.md`, `execute-plan.md` — which resolve it through the canonical `gsd-core/references/dispatch-isolation-gate.md` rather than branching on a runtime id; and, since #2486, by the runtime-neutral diagnostics — `/gsd:settings` gates its Worktrees recommendation and `/gsd:health` raises W025 from this axis, read through the sentinel-free `inspect-dispatch-isolation` verb). `resolveOrchestratorExec(orchestratorExec, cwd) → { ok:true, command, args, cwd } | { ok:false, reason }` (#2584 Phase 2, pure, no I/O — resolves the `runtime.orchestratorExec` descriptor field, a sibling of `runtime.hostBehaviors` in `capability.json` carrying `{command, args?, cwdFlag?}`, into the concrete argv/cwd a process-spawn primitive would use for a `dispatch.isolation: orchestrator-worktree` host; appends `[cwdFlag, cwd]` to `args` when `cwdFlag` is a non-empty string, e.g. codex `exec --cd `, opencode `run --dir `, kimi `--work-dir `; when `cwdFlag` is `null`/absent — kimi-code's process-cwd case — no flag is appended and `cwd` alone is returned for the caller to bind via the subprocess's own working-directory option; fail-closed `missing_command`/`invalid_cwd`/`invalid_args`/`invalid_cwd_flag`; CONSUMED since #2584 Phase 3 — `routeDispatchIsolation` resolves it into the `exec` field of `gsd_run query dispatch-isolation --json`, and `gsd-core/workflows/execute-phase/steps/executor-isolation-dispatch.md` process-spawns that `command`/`args`/`cwd`; #2652 adds a second consumer, `_negotiatedDispatchIsolation`, which probes it at install time against a placeholder target to decide whether an `orchestrator-worktree` declaration actually resolves). Interface: `negotiateHostCapabilities(host, engine?) → { protocolVersion, effective, points, warnings }` enforcing the trust-boundary invariant `effective ⊆ host-declared ∩ engine-known` (never augment with an undeclared or unknown/future-`protocolVersion` value — fail-closed via the most-restrictive-known `SAFE_DEFAULTS`); `degradationFor(point, axes) → { level, fallback }` (a pure Full/Degraded/Absent ladder table, never throws); `profileOf(axes) → 'programmatic-cli'|'declarative-cli'|'ide'|null`; plus `PROTOCOL_VERSION` (integer, starts at 1 — distinct from the package `version`/`engines.gsd` semver), `HOST_INTEGRATION_AXES` (the frozen closed vocabulary, single source of truth), `PROFILE_BASELINES`, and `shouldFlattenDispatch(dispatch) → boolean` (ADR-1239 Phase B / #1708 — graduates the #853 rule: returns `true` = run the orchestrator inline UNLESS the host is documented to background a nesting-capable orchestrator (`background === true && backgroundDispatch === true`); fail-closed to inline; exposed to the plan/execute workflows via the `gsd_run query dispatch-should-flatten --raw` CLI, which replaced the former scattered `RUNTIME === 'codex'` prose check). The runtime-descriptor validator (`gsd-core/bin/lib/capability-validator.cjs` `validateRuntimeBody`) mirrors the closed vocabulary inline (exported as `_HOST_INTEGRATION_VOCAB`) and is kept in lock-step by the parity guard `tests/host-integration-validator-parity.test.cjs`. Orthogonal axes (resolved explicitly per ADR-1239 Phase A): `commandStyle` (GSD emission style, retained) vs `commandSurface` (host surface type); `hookEvents` dialect vs `hookBus` ownership (a host with `hooksSurface:none` may still be `hookBus:host` — e.g. opencode); `runtimeCompat` (feature→host) vs these negotiated runtime→engine axes. Phase A defined the interface; Phase B (#1679) wires it incrementally — `destSubpath` write-confinement (#1704) and the typed documentation-sourced #853 dispatch-flatten (#1708, the first consumer of a negotiated `dispatch` axis); adapters/MCP/host-bindings remain Phases C–E. Source of truth: `gsd-core/bin/lib/host-integration.cjs` (generated from `src/host-integration.cts`). See ADR-1239 and ADR-1016. ### Statusline Host-integration hook (`hooks/gsd-statusline.js`) that renders the session status line: model name, context-window meter, workspace directory, and the GSD-state segment (`formatGsdState()` projecting `.planning/` STATE.md). `readGsdState()` is workstream-aware (#2850): when the walk-up finds no flat `.planning/STATE.md` but lands on a `.planning/workstreams/` directory, it resolves the active workstream via `resolveActiveWorkstream` (`active-workstream-store.cts`), called with an empty args array — only its env>store precedence applies for this caller, since the CLI leg is inert without argv — and `planningPaths`/`listAvailableWorkstreams` (`planning-workspace.cts`) for path/mode resolution, the same seams every other workstream-aware command uses, and reads that workstream's `STATE.md` instead. The store tier is `peekActiveWorkstream`, a read-only sibling of `getActiveWorkstream` that never deletes a stale/invalid pointer file — a renderer invoked on every prompt must never mutate persistent state as a side effect of drawing a screen (`getActiveWorkstream`'s self-heal is correct for a command, not a render). When workstream mode is detected but nothing resolves, it returns a `{noActiveWorkstream:true}` sentinel that `formatGsdState`/`formatGsdStateCompact` render as `"no active workstream"` — observable, never silent emptiness. Opt-in segments are gated by `.planning/config.json` keys (`statusline.show_last_command`, `statusline.context_position`, plus the approved `statusline.show_context_tokens` and `statusline.state_format`), each registered across `gsd-core/bin/shared/config-schema.manifest.json` + `src/config.cts` + the `loadConfig` whitelist + `docs/CONFIGURATION.md`. The compact GSD-state format consumes the canonical status vocabulary from `normalizeStateStatus()` (STATE.md Document Module) rather than a parallel keyword list. **Data-source boundary (ADR-2164):** the statusline sources only local, read-only data — it refines the stdin payload Claude Code already sends and may add a new *local* source (e.g. `git`), but does not read credentials or call external/network APIs for data; account/usage/platform-level state is out of scope. diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 95774c72f..ac5a326c3 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -345,7 +345,7 @@ All workflow toggles follow the **absent = enabled** pattern. If a key is missin | `workflow.max_discuss_passes` | number | `3` | Maximum number of question rounds in discuss-phase before the workflow stops asking. Useful in headless/auto mode to prevent infinite discussion loops. | | `workflow.skip_discuss` | boolean | `false` | When `true`, `/gsd-autonomous` bypasses the discuss-phase entirely, writing minimal CONTEXT.md from the ROADMAP phase goal. Useful for projects where developer preferences are fully captured in PROJECT.md/REQUIREMENTS.md. Added in v1.28 | | `workflow.text_mode` | boolean | `false` | Replaces AskUserQuestion TUI menus with plain-text numbered lists. Required for Claude Code remote sessions (`/rc` mode) where TUI menus don't render. Can also be set per-session with `--text` flag on discuss-phase. Added in v1.28 | -| `workflow.use_worktrees` | boolean | `true` | When `false`, disables git worktree isolation for parallel execution. Users who prefer sequential execution or whose environment does not support worktrees can disable this. Added in v1.31. **Branch-divergence note:** when your branch has diverged from `origin/HEAD`, GSD auto-degrades to sequential and prints a warning. See [`worktree.baseRef`](#worktree-settings) to restore parallel execution on a diverged branch. **Per-runtime note:** whether this key can be honored depends on the runtime's declared `dispatch.isolation` capability, not on its name (#2584). Runtimes whose own harness isolates each executor (**Claude Code**, **Cursor**) run parallel worktrees natively; runtimes exposing a headless exec with an explicit working directory (**Codex**, **OpenCode**, **Kimi**, **Kimi Code**) get worktrees GSD itself creates and merges — where a dispatch site can only drive the harness model, those hosts degrade to sequential with a warning rather than aborting. Every other runtime declares no isolation primitive, and forcing `use_worktrees: true` there still fails closed before any executor dispatch. See [Executor isolation per runtime](#executor-isolation-per-runtime). | +| `workflow.use_worktrees` | boolean | `true` | When `false`, disables git worktree isolation for parallel execution. Users who prefer sequential execution or whose environment does not support worktrees can disable this. Added in v1.31. **Branch-divergence note:** when your branch has diverged from `origin/HEAD`, GSD auto-degrades to sequential and prints a warning. See [`worktree.baseRef`](#worktree-settings) to restore parallel execution on a diverged branch. **Per-runtime note:** whether this key can be honored depends on the runtime's declared `dispatch.isolation` capability, not on its name (#2584). Runtimes whose own harness isolates each executor (**Claude Code**, **Cursor**) run parallel worktrees natively; runtimes exposing a headless exec with an explicit working directory (**Codex**, **OpenCode**, **Kimi**, **Kimi Code**) get worktrees GSD itself creates and merges — where a dispatch site can only drive the harness model, those hosts degrade to sequential with a warning rather than aborting. Every other runtime declares no isolation primitive, and forcing `use_worktrees: true` there still fails closed before any executor dispatch. `/gsd-health` reports such a value as warning `W025` (#2486). **Default on a non-Claude install:** if a worktree-capable non-Claude host is not isolating as described above, check whether the install stamped this key's default to `false` and set an explicit `use_worktrees: true`. See [Executor isolation per runtime](#executor-isolation-per-runtime). | | `workflow.worktree_skip_hooks` | boolean | `false` | When `true`, executor agents in worktree mode pass `--no-verify` (skipping pre-commit hooks) and post-wave hook validation runs against the merged result instead. Opt-in escape hatch for projects whose hooks cannot run in agent worktrees. Default `false` runs hooks on every commit (#2924). | | `workflow.code_review` | boolean | `true` | Enable `/gsd-code-review` and `/gsd-code-review --fix` commands. When `false`, the commands exit with a configuration gate message. Added in v1.34 | | `workflow.code_review_depth` | string | `standard` | Default review depth for `/gsd-code-review`: `quick` (pattern-matching only), `standard` (per-file analysis), or `deep` (cross-file with import graphs). Can be overridden per-run with `--depth=`. Added in v1.34 | @@ -388,6 +388,26 @@ All workflow toggles follow the **absent = enabled** pattern. If a key is missin |---------|------|---------|-------------| | `worktree.baseRef` | string | (unset) | Controls which ref the worktree-based parallel executor uses as the base when creating new phase/wave worktrees. When unset, the executor bases new worktrees on the repository default branch (`origin/HEAD`); if the current branch has diverged, execute-phase auto-degrades to sequential execution rather than halting (as of v1.4.0). Set to `"head"` to base new worktrees on the local `HEAD` instead — the appropriate choice when working on a branch that has diverged from the default branch, as it prevents the exit-42 base-mismatch halt and allows wave-based parallel execution to proceed normally. See [Fix the worktree base-mismatch (exit 42) error](how-to/fix-worktree-base-mismatch.md). | +### Executor isolation per runtime + +When `/gsd-execute-phase` runs a wave containing several independent plans, it can execute them concurrently — but only if the runtime can keep each executor isolated. Two executors sharing one checkout race on files, git state, hooks, and `.planning/`. Which runtimes can do this is a **declared capability** (`dispatch.isolation`), not a hardcoded list, so the scheduler behaves the same way for every host that declares the same value. + +| Isolation | Runtimes | What happens | +|---|---|---| +| `harness-worktree` | `claude`, `cursor` | The runtime's own harness creates and binds a git worktree per executor. GSD passes the host's isolation flag and runs no git itself. | +| `orchestrator-worktree` | `codex`, `opencode`, `kimi`, `kimi-code` | The runtime has no harness-native isolation, but exposes a headless exec that accepts a working directory. **GSD** creates the worktree, spawns each executor into it, then validates and merges the result. All git operations are performed by GSD, never by the sandboxed executor. | +| `none` | every other runtime | No isolation primitive — plans in a wave run sequentially. Setting `workflow.use_worktrees: true` here fails closed before any executor is dispatched. | + +You do not configure this directly: set `workflow.use_worktrees` and GSD negotiates the rest. `use_worktrees: false` forces sequential execution on **every** runtime, including the ones that support isolation. An unknown or undeclared isolation value always degrades to sequential — GSD never guesses its way into an unisolated parallel run. + +To see what your current runtime negotiated: + +```bash +gsd-tools query inspect-dispatch-isolation --json +``` + +(`inspect-dispatch-isolation` is the read-only form. The `dispatch-isolation` query is the executor-dispatch resolver: it records its decision to the isolation sentinel as a deliberate side effect, so it is not an inspection command.) + ## Code Quality Settings The `code_quality.*` namespace gates optional structural-analysis tooling that augments `/gsd-code-review`. Settings are additive: each tool is independently opt-in and off by default. diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index fb9a7c321..ae2914932 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -1580,7 +1580,8 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load function routeDispatchShouldFlatten({ args, cwd, raw, error }) { // #1708 / #853: typed query replacing the `RUNTIME === 'codex'` prose rule. // - // Resolves the current runtime (GSD_RUNTIME > config.runtime > 'claude'), + // Resolves the current runtime (GSD_RUNTIME > config.runtime > per-install + // .gsd-runtime marker > 'claude'), // looks up registry.runtimes[id].runtime.hostIntegration.dispatch, and // calls shouldFlattenDispatch(dispatch) from host-integration.cjs. // @@ -1634,7 +1635,8 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load // #853) for exactly this reason — it replaced a `RUNTIME === 'codex'` // prose rule. // - // Resolves the current runtime (GSD_RUNTIME > config.runtime > 'claude'), + // Resolves the current runtime (GSD_RUNTIME > config.runtime > per-install + // .gsd-runtime marker > 'claude'), // reads registry.runtimes[id].runtime.hostIntegration.dispatch.isolation, // and validates it against the closed vocabulary. // @@ -1669,12 +1671,83 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load // `per-plan-worktree-gate.md`) override the naturally-resolved mode // while still going through this single write path. Best-effort: a // sentinel write failure here must never fail the wave. - const VALID_ISOLATION = new Set(['harness-worktree', 'orchestrator-worktree', 'none']); + const decision = resolveDispatchIsolationDecision({ args, cwd }); + const runtimeId = decision.runtimeId; + let { isolation, exec, harnessFlag } = decision; + + // `--force-isolation ` overrides the naturally-resolved mode with + // context this resolver has no way to see on its own (e.g. the #2474 + // per-plan submodule intersection). Invalid/unrecognized values are + // ignored rather than erroring — this is a best-effort recording call, + // not a hard usage gate. Forcing to 'none' clears harnessFlag/exec since + // neither applies to sequential dispatch. + const forceIdx = args.indexOf('--force-isolation'); + const forcedIsolation = forceIdx !== -1 ? args[forceIdx + 1] : undefined; + if (forcedIsolation && DISPATCH_ISOLATION_VOCABULARY.has(forcedIsolation)) { + isolation = forcedIsolation; + if (isolation === 'none') { + harnessFlag = null; + exec = null; + } + } + + const phaseIdx = args.indexOf('--phase'); + const phaseArg = phaseIdx !== -1 && args[phaseIdx + 1] && !args[phaseIdx + 1].startsWith('--') + ? args[phaseIdx + 1] + : null; + const planIdx = args.indexOf('--plan'); + const planArg = planIdx !== -1 && args[planIdx + 1] && !args[planIdx + 1].startsWith('--') + ? args[planIdx + 1] + : null; + + // Side-effect write (#3045 CORE REDESIGN) — see the doc comment above. + // Never allowed to affect this query's own stdout contract or throw. + try { + writeDispatchIsolationSentinel(cwd, { isolation, harnessFlag, phase: phaseArg, plan: planArg }); + } catch { + // writeDispatchIsolationSentinel already swallows its own errors into + // a { recorded: false } result; this catch is defense in depth only. + } + + if (args.indexOf('--json') !== -1) { + output({ runtime: runtimeId, isolation, exec, harnessFlag }, raw); + } else { + process.stdout.write(isolation); + } + } + + const DISPATCH_ISOLATION_VOCABULARY = new Set(['harness-worktree', 'orchestrator-worktree', 'none']); + + /** + * Shared, side-effect-free resolution of the negotiated dispatch isolation: + * runtime (GSD_RUNTIME > config.runtime > per-install .gsd-runtime marker > + * 'claude') → declared + * `dispatch.isolation` → harness-flag / orchestrator-exec degrade rules. + * Extracted (#2486) so `routeDispatchIsolation` (the #3045 recording + * dispatch path) and `routeInspectDispatchIsolation` (the read-only + * inspection path) share exactly one negotiation implementation and cannot + * drift apart. Resolution only — the caller decides whether the decision is + * recorded to the sentinel. + */ + function resolveDispatchIsolationDecision({ args, cwd }) { let isolation = 'none'; let runtimeId = null; let exec = null; let harnessFlag = null; try { + // Deliberately `resolveRuntime`, NOT `resolveActiveRuntime`/`loadConfig`: + // loadConfig normalizes and rewrites legacy keys back to disk, and this + // resolver backs the sentinel-free `inspect-dispatch-isolation` verb, + // which must never write. resolveRuntime reads config.json directly. + // + // KNOWN LIMITATION, tracked separately: resolveRuntime stops at + // GSD_RUNTIME > config.runtime > 'claude' and does not consult the + // per-install `.gsd-runtime` marker, so on a non-Claude install whose + // project config carries no `runtime` key this resolves 'claude'. That is + // open-gsd/gsd-core#2395 — a pre-existing defect in the canonical + // resolver, not introduced here, and deliberately NOT fixed in this PR + // (its blast radius reaches every consumer of that resolver, so it is + // being handled on its own). const { resolveRuntime } = require('./lib/runtime-slash.cjs'); runtimeId = resolveRuntime(cwd); @@ -1683,7 +1756,7 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load ? registry.runtimes[runtimeId] : null; const declared = runtimeEntry?.runtime?.hostIntegration?.dispatch?.isolation ?? null; - if (typeof declared === 'string' && VALID_ISOLATION.has(declared)) { + if (typeof declared === 'string' && DISPATCH_ISOLATION_VOCABULARY.has(declared)) { isolation = declared; } @@ -1726,41 +1799,49 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load exec = null; harnessFlag = null; } + return { runtimeId, isolation, exec, harnessFlag }; + } - // `--force-isolation ` overrides the naturally-resolved mode with - // context this resolver has no way to see on its own (e.g. the #2474 - // per-plan submodule intersection). Invalid/unrecognized values are - // ignored rather than erroring — this is a best-effort recording call, - // not a hard usage gate. Forcing to 'none' clears harnessFlag/exec since - // neither applies to sequential dispatch. - const forceIdx = args.indexOf('--force-isolation'); - const forcedIsolation = forceIdx !== -1 ? args[forceIdx + 1] : undefined; - if (forcedIsolation && VALID_ISOLATION.has(forcedIsolation)) { - isolation = forcedIsolation; - if (isolation === 'none') { - harnessFlag = null; - exec = null; - } + function routeInspectDispatchIsolation({ args, cwd, raw }) { + // #2486: sentinel-free sibling of `dispatch-isolation` for INSPECTION + // surfaces — /gsd:health's W025 check and /gsd:settings' Worktrees + // branching. The dispatch verb above intentionally records its resolved + // decision to the isolation sentinel as an unconditional side effect + // (#3045 CORE REDESIGN): correct for executor dispatch, where the record + // must be structurally unskippable — but wrong for a read-only + // diagnostic. A health check that records a phase:null/plan:null + // sentinel can hard-block every executor dispatch for the sentinel's + // lifetime, across sessions sharing the main checkout. Inspection + // surfaces call this verb instead. Two claims, both narrower than + // "side-effect-free", and both exactly true (#2486 review, Majors 2 & 4): + // + // 1. SENTINEL-FREE, not write-free. This route writes nothing itself, and + // in particular never writes .gsd/dispatch-isolation-sentinel.json — + // the only write that can hard-block a later executor dispatch. It is + // NOT a claim of total filesystem purity: like every gsd-tools + // invocation, it runs the shared bootstrap and active-workstream + // resolution first, and getActiveWorkstream self-heals (unlinks) a + // stale or invalid pointer. That is pre-existing, verb-independent, + // and harmless to dispatch. + // + // 2. SHARED NEGOTIATION, for the arguments this verb accepts. Both verbs + // call resolveDispatchIsolationDecision, so the natural resolution + // cannot drift. It is NOT a claim of byte-identical output for every + // argv: routeDispatchIsolation applies --force-isolation AFTER the + // shared helper returns, so the same argv could otherwise yield + // 'none' there and the declared capability here. Rather than let a + // caller receive a silently different answer, this verb REJECTS the + // recording-only knobs outright — they exist to be recorded, and a + // read has nothing to record. + const RECORDING_ONLY_ARGS = ['--force-isolation', '--phase', '--plan']; + const rejected = RECORDING_ONLY_ARGS.filter((flag) => args.indexOf(flag) !== -1); + if (rejected.length > 0) { + error( + `inspect-dispatch-isolation: ${rejected.join(', ')} ${rejected.length === 1 ? 'is a' : 'are'} recording-only argument${rejected.length === 1 ? '' : 's'} and cannot be used on the inspection verb — it resolves the runtime's DECLARED capability and records nothing. Use 'query dispatch-isolation' if you need the override applied and the decision recorded.`, + ERROR_REASON.USAGE, + ); } - - const phaseIdx = args.indexOf('--phase'); - const phaseArg = phaseIdx !== -1 && args[phaseIdx + 1] && !args[phaseIdx + 1].startsWith('--') - ? args[phaseIdx + 1] - : null; - const planIdx = args.indexOf('--plan'); - const planArg = planIdx !== -1 && args[planIdx + 1] && !args[planIdx + 1].startsWith('--') - ? args[planIdx + 1] - : null; - - // Side-effect write (#3045 CORE REDESIGN) — see the doc comment above. - // Never allowed to affect this query's own stdout contract or throw. - try { - writeDispatchIsolationSentinel(cwd, { isolation, harnessFlag, phase: phaseArg, plan: planArg }); - } catch { - // writeDispatchIsolationSentinel already swallows its own errors into - // a { recorded: false } result; this catch is defense in depth only. - } - + const { runtimeId, isolation, exec, harnessFlag } = resolveDispatchIsolationDecision({ args, cwd }); if (args.indexOf('--json') !== -1) { output({ runtime: runtimeId, isolation, exec, harnessFlag }, raw); } else { @@ -3500,6 +3581,7 @@ const HOST_COMMAND_ROUTERS = { 'normalize-test-command': routeNormalizeTestCommand, 'dispatch-should-flatten': routeDispatchShouldFlatten, 'dispatch-isolation': routeDispatchIsolation, + 'inspect-dispatch-isolation': routeInspectDispatchIsolation, 'record-dispatch-isolation': routeRecordDispatchIsolation, 'resolve-dispatch-type': routeResolveDispatchType, 'agent-skills': routeAgentSkills, @@ -3754,7 +3836,7 @@ const TOP_LEVEL_USAGE = 'Usage: gsd-tools [args] [--raw] [--pick /dev/null) +_ISOLATION_RC=$? +if [ $_ISOLATION_RC -ne 0 ] || [ -z "$_INSPECTED_RAW" ]; then + INSPECTED_ISOLATION=none + INSPECTED_RESOLVED=false # no verdict learned — not the same as "declares none" +else + INSPECTED_ISOLATION="$_INSPECTED_RAW" + INSPECTED_RESOLVED=true +fi +case "$INSPECTED_ISOLATION" in + harness-worktree|orchestrator-worktree|none) ;; + *) INSPECTED_ISOLATION=none; INSPECTED_RESOLVED=false ;; # out of vocabulary is not a verdict either +esac + +USE_WORKTREES=$(gsd_run query config-get workflow.use_worktrees --raw 2>/dev/null) +if [ "$INSPECTED_ISOLATION" = "none" ] && [ -n "$USE_WORKTREES" ] && [ "$USE_WORKTREES" != "false" ]; then + if [ "$INSPECTED_RESOLVED" = "true" ]; then + echo "W025: the project config sets workflow.use_worktrees to a non-false value, but this runtime has no usable executor-isolation primitive — dispatch.isolation resolves to none, declared as none, or fail-closed because the capability could not be determined — so /gsd:execute-phase and /gsd:quick will fail closed. Fix: run /gsd:settings and answer No to Worktrees, or set workflow.use_worktrees: false in the project config (the active workstream's config.json when one is active). Set it explicitly rather than deleting the key — an absent key resolves to false only on an emit whose default was stamped to false, and to true otherwise." + else + echo "W025: the project config sets workflow.use_worktrees to a non-false value, and GSD could not resolve this runtime's executor-isolation capability ('gsd_run query inspect-dispatch-isolation' failed or returned nothing) — so it cannot tell whether /gsd:execute-phase and /gsd:quick will fail closed on that value. This is a report of an unverifiable config, NOT a finding that the runtime declares no primitive. Fix: re-run once the gsd-tools shim resolves; if the warning persists, run /gsd:settings and answer No to Worktrees." + fi +fi +``` + +If the check prints, append it to the Warnings section of the report as `[W025]` with the printed fix, include it in the displayed warning count, and report `Status: DEGRADED` if `validate.health` returned `healthy` (a config the execution workflows fail closed on is not a healthy planning state). It is not auto-repairable: an explicit `true` may be intentional for a worktree-capable install sharing the same `.planning/config.json`, so the remedy is the user's call (#2486). @@ -187,8 +236,11 @@ Report final status. | W018 | warning | MILESTONES.md missing entry for archived milestone snapshot | Yes (`--backfill`) | | W019 | warning | Unrecognized .planning/ root file — not a canonical GSD artifact | No | | W024 | warning | STATE.md was written many commits ago — treat its contents as approximate | No | +| W025 | warning | config.json: workflow.use_worktrees enabled on a runtime whose dispatch.isolation is none (#2486) | No | | I001 | info | Plan without SUMMARY (may be in progress) | No | +Note: the `W0NN` warning-code namespace is owned by `src/verify.cts` (`validate.health`), which also emits codes this table does not list (`W010`–`W017` and `W020`–`W023` as of #2486). `W001`–`W024` are all allocated, so this workflow's isolation warning is `W025`. Before assigning a new code here, grep `src/verify.cts` for the next free number — the table alone under-represents the live namespace, and two PRs in flight can otherwise claim the same code (which is exactly what happened between #2486 and #2573). + diff --git a/gsd-core/workflows/settings.md b/gsd-core/workflows/settings.md index 908024492..6aa4d00b1 100644 --- a/gsd-core/workflows/settings.md +++ b/gsd-core/workflows/settings.md @@ -59,7 +59,7 @@ Parse current values (default to `true` if not present): - `graphify.auto_update` — opt-in: auto-rebuild graph after main HEAD advances (#3347) (default: `false`) - `model_profile` — which model each agent uses (default: `balanced`) - `git.branching_strategy` — branching approach (default: `"none"`) -- `workflow.use_worktrees` — whether parallel executor agents run in worktree isolation (default: `true`) +- `workflow.use_worktrees` — whether parallel executor agents run in worktree isolation (honored when the runtime declares a `dispatch.isolation` primitive — `harness-worktree` or `orchestrator-worktree`; runtimes declaring `none` default it to `false` and fail closed on an explicit `true` — #1521, #2486, #2584) - `model_policy.provider` — provider slug for model policy (default: `null`; known values: anthropic, openai, google, qwen; set via /gsd:config --advanced) - `model_policy.budget` — budget level for model policy (default: `null`; known values: high, medium, low; set via /gsd:config --advanced) - `model_policy.high` — model ID for high-cost tier (default: `null`; set via /gsd:config --advanced) @@ -87,6 +87,30 @@ configure `model_overrides` manually in .planning/config.json to target specific models per agent. ``` +**Isolation resolution for the Worktrees question (#2486):** resolve the runtime's declared executor-isolation primitive and the current worktrees value before presenting the questions. Use `inspect-dispatch-isolation`, the **sentinel-free** inspection verb — never `dispatch-isolation`, whose #3045 contract records the resolved decision to the executor-dispatch sentinel as an unconditional side effect; a settings menu must not be able to stamp a sentinel the isolation guards then enforce against real dispatches. Sentinel-free is the exact claim: the shared CLI bootstrap still runs, but nothing it does can block a dispatch. The worktrees read deliberately carries no `--default`/fallback: an absent key must stay distinguishable from an explicit `false` (empty output = key absent), which the pre-selection rule below depends on. + +A failed query is not a capability verdict, so track the two apart: settings still fails closed, but +says so instead of claiming the runtime declares no primitive. The verb fail-closes an unknown runtime — or an +internal resolution error — to `none` and exits 0, so `INSPECTED_RESOLVED=false` means only "the query did not answer" +(#2486 review): + +```bash +_INSPECTED_RAW=$(gsd_run query inspect-dispatch-isolation --raw 2>/dev/null) +_ISOLATION_RC=$? +if [ $_ISOLATION_RC -ne 0 ] || [ -z "$_INSPECTED_RAW" ]; then + INSPECTED_ISOLATION=none + INSPECTED_RESOLVED=false # no verdict learned — not the same as "declares none" +else + INSPECTED_ISOLATION="$_INSPECTED_RAW" + INSPECTED_RESOLVED=true +fi +case "$INSPECTED_ISOLATION" in + harness-worktree|orchestrator-worktree|none) ;; + *) INSPECTED_ISOLATION=none; INSPECTED_RESOLVED=false ;; # out of vocabulary is not a verdict either +esac +USE_WORKTREES_CURRENT=$(gsd_run query config-get workflow.use_worktrees --raw 2>/dev/null) +``` + Use AskUserQuestion with current values pre-selected. Questions are grouped into six visual sections; the first question in each section carries the section-denoting `header` field (AskUserQuestion renders abbreviated section tags for grouping, max 12 chars). Section layout: @@ -113,6 +137,46 @@ Context Warnings, Research Qs **Conditional visibility — graphify.auto_update:** This question is shown only when the user's chosen `graphify.enabled` value is on. If `graphify.enabled` is off, omit the `graphify.auto_update` question and preserve the existing `graphify.auto_update` value in config (do not overwrite). Implementation: ask Graphify first; only ask Graph auto-update when Graphify is enabled. +**Conditional options — Worktrees (#2486):** whether a runtime can honor `workflow.use_worktrees: true` depends on its **declared `dispatch.isolation` capability, not its name** (#2584): `harness-worktree` (the host isolates each executor) and `orchestrator-worktree` (GSD drives the worktrees) both run parallel; `none` fails closed on an explicit `true`. Branch on the same negotiation the execution gate resolves, via the sentinel-free `inspect-dispatch-isolation` verb, which fail-closes unknown/undeclared/`undocumented` to `none`. The rule is one-directional: **never persist a value the isolation gate would fail closed on.** **Do not add a runtime-name test here.** Branch on `$INSPECTED_ISOLATION`: + +- **If `INSPECTED_ISOLATION` ≠ `none`** (`harness-worktree` or `orchestrator-worktree`): present the Worktrees question exactly as written below — unchanged behavior. The `none` branch is the entirety of the #2486 change. +- **If `INSPECTED_ISOLATION` = `none`:** replace the question's options with the two below — do NOT offer an enabling option, and NEVER write `workflow.use_worktrees: true` from this workflow when the runtime declares no isolation primitive, regardless of the user's answer: + +``` +{ + question: "Use git worktrees for parallel agent isolation?", + header: "Worktrees", + multiSelect: false, + options: [ + { label: "No (Recommended)", description: "Write use_worktrees: false. {FINDING}, so parallel plans run sequentially and {CONSEQUENCE}." }, + { label: "Leave unchanged", description: "Do not write the key. {ABSENCE}; an existing explicit value is kept intact (e.g. for a worktree-capable install sharing this config)." } + ] +} +``` + +**The three placeholders** carry the only difference between a capability GSD resolved and one it could not, so no text in this branch ever asserts a verdict that was not reached. Substitute them everywhere they appear — the two `description`s above, the pre-selection rationale, and the notice — from whichever column applies: + +| | `INSPECTED_RESOLVED=true` | `INSPECTED_RESOLVED=false` | +|---|---|---| +| `{FINDING}` | this runtime has no usable executor-isolation primitive | GSD could not resolve this runtime's executor-isolation capability | +| `{CONSEQUENCE}` | execution fails closed on an explicit true | GSD cannot tell whether execution will accept an explicit true | +| `{ABSENCE}` | absent, it resolves to false wherever this emit stamped that default | absent, it is the safe state until the capability resolves | + +Failing closed is right in both columns — never write `workflow.use_worktrees: true` from this branch either way. Only the explanation changes. + + Persistence: "No (Recommended)" → write `workflow.use_worktrees: false`; "Leave unchanged" → do not write `workflow.use_worktrees` at all (preserve the existing value or absence). + + Pre-selection: the generic "current values pre-selected" rule does not apply when `INSPECTED_ISOLATION` is `none` — an explicit `true` has no matching option by design. Pre-select "No (Recommended)" in every case, including an absent key (`$USE_WORKTREES_CURRENT` empty, the no-`--default` read's absent signal): absence resolves to `false` only where this emit stamped that default and to `true` otherwise, so leaving it absent can preserve the very state this branch prevents, while an explicit `false` is correct under either emit (#2486 review). The same applies when it is an explicit non-false value — the broken-inheritance case the notice below covers. "Leave unchanged" remains available for the deliberate shared-config case, but is never the default here. The default must be the repair the "(Recommended)" label points to, so accepting it never keeps a value this branch could not justify. In TEXT_MODE, mark that option as the default in the numbered list. + + Additionally, if `$USE_WORKTREES_CURRENT` is non-empty and not `false` (the config carries an explicit `true` — e.g. inherited from a worktree-capable install sharing the repo; empty means the key is absent, which needs no notice), prepend this notice before the question: + +``` +Note: the project config currently sets workflow.use_worktrees: true. +{FINDING}, so {CONSEQUENCE}. Choose "No" to repair it for this runtime, or +"Leave unchanged" to keep it for a worktree-capable install that shares this +config. +``` + ``` // Model profile is selected via a two-question split because AskUserQuestion enforces a // hard 4-option cap and there are 5 valid profiles (quality, balanced, budget, adaptive, @@ -411,7 +475,7 @@ Merge new settings into existing config.json: "research_before_questions": true/false, "discuss_mode": "discuss" | "assumptions", "skip_discuss": true/false, - "use_worktrees": true/false + "use_worktrees": true/false // never written as true when the runtime's dispatch.isolation is none; omitted entirely when the user chose "Leave unchanged" (#2486) }, "plan_review": { "source_grounding": true/false diff --git a/tests/emitted-drift-acks/2486-settings-worktrees-runtime.json b/tests/emitted-drift-acks/2486-settings-worktrees-runtime.json new file mode 100644 index 000000000..1a402a524 --- /dev/null +++ b/tests/emitted-drift-acks/2486-settings-worktrees-runtime.json @@ -0,0 +1,6 @@ +{ + "version": 1, + "paths": { + "settings.md": "#2486 \u2014 the Worktrees question now branches on the runtime's declared dispatch.isolation capability (resolved via the sentinel-free inspect-dispatch-isolation query) instead of offering Claude-only worktree isolation everywhere. Growth is the isolation-none branch, its explanatory notice, and the tri-state pre-selection rule for the broken-inheritance repair." + } +} diff --git a/tests/emitted-drift-acks/2573-state-head-freshness.json b/tests/emitted-drift-acks/2573-state-head-freshness.json index ecb372b44..f87f090a9 100644 --- a/tests/emitted-drift-acks/2573-state-head-freshness.json +++ b/tests/emitted-drift-acks/2573-state-head-freshness.json @@ -1,6 +1,6 @@ { "version": 1, "paths": { - "health.md": "#2573 registers W024 (STATE.md written many commits ago — treat its contents as approximate) in the health workflow's table, so the advisory health now emits is documented where every other W-code is listed. The growth is that single table row written inline, not relocated into an eagerly @-imported reference (ADR-1610 Decision 4). 12246 -> 12348 bytes (+102), DEFAULT tier, cap 40960." + "health.md": "#2573 registers W024 (STATE.md written many commits ago \u2014 treat its contents as approximate) in the health workflow's table, so the advisory health now emits is documented where every other W-code is listed. The growth is that single table row written inline, not relocated into an eagerly @-imported reference (ADR-1610 Decision 4). 12246 -> 12348 bytes (+102), DEFAULT tier, cap 40960.\n\nAlso acknowledged here because the seam permits exactly one ack source per path and #2573 landed on next first (#2486 rebase, 2026-08-11): #2486 \u2014 adds the W025 diagnostic that detects a persisted workflow.use_worktrees:true on a runtime whose declared isolation cannot honor it, resolved via the sentinel-free inspect-dispatch-isolation query. Growth is the new check, its prose, and the error_codes row." } } diff --git a/tests/gsd-agent-isolation-guard.test.cjs b/tests/gsd-agent-isolation-guard.test.cjs index 5eceb335b..01af113a3 100644 --- a/tests/gsd-agent-isolation-guard.test.cjs +++ b/tests/gsd-agent-isolation-guard.test.cjs @@ -874,3 +874,201 @@ describe('#3045 MINOR — writer/reader sentinel path derivation now agrees for assert.equal(read.isolation, 'harness-worktree'); }); }); + +describe('#2486 regression: inspect-dispatch-isolation is the sentinel-free read', () => { + // /gsd:health (W025) and /gsd:settings (Worktrees branching) must be able to + // learn the negotiated isolation WITHOUT recording it: the #3045 recording + // verb stamps a phase:null/plan:null sentinel the guard hooks then enforce + // against real executor dispatches — letting a read-only diagnostic + // hard-block execution for the sentinel's lifetime, across sessions. + + test('inspect-dispatch-isolation resolves the declared capability and writes NO sentinel', (t) => { + const dir = createTempProject('gsd-2486-inspect-'); + t.after(() => cleanup(dir)); + assert.equal(fs.existsSync(sentinelFile(dir)), false, 'precondition: no sentinel yet'); + const result = runGsdTools( + ['query', 'inspect-dispatch-isolation', '--raw'], + dir, + { GSD_RUNTIME: 'claude', HOME: dir }, + ); + assert.equal(result.success, true, result.error); + assert.equal(result.output.trim(), 'harness-worktree'); + assert.equal( + fs.existsSync(sentinelFile(dir)), + false, + 'inspection must not create .gsd/dispatch-isolation-sentinel.json', + ); + assert.equal(fs.existsSync(path.join(dir, '.gsd')), false, 'inspection must not even create the .gsd dir'); + }); + + + test('parity: inspect resolves byte-identically to the recording verb for every registry runtime', (t) => { + // Same negotiation implementation by construction (shared helper) — this + // pins the contract so a future edit cannot fork the two verbs apart. + for (const runtimeId of Object.keys(runtimes)) { + const dir = createTempProject('gsd-2486-parity-'); + t.after(() => cleanup(dir)); + const inspected = runGsdTools( + ['query', 'inspect-dispatch-isolation', '--raw'], + dir, + { GSD_RUNTIME: runtimeId, HOME: dir }, + ); + assert.equal(inspected.success, true, inspected.error); + assert.equal( + fs.existsSync(sentinelFile(dir)), + false, + `${runtimeId}: inspect must not write the sentinel`, + ); + + const dispatched = runGsdTools( + ['query', 'dispatch-isolation', '--raw'], + dir, + { GSD_RUNTIME: runtimeId, HOME: dir }, + ); + assert.equal(dispatched.success, true, dispatched.error); + assert.equal( + inspected.output.trim(), + dispatched.output.trim(), + `${runtimeId}: the two verbs must resolve the same isolation`, + ); + } + }); + + // #2486 review, Major 4: silently IGNORING these was the defect. The + // recording verb applies --force-isolation after the shared helper returns, + // so accepting-and-ignoring it here means the same argv yields 'none' from + // dispatch and the declared capability from inspect — a caller gets a + // different answer with no signal. Rejecting turns that into a loud usage + // error. This test fails if the verb ever goes back to accepting them. + for (const [flag, value] of [['--force-isolation', 'none'], ['--phase', '9'], ['--plan', 'p1']]) { + test(`inspect REJECTS the recording-only argument ${flag}`, (t) => { + const dir = createTempProject('gsd-2486-inspect-args-'); + t.after(() => cleanup(dir)); + // --json-errors so the assertion is on the TYPED reason, not on human + // prose: swapping ERROR_REASON.USAGE for UNKNOWN would keep a message + // regex green while breaking every machine consumer (#2486 review, + // round-9 Minor 4). + const result = runGsdTools( + ['query', 'inspect-dispatch-isolation', '--raw', flag, value, '--json-errors'], + dir, + { GSD_RUNTIME: 'claude', HOME: dir }, + ); + assert.equal(result.success, false, `${flag} must be a usage error, not a silently ignored argument`); + const envelope = JSON.parse(result.error.trim().split('\n').filter(Boolean).pop()); + assert.equal( + envelope.reason, + 'usage', + `${flag}: must be typed as a usage error — got ${JSON.stringify(envelope.reason)}`, + ); + assert.match( + envelope.message || '', + /recording-only/, + `${flag}: the message must say why the argument has no read-path meaning`, + ); + assert.equal(fs.existsSync(sentinelFile(dir)), false, 'a rejected inspection still records nothing'); + }); + } + + test('the divergence that rejection prevents: dispatch DOES honour --force-isolation', (t) => { + // Pins the asymmetry that makes rejection necessary rather than pedantic. + // If a future edit moved the override into the shared helper, inspect could + // safely accept the flag — and this test would still pass, correctly, while + // the rejection tests above would then be the ones to revisit. + const dir = createTempProject('gsd-2486-force-divergence-'); + t.after(() => cleanup(dir)); + const dispatched = runGsdTools( + ['query', 'dispatch-isolation', '--raw', '--force-isolation', 'none'], + dir, + { GSD_RUNTIME: 'claude', HOME: dir }, + ); + assert.equal(dispatched.success, true, dispatched.error); + assert.equal(dispatched.output.trim(), 'none', 'force is honoured on the recording verb'); + + const inspected = runGsdTools( + ['query', 'inspect-dispatch-isolation', '--raw'], + dir, + { GSD_RUNTIME: 'claude', HOME: dir }, + ); + assert.equal(inspected.success, true, inspected.error); + assert.equal( + inspected.output.trim(), + 'harness-worktree', + 'inspection reports the DECLARED capability, which is what the two would disagree on', + ); + }); + + // #2486 review, Minor 5: asserting inspection against a HANDWRITTEN key list + // does not test parity at all — changing the recording verb's JSON + // independently would leave it green. Compare against the real thing. + test('--json shape matches the recording verb, compared against its actual output', (t) => { + const inspectDir = createTempProject('gsd-2486-inspect-json-'); + const dispatchDir = createTempProject('gsd-2486-dispatch-json-'); + t.after(() => cleanup(inspectDir)); + t.after(() => cleanup(dispatchDir)); + + const inspected = runGsdTools( + ['query', 'inspect-dispatch-isolation', '--json'], + inspectDir, + { GSD_RUNTIME: 'cursor', HOME: inspectDir }, + ); + assert.equal(inspected.success, true, inspected.error); + const inspectedJson = JSON.parse(inspected.output); + + // Separate project dir: the recording verb writes a sentinel, and the + // inspection assertion below must not be able to see it. + const dispatched = runGsdTools( + ['query', 'dispatch-isolation', '--json'], + dispatchDir, + { GSD_RUNTIME: 'cursor', HOME: dispatchDir }, + ); + assert.equal(dispatched.success, true, dispatched.error); + const dispatchedJson = JSON.parse(dispatched.output); + + assert.deepEqual( + Object.keys(inspectedJson).sort(), + Object.keys(dispatchedJson).sort(), + 'consumers written against the recording verb JSON must be able to switch verbatim', + ); + assert.deepEqual( + inspectedJson, + dispatchedJson, + 'same runtime, same declared capability — every field must agree, not just the key set', + ); + assert.equal(inspectedJson.runtime, 'cursor'); + assert.equal(inspectedJson.isolation, 'harness-worktree'); + assert.equal(fs.existsSync(sentinelFile(inspectDir)), false, 'no sentinel from a --json inspection either'); + assert.equal(fs.existsSync(sentinelFile(dispatchDir)), true, 'control: the recording verb DID write one'); + }); + + // #2486 review, Minor 5 (second half): the registry parity test compares raw + // isolation only, so it never exercises the orchestrator `exec` branch that + // --cwd-target populates. Compare the full JSON there too. + test('parity holds on the orchestrator exec branch (--cwd-target), not just raw isolation', (t) => { + const inspectDir = createTempProject('gsd-2486-inspect-cwd-'); + const dispatchDir = createTempProject('gsd-2486-dispatch-cwd-'); + t.after(() => cleanup(inspectDir)); + t.after(() => cleanup(dispatchDir)); + + const inspected = runGsdTools( + ['query', 'inspect-dispatch-isolation', '--json', '--cwd-target', 'wt'], + inspectDir, + { GSD_RUNTIME: 'codex', HOME: inspectDir }, + ); + assert.equal(inspected.success, true, inspected.error); + const dispatched = runGsdTools( + ['query', 'dispatch-isolation', '--json', '--cwd-target', 'wt'], + dispatchDir, + { GSD_RUNTIME: 'codex', HOME: dispatchDir }, + ); + assert.equal(dispatched.success, true, dispatched.error); + + const inspectedJson = JSON.parse(inspected.output); + assert.deepEqual( + inspectedJson, + JSON.parse(dispatched.output), + 'the exec branch must resolve identically on both verbs', + ); + assert.equal(inspectedJson.isolation, 'orchestrator-worktree', 'precondition: codex is the orchestrator-worktree case'); + assert.ok(inspectedJson.exec, 'precondition: this branch actually populates exec, so the comparison means something'); + }); +}); diff --git a/tests/host-integration.test.cjs b/tests/host-integration.test.cjs index 31778e9a4..c302eb09a 100644 --- a/tests/host-integration.test.cjs +++ b/tests/host-integration.test.cjs @@ -2480,6 +2480,15 @@ describe('#2728 B1 — isolation degrades re-record through the single write pat 'delegate or make those sites re-record inline', ); + // #2486 note: the runtime-neutral diagnostics (health.md, settings.md) + // resolve isolation for a READ, not a dispatch, and deliberately name their + // state `INSPECTED_ISOLATION` rather than `ISOLATION`. That keeps them out + // of this scan by construction. An earlier revision exempted those two + // files instead; the exemption was file-wide, so any real dispatch block + // added to either one would have inherited it and escaped a guard whose + // name promises "every dispatch site" (#2486 review). Renaming the variable + // removes the carve-out entirely — a diagnostic that ever writes a literal + // `ISOLATION=none` is caught here like any other site. for (const file of scan) { const rel = path.relative(REPO_ROOT, file).replace(/\\/g, '/'); if (DELEGATED_TO_PER_PLAN_GATE.has(rel)) continue; diff --git a/tests/runtime-converters.test.cjs b/tests/runtime-converters.test.cjs index 79d2c7986..a7d7fae69 100644 --- a/tests/runtime-converters.test.cjs +++ b/tests/runtime-converters.test.cjs @@ -1530,6 +1530,465 @@ test('manager.md and autonomous.md no longer contain old "not claude" background }); } +// ──────────────────────────────────────────────────────────────────────── +// #2486: settings.md must not recommend or persist worktree isolation on +// non-Claude runtimes, and health.md must surface an inherited explicit +// use_worktrees=true before execution fails closed on it. Both rely on the +// #1521 stamping machinery, so the canonical runtime/use_worktrees reads in +// those two files are part of the runtime contract surface. +// ──────────────────────────────────────────────────────────────────────── +{ + const fs = require('node:fs'); + const path = require('node:path'); + const conversion = require('../gsd-core/bin/lib/runtime-artifact-conversion.cjs'); + const { NON_CLAUDE_RUNTIMES } = conversion; + + const RUNTIME_BRANCH_WORKFLOWS = ['settings.md', 'health.md']; + const CLAUDE_RUNTIME_LINE = 'config-get runtime --default claude --raw 2>/dev/null || echo "claude"'; + const TRUE_WT_LINE = 'config-get workflow.use_worktrees --raw 2>/dev/null || echo "true"'; + // #2486 review round 2 (#2584): the capability read that replaced the runtime-name + // gate. Round 4: `inspect-dispatch-isolation`, the side-effect-free inspection verb — + // the recording `dispatch-isolation` verb stamps the executor-dispatch sentinel as an + // unconditional #3045 side effect, which an inspection surface must never do. + // Round 9 (#2486 review, Major 3): this used to pin the single-line + // `ISOLATION=$(… || echo "none")` read. That shape was the defect — `|| echo + // "none"` collapses "this runtime declares no primitive" into "the resolver + // failed", and both surfaces then assert the former, which is false. The + // canonical read now captures the raw value and tracks whether a verdict was + // actually learned, the same shape the execution-side isolation gate uses. + // Pinned as two invariants rather than one long literal so that reformatting + // the block does not fail the test while a semantic regression still does. + const ISOLATION_LINE = '_INSPECTED_RAW=$(gsd_run query inspect-dispatch-isolation --raw 2>/dev/null)'; + const ISOLATION_RESOLVED_FLAG = 'INSPECTED_RESOLVED'; + const ISOLATION_COLLAPSING_FALLBACK = /inspect-dispatch-isolation --raw 2>\/dev\/null \|\| echo/; + const readWorkflow = (wf) => + fs.readFileSync(path.join(__dirname, '..', 'gsd-core', 'workflows', wf), 'utf8'); + + // Behavioral W025 coverage (#2486 Major 2) executes the shipped block, so it + // needs a subprocess and a CRLF-safe read. `readFileNormalized` normalizes at + // the READ boundary — a `\r?\n` fence regex alone leaves embedded CR inside + // the captured body, which bash then treats as part of the token + // (DEFECT.WINDOWS-CRLF-TEST-PORTABILITY). + // + // The subprocess goes through the process seam, never a hand-rolled + // spawnSync (CONTRIBUTING "Spawning a subprocess: use the process seam"). + // `runHook` already documents `interpreter: 'bash'` for running a shell + // script, so the harness is written to a file rather than passed as `-c`. + const { runHook } = require('./helpers/process-seam.cjs'); + const { readFileNormalized, createTempDir, cleanup: cleanupDir } = require('./helpers.cjs'); + // Skipped on Windows, where there is no bash. Checked by platform rather than + // by shelling out to `which`, which is itself non-portable. + const NO_BASH = process.platform === 'win32'; + + describe('#2486 regression: settings/health worktrees isolation branch', () => { + // Review round 2 (#2584 Phase 3): isolation is a DECLARED CAPABILITY, not a + // runtime name. cursor declares harness-worktree and codex/opencode/kimi/ + // kimi-code declare orchestrator-worktree, so a `RUNTIME != claude` gate + // would block a supported configuration on five runtimes and false-warn in + // health. Both surfaces branch on `dispatch-isolation` instead. + test('settings.md and health.md contain NO runtime-name gate for the worktrees branch', () => { + // allow-test-rule: source-text-is-the-product (#2486) + // Workflow .md text IS what the runtime loads — asserting on it tests the deployed contract. + for (const wf of RUNTIME_BRANCH_WORKFLOWS) { + const src = readWorkflow(wf); + assert.ok( + !src.includes(CLAUDE_RUNTIME_LINE), + `${wf}: the worktrees branch must not read the runtime name — gate on dispatch-isolation (#2584)`, + ); + assert.ok( + !/RUNTIME"?\s*(!=|=)\s*"?claude/.test(src), + `${wf}: residual RUNTIME-vs-claude comparison — execute-phase forbids a runtime-name fan-out`, + ); + // Prose gates count too: the shell-syntax check above missed two + // sentences that still asserted the obsolete non-Claude premise + // (the config-key list and the emitted-JSON schema comment). + assert.ok( + !/non-Claude (installs?|runtimes?)\b[^.]{0,120}\b(fail closed|default it to `?false)/i.test(src), + `${wf}: prose still states the obsolete "non-Claude cannot honor worktrees" premise — gate on dispatch.isolation`, + ); + assert.ok( + !/never written as true on a non-Claude runtime/i.test(src), + `${wf}: emitted-JSON comment still claims a runtime-name persistence rule`, + ); + } + }); + + // Round 4 note: an earlier revision carried a test here that encoded the + // #2728 merge order as a red assertion against quick.md/diagnose-issues.md. + // Deleted: a repo test cannot sequence merges — it only made this change + // unmergeable on its own schedule. The settings behavior change is instead + // scoped entirely to the `ISOLATION = none` branch (below), which needs + // nothing from any sibling PR; the `!= none` path is base behavior unchanged. + + test('settings.md and health.md resolve isolation with the sentinel-free inspection read', () => { + // allow-test-rule: source-text-is-the-product (#2486) + // Workflow .md text IS what the runtime loads — asserting on it tests the deployed contract. + for (const wf of RUNTIME_BRANCH_WORKFLOWS) { + const src = readWorkflow(wf); + assert.ok( + src.includes(ISOLATION_LINE), + `${wf}: missing the canonical inspect-dispatch-isolation read (fail-closes unknown/undocumented to none)`, + ); + // Major 3: fail-closed is right; reporting the failure AS a capability + // verdict is not. Both surfaces must be able to tell the two apart. + // #2486 review: the diagnostics name their state INSPECTED_ISOLATION, + // not ISOLATION. That is load-bearing, not cosmetic — #2728's + // "every dispatch-site degrade block re-records" guard scans for a + // literal `ISOLATION=none` in any workflow bash block, and a read-only + // surface has no sentinel to re-record. Naming the read differently + // keeps these files out of that scan BY CONSTRUCTION; the alternative + // was exempting the two files, which silently covered any future + // dispatch block added to them. + assert.ok( + !/^\s*ISOLATION=/m.test(src), + `${wf}: a read-only diagnostic must not assign the dispatch-site variable name ISOLATION — use INSPECTED_ISOLATION so #2728's re-record guard does not have to carve out this file`, + ); + assert.ok( + src.includes(ISOLATION_RESOLVED_FLAG), + `${wf}: must track ISOLATION_RESOLVED — a resolver failure is not a declaration of 'none' (#2486 review, Major 3)`, + ); + assert.ok( + !ISOLATION_COLLAPSING_FALLBACK.test(src), + `${wf}: '|| echo "none"' on the inspect read collapses "could not resolve" into "declares none" — capture the raw value and branch on ISOLATION_RESOLVED instead`, + ); + // #2486 round 4 (B1): the RECORDING resolver is dispatch-only. On + // current next, `query dispatch-isolation` persists its decision to + // .gsd/dispatch-isolation-sentinel.json as an unconditional #3045 side + // effect, and the isolation guard hooks hard-fail dispatches that + // disagree with the recorded sentinel. An inspection surface calling it + // would let /gsd:health or /gsd:settings hard-block executor dispatch + // for the sentinel's lifetime — across sessions, since the sentinel + // root resolves linked worktrees to the main checkout. + assert.ok( + !src.includes('query dispatch-isolation'), + `${wf}: calls the RECORDING dispatch-isolation verb — inspection surfaces must use inspect-dispatch-isolation, which never writes the sentinel`, + ); + assert.ok( + src.includes('"$INSPECTED_ISOLATION" = "none"') || src.includes('`INSPECTED_ISOLATION` = `none`'), + `${wf}: must gate on ISOLATION = none, the only value that cannot honor worktrees`, + ); + } + }); + + test('the isolation read is runtime-neutral — no per-runtime stamping rewrites it', () => { + // inspect-dispatch-isolation resolves the runtime internally and fail-closes, + // so unlike the #1521 runtime read it must survive every emit byte-identical. + // allow-test-rule: integration-test-input (#2486) + // The workflow source is fed to _applyRuntimeRewrites as real fixture input; + // the assertion is on the transformation's output. + for (const rt of NON_CLAUDE_RUNTIMES) { + for (const wf of RUNTIME_BRANCH_WORKFLOWS) { + const out = conversion._applyRuntimeRewrites(readWorkflow(wf), rt, `$HOME/.${rt}/`, true, undefined); + assert.ok( + out.includes(ISOLATION_LINE), + `${rt}/${wf}: the inspect-dispatch-isolation read must not be rewritten by per-runtime stamping`, + ); + } + // The settings tri-state read must survive stamping too: if + // _stampNonClaudeRuntimeDefaults ever matched the bare (no-fallback) + // shape, absence would again collapse into an explicit false and the + // pre-selection rule would go dead on that runtime. + const settingsOut = conversion._applyRuntimeRewrites(readWorkflow('settings.md'), rt, `$HOME/.${rt}/`, true, undefined); + assert.ok( + settingsOut.includes('USE_WORKTREES_CURRENT=$(gsd_run query config-get workflow.use_worktrees --raw 2>/dev/null)'), + `${rt}/settings.md: the bare tri-state worktrees read must survive per-runtime stamping byte-identical`, + ); + } + }); + + test('claude emit of settings.md and health.md keeps the recommended Yes option unchanged', () => { + const settingsOut = conversion._applyRuntimeRewrites(readWorkflow('settings.md'), 'claude', '$HOME/.claude/', true, undefined); + assert.ok( + settingsOut.includes('{ label: "Yes (Recommended)", description: "Each parallel executor runs in its own worktree branch — no conflicts between agents." }'), + 'claude/settings.md: the worktree-capable Worktrees question must be unchanged', + ); + }); + + test('settings.md source carries the isolation-none branch: no enabling option, never persist true', () => { + const src = readWorkflow('settings.md'); + assert.ok( + src.includes('**Conditional options — Worktrees (#2486):**'), + 'settings.md: missing the conditional-options block for the Worktrees question', + ); + // Round 4: the current-value read carries NO --default/fallback on purpose. + // _stampNonClaudeRuntimeDefaults rewrites the canonical fallback line to + // `--default false || echo "false"` on every non-Claude emit, which made + // key-absence indistinguishable from an explicit false — and the + // "pre-select Leave unchanged only when absent" rule dead there. The bare + // read signals absence as empty output and matches no stamp pattern. + assert.ok( + src.includes('USE_WORKTREES_CURRENT=$(gsd_run query config-get workflow.use_worktrees --raw 2>/dev/null)'), + 'settings.md: current worktrees value must use the bare tri-state read (empty = absent)', + ); + assert.ok( + !src.includes(`USE_WORKTREES_CURRENT=$(gsd_run query ${TRUE_WT_LINE})`), + 'settings.md: the stampable fallback read collapses absent into false on non-Claude emits', + ); + assert.ok( + src.includes('NEVER write `workflow.use_worktrees: true` from this workflow when the runtime declares no isolation primitive'), + 'settings.md: missing the never-persist-true instruction for isolation-none runtimes', + ); + assert.ok( + src.includes('{ label: "No (Recommended)", description: "Write use_worktrees: false.'), + 'settings.md: isolation-none branch must recommend No', + ); + assert.ok( + src.includes('{ label: "Leave unchanged", description: "Do not write the key.'), + 'settings.md: isolation-none branch must offer leaving the key untouched for shared configs', + ); + // Round 2: in the broken-inheritance case (explicit non-false value) the + // pre-selected default must be the repair, not "Leave unchanged". + // Round-5 review: this used to pin `Pre-select "Leave unchanged" only + // when the key is absent`. That assumed an absent key is already safe, + // which holds only on an emit that stamped the default to false — + // execute-phase/quick/diagnose read it as `|| echo "true"` otherwise. The + // recommended default must now be the explicit repair in every case. + assert.ok( + src.includes('Pre-select "No (Recommended)" in every case, including an absent key'), + 'settings.md: the recommended default must be the explicit false — an absent key is safe only on a stamped emit', + ); + assert.ok( + !/Pre-select "Leave unchanged" only when the key is absent/.test(src), + 'settings.md: the old absent-key-is-safe pre-selection rule is falsified on un-stamped emits (#2486 round-5 review)', + ); + assert.ok( + src.includes('when it is an explicit non-false value — the broken-inheritance case'), + 'settings.md: the broken-inheritance case must pre-select the recommended repair', + ); + }); + + test('health.md source carries the W025 isolation/worktrees compatibility check', () => { + const src = readWorkflow('health.md'); + assert.ok( + src.includes('W025:'), + 'health.md: missing the W025 diagnostic line', + ); + assert.ok( + src.includes('Status: DEGRADED'), + 'health.md: a config the execution workflows fail closed on must degrade the reported status', + ); + // #2486 review Major 1: W025's correctness must NOT depend on + // `_stampNonClaudeRuntimeDefaults` having rewritten the read. A + // `|| echo "true"` fallback makes an ABSENT key look like an explicit + // `true`, so the check fires on a config that never set it — correct only + // on a stamped emit, wrong on the un-stamped source/Claude emit. Pin the + // stamp-independent shape settings.md already uses. + assert.ok( + src.includes('USE_WORKTREES=$(gsd_run query config-get workflow.use_worktrees --raw 2>/dev/null)'), + 'health.md: the W025 worktrees read must be bare — a --default/|| fallback collapses key-absent into explicit-true (#2486 Major 1)', + ); + assert.ok( + !/workflow\.use_worktrees --raw 2>\/dev\/null \|\| echo/.test(src), + 'health.md: the W025 read reintroduced a fallback, so it depends on install-time stamping again (#2486 Major 1)', + ); + assert.ok( + src.includes('[ -n "$USE_WORKTREES" ]'), + 'health.md: W025 must require the key to be PRESENT before warning that the config "sets" it', + ); + }); + + // #2486 review Major 2: the coverage above is still text-matching. Execute + // the SHIPPED block so the predicate is proven by behavior — Major 1 would + // have been caught by any one of these cases. + test('W025 fires only on an explicit non-false key under isolation=none (behavioral, #2486 Major 2)', { skip: NO_BASH }, (t) => { + const scratch = createTempDir('gsd-w025-'); + t.after(() => cleanupDir(scratch)); + const src = readFileNormalized( + path.join(__dirname, '..', 'gsd-core', 'workflows', 'health.md'), + ); + const block = [...src.matchAll(/```bash\r?\n([\s\S]*?)```/g)] + .map(m => m[1]) + .find(b => b.includes('W025:')); + assert.ok(block, 'health.md: no ```bash block containing the W025 diagnostic'); + + /** Run the shipped block with `gsd_run` stubbed to the given answers. */ + const fire = (isolation, worktreesOut) => { + const harness = [ + 'set -u', + 'gsd_run() {', + ' case "$*" in', + ` *inspect-dispatch-isolation*) printf '%s' ${JSON.stringify(isolation)} ;;`, + // Faithful to the real CLI: `config-get` EXITS NON-ZERO for an absent + // key, it does not print empty and succeed. A stub that succeeds here + // would let `|| echo "true"` be reintroduced and still pass — the + // fallback only triggers on failure (#2486 round-7 review, Major 4). + worktreesOut === '' + ? ' *"config-get workflow.use_worktrees"*) return 1 ;;' + : ` *"config-get workflow.use_worktrees"*) printf '%s' ${JSON.stringify(worktreesOut)} ;;`, + " *) printf '' ;;", + ' esac; }', + block, + ].join('\n'); + const scriptPath = path.join(scratch, `w025-${isolation}-${worktreesOut || 'absent'}.sh`); + fs.writeFileSync(scriptPath, harness); + const res = runHook(scriptPath, [], { interpreter: 'bash' }); + assert.equal( + res.outcome, 'exited', + `W025 block did not complete cleanly: outcome=${res.outcome} ${res.stderr || ''}`, + ); + assert.equal(res.exitCode, 0, `W025 block exited ${res.exitCode}: ${res.stderr}`); + return res.stdout.includes('W025:'); + }; + + // The reviewer's exact repro: rt=qwen, source (un-stamped) emit, cfg={}. + // The key is absent, so `config-get` prints nothing. This FIRED before. + assert.equal( + fire('none', ''), + false, + 'W025 fired on an ABSENT use_worktrees key — the warning claims the config "sets" a non-false value, and it does not (#2486 Major 1 repro)', + ); + // The defect W025 actually exists for. + assert.equal( + fire('none', 'true'), + true, + 'W025 stayed quiet on an explicit true under isolation=none — that is the config execute-phase/quick fail closed on', + ); + assert.equal( + fire('none', 'false'), false, 'W025 fired on an explicit false — nothing to repair', + ); + // A host that CAN isolate is never the subject of this warning. + for (const iso of ['harness-worktree', 'orchestrator-worktree']) { + assert.equal( + fire(iso, 'true'), + false, + `W025 fired on ${iso}, which declares an isolation primitive — the gate is the declared capability, not the runtime name (#2584)`, + ); + } + }); + + // #2486 round-9 review, Major 3: the source-text pins above assert only + // that ISOLATION_RESOLVED is *mentioned*, so flipping the shipped block's + // `ISOLATION_RESOLVED=true` to `false` left every one of them green. What + // has to be pinned is the BRANCH: a resolver that answered and a resolver + // that failed must produce different W025 text, because the whole point is + // to stop reporting a failed query as a capability verdict. This test + // drives the shipped block twice and fails on the mutation. + test('W025 distinguishes a declared none from an unresolvable query (behavioral, #2486 Major 3)', { skip: NO_BASH }, (t) => { + const scratch = createTempDir('gsd-w025-provenance-'); + t.after(() => cleanupDir(scratch)); + const src = readFileNormalized( + path.join(__dirname, '..', 'gsd-core', 'workflows', 'health.md'), + ); + const block = [...src.matchAll(/```bash\r?\n([\s\S]*?)```/g)] + .map(m => m[1]) + .find(b => b.includes('W025:')); + assert.ok(block, 'health.md: no ```bash block containing the W025 diagnostic'); + + // `resolves` false = the inspect call exits non-zero, the real shape of a + // shim that cannot resolve. The worktrees key is an explicit true in both + // runs, so the ONLY difference is whether the capability query answered. + const runBlock = (resolves) => { + const harness = [ + 'set -u', + 'gsd_run() {', + ' case "$*" in', + resolves + ? " *inspect-dispatch-isolation*) printf 'none' ;;" + : ' *inspect-dispatch-isolation*) return 1 ;;', + " *\"config-get workflow.use_worktrees\"*) printf 'true' ;;", + " *) printf '' ;;", + ' esac; }', + block, + ].join('\n'); + const scriptPath = path.join(scratch, `w025-resolves-${resolves}.sh`); + fs.writeFileSync(scriptPath, harness); + const res = runHook(scriptPath, [], { interpreter: 'bash' }); + assert.equal(res.outcome, 'exited', `block did not complete: ${res.stderr || ''}`); + assert.equal(res.exitCode, 0, `block exited ${res.exitCode}: ${res.stderr}`); + return res.stdout; + }; + + const resolved = runBlock(true); + const unresolved = runBlock(false); + + // Both must warn — an explicit true is worth reporting either way. + assert.match(resolved, /W025:/, 'a resolved none with an explicit true must still warn'); + assert.match(unresolved, /W025:/, 'an unresolvable capability with an explicit true must still warn'); + + // ...but they must not say the same thing. + assert.notEqual( + resolved.trim(), + unresolved.trim(), + 'W025 emitted identical text whether or not the capability resolved — that is the Major 3 conflation', + ); + assert.match( + unresolved, + /could not resolve/i, + 'the unresolved branch must report that the query failed, not assert a capability verdict', + ); + assert.doesNotMatch( + unresolved, + /declares no executor-isolation primitive|has no usable executor-isolation primitive/i, + 'the unresolved branch must NOT claim the runtime has no primitive — nothing established that', + ); + assert.doesNotMatch( + resolved, + /could not resolve/i, + 'the resolved branch must state the capability finding, not a resolution failure', + ); + // Round-3 review: asserting only "differs and omits the other phrase" + // would stay green if the resolved text were replaced with anything at + // all. Pin what it must positively say — the capability finding and the + // consequence a user acts on. + assert.match( + resolved, + /no usable executor-isolation primitive/i, + 'the resolved branch must state the capability finding it actually reached', + ); + assert.match( + resolved, + /fail closed/i, + 'the resolved branch must state the consequence — that is what makes W025 actionable', + ); + assert.match( + resolved, + /use_worktrees/, + 'the resolved branch must name the offending key', + ); + // Both branches must still route the user to the same repair. + for (const [name, out] of [['resolved', resolved], ['unresolved', unresolved]]) { + assert.match(out, /(?:\/gsd[:-]settings|\$gsd-settings)\b/, `${name}: W025 must name the repair command`); + } + }); + + test('W025 is documented consistently across health.md and both config references', () => { + // The rename W020 -> W025 landed in health.md only; the two docs kept + // saying W020, which collides with a code src/verify.cts already emits. + for (const rel of ['gsd-core/workflows/health.md', 'docs/CONFIGURATION.md', 'gsd-core/references/planning-config.md']) { + const text = fs.readFileSync(path.join(__dirname, '..', rel), 'utf8'); + assert.ok(text.includes('W025'), `${rel}: must document the worktrees warning as W025`); + assert.ok( + !/\bW020\b[^)]{0,80}worktree/i.test(text), + `${rel}: stale W020 reference for the worktrees warning`, + ); + } + }); + + // Round 4 note: an earlier revision carried a W025-vs-src/verify.cts + // namespace-collision test here that read verify.cts as raw text — the + // source-grep shape RULESET.TESTS.delete-bad-tests says to delete, not + // exempt. Deleted without a behavioral replacement: verify.cts exposes no + // enumerable W-code registry to assert against, and building one is a + // shared-module refactor outside this fix. The namespace claim lives as + // guidance in health.md's error-codes note instead of as a fake test. + + test('the health.md error-codes table is not broken by the namespace note', () => { + // The note was inserted BETWEEN two rows, which terminates the GFM table + // and orphans the I001 row into literal pipe-delimited text. + const src = readWorkflow('health.md'); + const w025 = src.indexOf('| W025 |'); + const i001 = src.indexOf('| I001 |'); + const note = src.indexOf('Note: the `W0NN` warning-code namespace'); + assert.ok(w025 > -1 && i001 > -1 && note > -1, 'health.md: expected W025, I001 and the namespace note'); + assert.ok(i001 > w025, 'health.md: I001 row must follow the W025 row'); + assert.ok( + note > i001, + 'health.md: the namespace note must come AFTER the final table row — placing it between rows ends the table and orphans I001', + ); + }); + }); +} + // ──────────────────────────────────────────────────────────────────────── // Folded from tests/bug-2876-skill-frontmatter-quote.test.cjs — consolidation epic #1969 (B8 #1977) // ────────────────────────────────────────────────────────────────────────