enhance(#3897): ADR-3473 §8.3 rungs 2-4 — runtime marker, derived Codex sandbox, short-form depends_on (#3941)
* test(#3897): failing-first coverage for §8.3 rungs 2-4 ADR-3473 §8.3 has four rungs; #3883/PR #3896 shipped the first. This pins the other three RED before any fix. Rung 2 — the install marker has four readers and resolveRuntime is not one. resolveRuntime resolves GSD_RUNTIME > config.runtime > 'claude' and reads no marker at all, while bin/install.js writes one (#2297) and FOUR hand-rolled readInstallRuntimeMarker copies exist: src/model-resolver.cts:65 (cached, with test seams), hooks/gsd-agent-isolation-guard.js:112, and TWICE in hooks/gsd-cursor-subagent-start.js at :346 and :355. Four copies of one rule. Fixtures and seam names mined from PR #3382 rather than re-derived; it implemented this rung and was closed "not on the merits". Rung 3 — the sandbox map, and the fallback that was the real defect. Measured across all 35 files in agents/, deriving workspace-write iff tools: declares Write or Edit: - all 11 CODEX_AGENT_SANDBOX entries derive to their mapped value exactly, zero disagreements — the map carries nothing the contract does not - 24 roles fall through `|| 'read-only'`, of which 16 declare Write or Edit So the map is redundant and the silent fallback is the defect. The maintainer chose to derive but hold those 16 at read-only pending the question of whether Codex enforces sandbox_mode or merely advises; HALT.md records it. T20 asserts the emitted sandbox_mode PER ROLE against a captured baseline, not in aggregate — an aggregate passes while one role silently widens, which is the proxy-instead-of-identity shape this repo names. T24 and T25 fail on a stale hold, so the hold list cannot rot into the subset map being deleted. Rung 4 — shortFormToId, recovered rather than invented. I nearly reported this as another wrong §8.3 claim: `git log -S shortFormToId` returns only documentation commits. That was the wrong instrument. Direct inspection of sdk/src/query/phase.ts at 11918dcc3^ shows five occurrences, and the tests match that code rather than a guess at its semantics — including first-write-wins on a duplicate short form. T43 asserts at the consumer's output: the emitted `waves` map from the real CLI, which pre-fix collapses to {"1":[...]} because every short-form edge is dropped. A unit assertion on resolveDependencyId would have passed throughout this defect's life. Observed RED, this tree: rung 2 11/11 fail — no marker rung, no seams rung 3 T23,T24,T25,T26,T30 fail; T28 fails (validate agents passes a TOML whose sandbox_mode disagrees — it checks presence only) rung 4 T42,T44 fail; T43,T49 fail with waves collapsed to a single wave 1 Green and staying green: T20/T21/T22/T27 as captured baselines, #3885's unresolvable-token warning and wave-verdict suppression, and #3785's display-mapping passthrough. If the third tier over-reaches, those go red — that is their job. Disclosed weakness: T45 (a canonical id with no dash is not short-form indexed) cannot be isolated behaviorally, because planMap always masks it. It is a non-crash boundary pin, weaker than the other rows, and is recorded as such rather than presented as equivalent. Design: .gsd/phase/feat-3897-adr3473-83-rungs/40-design.md Test matrix: .gsd/phase/feat-3897-adr3473-83-rungs/50-test-matrix.md Decision: .gsd/phase/feat-3897-adr3473-83-rungs/HALT.md Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enhance(#3897): §8.3 rungs 2-4 — one marker reader, a derived sandbox, the third depends_on tier ADR-3473 §8.3 has four rungs. #3883/PR #3896 shipped the first. These are the other three. Rung 2 — the install marker had four readers, and resolveRuntime was not one. resolveRuntime resolved GSD_RUNTIME > config.runtime > 'claude' and read no marker, while bin/install.js writes one (#2297) and four hand-rolled readInstallRuntimeMarker copies existed: src/model-resolver.cts (cached, with seams), hooks/gsd-agent-isolation-guard.js, and twice in hooks/gsd-cursor-subagent-start.js. model-resolver's was already the house idiom, so it was promoted rather than replaced: src/runtime-slash.cts now owns it, and model-resolver plus both hooks delegate. The hooks reach it through ensureRuntimeBuild(), the seam lint-hooks-runtime-build-seam enforces. No import cycle existed - checked both directions before moving anything. The marker is the THIRD rung: env > project config > marker > 'claude'. N1 was checked rather than assumed, and my first reading of it was wrong. A marker holding an unknown name comes back essentially verbatim, which looked like a validation gap. Measured against the env rung with the same inputs - including "../../etc/passwd" and "claude;rm -rf /" - the two are identical, because they share resolveRuntimeNameFromCandidates. N1 asks for exactly that, and it is met. The residual (the shared normalizer normalizes shape, it does not validate against the known-runtime set) is pre-existing on the env rung and plausibly deliberate, since a new runtime should not need a code change. The marker also does not widen the trust boundary in any real sense: it lives inside the install tree beside the code, so anyone who can write it can write runtime-slash.cjs itself. Rung 3 — the map was redundant; the silent fallback was the defect. Measured across all 35 files in agents/, deriving workspace-write iff tools: declares Write or Edit: all 11 CODEX_AGENT_SANDBOX entries derive to their mapped value exactly, zero disagreements. The map carried nothing the contract did not already have, so it is DELETED rather than reconciled. What was actually broken is `|| 'read-only'`, which silently under-granted 24 of 35 roles. 16 of those 24 declare Write or Edit and would widen under derivation. Per the maintainer's decision (HALT.md), they are held at read-only pending the question of whether Codex enforces sandbox_mode or merely advises. Emitted TOML is therefore byte-identical for all 35 roles - asserted per role, not in aggregate, because an aggregate passes while one role silently widens. The hold list self-invalidates. A hold whose role no longer derives broader fails, and so does a hold naming a role with no agents/<name>.md. Without that it would rot into exactly the hand-maintained subset map being deleted, and this commit's own ledger claim would become false over time. Both cases were proved by injecting them and watching them throw. Two committed tests asserted the deleted map's existence and contents. They were pinning the thing being removed, so the tests moved rather than the production code: the 11 role-value pairs survive as a test-local PRE_3897_CODEX_AGENT_SANDBOX baseline, and the assertions now drive the real derivation against real agents/*.md. The coverage is preserved; only its source moved out of production code. validate agents gains checkCodexSandboxPosture, mirroring the existing checkCodexModelPosture: each installed TOML's sandbox_mode must equal the role's expected value, failing with role, expected and found. It previously checked file presence and manifest completeness only, so a TOML whose sandbox_mode disagreed passed. Rung 4 — shortFormToId, recovered rather than invented. I nearly reported this as another wrong §8.3 claim: git log -S returns only documentation commits. Wrong instrument. sdk/src/query/phase.ts at 11918dcc3^ carries five occurrences, and the implementation here matches that code rather than a guess at its semantics - including first-write-wins on a duplicate short form, deterministic from the sorted plan order. It resolves the bare plan number: depends_on: ["01"] now reaches 26-01-auth-hardening. That is a control-flow change, not a diagnostic one - plans that silently collapsed into a single wave 1 now execute in their declared waves, and execute-phase.md consumes those wave values. In-phase only, by construction: the map is built from this phase's rawPlans, so a same-named short form in another phase does not resolve. #3785's display-mapping passthrough and #3885's unresolvable-token warning and wave-verdict suppression are untouched and stay green. If the third tier had over-reached, those are what would have caught it. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): close a fail-open I introduced, and wire the posture check to its command Two blockers from review. Both are mine, and one is a security regression my own change created. 1. A held role could escape its hold by editing its own frontmatter. The Codex install loop set the sandbox identity from the agent's frontmatter `name:` field rather than from its filename, so the hold lookup keyed off a value the file itself declares: deriveCodexSandboxMode('gsd-doc-writer', <real file>) -> read-only deriveCodexSandboxMode('gsd-doc-writer-x', <same file, name: edited>) -> workspace-write deriveCodexSandboxMode('GSD-Doc-Writer', <same file, name: recased>) -> workspace-write What makes this a blocker rather than a nit is the DIRECTION. The deleted CODEX_AGENT_SANDBOX map had the identical lookup-key quirk, but it was an allowlist: an unmatched key fell back to read-only, which is safe. The new scheme derives workspace-write from the tool contract and uses the hold as a subtraction, so the same mismatch fails OPEN. I converted a fail-closed quirk into a fail-open one and did not notice; the isolated reviewer proved it by execution. Neither safety net caught it. validateCodexSandboxHolds only checks that <key>.md exists, never that a file's derived identity matches its key. checkCodexSandboxPosture looks the canonical source up by the installed TOML's filename, finds nothing for a renamed agent, and treats it as a custom non-roster agent — silently no violation. The identity is now the FILENAME STEM, which is what validateCodexSandboxHolds already validates and what an attacker editing frontmatter cannot change without renaming the file — at which point the existing validator catches it. The lookup is case-insensitive so a recase does not slip past either. The frontmatter name still drives the TOML body and filename, unchanged; only the sandbox identity moved. All 35 roster files were checked: name matches filename stem everywhere, so a stricter "they must agree or throw" invariant would have been safe against real content. It is deliberately NOT added — it would abort an install on a tampered file where emitting a correctly-derived read-only TOML is the safer outcome. Recorded as a fork rather than decided silently. 2. checkCodexSandboxPosture was exported and never called. cmdValidateAgents (src/verify.cts) called checkAgentsInstalled and checkCodexModelPosture only; grep for the sandbox check in that file returned nothing. So criterion 3 — "validate agents fails on semantic drift, not only on missing files" — was unmet, and `validate agents` behaved exactly as before. That is ADR-3473 Decision 2's named shape: a declared policy with no executor. It also meant the T28 test asserted at the helper's return value while the COMMAND stayed broken — the ADR-3180 Decision 4(b) failure this epic exists to close, committed by me while enforcing it elsewhere in the same epic. Now wired as an additive `sandbox_posture` field beside `codex_posture`, following the sibling precedent exactly. Drift is report-only, not a non-zero exit, because that is what checkCodexModelPosture does — two sibling posture checks disagreeing about whether a violation is fatal would be its own defect. The choice is recorded in a comment rather than left implicit. A consumer-output test now drives the real CLI and asserts on the emitted JSON, and was shown failing before the wiring and passing after. Also corrected a stale artifact: the design's Known limit L1 still claimed rung 3 was not in this deliverable, written while it was halted and false once the maintainer unblocked it. Verified after both fixes: the three bypass probes all return read-only, the per-role table is 35/35 byte-identical, and both hold self-invalidation cases still throw. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3897): the marker rung, the derived sandbox, and the bare plan-number depends_on Reference: the runtime precedence ladder in docs/CLI-TOOLS.md gains the install marker rung; docs/COMMANDS.md documents validate agents' new sandbox_posture field; docs/reference/plan-md.md documents that depends_on accepts the bare plan number. Explanation: a docs/features fragment keyed id 3897, so it cannot collide with a concurrent PR hand-allocating a section number, regenerated into FEATURES.md. ADR-3473 §8.3 gains an ANSWER blockquote in the document's own correction style, recording what was measured and built against the section's 2026-08-26 correction - including the qualification that checkAgentsInstalled itself still checks presence only, and the semantic assertion lives in a sibling wired into validate agents rather than folded into it. No how-to. Both user-visible changes are zero-step: a non-Claude install resolving its own runtime, and plans executing in their declared waves, both happen without the user doing anything. docs/how-to/control-the-reported-host-runtime.md covers a DIFFERENT ladder (resolveReportedRuntime / agent_runtime) that this change does not touch, and was deliberately left alone rather than edited by association. No tutorial - nothing multi-step to walk through. docs/AGENTS.md unchanged: it documents Claude-side tools frontmatter, never Codex sandbox_mode, and the emitted tools contract did not change. The prompt layer documents depends_on only by example, not by schema, so nothing there needed editing - and few-shot-examples/plan-checker.md already showed depends_on: ['01'], which now actually resolves. Translated copies of plan-md.md are untouched; the project treats translations as community-maintained. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): move the sandbox derivation out of the installer, off the install path, and off a third parser The full suite came back with 26 failures across four files. Three distinct causes, mapped individually rather than assuming the first explained the rest. A. Requiring bin/install.js printed the GSD banner to stdout and corrupted `validate agents` JSON. Unexpected token '', "[36m ██"... is not valid JSON checkCodexSandboxPosture reached deriveCodexSandboxMode by lazily requiring bin/install.js, whose module load prints the ASCII banner. So the command emitted banner bytes before its JSON and every JSON consumer broke, including ten tests that predate this branch. src/ reaching into bin/ was backwards layering that happened to also be loud. The derivation now lives in src/codex-agent-toml.cts - the existing Codex TOML domain module, no new module and no six-gate ripple - and both bin/install.js and src/agent-install-check.cts import it. One owner, which is §8.3's rule applied to the fix for §8.3. B. The stale-hold throw fired on a legitimate partial source dir, and masked a security assertion. validateCodexSandboxHolds treated "this hold's .md is absent from the install SOURCE dir" as a stale hold and threw. A test fixture, or any partial install source, legitimately contains a couple of agents. Worse, it threw BEFORE the path-escape check, so a test asserting that a `../../evil` frontmatter name is rejected got my unrelated error instead of the traversal rejection it was written for. A fail-closed check of mine was hiding a real security check. The "no stale holds, shrink-only" invariant is a property of the repo's canonical agents/ roster, not of whatever directory an install happens to read. It is off the runtime path and enforced where it belongs, in the tests that already existed for it. A partial source dir now installs cleanly, and the evil-name case throws with its own escapes-configHome message again. C. T8 depended on ambient process.env state. The marker/env parity assertion round-tripped through live process.env. It now compares against resolveExplicitRuntime's already-exported dependency-injection parameter - deterministic and hermetic, same claim. Proven still falsifiable rather than assumed: with the marker rung's normalization temporarily bypassed the two rungs diverge ("codex\n../../etc/passwd" vs "codex-../../etc/passwd") and the assertion fails, then passes again once reverted. One correction folded in along the way. The first version of the move added private _extractFrontmatterAndBody/_extractFrontmatterField helpers to codex-agent-toml.cts - a THIRD copy of frontmatter extraction, where the graph already shows two (bin/install.js:2348, runtime-artifact-conversion.cts:893). Adding a third inside the epic whose thesis is one implementation per rule is not defensible. deriveCodexSandboxMode no longer parses anything: it takes (identity, toolsValue) and each caller supplies the tools value using the extractor it already has. Both helpers are deleted. The identity argument is still the filename stem, so the fail-open fix is untouched. Verified after all three: `validate agents --raw` emits parseable JSON with no banner and both posture fields; the four hold-bypass probes still return read-only; the per-role table is 35/35 byte-identical at 26 read-only / 9 workspace-write; the hold list is still 16. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): drop a dev-only transitive dep, make the derivation total, retire a stale fallback test Suite down to 7 failures from 26. Three more causes, mapped individually. A. My extractor import dragged in a script that does not exist in an installed tree. Cannot find module '../../../scripts/fix-slash-commands.cjs' Chain: src/agent-install-check.cts imported runtime-artifact-conversion.cjs, which requires command-roster.cjs, whose line 36 requires ../../../scripts/fix-slash-commands.cjs. That path exists in the repo and not in an install, so every test exercising a synthetic install dir died at module load. I picked that extractor for convenience without checking what it pulls in - the same mistake that produced the banner bug, one layer further out. agent-install-check now uses a single-purpose extractToolsLine on codex-agent-toml.cts. That is deliberately NOT a general frontmatter parser: we deleted those helpers a commit ago for good reason, and this reads one line. Verified from outside the repo root that requiring either module prints nothing and does not throw. B. A test pinned the deleted name-based fallback. 'defaults unknown agents to read-only' called generateCodexAgentToml with a fixture declaring tools: Read, Write, Edit. Under derivation an unknown agent with a writing contract correctly derives workspace-write - design row S6, a new writing role gets the contract, not the pin. The behavior it asserted was the silent fallback this rung deleted; identity no longer decides the sandbox. Replaced with two rows rather than a flipped string: no tools declared -> read-only (absence is not a grant), and Write/Edit declared -> workspace-write. Strictly more coverage than the row it replaces. C. The stale-hold check still threw per derivation call. Last commit took the roster-existence check off the install path, but deriveCodexSandboxMode itself still threw when a hold's role did not derive broader FOR THE CONTENT IT WAS HANDED - so it fired on any synthetic fixture for a held role. The throw is gone, and it cost nothing: if a held role's content does not derive broader, the hold pins read-only and derivation returns read-only anyway, so the hold is a no-op and there is nothing to fail about. The staleness invariant is a property of the real agents/ roster, and validateCodexSandboxHolds still enforces it there - confirmed against the real roster after the change, not assumed. deriveCodexSandboxMode is now total: every (identity, toolsValue) including undefined and null returns read-only or workspace-write, never throws. Verified: validate agents emits parseable JSON; the four hold-bypass probes return read-only; the per-role table is 35/35 at 26 read-only / 9 workspace-write. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3897): put the rung-3 decision in the shipped docs instead of pointing at an ignored path The ADR entry and the feature fragment both ended their rung-3 explanation with "see .gsd/phase/feat-3897-adr3473-83-rungs/45-decision-rung3-sandbox.md". That directory is gitignored (.gitignore:55), so the rationale for holding 16 roles at read-only was reachable only from the machine that produced it. A reader of the ADR got a pointer to nothing. Both now carry the reasoning inline: the 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, because a faithful derivation widens 16 roles the deleted map never listed and that fell through its silent read-only default. The resolution is derive-and-hold - the derivation owns the rule now, each hold is released as its enforcement question is answered, and a hold is reversible where a widened sandbox that turns out to be enforced is not. Checked before assuming this was a defect class: CONTEXT.md cites .gsd/phase/<slug>/40-design.md as its standard Design: provenance line in eight module entries, and four other shipped docs do the same. Citing a phase artifact is an established convention here, so those are left alone. What was wrong was specific to these two: they put load-bearing rationale behind the pointer instead of provenance. docs/FEATURES.md regenerated from the fragment via scripts/gen-features.cjs rather than hand-edited. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): close a fail-open, stop a silent mis-resolution, and read a declaration as a declaration Two orthogonal reviews on the shipped sha. Three of the findings are the same failure class this epic exists to close, committed inside it. 1. BLOCKER - the sandbox was decided for one identity and applied to another. bin/install.js derived sandbox_mode for the filename stem and then wrote the result to `${name}.toml`, where name comes from the file's own frontmatter. Make the two disagree and a HELD role's artifact goes wide: rename gsd-doc-writer.md -> gsd-doc-writer-v2.md, keep name: gsd-doc-writer -> stem is unheld, derives workspace-write, lands on gsd-doc-writer.toml add any gsd-*.md whose frontmatter name: is a held role -> clobbers that role's toml with workspace-write Both emit read-only on origin/next, because the deleted map was an allowlist and a miss fell back safe. This is a regression my change introduced. The previous review round moved the HOLD KEY off frontmatter to the filename stem and left the OUTPUT PATH on frontmatter; my own comment at install.js:6985 calls that value attacker-editable, four lines above the line that uses it as the filename. The decision is now made over BOTH candidate identities, most-restrictive wins: if either the stem or the emitted name is held, the mode is read-only. 2. MAJOR - hold matching was toLowerCase() only, so confusables escaped. Turkish dotted/dotless i, fullwidth, NFD, trailing space/NBSP/dot/newline, ./ and ../agents/ all slipped the hold and emitted workspace-write. Identities are now basenamed, trimmed of NBSP/zero-width/control characters, NFKC-normalized and lowercased - and anything still carrying a character outside [a-z0-9._-] is treated as suspicious and derives read-only. We do not enumerate confusables; every shipped roster file is ASCII, so refusing to widen on an identity we cannot recognize is fail-closed with no false positives on real content. 3. MAJOR - the short-form depends_on tier mis-resolved SILENTLY. shortFormToId keyed on the last dash-segment of any canonical id with no constraint that it is a plan number, so a phase holding 09-FIX-auth-PLAN.md made depends_on: ["auth"] bind at wave 2 with zero warnings. This is the worst shape in the epic: the unresolvable-token warning fires on a DROPPED token, so a MIS-RESOLVED one is invisible and the tool reports a confident wave assignment built from a wrong edge. A wrong edge is worse than a missing one. The segment must now match /^\d+$/, which is exactly the contract docs/reference/plan-md.md already documents. This tier was recovered verbatim from the retired SDK lineage, which carried the same defect; we are deliberately NOT preserving it bug-for-bug, and the comment says so, so the next reader does not "restore" it. 4. MAJOR - the derivation was reading a declaration as an absence. extractToolsLine read one line, so a YAML list-form tools: block returned only its first item. Two roster files use list form, and gsd-nyquist-auditor declares Write and Edit there - parsed as "- Read", found no write tool, and emitted read-only. Rung 3's headline claim is that sandbox_mode derives from the declared tool contract; that claim was false for 2 of 35 roles and materially wrong for 1. Reading a declaration as an absence is the silent-drop class this epic exists to close. Renamed extractToolsValue and taught it both shapes. gsd-nyquist-auditor now derives workspace-write and joins CODEX_SANDBOX_HOLDS as its 17th entry, per the standing derive-and-hold decision - so emitted TOML stays byte-identical at 26 read-only / 9 workspace-write while the hold list finally records every role that would widen. A previous pass declined this fix because it moved the count; that inverts the priority. Byte-identity is preserved THROUGH the hold, not by leaving a parser broken. Divergence check, because this is where that bug hides: both paths feeding sandbox derivation - install.js's emitter and checkCodexSandboxPosture - now route through the one extractor. The tools readers in runtime-artifact-conversion and install.js's other frontmatter call sites serve Claude-side emission and do not feed sandbox derivation. Also fixed, each real: the posture check's `found` used a naive whole-file regex where its own sibling uses the block-aware scanner, so prose inside developer_instructions produced a false violation; `found` skipped truncatePostureValue and leaked a 300-char value into validate agents output; deriveCodexSandboxMode's absolute never-throws claim was false for an object with a throwing toString; T49 could not falsify cross-phase leakage (its target phase had its own 01, so a globally-scoped map passed too); T20/N6 iterated a hardcoded table and pinned the FIXTURE size, so a 36th agent would be silently unchecked; three tests reimplemented the code they were testing instead of importing it; and T2-T4 deleted GSD_RUNTIME without restoring it. Verified: hold list 17, gsd-nyquist-auditor derives workspace-write unheld and emits read-only held, roster 35/35 at 26/9, depends_on ["auth"] no longer resolves while ["01"] still does, both identity-bypass cases and every confusable vector emit read-only. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3897): the hold list is 17, and the reason the 17th was missing The count read 16 because the derivation could not read the declaration it claimed to derive from: the tools reader was single-line, so a YAML list-form tools: block returned only its first item and gsd-nyquist-auditor's declared Write and Edit were read as an absence. Both the ADR entry and the feature fragment now carry the corrected count and the reason for it, rather than a silently updated number. Deriving from a declaration you cannot parse is not deriving, and a flattering count is worse than a wrong one because it looks settled. docs/FEATURES.md regenerated from the fragment. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3897): backfill changeset pr number Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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 `<name>` 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,
|
||||
};
|
||||
|
||||
@@ -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<Record<string, string>> = 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 "<grant> except <exclusions>" 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<string | string[] | null | undefined>
|
||||
): { 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
|
||||
* `<agentsSrcDir>/<role>.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.'
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<string> = 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
|
||||
|
||||
@@ -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<string, RawPlan>,
|
||||
canonicalToId: Map<string, string>,
|
||||
shortFormToId?: Map<string, string>,
|
||||
): 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<string, string> {
|
||||
const shortFormToId = new Map<string, string>();
|
||||
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<string, RawPlan>,
|
||||
canonicalToId: Map<string, string>,
|
||||
shortFormToId?: Map<string, string>,
|
||||
): { level: Map<string, number>; visited: number; order: string[]; unresolved: Array<{ plan: string; token: string }> } {
|
||||
const level = new Map<string, number>();
|
||||
const inDeg = new Map<string, number>();
|
||||
const adj = new Map<string, string[]>();
|
||||
// #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,
|
||||
};
|
||||
|
||||
@@ -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
|
||||
// `<install>/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';
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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,
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user