From 4a1ed2531f1a84d8c8431c7f992f55782a5ba3a4 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 9 Aug 2026 21:50:49 -0400 Subject: [PATCH] enhance(#3242): validate codex .toml model posture, not just presence (#3290) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3242): failing-first suite for the codex posture health-check Specifies ADR-2313 D6 before the implementation exists, so the tests bind to the contract rather than to whatever the code happens to do. RED is established by construction, not by a remote run: checkCodexModelPosture and POSTURE_REASON are absent from the compiled lib today, so every row fails on the missing export. A remote checkpoint here would prove only that the function is missing, which is already known — so the run is deliberately deferred to the combined green checkpoint rather than spent proving a tautology. That makes the NEGATIVE PROOFS the rows that carry real signal. Every positive row passes even for a naive implementation that greps /model\s*=/ over the whole file. Six rows fail it: light-tier service_tier/model_verbosity decoupling (#774), hand-added keys, a commented pin, the model_verbosity key-prefix collision, the runtime no-op ordering, and the headline case — a literal `model = "sonnet"` inside the developer_instructions ''' block, which the emitter fills with agent prompts that discuss models constantly. Row 14's fixture was verified to discriminate before being written: a whole-file scan matches it and a header-slice scan does not. Without that check the test would pass trivially and prove nothing, which is the vacuous-test failure this epic has already hit repeatedly. Adversarial TOML fixtures are hand-authored against the real Codex shape rather than generated by generateCodexAgentToml, per #2371 — a fixture from the writer can only confirm what the writer already believed. Co-Authored-By: Claude Opus 5 * feat(#3242): validate codex .toml model posture, not just presence Implements ADR-2313 D6. checkCodexModelPosture is a new sibling export, not a branch inside checkAgentsInstalled — that function carries 33 upstream dependents, cyclomatic 25, and sits in two traced process flows, so it is deliberately left untouched. It imports isAnthropicFlavoredModel from model-catalog, a genuine leaf. That is what Phase 1's constant move bought: agent-install-check is documented as pure read/verify and imports only leaves, so reaching the rule through model-resolver would have dragged config-loader into it. Reads liberally, judges strictly, and never guesses. Tolerates comments, key order, whitespace, CRLF, and a BOM; anchors on full key names so model_verbosity does not satisfy a `model` probe; treats extra hand-added keys as none of its business, since the check is a predicate on the two fields the posture owns rather than a whitelist over the document. An unreadable file becomes a named violation and the loop keeps going. The scan covers only the header slice — the lines before the developer_instructions ''' marker. The emitter writes agent prompts into that block and GSD's prompts discuss models constantly, so a whole-file scan reports violations for prose. This is the highest-risk defect in the phase and the reason its fixture was verified to discriminate before being written. The non-codex short-circuit runs before any filesystem call, so a stray .toml under another runtime is never inspected. Wired through cmdValidateAgents as an additive codex_posture key, so a violating install is visible from a command a user actually runs rather than only from a library nothing calls. Also fixes a test defect found while implementing: .gitattributes forces `* text=auto eol=lf` repo-wide, so the committed CRLF fixture was normalized to LF in the index — `git ls-files --eol` reported `i/lf w/crlf`, the working copy being stale pre-normalization bytes. The CRLF row was asserting against a file that could not survive a fresh clone. CRLF is now derived at runtime, which puts it under the test's control rather than git's, instead of adding a .gitattributes exception that fights a deliberate repo-wide policy and that anyone could re-normalize. Co-Authored-By: Claude Opus 5 * docs(#3242): document the codex posture check where a user will look Three quadrants, filed by where the reader actually arrives. How-to (recover-and-troubleshoot.md, under Install and update problems) is titled by the SYMPTOM — "If Codex agents fail to spawn with a 400 about an unsupported model" — and opens with the verbatim error string. Someone hitting this does not know the words "posture" or "ADR-2313"; they have a 400 in their terminal and will search for that. Reference (COMMANDS.md) had no `validate agents` entry at all, though sibling gsd-tools subcommands are documented. Adding user-visible output to an undocumented command and then linking to it from the new how-to would have left a dangling reference. The entry carries the violation-reason table, since the frozen POSTURE_REASON enum is the machine-readable contract a reader needs rather than the prose. Both surfaces state that presence and posture are separate verdicts — a missing agent lands in `missing`, never as a posture violation. That is a deliberate design decision and would otherwise be invisible to someone watching one command emit both. Explanation stays in ADR-2313, which already covers D6 and the liberal-parse/strict-judge boundary. Pointing at it beats duplicating it into COMMANDS.md and creating two copies to drift. Co-Authored-By: Claude Opus 5 * fix(#3242): close two false negatives in the posture scan Both found by an isolated reviewer and reproduced before fixing. Both made the check report clean when it was not — the worst direction for this function, since the how-to tells users an empty violations list means the install is posture-clean. A quoted TOML key was never matched. `"model" = "sonnet"` is legal TOML, but the key pattern required a bare identifier, so the pin was silently invisible. Bare, "double" and 'single' quoted forms now normalize to the same key name. The block marker was found by unanchored whole-content search and used to truncate the header. A `description` value merely containing the literal text `developer_instructions = '''` truncated the scan before a real pin, and a user who hand-reordered `model` to sit after the block — still legal TOML — was never scanned at all. Fixed by changing the strategy rather than the regex: find the block's range, anchored at line start, and scan every line OUTSIDE it. That covers both failures and is strictly more correct than truncation, while still never reading prompt prose. An unterminated block excludes the rest of the file, which fails toward a false positive — the safe direction, since misreading prose as a pin wastes a user's time while the alternative hides a real one. Also corrects two overclaims of mine. The how-to named "v1.11", a version that does not exist — package.json is 1.10.0 and unreleased — so it now describes the boundary by behavior and links the ADR. And the test matrix asserted that a naive whole-file scan "fails exactly rows 12,13,14,15,16,25"; the reviewer computed that rows 12, 13, 15 and 16 produce the correct result against that baseline too. They guard real but *different* mistakes, and the matrix now says which one each catches instead of attributing them all to the header-slice defect. Co-Authored-By: Claude Opus 5 * fix(#3242): skip symlinked agent files instead of following them Security review finding. The scan listed entries with readdirSync and read them with readFileSync, which follows symlinks — so a symlink in the agents directory pointing anywhere would have its contents read, and any line matching the model pattern echoed into cmdValidateAgents' output through the `value` field. A read-and-echo primitive on an arbitrary path. It needs write access to the agents directory, so it crosses no new trust boundary today. Fixed anyway, for two reasons. This repo already does it correctly next door: cmdEffortSync filters with lstatSync().isFile() and the comment "Skip symlinks — only write regular files to avoid clobbering symlink targets." Being inconsistent with a sibling in the same subsystem IS the defect. And Phase 3 (#3243) extends that same cmdEffortSync to WRITE these files. Establishing symlink-following as the house pattern for Codex .toml handling here would hand Phase 3 a worse starting point while it writes rather than reads. Skipped silently rather than reported, matching the sibling: a symlinked agent file is a structural install choice, which checkAgentsInstalled owns, not a model-content posture defect. An lstat that itself throws excludes the file rather than crashing the scan. That does narrow the guarantee slightly, so the how-to now says an empty list means every REGULAR .toml is clean, and tells anyone symlinking their configs to check the targets by hand. Claiming a clean bill of health over files the check declined to open would be the same kind of false confidence the two false negatives above produced. Co-Authored-By: Claude Opus 5 * chore(#3242): backfill changeset pr number (#3290) --------- Co-authored-by: sim Co-authored-by: Claude Opus 5 --- .changeset/gentle-foxes-glide.md | 5 + docs/COMMANDS.md | 23 + docs/how-to/recover-and-troubleshoot.md | 47 ++ src/agent-install-check.cts | 247 ++++++ src/verify.cts | 10 +- tests/agent-install-check.test.cjs | 732 ++++++++++++++++++ tests/fixtures/adversarial/toml/README.md | 42 + .../adversarial/toml/bom-pinned-model.toml | 6 + .../adversarial/toml/commented-model-pin.toml | 7 + .../toml/model-in-developer-instructions.toml | 19 + .../model-verbosity-prefix-collision.toml | 6 + .../toml/whitespace-indented-model-pin.toml | 6 + 12 files changed, 1148 insertions(+), 2 deletions(-) create mode 100644 .changeset/gentle-foxes-glide.md create mode 100644 tests/fixtures/adversarial/toml/README.md create mode 100644 tests/fixtures/adversarial/toml/bom-pinned-model.toml create mode 100644 tests/fixtures/adversarial/toml/commented-model-pin.toml create mode 100644 tests/fixtures/adversarial/toml/model-in-developer-instructions.toml create mode 100644 tests/fixtures/adversarial/toml/model-verbosity-prefix-collision.toml create mode 100644 tests/fixtures/adversarial/toml/whitespace-indented-model-pin.toml diff --git a/.changeset/gentle-foxes-glide.md b/.changeset/gentle-foxes-glide.md new file mode 100644 index 000000000..29ff86b4b --- /dev/null +++ b/.changeset/gentle-foxes-glide.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3290 +--- +**`validate agents` now reports Codex `.toml` model posture, not just presence** — on a `codex` install it flags any agent whose `.toml` pins a GSD tier alias or a `claude-*` id (which Codex rejects with a 400, so the agent never spawns) or carries a `model_reasoning_effort` with no `model`. Previously the check confirmed only that agent files existed, so a stale install from before the passive-model posture reported healthy right up until a typed agent failed to start. Read-only — it names the offending agent and value and never edits your files. Reports `not_codex` and reads nothing on other runtimes. (#3242) diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index cc8e440eb..299d97f27 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -1736,6 +1736,29 @@ node gsd-tools.cjs roadmap upgrade --convention milestone-prefixed --apply # ap ## State Management Commands +### `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. + +**Prerequisites:** GSD installed for a runtime +**Produces:** Installed / missing / incomplete agent lists, plus a `codex_posture` report + +```bash +node gsd-tools.cjs validate agents +``` + +`codex_posture` is populated only when the active runtime is `codex`; on every other runtime it reports `not_codex` and reads nothing from disk. It is **read-only** — it reports violations and never edits your files. + +| Violation reason | Meaning | +|---|---| +| `anthropic_flavored_model` | The `.toml` pins a GSD tier alias (`opus`, `sonnet`, `haiku`, `fable`) or a `claude-*` id. Codex rejects these — the agent fails to spawn with a 400 | +| `orphaned_reasoning_effort` | A `model_reasoning_effort` with no `model`, leaving the model following your Codex session while the effort follows GSD ([#838](https://github.com/open-gsd/gsd-core/issues/838)) | +| `unreadable` | The file could not be read. Other agents are still checked | + +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. + +--- + ### `state validate` Detect drift between STATE.md and the actual filesystem. diff --git a/docs/how-to/recover-and-troubleshoot.md b/docs/how-to/recover-and-troubleshoot.md index 3e0cf8e6f..b12fc7ec8 100644 --- a/docs/how-to/recover-and-troubleshoot.md +++ b/docs/how-to/recover-and-troubleshoot.md @@ -265,6 +265,53 @@ npx @opengsd/gsd-core@latest --claude --local For runtime-specific install paths and troubleshooting, see [Install on your runtime](install-on-your-runtime.md). +### If Codex agents fail to spawn with a 400 about an unsupported model + +Symptom — a typed agent (`gsd-planner`, `gsd-executor`, …) fails to start and the whole +plan/execute flow falls back to a generic agent: + +``` +400 invalid_request_error: "The 'sonnet' model is not supported when using Codex with a ChatGPT account." +``` + +This means an installed `~/.codex/agents/.toml` still pins a model your Codex session cannot +serve. Installs made before the passive model posture landed embedded a per-tier model; a +ChatGPT-account session exposes only its own model, so the pin fails the request outright +([ADR-2313](../adr/2313-codex-passive-model-posture.md)). + +Check which agents are affected: + +```bash +node gsd-tools.cjs validate agents +``` + +The `codex_posture` section reports one violation per offending agent, naming the file and the +offending value. Two things it flags: + +- `anthropic_flavored_model` — the `.toml` pins a GSD tier alias (`opus`, `sonnet`, `haiku`, + `fable`) or a `claude-*` id. Codex rejects all of these. +- `orphaned_reasoning_effort` — a `model_reasoning_effort` with no `model`, which leaves the model + following your Codex session while the effort follows GSD. + +An empty violations list means every regular `.toml` in the directory is posture-clean, so the 400 +is coming from somewhere else — check that your Codex session itself is healthy. + +One thing the check deliberately does not inspect: an agent file that is a **symlink** is skipped +rather than followed, matching how the effort sync treats them. If you symlink your agent configs, +verify those targets by hand. + +**Fix:** re-run the installer. Current versions write no model at all, so agents inherit the session +model: + +```bash +npx @opengsd/gsd-core@latest --codex --global +``` + +If you are on an **API-key** account and genuinely want a pinned model, name a real Codex model id +per agent instead — see [How to configure model profiles](configure-model-profiles.md#codex-does-not-do-tier-routing--pin-explicitly-instead). + +> The check is read-only. It reports what is wrong and does not edit your files. + ### If an update overwrote your local changes Since v1.17, the installer backs up locally modified files to `gsd-local-patches/`. Reapply your changes: diff --git a/src/agent-install-check.cts b/src/agent-install-check.cts index e8878974d..99be1efce 100644 --- a/src/agent-install-check.cts +++ b/src/agent-install-check.cts @@ -16,6 +16,11 @@ import modelProfiles = require('./model-profiles.cjs'); const { MODEL_PROFILES } = modelProfiles; import { getGlobalConfigDir } from './runtime-homes.cjs'; import { getDirName, NO_LOCAL_CONFIG_DIR_SENTINEL } from './runtime-name-policy.cjs'; +// #3242 — model-catalog is a genuine leaf (only node:path + its own JSON), which is +// exactly why Phase 1 (#3241) moved isAnthropicFlavoredModel there: this module can +// consume it without dragging model-resolver's config-loader chain into a +// pure read/verify surface. +import { isAnthropicFlavoredModel } from './model-catalog.cjs'; interface AgentsInstalledResult { agents_installed: boolean; @@ -26,6 +31,132 @@ interface AgentsInstalledResult { agent_runtime: string; } +/** + * Frozen reason enum for {@link checkCodexModelPosture}. Per CONTRIBUTING's + * typed-IR rule ("Error / status / reason → a frozen enum"): callers and tests + * assert on these wire values, never on prose. Adding a member is a deliberate + * three-way coordinated change — enum, emitting site, and the enum-lock test. + */ +const POSTURE_REASON = Object.freeze({ + ANTHROPIC_FLAVORED_MODEL: 'anthropic_flavored_model', + ORPHANED_REASONING_EFFORT: 'orphaned_reasoning_effort', + UNREADABLE: 'unreadable', + NOT_CODEX: 'not_codex', + AGENTS_DIR_MISSING: 'agents_dir_missing', +}); + +type PostureReason = (typeof POSTURE_REASON)[keyof typeof POSTURE_REASON]; + +interface PostureViolation { + agent: string; + file: string; + reason: PostureReason; + value?: string; +} + +interface CodexModelPostureResult { + ok: boolean; + violations: PostureViolation[]; + 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. +function truncatePostureValue(value: string): string { + return value.length > 64 ? `${value.slice(0, 64)}…` : value; +} + +// The `developer_instructions` block is a TOML multi-line literal string +// (`developer_instructions = '''...'''`) that `generateCodexAgentToml` always +// emits after the header fields. Prompt prose inside that block discusses models +// constantly, so a `model = ...`-shaped line inside it must never be read as a +// live pin — but the block can legally appear anywhere in the file (a +// hand-reordered agent can move `model` after it), and another key's *value* can +// legally contain the literal text `developer_instructions = '''` (e.g. a +// `description` field quoting it) without that being the real block opener. So +// instead of truncating the file at the first textual occurrence of the marker +// anywhere in the content, this locates the block by its anchored line-start +// opener (`^\s*developer_instructions\s*=\s*'''`, never a mid-line/mid-value +// match) and its closing `'''` line, and excludes only the lines between them — +// every other line in the file, before AND after the block, is scanned. +// +// If no opener is found, nothing is excluded (the whole file is scanned). If the +// block is unterminated (no closing `'''` before EOF — a malformed file), the +// rest of the file is treated as inside the block: that is the safe direction, +// since misreading prompt prose as a pin past a malformed block is only a +// false positive (wastes a user's time), while the alternative — scanning past +// an unterminated block — risks hiding a real pin inside unclosed prose. The +// emitter always uses `'''` (a TOML literal string), never a `"""` basic +// multi-line string, so only `'''` is treated as the block delimiter here. +function findDeveloperInstructionsBlockRange(lines: string[]): { start: number; end: number } { + const openIndex = lines.findIndex((line) => /^\s*developer_instructions\s*=\s*'''/.test(line)); + if (openIndex === -1) { + return { start: -1, end: -1 }; + } + const afterOpenMarker = lines[openIndex].replace(/^\s*developer_instructions\s*=\s*'''/, ''); + if (afterOpenMarker.includes("'''")) { + // Same-line block: developer_instructions = '''one line''' + return { start: openIndex, end: openIndex }; + } + for (let i = openIndex + 1; i < lines.length; i++) { + if (lines[i].includes("'''")) { + return { start: openIndex, end: i }; + } + } + return { start: openIndex, end: lines.length - 1 }; +} + +// Strips a leading UTF-8 BOM (U+FEFF), which fs.readFileSync(..., 'utf8') does not +// strip on its own, and unwraps a TOML basic/literal string value's surrounding +// quotes so `model = "sonnet"` yields `sonnet`, not `"sonnet"`. +function stripBOM(content: string): string { + return content.charCodeAt(0) === 0xfeff ? content.slice(1) : content; +} + +function unquoteTomlValue(rawValue: string): string { + const trimmed = rawValue.trim(); + const quoted = trimmed.match(/^"([^"]*)"/) ?? trimmed.match(/^'([^']*)'/); + return quoted ? quoted[1] : trimmed; +} + +interface HeaderScanResult { + model: string | null; + hasReasoningEffort: boolean; +} + +// Line-oriented scan of every line OUTSIDE the `developer_instructions` block +// (see findDeveloperInstructionsBlockRange). Full-key-name anchoring — +// `^([A-Za-z_][\w]*)\s*=` for a bare key, or `^"([^"]*)"\s*=` / `^'([^']*)'\s*=` +// for TOML's legal quoted-key forms, normalized to the same key name — means +// `model_verbosity` / `model_reasoning_effort` never satisfy a `model` probe, +// and vice versa; `#`-prefixed lines (after trimming leading whitespace) are +// treated as comments, never live pins. +function scanTomlLines(content: string): HeaderScanResult { + const lines = content.split(/\r?\n/); + const { start, end } = findDeveloperInstructionsBlockRange(lines); + let model: string | null = null; + let hasReasoningEffort = false; + for (let i = 0; i < lines.length; i++) { + if (start !== -1 && i >= start && i <= end) continue; + const trimmed = lines[i].trim(); + if (trimmed === '' || trimmed.startsWith('#')) continue; + const match = trimmed.match(/^(?:"([^"]*)"|'([^']*)'|([A-Za-z_][\w]*))\s*=\s*(.*)$/); + if (!match) continue; + const key = match[1] ?? match[2] ?? match[3]; + const rawValue = match[4]; + if (key === 'model') { + model = unquoteTomlValue(rawValue); + } else if (key === 'model_reasoning_effort') { + hasReasoningEffort = true; + } + } + return { model, hasReasoningEffort }; +} + /** * Resolve the agents directory for the given runtime. * @@ -186,7 +317,123 @@ function checkAgentsInstalled(runtime?: string, projectRoot?: string): AgentsIns }; } +/** + * Validate Codex `.toml` agent files for Anthropic-flavored `model` pins and + * orphaned `model_reasoning_effort` values (ADR-2313 D6, #3242). + * + * A new sibling export to {@link checkAgentsInstalled}, deliberately — that + * function carries 33 upstream dependents and cyclomatic complexity 25, so this + * posture check gets zero new branches there (see 40-design.md "Rejected" #1). + * Presence is `checkAgentsInstalled`'s job; this function's job starts only once + * the runtime is confirmed `codex` and only inspects posture, never presence. + * + * Read-only: detects, never repairs (repair is Phase 3, #3243). + * + * @param runtime - the active runtime name; defaults to GSD_RUNTIME env, then 'claude' + * @param projectRoot - canonical project root for local-install discovery + */ +function checkCodexModelPosture(runtime?: string, projectRoot?: string): CodexModelPostureResult { + // Short-circuit BEFORE any filesystem access: a non-codex runtime must never + // have its agents directory resolved or a stray .toml inspected, however + // violating that file's contents would be if it were ever read (#3242 row 25). + 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)) { + // Presence is checkAgentsInstalled's job — an absent agents dir here is a + // distinct, non-violating outcome, not a failure of this check. + return { + ok: true, + violations: [], + checked: [], + agents_dir: agentsDir, + agent_runtime: resolvedRuntime, + reason: POSTURE_REASON.AGENTS_DIR_MISSING, + }; + } + + // Skip symlinks — matches cmdEffortSync's existing idiom in commands.cts + // ("Skip symlinks — only write regular files..."). Here the risk is reading + // (not writing) through a symlink: readFileSync follows symlinks, so an + // agents-dir symlink pointing at an arbitrary file would let that target's + // contents be echoed into this function's `value` field. A symlinked agent + // file is a structural install choice (checkAgentsInstalled's territory), + // not a model-content posture defect, so it is silently excluded from + // `checked` rather than reported as a distinct violation — same shape as + // cmdEffortSync, which silently drops symlinks from its file list rather + // than inventing a new skip/violation reason. + 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: PostureViolation[] = []; + + 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 { + // Never throws, never silently skips — an unreadable file is reported and + // the loop continues checking the rest (40-design.md "Rejected" #5). + violations.push({ agent: agentName, file: filePath, reason: POSTURE_REASON.UNREADABLE }); + continue; + } + + const { model, hasReasoningEffort } = scanTomlLines(stripBOM(raw)); + + if (model !== null && isAnthropicFlavoredModel(model)) { + violations.push({ + agent: agentName, + file: filePath, + reason: POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL, + value: truncatePostureValue(model), + }); + } else if (model === null && hasReasoningEffort) { + // #838 coupling: a reasoning-effort pin with no model pin means Codex + // inherits the session model while the effort pin silently disagrees. + violations.push({ + agent: agentName, + file: filePath, + reason: POSTURE_REASON.ORPHANED_REASONING_EFFORT, + }); + } + } + + return { + ok: violations.length === 0, + violations, + checked, + agents_dir: agentsDir, + agent_runtime: resolvedRuntime, + }; +} + export = { getAgentsDir, checkAgentsInstalled, + checkCodexModelPosture, + POSTURE_REASON, }; diff --git a/src/verify.cts b/src/verify.cts index 5d0abc988..fc8cc3e08 100644 --- a/src/verify.cts +++ b/src/verify.cts @@ -37,7 +37,7 @@ import { extractTaggedBlocks } from './markdown-sectionizer.cjs'; import { VALID_PROFILES, VALID_TIERS, VALID_PHASE_TYPES } from './model-catalog.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 } = agentInstallCheck; +const { checkAgentsInstalled, checkCodexModelPosture } = agentInstallCheck; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); const { output, error } = ioMod; @@ -2459,8 +2459,13 @@ function cmdValidateHealth( } function cmdValidateAgents(cwd: string, raw: boolean): void { - const agentStatus = checkAgentsInstalled(resolveRuntime(cwd), cwd); + const runtime = resolveRuntime(cwd); + const agentStatus = checkAgentsInstalled(runtime, cwd); const expected = Object.keys(MODEL_PROFILES); + // #3242 ADR-2313 D6 — additive: validates posture (never an Anthropic-flavored + // model or an orphaned reasoning-effort pin in a Codex agent .toml), not just + // presence. checkAgentsInstalled above is untouched. + const codexPosture = checkCodexModelPosture(runtime, cwd); output( { @@ -2470,6 +2475,7 @@ function cmdValidateAgents(cwd: string, raw: boolean): void { missing: agentStatus.missing_agents, incomplete: agentStatus.incomplete_agents, expected, + codex_posture: codexPosture, }, raw, ); diff --git a/tests/agent-install-check.test.cjs b/tests/agent-install-check.test.cjs index f6d4bdd47..9be7e7af2 100644 --- a/tests/agent-install-check.test.cjs +++ b/tests/agent-install-check.test.cjs @@ -37,6 +37,13 @@ const { getDirName } = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'li const MODEL_PROFILES = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'model-profiles.cjs')).MODEL_PROFILES; const EXPECTED_AGENTS = Object.keys(MODEL_PROFILES); +// #3242 — single source of truth for the Anthropic-flavored alias/id set (Phase 1 +// moved the predicate here), so the posture tests below can't silently drift from +// what the posture check is actually supposed to reject. +const { CLAUDE_AGENT_ALIASES } = require( + path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'model-catalog.cjs'), +); + // ─── Environment isolation ──────────────────────────────────────────────────── let savedAgentsDir; @@ -434,3 +441,728 @@ describe('checkAgentsInstalled', () => { assert.strictEqual(result.agent_runtime, 'cursor'); }); }); + +// ─── 3. checkCodexModelPosture behaviour (#3242, ADR-2313 D6) ───────────────── +// +// Spec: .gsd/phase/feat-3242-codex-posture-health-check/{40-design,50-test-matrix}.md +// Interface (not yet implemented — every test below is red until it lands): +// POSTURE_REASON: frozen enum { ANTHROPIC_FLAVORED_MODEL, ORPHANED_REASONING_EFFORT, +// UNREADABLE, NOT_CODEX, AGENTS_DIR_MISSING } +// checkCodexModelPosture(runtime?, projectRoot?) => { +// ok, violations: [{ agent, file, reason, value? }], checked, agents_dir, +// agent_runtime, reason? +// } +// +// Row numbers below (# N) map 1:1 to 50-test-matrix.md. Rows 12, 13, 14, 15, 16, 25 +// are the negative proofs the matrix calls out as the ones that actually discriminate +// a correct implementation from a naive whole-file `/model\s*=/` scan; each of those +// test names states the specific implementation mistake it catches. + +const POSTURE_FIXTURES_DIR = path.join(__dirname, 'fixtures', 'adversarial', 'toml'); + +function writeAgentToml(agentsDir, agentName, content) { + fs.mkdirSync(agentsDir, { recursive: true }); + fs.writeFileSync(path.join(agentsDir, `${agentName}.toml`), content); +} + +// Loads a hand-authored fixture (tests/fixtures/adversarial/toml/, #2371 provenance) +// as raw bytes so CRLF / BOM content is copied byte-for-byte, not re-encoded through +// a JS string round-trip that could normalize either. +function copyFixtureToml(agentsDir, agentName, fixtureFile) { + fs.mkdirSync(agentsDir, { recursive: true }); + const raw = fs.readFileSync(path.join(POSTURE_FIXTURES_DIR, fixtureFile)); + fs.writeFileSync(path.join(agentsDir, `${agentName}.toml`), raw); +} + +// Row 18a's CRLF fixture is derived at test runtime, never read from a committed file. +// `.gitattributes:2` is `* text=auto eol=lf`, repo-wide and deliberate, so a `\r\n` +// fixture committed to disk is normalized to LF on every commit and every checkout — +// a whole-file CRLF fixture proves nothing about CRLF handling once git has touched it. +// Authoring the LF content inline and converting it here keeps the CRLF-ness under the +// test's control instead of git's. (The BOM fixture is unaffected by eol=lf — a BOM is +// not a line ending — so it stays a committed file.) +function toCrlf(lfContent) { + return lfContent.replace(/\n/g, '\r\n'); +} + +// Fault injection for row 21 — monkeypatches node:fs's readFileSync and restores it +// in a `finally` INSIDE this helper (never in a test body, and never chmod 0o000, +// which root bypasses under Docker/CI and would give the test zero real coverage). +function withInjectedReadFailure(targetPath, injectedError, fn) { + const realReadFileSync = fs.readFileSync; + fs.readFileSync = function poisonedReadFileSync(target, ...args) { + const targetStr = typeof target === 'string' ? target : String(target); + if (targetStr === targetPath || path.resolve(targetStr) === path.resolve(targetPath)) { + throw injectedError; + } + return realReadFileSync.apply(fs, [target, ...args]); + }; + try { + return fn(); + } finally { + fs.readFileSync = realReadFileSync; + } +} + +describe('checkCodexModelPosture', () => { + let tmpDir; + let agentsDir; + + beforeEach(() => { + tmpDir = createTempDir('gsd-posture-check-'); + agentsDir = path.join(tmpDir, 'agents'); + process.env['GSD_AGENTS_DIR'] = agentsDir; + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // # 1 — runtime is not codex: a no-op, not a failure, and it must not even read + // the filesystem to get there (checkAgentsInstalled's job is presence; this + // function's job starts only once the runtime is actually codex). + test('row 1: non-codex runtime (claude) is a no-op — NOT_CODEX, no violations, no fs.readFileSync call', (t) => { + const reads = []; + t.mock.method(fs, 'readFileSync', (...args) => { + reads.push(args[0]); + throw new Error('unreachable: readFileSync must not be called for a non-codex runtime'); + }); + + const result = agentInstallCheck.checkCodexModelPosture('claude', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + assert.strictEqual(result.reason, agentInstallCheck.POSTURE_REASON.NOT_CODEX); + assert.deepStrictEqual(reads, [], 'non-codex runtime must short-circuit before any file read'); + }); + + // # 2 — codex runtime, agents dir absent: distinct from row-3 empty-dir and + // distinct from a violation. Presence is checkAgentsInstalled's job. + test('row 2: codex + agents dir absent — AGENTS_DIR_MISSING, not a violation', () => { + // agentsDir intentionally not created + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + assert.strictEqual(result.reason, agentInstallCheck.POSTURE_REASON.AGENTS_DIR_MISSING); + }); + + // # 3 — codex runtime, agents dir exists but is empty. + test('row 3: codex + empty agents dir — ok:true, checked:[]', () => { + fs.mkdirSync(agentsDir, { recursive: true }); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + assert.deepStrictEqual(result.checked, []); + }); + + // # 4 — one clean .toml (no model, no effort key at all). + test('row 4: clean .toml (no model, no effort) — ok:true, no violations', () => { + writeAgentToml( + agentsDir, + 'gsd-clean', + 'name = "gsd-clean"\ndescription = "a clean agent"\ndeveloper_instructions = \'\'\'\nDo the work.\n\'\'\'\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + }); + + // # 5 — the base case: a bare Claude tier alias pinned as `model`. + test('row 5: model = "sonnet" — ANTHROPIC_FLAVORED_MODEL naming the agent and the value', () => { + writeAgentToml( + agentsDir, + 'gsd-planner', + 'name = "gsd-planner"\nmodel = "sonnet"\ndeveloper_instructions = \'\'\'\nPlan.\n\'\'\'\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, false); + assert.strictEqual(result.violations.length, 1); + assert.strictEqual(result.violations[0].agent, 'gsd-planner'); + assert.strictEqual(result.violations[0].reason, agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL); + assert.strictEqual(result.violations[0].value, 'sonnet'); + assert.ok(result.violations[0].file.endsWith('gsd-planner.toml')); + }); + + // # 6 — table-driven over the full bare-alias set, sourced from model-catalog's + // CLAUDE_AGENT_ALIASES (not hardcoded) so this can't silently drift from the + // predicate the design says the posture check consumes. + for (const alias of CLAUDE_AGENT_ALIASES) { + test(`row 6: bare Claude alias "${alias}" — ANTHROPIC_FLAVORED_MODEL`, () => { + writeAgentToml( + agentsDir, + 'gsd-alias-agent', + `name = "gsd-alias-agent"\nmodel = "${alias}"\ndeveloper_instructions = '''\nWork.\n'''\n`, + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.violations.length, 1); + assert.strictEqual(result.violations[0].reason, agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL); + assert.strictEqual(result.violations[0].value, alias); + }); + } + + // # 7 — table-driven over full Claude model ids across provider namespacings, + // plus a case-insensitivity check. + for (const modelId of ['claude-opus-4-5', 'anthropic/claude-x', 'us.anthropic.claude-x', 'CLAUDE-X']) { + test(`row 7: Claude model id "${modelId}" — ANTHROPIC_FLAVORED_MODEL`, () => { + writeAgentToml( + agentsDir, + 'gsd-id-agent', + `name = "gsd-id-agent"\nmodel = "${modelId}"\ndeveloper_instructions = '''\nWork.\n'''\n`, + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.violations.length, 1); + assert.strictEqual(result.violations[0].reason, agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL); + }); + } + + // # 8 — the rule is Anthropic-flavored, never an allowlist: a real Codex/OpenAI id + // must never be flagged, however unfamiliar it looks. + test('row 8: model = "gpt-5.6-sol" — no violation (not an allowlist)', () => { + writeAgentToml( + agentsDir, + 'gsd-gpt-agent', + 'name = "gsd-gpt-agent"\nmodel = "gpt-5.6-sol"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + }); + + // # 9 — must not go stale on a hypothetical future OpenAI release id. + test('row 9: model = "some-future-model-id" — no violation', () => { + writeAgentToml( + agentsDir, + 'gsd-future-agent', + 'name = "gsd-future-agent"\nmodel = "some-future-model-id"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + }); + + // # 10 — #838 coupling: a static reasoning-effort with no model pin means Codex + // is inheriting the session model while GSD's effort pin silently disagrees. + test('row 10: model_reasoning_effort with no model — ORPHANED_REASONING_EFFORT', () => { + writeAgentToml( + agentsDir, + 'gsd-orphan-agent', + 'name = "gsd-orphan-agent"\nmodel_reasoning_effort = "high"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, false); + assert.strictEqual(result.violations.length, 1); + assert.strictEqual(result.violations[0].reason, agentInstallCheck.POSTURE_REASON.ORPHANED_REASONING_EFFORT); + assert.strictEqual(result.violations[0].agent, 'gsd-orphan-agent'); + }); + + // # 11 — the legal pinned pair: model + matching reasoning effort is intentional. + test('row 11: model + model_reasoning_effort, model legal — no violation', () => { + writeAgentToml( + agentsDir, + 'gsd-pinned-agent', + 'name = "gsd-pinned-agent"\nmodel = "gpt-5-codex"\nmodel_reasoning_effort = "high"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + }); + + // # 12 — NEGATIVE PROOF. Catches: over-generalizing the #838 model/effort + // coupling rule to service_tier/model_verbosity too. Those are #774's + // cost/verbosity knobs and are decoupled from `model` by design — an + // implementation that treats "any knob present without model" as orphaned + // fails this row. + test('row 12 (negative proof): service_tier + model_verbosity, no model — no violation', () => { + writeAgentToml( + agentsDir, + 'gsd-light-agent', + 'name = "gsd-light-agent"\nservice_tier = "flex"\nmodel_verbosity = "low"\ndeveloper_instructions = \'\'\'\nWork fast.\n\'\'\'\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + }); + + // # 13 — NEGATIVE PROOF. Catches: implementing the check as a whitelist over the + // whole TOML document (flagging any key GSD doesn't itself emit) instead of a + // predicate on exactly the two fields the posture owns (`model`, + // `model_reasoning_effort`). A hand-added `approval_policy` is legitimate and + // must never be flagged. + test('row 13 (negative proof): extra hand-added key (approval_policy) — no violation', () => { + writeAgentToml( + agentsDir, + 'gsd-custom-agent', + 'name = "gsd-custom-agent"\napproval_policy = "on-request"\ndeveloper_instructions = \'\'\'\nFollow policy.\n\'\'\'\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + }); + + // # 14 — NEGATIVE PROOF, the headline trap. Catches: scanning the WHOLE file with + // a line-oriented `/^model\s*=/m` instead of only the header slice (the lines + // before `developer_instructions = '''`). The fixture below contains a literal, + // unindented `model = "sonnet"` line INSIDE the developer_instructions block — + // verified (see PR description / dispatch notes) to trip a naive whole-file + // regex scan (`/^model\s*=/m.test(wholeFile) === true`) while the correct + // header-slice scan sees nothing (`/^model\s*=/m.test(headerOnly) === false`). + // If this test passed against a whole-file scanner it would prove nothing; it + // must fail against one. + test('row 14 (negative proof, headline): model = inside developer_instructions block — no violation', () => { + copyFixtureToml(agentsDir, 'gsd-planner', 'model-in-developer-instructions.toml'); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + }); + + // # 15 — NEGATIVE PROOF. Catches: a header-slice scan that doesn't skip comment + // lines, so a commented-out `# model = "sonnet"` still counts as a live pin. + test('row 15 (negative proof): commented-out model pin — no violation', () => { + copyFixtureToml(agentsDir, 'gsd-reviewer', 'commented-model-pin.toml'); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + }); + + // # 16 — NEGATIVE PROOF. Catches: probing for the key by substring/prefix + // (e.g. a bare `/model/` test) instead of anchoring on the full key name, so + // `model_verbosity` gets misidentified as a `model` pin. + test('row 16 (negative proof): model_verbosity only (key-prefix collision) — no violation', () => { + copyFixtureToml(agentsDir, 'gsd-analyst', 'model-verbosity-prefix-collision.toml'); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + }); + + // # 17 — boundary: whitespace around a REAL key must still be recognized as a pin. + test('row 17: indented / inner-spaced model pin — IS a violation', () => { + copyFixtureToml(agentsDir, 'gsd-tester', 'whitespace-indented-model-pin.toml'); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, false); + assert.strictEqual(result.violations.length, 1); + assert.strictEqual(result.violations[0].reason, agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL); + assert.strictEqual(result.violations[0].value, 'sonnet'); + }); + + // # 18 — cross-platform: CRLF and BOM must parse identically to plain LF. + test('row 18a: CRLF file with a pinned model — parsed identically to LF (still a violation)', () => { + const lfContent = + 'name = "gsd-scribe"\ndescription = "Writes changelog entries from merged PRs"\n' + + 'model = "sonnet"\ndeveloper_instructions = \'\'\'\n' + + 'Write a changelog entry summarizing the merged pull request.\n\'\'\'\n'; + writeAgentToml(agentsDir, 'gsd-scribe', toCrlf(lfContent)); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.violations.length, 1); + assert.strictEqual(result.violations[0].reason, agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL); + assert.strictEqual(result.violations[0].value, 'sonnet'); + }); + + test('row 18b: BOM-prefixed file with a pinned model — parsed identically to a BOM-free file', () => { + copyFixtureToml(agentsDir, 'gsd-archivist', 'bom-pinned-model.toml'); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.violations.length, 1); + assert.strictEqual(result.violations[0].reason, agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL); + assert.strictEqual(result.violations[0].value, 'sonnet'); + }); + + // # 19 — independence: multiple agents, some violating; deterministic order; + // clean agents are simply absent from violations (not present-but-empty). + test('row 19: several agents, some violating — one entry per offender, deterministic order, clean absent', () => { + writeAgentToml( + agentsDir, + 'gsd-alpha', + 'name = "gsd-alpha"\ndeveloper_instructions = \'\'\'\nClean.\n\'\'\'\n', + ); + writeAgentToml( + agentsDir, + 'gsd-bravo', + 'name = "gsd-bravo"\nmodel = "opus"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\n', + ); + writeAgentToml( + agentsDir, + 'gsd-charlie', + 'name = "gsd-charlie"\nmodel_reasoning_effort = "medium"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, false); + assert.strictEqual(result.violations.length, 2); + assert.strictEqual(result.violations[0].agent, 'gsd-bravo'); + assert.strictEqual(result.violations[1].agent, 'gsd-charlie'); + assert.ok( + !result.violations.some((v) => v.agent === 'gsd-alpha'), + 'clean agent must not appear in violations at all', + ); + assert.strictEqual(result.checked.length, 3); + }); + + // ─── Reviewer-found false negatives (quoted keys; block-marker truncation) ── + // + // Both defects are false negatives — checkCodexModelPosture reported ok:true + // when a real pin was present. Each test below is red against the + // implementation this PR replaces; see the PR description for exactly which + // assertion fails against each. + + // Defect 1: a quoted TOML key (`"model" = ...`) is legal TOML and was invisible + // to a key regex that required a bare identifier. + test('quoted key "model" = "sonnet" (double-quoted) — still ANTHROPIC_FLAVORED_MODEL', () => { + writeAgentToml( + agentsDir, + 'gsd-quoted-double', + 'name = "gsd-quoted-double"\n"model" = "sonnet"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, false); + assert.strictEqual(result.violations.length, 1); + assert.strictEqual(result.violations[0].reason, agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL); + assert.strictEqual(result.violations[0].value, 'sonnet'); + }); + + test("quoted key 'model' = \"sonnet\" (single-quoted) — still ANTHROPIC_FLAVORED_MODEL", () => { + writeAgentToml( + agentsDir, + 'gsd-quoted-single', + 'name = "gsd-quoted-single"\n\'model\' = "sonnet"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, false); + assert.strictEqual(result.violations.length, 1); + assert.strictEqual(result.violations[0].reason, agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL); + assert.strictEqual(result.violations[0].value, 'sonnet'); + }); + + // Defect 2a: the block marker used to be found by an unanchored whole-content + // search, so a `description` value that merely quotes the marker text earlier + // in the file truncated the header before a real, later `model` pin. + test('a description value quoting the marker text does not hide a real model pin before it', () => { + writeAgentToml( + agentsDir, + 'gsd-decoy-marker', + 'name = "gsd-decoy-marker"\n' + + 'description = "mentions developer_instructions = \'\'\' as an example string"\n' + + 'model = "sonnet"\n' + + 'developer_instructions = \'\'\'\nWork.\n\'\'\'\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, false); + assert.strictEqual(result.violations.length, 1); + assert.strictEqual(result.violations[0].reason, agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL); + assert.strictEqual(result.violations[0].value, 'sonnet'); + }); + + // Defect 2b: the old implementation only ever scanned the slice BEFORE the + // marker, so a hand-reordered file with `model` placed AFTER the + // developer_instructions block (still legal TOML) was never scanned at all. + test('a model pin placed AFTER the developer_instructions block is still flagged', () => { + writeAgentToml( + agentsDir, + 'gsd-reordered', + 'name = "gsd-reordered"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\nmodel = "sonnet"\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, false); + assert.strictEqual(result.violations.length, 1); + assert.strictEqual(result.violations[0].reason, agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL); + assert.strictEqual(result.violations[0].value, 'sonnet'); + }); + + // Defect-2 boundary: no developer_instructions block at all — every line must + // be scanned. NOTE: a bare-key version of this fixture is already green + // against the pre-fix implementation (its no-marker fallback already scanned + // the whole file), so this uses a quoted key to keep the assertion genuinely + // red pre-fix (via defect 1) while proving the no-block path is fully scanned. + test('a file with no developer_instructions block at all is fully scanned', () => { + writeAgentToml( + agentsDir, + 'gsd-noblock', + 'name = "gsd-noblock"\ndescription = "no prompt block on this agent"\n"model" = "sonnet"\n', + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, false); + assert.strictEqual(result.violations.length, 1); + assert.strictEqual(result.violations[0].reason, agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL); + assert.strictEqual(result.violations[0].value, 'sonnet'); + }); + + // # 21 — filesystem failure: an unreadable .toml is reported, not thrown, and + // does not abort checking the rest of the agents. Injected via fs.readFileSync + // monkeypatch/restore (see withInjectedReadFailure) rather than chmod 0o000, + // which root bypasses under Docker/CI. + test('row 21: unreadable .toml (EACCES) — UNREADABLE violation naming the file, other agents still checked, no throw', () => { + writeAgentToml(agentsDir, 'gsd-bad', 'name = "gsd-bad"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\n'); + writeAgentToml(agentsDir, 'gsd-good', 'name = "gsd-good"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\n'); + const badPath = path.join(agentsDir, 'gsd-bad.toml'); + const injected = Object.assign(new Error('injected EACCES'), { code: 'EACCES' }); + + const result = withInjectedReadFailure(badPath, injected, () => + agentInstallCheck.checkCodexModelPosture('codex', tmpDir), + ); + + assert.strictEqual(result.ok, false); + const unreadable = result.violations.find((v) => v.agent === 'gsd-bad'); + assert.ok(unreadable, 'unreadable file must produce a named violation, not a silent skip'); + assert.strictEqual(unreadable.reason, agentInstallCheck.POSTURE_REASON.UNREADABLE); + assert.ok(unreadable.file.endsWith('gsd-bad.toml')); + assert.strictEqual(result.checked.length, 2, 'the unreadable file must still be counted as checked'); + assert.ok( + !result.violations.some((v) => v.agent === 'gsd-good'), + 'the still-readable sibling must be checked and found clean', + ); + }); + + // Security review (#3242, MEDIUM): readdirSync + readFileSync followed symlinks, + // so a symlink in the agents directory pointing at an arbitrary file could have + // that file's content echoed into a violation's `value` field. Fixed by lstat- + // filtering to regular files only, matching cmdEffortSync's existing symlink + // guard in commands.cts. Symlinks are silently excluded (not reported), same + // as cmdEffortSync — see agent-install-check.cts inline comment for why. + test('symlink pointing at a file containing model = "sonnet" is never read — no violation names that value', (t) => { + const targetPath = path.join(tmpDir, 'outside-target.toml'); + fs.writeFileSync(targetPath, 'model = "sonnet"\n'); + fs.mkdirSync(agentsDir, { recursive: true }); + const symlinkPath = path.join(agentsDir, 'gsd-linked.toml'); + try { + fs.symlinkSync(targetPath, symlinkPath, 'file'); + } catch (error) { + if (error && ['EPERM', 'EACCES', 'ENOTSUP'].includes(error.code)) { + t.skip('symlink creation is not available on this platform'); + return; + } + throw error; + } + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + assert.deepStrictEqual(result.checked, [], 'the symlinked entry must not appear in checked'); + }); + + test('broken symlink in agents dir does not crash the scan — other agents still checked', (t) => { + fs.mkdirSync(agentsDir, { recursive: true }); + const brokenTarget = path.join(tmpDir, 'does-not-exist.toml'); + const brokenSymlink = path.join(agentsDir, 'gsd-broken.toml'); + try { + fs.symlinkSync(brokenTarget, brokenSymlink, 'file'); + } catch (error) { + if (error && ['EPERM', 'EACCES', 'ENOTSUP'].includes(error.code)) { + t.skip('symlink creation is not available on this platform'); + return; + } + throw error; + } + writeAgentToml(agentsDir, 'gsd-good', 'name = "gsd-good"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\n'); + + let result; + assert.doesNotThrow(() => { + result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + }); + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + assert.deepStrictEqual(result.checked, ['gsd-good'], 'the broken symlink must be excluded, the other agent still checked'); + }); + + test('a regular .toml file is still scanned normally (guard against over-filtering)', () => { + writeAgentToml(agentsDir, 'gsd-planner', 'name = "gsd-planner"\nmodel = "sonnet"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\n'); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, false); + assert.deepStrictEqual(result.checked, ['gsd-planner']); + assert.strictEqual(result.violations.length, 1); + assert.strictEqual(result.violations[0].reason, agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL); + assert.strictEqual(result.violations[0].value, 'sonnet'); + }); + + // # 22 — boundary: empty / whitespace-only .toml pins nothing. + test('row 22: empty / whitespace-only .toml — no violation', () => { + writeAgentToml(agentsDir, 'gsd-blank', ' \n\n\t\n'); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + }); + + // # 23 — hostile: an oversized (secret-shaped) model value must be truncated at + // 64 chars, matching bin/install.js's _warnCodexModelOverrideDropped truncation + // (`slice(0, 64) + '…'`) so an oversized/secret-shaped value cannot reach logs + // in full. + test('row 23: oversized model value — truncated at 64 chars, matching the installer cap', () => { + const oversized = `claude-${'x'.repeat(80)}`; // 87 chars, Anthropic-flavored (contains "claude") + writeAgentToml( + agentsDir, + 'gsd-oversized-agent', + `name = "gsd-oversized-agent"\nmodel = "${oversized}"\ndeveloper_instructions = '''\nWork.\n'''\n`, + ); + + const result = agentInstallCheck.checkCodexModelPosture('codex', tmpDir); + + assert.strictEqual(result.violations.length, 1); + const { value } = result.violations[0]; + assert.ok(value.length <= 65, `expected value capped at 64 chars (+ ellipsis), got length ${value.length}`); + assert.notStrictEqual(value, oversized, 'the full oversized value must not reach the violation untruncated'); + assert.strictEqual(value, `${oversized.slice(0, 64)}…`); + }); + + // # 25 — NEGATIVE PROOF. Catches: gating the runtime no-op AFTER already + // scanning the agents directory (e.g. deciding what to report only at the end), + // instead of short-circuiting before any file is read. A stray .toml that WOULD + // trip a violation if inspected must never be inspected for a non-codex runtime. + test('row 25 (negative proof): non-codex runtime with a stray violating .toml present — still NOT_CODEX, file never inspected', (t) => { + writeAgentToml( + agentsDir, + 'gsd-stray', + 'name = "gsd-stray"\nmodel = "sonnet"\ndeveloper_instructions = \'\'\'\nWork.\n\'\'\'\n', + ); + const reads = []; + t.mock.method(fs, 'readFileSync', (...args) => { + reads.push(args[0]); + throw new Error('unreachable: readFileSync must not be called for a non-codex runtime'); + }); + + const result = agentInstallCheck.checkCodexModelPosture('opencode', tmpDir); + + assert.strictEqual(result.ok, true); + assert.deepStrictEqual(result.violations, []); + assert.strictEqual(result.reason, agentInstallCheck.POSTURE_REASON.NOT_CODEX); + assert.deepStrictEqual(reads, [], 'the stray violating file must never be read for a non-codex runtime'); + }); +}); + +// # 24 — enum lock: adding a POSTURE_REASON value is a deliberate three-way +// coordinated change (enum, emitting site, this test), not a silent drift. Locks +// the exact key set, matching the repo's established shape for reason enums +// (see verify-reapply-patches.cjs's REASON). +describe('POSTURE_REASON enum', () => { + test('row 24: Object.keys(POSTURE_REASON).sort() is locked', () => { + assert.deepStrictEqual( + Object.keys(agentInstallCheck.POSTURE_REASON).sort(), + [ + 'AGENTS_DIR_MISSING', + 'ANTHROPIC_FLAVORED_MODEL', + 'NOT_CODEX', + 'ORPHANED_REASONING_EFFORT', + 'UNREADABLE', + ].sort(), + ); + }); + + test('POSTURE_REASON values are the frozen snake_case wire form, not prose', () => { + assert.strictEqual(agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL, 'anthropic_flavored_model'); + assert.strictEqual(agentInstallCheck.POSTURE_REASON.ORPHANED_REASONING_EFFORT, 'orphaned_reasoning_effort'); + assert.strictEqual(agentInstallCheck.POSTURE_REASON.UNREADABLE, 'unreadable'); + assert.strictEqual(agentInstallCheck.POSTURE_REASON.NOT_CODEX, 'not_codex'); + assert.strictEqual(agentInstallCheck.POSTURE_REASON.AGENTS_DIR_MISSING, 'agents_dir_missing'); + assert.ok(Object.isFrozen(agentInstallCheck.POSTURE_REASON)); + }); +}); + +// # 20 — keystone wiring: a library that works but is never called from the +// user-reachable command surface is the keystone-unwired failure the coverage +// gate exists to catch. Drives cmdValidateAgents directly (the same seam +// tests/verify.test.cjs already uses for cmdValidateHealth) and asserts through +// the command's structured JSON output — captured by monkeypatching +// node:fs.writeSync, the exact seam tests/io.test.cjs already establishes for +// io.cjs's output(), which writes via writeAllSync(1, ...) → fs.writeSync, NOT +// console.log — rather than calling checkCodexModelPosture a second time. +describe('cmdValidateAgents surfaces the Codex posture result (#3242 row 20)', () => { + let tmpDir; + let agentsDir; + + beforeEach(() => { + tmpDir = createTempDir('gsd-posture-wiring-'); + agentsDir = path.join(tmpDir, 'agents'); + process.env['GSD_AGENTS_DIR'] = agentsDir; + process.env['GSD_RUNTIME'] = 'codex'; + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('row 20: validate agents output carries the posture result for a violating install', (t) => { + const { cmdValidateAgents } = require( + path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'verify.cjs'), + ); + writeAgentToml( + agentsDir, + EXPECTED_AGENTS[0], + `name = "${EXPECTED_AGENTS[0]}"\nmodel = "sonnet"\ndeveloper_instructions = '''\nWork.\n'''\n`, + ); + + const written = []; + const realWriteSync = fs.writeSync; + t.mock.method(fs, 'writeSync', (fd, data, offset, length) => { + if (fd !== 1) { + return realWriteSync.call(fs, fd, data, offset, length); + } + const chunk = Buffer.isBuffer(data) + ? data.subarray(offset ?? 0, length === undefined ? data.length : (offset ?? 0) + length).toString('utf8') + : String(data); + written.push(chunk); + return Buffer.byteLength(chunk, 'utf8'); + }); + + cmdValidateAgents(tmpDir, false); + + const parsed = JSON.parse(written.join('')); + assert.ok( + parsed.codex_posture, + `expected cmdValidateAgents output to carry a codex_posture key, got keys: ${Object.keys(parsed).join(', ')}`, + ); + assert.strictEqual(parsed.codex_posture.ok, false); + assert.strictEqual(parsed.codex_posture.violations.length, 1); + assert.strictEqual(parsed.codex_posture.violations[0].agent, EXPECTED_AGENTS[0]); + assert.strictEqual( + parsed.codex_posture.violations[0].reason, + agentInstallCheck.POSTURE_REASON.ANTHROPIC_FLAVORED_MODEL, + ); + }); +}); diff --git a/tests/fixtures/adversarial/toml/README.md b/tests/fixtures/adversarial/toml/README.md new file mode 100644 index 000000000..100bde467 --- /dev/null +++ b/tests/fixtures/adversarial/toml/README.md @@ -0,0 +1,42 @@ +# Adversarial Codex-agent TOML Fixtures (#3242) + +Reusable hostile inputs for `gsd-core/bin/lib/agent-install-check.cjs` +`checkCodexModelPosture()`. + +Per CONTRIBUTING's fixture-provenance rule (#2371), these files are +**hand-authored against the real Codex `.toml` shape** (the header-field / +`developer_instructions = '''...'''` layout `generateCodexAgentToml` in +`bin/install.js` emits) — none of them were produced by calling that +writer. A fixture generated by the gate's own emitter can only confirm +what the emitter already believes about its own output; it cannot surface +a shape the emitter's author didn't anticipate. + +Tests load them by relative path from +`tests/agent-install-check.test.cjs` so the corpus can grow without +touching the writer. + +Categories present: + +- `model-in-developer-instructions.toml` — the headline trap. Contains a + literal `model = "sonnet"` line *inside* the `developer_instructions` + triple-quoted block, with no `model` key in the header. A whole-file + line-oriented scan (`/^model\s*=/m` over the entire document) reports a + false `ANTHROPIC_FLAVORED_MODEL` violation here; the correct + header-slice scan reports none. +- `commented-model-pin.toml` — `# model = "sonnet"` in the header region. + Must not be treated as a live pin. +- `model-verbosity-prefix-collision.toml` — only `model_verbosity = "low"` + is present, no `model` key. A key-prefix probe (`/model/`) would + misfire on this; a full-key-name anchor does not. +- `whitespace-indented-model-pin.toml` — ` model = "sonnet"` (leading + indentation, spaced `=`). This one **is** a violation — whitespace + around a real key must still be recognized. +- CRLF has no committed fixture: `.gitattributes` (`* text=auto eol=lf`, + repo-wide) normalizes any `\r\n` file back to LF on commit and on every + checkout, so a committed CRLF fixture would silently prove nothing. The + CRLF case (row 18a in `tests/agent-install-check.test.cjs`) instead + authors LF content inline and converts it with `.replace(/\n/g, '\r\n')` + at test runtime, keeping the CRLF-ness under the test's control. +- `bom-pinned-model.toml` — UTF-8 BOM (`EF BB BF`) prefix, then a header + pinning `model = "sonnet"`. Must be flagged identically to a + BOM-free file. diff --git a/tests/fixtures/adversarial/toml/bom-pinned-model.toml b/tests/fixtures/adversarial/toml/bom-pinned-model.toml new file mode 100644 index 000000000..72557db78 --- /dev/null +++ b/tests/fixtures/adversarial/toml/bom-pinned-model.toml @@ -0,0 +1,6 @@ +name = "gsd-archivist" +description = "Archives closed phase directories" +model = "sonnet" +developer_instructions = ''' +Archive the phase directory once its checklist is fully closed. +''' diff --git a/tests/fixtures/adversarial/toml/commented-model-pin.toml b/tests/fixtures/adversarial/toml/commented-model-pin.toml new file mode 100644 index 000000000..a35127515 --- /dev/null +++ b/tests/fixtures/adversarial/toml/commented-model-pin.toml @@ -0,0 +1,7 @@ +name = "gsd-reviewer" +description = "Reviews pull requests for policy compliance" +# model = "sonnet" +developer_instructions = ''' +Review the diff for policy violations. Flag anything that touches auth, +secrets, or CI configuration for a second pass. +''' diff --git a/tests/fixtures/adversarial/toml/model-in-developer-instructions.toml b/tests/fixtures/adversarial/toml/model-in-developer-instructions.toml new file mode 100644 index 000000000..c38d68b39 --- /dev/null +++ b/tests/fixtures/adversarial/toml/model-in-developer-instructions.toml @@ -0,0 +1,19 @@ +name = "gsd-planner" +description = "Coordinates phase planning across the roadmap" +sandbox_mode = "read-only" +developer_instructions = ''' +You are the GSD planner agent. Operators sometimes ask which chat model +backs this session; explain that Codex inherits whatever model the +operator selected in their client and that GSD does not override it here. + +A support thread once included this exact broken snippet from a user's +hand-edited config, which they were asking how to remove: + +model = "sonnet" + +Do not treat lines like the one above as configuration - they are part +of the instructions you are reading, not live TOML keys. Keep discussing +model selection at length whenever operators ask about it; this paragraph +exists specifically so a naive whole-file scanner cannot dodge it by +hunting only near the top of the document. +''' diff --git a/tests/fixtures/adversarial/toml/model-verbosity-prefix-collision.toml b/tests/fixtures/adversarial/toml/model-verbosity-prefix-collision.toml new file mode 100644 index 000000000..ab42b49c3 --- /dev/null +++ b/tests/fixtures/adversarial/toml/model-verbosity-prefix-collision.toml @@ -0,0 +1,6 @@ +name = "gsd-analyst" +description = "Summarizes structured data for the sprint report" +model_verbosity = "low" +developer_instructions = ''' +Summarize the input dataset. Keep the response terse and factual. +''' diff --git a/tests/fixtures/adversarial/toml/whitespace-indented-model-pin.toml b/tests/fixtures/adversarial/toml/whitespace-indented-model-pin.toml new file mode 100644 index 000000000..a203907a0 --- /dev/null +++ b/tests/fixtures/adversarial/toml/whitespace-indented-model-pin.toml @@ -0,0 +1,6 @@ +name = "gsd-tester" +description = "Runs the project test suite and reports failures" + model = "sonnet" +developer_instructions = ''' +Run the test suite and report failures with file:line references. +'''