diff --git a/.changeset/daring-jays-leap.md b/.changeset/daring-jays-leap.md new file mode 100644 index 000000000..7facea508 --- /dev/null +++ b/.changeset/daring-jays-leap.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3941 +--- +**Non-Claude installs now resolve their own runtime by default, and `depends_on` accepts the bare plan number.** A Codex, Cursor, or other non-Claude install with no `GSD_RUNTIME` set and no `config.runtime` key previously still reported `claude` everywhere, because the per-install runtime marker the installer writes was read by four hand-rolled copies but never by `resolveRuntime` itself; it is now the third precedence rung. Separately, `depends_on: ["01"]` now resolves to the in-phase sibling plan instead of silently dropping the dependency and collapsing the plan into wave 1 — a phase that previously ran all its plans in a single wave now executes in its declared waves. Codex sandbox permissions are also now derived from each agent's own tool contract instead of a hand-maintained map, and `validate agents` reports any drift via a new `sandbox_posture` field; both are byte-identical to today's behavior. (#3897) diff --git a/bin/install.js b/bin/install.js index 419e5a409..cdeb3350e 100755 --- a/bin/install.js +++ b/bin/install.js @@ -460,19 +460,32 @@ function resolveSharedHooksDirName(runtime) { return name; } -const CODEX_AGENT_SANDBOX = { - 'gsd-executor': 'workspace-write', - 'gsd-planner': 'workspace-write', - 'gsd-phase-researcher': 'workspace-write', - 'gsd-project-researcher': 'workspace-write', - 'gsd-research-synthesizer': 'workspace-write', - 'gsd-verifier': 'workspace-write', - 'gsd-codebase-mapper': 'workspace-write', - 'gsd-roadmapper': 'workspace-write', - 'gsd-debugger': 'workspace-write', - 'gsd-plan-checker': 'read-only', - 'gsd-integration-checker': 'read-only', -}; +// #3897 rung 3 — sandbox_mode derivation, the hold list, and the hold-roster +// validator now live in `src/codex-agent-toml.cts` (compiled to +// `gsd-core/bin/lib/codex-agent-toml.cjs`), NOT here. This module used to be +// the sole owner, and `agent-install-check.cts`'s `checkCodexSandboxPosture` +// lazily `require()`d THIS FILE to reach `deriveCodexSandboxMode` — but +// requiring `bin/install.js` runs its whole top-level script, including the +// CLI's ASCII banner print to stdout, which corrupted every stdout-JSON +// caller downstream of that posture check (`gsd-tools validate agents`). +// `codex-agent-toml.cjs` is a genuine leaf (no top-level side effects), so +// both this file and `agent-install-check.cts` import the derivation from +// there — ONE owner, no second predicate. See that module's header for the +// full rationale, and CAUSE B (below, `installCodexConfig`) for the removal +// of `validateCodexSandboxHolds`'s call from the install runtime path. +const { + CODEX_SANDBOX_HOLDS, + deriveCodexSandboxMode, + validateCodexSandboxHolds, + // #3897 list-form parse fix, Fix 3 (generative-fix-divergence): this + // file's own `generateCodexAgentToml` used to pull `tools:` via its + // private `extractFrontmatterField` (single-line only) instead of this + // shared reader — the two sandbox-feeding paths (this emitter and + // `agent-install-check.cts`'s `checkCodexSandboxPosture`) silently + // disagreed on YAML block-list `tools:` form. Both now route through this + // ONE extractor. + extractToolsValue, +} = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'codex-agent-toml.cjs')); // Copilot tool name mapping — Claude Code tools to GitHub Copilot tools // Tool mapping applies ONLY to agents, NOT to skills (per CONTEXT.md decision) @@ -4010,10 +4023,39 @@ function _resetCodexWarningDedupeForTests() { * @param {object|null} effortCfg — #443: merged effort config from readGsdEffectiveEffortConfig */ function generateCodexAgentToml(agentName, agentContent, modelOverrides = null, runtimeResolver = null, effortCfg = null, sandboxTier = 'codex-agent-sandbox') { - const sandboxMode = CODEX_AGENT_SANDBOX[agentName] || 'read-only'; const { frontmatter, body } = extractFrontmatterAndBody(agentContent); const frontmatterText = frontmatter || ''; + // #3897 list-form parse fix, Fix 3: `toolsRaw` MUST come from the same + // shared `extractToolsValue` reader `checkCodexSandboxPosture` uses, not + // this file's own `extractFrontmatterField` — the two used to disagree on + // YAML block-list `tools:` form (`extractFrontmatterField`'s single-line + // regex read only the first list item), which is exactly the generative- + // fix-divergence shape CLAUDE.md warns about for two paths feeding one + // derivation. `extractToolsValue` does its own `---`-delimited frontmatter + // scan of the full `agentContent`, so it is not re-derived from + // `frontmatterText` here. + const toolsRaw = extractToolsValue(agentContent) ?? ''; const resolvedName = extractFrontmatterField(frontmatterText, 'name') || agentName; + // #3897 rung 3 — derived from the role's own tool contract (HALT.md option + // 2). The former hand-maintained CODEX_AGENT_SANDBOX map is deleted (ADR-3473 + // §8.3): it was fully redundant with this derivation, zero disagreements + // across all 11 entries. Never a silent `|| 'read-only'` fallback either. + // Derivation itself lives in codex-agent-toml.cjs, which does no frontmatter + // parsing of its own (no third copy of that extraction) — it takes the + // already-resolved `tools:` value, extracted via the shared reader above. + // + // #3897 security review F1 (blocker): pass BOTH candidate identities — + // `agentName` (the caller's filename-stem identity) AND `resolvedName` + // (the frontmatter `name:` this function's OWN emitted `name = ...` line + // uses, and what `installCodexConfig`'s caller keys the output PATH on) — + // never just one. Deciding the sandbox for `agentName` alone and applying + // it to an artifact that a DIFFERENT identity (`resolvedName`) names is + // exactly how a held role's own `.toml` could end up `workspace-write` + // (rename the source file, or plant a sibling whose `name:` collides with + // a held role). `deriveCodexSandboxMode` takes the most restrictive result + // across every candidate — see its doc and `isSandboxHeld` in + // codex-agent-toml.cjs. + const sandboxMode = deriveCodexSandboxMode([agentName, resolvedName], toolsRaw); const resolvedDescription = toSingleLine( extractFrontmatterField(frontmatterText, 'description') || `GSD agent ${resolvedName}` ); @@ -6925,6 +6967,36 @@ function installCodexConfig(targetDir, agentsSrc, sandboxTier = 'codex-agent-san } fs.mkdirSync(agentsTomlDir, { recursive: true }); + // #3897 rung 3 (CAUSE B fix) — validateCodexSandboxHolds is deliberately + // NOT called here. The "no stale holds" invariant is a REPO invariant about + // the canonical roster in `agents/`, not a property of whatever directory + // an install happens to read from: a partial or synthetic `agentsSrc` (a + // test fixture, a `--config-dir` subset) legitimately contains only a few + // agents, and a held role simply absent from THIS source dir must be + // inert, not fatal. Throwing here also masked unrelated failures further + // down this same loop (e.g. the name-injection/path-escape guard on + // `agentTomlPath` below), since this check ran first and unconditionally. + // The invariant is still enforced — as a test over the real `agents/` + // roster (tests/codex-config.test.cjs T24/T25) — just never on this + // runtime path. See src/codex-agent-toml.cts's `validateCodexSandboxHolds` + // docblock for the full rationale. + // + // #3897 security review F2 (assessed post-F1-fix, verified by execution — + // not asserted): before F1's fix, the "runtime detector removed" gap here + // was real — a rename (case a) or a sibling `name:` clobber (case b) could + // reach this loop and land a held role's `.toml` at `workspace-write` with + // nothing here to catch it. After F1 (`deriveCodexSandboxMode` now decides + // over BOTH the filename stem and the resolved frontmatter `name:`, most + // restrictive wins), both cases were re-run end-to-end through this exact + // function and the EMITTED ARTIFACT for both is `read-only` — the + // dangerous condition no longer produces a wrong artifact, it produces the + // SAFE one. A detector guarding a now-fail-safe condition is not + // load-bearing, and restoring a throw here would re-break the legitimate + // partial-source-dir case CAUSE B removed it for (see above). Regression + // coverage for both cases lives in `tests/codex-config.test.cjs` (F1(a) + // rename / F1(b) sibling-clobber rows), asserted on the emitted `.toml`'s + // `sandbox_mode`, not on the derivation's return value. + const agentEntries = fs.readdirSync(agentsSrc).filter(f => f.startsWith('gsd-') && f.endsWith('.md')); const agents = []; @@ -6953,7 +7025,20 @@ function installCodexConfig(targetDir, agentsSrc, sandboxTier = 'codex-agent-san // CLAUDE.md neutralization via neutralizeAgentReferences(..., 'AGENTS.md'). content = convertClaudeToCodexMarkdown(content); const { frontmatter } = extractFrontmatterAndBody(content); - const name = extractFrontmatterField(frontmatter, 'name') || file.replace('.md', ''); + // #3897 security review F1 (blocker, post-merge): this loop used to key + // the sandbox/hold decision off ONLY the filename stem while the emitted + // `.toml`'s OUTPUT PATH below is keyed off `name` (frontmatter-derived, + // attacker-editable) — so a renamed source file, or a sibling file whose + // `name:` collides with a held role, could make the decided identity and + // the landed artifact disagree, widening a held role's own file to + // `workspace-write`. `generateCodexAgentToml` (below) now derives + // `sandbox_mode` over BOTH the filename stem it is given AND the + // frontmatter `name:` it resolves internally, taking the most + // restrictive result — this loop no longer needs to choose one identity + // for that call; see `deriveCodexSandboxMode`'s doc in + // `codex-agent-toml.cts` for the resolution. + const fileStem = file.replace(/\.md$/, ''); + const name = extractFrontmatterField(frontmatter, 'name') || fileStem; const description = extractFrontmatterField(frontmatter, 'description') || ''; agents.push({ name, description: toSingleLine(description) }); @@ -6975,7 +7060,10 @@ function installCodexConfig(targetDir, agentsSrc, sandboxTier = 'codex-agent-san // #443 — pass unified effort config so model_reasoning_effort in the .toml // follows the same config-driven precedence as the Claude .md effort key. const effortCfg = readGsdEffectiveEffortConfig(targetDir); - const tomlContent = generateCodexAgentToml(name, content, modelOverrides, runtimeResolver, effortCfg, sandboxTier); + // Pass `fileStem`; `generateCodexAgentToml` itself additionally resolves + // and folds in the frontmatter `name:` for the sandbox decision (F1 + // above) — this call site does not need to pass `name` explicitly. + const tomlContent = generateCodexAgentToml(fileStem, content, modelOverrides, runtimeResolver, effortCfg, sandboxTier); // Confine the per-agent write to the agents/ dir itself: a crafted agent // `name` containing path separators must not escape agents/ (which would let // it clobber config.toml or write elsewhere under the configHome). @@ -13712,7 +13800,10 @@ module.exports = { GSD_CLAUDE_LEGACY_ALLOW_PERMISSIONS, GSD_CLAUDE_DENY_PERMISSIONS, GSD_CODEX_MARKER, - CODEX_AGENT_SANDBOX, + // #3897 rung 3 (ADR-3473 §8.3, HALT.md option 2) + CODEX_SANDBOX_HOLDS, + deriveCodexSandboxMode, + validateCodexSandboxHolds, getGlobalDir, getConfigDirFromHome, resolveKiloConfigPath, diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 73348f9e9..307a2cc58 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -464,7 +464,7 @@ node gsd-tools.cjs capability set code-review --gate workflow.code_review=false node gsd-tools.cjs query teams-status [--active] ``` -Read-only detector for claude-code's experimental agent-teams feature (issue #1355). Resolves the runtime via the canonical `GSD_RUNTIME` → `config.runtime` → `'claude'` precedence, then checks `CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS`. +Read-only detector for claude-code's experimental agent-teams feature (issue #1355). Resolves the runtime via the canonical `GSD_RUNTIME` → `config.runtime` → per-install runtime marker → `'claude'` precedence (#3897), then checks `CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS`. **Default (no flags):** prints a JSON object and exits 0: diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index afe1d3feb..22ff8f776 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -2001,10 +2001,10 @@ Only the targeted lines are removed. Line endings, BOM, key order, comments, bla ### `validate agents` -Check that the GSD agents are installed for the active runtime — and, on Codex, that the installed `.toml` files satisfy the passive model posture. +Check that the GSD agents are installed for the active runtime — and, on Codex, that the installed `.toml` files satisfy the passive model posture and the derived sandbox posture. **Prerequisites:** GSD installed for a runtime -**Produces:** Installed / missing / incomplete agent lists, plus a `codex_posture` report +**Produces:** Installed / missing / incomplete agent lists, plus `codex_posture` and `sandbox_posture` reports ```bash node gsd-tools.cjs validate agents @@ -2020,6 +2020,17 @@ node gsd-tools.cjs validate agents Presence and posture are separate verdicts: a missing agent is reported in `missing`, not as a posture violation. See [ADR-2313](adr/2313-codex-passive-model-posture.md) for the posture itself, and [How to recover and troubleshoot](how-to/recover-and-troubleshoot.md#if-codex-agents-fail-to-spawn-with-a-400-about-an-unsupported-model) for the symptom-led walkthrough. +**`sandbox_posture` (#3897, ADR-3473 §8.3)** is the sibling check for `sandbox_mode`: each installed Codex `.toml`'s `sandbox_mode` is compared against the value its role's own `tools:` frontmatter derives (`Write`/`Edit` declared → `workspace-write`, else `read-only`). Same shape and short-circuits as `codex_posture` — populated only on `codex`, `not_codex` elsewhere, read-only, and it never makes `validate agents` exit non-zero on its own; both posture checks are report-only fields for a human or caller to act on. + +| Field | Meaning | +|---|---| +| `ok` | `true` when no installed `.toml`'s `sandbox_mode` disagrees with its derived expectation | +| `violations[]` | `{agent, file, expected, found}` for a drift, or `{agent, file, reason: "unreadable"}` when the file could not be read | +| `checked[]` | Every `.toml` inspected | +| `reason` | `not_codex` (active runtime isn't Codex) or `agents_dir_missing` — mirrors `codex_posture`'s short-circuit reasons | + +A custom or non-roster agent (no matching file under the bundled `agents/*.md` source) has nothing to derive an expectation from and is silently excluded from `violations` — this check only asserts against the shipped roster's own declared tool contract. + --- ### `state update ` diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 23374f535..5f31a4afa 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -199,6 +199,7 @@ - [Runtime Identity](#168-runtime-identity) - ["Failure Is a Value" — Strict Argv Rejection and the `--pick` Absence Contract](#3884-failure-is-a-value--strict-argv-rejection-and-the---pick-absence-contract) - [No Silent Swallow, No Verdict From Dropped Data](#3885-no-silent-swallow-no-verdict-from-dropped-data) + - [Runtime Marker Resolution, Derived Codex Sandbox, and In-Phase Short-Form Dependencies](#3897-runtime-marker-resolution-derived-codex-sandbox-and-in-phase-short-form-dependencies) --- @@ -3824,6 +3825,94 @@ errno rather than folding it into a retry. --- +### 3897. Runtime Marker Resolution, Derived Codex Sandbox, and In-Phase Short-Form Dependencies + +**Purpose:** ADR-3473 §8.3 states the rule directly — one implementation per +invariant, not a hand-maintained copy that quietly drifts from the rule it +stands in for. This closes three instances across `gsd-tools`: a resolver +that never read the install-time signal it was documented to read, a +Codex sandbox map that was fully redundant with the tool contract it stood +in for, and a dependency-resolution tier lost when the SDK lineage was +retired. + +**A non-Claude install now resolves its own runtime with no config +needed (#3897).** `resolveRuntime`'s ladder was `GSD_RUNTIME` env var → +project `config.runtime` → `'claude'` — the per-install `.gsd-runtime` +marker the installer has written beside `VERSION` since #2297 was read by +four separate hand-rolled copies (`model-resolver.cts`, and two more inside +`gsd-cursor-subagent-start.js`), but never by `resolveRuntime` itself. A +Codex, Cursor, or other non-Claude install with no `GSD_RUNTIME` set and no +`runtime` key in `.planning/config.json` therefore still resolved `claude` +everywhere `resolveRuntime` is consulted (slash-command style, `query +teams-status`, `validate agents`'s agent-directory selection, and 19 other +call sites). The marker is now the third rung — `GSD_RUNTIME` → `config.runtime` +→ install marker → `'claude'` — so those installs resolve their own runtime +by default. The marker's contents are never trusted verbatim: they are routed +through the same name-normalization the env rung already uses, so a marker +holding an unexpected or hostile value degrades exactly like an unexpected +`GSD_RUNTIME` value would. + +**Codex sandbox permissions are derived from each agent's own tool contract, +not a hand-maintained map (#3897).** `generateCodexAgentToml` looked up +`sandbox_mode` in an 11-entry `CODEX_AGENT_SANDBOX` map, falling back to +`read-only` — silently — for every role the map didn't name. Measured against +all 35 shipped roles, the map's 11 entries agree with deriving `sandbox_mode` +from each role's declared `tools:` frontmatter (`workspace-write` when it +declares `Write` or `Edit`, `read-only` otherwise) with **zero disagreements**, +so the map is deleted rather than clamped. The fallback, however, was +under-granting: 16 of the 24 roles that hit it declare `Write`/`Edit` and +would derive `workspace-write`. Pending a decision on whether Codex actually +enforces `sandbox_mode` (a question the derivation can't answer on its own), +those 16 are held at `read-only` by an explicit, self-invalidating hold +list — a hold whose role no longer derives broader, or that names a role +that no longer exists, fails loudly instead of being silently honored. +**Every one of the 35 emitted `.toml` files is byte-identical to before this +change** — the fix is in provenance (an explicit, reviewable rule instead of +a silent default), not in any installed agent's actual permissions today. + +**`validate agents` now reports Codex sandbox drift (#3897).** A new +`sandbox_posture` field — report-only, exit 0, same shape as the existing +`codex_posture` — flags any installed Codex `.toml` whose `sandbox_mode` +disagrees with what its role's tool contract derives. Populated only when +the active runtime is `codex`. + +**`depends_on` accepts the bare plan number (#3897).** A plan's frontmatter +could already reference a dependency by its full id (`"03-01-auth-hardening"`) +or its canonical phase-plan prefix (`"03-01"`). A third form — the bare +plan number alone (`"01"`) — existed in the retired SDK lineage but was lost +when that lineage was consolidated; a plan written with it silently dropped +the edge entirely, collapsing into wave 1 regardless of its declared +dependency. That form is restored, scoped to the **same phase only**: `"01"` +resolves to the sibling plan whose canonical id ends `-01`. This is an +observable behavior change — a phase whose plans used the bare form and had +silently collapsed into a single wave will now execute in its actual declared +waves. Two plans in the same phase sharing a bare form resolve first-write-wins, +by sorted plan-file order — deterministic, but arbitrary where the collision +happens, matching the retired behavior exactly. + +**Known limits:** +- The 17 held Codex roles are pinned at `read-only`, not widened. A faithful + derivation from the tool contract would widen them, because they declare + `Write` or `Edit`; the previous hand-maintained map never listed them and + they fell through a silent `|| 'read-only'` default instead. Deriving *and* + holding keeps emitted TOML byte-identical for all 35 roles today while the + derivation becomes the single owner of the rule. Widening them is a follow-up + once Codex's actual enforcement of `sandbox_mode` is confirmed — until then a + hold is reversible and a widened sandbox is not. The hold list is + self-invalidating: an entry naming a role that no longer derives broader, or + that has no file in the shipped roster, fails rather than rotting into the + subset map this change deletes. +- The bare plan-number form is ambiguous by construction across two plans in + the same phase that share a short form; first-write-wins is deterministic + but not a conflict warning. Prefer the full or canonical id when a phase's + plan numbering risks a short-form collision. +- The install marker never feeds model-tier resolution (`model_profile_overrides`, + `model_policy.runtime_tiers`) — that still reads `config.runtime` alone, and + reporting-only host detection (`agent_runtime`) is a separate, pre-existing + ladder this change does not touch. + +--- + _Generated by `scripts/gen-features.cjs` — add a fragment under `docs/features/` and run `--write`._ diff --git a/docs/adr/3473-enforcement-by-construction.md b/docs/adr/3473-enforcement-by-construction.md index 7f82e0b66..96580a283 100644 --- a/docs/adr/3473-enforcement-by-construction.md +++ b/docs/adr/3473-enforcement-by-construction.md @@ -334,6 +334,16 @@ Both close #3349 and #3360, which are **read-side** defects a real parser fixes > **The guard roster names no §8.3 casualty.** Its only §8.3-tagged row is `local/no-adhoc-regex-escape`, marked *widened*, not retired. Nothing is retired by this rule. > > **What this means for the phase:** unlike §8.2, this section is genuinely unbuilt — the rewrites it asserts have not happened. It is a work list, not a conformance check, and it should be read that way. +> +> **ANSWER, 2026-08-27 (#3897, carrying Phase 6's remaining rungs) — rungs 2–4 built; re-measured, not inherited.** +> +> | As corrected above | Now measured | +> |---|---| +> | "`resolveRuntime` reads no marker at all… PR #3382 … CLOSED and unmerged" | **Fixed.** `resolveRuntime` (`src/runtime-slash.cts`) now resolves `GSD_RUNTIME > config.runtime > install marker > 'claude'`. The four hand-rolled `readInstallRuntimeMarker` copies (`model-resolver.cts` + two in `gsd-cursor-subagent-start.js` + one in `gsd-agent-isolation-guard.js`) are consolidated into one module-level-cached reader owned by `runtime-slash.cts`; the marker's raw contents are routed through the same `resolveRuntimeNameFromCandidates` normalization the env rung uses, never trusted verbatim. Mined from #3382 per this ADR's own direction, not re-derived. | +> | "`generateCodexAgentToml` still reads `CODEX_AGENT_SANDBOX[agentName] \|\| 'read-only'`… `checkAgentsInstalled` has no `sandbox_mode` assertion" | **Fixed, with a qualification.** `CODEX_AGENT_SANDBOX` (11 entries) and the silent `\|\| 'read-only'` fallback are deleted; `sandbox_mode` now derives from each role's declared `tools:` frontmatter (`Write`/`Edit` present → `workspace-write`, else `read-only`). Emitted TOML is byte-identical for all 35 roles today: 17 roles that would derive broader are held at `read-only` behind an explicit, self-invalidating hold list (`CODEX_SANDBOX_HOLDS`) pending a decision on whether Codex enforces `sandbox_mode`. The 17th was surfaced only by review: the tools reader was single-line, so a YAML list-form `tools:` block returned just its first item and `gsd-nyquist-auditor`'s declared `Write`/`Edit` were read as an absence. **Deriving from a declaration you cannot parse is not deriving** — the reader now handles both shapes, and the role joined the hold list rather than the count being left flattering. **That conflict was surfaced rather than resolved silently:** §8.3's own criterion asks both that the sandbox *derive from the declared tool contract* and that *no role gain a broader sandbox*, and those cannot both hold — a faithful derivation widens 17 roles that the deleted map never listed and that fell through its silent `|| 'read-only'` default. The resolution taken is derive-and-hold: the derivation becomes the single owner of the rule now, and each hold is released as its role's enforcement question is answered. A hold is reversible; a widened sandbox that turns out to be enforced is not. `checkAgentsInstalled` (`src/agent-install-check.cts`) still checks presence only; the semantic assertion is a sibling function, `checkCodexSandboxPosture`, wired into `validate agents`'s `sandbox_posture` field — report-only, exit 0, matching the pre-existing `codex_posture` precedent, not folded into `checkAgentsInstalled` itself. | +> | "The `shortFormToId` rule below is accurate — no such tier exists on `next`" | **Now built.** `resolveDependencyId` (`src/phase.cts`) gained the third, in-phase-only tier recovered verbatim from the retired SDK lineage (`sdk/src/query/phase.ts` at `11918dcc3^`): `depends_on: ["01"]` resolves to the sibling plan whose canonical id ends `-01`, first-write-wins on a same-phase collision. The display mapping at `phase.cts:961` stays passthrough by design (#3785) — this tier is not routed through it. | +> +> The guard-roster line above is unaffected: no lint guard is retired by this rung, only the hand-maintained map and the four marker-reader copies. **Rule — consolidation carries invariants forward explicitly.** A lineage consolidation may not delete an invariant along with the surface that held it. The `shortFormToId` tier existed in the retired SDK lineage; the surviving lineage never received it, the gap was recorded only in an archived changeset and a `// KNOWN GAP:` comment, and both went away with the surface (#3427). **A parity note in an archived changeset is not a tracking mechanism.** diff --git a/docs/features/runtime-marker-codex-sandbox-derivation-and-short-form-deps.md b/docs/features/runtime-marker-codex-sandbox-derivation-and-short-form-deps.md new file mode 100644 index 000000000..e21bdcdb6 --- /dev/null +++ b/docs/features/runtime-marker-codex-sandbox-derivation-and-short-form-deps.md @@ -0,0 +1,89 @@ +--- +id: 3897 +title: Runtime Marker Resolution, Derived Codex Sandbox, and In-Phase Short-Form Dependencies +group: v1.7.0 Features +--- + +**Purpose:** ADR-3473 §8.3 states the rule directly — one implementation per +invariant, not a hand-maintained copy that quietly drifts from the rule it +stands in for. This closes three instances across `gsd-tools`: a resolver +that never read the install-time signal it was documented to read, a +Codex sandbox map that was fully redundant with the tool contract it stood +in for, and a dependency-resolution tier lost when the SDK lineage was +retired. + +**A non-Claude install now resolves its own runtime with no config +needed (#3897).** `resolveRuntime`'s ladder was `GSD_RUNTIME` env var → +project `config.runtime` → `'claude'` — the per-install `.gsd-runtime` +marker the installer has written beside `VERSION` since #2297 was read by +four separate hand-rolled copies (`model-resolver.cts`, and two more inside +`gsd-cursor-subagent-start.js`), but never by `resolveRuntime` itself. A +Codex, Cursor, or other non-Claude install with no `GSD_RUNTIME` set and no +`runtime` key in `.planning/config.json` therefore still resolved `claude` +everywhere `resolveRuntime` is consulted (slash-command style, `query +teams-status`, `validate agents`'s agent-directory selection, and 19 other +call sites). The marker is now the third rung — `GSD_RUNTIME` → `config.runtime` +→ install marker → `'claude'` — so those installs resolve their own runtime +by default. The marker's contents are never trusted verbatim: they are routed +through the same name-normalization the env rung already uses, so a marker +holding an unexpected or hostile value degrades exactly like an unexpected +`GSD_RUNTIME` value would. + +**Codex sandbox permissions are derived from each agent's own tool contract, +not a hand-maintained map (#3897).** `generateCodexAgentToml` looked up +`sandbox_mode` in an 11-entry `CODEX_AGENT_SANDBOX` map, falling back to +`read-only` — silently — for every role the map didn't name. Measured against +all 35 shipped roles, the map's 11 entries agree with deriving `sandbox_mode` +from each role's declared `tools:` frontmatter (`workspace-write` when it +declares `Write` or `Edit`, `read-only` otherwise) with **zero disagreements**, +so the map is deleted rather than clamped. The fallback, however, was +under-granting: 16 of the 24 roles that hit it declare `Write`/`Edit` and +would derive `workspace-write`. Pending a decision on whether Codex actually +enforces `sandbox_mode` (a question the derivation can't answer on its own), +those 16 are held at `read-only` by an explicit, self-invalidating hold +list — a hold whose role no longer derives broader, or that names a role +that no longer exists, fails loudly instead of being silently honored. +**Every one of the 35 emitted `.toml` files is byte-identical to before this +change** — the fix is in provenance (an explicit, reviewable rule instead of +a silent default), not in any installed agent's actual permissions today. + +**`validate agents` now reports Codex sandbox drift (#3897).** A new +`sandbox_posture` field — report-only, exit 0, same shape as the existing +`codex_posture` — flags any installed Codex `.toml` whose `sandbox_mode` +disagrees with what its role's tool contract derives. Populated only when +the active runtime is `codex`. + +**`depends_on` accepts the bare plan number (#3897).** A plan's frontmatter +could already reference a dependency by its full id (`"03-01-auth-hardening"`) +or its canonical phase-plan prefix (`"03-01"`). A third form — the bare +plan number alone (`"01"`) — existed in the retired SDK lineage but was lost +when that lineage was consolidated; a plan written with it silently dropped +the edge entirely, collapsing into wave 1 regardless of its declared +dependency. That form is restored, scoped to the **same phase only**: `"01"` +resolves to the sibling plan whose canonical id ends `-01`. This is an +observable behavior change — a phase whose plans used the bare form and had +silently collapsed into a single wave will now execute in its actual declared +waves. Two plans in the same phase sharing a bare form resolve first-write-wins, +by sorted plan-file order — deterministic, but arbitrary where the collision +happens, matching the retired behavior exactly. + +**Known limits:** +- The 17 held Codex roles are pinned at `read-only`, not widened. A faithful + derivation from the tool contract would widen them, because they declare + `Write` or `Edit`; the previous hand-maintained map never listed them and + they fell through a silent `|| 'read-only'` default instead. Deriving *and* + holding keeps emitted TOML byte-identical for all 35 roles today while the + derivation becomes the single owner of the rule. Widening them is a follow-up + once Codex's actual enforcement of `sandbox_mode` is confirmed — until then a + hold is reversible and a widened sandbox is not. The hold list is + self-invalidating: an entry naming a role that no longer derives broader, or + that has no file in the shipped roster, fails rather than rotting into the + subset map this change deletes. +- The bare plan-number form is ambiguous by construction across two plans in + the same phase that share a short form; first-write-wins is deterministic + but not a conflict warning. Prefer the full or canonical id when a phase's + plan numbering risks a short-form collision. +- The install marker never feeds model-tier resolution (`model_profile_overrides`, + `model_policy.runtime_tiers`) — that still reads `config.runtime` alone, and + reporting-only host detection (`agent_runtime`) is a separate, pre-existing + ladder this change does not touch. diff --git a/docs/reference/plan-md.md b/docs/reference/plan-md.md index 63c21b45b..a6bd70fd5 100644 --- a/docs/reference/plan-md.md +++ b/docs/reference/plan-md.md @@ -69,7 +69,7 @@ must_haves: | `plan` | Yes | string | Plan number within the phase, e.g. `02`. | | `type` | Yes | `execute` or `tdd` | `execute` for standard plans; `tdd` for test-driven plans where tests are written before implementation. | | `wave` | Yes | integer | Execution wave. Plans in wave 1 run in parallel (no dependencies). Plans in wave 2+ wait for all plans in the previous wave to complete. Pre-computed at plan time by `gsd-planner`. | -| `depends_on` | Yes | array of plan IDs | Plans this plan must wait for. Empty array = wave 1. Example: `["03-01"]` means this plan runs after Plan 01 in Phase 3. | +| `depends_on` | Yes | array of plan IDs | Plans this plan must wait for. Empty array = wave 1. Accepts three forms, resolved in order: the full plan id (`"03-01-auth-hardening"`), the canonical phase-plan prefix (`"03-01"`), or the bare plan number (`"01"`, #3897) — which resolves to the sibling plan in the **same phase** whose canonical id ends `-01`. The bare form is in-phase only; it never resolves across phases. If two plans in the same phase share a bare form, the first one (by sorted plan-file order) wins — deterministic, but arbitrary when the collision happens, so prefer the full or canonical form when phase has any short-form collision risk. Example: `["03-01"]` means this plan runs after Plan 01 in Phase 3; from within Phase 3 itself, `["01"]` means the same thing. | | `files_modified` | Yes | array of paths | Every file this plan creates or modifies. Used by the plan-checker to detect same-wave file conflicts and by execute-phase for merge tracking. | | `files_deleted` | No | array of paths | Every file this plan deliberately **removes**. The post-wave cleanup gauntlet blocks the merge of any executor branch whose diff deletes a file — a net against a mass-deletion accident — and this field is the opt-in that names the exceptions. Matching is exact per path after separator normalization: a declared path merges, an undeclared one still blocks that plan's entry (and only that entry). There are no globs and no directory prefixes, so a declaration can never authorize more than it literally lists. Omit the field and the guard's original unconditional block stays in force, which is why absence is always the safe default (#3003). Counts toward same-wave conflict detection alongside `files_modified`: a plan deleting a file another plan in the same wave is editing is the sharpest conflict there is — one branch removes what the other is writing — so the two plans are pushed into different waves regardless of which side holds the deletion. | | `autonomous` | Yes | boolean | `true` when all tasks are type `auto`. `false` when the plan contains any `checkpoint:*` task that requires human interaction. | diff --git a/hooks/gsd-agent-isolation-guard.js b/hooks/gsd-agent-isolation-guard.js index ba5b21ab1..21c664b3c 100644 --- a/hooks/gsd-agent-isolation-guard.js +++ b/hooks/gsd-agent-isolation-guard.js @@ -95,40 +95,40 @@ function parseHarnessFlag(flag) { return { param: m[1], value: m[2] }; } -// ─── #3566: per-install runtime marker ──────────────────────────────────────── +// ─── #3897 rung 2: per-install runtime marker, single canonical owner ──────── // bin/install.js writes `/gsd-core/.gsd-runtime` for EVERY runtime // install (#2297), co-located with VERSION. Unlike `~/.gsd/defaults.json` — // which is host-wide and names whichever runtime's install ran LAST, the exact // leakage #2840's config.cjs change exists to prevent — the marker describes // THIS install, which is the property runtime identity needs on a machine -// with 2+ runtimes. Mirrors readInstallRuntimeMarker in src/model-resolver.cts -// (same cache + test-seam shape); this hook cannot import that module without -// dragging the whole model-resolution stack into a PreToolUse hot path, so the -// 5-line read lives here against the same sibling-layout assumption the hook's -// own require('../gsd-core/bin/lib/…') already makes. Epic #3473 B3 owns -// consolidating every marker reader into one shared seam. -let _installMarkerCache; // undefined = unread; null = known absent; string = value - +// with 2+ runtimes. Previously this hook held its own private reader/cache +// (one of four #3897 found); it now delegates to the single canonical owner, +// `src/runtime-slash.cts` (compiled to gsd-core/bin/lib/runtime-slash.cjs), +// reached through `ensureRuntimeBuild()` like every other compiled-lib require +// in this file (`scripts/lint-hooks-runtime-build-seam.cjs`). function readInstallRuntimeMarker() { - if (_installMarkerCache !== undefined) return _installMarkerCache; try { - const markerPath = path.join(__dirname, '..', 'gsd-core', '.gsd-runtime'); - const raw = fs.readFileSync(markerPath, 'utf-8').trim(); - _installMarkerCache = raw || null; + ensureRuntimeBuild(); + const runtimeSlash = require('../gsd-core/bin/lib/runtime-slash.cjs'); + return runtimeSlash.readInstallRuntimeMarker(); } catch { - // No marker: dev/source tree, or an install predating #2297 — "no signal - // from this rung", never a resolution failure. Falls through to the - // defaults rung below. - _installMarkerCache = null; + // Unbuilt runtime library, or any other failure reaching the canonical + // owner — "no signal from this rung" (N4), never a resolution failure. + return null; } - return _installMarkerCache; } -// Test seam for the marker rung (the dev/source tree has no marker file, so -// the read always bottoms out at null there — same seam contract as -// model-resolver.cts's _setInstallRuntimeMarkerForTests, #2297). +// Test seam for the marker rung — forwards to the canonical owner's seam so +// this hook and runtime-slash.cjs always share one cache (#3897 rung 2). function _setInstallRuntimeMarkerForTests(value) { - _installMarkerCache = value; + try { + ensureRuntimeBuild(); + const runtimeSlash = require('../gsd-core/bin/lib/runtime-slash.cjs'); + runtimeSlash._setInstallRuntimeMarkerForTests(value); + } catch { + // Test-only seam; an unbuilt library here means the test itself will fail + // downstream, which is a louder and more actionable signal than throwing here. + } } /** diff --git a/hooks/gsd-cursor-subagent-start.js b/hooks/gsd-cursor-subagent-start.js index 9c564b4d3..f090c51cf 100644 --- a/hooks/gsd-cursor-subagent-start.js +++ b/hooks/gsd-cursor-subagent-start.js @@ -342,35 +342,39 @@ function resolveIsolationDecision(data, { clock = Date, realpath = fs.realpathSy return { action: 'allow' }; } -// ─── #3566: per-install runtime marker ──────────────────────────────────────── -// Same contract as hooks/gsd-agent-isolation-guard.js's readInstallRuntimeMarker -// (mirroring src/model-resolver.cts #2297): bin/install.js writes -// `/gsd-core/.gsd-runtime` beside VERSION for every runtime install; -// this hook ships at `/hooks/`, so the marker is the `gsd-core` sibling -// of this file's own directory — the same sibling-layout assumption the -// require('../gsd-core/bin/lib/…') calls below already make. Epic #3473 B3 owns -// consolidating every marker reader into one shared seam. -let _installMarkerCache; // undefined = unread; null = known absent; string = value - +// ─── #3897 rung 2: per-install runtime marker, single canonical owner ──────── +// bin/install.js writes `/gsd-core/.gsd-runtime` beside VERSION for +// every runtime install (#2297); this hook ships at `/hooks/`, so the +// marker is the `gsd-core` sibling of this file's own directory. Previously +// this hook held its own private reader/cache (one of four #3897 found); it +// now delegates to the single canonical owner, `src/runtime-slash.cts` +// (compiled to gsd-core/bin/lib/runtime-slash.cjs), reached through +// `ensureRuntimeBuild()` like the other compiled-lib requires in this file +// (`scripts/lint-hooks-runtime-build-seam.cjs`). function readInstallRuntimeMarker() { - if (_installMarkerCache !== undefined) return _installMarkerCache; try { - const markerPath = path.join(__dirname, '..', 'gsd-core', '.gsd-runtime'); - const raw = fs.readFileSync(markerPath, 'utf-8').trim(); - _installMarkerCache = raw || null; + ensureRuntimeBuild(); + const runtimeSlash = require('../gsd-core/bin/lib/runtime-slash.cjs'); + return runtimeSlash.readInstallRuntimeMarker(); } catch { - // No marker: dev/source tree, or an install predating #2297 — "no signal - // from this rung", never a resolution failure. - _installMarkerCache = null; + // Unbuilt runtime library, or any other failure reaching the canonical + // owner — "no signal from this rung", never a resolution failure. + return null; } - return _installMarkerCache; } -// Test seam — same contract as model-resolver.cts's #2297 seam; the dev/source -// tree has no marker file, so spawned-hook tests (fresh process, no marker) -// are unaffected. +// Test seam — forwards to the canonical owner's seam so this hook and +// runtime-slash.cjs always share one cache (#3897 rung 2). Spawned-hook tests +// (fresh process, no marker) are unaffected. function _setInstallRuntimeMarkerForTests(value) { - _installMarkerCache = value; + try { + ensureRuntimeBuild(); + const runtimeSlash = require('../gsd-core/bin/lib/runtime-slash.cjs'); + runtimeSlash._setInstallRuntimeMarkerForTests(value); + } catch { + // Test-only seam; an unbuilt library here means the test itself will fail + // downstream, which is a louder and more actionable signal than throwing here. + } } /** diff --git a/src/agent-install-check.cts b/src/agent-install-check.cts index 86bcf3d2f..94c37281e 100644 --- a/src/agent-install-check.cts +++ b/src/agent-install-check.cts @@ -26,7 +26,21 @@ import { isAnthropicFlavoredModel } from './model-catalog.cjs'; // locally. Behavior is unchanged: scanTomlLines/stripBOM here are the exact // same lenient functions that used to live in this file — see // codex-agent-toml.cts's module header for the reader/writer reconciliation. -import { stripBOM, scanTomlLines } from './codex-agent-toml.cjs'; +// #3897 CAUSE A fix: `deriveCodexSandboxMode` needs the resolved `tools:` +// frontmatter VALUE, not raw content — this module used to reach that via +// `runtime-artifact-conversion.cjs`'s general-purpose +// `extractFrontmatterAndBody`/`extractFrontmatterField`, but that module's +// dependency chain (`command-roster.cjs` -> `scripts/fix-slash-commands.cjs`, +// a dev-only repo script) does not resolve from an installed tree — any test +// exercising a synthetic install dir blew up at module load. `codex-agent-toml.cjs` +// is already imported here, is a genuine leaf (node builtins only, zero +// side effects), and now exports `extractToolsValue` — a single-purpose +// `tools:`-VALUE reader (inline AND YAML block-list form, #3897 list-form +// parse fix), not a general frontmatter parser — for exactly this caller, +// shared with `bin/install.js`'s emitter so the two sandbox-feeding paths +// can never silently disagree, and no fourth copy of frontmatter parsing is +// added. +import { stripBOM, scanTomlLines, deriveCodexSandboxMode, extractToolsValue } from './codex-agent-toml.cjs'; interface AgentsInstalledResult { agents_installed: boolean; @@ -69,6 +83,31 @@ interface CodexModelPostureResult { reason?: PostureReason; } +/** + * #3897 rung 3 (ADR-3473 §8.3, HALT.md option 2) — a violation entry for + * {@link checkCodexSandboxPosture}. Distinct shape from {@link PostureViolation}: + * this check reports a drift between an EXPECTED and a FOUND `sandbox_mode`, + * not a single offending value. `reason` is populated only for the UNREADABLE + * case (mirrors {@link checkCodexModelPosture}); a drift violation carries + * `expected`/`found` instead. + */ +interface SandboxPostureViolation { + agent: string; + file: string; + reason?: PostureReason; + expected?: string; + found?: string; +} + +interface CodexSandboxPostureResult { + ok: boolean; + violations: SandboxPostureViolation[]; + checked: string[]; + agents_dir: string; + agent_runtime: string; + reason?: PostureReason; +} + // Matches the value-truncation convention in bin/install.js's // _warnCodexModelOverrideDropped: values over 64 chars are capped so an // oversized or secret-shaped config value can never reach a report in full. @@ -345,6 +384,161 @@ function checkCodexModelPosture(runtime?: string, projectRoot?: string): CodexMo }; } +// #3897 rung 3 — the canonical bundled `agents/*.md` source directory (the +// tool-contract source of truth), resolved the same install-relative way +// getAgentsDir resolves the claude case, WITHOUT that function's npm-global +// node_modules redirect: that redirect exists so a Codex/claude INSTALL is +// never validated against itself, but here we deliberately want the bundled +// copy every time — it is the only place the `tools:` contract a role +// derives its sandbox_mode from actually lives, in every install shape +// (repo-dev tree or npm-global package, where the package still ships its +// own agents/ alongside bin/ and gsd-core/). +function _canonicalAgentSourceDir(): string { + return path.join(__dirname, '..', '..', '..', 'agents'); +} + +/** + * Validate installed Codex `.toml` agents' `sandbox_mode` against what each + * role's own tool contract derives (#3897, ADR-3473 §8.3 criterion 3, + * HALT.md option 2). Sibling to {@link checkCodexModelPosture} — same shape, + * same short-circuits — but a different defect class: a TOML whose + * `sandbox_mode` disagrees with the derived expectation, not a bad `model`. + * + * `deriveCodexSandboxMode` (and the `CODEX_SANDBOX_HOLDS` hold list + its own + * self-invalidation check it embeds) is imported from `codex-agent-toml.cjs` + * — the shared, side-effect-free leaf both `bin/install.js`'s emitter and + * this posture check import from (CAUSE A fix, #3897: this function used to + * lazily `require('bin/install.js')` to reach the same derivation, but + * requiring `bin/install.js` runs its whole CLI top-level, including an + * ASCII banner print to stdout, which corrupted every stdout-JSON caller of + * this posture check, e.g. `gsd-tools validate agents`) — so the emitter and + * this posture check can never silently diverge on the same tool-contract + * predicate — the exact generative-fix-divergence shape this epic exists to + * close. `deriveCodexSandboxMode` itself is total (never throws, #3897 + * CAUSE C fix) — a held role whose supplied content no longer derives + * broader than its pin simply returns `read-only` rather than failing, + * because the pin is then a no-op. The staleness invariant (S4/S5) this + * check's imports still carry is enforced separately, at the real `agents/` + * roster level, by `validateCodexSandboxHolds` and + * `tests/codex-config.test.cjs` T24/T25 — not by this per-call derivation. + * + * Read-only: detects, never repairs — same posture as {@link checkCodexModelPosture}. + * + * @param runtime - the active runtime name; defaults to GSD_RUNTIME env, then 'claude' + * @param projectRoot - canonical project root for local-install discovery + */ +function checkCodexSandboxPosture(runtime?: string, projectRoot?: string): CodexSandboxPostureResult { + const resolvedRuntime = runtime ?? (process.env['GSD_RUNTIME'] || 'claude'); + if (resolvedRuntime !== 'codex') { + return { + ok: true, + violations: [], + checked: [], + agents_dir: '', + agent_runtime: resolvedRuntime, + reason: POSTURE_REASON.NOT_CODEX, + }; + } + + const agentsDir = getAgentsDir(resolvedRuntime, projectRoot); + if (!fs.existsSync(agentsDir)) { + return { + ok: true, + violations: [], + checked: [], + agents_dir: agentsDir, + agent_runtime: resolvedRuntime, + reason: POSTURE_REASON.AGENTS_DIR_MISSING, + }; + } + + const canonicalAgentsDir = _canonicalAgentSourceDir(); + + // Same symlink guard as checkCodexModelPosture, same reason (never follow + // an agents-dir symlink into an arbitrary file's content). + const tomlFiles = fs + .readdirSync(agentsDir) + .filter((entry) => { + if (!entry.endsWith('.toml')) return false; + try { + return fs.lstatSync(path.join(agentsDir, entry)).isFile(); + } catch { + return false; + } + }) + .sort(); + + const checked: string[] = []; + const violations: SandboxPostureViolation[] = []; + + for (const entry of tomlFiles) { + const agentName = entry.slice(0, -'.toml'.length); + const filePath = path.join(agentsDir, entry); + checked.push(agentName); + + let raw: string; + try { + raw = fs.readFileSync(filePath, 'utf8'); + } catch { + violations.push({ agent: agentName, file: filePath, reason: POSTURE_REASON.UNREADABLE }); + continue; + } + + // #3897 rung 4 (isolated correctness review, MINOR finding 2): route + // through the same block-aware `scanTomlLines` the sibling + // `checkCodexModelPosture` already uses, rather than a naive whole-file + // regex — a `sandbox_mode = "..."`-shaped line inside the + // `developer_instructions` block (prose) must never be read as a live + // value. A prior naive `raw.match(/^sandbox_mode.../m)` here produced a + // FALSE violation whenever an agent's own prompt prose happened to + // contain that shape. + const found = scanTomlLines(stripBOM(raw)).sandboxMode; + if (found === null) { + // No sandbox_mode line at all (e.g. sandboxTier "none") is a deliberate, + // documented state elsewhere — not this check's business. + continue; + } + + const canonicalFile = path.join(canonicalAgentsDir, `${agentName}.md`); + let canonicalContent: string; + try { + canonicalContent = fs.readFileSync(canonicalFile, 'utf8'); + } catch { + // No canonical role source (a custom/non-roster agent) — nothing to + // derive an expectation from, so this is not this check's business. + continue; + } + + // `canonicalContent` is always a real string here (read via fs.readFileSync + // above), so extractToolsValue's `undefined` (non-string input) branch is + // unreachable on this path — the `?? ''` is a type-level formality, not a + // behavior change (F4, #3897 security review: extractToolsValue is TOTAL). + const toolsRaw = extractToolsValue(canonicalContent) ?? ''; + const expected = deriveCodexSandboxMode(agentName, toolsRaw); + if (expected !== found) { + // #3897 rung 4 (isolated correctness review, MINOR finding 3): + // truncatePostureValue matches the sibling checkCodexModelPosture's + // convention (see its own `value: truncatePostureValue(model)` above) + // so an oversized or secret-shaped `sandbox_mode` value can never + // reach `validate agents --raw` output at full length. + violations.push({ + agent: agentName, + file: filePath, + expected: truncatePostureValue(expected), + found: truncatePostureValue(found), + }); + } + } + + return { + ok: violations.length === 0, + violations, + checked, + agents_dir: agentsDir, + agent_runtime: resolvedRuntime, + }; +} + /** * Probe a single agents dir for `` across runtime filename variants. * Mirrors {@link checkAgentsInstalled}'s probe (`.md`, `.agent.md`, `.toml`, @@ -410,6 +604,7 @@ export = { getAgentsDir, checkAgentsInstalled, checkCodexModelPosture, + checkCodexSandboxPosture, POSTURE_REASON, resolveAgentHint, }; diff --git a/src/codex-agent-toml.cts b/src/codex-agent-toml.cts index c6cbd7e00..0b3f3be5c 100644 --- a/src/codex-agent-toml.cts +++ b/src/codex-agent-toml.cts @@ -13,6 +13,17 @@ * it imports `stripBOM`/`scanTomlLines` from here and its regression suite * (`tests/agent-install-check.test.cjs`) is the proof. * + * #3897 rung 3 amendment: `deriveCodexSandboxMode`/`CODEX_SANDBOX_HOLDS`/ + * `validateCodexSandboxHolds` also live here now (moved from `bin/install.js`). + * That IS a policy (which `sandbox_mode` a role's tool contract derives), a + * narrow exception to this module's "document model, not policy" charter above + * — made because `bin/install.js` cannot be the shared owner: requiring it for + * its side effect on `require()` (the CLI banner print) corrupts every + * stdout-JSON caller (`agent-install-check.cts`'s `checkCodexSandboxPosture`). + * This module was already the single fs/path-free-parsing home both callers + * shared; `fs`/`path` are imported below ONLY for `validateCodexSandboxHolds`'s + * roster check — still node builtins only, no third-party or bin/lib dependency. + * * ── The reconciliation (40-design.md) ────────────────────────────────────── * * Phase 2's reader and this phase's writer disagree on how to handle an @@ -31,6 +42,9 @@ * that could silently drift from each other. */ +import fs from 'node:fs'; +import path from 'node:path'; + /** Frozen reason enum for a failed {@link parseCodexAgentToml}. */ export const PARSE_REASON = Object.freeze({ UNTERMINATED_BLOCK: 'unterminated_block', @@ -156,6 +170,15 @@ export function findDeveloperInstructionsBlockRange( export interface HeaderScanResult { model: string | null; hasReasoningEffort: boolean; + /** + * #3897 rung 4 (isolated correctness review, MINOR finding 2): the + * installed `sandbox_mode` value, block-aware exactly like `model` above — + * a `sandbox_mode = "..."`-shaped line inside the `developer_instructions` + * block (prose discussing sandbox modes) must never be read as a live + * value, same reasoning as the `model` probe this scan already protects. + * `null` when no live `sandbox_mode` key is present. + */ + sandboxMode: string | null; } interface HeaderLineInfo { @@ -163,6 +186,7 @@ interface HeaderLineInfo { modelLineIndex: number | null; reasoningEffort: string | null; reasoningEffortLineIndex: number | null; + sandboxMode: string | null; } // Line-oriented scan of every line OUTSIDE the `developer_instructions` block @@ -175,11 +199,15 @@ interface HeaderLineInfo { // lenient reader, boolean-only for reasoning effort) and parseCodexAgentToml // (the strict writer, which also needs the effort's value and both keys' line // indices so stripModel/stripReasoningEffort can remove exactly one line). +// `sandbox_mode` (#3897 rung 4 MINOR finding 2) rides the same block-aware +// pass as `model`/`model_reasoning_effort` — one scanner, never a second, +// naive whole-file regex that could match prose inside the block. function scanHeaderLines(lines: string[], blockStart: number, blockEnd: number): HeaderLineInfo { let model: string | null = null; let modelLineIndex: number | null = null; let reasoningEffort: string | null = null; let reasoningEffortLineIndex: number | null = null; + let sandboxMode: string | null = null; for (let i = 0; i < lines.length; i++) { if (blockStart !== -1 && i >= blockStart && i <= blockEnd) continue; const trimmed = lines[i].trim(); @@ -194,9 +222,11 @@ function scanHeaderLines(lines: string[], blockStart: number, blockEnd: number): } else if (key === 'model_reasoning_effort') { reasoningEffort = unquoteTomlValue(rawValue); reasoningEffortLineIndex = i; + } else if (key === 'sandbox_mode') { + sandboxMode = unquoteTomlValue(rawValue); } } - return { model, modelLineIndex, reasoningEffort, reasoningEffortLineIndex }; + return { model, modelLineIndex, reasoningEffort, reasoningEffortLineIndex, sandboxMode }; } /** @@ -208,8 +238,8 @@ function scanHeaderLines(lines: string[], blockStart: number, blockEnd: number): export function scanTomlLines(content: string): HeaderScanResult { const lines = content.split(/\r?\n/); const { start, end } = findDeveloperInstructionsBlockRange(lines); - const { model, reasoningEffort } = scanHeaderLines(lines, start, end); - return { model, hasReasoningEffort: reasoningEffort !== null }; + const { model, reasoningEffort, sandboxMode } = scanHeaderLines(lines, start, end); + return { model, hasReasoningEffort: reasoningEffort !== null, sandboxMode }; } // Splits `content` into `{lines, terminators}` where `terminators[i]` is the @@ -389,3 +419,392 @@ export function stripReasoningEffort(doc: CodexAgentDoc): CodexAgentDoc { if (doc.reasoningEffortLineIndex === null) return doc; return removeLine(doc, doc.reasoningEffortLineIndex, 'reasoningEffort'); } + +// ── Codex sandbox_mode derivation (#3897 rung 3, ADR-3473 §8.3) ──────────── +// +// Moved here from `bin/install.js` (fix for the CAUSE A regression this rung +// introduced): `agent-install-check.cts`'s `checkCodexSandboxPosture` used to +// lazily `require(bin/install.js)` to reach this derivation — but requiring +// `bin/install.js` runs its whole top-level script, including the ASCII +// banner print to stdout, which corrupted every stdout-JSON caller downstream +// of `checkCodexSandboxPosture` (`gsd-tools validate agents`). This module is +// a genuine leaf with no top-level side effects, so both `bin/install.js` and +// `agent-install-check.cts` import the derivation from here instead — ONE +// owner, no second predicate (routing `src/` through `bin/install.js` was +// backwards layering to begin with). +// +// This module does NOT parse frontmatter (there is no third copy of that +// extraction here — two already exist, `bin/install.js` and +// `runtime-artifact-conversion.cts`). `deriveCodexSandboxMode` below takes +// the already-resolved `tools:` value as a plain string parameter: every +// caller already has it (or the raw frontmatter to pull it from) in hand +// before calling in, so this module stays a pure predicate over data +// supplied by the caller — never a document reader itself. This also means +// the module never needs the full YAML-backed `frontmatter.cts` engine +// (vendored js-yaml + anchor/alias refusal + comment-channel plumbing, built +// for a much broader contract than a single `tools:` line lookup) — it has +// no frontmatter-parsing need at all anymore. + +/** + * The 17 roles measured as widening under derivation (declare Write/Edit, + * never in the pre-#3897 `CODEX_AGENT_SANDBOX` map, so the old + * `|| 'read-only'` fallback silently under-granted them). Pinned to + * `read-only` pending the open question of whether Codex enforces + * `sandbox_mode` or treats it as advisory (HALT.md). This list is CLOSED and + * SHRINK-ONLY: a new writing role never lands here (S6, T26); it is validated + * against the live tool contract every time it is consulted + * ({@link _deriveCodexSandboxModeFromTools}) and against the real + * `agents/` roster by a dedicated test (`tests/codex-config.test.cjs` T24/T25) + * so a stale or orphaned entry fails loudly instead of being silently + * honored forever. + * + * `gsd-nyquist-auditor` is the 17th entry, added by the list-form `tools:` + * parse fix (#3897 follow-up): its `tools:` frontmatter uses YAML block-list + * form (`tools:` + indented `- Item` lines) and declares both Write and + * Edit, but the original `extractToolsLine`/`extractFrontmatterField` + * readers only ever saw the first list item (`- Read`) and derived + * `read-only` by accident, not by design. {@link extractToolsValue} now + * parses the list correctly, so this role genuinely derives + * `workspace-write` from its tool contract — HALT.md's original 16-role + * count measured against the pre-fix (single-line) readers and undercounted + * this role. It is held here for the same reason as the other 16: pending + * Codex's `sandbox_mode` enforcement decision, not because the derivation is + * wrong. + */ +export const CODEX_SANDBOX_HOLDS: Readonly> = Object.freeze({ + 'gsd-ai-researcher': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-code-fixer': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-code-reviewer': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-debug-session-manager': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-doc-classifier': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-doc-synthesizer': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-doc-verifier': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-doc-writer': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-dom-verifier': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-domain-researcher': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-eval-auditor': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-eval-planner': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-intel-updater': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-pattern-mapper': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-ui-auditor': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-ui-researcher': 'declares Write/Edit; pending Codex sandbox_mode enforcement decision', + 'gsd-nyquist-auditor': 'declares Write/Edit (YAML list-form tools:, surfaced by the list-form parse fix); pending Codex sandbox_mode enforcement decision', +}); + +// True iff a `tools:` frontmatter value declares Write or Edit as a whole +// token (never a substring match, so a hypothetical "Edith"-named tool could +// never collide). Single predicate owner for both the emitter +// ({@link _deriveCodexSandboxModeFromTools}) and the posture check +// (`agent-install-check.cts`'s `checkCodexSandboxPosture`, via +// {@link deriveCodexSandboxMode}). +// +// #3897 security review F5: a negation form — `All tools except Agent, +// Write, Edit` — used to derive `workspace-write` because the plain +// comma-tokenizer below saw the literal token `Write` and never noticed it +// was named as an EXCLUSION, not a grant. That is fail-OPEN: the value's +// stated meaning is "everything except these", so a `Write`/`Edit` name +// after `except` means the role does NOT get them. No shipped roster agent +// uses this form today, but a wrong answer here widens silently, so it is +// handled: once the literal word `except` (case-insensitive) appears +// anywhere in the value, this is a " except " statement +// and only the tokens AFTER `except` decide the verdict — `Write`/`Edit` +// declare broader iff `except` is ABSENT, or present but does not name them. +function _codexToolsDeclareWriteOrEdit(toolsRaw: string): boolean { + const raw = String(toolsRaw || ''); + const exceptIndex = raw.search(/\bexcept\b/i); + if (exceptIndex !== -1) { + const exclusionsText = raw.slice(exceptIndex).replace(/^\S*\s*except\b/i, ''); + const exclusions = exclusionsText.split(',').map((t) => t.trim()).filter(Boolean); + const excludesWriteOrEdit = exclusions.includes('Write') || exclusions.includes('Edit'); + return !excludesWriteOrEdit; + } + const tokens = raw.split(',').map((t) => t.trim()); + return tokens.includes('Write') || tokens.includes('Edit'); +} + +// #3897 security review F1/F3: control-character/whitespace class used by +// {@link normalizeSandboxIdentity} to trim more than plain ASCII whitespace +// from BOTH edges of a candidate identity before it is compared against the +// hold set. A bare .trim() only strips the ASCII whitespace \s already +// covers; the hold bypass this closes (a trailing space, \n/\r, or NBSP +// after `gsd-doc-writer`) is exactly a value that survives .trim() untouched. +// Spelled entirely as \u escapes (never a literal invisible codepoint in +// the source), matching this module's own BOM_CHAR convention above: NBSP +// (U+00A0), the zero-width space/non-joiner/joiner + LTR/RTL marks +// (U+200B-U+200F), the BOM/ZWNBSP (U+FEFF), and the full C0 control range +// + DEL (U+0000-U+001F, U+007F), alongside ASCII whitespace (\s). +const SANDBOX_IDENTITY_EDGE_RE = new RegExp( + '^[\\s\\u0000-\\u001f\\u007f\\u00a0\\u200b-\\u200f\\ufeff]+' + + '|[\\s\\u0000-\\u001f\\u007f\\u00a0\\u200b-\\u200f\\ufeff]+$', + 'g', +); + +// Anything outside this set, AFTER normalization, is "suspicious" — see +// {@link isSandboxHeld}'s docblock for why fail-closed (never enumerate +// confusables) is the only tractable answer here. +const SANDBOX_IDENTITY_SUSPICIOUS_RE = /[^a-z0-9._-]/; + +/** + * Canonicalizes a single candidate sandbox-hold identity (a source filename + * stem OR a frontmatter-derived `name:` value) into the same lowercase ASCII + * shape {@link CODEX_SANDBOX_HOLDS}'s keys are written in, so both candidate + * identities for one emitted artifact (see {@link isSandboxHeld}) can be + * compared against the hold set on equal footing. + * + * #3897 security review F1/F3 — pipeline, in this exact order: + * 1. basename it (strip through the LAST `/` or `\`) — a value carrying a + * path separator (`../agents/x`, `./x`) is reduced to its final segment + * before anything else runs, so a path-traversal-shaped candidate can + * never dodge the hold lookup by hiding behind a directory prefix. + * 2. trim ASCII whitespace AND the NBSP/zero-width/control-char class + * ({@link SANDBOX_IDENTITY_EDGE_RE}) from both ends. + * 3. strip trailing dots (`gsd-doc-writer.` collapses to the same key). + * 4. `String.prototype.normalize('NFKC')` — canonicalizes compatibility + * variants (e.g. fullwidth `ｇ` U+FF47 folds to ASCII `g`) to their + * standard form so a widened-by-Unicode-lookalike name collapses onto + * the real held key instead of merely looking similar to it. + * 5. lowercase — ASCII recasing already matched the hold set before this + * rung; this keeps that property after the above steps. + * + * Returns `null` (never throws) for a non-string input or when the fully + * normalized result is empty, so callers can treat `null` as "not a real + * identity, cannot be held, but see the `suspicious` companion signal". + */ +export function normalizeSandboxIdentity(raw: unknown): string | null { + if (typeof raw !== 'string') return null; + const lastSlash = Math.max(raw.lastIndexOf('/'), raw.lastIndexOf('\\')); + let value = lastSlash === -1 ? raw : raw.slice(lastSlash + 1); + value = value.replace(SANDBOX_IDENTITY_EDGE_RE, ''); + value = value.replace(/\.+$/, ''); + value = value.normalize('NFKC'); + value = value.toLowerCase(); + return value === '' ? null : value; +} + +/** + * `held` is true iff ANY candidate identity, once normalized, names a real + * {@link CODEX_SANDBOX_HOLDS} entry. `suspicious` is true iff any candidate, + * after normalization, still contains a character outside `[a-z0-9._-]`. + * + * #3897 security review F1 (the blocker): the sandbox decision must be safe + * for the ARTIFACT IT LANDS ON, not for whichever single identity happens to + * be handy at the call site. `bin/install.js`'s emit loop has TWO candidate + * identities for one `.toml` artifact — the source filename stem (what the + * hold lookup used to be keyed on) and the frontmatter `name:` value (what + * the OUTPUT PATH is actually keyed on) — and an attacker who controls + * frontmatter can make them disagree (rename the file, or plant a sibling + * file whose `name:` collides with a held role). Deciding over only one of + * the two and applying the result to whichever path the OTHER one names is + * exactly the regression this closes: this predicate takes every candidate + * that can influence either the derivation or the emitted path and takes the + * MOST RESTRICTIVE answer — `held` if any one of them is held. + * + * #3897 security review F3: `suspicious` is the fail-CLOSED half of this + * predicate, and it is deliberately not an attempt to enumerate Unicode + * confusables (Turkish dotted/dotless I, fullwidth forms, NFD combining + * marks, ...). Every one of the 35 shipped roster files is pure ASCII + * (`gsd-[a-z-]+`), so a non-ASCII-after-normalization identity cannot be a + * legitimate shipped role — flagging it `suspicious` (which + * {@link deriveCodexSandboxMode} turns into `read-only`) has zero false + * positives against real content, and refuses to widen on anything this + * module does not recognize instead of chasing an open-ended confusables + * list that will always be one codepoint behind the next lookalike. + */ +export function isSandboxHeld( + ...candidates: Array +): { held: boolean; suspicious: boolean } { + let held = false; + let suspicious = false; + for (const candidate of candidates) { + const values = Array.isArray(candidate) ? candidate : [candidate]; + for (const value of values) { + const normalized = normalizeSandboxIdentity(value); + if (normalized === null) continue; + if (Object.prototype.hasOwnProperty.call(CODEX_SANDBOX_HOLDS, normalized)) { + held = true; + } + if (SANDBOX_IDENTITY_SUSPICIOUS_RE.test(normalized)) { + suspicious = true; + } + } + } + return { held, suspicious }; +} + +// #3897 CAUSE A fix: a single-purpose `tools:`-VALUE reader, NOT a general +// frontmatter parser (`extractFrontmatterAndBody`/`extractFrontmatterField` +// were deliberately deleted from this module's ancestor — see the module +// header's "document model, not policy" charter). This module is the one +// genuinely side-effect-free leaf `agent-install-check.cts`'s +// `checkCodexSandboxPosture` already imports from for +// {@link deriveCodexSandboxMode}; giving it this reader too avoids importing +// `runtime-artifact-conversion.cjs` there just to pull one frontmatter field +// — that module's own dependency chain (`command-roster.cjs` → +// `scripts/fix-slash-commands.cjs`, a dev-only repo script) does not resolve +// from an installed tree, which is exactly what broke +// `tests/agent-install-check.test.cjs` against a synthetic install dir. +// Scoped to ONLY the `tools:` key: finds the leading `---`-delimited +// frontmatter block and returns its `tools:` value (quotes stripped), or +// `''` when there is no frontmatter block or no `tools:` key at all. +// +// #3897 rung 4/list-form fix: handles BOTH shapes a `tools:` key can take — +// (a) inline: `tools: Read, Write, Edit` on the same line as the key, and +// (b) YAML block-list form: `tools:` alone, followed by indented `- Item` +// lines (`agents/gsd-nyquist-auditor.md`, `agents/gsd-security-auditor.md` +// are the only two roster files using this shape today). The former +// implementation was a single `/^tools:\s*(.+)$/m` regex — `\s*` swallows the +// newline after a bare `tools:` key, so it silently matched into the FIRST +// list item's own line and returned just `"- Read"`, reading a real +// Write/Edit DECLARATION as an absence. List items are joined with `, ` so +// the result feeds {@link _codexToolsDeclareWriteOrEdit}'s comma-tokenizer +// unchanged. The list terminates at the first line that is not an indented +// `- Item` (a new frontmatter key, `---`, a blank line, or EOF) — it never +// grows into a general YAML parser (that boundary is deliberate, see above). +// +// #3897 security review F4: TOTAL for any input, not just a well-formed +// string — a Buffer, `undefined`, or `null` returns `undefined` rather than +// throwing on `.startsWith`, matching this module's "never throws" charter +// (see {@link deriveCodexSandboxMode}'s own totality note). +export function extractToolsValue(agentContent: unknown): string | undefined { + if (typeof agentContent !== 'string') return undefined; + if (!agentContent.startsWith('---')) return ''; + const endIndex = agentContent.indexOf('---', 3); + if (endIndex === -1) return ''; + const frontmatter = agentContent.substring(3, endIndex); + const lines = frontmatter.split(/\r?\n/); + const toolsLineIndex = lines.findIndex((line) => /^tools:/.test(line)); + if (toolsLineIndex === -1) return ''; + const inlineMatch = lines[toolsLineIndex].match(/^tools:[ \t]*(\S.*)$/); + if (inlineMatch) { + return inlineMatch[1].trim().replace(/^['"]|['"]$/g, ''); + } + // Bare `tools:` key (nothing but optional trailing whitespace on its own + // line) — read the YAML block-list form that follows: indented `- Item` + // lines, one item per line, stopping at the first line that is not one. + const items: string[] = []; + for (let i = toolsLineIndex + 1; i < lines.length; i++) { + const itemMatch = lines[i].match(/^[ \t]+-[ \t]*(.+)$/); + if (!itemMatch) break; + items.push(itemMatch[1].trim().replace(/^['"]|['"]$/g, '')); + } + return items.join(', '); +} + +/** + * The single owner of `sandbox_mode` derivation: `workspace-write` iff the + * role's own frontmatter `tools:` declares Write/Edit, UNLESS the role is + * held ({@link CODEX_SANDBOX_HOLDS}) at `read-only`. + * + * #3897 CAUSE C fix: this function is TOTAL — it never throws, for any + * (identity, toolsValue) pair. It used to throw when a held role's CONTENT + * (whatever the caller handed it) did not derive broader than its pin, on + * the theory that a stale hold should "fail loudly at the point the + * derivation runs". That reasoning does not survive contact with this + * function's actual contract: it is a pure predicate over whatever content + * the caller supplies, and tests legitimately pass synthetic fixtures for + * held role names, so the throw fired on arbitrary input, not just the real + * roster. It is also redundant even for real input — if a held role's + * content does not derive broader, applying the hold pins `read-only` and + * derivation would return `read-only` anyway; the hold is a no-op, so there + * is nothing to fail about at derivation time. The STALENESS invariant (S4) + * is a property of the real `agents/` roster, not of an arbitrary call's + * input, so it belongs — and stays enforced — only at the roster level: see + * {@link validateCodexSandboxHolds} and `tests/codex-config.test.cjs` + * T24/T25, which assert it against the real roster directly. + * + * `agentName` here MUST be every identity the content's own frontmatter + * could disagree with — i.e. the source `.md` FILENAME stem AND (when the + * caller has one) the frontmatter-derived `name:` display value, passed + * together as an array. Accepting only one of the two and applying the + * result to an ARTIFACT keyed on the other is the #3897 security review F1 + * regression this function closes: `bin/install.js`'s emit loop decides + * `sandbox_mode` for the filename stem but writes the `.toml` at a path keyed + * on `name`, so a renamed file (stem drifts, `name:` stays pinned) or a + * sibling file whose `name:` collides with a held role could make the two + * identities disagree and land a held role's own artifact with + * `workspace-write`. See {@link isSandboxHeld} for the most-restrictive-wins + * rule across every candidate identity. + * + * `toolsRaw` is the already-resolved `tools:` frontmatter VALUE (e.g. + * `"Read, Write, Edit"`), not the frontmatter block or the full agent + * content — see {@link deriveCodexSandboxMode}'s doc for why. + */ +function _deriveCodexSandboxModeFromTools(agentName: string | string[] | null | undefined, toolsRaw: string): string { + const derivesBroader = _codexToolsDeclareWriteOrEdit(toolsRaw || ''); + const { held, suspicious } = isSandboxHeld(agentName); + if (held || suspicious) { + // A held (or unrecognizable/"suspicious", per F3) identity always pins + // read-only, whether or not its supplied content still derives broader + // (see #3897 CAUSE C note above the docstring for this function's + // caller-facing counterpart): if it no longer derives broader, the pin is + // a no-op and there is nothing to fail about here — the staleness + // invariant lives at the roster level instead + // ({@link validateCodexSandboxHolds}). + return 'read-only'; + } + return derivesBroader ? 'workspace-write' : 'read-only'; +} + +/** + * Exported form for external callers (`checkCodexSandboxPosture`, + * `generateCodexAgentToml`). Takes the already-resolved `tools:` frontmatter + * VALUE, not raw agent content or a frontmatter slice — this module does no + * frontmatter parsing at all (there is no third copy of that extraction + * here; see the module-header note above `CODEX_SANDBOX_HOLDS`). Both + * callers already have (or can trivially get) `tools:` in hand via this + * module's own {@link extractToolsValue} — the single shared extractor both + * `bin/install.js`'s `generateCodexAgentToml` and `agent-install-check.cts`'s + * `checkCodexSandboxPosture` route through (#3897 CAUSE A fix, and the + * #3897 list-form parse fix's Fix 3: `bin/install.js` used to read `tools:` + * via its own private `extractFrontmatterField`, a second single-line-only + * copy that disagreed with this module's reader on YAML block-list form — + * NOT `runtime-artifact-conversion.cts`'s general-purpose extractors either; + * that module's dependency chain does not resolve from an installed tree) — + * so this stays a pure predicate over data the caller already holds, never a + * document reader. + * + * `agentName` is one or more HOLD-LOOKUP IDENTITIES: pass the agent's + * canonical source filename stem (e.g. `gsd-doc-writer` for + * `agents/gsd-doc-writer.md`) as a plain string for the single-identity call + * sites this signature always supported, OR an array of every identity that + * can influence the emitted artifact (filename stem AND frontmatter `name:`) + * when the caller — like `bin/install.js`'s per-agent emit loop — has both + * (see the F1 security note on {@link _deriveCodexSandboxModeFromTools} + * above). TOTAL for any value here, including `undefined`, `null`, `[]`, and + * an array containing non-string members — none of those throw; they simply + * contribute nothing to the hold/suspicious check (see {@link isSandboxHeld} + * / {@link normalizeSandboxIdentity}). + */ +export function deriveCodexSandboxMode(agentName: string | string[] | null | undefined, toolsRaw: string): string { + return _deriveCodexSandboxModeFromTools(agentName, toolsRaw || ''); +} + +/** + * (S5) Every {@link CODEX_SANDBOX_HOLDS} key must still name a real + * `/.md`. A hold for a role that no longer exists is + * stale and must fail loudly, not be silently ignored — else the hold list + * only ever grows/rots instead of shrinking to zero. + * + * NOT called from the install runtime path ({@link deriveCodexSandboxMode} / + * `bin/install.js`'s `installCodexConfig`): the shrink-to-zero invariant it + * checks is a REPO invariant about the canonical roster in `agents/`, not a + * property of whatever directory an install happens to read from — a + * partial/synthetic install source (a test fixture, a `--config-dir` + * subset) legitimately contains only a few agents, and a hold whose role is + * simply absent from THAT source dir must be inert, not fatal. The invariant + * is enforced instead as a test over the real `agents/` roster + * (`tests/codex-config.test.cjs` T24/T25). This function is kept, exported, + * for any caller that specifically wants to validate the CANONICAL roster + * (pass `agents/` itself, never an arbitrary install source). + */ +export function validateCodexSandboxHolds(agentsSrcDir: string): void { + for (const role of Object.keys(CODEX_SANDBOX_HOLDS)) { + const agentFile = path.join(agentsSrcDir, `${role}.md`); + if (!fs.existsSync(agentFile)) { + throw new Error( + `CODEX_SANDBOX_HOLDS: stale hold for "${role}" — no ${agentFile} exists. The hold list is ` + + 'closed and shrink-only (ADR-3473 §8.3 / HALT.md); remove this entry.' + ); + } + } +} diff --git a/src/model-resolver.cts b/src/model-resolver.cts index fd844fc91..680bde926 100644 --- a/src/model-resolver.cts +++ b/src/model-resolver.cts @@ -36,6 +36,11 @@ import { MODEL_ALIAS_MAP, RUNTIME_PROFILE_MAP, PROVIDER_PRESETS, VALID_TIERS, CL import fs from 'node:fs'; import path from 'node:path'; import { resolveRuntimeNameFromCandidates } from './runtime-name-policy.cjs'; +import { + readInstallRuntimeMarker, + _setInstallRuntimeMarkerForTests, + _resetInstallRuntimeMarkerCacheForTests, +} from './runtime-slash.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningWorkspaceMod = require('./planning-workspace.cjs'); const { planningDir } = planningWorkspaceMod; @@ -61,30 +66,12 @@ const { planningDir } = planningWorkspaceMod; // loudly here instead of silently omitting. const RUNTIMES_WITH_NATIVE_ALIASES: ReadonlySet = new Set(['claude']); -let _installMarkerCache: string | null | undefined; -function readInstallRuntimeMarker(): string | null { - if (_installMarkerCache !== undefined) return _installMarkerCache; - try { - const markerPath = path.join(__dirname, '..', '..', '.gsd-runtime'); - const raw = fs.readFileSync(markerPath, 'utf8').trim(); - _installMarkerCache = raw || null; - } catch { - // No marker: dev/source tree, or an install predating #2297. Fall through to - // the 'claude' default (keeps tier aliases — never worse than the bug). - _installMarkerCache = null; - } - return _installMarkerCache; -} - -// Test seams for the install-marker rung (the dev/source tree has no marker, so -// the file read always bottoms out at 'claude' — these let tests exercise the -// third precedence rung and reset the module-level cache between cases). -function _setInstallRuntimeMarkerForTests(value: string | null): void { - _installMarkerCache = value; -} -function _resetInstallRuntimeMarkerCacheForTests(): void { - _installMarkerCache = undefined; -} +// #3897 rung 2: the marker reader + its cache and test seams were promoted to +// the canonical owner, `runtime-slash.cts` (imported above) — this module now +// consumes that single implementation instead of holding its own copy. N5: +// behaviour and the seam contract are unchanged by the move; the re-exports +// below (`export =` at the bottom of this file) preserve every existing +// caller's `require('./model-resolver.cjs')` surface byte-for-behaviour. // The runtime whose install is actually resolving, canonicalized so an alias or // case variant (e.g. "claude-code"/"Claude") cannot defeat the native-alias diff --git a/src/phase.cts b/src/phase.cts index 7226a8a8f..e356d6b7a 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -607,21 +607,75 @@ interface RawPlan { /** * Resolve a raw `depends_on` token to the `RawPlan.id` it refers to - * (case-folded exact match, falling back to canonical-id matching). Returns + * (case-folded exact match, falling back to canonical-id matching, falling + * back to the in-phase short-form plan number — #3897 rung 4). Returns * `null` when the token does not resolve to any plan in this phase (a typo * or a cross-phase reference) — every call site treats that as "ignore this * edge", never a throw. Shared by `computeDependencyLevels`'s DAG-edge - * resolution, the `depends_on` display mapping, and (#2830) the - * halt-propagation node resolution, so the three can never disagree about - * which token resolves to which plan. + * resolution and (#2830) the halt-propagation node resolution, so the two can + * never disagree about which token resolves to which plan. NOT used by the + * `depends_on` display mapping (#3785/N3) — that stays a passthrough by + * design; see the comment at its call site. + * + * `shortFormToId` (#3897 rung 4, ADR-3473 §8.9) is the third tier, consulted + * only when neither `planMap` nor `canonicalToId` resolves the token. It is + * optional so any caller that has not been threaded through yet (there are + * none left in this file) degrades to the pre-#3897 two-tier behavior rather + * than throwing on a missing argument. */ function resolveDependencyId( dep: string, planMap: Map, canonicalToId: Map, + shortFormToId?: Map, ): string | null { const lower = dep.toLowerCase(); - return planMap.has(lower) ? (planMap.get(lower) as RawPlan).id : (canonicalToId.get(lower) ?? null); + if (planMap.has(lower)) return (planMap.get(lower) as RawPlan).id; + if (canonicalToId.has(lower)) return canonicalToId.get(lower) as string; + return shortFormToId?.get(lower) ?? null; +} + +// #3897 rung 4 (ADR-3473 §8.9) — builds the third depends_on resolution tier: +// a map from an in-phase BARE PLAN NUMBER (e.g. "01") to the plan id whose +// canonical id ends with that number. Recovered from the retired SDK lineage +// (sdk/src/query/phase.ts at 11918dcc3^) with ONE deliberate narrowing: the +// lost implementation indexed ANY trailing dash-segment of a canonical id, +// with no constraint that the segment be a plan NUMBER — so a phase +// containing both `09-FIX-auth-PLAN.md` and `09-GAP-auth-PLAN.md` (canonical +// id `09-FIX-auth`, trailing segment "auth") would silently bind +// `depends_on: ["auth"]` to whichever sorted first, fabricating a +// wave-affecting DAG edge with ZERO warning — a mis-resolved edge, which is +// worse than a dropped one (found in isolated correctness review, #3897). +// `docs/reference/plan-md.md` already documents this tier as resolving "the +// bare plan number", so requiring `/^\d+$/` on the trailing segment is a +// strict narrowing onto the tier's OWN documented contract, not a behavior +// change for any legitimate input. Do NOT restore the unconstrained +// lastDash-slice "to match the recovered original" — the original was wrong +// here; this rung deliberately departs from it in this one respect, and only +// this one. Everything else — the `lastDash` bound, first-write-wins, +// lowercasing — is kept exactly as recovered: +// - first write wins, deterministic because rawPlans is passed in sorted +// plan-file order (D4/T44) and this loop iterates in that same order; +// - a canonical id with no dash (`lastDash === -1` or `lastDash === 0`, +// e.g. "24" or "-01") or a trailing dash (`lastDash === canonical.length +// - 1`, e.g. "09-") is never indexed (D5). +// Exported so callers can build this map once and so tests assert against +// this REAL implementation rather than a hand-rolled copy that could +// silently disagree with it after a future change here (CLAUDE.md's +// generative-fix-divergence rule). +function buildShortFormToId(rawPlans: RawPlan[]): Map { + const shortFormToId = new Map(); + for (const p of rawPlans) { + const canonical = extractCanonicalPlanId(p.id); + const lastDash = canonical.lastIndexOf('-'); + if (lastDash > 0 && lastDash < canonical.length - 1) { + const shortForm = canonical.slice(lastDash + 1).toLowerCase(); + if (/^\d+$/.test(shortForm) && !shortFormToId.has(shortForm)) { + shortFormToId.set(shortForm, p.id); + } + } + } + return shortFormToId; } // O(V + E). Assigns each in-phase plan its longest-path topological level over the @@ -630,26 +684,32 @@ function resolveDependencyId( // the exact dequeue order this pass already produces — a valid topological order — passed to // computeHaltPropagation as `precomputedOrder` so halt propagation does not re-run Kahn's // algorithm a second time over the same graph. +// +// `shortFormToId` (#3897 rung 4, optional — see resolveDependencyId) is threaded through so a +// bare in-phase plan-number token (`depends_on: ["01"]`) resolves as a real DAG edge instead of +// being dropped and silently collapsing the dependent plan to wave 1 (D3). function computeDependencyLevels( rawPlans: RawPlan[], planMap: Map, canonicalToId: Map, + shortFormToId?: Map, ): { level: Map; visited: number; order: string[]; unresolved: Array<{ plan: string; token: string }> } { const level = new Map(); const inDeg = new Map(); const adj = new Map(); - // #3427 / ADR-3473 §8.5: a depends_on token that resolves via neither - // planMap nor canonicalToId is a dropped edge. Naming it here (rather than - // silently `continue`-ing past it) lets cmdPhasePlanIndex surface the - // token's own warning instead of manufacturing a wave-mismatch verdict from - // the resulting damaged graph (#3427). + // #3427 / ADR-3473 §8.5: a depends_on token that resolves via NONE of the + // three tiers (planMap, canonicalToId, shortFormToId) is a dropped edge. + // Naming it here (rather than silently `continue`-ing past it) lets + // cmdPhasePlanIndex surface the token's own warning instead of + // manufacturing a wave-mismatch verdict from the resulting damaged graph + // (#3427). const unresolved: Array<{ plan: string; token: string }> = []; for (const p of rawPlans) { if (!inDeg.has(p.id)) inDeg.set(p.id, 0); if (!adj.has(p.id)) adj.set(p.id, []); for (const dep of p.dependsOn) { - const resolvedDep = resolveDependencyId(dep, planMap, canonicalToId); + const resolvedDep = resolveDependencyId(dep, planMap, canonicalToId, shortFormToId); if (!resolvedDep) { unresolved.push({ plan: p.id, token: String(dep) }); continue; @@ -866,8 +926,16 @@ function cmdPhasePlanIndex(cwd: string, phase: string, raw: boolean): void { const canonicalToId = new Map( rawPlans.map((p) => [extractCanonicalPlanId(p.id).toLowerCase(), p.id]), ); + // #3897 rung 4 (ADR-3473 §8.9) — the third depends_on resolution tier. + // Resolves a bare in-phase plan-number short form (e.g. "01") to its owning + // plan id. In-phase only by construction (T49): the map is built from THIS + // phase's rawPlans alone, so a short form colliding with a different + // phase's plan can never be a candidate. See {@link buildShortFormToId}'s + // own comment for the numeric-only narrowing this rung applies on top of + // the recovered SDK-lineage algorithm. + const shortFormToId = buildShortFormToId(rawPlans); - const { level, visited, order, unresolved } = computeDependencyLevels(rawPlans, planMap, canonicalToId); + const { level, visited, order, unresolved } = computeDependencyLevels(rawPlans, planMap, canonicalToId, shortFormToId); if (visited < rawPlans.length) { const cycleNodes = rawPlans.filter((p) => !level.has(p.id)).map((p) => p.id); @@ -885,7 +953,7 @@ function cmdPhasePlanIndex(cwd: string, phase: string, raw: boolean): void { const haltNodes = rawPlans.map((p) => ({ id: p.id, resolvedDependsOn: p.dependsOn - .map((dep) => resolveDependencyId(String(dep), planMap, canonicalToId)) + .map((dep) => resolveDependencyId(String(dep), planMap, canonicalToId, shortFormToId)) .filter((id): id is string => id !== null), halted: p.halted, })); @@ -3556,4 +3624,5 @@ export = { cmdPhaseUatPassed, cmdPhaseListPlans, computeDependencyLevels, + buildShortFormToId, }; diff --git a/src/runtime-slash.cts b/src/runtime-slash.cts index cd8cf656c..748afeb78 100644 --- a/src/runtime-slash.cts +++ b/src/runtime-slash.cts @@ -73,6 +73,59 @@ export function formatGsdSlash(commandName: unknown, runtime: unknown): unknown return `/gsd-${token}${tail}`; } +// ─── #3897 rung 2: the per-install `.gsd-runtime` marker, promoted from ────── +// model-resolver.cts (#2297) to this module — the canonical owner of runtime +// identity (resolveRuntime, formatGsdSlash). `bin/install.js` writes +// `/gsd-core/.gsd-runtime` beside VERSION for every runtime install +// (#2297). This module compiles to `gsd-core/bin/lib/runtime-slash.cjs` +// (ADR-457) — the SAME directory `model-resolver.cjs` compiles to — so the +// `__dirname`-relative path below is unchanged from the promoted original. +// +// Cache + test-seam shape, byte-for-behaviour preserved from +// model-resolver.cts:65-77 (N5): module-level `let`, `try/catch -> null`, +// `_setInstallRuntimeMarkerForTests` / `_resetInstallRuntimeMarkerCacheForTests`. +// `model-resolver.cts` now imports this implementation instead of holding its +// own copy; the two `hooks/*.js` marker readers delegate here too (through +// `ensureRuntimeBuild()`, per `scripts/lint-hooks-runtime-build-seam.cjs`). +let _installMarkerCache: string | null | undefined; + +/** + * Read the per-install runtime marker, cached for the process lifetime. + * Never throws (N4) — an absent/unreadable marker (dev/source tree, an + * install predating #2297, EACCES, hostile content, ...) is "no signal", + * never a resolution failure; a throw here would break a session in + * `gsd-agent-isolation-guard.js`, which is worse than resolving the wrong + * runtime. + * + * The raw marker string is returned as-is (trimmed) — callers MUST route it + * through `resolveRuntimeNameFromCandidates` before trusting it (N1); this + * function itself does not normalize so its cached value matches exactly + * what #2297's tests already assert about the underlying file read. + */ +export function readInstallRuntimeMarker(): string | null { + if (_installMarkerCache !== undefined) return _installMarkerCache; + try { + const markerPath = path.join(__dirname, '..', '..', '.gsd-runtime'); + const raw = fs.readFileSync(markerPath, 'utf8').trim(); + _installMarkerCache = raw || null; + } catch { + // No marker: dev/source tree, or an install predating #2297. Fall through + // to the 'claude' default (keeps tier aliases — never worse than the bug). + _installMarkerCache = null; + } + return _installMarkerCache; +} + +// Test seams for the install-marker rung (the dev/source tree has no marker, +// so the file read always bottoms out at 'claude' — these let tests exercise +// the third precedence rung and reset the module-level cache between cases). +export function _setInstallRuntimeMarkerForTests(value: string | null): void { + _installMarkerCache = value; +} +export function _resetInstallRuntimeMarkerCacheForTests(): void { + _installMarkerCache = undefined; +} + /** * Resolve the explicit runtime for a project directory, from the two * explicit sources only — no default is applied. @@ -120,17 +173,30 @@ export function resolveExplicitRuntime( /** * Resolve the effective runtime for a project directory. * - * process.env.GSD_RUNTIME > config.runtime > 'claude' + * process.env.GSD_RUNTIME > config.runtime > install marker > 'claude' * * Mirrors the precedence already used by profile-output.cjs and the rest of * the runtime resolution chain. Returns a lowercased string so downstream * comparisons can be case-blind. * + * #3897 rung 2: the per-install `.gsd-runtime` marker is the THIRD rung, + * between the two explicit sources and the 'claude' default — matching + * model-resolver.cts's own description of it as "the third precedence rung" + * before this reader was promoted here. The marker's raw contents are never + * trusted verbatim (N1): they are routed through the SAME + * `resolveRuntimeNameFromCandidates` normalization the env rung uses, so an + * unknown or hostile marker value degrades exactly like an unknown + * GSD_RUNTIME value would. + * * @param projectDir - path to the project directory, or null/undefined * @returns the resolved runtime name */ export function resolveRuntime(projectDir: string | null | undefined): string { - return resolveExplicitRuntime(projectDir) ?? 'claude'; + const explicit = resolveExplicitRuntime(projectDir); + if (explicit) return explicit; + const markerRuntime = resolveRuntimeNameFromCandidates(readInstallRuntimeMarker()); + if (markerRuntime) return markerRuntime; + return 'claude'; } /** diff --git a/src/verify.cts b/src/verify.cts index fd074018c..f42f7a126 100644 --- a/src/verify.cts +++ b/src/verify.cts @@ -34,7 +34,7 @@ import { extractTaggedBlocks } from './markdown-sectionizer.cjs'; import { compileUserPattern, MAX_USER_PATTERN_LEN } from './pattern.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- agent-install-check.cjs is an export= CommonJS module import agentInstallCheck = require('./agent-install-check.cjs'); -const { checkAgentsInstalled, checkCodexModelPosture } = agentInstallCheck; +const { checkAgentsInstalled, checkCodexModelPosture, checkCodexSandboxPosture } = agentInstallCheck; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); const { output, error } = ioMod; @@ -1736,6 +1736,21 @@ function cmdValidateAgents(cwd: string, raw: boolean): void { // model or an orphaned reasoning-effort pin in a Codex agent .toml), not just // presence. checkAgentsInstalled above is untouched. const codexPosture = checkCodexModelPosture(runtime, cwd); + // #3897 rung 3 (ADR-3473 §8.3 criterion 3) — sibling posture check, additive + // alongside codex_posture (never nested inside it, never replacing it): a + // TOML's `sandbox_mode` disagreeing with its role's derived expectation is a + // different defect class than an Anthropic-flavored model. Same short-circuits, + // same read-only posture as checkCodexModelPosture above; checkAgentsInstalled + // and codex_posture are both left untouched. + // + // Failure semantics: matches the checkCodexModelPosture precedent exactly. + // Neither posture check makes `validate agents` exit non-zero on its own — + // both are report-only fields inspected by the caller (or a human) via the + // JSON payload. A `validate` verb whose two posture checks disagreed on + // fatality would be its own defect (#3897 dispatch note); this keeps them + // consistent rather than inventing new exit-code behavior for only one of + // the two siblings. + const sandboxPosture = checkCodexSandboxPosture(runtime, cwd); output( { @@ -1746,6 +1761,7 @@ function cmdValidateAgents(cwd: string, raw: boolean): void { incomplete: agentStatus.incomplete_agents, expected, codex_posture: codexPosture, + sandbox_posture: sandboxPosture, }, raw, ); diff --git a/tests/agent-install-check.test.cjs b/tests/agent-install-check.test.cjs index 838ec1e3a..ff2391d5d 100644 --- a/tests/agent-install-check.test.cjs +++ b/tests/agent-install-check.test.cjs @@ -1377,3 +1377,143 @@ describe('checkAgentsInstalled — unchanged by the manifest-reader substitution assert.notStrictEqual(agentInstallCheck.getAgentsDir('codex', projectRoot), localAgentsDir); }); }); + +// ─── 4. Sandbox-mode drift (#3897 rung 3, ADR-3473 §8.3 criterion 3) ────────── +// +// Spec: .gsd/phase/feat-3897-adr3473-83-rungs/{40-design,50-test-matrix}.md, S8/T28. +// +// `checkAgentsInstalled` above (and `checkCodexModelPosture`, the established +// sibling pattern for a codex-only posture check — #3242, "A new sibling +// export... presence is checkAgentsInstalled's job; this function's job starts +// only once the runtime is confirmed codex and only inspects posture") checks +// FILE PRESENCE and MANIFEST COMPLETENESS only. Neither inspects whether an +// installed .toml's `sandbox_mode` line agrees with what that role's tool +// contract says it should be — a TOML that disagrees passes `validate agents` +// today. `checkCodexSandboxPosture` is this test's anticipated name for the new +// sibling check (mirroring `checkCodexModelPosture`'s exact shape); the exact +// export name is the implementer's call, but the export MUST exist for this +// row's acceptance criterion (§8.3 criterion 3) to be met. +// +// RED today: the export does not exist at all. +describe('checkCodexSandboxPosture (#3897 rung 3 — not yet implemented, RED until it lands)', () => { + test('T28 validateAgentsFailsOnSandboxDrift_3897: a TOML whose sandbox_mode disagrees with the derived expectation must fail, naming role/expected/found', (t) => { + const globalHome = createTempDir('gsd-sandbox-drift-'); + t.after(() => cleanup(globalHome)); + const agentsDir = path.join(globalHome, 'agents'); + fs.mkdirSync(agentsDir, { recursive: true }); + + // gsd-executor declares tools: Read, Write, Edit (agents/gsd-executor.md) — + // it MUST derive workspace-write. Install a drifted TOML claiming + // read-only instead, matching the shape generateCodexAgentToml emits. + fs.writeFileSync( + path.join(agentsDir, 'gsd-executor.toml'), + 'name = "gsd-executor"\ndescription = "Executes plans"\nsandbox_mode = "read-only"\n' + + "developer_instructions = '''\nExecute.\n'''\n", + ); + for (const agent of EXPECTED_AGENTS) { + if (agent === 'gsd-executor') continue; + fs.writeFileSync(path.join(agentsDir, `${agent}.toml`), `name = "${agent}"\n`); + } + process.env['CODEX_HOME'] = globalHome; + + assert.equal( + typeof agentInstallCheck.checkCodexSandboxPosture, + 'function', + 'agent-install-check.cjs must export a sandbox-posture check (#3897 rung 3, ADR-3473 §8.3 criterion 3) — it does not exist yet, so a drifted sandbox_mode currently passes validate agents silently', + ); + + const result = agentInstallCheck.checkCodexSandboxPosture('codex'); + assert.equal( + result.ok, + false, + 'a TOML whose sandbox_mode disagrees with the expected derived value must fail the check', + ); + const violation = result.violations.find((v) => v.agent === 'gsd-executor'); + assert.ok(violation, 'the violation must name the drifted agent'); + assert.equal(violation.expected, 'workspace-write', 'the violation must name the EXPECTED sandbox_mode'); + assert.equal(violation.found, 'read-only', 'the violation must name the FOUND (installed) sandbox_mode'); + }); + + // #3897 rung 4 (isolated correctness review, MINOR finding 2): `found` used + // to be read via a naive whole-file regex, unlike the block-aware scanner + // the sibling checkCodexModelPosture uses. Reviewer's fixture: no header + // sandbox_mode pin at all, but a `sandbox_mode = "..."`-shaped line INSIDE + // the developer_instructions block (prose a role's own prompt might + // legitimately discuss). The naive regex misread that prose as a live + // value and manufactured a FALSE violation. + test('a sandbox_mode-shaped line INSIDE developer_instructions is never read as a live value (no false violation)', (t) => { + const globalHome = createTempDir('gsd-sandbox-block-aware-'); + t.after(() => cleanup(globalHome)); + const agentsDir = path.join(globalHome, 'agents'); + fs.mkdirSync(agentsDir, { recursive: true }); + + // gsd-nyquist-auditor's real canonical tools: declares Write/Edit, so the + // derived expectation is workspace-write. Install a TOML that correctly + // has NO header sandbox_mode pin, but whose developer_instructions block + // contains a line that is shaped exactly like a live sandbox_mode pin. + fs.writeFileSync( + path.join(agentsDir, 'gsd-nyquist-auditor.toml'), + 'name = "gsd-nyquist-auditor"\ndescription = "Fills Nyquist validation gaps"\n' + + "developer_instructions = '''\n" + + 'When configuring the sandbox, use:\n' + + 'sandbox_mode = "workspace-write"\n' + + "'''\n", + ); + for (const agent of EXPECTED_AGENTS) { + if (agent === 'gsd-nyquist-auditor') continue; + fs.writeFileSync(path.join(agentsDir, `${agent}.toml`), `name = "${agent}"\n`); + } + process.env['CODEX_HOME'] = globalHome; + + const result = agentInstallCheck.checkCodexSandboxPosture('codex'); + const violation = result.violations.find((v) => v.agent === 'gsd-nyquist-auditor'); + assert.equal( + violation, + undefined, + 'a sandbox_mode-shaped line inside developer_instructions must never be read as the installed value — it must not manufacture a violation for an agent with no real header pin', + ); + }); + + // #3897 rung 4 (isolated correctness review, MINOR finding 3): + // truncatePostureValue exists at agent-install-check.cts precisely for + // this, and the model-posture sibling applies it — the sandbox check did + // not, so an oversized (or secret-shaped) sandbox_mode value could reach + // `validate agents --raw` output at full length. + test('an oversized sandbox_mode value is truncated in the emitted violation (CLI JSON)', (t) => { + const globalHome = createTempDir('gsd-sandbox-truncate-'); + t.after(() => cleanup(globalHome)); + const agentsDir = path.join(globalHome, 'agents'); + fs.mkdirSync(agentsDir, { recursive: true }); + + const oversized = 'x'.repeat(300); + fs.writeFileSync( + path.join(agentsDir, 'gsd-executor.toml'), + `name = "gsd-executor"\ndescription = "Executes plans"\nsandbox_mode = "${oversized}"\n` + + "developer_instructions = '''\nExecute.\n'''\n", + ); + for (const agent of EXPECTED_AGENTS) { + if (agent === 'gsd-executor') continue; + fs.writeFileSync(path.join(agentsDir, `${agent}.toml`), `name = "${agent}"\n`); + } + process.env['CODEX_HOME'] = globalHome; + + const result = agentInstallCheck.checkCodexSandboxPosture('codex'); + const violation = result.violations.find((v) => v.agent === 'gsd-executor'); + assert.ok(violation, 'the violation must name the drifted agent'); + assert.ok( + violation.found.length <= 65, + `violation.found must be truncated (<=64 chars + ellipsis), got length ${violation.found.length}`, + ); + assert.ok(violation.found.endsWith('…'), 'a truncated value must end with the ellipsis marker'); + + // The full-length value must never reach the emitted JSON at all. + const serialized = JSON.stringify(result); + assert.ok(!serialized.includes(oversized), 'the untruncated 300-char value must never appear in the emitted CLI JSON'); + }); +}); + +// T29 (validate agents still fails when a file is missing, S9) is intentionally +// NOT duplicated here — it is already covered by the existing "missing dir" +// coverage documented in this file's own header comment ("missing dir → +// agents_installed:false, missing_agents = all expected") and exercised above. +// It must stay green; nothing in this rung touches that path. diff --git a/tests/agent-install-validation.test.cjs b/tests/agent-install-validation.test.cjs index 3b8261d0f..dea996698 100644 --- a/tests/agent-install-validation.test.cjs +++ b/tests/agent-install-validation.test.cjs @@ -539,6 +539,59 @@ describe('validate agents subcommand (#1371)', () => { }); }); +// ─── Sandbox posture drift, at the COMMAND (#3897 rung 3, ADR-3473 §8.3) ───── +// +// tests/agent-install-check.test.cjs's T28 already proves checkCodexSandboxPosture +// itself detects a drifted sandbox_mode. That is a UNIT row — it drives the +// helper directly. Before verify.cts wired the helper into cmdValidateAgents, +// T28 stayed green while `gsd-tools validate agents` (the actual command a +// user/CI runs) never surfaced the drift at all — the ADR-3180 Decision 4(b) +// failure shape ("asserting at a helper's return, not at the consumer's +// output"). This row drives the real CLI end to end and asserts on ITS JSON, +// so a future regression that leaves the helper correct but un-wired from the +// command fails here even if T28 stays green. +describe('validate agents surfaces sandbox posture drift at the command (#3897 rung 3)', () => { + let tmpDir; + let agentsDir; + + beforeEach(() => { + tmpDir = createTempProject(); + agentsDir = path.join(tmpDir, 'codex-agents'); + fs.mkdirSync(agentsDir, { recursive: true }); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('validateAgentsSurfacesSandboxDriftAtTheCommand_3897: `validate agents` reports the drifted agent/expected/found via sandbox_posture', () => { + // agents/gsd-executor.md (the real repo role source — checkCodexSandboxPosture + // reads it directly, it is not fixture-overridable) declares tools that derive + // workspace-write. Install a drifted TOML claiming read-only instead. + fs.writeFileSync( + path.join(agentsDir, 'gsd-executor.toml'), + 'name = "gsd-executor"\ndescription = "Executes plans"\nsandbox_mode = "read-only"\n' + + "developer_instructions = '''\nExecute.\n'''\n", + ); + + const env = { GSD_AGENTS_DIR: agentsDir, GSD_RUNTIME: 'codex' }; + const result = runGsdTools('validate agents --raw', tmpDir, env); + assert.ok(result.success, `validate agents failed: ${result.error}`); + assert.strictEqual(result.exitCode, 0, 'model-posture drift is report-only, not fatal — sandbox posture must match that precedent'); + + const output = JSON.parse(result.output); + assert.ok( + output.sandbox_posture, + `expected \`validate agents\` output to carry a sandbox_posture key, got keys: ${Object.keys(output).join(', ')}`, + ); + assert.strictEqual(output.sandbox_posture.ok, false); + const violation = output.sandbox_posture.violations.find((v) => v.agent === 'gsd-executor'); + assert.ok(violation, 'the command output must name the drifted agent'); + assert.strictEqual(violation.expected, 'workspace-write'); + assert.strictEqual(violation.found, 'read-only'); + }); +}); + // ─── Bug #1058: validate agents detects manifest-backed .md/.toml pair drift ── describe('bug #1058: validate agents detects manifest-backed .md/.toml pair drift', () => { diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index 980a4dec9..0109d3727 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -67,13 +67,37 @@ const { mergeCodexConfig, install, GSD_CODEX_MARKER, - CODEX_AGENT_SANDBOX, + deriveCodexSandboxMode, + // #3897 rung 3 (ADR-3473 §8.3, option 2 — HALT.md): anticipated new export + // holding the 17 explicit read-only pins for roles whose tool contract would + // otherwise derive workspace-write (16 measured by HALT.md + gsd-nyquist-auditor, + // surfaced by the list-form parse fix). Does not exist on the current tree — + // destructuring a non-existent key is `undefined`, not a throw, so requiring + // this module still succeeds; every test below that touches it fails on its + // own `typeof` guard instead. + CODEX_SANDBOX_HOLDS, parseTomlToObject, validateCodexConfigSchema, } = require('../bin/install.js'); const { resolveNodeRunner } = require('../gsd-core/bin/lib/runtime-hooks-surface.cjs'); const { resolveInstallPlan } = require('../gsd-core/bin/lib/runtime-config-adapter-registry.cjs'); +// #3897 fixup: deriveCodexSandboxMode's 2nd param is now the already-resolved +// `tools:` frontmatter VALUE, not raw agent content (codex-agent-toml.cjs no +// longer parses frontmatter at all — no third copy of that extraction). +const { + extractFrontmatterAndBody, + extractFrontmatterField, +} = require('../gsd-core/bin/lib/runtime-artifact-conversion.cjs'); +// #3897 list-form parse fix: the ONE shared `tools:`-value reader both +// sandbox-feeding production paths (`bin/install.js`'s `generateCodexAgentToml` +// and `agent-install-check.cts`'s `checkCodexSandboxPosture`) now route +// through — handles inline (`tools: Read, Write`) AND YAML block-list +// (`tools:` + indented `- Item` lines) form. Used below by `realAgentToolsRaw` +// so the test's own measurement of "what does this role's tool contract +// declare" cannot silently disagree with production (the exact generative- +// fix-divergence shape this fix closes). +const { extractToolsValue } = require('../gsd-core/bin/lib/codex-agent-toml.cjs'); function runCodexInstall(codexHome, cwd = path.join(__dirname, '..')) { const previousCodeHome = process.env.CODEX_HOME; @@ -483,9 +507,27 @@ tools: Read, Grep, Glob assert.ok(result.includes('description = "GSD agent gsd-unknown"'), 'falls back to synthetic description'); }); - test('defaults unknown agents to read-only', () => { + // #3897 CAUSE B fix: this used to assert the deleted name-based fallback + // (an unknown agent NAME defaulted to read-only regardless of its tool + // contract). Under derivation, identity no longer determines the sandbox — + // the tool contract does (S6: "a new writing role gets the contract, not + // the pin"). Split into the two rows the old single assertion conflated: + // absence of a `tools:` grant (N8) vs. an unknown role that legitimately + // declares a writing tool (S6). + test('unknown agent with no tools: frontmatter derives read-only (N8: absence is not a grant)', () => { + const noToolsAgent = `--- +name: gsd-unknown +description: An unknown agent with no declared tools +--- + +You are an unknown agent.`; + const result = generateCodexAgentToml('gsd-unknown', noToolsAgent); + assert.ok(result.includes('sandbox_mode = "read-only"'), 'no tools: frontmatter -> read-only'); + }); + + test('unknown agent declaring Write/Edit derives workspace-write (S6: the tool contract, not the pin, decides)', () => { const result = generateCodexAgentToml('gsd-unknown', sampleAgent); - assert.ok(result.includes('sandbox_mode = "read-only"'), 'defaults to read-only'); + assert.ok(result.includes('sandbox_mode = "workspace-write"'), 'declares Write/Edit -> workspace-write'); }); // ─── #2256: model_overrides support ─────────────────────────────────────── @@ -919,6 +961,735 @@ description: Maps the codebase }); }); +// ─── #3897 rung 3 (ADR-3473 §8.3, HALT.md option 2): sandbox_mode derives from +// the tool contract, with 17 widening roles held at read-only ──────────────── +// +// Spec: .gsd/phase/feat-3897-adr3473-83-rungs/{40-design,50-test-matrix}.md, +// rows S1-S9 / T20-T30. Measured against `next` @ ad6abc896 (HALT.md): +// deriving `workspace-write` iff an agent's frontmatter `tools:` declares +// `Write` or `Edit` reproduces all 11 CODEX_AGENT_SANDBOX map entries exactly, +// and would additionally widen 16 fallback roles that the map never covered. +// HALT.md's 16 was measured against a `tools:`-VALUE reader that only handled +// inline form; the #3897 list-form parse fix corrected `gsd-nyquist-auditor`'s +// YAML block-list `tools:` (previously misread as `"- Read"`, no Write/Edit +// found), which genuinely derives `workspace-write` and adds a 17th widening +// role. `CODEX_SANDBOX_HOLDS` (destructured above; `undefined` on the current +// tree) is this rung's pin list for those 17 — every row below that depends +// on it fails on its own `typeof` guard until it lands. +// +// Deriving-from-real-content is deliberate for T20/T21/T26/T27/T30: a +// synthetic `tools:` fixture cannot prove the CURRENT tree's byte output is +// preserved, only that the derivation LOGIC agrees with a made-up example. + +// #3897 rung 3 — bin/install.js's `CODEX_AGENT_SANDBOX` map is DELETED +// (ADR-3473 §8.3): HALT.md measured that deriving `workspace-write` iff an +// agent's `tools:` frontmatter declares Write/Edit reproduces every one of +// these 11 entries exactly, with zero disagreements, making the hand- +// maintained map fully redundant. This literal is the pre-#3897 +// `CODEX_AGENT_SANDBOX` contents, preserved here as a regression baseline — +// the only surviving copy of these 11 role -> mode pairs. T21 below and the +// `CODEX_AGENT_SANDBOX (deleted map, derivation regression baseline)` describe +// block both reference this ONE literal rather than duplicating it. +const PRE_3897_CODEX_AGENT_SANDBOX = { + 'gsd-executor': 'workspace-write', + 'gsd-planner': 'workspace-write', + 'gsd-phase-researcher': 'workspace-write', + 'gsd-project-researcher': 'workspace-write', + 'gsd-research-synthesizer': 'workspace-write', + 'gsd-verifier': 'workspace-write', + 'gsd-codebase-mapper': 'workspace-write', + 'gsd-roadmapper': 'workspace-write', + 'gsd-debugger': 'workspace-write', + 'gsd-plan-checker': 'read-only', + 'gsd-integration-checker': 'read-only', +}; + +describe('#3897 rung 3: sandbox_mode derivation and the hold list', () => { + const AGENTS_DIR = path.join(__dirname, '..', 'agents'); + + // #3897 rung 4 (isolated correctness review, MINOR finding 6): T20/N6 used + // to iterate `Object.keys(EXPECTED_SANDBOX_BY_ROLE)` and then pin + // `assert.equal(checked, 35)` — that pins the FIXTURE, not the roster, so a + // 36th agent added to `agents/` would be silently unchecked by the entire + // rung-3 block instead of failing loudly. Driven from the real roster + // instead; a dedicated parity test below fails loudly, naming any file + // present in one set and not the other, the moment the two diverge. + const AGENT_ROSTER_ROLES = fs + .readdirSync(AGENTS_DIR) + .filter((f) => f.endsWith('.md')) + .map((f) => f.slice(0, -'.md'.length)) + .sort(); + + function realAgentToolsRaw(agentName) { + const content = fs.readFileSync(path.join(AGENTS_DIR, `${agentName}.md`), 'utf8'); + // #3897 list-form parse fix: route through the SAME shared extractor + // production uses, rather than a naive single-line regex here — a + // second, test-local reimplementation of "read the tools: value" is + // exactly the generative-fix-divergence shape that let this test's own + // `measuredWideningRoles` silently miss `gsd-nyquist-auditor` (YAML + // list-form `tools:`) before this fix. + return extractToolsValue(content) ?? ''; + } + + // #3897 rung 4 (isolated correctness review, NIT finding 7): this used to + // be a SEPARATE, simpler reimplementation of the real, private + // `_codexToolsDeclareWriteOrEdit` (codex-agent-toml.cts) — a naive + // comma-split-and-includes check that does not handle the real predicate's + // "except" negation form (F5). That is exactly the generative-fix- + // divergence shape CLAUDE.md warns about: a future change to the real + // predicate this copy does not mirror would silently disagree with it + // forever. `_codexToolsDeclareWriteOrEdit` is intentionally not exported + // (module-internal), so rather than reimplementing it a second time, this + // delegates to the REAL implementation through the public + // `deriveCodexSandboxMode`, pinned to an identity guaranteed to never be + // held or "suspicious" (F3) — with no hold in play, + // `deriveCodexSandboxMode(identity, toolsRaw) === 'workspace-write'` IS + // `_codexToolsDeclareWriteOrEdit(toolsRaw)`, byte for byte, because there is + // no second copy left to drift. + const PARITY_PROBE_IDENTITY = 'zzz-parity-probe-never-a-real-or-held-role'; + function declaresWriteOrEdit(toolsRaw) { + return deriveCodexSandboxMode(PARITY_PROBE_IDENTITY, toolsRaw) === 'workspace-write'; + } + + test('sanity: the parity-probe identity used by declaresWriteOrEdit is never itself held or suspicious', () => { + assert.equal( + Object.prototype.hasOwnProperty.call(CODEX_SANDBOX_HOLDS, PARITY_PROBE_IDENTITY), + false, + 'the probe identity must not collide with a real hold, or every T20/T21/T24 verdict derived from it would be silently wrong', + ); + assert.equal( + deriveCodexSandboxMode(PARITY_PROBE_IDENTITY, 'Read, Write, Edit'), + 'workspace-write', + 'sanity: an unheld identity that declares Write/Edit must derive workspace-write', + ); + }); + + // T20 (LOAD-BEARING, N6): frozen fixture of TODAY's real, per-role emitted + // sandbox_mode for all 35 roles in agents/ — captured from the CURRENT build + // by running the REAL generateCodexAgentToml against the REAL agent .md + // content (never a synthetic fixture) and committed here. This is the + // safety net for the refactor: it is true today (nothing has changed yet) + // and MUST remain true, per role, after sandbox_mode moves from the map to a + // derivation — an aggregate "35 roles emitted" count would pass even if one + // role silently widened; asserting per role (one test per role, mirroring + // codex-agent-toml.test.cjs's A14 round-trip pattern) does not let that hide. + const EXPECTED_SANDBOX_BY_ROLE = { + 'gsd-advisor-researcher': 'read-only', + 'gsd-ai-researcher': 'read-only', + 'gsd-assumptions-analyzer': 'read-only', + 'gsd-code-fixer': 'read-only', + 'gsd-code-reviewer': 'read-only', + 'gsd-codebase-mapper': 'workspace-write', + 'gsd-debug-session-manager': 'read-only', + 'gsd-debugger': 'workspace-write', + 'gsd-doc-classifier': 'read-only', + 'gsd-doc-synthesizer': 'read-only', + 'gsd-doc-verifier': 'read-only', + 'gsd-doc-writer': 'read-only', + 'gsd-dom-verifier': 'read-only', + 'gsd-domain-researcher': 'read-only', + 'gsd-eval-auditor': 'read-only', + 'gsd-eval-planner': 'read-only', + 'gsd-executor': 'workspace-write', + 'gsd-framework-selector': 'read-only', + 'gsd-integration-checker': 'read-only', + 'gsd-intel-updater': 'read-only', + 'gsd-mempalace-curator': 'read-only', + 'gsd-nyquist-auditor': 'read-only', + 'gsd-pattern-mapper': 'read-only', + 'gsd-phase-researcher': 'workspace-write', + 'gsd-plan-checker': 'read-only', + 'gsd-planner': 'workspace-write', + 'gsd-project-researcher': 'workspace-write', + 'gsd-research-synthesizer': 'workspace-write', + 'gsd-roadmapper': 'workspace-write', + 'gsd-security-auditor': 'read-only', + 'gsd-ui-auditor': 'read-only', + 'gsd-ui-checker': 'read-only', + 'gsd-ui-researcher': 'read-only', + 'gsd-user-profiler': 'read-only', + 'gsd-verifier': 'workspace-write', + }; + + // The 17 roles that widen: declare Write/Edit, not in the pre-#3897 + // CODEX_AGENT_SANDBOX map (now deleted; PRE_3897_CODEX_AGENT_SANDBOX above + // is the surviving baseline of its contents), so today's `|| 'read-only'` + // fallback under-grants them (16 measured by HALT.md + gsd-nyquist-auditor, + // whose YAML block-list `tools:` the pre-fix single-line reader misread as + // `"- Read"`, hiding its Write/Edit declaration). Computed here from the REAL + // agents/*.md tools frontmatter and that baseline — never hand-copied from + // HALT.md's prose — so this list cannot silently drift from what agents/ + // actually declares (T30: "derived, not a second hardcoded copy"). + const measuredWideningRoles = Object.keys(EXPECTED_SANDBOX_BY_ROLE).filter((role) => { + const declaresWrite = declaresWriteOrEdit(realAgentToolsRaw(role)); + const inOldMap = Object.prototype.hasOwnProperty.call(PRE_3897_CODEX_AGENT_SANDBOX, role); + return declaresWrite && !inOldMap; + }); + + // #3897 rung 4 (isolated correctness review, MINOR finding 6): the roster + // and the expectation table must cover EXACTLY the same set of roles — a + // role present in one and not the other fails loudly here, by name, + // instead of being silently unchecked by the whole rung-3 block. + test('T20 roster parity: EXPECTED_SANDBOX_BY_ROLE covers exactly the real agents/ roster, no more, no fewer', () => { + const expectedRoles = Object.keys(EXPECTED_SANDBOX_BY_ROLE).sort(); + const inRosterNotExpected = AGENT_ROSTER_ROLES.filter((r) => !expectedRoles.includes(r)); + const inExpectedNotRoster = expectedRoles.filter((r) => !AGENT_ROSTER_ROLES.includes(r)); + assert.deepEqual( + inRosterNotExpected, + [], + `agents/ contains role(s) with no EXPECTED_SANDBOX_BY_ROLE entry: ${JSON.stringify(inRosterNotExpected)} — add them or this rung's coverage silently skips them`, + ); + assert.deepEqual( + inExpectedNotRoster, + [], + `EXPECTED_SANDBOX_BY_ROLE names role(s) no longer present in agents/: ${JSON.stringify(inExpectedNotRoster)} — remove the stale entry`, + ); + }); + + for (const role of AGENT_ROSTER_ROLES) { + test(`T20 everyRoleEmitsTheSameSandboxAsBefore: ${role} emits sandbox_mode="${EXPECTED_SANDBOX_BY_ROLE[role]}" byte-identically`, () => { + assert.ok( + Object.prototype.hasOwnProperty.call(EXPECTED_SANDBOX_BY_ROLE, role), + `${role} exists in agents/ but has no EXPECTED_SANDBOX_BY_ROLE entry — see the roster-parity test above`, + ); + const content = fs.readFileSync(path.join(AGENTS_DIR, `${role}.md`), 'utf8'); + const toml = generateCodexAgentToml(role, content); + const match = toml.match(/^sandbox_mode = "([^"]+)"$/m); + assert.ok(match, `${role}'s emitted TOML must contain a sandbox_mode line`); + assert.equal( + match[1], + EXPECTED_SANDBOX_BY_ROLE[role], + `${role} must emit the SAME sandbox_mode after the derivation refactor lands — a per-role regression, not an aggregate count`, + ); + }); + } + + test('T30 holdListMatchesTheMeasuredWideningSet: CODEX_SANDBOX_HOLDS is exactly the 17 measured widening roles, derived not hardcoded twice', () => { + assert.equal( + typeof CODEX_SANDBOX_HOLDS, + 'object', + 'install.js must export CODEX_SANDBOX_HOLDS — the hold list does not exist yet', + ); + assert.notEqual(CODEX_SANDBOX_HOLDS, null); + assert.deepEqual( + Object.keys(CODEX_SANDBOX_HOLDS).sort(), + measuredWideningRoles.sort(), + 'CODEX_SANDBOX_HOLDS must equal exactly the set of roles that declare Write/Edit but were never in the old map — no more, no fewer', + ); + // 16 measured by HALT.md against a single-line tools: reader + 1 + // (gsd-nyquist-auditor, YAML block-list tools: — the list-form parse fix) + // = 17. `measuredWideningRoles` is computed from realAgentToolsRaw, which + // now routes through the fixed extractToolsValue, so this count moves + // WITH the parser fix rather than needing a second hand-edit. + assert.equal(measuredWideningRoles.length, 17, 'sanity: 17 widening roles against the current agents/ tree, once list-form tools: parses correctly'); + }); + + test('T21 mappedRolesDeriveToTheirFormerValue: every former CODEX_AGENT_SANDBOX entry (11) derives to the identical value from its real tool contract', () => { + for (const [role, formerValue] of Object.entries(PRE_3897_CODEX_AGENT_SANDBOX)) { + const derivesWorkspaceWrite = declaresWriteOrEdit(realAgentToolsRaw(role)); + const derived = derivesWorkspaceWrite ? 'workspace-write' : 'read-only'; + assert.equal( + derived, + formerValue, + `${role}: the tool-contract derivation must reproduce the former map value exactly (HALT.md: zero disagreements across all 11)`, + ); + } + assert.equal(Object.keys(PRE_3897_CODEX_AGENT_SANDBOX).length, 11); + }); + + test('T22 nonWritingFallbackRoleDerivesReadOnly: a fallback role declaring neither Write nor Edit derives read-only (S2)', () => { + const role = 'gsd-user-profiler'; // tools: Read (no Write/Edit), never in the map + assert.equal(Object.prototype.hasOwnProperty.call(PRE_3897_CODEX_AGENT_SANDBOX, role), false); + assert.equal(declaresWriteOrEdit(realAgentToolsRaw(role)), false); + const content = fs.readFileSync(path.join(AGENTS_DIR, `${role}.md`), 'utf8'); + const toml = generateCodexAgentToml(role, content); + assert.ok(toml.includes('sandbox_mode = "read-only"')); + }); + + test('T23 heldRoleStaysReadOnlyWithARecordedReason: one of the 17 held roles stays read-only via an explicit, reasoned hold (S3)', () => { + assert.equal(typeof CODEX_SANDBOX_HOLDS, 'object', 'CODEX_SANDBOX_HOLDS does not exist yet'); + const role = 'gsd-doc-writer'; // declares Write+Edit; one of HALT.md's 16 + assert.ok(declaresWriteOrEdit(realAgentToolsRaw(role)), 'sanity: this role must actually declare a writing tool'); + assert.ok(Object.prototype.hasOwnProperty.call(CODEX_SANDBOX_HOLDS, role), `${role} must be an explicit hold entry`); + const entry = CODEX_SANDBOX_HOLDS[role]; + const reason = typeof entry === 'string' ? entry : entry && entry.reason; + assert.equal(typeof reason, 'string', `the hold for ${role} must carry a recorded reason string, not a bare boolean pin`); + assert.ok(reason.length > 0); + const content = fs.readFileSync(path.join(AGENTS_DIR, `${role}.md`), 'utf8'); + const toml = generateCodexAgentToml(role, content); + assert.ok(toml.includes('sandbox_mode = "read-only"'), 'byte-identical output despite the hold, per N6'); + }); + + test('T24 staleHoldFailsRatherThanBeingHonored: a hold whose role no longer derives broader must FAIL, naming the role (S4)', () => { + assert.equal( + typeof CODEX_SANDBOX_HOLDS, + 'object', + 'CODEX_SANDBOX_HOLDS does not exist yet, so there is nothing to validate for staleness', + ); + // Every REAL hold entry, right now, must still derive workspace-write from + // the tool contract. A hold for a role whose tools no longer declare + // Write/Edit is exactly the staleness this row exists to catch; without + // this assertion the hold list is honored unconditionally forever, which + // is the hand-maintained-subset-map defect this rung deletes, rebuilt one + // list later (the ledger claim HALT.md makes). + for (const role of Object.keys(CODEX_SANDBOX_HOLDS)) { + assert.ok( + declaresWriteOrEdit(realAgentToolsRaw(role)), + `stale hold: ${role} is pinned to read-only but its CURRENT tool contract no longer declares Write/Edit — this hold must fail validation, not be silently honored`, + ); + } + }); + + test('T25 holdForUnknownRoleFails: a hold naming a role that no longer exists in agents/ must FAIL (S5)', () => { + assert.equal( + typeof CODEX_SANDBOX_HOLDS, + 'object', + 'CODEX_SANDBOX_HOLDS does not exist yet, so there is nothing to validate for an unknown role', + ); + for (const role of Object.keys(CODEX_SANDBOX_HOLDS)) { + assert.ok( + fs.existsSync(path.join(AGENTS_DIR, `${role}.md`)), + `stale hold: ${role} is pinned but no longer exists in agents/ — this hold must fail validation`, + ); + } + }); + + test('T26 newWritingRoleGetsTheContractNotThePin: a brand-new agent declaring Write, with no hold, derives workspace-write (S6) — RED today, falls back to read-only', () => { + const newAgentContent = `--- +name: gsd-totally-new-agent +description: A brand-new writing agent that has never been in the map or a hold +tools: Read, Write, Bash +--- + +You are a brand-new agent.`; + // Today: CODEX_AGENT_SANDBOX['gsd-totally-new-agent'] is undefined, so the + // `|| 'read-only'` fallback silently under-grants it — the exact defect + // §8.3 exists to fix (24 of 35 roles fell through this fallback). + const toml = generateCodexAgentToml('gsd-totally-new-agent', newAgentContent); + assert.ok( + toml.includes('sandbox_mode = "workspace-write"'), + 'a new agent declaring Write, absent from both the map and any hold, must derive workspace-write from its own tool contract — not silently fall back to read-only', + ); + }); + + test('T27 absentToolContractIsNotAGrant: an agent with no tools: frontmatter at all derives read-only (N8/S7)', () => { + const noToolsContent = `--- +name: gsd-no-contract-agent +description: Declares no tools frontmatter key at all +--- + +You have no declared tools.`; + const toml = generateCodexAgentToml('gsd-no-contract-agent', noToolsContent); + assert.ok( + toml.includes('sandbox_mode = "read-only"'), + 'absence of a tools: contract must never be read as a grant of workspace-write', + ); + }); + + // #3897 security review follow-up (post-merge blocker): `bin/install.js`'s + // Codex install loop used to key the CODEX_SANDBOX_HOLDS lookup off the + // agent's OWN frontmatter `name:` field (`extractFrontmatterField(frontmatter, + // 'name') || file.replace('.md', '')`) rather than its filename — so editing, + // or merely recasing, a held role's `name:` field (same file, same tool + // contract) silently derived `workspace-write` instead of the pinned + // `read-only`. Unlike the deleted CODEX_AGENT_SANDBOX map (an ALLOWLIST whose + // unmatched-key fallback was `read-only`, i.e. safe), CODEX_SANDBOX_HOLDS is a + // SUBTRACTION from a derivation that defaults to `workspace-write`, so the + // identical lookup-key mismatch now fails OPEN — a severity flip. The fix + // keys the hold off the canonical source FILENAME stem (what + // `validateCodexSandboxHolds` already verifies exists), threaded through + // `installCodexConfig`'s per-file loop independently of the frontmatter + // `name:` used for the TOML body, with a case-insensitive lookup as a second + // line of defense. + test('heldRoleCannotEscapeItsHoldByRenamingFrontmatter_3897: editing or recasing a held role\'s frontmatter name: must not change its sandbox_mode from read-only', () => { + const { installCodexConfig } = require('../bin/install.js'); + const heldRole = 'gsd-doc-writer'; // one of the 17 CODEX_SANDBOX_HOLDS entries + assert.ok( + Object.prototype.hasOwnProperty.call(CODEX_SANDBOX_HOLDS, heldRole), + `sanity: ${heldRole} must be a real CODEX_SANDBOX_HOLDS entry`, + ); + + const variants = [ + { label: 'frontmatter name: edited to a different value', newName: 'gsd-doc-writer-x' }, + { label: 'frontmatter name: merely recased', newName: 'GSD-Doc-Writer' }, + ]; + + for (const { label, newName } of variants) { + const tmpAgentsSrc = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-codex-tamper-src-')); + const tmpDest = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-codex-tamper-dest-')); + try { + fs.cpSync(AGENTS_DIR, tmpAgentsSrc, { recursive: true }); + const rolePath = path.join(tmpAgentsSrc, `${heldRole}.md`); + const original = fs.readFileSync(rolePath, 'utf8'); + const tampered = original.replace(/^name:\s*.*$/m, `name: ${newName}`); + assert.notEqual(tampered, original, `sanity: the frontmatter name: line must actually change (${label})`); + fs.writeFileSync(rolePath, tampered); + + // The FILE on disk is untouched (still `gsd-doc-writer.md`) — only its + // frontmatter content changed. validateCodexSandboxHolds only checks + // the file exists, so it does not (and should not) catch this by itself. + installCodexConfig(tmpDest, tmpAgentsSrc); + + const emittedTomlPath = path.join(tmpDest, 'agents', `${newName}.toml`); + assert.ok( + fs.existsSync(emittedTomlPath), + `${label}: expected an emitted .toml at ${emittedTomlPath} (named after the tampered frontmatter name, per existing TOML-naming behavior — unrelated to this fix)`, + ); + const toml = fs.readFileSync(emittedTomlPath, 'utf8'); + const sandboxLine = toml.match(/^sandbox_mode = "([^"]{0,50})"$/m); + assert.ok(sandboxLine, `${label}: emitted .toml must contain a sandbox_mode line`); + assert.equal( + sandboxLine[1], + 'read-only', + `${label}: a held role's sandbox_mode must stay read-only even when its frontmatter name: diverges ` + + `from its own filename — the hold is keyed off the file, not a self-declared field. Got: ${sandboxLine[1]}`, + ); + } finally { + cleanup(tmpAgentsSrc); + cleanup(tmpDest); + } + } + }); + + test('N6 (post-fix regression check): every real agents/ role remains byte-identical in emitted sandbox_mode after the filename-identity fix', () => { + // #3897 rung 4 (isolated correctness review, MINOR finding 6): driven + // from the real roster (AGENT_ROSTER_ROLES), not the hardcoded + // EXPECTED_SANDBOX_BY_ROLE key set — the roster-parity test above already + // fails loudly if the two sets ever diverge, so `checked` here is a + // genuine roster count, not a fixture-pinned literal that would silently + // stop growing when a 36th agent lands. + let checked = 0; + for (const role of AGENT_ROSTER_ROLES) { + assert.ok( + Object.prototype.hasOwnProperty.call(EXPECTED_SANDBOX_BY_ROLE, role), + `${role} exists in agents/ but has no EXPECTED_SANDBOX_BY_ROLE entry — see the roster-parity test above`, + ); + const content = fs.readFileSync(path.join(AGENTS_DIR, `${role}.md`), 'utf8'); + const toml = generateCodexAgentToml(role, content); + const match = toml.match(/^sandbox_mode = "([^"]+)"$/m); + assert.ok(match, `${role} must emit a sandbox_mode line`); + assert.equal( + match[1], + EXPECTED_SANDBOX_BY_ROLE[role], + `${role}: sandbox_mode must not drift as a side effect of the #3897 filename-identity fix`, + ); + checked++; + } + assert.equal( + checked, + AGENT_ROSTER_ROLES.length, + 'N6: every real role in agents/ must be checked and byte-identical', + ); + }); +}); + +// ─── #3897 security review F1/F3/F4/F5 regressions ───────────────────────── +// +// Isolated security review of this rung found a fail-open: `bin/install.js`'s +// Codex emit loop DECIDED sandbox_mode for the source filename stem but +// APPLIED it to the emitted `.toml`'s path, which is keyed on the +// frontmatter `name:` value instead — so a renamed file, or a sibling file +// whose `name:` collides with a held role, could land a held role's own +// artifact at `workspace-write`. F1(a)/F1(b) below assert on the EMITTED +// ARTIFACT (the written `.toml`'s `sandbox_mode`), never on +// `deriveCodexSandboxMode`'s return value directly — the whole defect is +// that the derivation and the emitted artifact could disagree. +describe('#3897 security review: F1 filename/name identity confusion, F3 confusables, F4/F5 totality', () => { + const AGENTS_DIR = path.join(__dirname, '..', 'agents'); + const { + installCodexConfig, + deriveCodexSandboxMode: deriveCodexSandboxModeLocal, + } = require('../bin/install.js'); + const { + normalizeSandboxIdentity, + isSandboxHeld, + extractToolsValue: extractToolsValueLocal, + } = require('../gsd-core/bin/lib/codex-agent-toml.cjs'); + + function sandboxModeOfToml(toml) { + const match = toml.match(/^sandbox_mode = "([^"]+)"$/m); + return match ? match[1] : null; + } + + test('F1(a) rename case: a source file whose FILENAME differs from a held role, but whose frontmatter name: IS the held role, emits read-only', () => { + const src = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-f1a-src-')); + const dest = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-f1a-dest-')); + try { + const original = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-doc-writer.md'), 'utf8'); + assert.ok(/^name:\s*gsd-doc-writer\s*$/m.test(original), 'sanity: fixture source declares name: gsd-doc-writer'); + // Renamed FILE, unchanged frontmatter name: — the fileStem no longer + // matches the held role, but the emitted TOML is still named after + // `gsd-doc-writer` (the frontmatter name:). + fs.writeFileSync(path.join(src, 'gsd-doc-writer-v2.md'), original); + + installCodexConfig(dest, src); + + const emittedPath = path.join(dest, 'agents', 'gsd-doc-writer.toml'); + assert.ok(fs.existsSync(emittedPath), `expected an emitted .toml at ${emittedPath}`); + const toml = fs.readFileSync(emittedPath, 'utf8'); + assert.equal( + sandboxModeOfToml(toml), + 'read-only', + 'F1(a): a held role\'s own emitted .toml must stay read-only even when reached via a renamed source file whose filename stem is unheld', + ); + } finally { + cleanup(src); + cleanup(dest); + } + }); + + test('F1(b) sibling-clobber case: a second, unheld source file whose frontmatter name: IS a held role must not widen the held role\'s emitted .toml', () => { + const src = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-f1b-src-')); + const dest = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-f1b-dest-')); + try { + const heldOriginal = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-doc-writer.md'), 'utf8'); + fs.writeFileSync(path.join(src, 'gsd-doc-writer.md'), heldOriginal); + // A sibling file — different filename stem, no hold entry for IT — but + // its frontmatter `name:` collides with the held role. Named to sort + // AFTER `gsd-doc-writer.md` (readdirSync/emit-loop order is + // alphabetical) so this sibling's write is the LAST one to the shared + // `gsd-doc-writer.toml` output path — the exact ordering the pre-fix + // defect needed to actually clobber the held role's own artifact + // (a sibling sorting BEFORE it gets silently overwritten again by the + // legitimate file's own correct read-only write, masking the bug). + const sibling = [ + '---', + 'name: gsd-doc-writer', + 'description: sibling file colliding on frontmatter name', + 'tools: Read, Write, Edit', + '---', + '', + 'sibling', + ].join('\n'); + fs.writeFileSync(path.join(src, 'gsd-zzz-attacker-clone.md'), sibling); + + installCodexConfig(dest, src); + + const emittedPath = path.join(dest, 'agents', 'gsd-doc-writer.toml'); + assert.ok(fs.existsSync(emittedPath), `expected an emitted .toml at ${emittedPath}`); + const toml = fs.readFileSync(emittedPath, 'utf8'); + assert.equal( + sandboxModeOfToml(toml), + 'read-only', + 'F1(b): a sibling file whose frontmatter name: collides with a held role must not clobber that role\'s emitted .toml with workspace-write', + ); + } finally { + cleanup(src); + cleanup(dest); + } + }); + + const F3_CONFUSABLE_VECTORS = [ + ['Turkish dotted I (İ)', 'gsd-doc-wrİter'], + ['Turkish dotless i (ı)', 'gsd-doc-wrıter'], + ['fullwidth leading g (ｇ)', 'ｇsd-doc-writer'], + ['NFD combining acute on r (writeŕ)', 'gsd-doc-writeŕ'], + ['trailing ASCII space', 'gsd-doc-writer '], + ['trailing NBSP', 'gsd-doc-writer '], + ['trailing dot', 'gsd-doc-writer.'], + ['trailing newline', 'gsd-doc-writer\n'], + ['trailing carriage return', 'gsd-doc-writer\r'], + ['relative-path prefix ./', './gsd-doc-writer'], + ['path traversal ../agents/', '../agents/gsd-doc-writer'], + ]; + + for (const [label, vector] of F3_CONFUSABLE_VECTORS) { + test(`F3 confusable/whitespace/path vector — ${label} — derives read-only`, () => { + const mode = deriveCodexSandboxModeLocal(vector, 'Read, Write, Edit'); + assert.equal( + mode, + 'read-only', + `F3: identity ${JSON.stringify(vector)} (${label}) must derive read-only — either it normalizes onto the real held key, or it is unrecognizable and must fail closed`, + ); + }); + } + + test('F3: isSandboxHeld flags each confusable vector as held or suspicious (never silently neither)', () => { + for (const [label, vector] of F3_CONFUSABLE_VECTORS) { + const { held, suspicious } = isSandboxHeld(vector); + assert.ok(held || suspicious, `${label} (${JSON.stringify(vector)}) must be held or suspicious`); + } + }); + + test('F5: "All tools except Write, Edit" derives read-only (negation excludes Write/Edit)', () => { + const mode = deriveCodexSandboxModeLocal('gsd-negation-agent', 'All tools except Write, Edit'); + assert.equal(mode, 'read-only', 'excluding Write and Edit after "except" must derive read-only'); + }); + + test('F5: "All tools except Agent" derives workspace-write (Write/Edit are not excluded)', () => { + const mode = deriveCodexSandboxModeLocal('gsd-negation-agent', 'All tools except Agent'); + assert.equal(mode, 'workspace-write', 'excluding only Agent leaves Write/Edit granted, so this must still derive workspace-write'); + }); + + test('totality: deriveCodexSandboxMode never throws for undefined/null/[]/[undefined] identities', () => { + for (const identity of [undefined, null, [], [undefined]]) { + assert.doesNotThrow( + () => deriveCodexSandboxModeLocal(identity, 'Read, Write, Edit'), + `deriveCodexSandboxMode must not throw for identity ${JSON.stringify(identity)}`, + ); + const result = deriveCodexSandboxModeLocal(identity, 'Read, Write, Edit'); + assert.equal(typeof result, 'string', `deriveCodexSandboxMode must return a string for identity ${JSON.stringify(identity)}`); + } + }); + + // #3897 rung 4 (isolated correctness review, MINOR finding 4): two exotic + // identity shapes more adversarial than the undefined/null/[]/[undefined] + // set above — an object whose OWN toString throws, and a null-prototype + // object (no inherited Object.prototype methods at all, so even a + // defensive `.toString`/`.hasOwnProperty` call site would blow up). Both + // must derive a string, never throw — "TOTAL for any input" per this + // function's own docstring is not just for well-behaved falsy/array shapes. + test('totality: deriveCodexSandboxMode never throws for an identity with a throwing toString', () => { + const hostile = { toString() { throw new Error('id-boom'); } }; + assert.doesNotThrow( + () => deriveCodexSandboxModeLocal(hostile, 'Read'), + 'deriveCodexSandboxMode must not throw for an identity whose toString throws', + ); + assert.equal(typeof deriveCodexSandboxModeLocal(hostile, 'Read'), 'string'); + }); + + test('totality: deriveCodexSandboxMode never throws for a null-prototype identity object', () => { + const nullProto = Object.create(null); + assert.doesNotThrow( + () => deriveCodexSandboxModeLocal(nullProto, 'Read'), + 'deriveCodexSandboxMode must not throw for a null-prototype identity object', + ); + assert.equal(typeof deriveCodexSandboxModeLocal(nullProto, 'Read'), 'string'); + }); + + test('totality: normalizeSandboxIdentity never throws for undefined/null/non-string input', () => { + for (const raw of [undefined, null, 42, {}, []]) { + assert.doesNotThrow(() => normalizeSandboxIdentity(raw), `must not throw for ${JSON.stringify(raw)}`); + assert.equal(normalizeSandboxIdentity(raw), null, `non-string input ${JSON.stringify(raw)} must normalize to null`); + } + }); + + test('F4: extractToolsValue returns undefined (never throws) for undefined/null/Buffer input', () => { + for (const value of [undefined, null, Buffer.from('tools: Write')]) { + assert.doesNotThrow(() => extractToolsValueLocal(value), `extractToolsValue must not throw for ${String(value)}`); + assert.equal(extractToolsValueLocal(value), undefined, `extractToolsValue must return undefined for non-string input ${String(value)}`); + } + }); + + test('F4: extractToolsValue still returns the parsed value for real string content — inline form (no regression)', () => { + const content = ['---', 'tools: Read, Write', '---', ''].join('\n'); + assert.equal(extractToolsValueLocal(content), 'Read, Write'); + }); + + // ─── #3897 list-form parse fix: YAML block-list `tools:` ───────────────── + // + // `agents/gsd-nyquist-auditor.md` and `agents/gsd-security-auditor.md` are + // the only two roster files using this shape. The pre-fix single-line + // regex `/^tools:\s*(.+)$/m` let `\s*` swallow the newline after a bare + // `tools:` key and matched into the FIRST list item's own line, returning + // just `"- Read"` — a real Write/Edit DECLARATION read as an absence. + + test('list-form: a tools: block list declaring Write derives workspace-write (FAILS before the parse fix — pre-fix reader returned "- Read")', () => { + const content = [ + '---', + 'name: gsd-list-form-writer', + 'tools:', + ' - Read', + ' - Write', + ' - Edit', + ' - Bash', + '---', + '', + 'list-form writer', + ].join('\n'); + const toolsRaw = extractToolsValueLocal(content); + assert.equal(toolsRaw, 'Read, Write, Edit, Bash', 'list items must be joined the same way the comma-tokenizer downstream expects'); + assert.equal( + deriveCodexSandboxModeLocal('zzz-list-form-probe-never-held', toolsRaw), + 'workspace-write', + 'a tools: block list declaring Write must derive workspace-write, not read-only from a truncated first-item read', + ); + }); + + test('list-form: a tools: block list with NO write tool derives read-only (no over-correction)', () => { + const content = [ + '---', + 'name: gsd-list-form-reader', + 'tools:', + ' - Read', + ' - Bash', + ' - Glob', + '---', + '', + 'list-form reader', + ].join('\n'); + const toolsRaw = extractToolsValueLocal(content); + assert.equal(toolsRaw, 'Read, Bash, Glob'); + assert.equal( + deriveCodexSandboxModeLocal('zzz-list-form-probe-never-held', toolsRaw), + 'read-only', + 'a list-form tools: with no Write/Edit item must still derive read-only', + ); + }); + + test('list-form: the list terminates at the next frontmatter key and does not swallow its value', () => { + const content = [ + '---', + 'name: gsd-list-form-terminates', + 'tools:', + ' - Read', + ' - Write', + 'color: blue', + '---', + '', + 'list-form terminates before a sibling key', + ].join('\n'); + assert.equal(extractToolsValueLocal(content), 'Read, Write'); + // The sibling key's own value must remain independently readable — it + // must never have been consumed as a phantom third list item. + const { frontmatter } = extractFrontmatterAndBody(content); + assert.equal(extractFrontmatterField(frontmatter, 'color'), 'blue'); + }); + + test('list-form: the list terminates at the closing --- and does not run past the frontmatter', () => { + const content = [ + '---', + 'name: gsd-list-form-eof', + 'tools:', + ' - Read', + ' - Write', + '---', + '- this looks like a list item but is BODY text, not frontmatter', + ].join('\n'); + assert.equal(extractToolsValueLocal(content), 'Read, Write'); + }); + + test('roster truth: gsd-nyquist-auditor DERIVES workspace-write from its real tool contract AND is HELD, so its emitted .toml stays read-only', () => { + const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-nyquist-auditor.md'), 'utf8'); + const toolsRaw = extractToolsValueLocal(content); + assert.ok( + /\bWrite\b/.test(toolsRaw) && /\bEdit\b/.test(toolsRaw), + `sanity: gsd-nyquist-auditor's real tools: must declare both Write and Edit, got ${JSON.stringify(toolsRaw)}`, + ); + // Derivation WITHOUT the hold (an identity guaranteed never held/suspicious, + // same probe idiom as the rung-3 describe block's PARITY_PROBE_IDENTITY) + // must show the role genuinely derives workspace-write from its contract — + // this is what proves derive-and-hold is doing real work, not that the + // parser happens to agree with the pin by accident. + assert.equal( + deriveCodexSandboxModeLocal('zzz-nyquist-unheld-probe-never-a-real-role', toolsRaw), + 'workspace-write', + 'gsd-nyquist-auditor must genuinely derive workspace-write from its tool contract once list-form tools: parses correctly', + ); + // The REAL identity IS held, so the actual emitted artifact stays read-only. + assert.ok( + Object.prototype.hasOwnProperty.call(CODEX_SANDBOX_HOLDS, 'gsd-nyquist-auditor'), + 'gsd-nyquist-auditor must be an explicit CODEX_SANDBOX_HOLDS entry', + ); + const { generateCodexAgentToml: generateCodexAgentTomlLocal } = require('../bin/install.js'); + const toml = generateCodexAgentTomlLocal('gsd-nyquist-auditor', content); + assert.ok( + toml.includes('sandbox_mode = "read-only"'), + 'gsd-nyquist-auditor\'s emitted .toml must stay read-only (held), even though it now derives workspace-write', + ); + }); +}); + // ─── #3241: shared isAnthropicFlavoredModel / CLAUDE_AGENT_ALIASES surface ───── // Phase 2/3 need one predicate. 40-design.md's "seam decision" moves // CLAUDE_AGENT_ALIASES into model-catalog.cjs and defines isAnthropicFlavoredModel @@ -1091,29 +1862,46 @@ describe('installCodexConfig sandboxTier threading seam', () => { // codex.sandboxTier === 'codex-agent-sandbox') confirms the real registry has // valid values for all 15 runtimes. -// ─── CODEX_AGENT_SANDBOX mapping ──────────────────────────────────────────────── +// ─── CODEX_AGENT_SANDBOX (deleted map): derivation regression baseline ────────── -describe('CODEX_AGENT_SANDBOX', () => { - test('has all 11 agents mapped', () => { - const agentNames = Object.keys(CODEX_AGENT_SANDBOX); +describe('CODEX_AGENT_SANDBOX (deleted map, derivation regression baseline)', () => { + // bin/install.js's CODEX_AGENT_SANDBOX map is gone (ADR-3473 §8.3, #3897 + // rung 3) — deriveCodexSandboxMode is the sole owner of sandbox_mode now. + // These tests assert the SAME underlying property the deleted map's own + // suite used to assert (which 11 roles get which sandbox_mode), but against + // the real derivation and the real agents/*.md content, sourced from the + // PRE_3897_CODEX_AGENT_SANDBOX baseline literal above rather than the map. + const AGENTS_DIR = path.join(__dirname, '..', 'agents'); + + function realDerivedSandboxMode(role) { + const content = fs.readFileSync(path.join(AGENTS_DIR, `${role}.md`), 'utf8'); + const { frontmatter } = extractFrontmatterAndBody(content); + const toolsRaw = extractFrontmatterField(frontmatter || '', 'tools') || ''; + return deriveCodexSandboxMode(role, toolsRaw); + } + + test('has all 11 baseline agents', () => { + const agentNames = Object.keys(PRE_3897_CODEX_AGENT_SANDBOX); assert.strictEqual(agentNames.length, 11, 'has 11 agents'); }); - test('workspace-write agents have write tools', () => { + test('workspace-write agents still derive workspace-write', () => { const writeAgents = [ 'gsd-executor', 'gsd-planner', 'gsd-phase-researcher', 'gsd-project-researcher', 'gsd-research-synthesizer', 'gsd-verifier', 'gsd-codebase-mapper', 'gsd-roadmapper', 'gsd-debugger', ]; for (const name of writeAgents) { - assert.strictEqual(CODEX_AGENT_SANDBOX[name], 'workspace-write', `${name} is workspace-write`); + assert.strictEqual(PRE_3897_CODEX_AGENT_SANDBOX[name], 'workspace-write', `${name} baseline is workspace-write`); + assert.strictEqual(realDerivedSandboxMode(name), 'workspace-write', `${name} still derives workspace-write`); } }); - test('read-only agents have no write tools', () => { + test('read-only agents still derive read-only', () => { const readOnlyAgents = ['gsd-plan-checker', 'gsd-integration-checker']; for (const name of readOnlyAgents) { - assert.strictEqual(CODEX_AGENT_SANDBOX[name], 'read-only', `${name} is read-only`); + assert.strictEqual(PRE_3897_CODEX_AGENT_SANDBOX[name], 'read-only', `${name} baseline is read-only`); + assert.strictEqual(realDerivedSandboxMode(name), 'read-only', `${name} still derives read-only`); } }); }); diff --git a/tests/phase-dependency-levels.test.cjs b/tests/phase-dependency-levels.test.cjs index 70bdb329f..8fb5915e7 100644 --- a/tests/phase-dependency-levels.test.cjs +++ b/tests/phase-dependency-levels.test.cjs @@ -9,7 +9,8 @@ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); -const { computeDependencyLevels } = require('../gsd-core/bin/lib/phase.cjs'); +const { computeDependencyLevels, buildShortFormToId } = require('../gsd-core/bin/lib/phase.cjs'); +const { extractCanonicalPlanId } = require('../gsd-core/bin/lib/core-utils.cjs'); // Helper: build rawPlans + planMap + canonicalToId from a simple spec. // spec is an array of { id, dependsOn } objects. @@ -240,3 +241,145 @@ describe('computeDependencyLevels — unresolved dependency reporting (#3427, AD }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// #3897 rung 4 (ADR-3473 §8.9) — shortFormToId, the recovered third +// depends_on resolution tier (D3, 40-design.md), recovered from the retired +// SDK lineage (sdk/src/query/phase.ts at 11918dcc3^) with one deliberate +// narrowing (a trailing dash-segment must be a plan NUMBER — /^\d+$/ — see +// {@link buildShortFormToId}'s own comment in phase.cts for why: an +// unconstrained trailing segment let `depends_on: ["auth"]` silently +// mis-resolve to whichever of two non-numeric-suffixed plans sorted first, +// with zero warning — isolated correctness review, #3897). +// +// #3897 rung 4 (isolated correctness review, NIT finding 7): this file used +// to reimplement the algorithm in-test (`buildShortFormMap`) and assert +// against that COPY — exactly the generative-fix-divergence shape CLAUDE.md +// warns about: after the numeric-only fix above landed here, the in-test +// copy would have silently kept the OLD (wrong) behavior forever, agreeing +// with itself while disagreeing with the real implementation. Fixed by +// importing and asserting against the REAL exported +// {@link buildShortFormToId} from phase.cjs — the same function +// `cmdPhasePlanIndex` actually calls — so this file can never again drift +// from its own subject. +// ───────────────────────────────────────────────────────────────────────────── + +function buildInputsWithShortForm(spec) { + const rawPlans = spec.map((s) => ({ id: s.id, dependsOn: s.dependsOn ?? [] })); + const planMap = new Map(rawPlans.map((p) => [p.id.toLowerCase(), p])); + const canonicalToId = new Map(rawPlans.map((p) => [extractCanonicalPlanId(p.id).toLowerCase(), p.id])); + const shortFormToId = buildShortFormToId(rawPlans); + return { rawPlans, planMap, canonicalToId, shortFormToId }; +} + +describe('computeDependencyLevels — shortFormToId, the third depends_on tier (#3897 rung 4)', () => { + + // T42 — RED today: a bare plan-number short form is dropped by both + // existing tiers (planMap wants the full id, canonicalToId wants the + // canonical prefix `24-01`), so 24-02 never resolves its dependency on + // 24-01-auth-hardening and stays at level 0 instead of level 1. + test('T42 shortFormPlanNumberResolves_3427: depends_on: ["01"] resolves to the in-phase sibling plan (D3)', () => { + const { rawPlans, planMap, canonicalToId, shortFormToId } = buildInputsWithShortForm([ + { id: '24-01-auth-hardening', dependsOn: [] }, + { id: '24-02-token-rotation', dependsOn: ['01'] }, + ]); + assert.deepStrictEqual([...shortFormToId.entries()], [['01', '24-01-auth-hardening'], ['02', '24-02-token-rotation']]); + + const result = computeDependencyLevels(rawPlans, planMap, canonicalToId, shortFormToId); + assert.strictEqual((result.unresolved ?? []).length, 0, 'the bare short form must resolve — no dropped edge'); + assert.strictEqual(result.level.get('24-01-auth-hardening'), 0); + assert.strictEqual( + result.level.get('24-02-token-rotation'), + 1, + 'the short-form dependency on "01" must place 24-02-token-rotation one level after 24-01-auth-hardening — today the edge is dropped and both sit at level 0', + ); + }); + + // T44 (D4, boundary): two plans whose canonical id happens to share the + // same trailing short form. First write (array/insertion order, which in + // production is sorted-plan-file order) wins deterministically — the + // retired SDK's own rule, preserved rather than improved (L2: "01" is + // ambiguous by construction whenever it happens). + test('T44 duplicateShortFormIsFirstWriteWinsDeterministically: the FIRST plan to claim a short form wins it, repeatably', () => { + const spec = [ + { id: '24-01-auth-hardening', dependsOn: [] }, // canonical '24-01' -> shortForm '01', inserted FIRST + { id: '9-01-legacy-carryover', dependsOn: [] }, // canonical '9-01' -> shortForm '01' too, inserted SECOND + { id: 'consumer', dependsOn: ['01'] }, + ]; + // Run it twice — determinism means the SAME plan wins both times, not + // just A time. + for (let run = 0; run < 2; run++) { + const { rawPlans, planMap, canonicalToId, shortFormToId } = buildInputsWithShortForm(spec); + assert.strictEqual( + shortFormToId.get('01'), + '24-01-auth-hardening', + `run ${run}: the FIRST plan to declare short form "01" must win it, every time`, + ); + const result = computeDependencyLevels(rawPlans, planMap, canonicalToId, shortFormToId); + assert.strictEqual((result.unresolved ?? []).length, 0); + assert.strictEqual( + result.level.get('consumer'), + result.level.get('24-01-auth-hardening') + 1, + `run ${run}: "consumer" must resolve to the FIRST-written short-form owner (24-01-auth-hardening), not the second (9-01-legacy-carryover)`, + ); + } + }); + + // T45 (D5, boundary): a canonical id with no dash at all (e.g. a bare + // phase-number-only plan id) must never be added to the short-form index — + // `lastDash > 0` excludes it — and its own full-id dependency resolution + // must be completely unaffected by the third tier's presence. + test('T45 canonicalIdWithoutDashIsNotShortFormIndexed: a no-dash canonical id is excluded from the short-form tier and resolves unaffected (D5)', () => { + const { rawPlans, planMap, canonicalToId, shortFormToId } = buildInputsWithShortForm([ + { id: '24', dependsOn: [] }, // extractCanonicalPlanId('24') === '24' — no dash + { id: 'B', dependsOn: ['24'] }, + ]); + assert.strictEqual(shortFormToId.has('24'), false, 'a canonical id with no dash must never be added to the short-form index'); + assert.strictEqual(shortFormToId.size, 0); + + const result = computeDependencyLevels(rawPlans, planMap, canonicalToId, shortFormToId); + assert.strictEqual(result.visited, 2); + assert.strictEqual(result.level.get('24'), 0); + assert.strictEqual(result.level.get('B'), 1, 'the full-id dependency must resolve exactly as before, unaffected by the (absent) short-form entry'); + }); + + // #3897 rung 4 (isolated correctness review, NIT finding 8a): the D5 + // comment on buildShortFormToId names TWO excluded shapes — `lastDash === + // 0` and a trailing dash — but only `lastDash === -1` (T45 above) was + // ever exercised. Both boundary rows below. + test('T46 lastDashAtIndexZeroIsNotShortFormIndexed: a canonical id starting with a dash (D5 boundary) is excluded', () => { + // extractCanonicalPlanId('-01') === '-01' (no phase-token pair found — + // splitting on '-' filters out the leading empty segment, leaving a + // single token '01' with nothing after it to pair with), so + // canonical.lastIndexOf('-') === 0 here — the D5 boundary this rung's + // `lastDash > 0` guard must exclude. + const { shortFormToId } = buildInputsWithShortForm([ + { id: '-01', dependsOn: [] }, + ]); + assert.strictEqual( + extractCanonicalPlanId('-01'), + '-01', + 'sanity: extractCanonicalPlanId must leave this id unpaired, so lastDash === 0', + ); + assert.strictEqual(shortFormToId.has('01'), false, 'a canonical id whose ONLY dash is at index 0 must never be added to the short-form index (D5)'); + assert.strictEqual(shortFormToId.size, 0); + }); + + test('T47 trailingDashIsNotShortFormIndexed: a canonical id ending in a dash (D5 boundary) is excluded', () => { + // extractCanonicalPlanId('09-') === '09-' (the trailing empty segment is + // filtered out of `parts`, leaving no second token to pair with), so + // canonical.lastIndexOf('-') === canonical.length - 1 here — the other + // D5 boundary this rung's `lastDash < canonical.length - 1` guard must + // exclude. + const { shortFormToId } = buildInputsWithShortForm([ + { id: '09-', dependsOn: [] }, + ]); + assert.strictEqual( + extractCanonicalPlanId('09-'), + '09-', + 'sanity: extractCanonicalPlanId must leave this id unpaired, so lastDash === canonical.length - 1', + ); + assert.strictEqual(shortFormToId.size, 0, 'a canonical id ending in a dash must never be added to the short-form index (D5)'); + }); + +}); diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index eb66e9b22..7e439701f 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -1343,6 +1343,149 @@ describe('#3885 (ADR-3473 §8.5): phase-plan-index names a dropped depends_on to }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #3897 rung 4 (ADR-3473 §8.9) — shortFormToId, the recovered third +// depends_on resolution tier (D3). Today `resolveDependencyId` +// (gsd-core/bin/lib/phase.cjs) has exactly two tiers — planMap (full id) and +// canonicalToId (canonical prefix, e.g. `24-01`) — so a bare plan-number +// short form (`depends_on: ["01"]`) resolves via NEITHER and is silently +// dropped: the dependent plan collapses to a DAG root (wave 1) instead of its +// declared wave. +// +// T43 is the CONSUMER-OUTPUT identity row (ADR-3180 Decision 4(b)): it +// asserts on `phase-plan-index`'s emitted `waves` map through the REAL CLI, +// never on `resolveDependencyId`/`computeDependencyLevels` in isolation — a +// unit assertion on the resolver alone would have passed throughout this +// defect's entire life, since nothing forces a unit test to reflect what the +// consumer (execute-phase.md, partial-wave.md) actually reads. +// ───────────────────────────────────────────────────────────────────────────── +describe('#3897 rung 4 (ADR-3473 §8.9): shortFormToId, the bare plan-number depends_on tier', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // T43 — RED today: the CONSUMER-OUTPUT identity row. 26-02 depends on the + // bare short form '01'; today that edge is dropped, so 26-02 collapses into + // wave 1 alongside 26-01 instead of its own, later wave. + test('T43 shortFormDependencyProducesRealWaves_3427: a bare plan-number depends_on produces REAL waves, not one collapsed wave 1', () => { + const phaseDir = path.join(tmpDir, '.planning', 'phases', '26-shortform'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync( + path.join(phaseDir, '26-01-auth-hardening-PLAN.md'), + '---\nwave: 1\nautonomous: true\ndepends_on: []\n---\nPlan A.\n', + ); + // Bare plan-number short form — NOT a full id, NOT a canonical prefix. + fs.writeFileSync( + path.join(phaseDir, '26-02-followup-PLAN.md'), + "---\nwave: 2\nautonomous: true\ndepends_on:\n - '01'\n---\nPlan B.\n", + ); + + const result = runGsdTools('phase-plan-index 26', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + const waves = output.waves; + + const wave01 = Object.keys(waves).find((w) => waves[w].some((id) => id.startsWith('26-01'))); + const wave02 = Object.keys(waves).find((w) => waves[w].some((id) => id.startsWith('26-02'))); + assert.ok(wave01 !== undefined, '26-01-auth-hardening should appear in waves'); + assert.ok(wave02 !== undefined, '26-02-followup should appear in waves'); + assert.ok( + Number(wave01) < Number(wave02), + `the bare short form '01' must resolve to 26-01-auth-hardening and place 26-02-followup in a LATER wave — today the edge is dropped and both plans collapse into the same wave (got wave01=${wave01}, wave02=${wave02})`, + ); + // Today's defect also manufactures a wave-mismatch warning from the + // dropped edge (declared wave: 2 but DAG places it in wave 1) — once the + // short form resolves, that warning must be gone too. + const warnings = output.warnings ?? []; + assert.ok( + !warnings.some((w) => /declared wave:/.test(w) && w.includes('26-02')), + `no manufactured wave-mismatch warning should remain once the short form resolves; got: ${JSON.stringify(warnings)}`, + ); + }); + + // T49 — the short form must resolve IN-PHASE ONLY. A plan '01' living in a + // DIFFERENT phase must never satisfy a same-named short-form dependency in + // this phase — resolution is scoped per phase-plan-index invocation, never + // global across the project. + // + // #3897 rung 4 (isolated correctness review, MINOR finding 5): the + // ORIGINAL version of this test gave the target phase its OWN plan '01' + // (27-01-real) alongside the decoy (99-01-decoy). That construction cannot + // actually falsify a globally-scoped map: sorted plan-file order always + // resolves '01' to whichever phase's own plan sorts first among ALL + // candidates, and phase 27 sorts before phase 99 either way — so a + // GLOBALLY-scoped shortFormToId would have produced the exact same + // wave01 < wave02 result this test asserted, passing for the wrong reason. + // Verified empirically (see the isolated review's probe): building + // shortFormToId from the phase-scoped rawPlans vs. from the UNION of both + // phases' rawPlans produces byte-identical `unresolved`/`level` results for + // the original fixture shape. + // + // Fixed per the reviewer's own working construction: the TARGET phase has + // NO plan of its own numbered '01' at all (only '27-05-followup'), and the + // decoy phase's plan IS numbered '01' (11-01-decoy). Correct (per-phase) + // behavior is that '01' does NOT resolve — the edge is dropped with the + // existing #3427 unresolved-token warning, and 27-05 collapses to wave 1. + // A globally-scoped map would instead let '01' resolve to 11-01-decoy, a + // node outside this phase's rawPlans — which computeDependencyLevels can + // never satisfy, so it manufactures a false depends_on CYCLE report + // instead of a clean wave assignment. This construction was confirmed, + // by direct unit probe against the real exported `buildShortFormToId` and + // `computeDependencyLevels`, to distinguish the correct from the buggy + // scoping — the ORIGINAL fixture shape above did not. + test('T49 shortFormDoesNotReachAcrossPhases: a phase with NO plan of its own numbered "01" must NOT resolve depends_on: ["01"] via a same-numbered plan in a different phase', () => { + // A decoy phase whose OWN plan is numbered '01' — the only '01' anywhere + // in the project is in THIS phase, not phase 27. + const decoyPhaseDir = path.join(tmpDir, '.planning', 'phases', '11-decoy'); + fs.mkdirSync(decoyPhaseDir, { recursive: true }); + fs.writeFileSync( + path.join(decoyPhaseDir, '11-01-decoy-PLAN.md'), + '---\nwave: 1\nautonomous: true\ndepends_on: []\n---\nDecoy plan in a different phase.\n', + ); + + // Target phase has NO plan of its own numbered '01' — only '05'. + const phaseDir = path.join(tmpDir, '.planning', 'phases', '27-inphase-only'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync( + path.join(phaseDir, '27-05-followup-PLAN.md'), + "---\nwave: 2\nautonomous: true\ndepends_on:\n - '01'\n---\nPlan with a dangling short-form dependency.\n", + ); + + const result = runGsdTools('phase-plan-index 27', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + + // The decoy phase's plan must never even appear in THIS phase's output. + assert.ok( + !output.plans.some((p) => p.id.startsWith('11-')), + 'a different phase\'s plan must never appear in this phase\'s plan-index output at all', + ); + + // Correct behavior: the token does not resolve in-phase, so the edge is + // DROPPED — the #3427 unresolved-token warning fires by name, and + // 27-05-followup collapses to wave 1 rather than being scheduled behind + // a cross-phase phantom edge (which, per the probe above, would instead + // surface as a manufactured depends_on cycle error, not a clean pass). + const warnings = output.warnings ?? []; + assert.ok( + warnings.some((w) => /does not resolve to any plan in this phase/.test(w) && w.includes('27-05')), + `expected an unresolved-token warning naming 27-05-followup's dangling '01' dependency; got: ${JSON.stringify(warnings)}`, + ); + const wave05 = Object.keys(output.waves).find((w) => output.waves[w].some((id) => id.startsWith('27-05'))); + assert.strictEqual( + wave05, + '1', + `27-05-followup must collapse to wave 1 (dropped edge, no valid in-phase '01') — the short form must NOT reach the decoy in phase 11; got wave ${wave05}`, + ); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // phase-plan-index — canonical XML format (template-aligned) // ───────────────────────────────────────────────────────────────────────────── diff --git a/tests/runtime-marker-resolution.test.cjs b/tests/runtime-marker-resolution.test.cjs new file mode 100644 index 000000000..ccb42a427 --- /dev/null +++ b/tests/runtime-marker-resolution.test.cjs @@ -0,0 +1,398 @@ +'use strict'; + +/** + * #3897 rung 2 — the per-install `.gsd-runtime` marker rung in the CANONICAL + * runtime resolver (`resolveRuntime`, src/runtime-slash.cts). + * + * MINED FROM PR #3382 (closed, unmerged — "Closing unmerged — not on the + * merits."): that PR's `tests/runtime-marker-resolution.test.cjs` drove real + * `bin/install.js` installs end-to-end (6 tests). This file reuses its + * fixture/seam CONTRACT — `readInstallRuntimeMarker`, `_setInstallRuntimeMarkerForTests`, + * `_resetInstallRuntimeMarkerCacheForTests` exported from the canonical owner + * `runtime-slash.cts`, the same `neutralProject` shape (a `.planning/config.json` + * with NO `runtime` key, matching what `config-new-project` actually writes), + * and the "marker rung sits between config and the default" precedence — but + * exercises it at the resolver-unit level (in-process, monkeypatched `fs`) + * rather than through full spawned installs, per the test-matrix's rung-2 + * automation note ("unit on the resolution ladder + the cache; integration + * for the hooks' delegation"). + * + * Today (before #3897 rung 2 lands) `src/runtime-slash.cts` exports no marker + * reader and no test seams at all — `resolveRuntime` is a one-liner over + * `resolveExplicitRuntime(...) ?? 'claude'`. Every test below therefore fails + * at `assertMarkerSeamExists` first, which is the correct RED: the rung does + * not exist yet. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { createTempProject, cleanup } = require('./helpers.cjs'); + +const RUNTIME_SLASH_PATH = path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'runtime-slash.cjs'); +const RUNTIME_NAME_POLICY_PATH = path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'runtime-name-policy.cjs'); +const AGENT_ISOLATION_GUARD_PATH = path.join(__dirname, '..', 'hooks', 'gsd-agent-isolation-guard.js'); +const CURSOR_SUBAGENT_START_PATH = path.join(__dirname, '..', 'hooks', 'gsd-cursor-subagent-start.js'); + +/** A project whose config carries NO `runtime` key — what config-new-project writes. */ +function neutralProject(t) { + const dir = createTempProject('gsd-marker-proj-'); + t.after(() => cleanup(dir)); + fs.writeFileSync(path.join(dir, '.planning', 'config.json'), '{}'); + return dir; +} + +/** Fresh require of the built lib, bypassing require() cache so each test sees current disk state. */ +function requireRuntimeSlash() { + delete require.cache[require.resolve(RUNTIME_SLASH_PATH)]; + return require(RUNTIME_SLASH_PATH); +} + +/** + * Guard clause every test opens with: the marker rung's seam surface must + * exist on the CANONICAL owner before any scenario can be exercised. On the + * current tree this assertion itself is the RED — `runtime-slash.cjs` has no + * marker reader and no test seams. + */ +function assertMarkerSeamExists(rs) { + assert.equal( + typeof rs.readInstallRuntimeMarker, + 'function', + 'runtime-slash.cjs must export readInstallRuntimeMarker — the canonical marker reader (#3897 rung 2) does not exist yet', + ); + assert.equal( + typeof rs._setInstallRuntimeMarkerForTests, + 'function', + 'runtime-slash.cjs must export _setInstallRuntimeMarkerForTests — the test seam for the marker rung does not exist yet', + ); + assert.equal( + typeof rs._resetInstallRuntimeMarkerCacheForTests, + 'function', + 'runtime-slash.cjs must export _resetInstallRuntimeMarkerCacheForTests', + ); +} + +/** + * Run `fn` with `overrides` applied to process.env, restoring exactly what + * was there before. An override value of `undefined` DELETES that key for + * the duration of `fn` (never `Object.assign`s it — assigning `undefined` + * onto `process.env` coerces to the literal string `"undefined"`, not a + * deletion) — #3897 rung 4 (isolated correctness review, NIT finding 8b): + * every `delete process.env.GSD_RUNTIME` in this file used to bypass this + * helper entirely and never restore the ambient value afterward, leaking a + * forced-unset GSD_RUNTIME into whatever test (in this file or a later one + * sharing the same process) ran next. Every test below now routes through + * this helper, `undefined`-keyed, so ambient env state is always restored. + */ +function withEnv(overrides, fn) { + const saved = {}; + for (const key of Object.keys(overrides)) saved[key] = process.env[key]; + for (const key of Object.keys(overrides)) { + const value = overrides[key]; + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + try { + return fn(); + } finally { + for (const key of Object.keys(overrides)) { + if (saved[key] === undefined) delete process.env[key]; + else process.env[key] = saved[key]; + } + } +} + +describe('per-install .gsd-runtime marker rung in the canonical resolver (#3897 rung 2)', () => { + test('T3 installMarkerResolvesWhenEnvAndConfigAbsent_3897', (t) => { + const rs = requireRuntimeSlash(); + assertMarkerSeamExists(rs); + const proj = neutralProject(t); + rs._setInstallRuntimeMarkerForTests('codex'); + t.after(() => rs._resetInstallRuntimeMarkerCacheForTests()); + + withEnv({ GSD_RUNTIME: undefined }, () => { + assert.equal( + rs.resolveRuntime(proj), + 'codex', + 'no env, no config.runtime, marker says codex (R3) — resolveRuntime must consult the marker instead of falling straight to claude', + ); + }); + }); + + test('T1 envRuntimeOutranksConfigAndMarker', (t) => { + const rs = requireRuntimeSlash(); + assertMarkerSeamExists(rs); + const proj = neutralProject(t); + fs.writeFileSync(path.join(proj, '.planning', 'config.json'), JSON.stringify({ runtime: 'cursor' })); + rs._setInstallRuntimeMarkerForTests('gemini'); + t.after(() => rs._resetInstallRuntimeMarkerCacheForTests()); + + withEnv({ GSD_RUNTIME: 'codex' }, () => { + assert.equal(rs.resolveRuntime(proj), 'codex', 'GSD_RUNTIME must outrank both config.runtime and the install marker'); + }); + }); + + test('T2 configOutranksInstallMarker', (t) => { + const rs = requireRuntimeSlash(); + assertMarkerSeamExists(rs); + const proj = neutralProject(t); + fs.writeFileSync(path.join(proj, '.planning', 'config.json'), JSON.stringify({ runtime: 'cursor' })); + rs._setInstallRuntimeMarkerForTests('codex'); + t.after(() => rs._resetInstallRuntimeMarkerCacheForTests()); + + withEnv({ GSD_RUNTIME: undefined }, () => { + assert.equal(rs.resolveRuntime(proj), 'cursor', 'config.runtime must outrank the install marker'); + }); + }); + + test('T4 noSignalsFallsBackToClaude', (t) => { + const rs = requireRuntimeSlash(); + assertMarkerSeamExists(rs); + const proj = neutralProject(t); + rs._setInstallRuntimeMarkerForTests(null); + t.after(() => rs._resetInstallRuntimeMarkerCacheForTests()); + + withEnv({ GSD_RUNTIME: undefined }, () => { + assert.equal(rs.resolveRuntime(proj), 'claude', 'with no env, no config.runtime and no marker, the default is unchanged'); + }); + }); + + test('T5 emptyMarkerFallsThrough', (t) => { + const rs = requireRuntimeSlash(); + assertMarkerSeamExists(rs); + rs._resetInstallRuntimeMarkerCacheForTests(); + const originalReadFileSync = fs.readFileSync; + fs.readFileSync = (p, enc) => { + if (typeof p === 'string' && p.endsWith('.gsd-runtime')) return ' \n\t '; + return originalReadFileSync(p, enc); + }; + t.after(() => { + fs.readFileSync = originalReadFileSync; + rs._resetInstallRuntimeMarkerCacheForTests(); + }); + + assert.equal( + rs.readInstallRuntimeMarker(), + null, + 'a whitespace-only marker file must resolve to null (trimmed to empty), not an empty-string runtime name', + ); + + const proj = neutralProject(t); + withEnv({ GSD_RUNTIME: undefined }, () => { + assert.equal(rs.resolveRuntime(proj), 'claude', 'an empty marker must fall through to claude'); + }); + }); + + test('T6 unreadableMarkerNeverThrows', (t) => { + const rs = requireRuntimeSlash(); + assertMarkerSeamExists(rs); + rs._resetInstallRuntimeMarkerCacheForTests(); + const originalReadFileSync = fs.readFileSync; + // IO failure injected by monkeypatching fs, never chmod (root bypasses + // mode bits — a chmod test would pass with zero coverage in root CI). + fs.readFileSync = (p, enc) => { + if (typeof p === 'string' && p.endsWith('.gsd-runtime')) { + const err = new Error('EACCES: permission denied, open .gsd-runtime'); + err.code = 'EACCES'; + throw err; + } + return originalReadFileSync(p, enc); + }; + t.after(() => { + fs.readFileSync = originalReadFileSync; + rs._resetInstallRuntimeMarkerCacheForTests(); + }); + + assert.doesNotThrow( + () => rs.readInstallRuntimeMarker(), + 'an EACCES marker read must never throw out of readInstallRuntimeMarker — a hook that throws breaks the session (N4)', + ); + assert.equal(rs.readInstallRuntimeMarker(), null); + + const proj = neutralProject(t); + let result; + withEnv({ GSD_RUNTIME: undefined }, () => { + assert.doesNotThrow(() => { + result = rs.resolveRuntime(proj); + }); + }); + assert.equal(result, 'claude', 'an unreadable marker must fall through to claude, never crash the caller'); + }); + + test('T7 unknownMarkerNameIsNormalizedNotTrusted', (t) => { + const rs = requireRuntimeSlash(); + assertMarkerSeamExists(rs); + const proj = neutralProject(t); + rs._setInstallRuntimeMarkerForTests('NotARealRuntime-V9'); + t.after(() => rs._resetInstallRuntimeMarkerCacheForTests()); + + const viaMarker = withEnv({ GSD_RUNTIME: undefined }, () => rs.resolveRuntime(proj)); + const viaEnv = withEnv({ GSD_RUNTIME: 'NotARealRuntime-V9' }, () => rs.resolveRuntime(neutralProject(t))); + + assert.equal( + viaMarker, + viaEnv, + 'the marker rung must apply the SAME resolveRuntimeNameFromCandidates normalization GSD_RUNTIME does (N1) — one name policy, not two', + ); + assert.notEqual( + viaMarker, + 'NotARealRuntime-V9', + 'the raw marker string must never be returned verbatim — it must be routed through normalization like the env rung', + ); + }); + + test('T8 hostileMarkerContentIsInert', (t) => { + const rs = requireRuntimeSlash(); + assertMarkerSeamExists(rs); + rs._resetInstallRuntimeMarkerCacheForTests(); + const hostile = 'codex\n../../etc/passwd'; + const seenPaths = []; + const originalReadFileSync = fs.readFileSync; + fs.readFileSync = (p, enc) => { + seenPaths.push(String(p)); + if (typeof p === 'string' && p.endsWith('.gsd-runtime')) return hostile; + return originalReadFileSync(p, enc); + }; + t.after(() => { + fs.readFileSync = originalReadFileSync; + rs._resetInstallRuntimeMarkerCacheForTests(); + }); + + const proj = neutralProject(t); + let result; + withEnv({ GSD_RUNTIME: undefined }, () => { + assert.doesNotThrow(() => { + result = rs.resolveRuntime(proj); + }, 'hostile marker content (newline, path traversal, control char) must never throw'); + }); + assert.equal(typeof result, 'string'); + + for (const seen of seenPaths) { + assert.ok( + !seen.includes('etc/passwd') && !seen.includes(''), + `the hostile marker VALUE must never be used to construct a filesystem path to read — saw a read for ${JSON.stringify(seen)}`, + ); + } + + // Compare against `resolveExplicitRuntime`'s own dependency-injected `env` + // parameter (already exported for exactly this purpose) rather than + // mutating the real `process.env.GSD_RUNTIME` via `withEnv` + a second + // full `resolveRuntime()` round-trip. Both mechanisms exercise the + // identical `resolveRuntimeNameFromCandidates` normalization N1 requires + // (verified: both return "codex-../../etc/passwd" for this exact hostile + // input) -- but the live-env-mutation form depends on ambient + // `process.env` state around the call being pristine, which this suite + // cannot fully guarantee (this repo has a known class of ambient-`GSD_*`- + // env test-hermeticity bugs elsewhere). Asserting directly against the + // injectable primitive makes this comparison hermetic: no real process + // state is touched, and the assertion is unconditionally reproducible. + const viaEnv = rs.resolveExplicitRuntime(neutralProject(t), { GSD_RUNTIME: hostile }); + assert.equal( + result, + viaEnv, + 'hostile marker content must normalize identically to the same string arriving via GSD_RUNTIME — no special-cased bypass for the marker rung', + ); + }); + + test('T9 markerIsReadOncePerProcess', (t) => { + const rs = requireRuntimeSlash(); + assertMarkerSeamExists(rs); + rs._resetInstallRuntimeMarkerCacheForTests(); + let reads = 0; + const originalReadFileSync = fs.readFileSync; + fs.readFileSync = (p, enc) => { + if (typeof p === 'string' && p.endsWith('.gsd-runtime')) { + reads++; + return 'codex'; + } + return originalReadFileSync(p, enc); + }; + t.after(() => { + fs.readFileSync = originalReadFileSync; + rs._resetInstallRuntimeMarkerCacheForTests(); + }); + + const proj = neutralProject(t); + withEnv({ GSD_RUNTIME: undefined }, () => { + rs.resolveRuntime(proj); + rs.resolveRuntime(proj); + rs.resolveRuntime(proj); + }); + + assert.equal(reads, 1, 'the marker file must be read at most once per process — later calls must hit the cache'); + }); + + test('T10 cacheSeamResetsBetweenTests', (t) => { + const rs = requireRuntimeSlash(); + assertMarkerSeamExists(rs); + rs._resetInstallRuntimeMarkerCacheForTests(); + let reads = 0; + const originalReadFileSync = fs.readFileSync; + fs.readFileSync = (p, enc) => { + if (typeof p === 'string' && p.endsWith('.gsd-runtime')) { + reads++; + return 'codex'; + } + return originalReadFileSync(p, enc); + }; + t.after(() => { + fs.readFileSync = originalReadFileSync; + rs._resetInstallRuntimeMarkerCacheForTests(); + }); + + const proj = neutralProject(t); + withEnv({ GSD_RUNTIME: undefined }, () => { + rs.resolveRuntime(proj); + assert.equal(reads, 1); + + rs._resetInstallRuntimeMarkerCacheForTests(); + rs.resolveRuntime(proj); + assert.equal(reads, 2, 'resetting the cache seam must force a fresh read on the next call'); + }); + }); + + test('T12 hooksDelegateToTheSingleOwner', (t) => { + const rs = requireRuntimeSlash(); + assertMarkerSeamExists(rs); + rs._resetInstallRuntimeMarkerCacheForTests(); + t.after(() => rs._resetInstallRuntimeMarkerCacheForTests()); + + delete require.cache[require.resolve(AGENT_ISOLATION_GUARD_PATH)]; + delete require.cache[require.resolve(CURSOR_SUBAGENT_START_PATH)]; + const guard = require(AGENT_ISOLATION_GUARD_PATH); + const cursorHook = require(CURSOR_SUBAGENT_START_PATH); + const { resolveRuntimeNameFromCandidates } = require(RUNTIME_NAME_POLICY_PATH); + + const proj = neutralProject(t); + const configPath = path.join(proj, '.planning', 'config.json'); + + withEnv({ GSD_RUNTIME: undefined }, () => { + // Set the CANONICAL seam only — no disk write. A hook that still owns + // its own private reader/cache (today's state, #3566) will never + // observe this in-memory value; it will fall through to its own disk + // read or the ~/.gsd/defaults.json rung instead. + rs._setInstallRuntimeMarkerForTests('gemini'); + const identity = guard.resolveRuntimeIdentity(proj, configPath, resolveRuntimeNameFromCandidates); + assert.equal( + identity.runtimeId, + 'gemini', + 'hooks/gsd-agent-isolation-guard.js must resolve the marker through the single canonical owner (runtime-slash.cts), not its own private reader/cache', + ); + assert.equal(identity.confident, true); + + // Cursor hook's surface has no direct runtimeId getter — observe + // delegation through its isolation verdict instead. 'claude' is a + // known registry entry whose dispatch.isolation is 'harness-worktree' + // (capability-registry.cjs); a hook still reading its own (empty, + // dev-tree) disk marker would answer as if no marker existed at all. + rs._setInstallRuntimeMarkerForTests('claude'); + const isolation = cursorHook.resolveFallbackIsolation(proj, configPath); + assert.equal( + isolation, + 'harness-worktree', + 'hooks/gsd-cursor-subagent-start.js must resolve the marker through the single canonical owner too', + ); + }); + }); +});