diff --git a/.changeset/lucky-cats-romp.md b/.changeset/lucky-cats-romp.md new file mode 100644 index 000000000..752cb73cf --- /dev/null +++ b/.changeset/lucky-cats-romp.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3994 +--- +**Two QA oracles no longer report findings against the wrong field.** `routing-validity` demanded a live-command token from `recommended`, which is an action id by design, and also validated `recommended_command`, a field nothing in the repo produces; it now checks the fields that actually carry tokens. `value-hygiene` reported command tokens such as `/gsd:progress` as leaked absolute paths, and now exempts them by value shape rather than by key name, so a `command` field holding a genuine absolute path is still reported. (#3913) diff --git a/.changeset/sturdy-eagles-wave.md b/.changeset/sturdy-eagles-wave.md new file mode 100644 index 000000000..31b19c611 --- /dev/null +++ b/.changeset/sturdy-eagles-wave.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3994 +--- +**A generated exit-code reference at `docs/reference/exit-codes.md`.** Every registered exit code now has a page giving its number, name, meaning and owning band, alongside why `0` and `1` are unallocatable and why `3`-`13` are reserved by Node — so a `69` in a CI log has somewhere to be looked up. The page is generated from the same declaration the registry itself is built from and is `--check`-gated against drift. (#3913) diff --git a/docs/README.md b/docs/README.md index 3840f31bd..7499c90c3 100644 --- a/docs/README.md +++ b/docs/README.md @@ -95,6 +95,7 @@ Language versions: [English](README.md) · [Português (pt-BR)](pt-BR/README.md) - [Review and verification capabilities](reference/review-verification-capabilities.md) — code review, security, and Nyquist capability ownership and hook contracts - [Gate predicates](reference/gate-predicates.md) — canonical specification of the phase-gate predicate vocabulary - [Capability matrix](reference/capability-matrix.md) — generated catalogue of every capability's role, tier, extension points, hook kinds, and `engines.gsd` +- [Exit code reference](reference/exit-codes.md) — generated catalogue of every registered process exit code, its name, meaning, and owning module, plus the reserved bands and the v1/v2 exit contract - [Capability manifest](reference/capability-manifest.md) — the full `capability.json` schema and validation rules - [`gsd capability` command](reference/gsd-capability-command.md) — install / update / remove / list reference for third-party capabilities - [Workflow fragments](reference/workflow-fragments.md) — in-file `` marker grammar for fragmentizing workflow markdown at emission time diff --git a/docs/adr/3889-process-exit-contract.md b/docs/adr/3889-process-exit-contract.md index 404d38830..5810b1204 100644 --- a/docs/adr/3889-process-exit-contract.md +++ b/docs/adr/3889-process-exit-contract.md @@ -273,6 +273,56 @@ and it is the property ADR-2980's declined Option 3 lacked. `n/no-process-exit: 'off'` exemption block, and the duplicated `scripts/lib/cli-exit.cjs`; **added** one rule (`local/require-registered-exit`) plus the registry's own `--check`. Net **−2**. +> **Amendment — 2026-08-28: the ledger above is superseded (#3913).** +> +> Three of its terms did not survive contact with the code, and the net it states was never +> achievable. It is left in place unedited so the drift is legible; the measured ledger is below. +> +> 1. **`scripts/lib/cli-exit.cjs` was not deleted.** Phase 0 changed it from a duplicated module +> into a *generated* one, so the line retires a copy that still exists. The epic body was +> corrected to net **−1** at the time; this ADR's line was not. +> 2. **"their four baseline entries" was five** — four `soft-error-exit-zero` and one +> `untyped-success`, counted directly off `tests/qa/smell-baseline.json`. Issue #3913 repeated +> the figure of four, and so did the first recon pass of this phase. This is the second +> documented count in this epic to drift, after [ADR-2980](2980-payload-carried-error-is-a-degraded-result.md)'s +> 60-versus-64 `output({error})` sites. +> 3. **Only one of the two oracles was retired.** `soft-error-exit-zero` is deleted: its condition +> is now *declared* by the exit contract (`output({error})` → `DEGRADED`, 0 under v1 and 80 +> under v2), so an inert SMELL restating it is ledger inflation. `untyped-success` is **not** +> deleted — it asserts that a `KIND.PROSE` command exposes no typed surface, which has nothing +> to do with exit codes, and neither the registry nor `local/require-registered-exit` replaces +> it. The clause this ADR and #3913 both rely on — *"the registry and the ESLint rule enforce +> the same property by construction"* — is simply false for it. Rather than drop the property, +> it was **promoted from SMELL to VIOLATION**, so it can now fail a build, which it never could +> before (`runOracles`'s `get failed()` returns `violations` only). +> +> **Measured ledger for the epic as delivered:** −1 oracle (`soft-error-exit-zero`), −1 +> `n/no-process-exit: 'off'` exemption block, +1 rule (`local/require-registered-exit`), +1 registry +> `--check`, +1 generated-docs `--check` (`docs/reference/exit-codes.md`). One further guard changed +> strength rather than count: `untyped-success` SMELL → VIOLATION. Two mis-scoped oracles were +> corrected in passing (`routing-validity`, `value-hygiene`) — see #3913. +> +> **Net −1 by count**, not −4. +> +> The −4 first written here counted the five pruned `smell-baseline.json` entries in the same units +> as oracles and lint rules. They are not guards — they are *acknowledgements* that a guard fired. +> Removing them strengthens the surface rather than removing anything from it, and folding them into +> the count inflates the negative by five. An ADR about honest accounting should not pad its own +> ledger, so the entries are recorded as what they are and excluded from the count. +> +> **Two limits on how strong this is, stated rather than implied:** +> +> - **`soft-error-exit-zero`'s condition is now declared, not enforced.** No oracle references +> `KIND.SOFT_ERROR` after its deletion, and the corpus still contains four soft-error steps that +> nothing observes. Under `v1` — the default — those sites still exit `0`. The condition is +> modelled in the contract and projects to `80` only under the opt-in `v2`. That is a deliberate +> trade, not an equivalent replacement, and "the registry enforces the same property" would be too +> strong a claim for it. +> - **`untyped-success` is promoted against a corpus that no longer exercises the case.** Its +> `KIND.PROSE` count reached 0 because the sole prose-producing step was changed to +> `smart-entry --json`. The promotion is real and the guard now fails a build — but what it +> currently guards is that no *new* prose-only step appears, not that an existing one is caught. + ## Revisit if - Domain allocations exceed roughly a dozen. That means the generic four are wrong, not that the diff --git a/docs/reference/exit-codes.md b/docs/reference/exit-codes.md new file mode 100644 index 000000000..6062919b5 --- /dev/null +++ b/docs/reference/exit-codes.md @@ -0,0 +1,67 @@ +# Exit code reference + +> **Generated file — do not edit by hand.** +> This page is generated from the exit-code declaration +> (`gsd-core/bin/shared/exit-codes.json`) by `scripts/gen-exit-code-docs.cjs` +> and kept honest by a drift guard in `npm run lint:generated-sync` (which runs +> `node scripts/gen-exit-code-docs.cjs --check`). Any manual edit is overwritten +> on the next generation run. To register a new code, add an entry to the +> declaration and run `node scripts/gen-exit-code-registry.cjs --write && node +> scripts/gen-exit-code-docs.cjs --write`. + +See also: [ADR-3889 — one exit-code registry](../adr/3889-process-exit-contract.md) — +[Adopt the v2 exit contract](../how-to/adopt-the-v2-exit-contract.md) — +[JSON error mode](../json-errors.md) + +--- + +## Registered codes (6) + +Every process-level exit code `gsd-tools`, its hooks, and its scripts may terminate +with, by name, meaning, and the module that owns it. + +| code | name | meaning | owning module | authorized by | +|---|---|---|---|---| +| 2 | `HOOK_DENY` | Hook protocol deny — the harness blocks the tool call | `hook-adapter` | ADR-3889 | +| 64 | `USAGE` | Caller error — bad argv, unknown subcommand, missing argument | `generic` | ADR-3889 | +| 66 | `NO_INPUT` | Ran; zero units were in scope, and that emptiness is known to be genuine | `generic` | ADR-3889 | +| 69 | `UNAVAILABLE` | Could not run — prerequisite absent, input unreadable, scope unestablished | `generic` | ADR-3889 | +| 70 | `INTERNAL` | Self-failure — crash, timeout, killed subprocess | `generic` | ADR-3889 | +| 80 | `DEGRADED` | Ran to completion and is reporting a condition through its result payload rather than as a process failure | `gsd-tools` | ADR-3889 + ADR-2980 | + +--- + +## Reserved bands + +The registered codes above are not chosen freely — each falls inside one of a +fixed set of allocatable bands (ADR-3889 §1). A code outside these bands can +never be registered; validation rejects it before it reaches the tables above. +This is what makes an unfamiliar number in a CI log actionable: look up its +band first, then its registered name if it has one. + +| Band | Meaning | +|---|---| +| `0`–`1` | **Free — never allocatable.** `0` is the universal "succeeded" convention and `1` is the universal "failed, no further detail" convention across nearly every CLI ecosystem. Registering either here would collide with that universal meaning instead of adding a distinct, named signal — so the registry leaves both permanently unallocated. | +| `2` | Reserved exclusively to the Claude Code hook-protocol deny (`HOOK_DENY`) — owned by `hook-adapter` and no other module. | +| `3`–`13` | **Node-reserved.** Node.js itself assigns meaning to this range (e.g. internal JavaScript errors, fatal exceptions, invalid argument errors) before a GSD process ever gets a chance to project its own outcome. Allocating one of these would be indistinguishable from a Node-level failure the process never intended to report. | +| `14`–`63`, `79`, `126+` | Outside every allocatable band — not Node-reserved, but also not opened for GSD use. `126`+ additionally collides with the shell convention for "command not executable" / "signal N" (`128+N`), which a process exit code must never impersonate. | +| `64`–`78` | **Generic band.** Codes any module may use for caller-facing, non-domain-specific outcomes (bad argv, no input in scope, a missing prerequisite, an internal crash). | +| `80`–`125` | **Domain band.** Codes reserved for a specific product surface's own vocabulary — currently only `gsd-tools`' `DEGRADED` (a completed run reporting a condition through its payload rather than as a process failure). | + +## The v1/v2 exit contract + +ADR-3889 §4 adds a **version projection** on top of this registry, not a second +registry: every registered name above projects to the *same* code under both +contract versions, with one deliberate exception — `DEGRADED`. Under the +default, backward-compatible `v1` contract, a payload-carried error +(`output({error})`) still exits `0`, exactly as +[ADR-2980](../adr/2980-payload-carried-error-is-a-degraded-result.md) ratified +for the pre-existing call sites that already depended on that behavior — **64** +call sites across 9 modules per that ADR's own amendment (its original text +said ~60). Under the +opt-in `v2` contract, the same outcome exits `80` (`DEGRADED`) instead, so a +caller that wants to branch on the exit code alone — without parsing stdout — +can opt in without breaking every existing consumer. See +[Adopt the v2 exit contract](../how-to/adopt-the-v2-exit-contract.md) for how to +turn this on, and [JSON error mode](../json-errors.md) for the full fault vs. +degraded-result channel taxonomy this registry sits underneath. diff --git a/package.json b/package.json index 69b18742e..2ed09f069 100644 --- a/package.json +++ b/package.json @@ -108,7 +108,7 @@ "gen:registry": "node scripts/gen-registry.cjs --write", "gen:install-tree": "node scripts/gen-install-tree-fixtures.cjs", "gen:section-manifest": "node scripts/gen-section-manifest.cjs --write", - "regen:derived": "npm run build && npm run gen:registry && node scripts/gen-adr-index.cjs --write && node scripts/gen-features.cjs --write && node scripts/gen-capability-matrix.cjs --write && node scripts/gen-inventory-manifest.cjs --write && node scripts/gen-context-index.cjs --write && node scripts/gen-state-md-docs.cjs --write && npm run gen:section-manifest && node scripts/sync-manifest-versions.cjs && npm run gen:install-tree && node scripts/gen-scripts-cli-exit.cjs --write && node scripts/gen-hooks-cli-exit.cjs --write && node scripts/gen-exit-code-registry.cjs --write", + "regen:derived": "npm run build && npm run gen:registry && node scripts/gen-adr-index.cjs --write && node scripts/gen-features.cjs --write && node scripts/gen-capability-matrix.cjs --write && node scripts/gen-inventory-manifest.cjs --write && node scripts/gen-context-index.cjs --write && node scripts/gen-state-md-docs.cjs --write && npm run gen:section-manifest && node scripts/sync-manifest-versions.cjs && npm run gen:install-tree && node scripts/gen-scripts-cli-exit.cjs --write && node scripts/gen-hooks-cli-exit.cjs --write && node scripts/gen-exit-code-registry.cjs --write && node scripts/gen-exit-code-docs.cjs --write", "validate:registry": "node scripts/validate-registry.cjs", "prepack": "npm run build:lib", "prepare": "npm run build:lib", @@ -129,7 +129,7 @@ "lint:test-file-count": "node scripts/lint-test-file-count.cjs", "lint:pr-checks": "node scripts/lint-pr-check-project-dir.cjs", "lint:changeset": "node scripts/changeset/lint.cjs", - "lint:generated-sync": "node scripts/gen-capability-registry.cjs --check && node scripts/gen-loop-host-contract.cjs --check && node scripts/gen-capability-matrix.cjs --check && node scripts/sync-manifest-versions.cjs --check && node scripts/gen-inventory-manifest.cjs --check && node scripts/generate-package-identity.cjs --check && node scripts/gen-plugin-skills.cjs --check && node scripts/gen-registry.cjs --check && node scripts/gen-adr-index.cjs --check && node scripts/gen-features.cjs --check && node scripts/check-glossary-refs.cjs --check && node scripts/lint-compiled-artifact-sync.cjs --check && node scripts/gen-context-index.cjs --check && node scripts/gen-section-manifest.cjs --check && node scripts/gen-health-docs.cjs --check && node scripts/gen-state-md-docs.cjs --check && node scripts/gen-scripts-cli-exit.cjs --check && node scripts/gen-hooks-cli-exit.cjs --check && node scripts/gen-exit-code-registry.cjs --check", + "lint:generated-sync": "node scripts/gen-capability-registry.cjs --check && node scripts/gen-loop-host-contract.cjs --check && node scripts/gen-capability-matrix.cjs --check && node scripts/sync-manifest-versions.cjs --check && node scripts/gen-inventory-manifest.cjs --check && node scripts/generate-package-identity.cjs --check && node scripts/gen-plugin-skills.cjs --check && node scripts/gen-registry.cjs --check && node scripts/gen-adr-index.cjs --check && node scripts/gen-features.cjs --check && node scripts/check-glossary-refs.cjs --check && node scripts/lint-compiled-artifact-sync.cjs --check && node scripts/gen-context-index.cjs --check && node scripts/gen-section-manifest.cjs --check && node scripts/gen-health-docs.cjs --check && node scripts/gen-state-md-docs.cjs --check && node scripts/gen-scripts-cli-exit.cjs --check && node scripts/gen-hooks-cli-exit.cjs --check && node scripts/gen-exit-code-registry.cjs --check && node scripts/gen-exit-code-docs.cjs --check", "lint:docs": "node scripts/lint-docs-required.cjs", "lint:qa-smells": "node scripts/qa-smell-ratchet.cjs", "lint:legacy-name": "node scripts/lint-legacy-dir-name.cjs", diff --git a/scripts/docs-guard-registry.cjs b/scripts/docs-guard-registry.cjs index 6af045e52..768a8bc16 100644 --- a/scripts/docs-guard-registry.cjs +++ b/scripts/docs-guard-registry.cjs @@ -234,6 +234,9 @@ const DOCS_GUARD_TESTS = { 'docs/reference/host-integration-capability-matrix.md', ], 'tests/execute-phase-wave.test.cjs': ['docs/COMMANDS.md'], + // #3913 (ADR-3889 terminal phase): reads the generated docs/reference/exit-codes.md + // (content invariants, F1/F3) and docs/README.md (F4, the index link). + 'tests/exit-code-registry.test.cjs': ['docs/reference/exit-codes.md', 'docs/README.md'], 'tests/external-job-waiting.test.cjs': ['docs/reference/planning-artifacts.md'], // Rows 8/9 (negative controls) read real docs/registries/eos.json and // docs/adr/0001-dispatch-policy-module.md and assert on their EXACT diff --git a/scripts/gen-exit-code-docs.cjs b/scripts/gen-exit-code-docs.cjs new file mode 100644 index 000000000..0cb2e610f --- /dev/null +++ b/scripts/gen-exit-code-docs.cjs @@ -0,0 +1,318 @@ +#!/usr/bin/env node +'use strict'; + +/** + * gen-exit-code-docs.cjs — ADR-3889 (#3913), the terminal phase of the exit- + * code-registry epic. + * + * Generates docs/reference/exit-codes.md FROM the exit-code declaration + * (gsd-core/bin/shared/exit-codes.json), so the human-facing reference page + * can never drift from the actual registered codes — the same allocator-less + * failure mode this epic exists to close, now closed for the doc surface too. + * Modelled directly on scripts/gen-capability-matrix.cjs -> + * docs/reference/capability-matrix.md: same --check/--write/stdout modes, + * same "generated, do not edit" banner convention, same normalize-then- + * byte-compare drift check. + * + * The declaration JSON is read directly (not the compiled + * gsd-core/bin/lib/exit-code-registry.cjs artifact, which is gitignored tsc + * output) so this generator — like scripts/gen-exit-code-registry.cjs itself — + * works on an unbuilt clone. The "Reserved bands" table below is DERIVED, + * not retyped: its ranges and allocatable/reserved status come from + * `classifyBand`/`computeBandRanges`, which are themselves composed + * entirely from `isAllocatableCode`/`bandFor` in + * scripts/gen-exit-code-registry.cjs (all four imported below). Only the + * per-band RATIONALE prose (BAND_PROSE) is hand-authored; widening or + * narrowing a band in gen-exit-code-registry.cjs changes the ranges this + * page renders, with no second literal to keep in sync. + * + * Usage: + * node scripts/gen-exit-code-docs.cjs # print to stdout + * node scripts/gen-exit-code-docs.cjs --write # write the committed file + * node scripts/gen-exit-code-docs.cjs --check # exit 1 if the committed file is stale + * node scripts/gen-exit-code-docs.cjs --declaration --out # override for tests + */ + +const fs = require('fs'); +const path = require('path'); +const { ExitError, runMain } = require('./lib/cli-exit.cjs'); +const { + DEFAULT_DECLARATION_PATH, + loadDeclaration, + validateEntries, + computeBandRanges, + BANDS, +} = require('./gen-exit-code-registry.cjs'); + +const ROOT = path.resolve(__dirname, '..'); +const DOC_PATH = path.join(ROOT, 'docs', 'reference', 'exit-codes.md'); + +/** + * How far above the highest defined band (125, the top of 'domain') to scan + * when deriving band ranges. Large enough to prove the 'shell-signal' + * (126+) run is genuinely open-ended (classifyBand is constant well past + * it), not just an artifact of stopping the scan too early. + */ +const BAND_SCAN_MAX = 500; + +/** Hand-authored rationale prose per band category — the only part of the + * "Reserved bands" table that stays authored; the ranges themselves are + * derived by computeBandRanges from isAllocatableCode/bandFor. */ +const BAND_PROSE = Object.freeze({ + free: '**Free — never allocatable.** `0` is the universal "succeeded" convention and `1` is the universal "failed, no further detail" convention across nearly every CLI ecosystem. Registering either here would collide with that universal meaning instead of adding a distinct, named signal — so the registry leaves both permanently unallocated.', + 'hook-only': 'Reserved exclusively to the Claude Code hook-protocol deny (`HOOK_DENY`) — owned by `hook-adapter` and no other module.', + 'node-reserved': '**Node-reserved.** Node.js itself assigns meaning to this range (e.g. internal JavaScript errors, fatal exceptions, invalid argument errors) before a GSD process ever gets a chance to project its own outcome. Allocating one of these would be indistinguishable from a Node-level failure the process never intended to report.', + 'outside-every-band': 'Outside every allocatable band — not Node-reserved, but also not opened for GSD use. `126`+ additionally collides with the shell convention for "command not executable" / "signal N" (`128+N`), which a process exit code must never impersonate.', + generic: '**Generic band.** Codes any module may use for caller-facing, non-domain-specific outcomes (bad argv, no input in scope, a missing prerequisite, an internal crash).', + domain: '**Domain band.** Codes reserved for a specific product surface\'s own vocabulary — currently only `gsd-tools`\' `DEGRADED` (a completed run reporting a condition through its payload rather than as a process failure).', +}); + +/** + * The 'shell-signal' category (126+) is folded into the SAME rendered row + * as 'outside-every-band' (both read "not opened for GSD use" to a reader — + * the original hand-authored table merged them into one row, and the prose + * above documents the 126+ collision inline). Every other category gets its + * own row, in this fixed display order. + */ +const CATEGORY_ROW_ORDER = ['free', 'hook-only', 'node-reserved', 'outside-every-band', 'generic', 'domain']; +const CATEGORY_MERGE_INTO = Object.freeze({ 'shell-signal': 'outside-every-band' }); + +/** + * Guard against category-set drift across the three authored lists the + * "Reserved bands" table depends on: BANDS (gen-exit-code-registry.cjs — the + * single source of every category classifyBand can ever return), + * CATEGORY_ROW_ORDER (the rendered row order here), and BAND_PROSE (the + * hand-authored rationale text per row). Nothing type-checks that these three + * agree, so adding a BANDS category with no row/prose entry silently drops it + * from the table, and a row with no prose entry renders the literal string + * "undefined" — see #3913 P9 review finding (unchecked `BAND_PROSE[category]` + * lookup) and its root cause: three authored lists, only one of them derived. + * Throws loudly the first time any of the three sets diverge instead of + * silently omitting a row or rendering `undefined`. + */ +function assertBandCategoriesConsistent() { + // Every category classifyBand can ever return: each BANDS entry's own + // category, plus the synthetic 'outside-every-band' residual classifyBand + // falls back to for a code no BANDS entry claims (14-63, 79). + const producedCategories = new Set([...BANDS.map((b) => b.category), 'outside-every-band']); + + // 1. Every produced category must resolve — directly, or via + // CATEGORY_MERGE_INTO — to a row this page actually renders. Otherwise + // it is silently dropped from the table (the #3913 P9 finding). + for (const category of producedCategories) { + const rowCategory = CATEGORY_MERGE_INTO[category] || category; + if (!CATEGORY_ROW_ORDER.includes(rowCategory)) { + throw new ExitError(1, `fail_band_category_unaccounted: BANDS category "${category}" resolves to row ` + + `"${rowCategory}", which is in neither CATEGORY_ROW_ORDER nor a CATEGORY_MERGE_INTO target — add it to ` + + 'one or the other in scripts/gen-exit-code-docs.cjs.'); + } + } + + // 2. Every rendered row must have hand-authored rationale prose — otherwise + // the table cell literally renders the string "undefined". + for (const category of CATEGORY_ROW_ORDER) { + if (!(category in BAND_PROSE)) { + throw new ExitError(1, `fail_band_prose_missing: CATEGORY_ROW_ORDER category "${category}" has no ` + + 'BAND_PROSE entry — it would render the literal string "undefined" in the generated table.'); + } + } + + // 3. Reverse drift: a CATEGORY_ROW_ORDER or BAND_PROSE entry naming a + // category no BANDS entry (nor the 'outside-every-band' residual) + // produces is stale prose for a band that no longer exists. + const resolvedRowCategories = new Set( + [...producedCategories].map((category) => CATEGORY_MERGE_INTO[category] || category), + ); + for (const category of CATEGORY_ROW_ORDER) { + if (!resolvedRowCategories.has(category)) { + throw new ExitError(1, `fail_band_category_stale: CATEGORY_ROW_ORDER names "${category}", which no BANDS ` + + 'entry (directly, or via CATEGORY_MERGE_INTO) produces — remove it, or fix the drift between BANDS in ' + + 'scripts/gen-exit-code-registry.cjs and CATEGORY_MERGE_INTO here.'); + } + } + for (const category of Object.keys(BAND_PROSE)) { + if (!CATEGORY_ROW_ORDER.includes(category)) { + throw new ExitError(1, `fail_band_category_stale: BAND_PROSE has an entry for "${category}", which is not ` + + 'in CATEGORY_ROW_ORDER — remove the stale prose, or add the row.'); + } + } +} + +/** Format one contiguous range as a Markdown-table-cell token. */ +function formatRange(range) { + if (range.openEnded) return `\`${range.start}+\``; + if (range.start === range.end) return `\`${range.start}\``; + return `\`${range.start}\`–\`${range.end}\``; +} + +/** + * Render the "Reserved bands" table. Ranges come from computeBandRanges + * (itself derived from isAllocatableCode/bandFor); only BAND_PROSE and the + * row grouping/order above are authored. + */ +function renderBandTable() { + assertBandCategoriesConsistent(); + + const byCategory = new Map(); + for (const { category, ranges } of computeBandRanges(BAND_SCAN_MAX)) { + const rowCategory = CATEGORY_MERGE_INTO[category] || category; + if (!byCategory.has(rowCategory)) byCategory.set(rowCategory, []); + byCategory.get(rowCategory).push(...ranges); + } + + const rows = CATEGORY_ROW_ORDER.map((category) => { + const ranges = byCategory.get(category) || []; + const label = ranges.map(formatRange).join(', '); + return `| ${label} | ${BAND_PROSE[category]} |`; + }); + + return ['| Band | Meaning |', '|---|---|', ...rows].join('\n'); +} + +/** Load + validate the declaration, throwing (loud) on anything malformed — mirrors gen-exit-code-registry.cjs's own gate. */ +function loadEntries(declarationPath) { + const loaded = loadDeclaration(declarationPath); + if (!loaded.ok) { + throw new ExitError(1, `${loaded.reason}: ${loaded.message}`); + } + const validated = validateEntries(loaded.entries); + if (!validated.ok) { + throw new ExitError(1, `${validated.reason}: ${validated.message}`); + } + return loaded.entries; +} + +function renderRegisteredTable(entries) { + const rows = [...entries] + .sort((a, b) => a.code - b.code) + .map((e) => `| ${e.code} | \`${e.name}\` | ${e.meaning} | \`${e.owner}\` | ${e.authorizedBy} |`); + return [ + '| code | name | meaning | owning module | authorized by |', + '|---|---|---|---|---|', + ...rows, + ].join('\n'); +} + +function buildDoc(entries) { + const registeredTable = renderRegisteredTable(entries); + const registeredCount = entries.length; + const bandTable = renderBandTable(); + + return `# Exit code reference + +> **Generated file — do not edit by hand.** +> This page is generated from the exit-code declaration +> (\`gsd-core/bin/shared/exit-codes.json\`) by \`scripts/gen-exit-code-docs.cjs\` +> and kept honest by a drift guard in \`npm run lint:generated-sync\` (which runs +> \`node scripts/gen-exit-code-docs.cjs --check\`). Any manual edit is overwritten +> on the next generation run. To register a new code, add an entry to the +> declaration and run \`node scripts/gen-exit-code-registry.cjs --write && node +> scripts/gen-exit-code-docs.cjs --write\`. + +See also: [ADR-3889 — one exit-code registry](../adr/3889-process-exit-contract.md) — +[Adopt the v2 exit contract](../how-to/adopt-the-v2-exit-contract.md) — +[JSON error mode](../json-errors.md) + +--- + +## Registered codes (${registeredCount}) + +Every process-level exit code \`gsd-tools\`, its hooks, and its scripts may terminate +with, by name, meaning, and the module that owns it. + +${registeredTable} + +--- + +## Reserved bands + +The registered codes above are not chosen freely — each falls inside one of a +fixed set of allocatable bands (ADR-3889 §1). A code outside these bands can +never be registered; validation rejects it before it reaches the tables above. +This is what makes an unfamiliar number in a CI log actionable: look up its +band first, then its registered name if it has one. + +${bandTable} + +## The v1/v2 exit contract + +ADR-3889 §4 adds a **version projection** on top of this registry, not a second +registry: every registered name above projects to the *same* code under both +contract versions, with one deliberate exception — \`DEGRADED\`. Under the +default, backward-compatible \`v1\` contract, a payload-carried error +(\`output({error})\`) still exits \`0\`, exactly as +[ADR-2980](../adr/2980-payload-carried-error-is-a-degraded-result.md) ratified +for the pre-existing call sites that already depended on that behavior — **64** +call sites across 9 modules per that ADR's own amendment (its original text +said ~60). Under the +opt-in \`v2\` contract, the same outcome exits \`80\` (\`DEGRADED\`) instead, so a +caller that wants to branch on the exit code alone — without parsing stdout — +can opt in without breaking every existing consumer. See +[Adopt the v2 exit contract](../how-to/adopt-the-v2-exit-contract.md) for how to +turn this on, and [JSON error mode](../json-errors.md) for the full fault vs. +degraded-result channel taxonomy this registry sits underneath. +`; +} + +/** Normalize CRLF→LF + ensure a single trailing newline, for cross-platform compare. */ +function normalize(s) { + return s.replace(/\r\n/g, '\n').replace(/\n+$/, '\n'); +} + +/** + * Parse the small flag set this CLI accepts. `--declaration`/`--out` exist + * only so tests can redirect both the source and the target to a tmpdir + * without ever touching the real committed declaration or doc page — the + * same reason scripts/gen-exit-code-registry.cjs and + * scripts/gen-capability-matrix.cjs's sibling generators take path overrides. + */ +function parseArgs(argv) { + let mode = null; + let declarationPath = null; + let outPath = null; + for (let i = 0; i < argv.length; i++) { + const arg = argv[i]; + if (arg === '--check' || arg === '--write') { + mode = arg.slice(2); + } else if (arg === '--declaration') { + declarationPath = argv[++i]; + } else if (arg === '--out') { + outPath = argv[++i]; + } else { + throw new ExitError(1, `unrecognized argument: ${arg}`); + } + } + return { mode, declarationPath, outPath }; +} + +function main() { + const { mode, declarationPath, outPath } = parseArgs(process.argv.slice(2)); + const docPath = outPath || DOC_PATH; + const entries = loadEntries(declarationPath || DEFAULT_DECLARATION_PATH); + const content = buildDoc(entries); + + if (mode === 'check') { + let committed; + try { + committed = fs.readFileSync(docPath, 'utf8'); + } catch { + throw new ExitError(1, `${path.relative(ROOT, docPath)} is missing. Run:\n node scripts/gen-exit-code-docs.cjs --write`); + } + if (normalize(committed) !== normalize(content)) { + throw new ExitError(1, `${path.relative(ROOT, docPath)} is stale. Run:\n node scripts/gen-exit-code-docs.cjs --write`); + } + console.log(`${path.relative(ROOT, docPath)} is up to date.`); + return; + } + if (mode === 'write') { + fs.mkdirSync(path.dirname(docPath), { recursive: true }); + fs.writeFileSync(docPath, content, 'utf8'); + console.log(`Wrote ${path.relative(ROOT, docPath)}`); + return; + } + process.stdout.write(content); +} + +if (require.main === module) runMain(main); + +module.exports = { buildDoc, loadEntries, DOC_PATH }; diff --git a/scripts/gen-exit-code-registry.cjs b/scripts/gen-exit-code-registry.cjs index a5695f723..24ca69394 100644 --- a/scripts/gen-exit-code-registry.cjs +++ b/scripts/gen-exit-code-registry.cjs @@ -95,6 +95,7 @@ const REASON = Object.freeze({ RESERVED_CODE: 'fail_reserved_code', FORBIDDEN_OWNER: 'fail_forbidden_owner', MISSING_ARTIFACT: 'fail_missing_artifact', + INVALID_CHARACTERS: 'fail_invalid_characters', }); const USAGE_MESSAGE = [ @@ -117,6 +118,39 @@ const NAME_RE = /^[A-Z][A-Z0-9_]*$/; /** Fields every entry must carry as a non-empty, non-whitespace-only string. */ const REQUIRED_STRING_FIELDS = ['meaning', 'owner', 'authorizedBy']; +/** + * Characters forbidden in any declaration string field: a literal pipe `|` + * (the Markdown table cell delimiter `gen-exit-code-docs.cjs`'s + * `renderRegisteredTable` interpolates these fields into — an unescaped `|` + * breaks the row and everything after it lands verbatim in the rendered + * page, including a forged Markdown heading), and any C0 control character + * (`\x00`-`\x1F`, `\x7F`) — which subsumes CR (`\r`) and LF (`\n`), both of + * which would otherwise let a single declaration entry inject an entire + * extra row (or non-table content) into the generated table. + * + * Enforced HERE, at the validator both generators share (`gen-exit-code-registry.cjs`'s + * `validateEntry`, called by `gen-exit-code-docs.cjs`'s `loadEntries`), not + * at the docs renderer — failing closed at the declaration source is + * correct: an escaping fix at render time would let a malformed declaration + * through validation and only cosmetically repair the symptom (#3913 P9 + * SEC-3). + * + * Checked via char codes rather than a literal control-character regex + * range — same approach as `scripts/registry-schema.cjs`'s + * `hasDisallowedControlChar` — so this never trips ESLint's `no-control-regex`. + * + * @param {string} v + * @returns {boolean} + */ +function hasForbiddenDeclarationChar(v) { + for (let i = 0; i < v.length; i += 1) { + const code = v.charCodeAt(i); + if (code === 0x7c) return true; // '|' + if (code < 0x20 || code === 0x7f) return true; // C0 control chars (incl. CR/LF/tab) + DEL + } + return false; +} + /** * Bands, per ADR-3889 §1: * 0, 1 free (not allocatable here) @@ -125,26 +159,44 @@ const REQUIRED_STRING_FIELDS = ['meaning', 'owner', 'authorizedBy']; * 14-63, 79, 126+ outside every band * 64-78 generic * 80-125 domain + * + * SINGLE SOURCE for every band boundary in this file: `isAllocatableCode`, + * `bandFor`, and (via scripts/gen-exit-code-docs.cjs's `classifyBand`) the + * generated "Reserved bands" table are ALL derived from this one ordered + * list of `{category, allocatable, test}` predicates — there is no second + * hand-typed range anywhere else. Widening or narrowing a band means editing + * a `test` function here; every consumer (validation, the docs page) picks + * that change up with nothing else to keep in sync. */ +const BANDS = Object.freeze([ + { category: 'free', allocatable: false, test: (code) => code === 0 || code === 1 }, + { category: 'hook-only', allocatable: true, test: (code) => code === 2 }, + { category: 'node-reserved', allocatable: false, test: (code) => code >= 3 && code <= 13 }, + { category: 'generic', allocatable: true, test: (code) => code >= 64 && code <= 78 }, + { category: 'domain', allocatable: true, test: (code) => code >= 80 && code <= 125 }, + { category: 'shell-signal', allocatable: false, test: (code) => code >= 126 }, +]); + +/** The band a code falls into, or `null` for the residual "outside every band" gap (14-63, 79) that no BANDS entry above claims. */ +function bandEntryFor(code) { + return BANDS.find((band) => band.test(code)) || null; +} + function isAllocatableCode(code) { - if (code === 2) return true; - if (code >= 64 && code <= 78) return true; - if (code >= 80 && code <= 125) return true; - return false; + const band = bandEntryFor(code); + return band !== null && band.allocatable; } /** * Label the non-allocatable band a rejected code falls into, per the same - * range boundaries documented on isAllocatableCode/ADR-3889 §1. Only called - * for codes that already failed isAllocatableCode, so 2 and 64-125 never - * reach here. + * BANDS table isAllocatableCode reads. Only called for codes that already + * failed isAllocatableCode, so an allocatable band's category is never + * returned here. * @returns {string} */ function bandFor(code) { - if (code === 0 || code === 1) return 'free'; - if (code >= 3 && code <= 13) return 'node-reserved'; - if (code >= 126) return 'shell-signal'; - return 'outside-every-band'; // 14-63, 79 + const band = bandEntryFor(code); + return band ? band.category : 'outside-every-band'; // 14-63, 79 } /** @@ -190,6 +242,15 @@ function validateEntry(entry, index) { context: { field, index, code, name }, }; } + if (hasForbiddenDeclarationChar(value)) { + return { + ok: false, + reason: REASON.INVALID_CHARACTERS, + message: `entry[${index}] (${name}).${field} must not contain a "|", CR, LF, or other control character ` + + `(these are interpolated into a Markdown table cell by gen-exit-code-docs.cjs), received ${JSON.stringify(value)}`, + context: { field, index, code, name }, + }; + } } if (!isAllocatableCode(code)) { @@ -737,6 +798,70 @@ function main() { if (require.main === module) process.exitCode = main(); +/** + * Classify a single code into one of the band categories the docs page + * renders a row for. Reads the SAME `bandEntryFor`/BANDS table that + * `isAllocatableCode`/`bandFor` are themselves derived from — there is no + * second, independently-typed band boundary anywhere in this classification, + * so widening or narrowing a band (editing a `test` in BANDS) changes what + * this returns too, with nothing else to keep in sync. + * @param {number} code + * @returns {'free'|'hook-only'|'node-reserved'|'outside-every-band'|'generic'|'domain'|'shell-signal'} + */ +function classifyBand(code) { + const band = bandEntryFor(code); + return band ? band.category : 'outside-every-band'; // 14-63, 79 +} + +/** + * Scan the code space [0, maxCode] and group it into maximal contiguous + * runs of the same classifyBand() category, in the order those categories + * first appear. The run touching `maxCode` is marked `openEnded: true` for + * any category whose classification never changes past that point + * (currently only 'shell-signal', since bandFor(code >= 126) is constant), + * so the docs renderer can print it as `126+` instead of a false upper + * bound. This is what makes the docs page's band table a DERIVED artifact: + * widening a band in isAllocatableCode/bandFor changes what this function + * returns, which changes the rendered table, with no second literal to + * keep in sync. + * @param {number} maxCode + * @returns {Array<{category:string, ranges:Array<{start:number,end:number,openEnded:boolean}>}>} + */ +function computeBandRanges(maxCode) { + /** @type {Map>} */ + const byCategory = new Map(); + const order = []; + + let runCategory = null; + let runStart = null; + for (let code = 0; code <= maxCode; code += 1) { + const category = classifyBand(code); + if (category !== runCategory) { + if (runCategory !== null) { + pushRun(byCategory, order, runCategory, runStart, code - 1, false); + } + runCategory = category; + runStart = code; + } + } + // Close the final run. It is open-ended (unbounded above) exactly when its + // category classification is constant for every code beyond maxCode too — + // true today only for 'shell-signal', since bandFor treats every code + // >= 126 identically with no further upper boundary. + const openEnded = classifyBand(maxCode) === classifyBand(maxCode + 1); + pushRun(byCategory, order, runCategory, runStart, maxCode, openEnded); + + return order.map((category) => ({ category, ranges: byCategory.get(category) })); +} + +function pushRun(byCategory, order, category, start, end, openEnded) { + if (!byCategory.has(category)) { + byCategory.set(category, []); + order.push(category); + } + byCategory.get(category).push({ start, end, openEnded }); +} + module.exports = { REASON, USAGE_MESSAGE, @@ -747,8 +872,13 @@ module.exports = { DEFAULT_DTS_OUTPUT_PATH, DEFAULT_SH_OUTPUT_PATH, ENTRY_FIELD_TYPES, + BANDS, + bandEntryFor, isAllocatableCode, bandFor, + classifyBand, + computeBandRanges, + hasForbiddenDeclarationChar, validateEntry, validateEntries, loadDeclaration, diff --git a/tests/exit-code-registry.test.cjs b/tests/exit-code-registry.test.cjs index d27dc63ab..4aecb539c 100644 --- a/tests/exit-code-registry.test.cjs +++ b/tests/exit-code-registry.test.cjs @@ -29,6 +29,7 @@ const { runNode } = require('./helpers/process-seam.cjs'); const { createTempDir, cleanup } = require('./helpers.cjs'); const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const fc = require('./helpers/fast-check-setup.cjs'); +const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); const REPO_ROOT = path.resolve(__dirname, '..'); const GEN_SCRIPT = path.join(REPO_ROOT, 'scripts', 'gen-exit-code-registry.cjs'); @@ -205,6 +206,7 @@ describe('gen-exit-code-registry: REASON', () => { 'OK', 'DRIFTED', 'USAGE', 'MISSING_DECLARATION', 'MALFORMED_DECLARATION', 'NOT_AN_ARRAY', 'EMPTY_DECLARATION', 'INVALID_ENTRY', 'DUPLICATE_CODE', 'DUPLICATE_NAME', 'RESERVED_CODE', 'FORBIDDEN_OWNER', 'MISSING_ARTIFACT', + 'INVALID_CHARACTERS', ]; test('is frozen', () => { @@ -302,6 +304,49 @@ describe('gen-exit-code-registry: required string fields', () => { }); }); +// #3913 P9 SEC-3: a declaration string field carrying a `|`, CR, LF, or other +// control character breaks the Markdown table gen-exit-code-docs.cjs +// interpolates it into (a `|` splits the row; a `\n` can forge an entire +// extra row, including a fake Markdown heading). Rejected at the shared +// validator (validateEntry), not the renderer, so both generators inherit +// the fix. +describe('gen-exit-code-registry: forbidden characters in declaration string fields (#3913 P9 SEC-3)', () => { + const fields = ['meaning', 'owner', 'authorizedBy']; + const badValues = [ + ['a literal pipe', 'contains | a pipe'], + ['a CR', 'contains\ra CR'], + ['a LF', 'contains\na LF'], + ['a CRLF', 'contains\r\na CRLF'], + ['a NUL byte', 'contains\x00a NUL'], + ['a DEL byte', 'contains\x7fa DEL'], + ]; + + for (const field of fields) { + for (const [label, bad] of badValues) { + test(`"${field}" containing ${label} -> INVALID_CHARACTERS`, () => { + // F3a: failing-first against the pre-fix validator, this entry would + // have passed validateEntry entirely (no character check existed). + const entry = makeEntry({ [field]: bad }); + const result = generator.validateEntry(entry, 0); + assert.equal(result.ok, false); + assert.equal(result.reason, generator.REASON.INVALID_CHARACTERS); + }); + } + } + + test('a clean value with none of the forbidden characters is accepted', () => { + const result = generator.validateEntry(makeEntry({ meaning: 'a perfectly normal meaning, with commas' }), 0); + assert.deepEqual(result, { ok: true }); + }); + + test('hasForbiddenDeclarationChar is the exact predicate validateEntry uses (no drift)', () => { + assert.equal(generator.hasForbiddenDeclarationChar('clean'), false); + assert.equal(generator.hasForbiddenDeclarationChar('a | pipe'), true); + assert.equal(generator.hasForbiddenDeclarationChar('a\nnewline'), true); + assert.equal(generator.hasForbiddenDeclarationChar('a\rreturn'), true); + }); +}); + describe('gen-exit-code-registry: cross-entry invariants', () => { test('duplicate code -> DUPLICATE_CODE, context carries the code and both names', () => { const entries = [ @@ -639,6 +684,10 @@ describe('gen-exit-code-registry: CLI positive controls for the ten guard rows', ['code "64" (string)', () => [{ ...validBase(), code: '64', name: 'STRCODE' }], 'INVALID_ENTRY'], ['missing meaning', () => [{ code: 64, name: 'NO_MEANING', owner: 'generic', authorizedBy: 'ADR-3889' }], 'INVALID_ENTRY'], ['[] empty declaration', () => [], 'EMPTY_DECLARATION'], + // F3a: a `meaning` carrying a `|` or a newline must be REJECTED with a non-zero exit — + // failing-first against the pre-fix validator (#3913 P9 SEC-3). + ['meaning with a pipe', () => [{ ...validBase(), code: 64, name: 'PIPE_MEANING', meaning: 'a | pipe breaks the table' }], 'INVALID_CHARACTERS'], + ['meaning with a newline', () => [{ ...validBase(), code: 64, name: 'NEWLINE_MEANING', meaning: 'a\nforged heading' }], 'INVALID_CHARACTERS'], ]; for (const [label, buildEntries, expectedReasonKey] of rows) { @@ -668,6 +717,291 @@ describe('gen-exit-code-registry: CLI positive controls for the ten guard rows', }); }); +// ── gen-exit-code-docs.cjs: docs/reference/exit-codes.md (P9, #3913, matrix F) ── +describe('gen-exit-code-docs: generated exit-code reference page (matrix F1-F4)', () => { + const DOCS_GEN_SCRIPT = path.join(REPO_ROOT, 'scripts', 'gen-exit-code-docs.cjs'); + const REAL_DOC_PATH = path.join(REPO_ROOT, 'docs', 'reference', 'exit-codes.md'); + const README_PATH = path.join(REPO_ROOT, 'docs', 'README.md'); + const docsGenerator = require(DOCS_GEN_SCRIPT); + + function runDocsGen(args, opts = {}) { + return runNode([DOCS_GEN_SCRIPT, ...args], { timeoutMs: PROBE_TIMEOUT_MS, ...opts }); + } + + // F1: every code in the registry appears in the generated page with its + // name — asserted over the ENUMERATED registry, so a newly allocated code + // fails until documented. + test('F1: every registered code appears in the generated page with its name', () => { + const md = fs.readFileSync(REAL_DOC_PATH, 'utf8'); + assert.ok(registry.EXIT_CODES.length > 0, 'precondition: the registry is non-empty'); + for (const entry of registry.EXIT_CODES) { + const row = md.split(/\r?\n/).find((l) => l.startsWith(`| ${entry.code} |`)); + assert.ok(row, `code ${entry.code} (${entry.name}) must appear as a row in the generated page`); + assert.ok(row.includes(`\`${entry.name}\``), `code ${entry.code}'s row must carry its name ${entry.name}`); + } + }); + + // F2: `--check` exits non-zero when the committed page diverges from a + // fresh render. Proven by mutating a COPY (via --declaration/--out + // redirection to a tmpdir, never the real committed file — test files in + // this repo run in parallel) and observing the non-zero exit — not by + // reading the code. + describe('F2: --check catches drift (mutate-then-observe, not read-the-code)', () => { + let tmpDir; + before(() => { + tmpDir = createTempDir('gsd-exit-code-docs-f2-'); + }); + after(() => { + cleanup(tmpDir); + }); + + test('a freshly generated copy of the real committed page passes --check', () => { + const decl = path.join(tmpDir, 'decl.json'); + fs.copyFileSync(REAL_DECLARATION_PATH, decl); + const out = path.join(tmpDir, 'exit-codes.md'); + const write = runDocsGen(['--write', '--declaration', decl, '--out', out]); + assert.equal(write.exitCode, 0, write.stderr); + const check = runDocsGen(['--check', '--declaration', decl, '--out', out]); + assert.equal(check.exitCode, 0, check.stderr); + }); + + test('mutating the generated page then running --check exits non-zero', () => { + const decl = path.join(tmpDir, 'decl-mutate.json'); + fs.copyFileSync(REAL_DECLARATION_PATH, decl); + const out = path.join(tmpDir, 'exit-codes-mutate.md'); + assert.equal(runDocsGen(['--write', '--declaration', decl, '--out', out]).exitCode, 0); + + // Mutate the generated copy — e.g. a hand-edit drifting from the + // generator's own output — then observe the ACTUAL exit code. + fs.appendFileSync(out, '\n\n'); + const check = runDocsGen(['--check', '--declaration', decl, '--out', out]); + assert.notEqual(check.exitCode, 0, 'a drifted page must fail --check, not pass it'); + }); + }); + + // F3: `--check` exits 0 on the committed tree (the generator is idempotent). + test('F3: --check exits 0 against the real committed page', () => { + const result = runDocsGen(['--check']); + assert.equal(result.exitCode, 0, result.stderr); + }); + + // F4: the page is reachable from docs/README.md. + test('F4: docs/README.md links to the generated exit-code reference page', () => { + const readme = fs.readFileSync(README_PATH, 'utf8'); + assert.ok(readme.includes('reference/exit-codes.md'), 'docs/README.md must index docs/reference/exit-codes.md'); + }); + + // Real parity assertion (replaces a near-tautological pair of `.includes()` + // checks — `md.includes('3')` matches "ADR-3889", not the Node-reserved + // band). Enumerates the FULL scanned code space and asserts the rendered + // "Reserved bands" table's ranges and row grouping are exactly what + // computeBandRanges/classifyBand — themselves composed only from + // isAllocatableCode/bandFor — return today. This fails the moment the + // generator's band table is re-hardcoded as a literal that stops tracking + // gen-exit-code-registry.cjs's own band logic. + test('parity: every rendered band range and status is DERIVED from isAllocatableCode/bandFor, not retyped', () => { + const md = fs.readFileSync(REAL_DOC_PATH, 'utf8'); + const ranges = generator.computeBandRanges(500); + for (const { category, ranges: subRanges } of ranges) { + for (const range of subRanges) { + // Every individual code in every derived range must actually + // classify into that category right now — i.e. the derivation is + // self-consistent over the enumerated space, not just internally + // coherent by construction. + for (let code = range.start; code <= range.end; code += 1) { + assert.equal(generator.classifyBand(code), category, `code ${code} must classify as ${category}`); + } + // And the rendered page must actually contain a band-table token + // for this range's boundary (its start or its formatted label), + // so a hand-edited/stale table (the #1 defect: a literal that + // never changed when the band logic did) is caught here too. + const label = range.openEnded ? `${range.start}+` : (range.start === range.end ? `${range.start}` : `${range.start}\`–\`${range.end}`); + assert.ok(md.includes(`\`${label}\``), `rendered page must contain the derived band label for ${category}: \`${label}\``); + } + } + }); + + // Regression for #1: proves the band table is genuinely DERIVED from + // isAllocatableCode/bandFor rather than a second hand-typed literal. Widens + // the band logic in a SCRATCH COPY of both generator scripts (never the + // real committed files) so that 14-63 becomes allocatable, then runs + // `--check` against the REAL committed page with that widened logic. If + // the band table were still hand-typed (the pre-fix defect), --check would + // stay green because the template string never changed; with the fix, the + // freshly-rendered table for the widened logic diverges from the committed + // page's band table and --check must exit non-zero. + test('band table is DERIVED: widening isAllocatableCode without regenerating fails --check (regression, #1)', () => { + const tmp = createTempDir('gsd-exit-code-docs-band-parity-'); + try { + const registrySrc = fs.readFileSync(GEN_SCRIPT, 'utf8'); + // Widen the SAME BANDS table isAllocatableCode/bandFor/classifyBand are + // all derived from — this is the realistic "someone widens a band" + // edit the reviewer demonstrated, not a change to a derived function. + const NEEDLE = "{ category: 'generic', allocatable: true, test: (code) => code >= 64 && code <= 78 }"; + assert.ok(registrySrc.includes(NEEDLE), 'precondition: gen-exit-code-registry.cjs must still contain the BANDS entry this test widens'); + const widened = registrySrc.replace( + NEEDLE, + "{ category: 'generic', allocatable: true, test: (code) => (code >= 64 && code <= 78) || (code >= 14 && code <= 63) }", + ); + assert.notEqual(widened, registrySrc, 'precondition: the widen replacement must actually change the source'); + fs.writeFileSync(path.join(tmp, 'gen-exit-code-registry.cjs'), widened, 'utf8'); + + const docsSrc = fs.readFileSync(DOCS_GEN_SCRIPT, 'utf8'); + fs.writeFileSync(path.join(tmp, 'gen-exit-code-docs.cjs'), docsSrc, 'utf8'); + fs.mkdirSync(path.join(tmp, 'lib'), { recursive: true }); + fs.copyFileSync(path.join(REPO_ROOT, 'scripts', 'lib', 'cli-exit.cjs'), path.join(tmp, 'lib', 'cli-exit.cjs')); + + const check = runNode([path.join(tmp, 'gen-exit-code-docs.cjs'), '--check'], { timeoutMs: PROBE_TIMEOUT_MS }); + assert.notEqual(check.exitCode, 0, 'a widened band (14-63 admitted) must invalidate the committed page — if this passes, the band table is a hand-typed literal again, not derived from isAllocatableCode/bandFor'); + } finally { + cleanup(tmp); + } + }); + + // T1 (#3913 P9 review follow-up): a BANDS category present in neither + // CATEGORY_ROW_ORDER nor CATEGORY_MERGE_INTO must make generation THROW — + // not silently omit the new band from the rendered table while --check + // stays green (the exact defect assertBandCategoriesConsistent in + // gen-exit-code-docs.cjs closes). Driven via a scratch copy of both + // generator scripts, mirroring the widen-a-band regression test above — + // never the real committed files. + // Every T1-T3 scratch copy needs the same three sibling files + // gen-exit-code-docs.cjs's own `require`s resolve relative to itself: + // gen-exit-code-registry.cjs (source of BANDS), lib/cli-exit.cjs, and + // lib/exit-code-registry.cjs (cli-exit.cjs's own dependency). Loading the + // scratch module directly (never via `--check`/`--write`) and calling + // `loadEntries`/`buildDoc` against the REAL committed declaration isolates + // the assertion under test from the doc-page-drift machinery entirely. + function scaffoldScratchDocsModule(tmp, docsSrc) { + fs.mkdirSync(path.join(tmp, 'lib'), { recursive: true }); + fs.copyFileSync(path.join(REPO_ROOT, 'scripts', 'lib', 'cli-exit.cjs'), path.join(tmp, 'lib', 'cli-exit.cjs')); + fs.copyFileSync(path.join(REPO_ROOT, 'scripts', 'lib', 'exit-code-registry.cjs'), path.join(tmp, 'lib', 'exit-code-registry.cjs')); + const entryPath = path.join(tmp, 'docsgen-entry.cjs'); + fs.writeFileSync(entryPath, docsSrc, 'utf8'); + return entryPath; + } + + test('T1: a BANDS category with no CATEGORY_ROW_ORDER/CATEGORY_MERGE_INTO entry throws generation, not a silent omission', () => { + const tmp = createTempDir('gsd-exit-code-docs-p9cat-'); + try { + const registrySrc = fs.readFileSync(GEN_SCRIPT, 'utf8'); + const NEEDLE = " { category: 'shell-signal', allocatable: false, test: (code) => code >= 126 },\n]);"; + assert.ok(registrySrc.includes(NEEDLE), 'precondition: BANDS closing entry must still match'); + const mutated = registrySrc.replace( + NEEDLE, + " { category: 'shell-signal', allocatable: false, test: (code) => code >= 126 },\n" + + " { category: 'brand-new-band', allocatable: true, test: (code) => code === 40 },\n]);", + ); + assert.notEqual(mutated, registrySrc, 'precondition: the BANDS injection must actually change the source'); + fs.writeFileSync(path.join(tmp, 'gen-exit-code-registry.cjs'), mutated, 'utf8'); + + const entryPath = scaffoldScratchDocsModule(tmp, fs.readFileSync(DOCS_GEN_SCRIPT, 'utf8')); + const scratchDocs = require(entryPath); + const entries = scratchDocs.loadEntries(REAL_DECLARATION_PATH); + + // Pre-fix (RED): buildDoc succeeds and silently omits the new band — + // it never appears anywhere in the rendered table. Post-fix (GREEN): + // buildDoc throws naming the unaccounted category. + assert.throws( + () => scratchDocs.buildDoc(entries), + /fail_band_category_unaccounted:.*brand-new-band/, + 'an unaccounted BANDS category must fail generation loudly, not render a table that silently omits it', + ); + } finally { + cleanup(tmp); + } + }); + + // T2: a CATEGORY_ROW_ORDER entry with no BAND_PROSE entry must throw rather + // than rendering the literal string "undefined" into the table. + test('T2: a CATEGORY_ROW_ORDER entry with no BAND_PROSE entry throws rather than rendering undefined', () => { + const tmp = createTempDir('gsd-exit-code-docs-p9cat-'); + try { + fs.copyFileSync(GEN_SCRIPT, path.join(tmp, 'gen-exit-code-registry.cjs')); + + const docsSrc = fs.readFileSync(DOCS_GEN_SCRIPT, 'utf8'); + const NEEDLE = " domain: '**Domain band.**"; + assert.ok(docsSrc.includes(NEEDLE), 'precondition: BAND_PROSE.domain entry must still match'); + // Delete the `domain` prose entry entirely while leaving `domain` in + // CATEGORY_ROW_ORDER — the exact stale-in-one-list-not-the-other shape. + // Line-filtered via splitLines (not a bare-`\n` regex) so this stays + // correct under Windows git-autocrlf CRLF line endings too. + const mutated = splitLines(docsSrc).filter((line) => !line.startsWith(NEEDLE)).join('\n'); + assert.notEqual(mutated, docsSrc, 'precondition: the BAND_PROSE deletion must actually change the source'); + assert.ok(!mutated.includes("domain: '**Domain band.**"), 'precondition: BAND_PROSE.domain must actually be gone'); + + const entryPath = scaffoldScratchDocsModule(tmp, mutated); + const scratchDocs = require(entryPath); + const entries = scratchDocs.loadEntries(REAL_DECLARATION_PATH); + + assert.throws( + () => scratchDocs.buildDoc(entries), + /fail_band_prose_missing:.*domain/, + 'a CATEGORY_ROW_ORDER entry missing from BAND_PROSE must fail generation loudly, not render "undefined"', + ); + } finally { + cleanup(tmp); + } + }); + + // T3: a stale BAND_PROSE category that no BANDS entry (directly, or via + // CATEGORY_MERGE_INTO) produces must throw — dead prose for a band that no + // longer exists is the same drift in the other direction. + test('T3: a stale BAND_PROSE category no BANDS entry produces throws', () => { + const tmp = createTempDir('gsd-exit-code-docs-p9cat-'); + try { + fs.copyFileSync(GEN_SCRIPT, path.join(tmp, 'gen-exit-code-registry.cjs')); + + const docsSrc = fs.readFileSync(DOCS_GEN_SCRIPT, 'utf8'); + const NEEDLE = "const BAND_PROSE = Object.freeze({\n"; + assert.ok(docsSrc.includes(NEEDLE), 'precondition: BAND_PROSE opening must still match'); + const mutated = docsSrc.replace( + NEEDLE, + `${NEEDLE} 'long-retired-band': 'This band was retired and no BANDS entry produces it any more.',\n`, + ); + assert.notEqual(mutated, docsSrc, 'precondition: the stale BAND_PROSE injection must actually change the source'); + + const entryPath = scaffoldScratchDocsModule(tmp, mutated); + const scratchDocs = require(entryPath); + const entries = scratchDocs.loadEntries(REAL_DECLARATION_PATH); + + assert.throws( + () => scratchDocs.buildDoc(entries), + /fail_band_category_stale:.*long-retired-band/, + 'a stale BAND_PROSE category with no producing BANDS entry must fail generation loudly', + ); + } finally { + cleanup(tmp); + } + }); + + // T4 (positive control): the REAL, unmodified configuration renders all six + // rows with no literal "undefined" anywhere in the page. Without this, a + // fix that throws unconditionally (rather than only on genuine drift) would + // still pass T1-T3 by accident. + test('T4: the real unmodified configuration renders all six band rows with no "undefined" in the page', () => { + const result = runDocsGen(['--check']); + assert.equal(result.exitCode, 0, result.stderr); + const md = fs.readFileSync(REAL_DOC_PATH, 'utf8'); + assert.ok(!md.includes('undefined'), 'the generated page must never contain the literal string "undefined"'); + const bandTableSection = md.slice(md.indexOf('| Band | Meaning |'), md.indexOf('## The v1/v2 exit contract')); + // `|---|---|` starts with `|-`, not `| `, so it is already excluded by + // this pattern — only the header line (`| Band | Meaning |`) needs + // subtracting to leave just the data rows. + const rowCount = (bandTableSection.match(/^\| /gm) || []).length - 1; + assert.equal(rowCount, 6, 'the Reserved bands table must render exactly six rows (free, hook-only, node-reserved, outside-every-band, generic, domain)'); + }); + + // Forward guard, not a regression test: buildDoc has no source of + // non-determinism (no Date.now/Math.random/env read), so this cannot + // currently fail — it exists to catch a FUTURE change that introduces one. + test('forward guard: buildDoc stays pure if a future change adds a non-deterministic input', () => { + const once = docsGenerator.buildDoc(registry.EXIT_CODES); + const twice = docsGenerator.buildDoc(registry.EXIT_CODES); + assert.equal(once, twice); + }); +}); + // ── fast-check properties ───────────────────────────────────────────────────── describe('exit-code-registry: fast-check properties', () => { test('nameForExitCode(exitCodeFor(name)) round-trips for every registered name', () => { diff --git a/tests/helpers/live-command-registry.cjs b/tests/helpers/live-command-registry.cjs index 004cfc9b4..d6cd1c2a1 100644 --- a/tests/helpers/live-command-registry.cjs +++ b/tests/helpers/live-command-registry.cjs @@ -36,6 +36,62 @@ const COMMANDS_DIR = path.join(__dirname, '..', '..', 'commands', 'gsd'); // Module-level memoization — set on first call, reused thereafter. let _cache = null; +/** + * The exhaustive set of prefixes `getLiveCommandTokens()` ever emits — kept + * here, alongside the token-emission logic itself, as the single source of + * truth so a shape-based "does this string look like a live-command token" + * check (see `tests/qa/oracles.cjs` value-hygiene) never drifts from what + * this file actually generates. Every token this module produces is exactly + * `${prefix}${slug}` for one of these three prefixes — see the `tokens.add` + * calls in `getLiveCommandTokens()` below, which is the sole producer. + */ +const LIVE_COMMAND_TOKEN_PREFIXES = Object.freeze(['/gsd-', '/gsd:', '$gsd-']); + +/** + * Extract the first whitespace-delimited "word" of a string, e.g. + * `"/gsd-plan-phase 2"` -> `"/gsd-plan-phase"`. Real command-token payloads + * carry trailing arguments (`init`'s `recommended_actions[].command` is + * literally `/gsd-plan-phase 2`), so an exact-match test against the WHOLE + * string would reject every argument-carrying token — the token itself is + * always the first word. + * + * @param {string} value + * @returns {string} + */ +function firstToken(value) { + const match = value.match(/^\S+/); + return match ? match[0] : value; +} + +/** + * Is `value`'s first whitespace-delimited word an EXACT member of + * `liveTokens` (a `Set`, normally `getLiveCommandTokens()`)? + * + * This is the SOLE shared predicate behind every "is this string a live + * command token" check in the QA harness — `tests/qa/oracles.cjs`'s + * `value-hygiene` command-token exemption and its `routing-validity` check — + * so the two can never independently drift on what counts as a live command + * token (#3913 P9 security review: this is the THIRD iteration of that + * exemption. First it was keyed on the leaf name `command`; then on a bare + * `String.startsWith` prefix test with an unconstrained remainder, which + * exempted any string merely SHARING A PREFIX with a real token — e.g. + * `/gsd-x/../../../Users/someone/.ssh/id_rsa` or `/gsd:/etc/passwd` both + * satisfied `startsWith('/gsd-')`/`startsWith('/gsd:')`). + * + * Deliberately NOT a `startsWith` fast path over `LIVE_COMMAND_TOKEN_PREFIXES` + * short-circuiting this check: exact membership of the first word is the + * WHOLE predicate, so a leaked path can never pass merely by sharing a + * token's first few bytes. + * + * @param {unknown} value + * @param {Set} liveTokens + * @returns {boolean} + */ +function isLiveCommandToken(value, liveTokens) { + if (typeof value !== 'string') return false; + return liveTokens.has(firstToken(value)); +} + /** * Parse the YAML frontmatter `name:` field from a command file's content. * Returns the slug (e.g. "help", "plan-phase", "context") or null if the @@ -129,4 +185,4 @@ function getLiveCommandTokens() { return _cache; } -module.exports = { getLiveCommandTokens }; +module.exports = { getLiveCommandTokens, LIVE_COMMAND_TOKEN_PREFIXES, firstToken, isLiveCommandToken }; diff --git a/tests/loop-walk.qa.test.cjs b/tests/loop-walk.qa.test.cjs index 508f8715e..2e3c603f9 100644 --- a/tests/loop-walk.qa.test.cjs +++ b/tests/loop-walk.qa.test.cjs @@ -29,7 +29,7 @@ const { MUTATIONS, apply, NOOP } = require('./qa/mutations.cjs'); const { loadScenario, runScenario, assertWiringIsLive } = require('./qa/scenario.cjs'); const { buildReport } = require('./qa/report.cjs'); const { resolveRef } = require('./qa/fixtures/index.cjs'); -const { resolveWithin, resolveForCompare } = require('./qa/paths.cjs'); +const { resolveWithin, resolveForCompare, isUnderProjectDir } = require('./qa/paths.cjs'); const { LOOP_HOST_CONTRACT } = require('../gsd-core/bin/lib/loop-host-contract.cjs'); const { extractFrontmatter } = require('../gsd-core/bin/lib/frontmatter.cjs'); const { evaluateUatPassed } = require('../gsd-core/bin/lib/uat-predicate.cjs'); @@ -235,8 +235,10 @@ describe('RunResult classification', () => { }); describe('oracle self-tests', () => { - test('ORACLES has exactly 10 entries (7 violation-severity + 3 smell-severity)', () => { - assert.strictEqual(ORACLES.length, 10); + test('ORACLES has exactly 9 entries (8 violation-severity + 1 smell-severity) (#3913)', () => { + // #3913: soft-error-exit-zero deleted (inert SMELL; the exit contract now + // models the same condition), untyped-success promoted SMELL -> VIOLATION. + assert.strictEqual(ORACLES.length, 9); }); test('exit-contract passes on a clean context', () => { @@ -380,6 +382,92 @@ describe('oracle self-tests', () => { assert.strictEqual(outcome.severity, SEVERITY.SMELL); }); + test('value-hygiene has no finding for a live-command TOKEN under actions[].command (regression guard — FAILS pre-fix, measured)', (t) => { + const projectDir = createTempDir('gsd-hygiene-command-token-'); + t.after(() => cleanup(projectDir)); + const outcome = getOracle('value-hygiene').check({ + result: { json: { actions: [{ id: 'a', command: '/gsd:progress' }] } }, + projectDir, + }); + assert.strictEqual(outcome.ok, true); + }); + + test('value-hygiene SMELLs on a genuine absolute-path leak stored under a key named "command"', (t) => { + const projectDir = createTempDir('gsd-hygiene-command-path-leak-'); + t.after(() => cleanup(projectDir)); + const outcome = getOracle('value-hygiene').check({ + result: { json: { actions: [{ id: 'a', command: '/Users/someone/elsewhere/bin/tool' }] } }, + projectDir, + }); + assert.strictEqual(outcome.ok, false); + assert.strictEqual(outcome.severity, SEVERITY.SMELL); + assert.strictEqual(outcome.subject.key, '$.actions[0].command'); + }); + + // #3913 P9 SEC-1: the shape-based exemption was too loose — `String.startsWith` + // against a live-command prefix with NO constraint on the remainder let a + // leaked path through as long as its first bytes happened to match a real + // token prefix. Fixed by requiring the string's first whitespace-delimited + // word to be an EXACT member of getLiveCommandTokens(). + + test('F1a: a leaked path sharing a prefix with a real token ("/gsd-x/../../../Users/someone/.ssh/id_rsa") IS a SMELL (failing-first against the pre-fix startsWith exemption)', (t) => { + const projectDir = createTempDir('gsd-hygiene-sec1-traversal-'); + t.after(() => cleanup(projectDir)); + const outcome = getOracle('value-hygiene').check({ + result: { json: { command: '/gsd-x/../../../Users/someone/.ssh/id_rsa' } }, + projectDir, + }); + assert.strictEqual(outcome.ok, false); + assert.strictEqual(outcome.severity, SEVERITY.SMELL); + }); + + test('F1b: a leaked path sharing a colon-style prefix ("/gsd:/etc/passwd") IS a SMELL', (t) => { + const projectDir = createTempDir('gsd-hygiene-sec1-colon-'); + t.after(() => cleanup(projectDir)); + const outcome = getOracle('value-hygiene').check({ + result: { json: { command: '/gsd:/etc/passwd' } }, + projectDir, + }); + assert.strictEqual(outcome.ok, false); + assert.strictEqual(outcome.severity, SEVERITY.SMELL); + }); + + test('F1c: an exact live-command token ("/gsd-progress") is exempt (regression guard)', (t) => { + const projectDir = createTempDir('gsd-hygiene-sec1-exact-'); + t.after(() => cleanup(projectDir)); + const outcome = getOracle('value-hygiene').check({ + result: { json: { command: '/gsd-progress' } }, + projectDir, + }); + assert.strictEqual(outcome.ok, true); + }); + + test('F1d: a live-command token carrying trailing arguments ("/gsd-plan-phase 2") is exempt (the case a naive exact-match-on-the-whole-string breaks)', (t) => { + const projectDir = createTempDir('gsd-hygiene-sec1-args-'); + t.after(() => cleanup(projectDir)); + const outcome = getOracle('value-hygiene').check({ + result: { json: { command: '/gsd-plan-phase 2' } }, + projectDir, + }); + assert.strictEqual(outcome.ok, true); + }); + + // Forward guard, not a regression test: 'progress' matches neither the + // live-command-token exemption nor the path-leak detection under either + // the pre-#3913-SEC-1 (startsWith) or post-fix (exact-token) shape, so + // this cannot currently fail against either — it exists to catch a FUTURE + // change that starts false-positiving on ordinary non-path, non-token + // command values. + test('forward guard: value-hygiene is unaffected by a "command" value that is neither a live-command token nor an absolute path', (t) => { + const projectDir = createTempDir('gsd-hygiene-command-plain-'); + t.after(() => cleanup(projectDir)); + const outcome = getOracle('value-hygiene').check({ + result: { json: { actions: [{ id: 'a', command: 'progress' }] } }, + projectDir, + }); + assert.strictEqual(outcome.ok, true); + }); + test('value-hygiene does not crash on a cyclic json object', () => { const cyclic = {}; cyclic.self = cyclic; @@ -548,22 +636,159 @@ describe('oracle self-tests', () => { assert.strictEqual(outcome.subject.to, 2); }); - test('routing-validity passes on a clean context', () => { + // #3913 P9 matrix "A" — routing-validity was rescoped to validate the fields + // that actually carry command tokens (actions[].command, next.command) + // instead of demanding a token from the bare `recommended` action id, which + // is an id by design (src/smart-entry.cts:766) and never a command token. + + test('A1: a bare-id recommended paired with a matching actions[].command PASSES', () => { + // Failing-first: against the pre-#3913 oracles.cjs this fails because the + // old check demanded ctx.liveCommands.includes(json.recommended) directly + // ("discuss-phase" is not a command token). Red/green captured manually + // (see PR evidence); this is the permanent regression test. const outcome = getOracle('routing-validity').check({ - result: { json: { recommended: '/gsd-plan-phase' } }, - liveCommands: ['/gsd-plan-phase'], + result: { + json: { + recommended: 'discuss-phase', + actions: [{ id: 'discuss-phase', label: 'Discuss', command: '/gsd:discuss-phase', recommended: true }], + }, + }, + liveCommands: ['/gsd:discuss-phase'], }); assert.strictEqual(outcome.ok, true); }); - test('routing-validity fails on a broken context (token not in liveCommands)', () => { + test('A2 (positive control): actions[].command naming a NON-live command still FAILS', () => { + // Without this, deleting the routing-validity check entirely would also + // satisfy A1 — this proves the oracle can still produce a VIOLATION. const outcome = getOracle('routing-validity').check({ - result: { json: { recommended: '/gsd-plan-phase' } }, - liveCommands: ['/gsd-something-else'], + result: { + json: { + recommended: 'discuss-phase', + actions: [{ id: 'discuss-phase', label: 'Discuss', command: '/gsd:not-a-live-command', recommended: true }], + }, + }, + liveCommands: ['/gsd:discuss-phase'], }); assert.strictEqual(outcome.ok, false); + assert.strictEqual(outcome.severity, SEVERITY.VIOLATION); assert.strictEqual(typeof outcome.detail, 'string'); assert.ok(outcome.detail.length > 0); + assert.strictEqual(outcome.subject.value, '/gsd:not-a-live-command'); + }); + + // Forward guard, not a regression test: a payload with nothing for the + // oracle to inspect returns ok under both the old and new oracle shape + // (the old oracle also returned ok for this input) — it exists to catch a + // FUTURE change that starts engaging on an empty/no-routing payload. + test('forward guard — A4: a payload with no recommended and no actions/next.command is not engaged (ok)', () => { + const outcome = getOracle('routing-validity').check({ + result: { json: { situation: 'complete', summary: 'done' } }, + liveCommands: ['/gsd:discuss-phase'], + }); + assert.strictEqual(outcome.ok, true); + }); + + test('routing-validity also validates next.command (state-contract shape)', () => { + const outcome = getOracle('routing-validity').check({ + result: { json: { next: { command: '/gsd:not-live', label: 'x', reason: 'y' } } }, + liveCommands: ['/gsd:discuss-phase'], + }); + assert.strictEqual(outcome.ok, false); + assert.strictEqual(outcome.severity, SEVERITY.VIOLATION); + assert.strictEqual(outcome.subject.key, 'next.command'); + }); + + // #3913 P9 SEC-2: routing-validity enumerated only actions[].command and + // next.command, so a non-live token in ANY other field-shape was invisible + // to the oracle. Fixed by walking the entire payload for any string whose + // first word matches the live-command prefix set. Each F2a case is + // failing-first against the pre-fix two-field-path version. + + const F2A_LIVE_COMMANDS = ['/gsd-progress']; + + test('F2a: a non-live token in recommended_command is a VIOLATION', () => { + const outcome = getOracle('routing-validity').check({ + result: { json: { recommended_command: '/gsd-nope' } }, + liveCommands: F2A_LIVE_COMMANDS, + }); + assert.strictEqual(outcome.ok, false); + assert.strictEqual(outcome.severity, SEVERITY.VIOLATION); + assert.strictEqual(outcome.subject.value, '/gsd-nope'); + }); + + test('F2a: a non-live token as a bare STRING next is a VIOLATION', () => { + const outcome = getOracle('routing-validity').check({ + result: { json: { next: '/gsd-nope' } }, + liveCommands: F2A_LIVE_COMMANDS, + }); + assert.strictEqual(outcome.ok, false); + assert.strictEqual(outcome.severity, SEVERITY.VIOLATION); + assert.strictEqual(outcome.subject.value, '/gsd-nope'); + }); + + test('F2a: a non-live token in steps[].command is a VIOLATION', () => { + const outcome = getOracle('routing-validity').check({ + result: { json: { steps: [{ command: '/gsd-nope' }] } }, + liveCommands: F2A_LIVE_COMMANDS, + }); + assert.strictEqual(outcome.ok, false); + assert.strictEqual(outcome.severity, SEVERITY.VIOLATION); + assert.strictEqual(outcome.subject.key, 'steps[0].command'); + assert.strictEqual(outcome.subject.value, '/gsd-nope'); + }); + + test('F2b: a LIVE token in recommended_command, string next, and steps[].command each PASS', () => { + for (const json of [ + { recommended_command: '/gsd-progress' }, + { next: '/gsd-progress' }, + { steps: [{ command: '/gsd-progress' }] }, + ]) { + const outcome = getOracle('routing-validity').check({ result: { json }, liveCommands: F2A_LIVE_COMMANDS }); + assert.strictEqual(outcome.ok, true, `expected ok for ${JSON.stringify(json)}`); + } + }); + + test('F2: a non-live token surfaces through next as an array of {command} entries', () => { + const outcome = getOracle('routing-validity').check({ + result: { json: { next: [{ command: '/gsd-nope' }] } }, + liveCommands: F2A_LIVE_COMMANDS, + }); + assert.strictEqual(outcome.ok, false); + assert.strictEqual(outcome.severity, SEVERITY.VIOLATION); + }); + + test('F2: a non-live token surfaces when actions is an object map instead of an array', () => { + const outcome = getOracle('routing-validity').check({ + result: { json: { actions: { a: { command: '/gsd-nope' } } } }, + liveCommands: F2A_LIVE_COMMANDS, + }); + assert.strictEqual(outcome.ok, false); + assert.strictEqual(outcome.severity, SEVERITY.VIOLATION); + }); + + test('F2: a non-live token surfaces via actions[].next.command', () => { + const outcome = getOracle('routing-validity').check({ + result: { json: { actions: [{ next: { command: '/gsd-nope' } }] } }, + liveCommands: F2A_LIVE_COMMANDS, + }); + assert.strictEqual(outcome.ok, false); + assert.strictEqual(outcome.severity, SEVERITY.VIOLATION); + }); + + test('shared predicate: a live token routing-validity vouches for is exactly what value-hygiene exempts', (t) => { + const projectDir = createTempDir('gsd-shared-predicate-'); + t.after(() => cleanup(projectDir)); + const routing = getOracle('routing-validity').check({ + result: { json: { recommended_command: '/gsd-progress' } }, + liveCommands: F2A_LIVE_COMMANDS, + }); + assert.strictEqual(routing.ok, true); + const hygiene = getOracle('value-hygiene').check({ + result: { json: { command: '/gsd-progress' } }, + projectDir, + }); + assert.strictEqual(hygiene.ok, true); }); test('determinism passes on a clean context', () => { @@ -584,40 +809,52 @@ describe('oracle self-tests', () => { assert.ok(outcome.detail.length > 0); }); - test('soft-error-exit-zero passes on a clean context', () => { - const outcome = getOracle('soft-error-exit-zero').check({ result: { kind: KIND.JSON, argv: ['progress'] } }); - assert.strictEqual(outcome.ok, true); + // #3913 D1/D2 — soft-error-exit-zero was deleted outright: it was an inert + // SMELL (never reaches `failed`) restating a condition the exit contract + // now models directly (v1 exit 0 / v2 exit 80 for an output({error}) path). + + test('D1: soft-error-exit-zero appears in no oracle in ORACLES', () => { + assert.strictEqual(ORACLES.some((o) => o.id === 'soft-error-exit-zero'), false); }); - test('soft-error-exit-zero SMELLs (not a violation) on a SOFT_ERROR result', () => { + test('D2: runOracles over a KIND.SOFT_ERROR result produces no finding with id soft-error-exit-zero', () => { const ctx = { result: { kind: KIND.SOFT_ERROR, argv: ['progress'], json: { error: 'no phases found' } } }; - const outcome = getOracle('soft-error-exit-zero').check(ctx); - assert.strictEqual(outcome.ok, false); - assert.strictEqual(outcome.severity, SEVERITY.SMELL); - assert.strictEqual(typeof outcome.detail, 'string'); - assert.ok(outcome.detail.length > 0); - assert.deepEqual(outcome.subject.argv, ['progress'], 'subject.argv must name the offending command'); - const { violations, smells } = runOracles(ctx); - assert.strictEqual(violations.some((v) => v.id === 'soft-error-exit-zero'), false); - assert.strictEqual(smells.some((s) => s.id === 'soft-error-exit-zero'), true); + const { violations, smells, failed } = runOracles(ctx); + assert.strictEqual(violations.some((f) => f.id === 'soft-error-exit-zero'), false); + assert.strictEqual(smells.some((f) => f.id === 'soft-error-exit-zero'), false); + assert.strictEqual(failed.some((f) => f.id === 'soft-error-exit-zero'), false); }); - test('untyped-success passes on a clean context', () => { + // #3913 C — untyped-success promoted SEVERITY.SMELL -> SEVERITY.VIOLATION. + // Nothing else in the repo asserts "the recommended token matches a live + // command" for a PROSE-only surface, so this stays a real, enforced guard + // rather than being deleted alongside soft-error-exit-zero. + + test('C3: untyped-success passes (is not engaged) on a non-PROSE context', () => { const outcome = getOracle('untyped-success').check({ result: { kind: KIND.JSON, argv: ['progress'] } }); assert.strictEqual(outcome.ok, true); }); - test('untyped-success SMELLs (not a violation) on a PROSE result', () => { + test('C1 + C2: untyped-success is a VIOLATION on a PROSE result, and reaches runOracles(...).failed', () => { + // C1: severity is VIOLATION, not SMELL. const ctx = { result: { kind: KIND.PROSE, argv: ['init'] } }; const outcome = getOracle('untyped-success').check(ctx); assert.strictEqual(outcome.ok, false); - assert.strictEqual(outcome.severity, SEVERITY.SMELL); + assert.strictEqual(outcome.severity, SEVERITY.VIOLATION); assert.strictEqual(typeof outcome.detail, 'string'); assert.ok(outcome.detail.length > 0); assert.deepEqual(outcome.subject.argv, ['init'], 'subject.argv must name the offending command'); - const { violations, smells } = runOracles(ctx); - assert.strictEqual(violations.some((v) => v.id === 'untyped-success'), false); - assert.strictEqual(smells.some((s) => s.id === 'untyped-success'), true); + + // C2 — the anti-vacuity test for the whole phase: a promotion that is + // cosmetic would still route this finding to `smells`, where it can never + // redden a build. Failing-first: against the pre-#3913 oracles.cjs this + // fails because the finding lands in `smells`, not `failed` (red/green + // captured manually; see PR evidence). This is the permanent regression + // test for that identity. + const { violations, smells, failed } = runOracles(ctx); + assert.strictEqual(smells.some((s) => s.id === 'untyped-success'), false); + assert.strictEqual(violations.some((v) => v.id === 'untyped-success'), true); + assert.strictEqual(failed.some((f) => f.id === 'untyped-success'), true); }); test('contract-conflict passes on a clean context', () => { @@ -644,16 +881,20 @@ describe('oracle self-tests', () => { describe('severity model', () => { test('a smell must never appear in failed (the contract that keeps smells from breaking builds)', () => { - const ctx = { result: { kind: KIND.PROSE, argv: ['init'] } }; + // #3913: untyped-success was promoted to a VIOLATION and can no longer + // serve as the SMELL example here — contract-conflict is now the sole + // remaining SMELL-severity oracle. + const ctx = { jsonErrorMode: true, result: { kind: KIND.UNSTRUCTURED_ERROR, argv: ['bad-usage'] } }; const { failed, smells } = runOracles(ctx); - assert.ok(smells.some((s) => s.id === 'untyped-success')); - assert.strictEqual(failed.some((f) => f.id === 'untyped-success'), false); + assert.ok(smells.some((s) => s.id === 'contract-conflict')); + assert.strictEqual(failed.some((f) => f.id === 'contract-conflict'), false); }); test('failed.length === violations.length for a context producing both a violation and a smell', () => { - // value-hygiene fires a VIOLATION on the NaN leaf; untyped-success fires a - // SMELL on the PROSE kind. Both fire from the same ctx. - const ctx = { result: { kind: KIND.PROSE, argv: ['init'], json: { a: Number.NaN } } }; + // value-hygiene fires a VIOLATION on the NaN leaf; contract-conflict fires + // a SMELL on the UNSTRUCTURED_ERROR kind under jsonErrorMode. Both fire + // from the same ctx. + const ctx = { jsonErrorMode: true, result: { kind: KIND.UNSTRUCTURED_ERROR, argv: ['bad-usage'], json: { a: Number.NaN } } }; const { failed, violations, smells } = runOracles(ctx); assert.ok(violations.length > 0); assert.ok(smells.length > 0); @@ -958,6 +1199,37 @@ describe('path containment', () => { assert.strictEqual(fs.existsSync(path.join(walk.dir, '.planning', 'PROJECT.md')), true); }); + test('isUnderProjectDir treats a Windows DOS 8.3 short-name alias and its long form as the same directory (Windows CI regression: RUNNER~1 vs runneradmin)', () => { + // Real-world shape observed on windows-latest CI: one side resolved via a path that still + // carries the 8.3 short-name segment (RUNNER~1), the other via the fully expanded long name + // (runneradmin) -- same directory on disk, two different strings, unless the realpath + // implementation used for comparison actually expands the short name. Only + // `fs.realpathSync.native` performs that expansion; plain `fs.realpathSync` does not, which + // was the root cause. Injected here (rather than skipped on non-Windows) via the + // `realpathFn` seam so this regression is caught on every platform, not just Windows. + const shortForm = 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\gsd-loop-walk-GP8Q6U'; + const longForm = 'C:/Users/runneradmin/AppData/Local/Temp/gsd-loop-walk-GP8Q6U'; + const fakeNativeRealpath = (p) => (p === shortForm ? longForm : p); + + const resolvedProjectDir = resolveForCompare(shortForm, fakeNativeRealpath); + const resolvedCandidate = resolveForCompare(longForm, fakeNativeRealpath); + + assert.strictEqual(resolvedProjectDir, longForm); + assert.strictEqual(resolvedCandidate, longForm); + assert.strictEqual(isUnderProjectDir(resolvedCandidate, resolvedProjectDir), true); + }); + + test('isUnderProjectDir positive control: a genuinely outside path is still reported as outside, even through the injected realpathFn seam', () => { + const projectDir = 'C:/Users/runneradmin/AppData/Local/Temp/gsd-loop-walk-GP8Q6U'; + const outsideDir = 'C:/Users/runneradmin/AppData/Local/Temp/gsd-loop-walk-OTHER'; + const identityRealpath = (p) => p; + + const resolvedProjectDir = resolveForCompare(projectDir, identityRealpath); + const resolvedCandidate = resolveForCompare(outsideDir, identityRealpath); + + assert.strictEqual(isUnderProjectDir(resolvedCandidate, resolvedProjectDir), false); + }); + test('mutations apply("delete", ...) rejects a traversing relPath', (t) => { const mutDir = createTempDir('gsd-pathguard-delete-'); t.after(() => cleanup(mutDir)); @@ -1026,11 +1298,13 @@ describe('greenfield walk (end-to-end)', () => { // tests/helpers/live-command-registry.cjs's real API: getLiveCommandTokens() // returns a memoized Set of every live slash-command token derived // from commands/gsd/*.md frontmatter (e.g. "/gsd-plan-phase"). The - // routing-validity oracle checks result.json.recommended against this set. + // routing-validity oracle (#3913) checks command tokens carried by + // result.json.actions[].command / result.json.next.command against this + // set — never the bare result.json.recommended action id. liveCommands = [...getLiveCommandTokens()]; }); - test('runs every step of the greenfield-happy-path scenario clean', () => { + test('runs every step of the greenfield-happy-path scenario clean (A3: the real smart-entry --json payload passes routing-validity)', () => { const scenarioPath = path.join(__dirname, 'qa', 'scenarios', 'greenfield-happy-path.json'); const scenario = loadScenario(scenarioPath); const report = runScenario(scenario, { LoopWalk, runOracles, liveCommands }); @@ -1042,19 +1316,24 @@ describe('greenfield walk (end-to-end)', () => { } assert.strictEqual(report.ok, true); - // Anti-vacuity: a QA harness that reports NOTHING on a first real walk - // against the actual CLI is far more likely to be mis-specified (oracles - // that never fire, a wiring bug that drops ctx fields, a scenario that - // never exercises the paths that produce smells) than the engine is - // genuinely flawless. "Found nothing" is itself a failure signal for a - // harness whose whole job is to keep known trade-offs visible, so the - // walk must be able to speak at least once. Deliberately NOT asserting an - // exact smell count or exact oracle ids here — that would pin today's - // engine behavior into the test and defeat the point of a smell channel - // that is allowed to evolve without becoming a build break. - const totalSmells = report.steps.reduce((sum, step) => sum + step.smells.length, 0); - assert.ok(totalSmells > 0, 'expected the greenfield walk to surface at least one smell'); - assert.ok(report.smellSummary.length > 0, 'expected a non-empty smellSummary'); + // #3913: this scenario's corpus previously carried exactly two firing + // smells — soft-error-exit-zero on state-snapshot, and untyped-success on + // smart-entry (before it gained a --json surface). Both are now fixed: + // soft-error-exit-zero is deleted outright, and adding --json to + // smart-entry gives it a typed surface so untyped-success (now a + // VIOLATION) never fires against it. So this corpus is genuinely + // smell-free — asserting `totalSmells === 0` is the honest anti-vacuity + // check here. The harness's ability to actually SPEAK a finding (not just + // report nothing) is exercised elsewhere: every oracle's own pass/fail + // self-test above, the C1/C2 promoted-violation tests, and the wiring + // self-test below. + const allSmells = report.steps.flatMap((step) => + step.smells.map((smell) => ({ at: step.at, id: smell.id, subject: smell.subject, detail: smell.detail }))); + assert.strictEqual( + allSmells.length, + 0, + `expected the greenfield walk to be smell-free after #3913, found ${allSmells.length}:\n${JSON.stringify(allSmells, null, 2)}`, + ); }); }); @@ -1158,6 +1437,29 @@ describe('scenario discovery (mutations wired for real)', () => { + 'every corruption was silently absorbed', ); }); + + test('B2: across every discovered scenario, the count of steps classified KIND.PROSE is exactly 0 (#3913)', () => { + // Asserted over the ENUMERATED corpus, not sampled — the promoted + // untyped-success oracle is only safe to run as a VIOLATION because no + // executed step anywhere in the corpus emits KIND.PROSE today. A future + // prose-only step reddens the build via untyped-success, which is the + // intended behavior (see oracles.cjs's describe text for that oracle). + const liveCommands = [...getLiveCommandTokens()]; + const allFiles = discoverScenarioFiles(); + assert.ok(allFiles.length > 0, 'expected at least one scenario file — otherwise this count is vacuous'); + let proseSteps = 0; + let totalSteps = 0; + for (const file of allFiles) { + const scenario = loadScenario(file); + const report = runScenario(scenario, { LoopWalk, runOracles, liveCommands }); + for (const step of report.steps) { + totalSteps += 1; + if (step.kind === KIND.PROSE) proseSteps += 1; + } + } + assert.ok(totalSteps > 0, 'expected at least one executed step across the corpus'); + assert.strictEqual(proseSteps, 0, `expected 0 KIND.PROSE steps across ${totalSteps} executed steps`); + }); }); describe('wiring self-test (anti-vacuity)', () => { @@ -1683,3 +1985,83 @@ describe('qa-smell-ratchet gate (#3597)', () => { assert.deepStrictEqual(found.smells, []); }); }); + +describe('smell-baseline.json pruning (#3913 matrix E)', () => { + const BASELINE_PATH = path.join(__dirname, 'qa', 'smell-baseline.json'); + + /** @returns {{version: number, smells: Array<{key:string,id:string,scenario:string,issue:unknown,reason?:string}>}} */ + function readBaseline() { + return JSON.parse(fs.readFileSync(BASELINE_PATH, 'utf-8')); + } + + test('E1: smell-baseline.json contains zero entries with id soft-error-exit-zero or untyped-success', () => { + const baseline = readBaseline(); + const stale = baseline.smells.filter( + (s) => s.id === 'soft-error-exit-zero' || s.id === 'untyped-success', + ); + assert.deepStrictEqual( + stale, + [], + `expected zero soft-error-exit-zero/untyped-success entries, found: ${JSON.stringify(stale)}`, + ); + }); + + // The real post-condition phase 8 drove toward: the baseline is EMPTY, not + // merely "every entry (if any) is well-formed". Asserting emptiness + // directly is what actually pins today's state — a loop over `[]` below + // would trivially pass regardless of whether the fix ever shipped. + test('E3a: smell-baseline.json currently carries zero entries', () => { + const baseline = readBaseline(); + assert.deepStrictEqual(baseline.smells, [], `expected an empty baseline, found: ${JSON.stringify(baseline.smells)}`); + }); + + /** The same predicate the baseline-entry check enforces, isolated so a synthetic fixture can actually drive it (the real baseline has no entries to iterate today). */ + function isPositiveIntegerIssue(entry) { + return Number.isInteger(entry.issue) && entry.issue > 0; + } + + // E3b: drives isPositiveIntegerIssue over a SYNTHETIC fixture, since the + // real baseline's `smells` array is empty and a loop over it never + // executes its body — a bare `for (const entry of baseline.smells)` test + // against the live file is vacuous by construction. This is what actually + // proves the invariant can fail. + test('E3b: a baseline entry with a non-positive or non-integer issue fails the check (synthetic fixture)', () => { + assert.strictEqual(isPositiveIntegerIssue({ key: 'k', issue: 3913 }), true); + for (const bad of [ + { key: 'zero', issue: 0 }, + { key: 'negative', issue: -1 }, + { key: 'float', issue: 3.5 }, + { key: 'string', issue: '3913' }, + { key: 'missing', issue: undefined }, + ]) { + assert.strictEqual(isPositiveIntegerIssue(bad), false, `entry ${JSON.stringify(bad)} must fail the positive-integer-issue check`); + } + }); + + // Forward guard, not a regression test: if a future PR re-adds baseline + // entries, EVERY one of them must still satisfy isPositiveIntegerIssue. + // Currently vacuous (smells is []) — it exists to catch a future + // regression, not today's state (see E3a/E3b for the tests that can + // actually fail right now). + test('forward guard: every surviving baseline entry (if any are ever re-added) must cite a positive-integer issue', () => { + const baseline = readBaseline(); + for (const entry of baseline.smells) { + assert.strictEqual( + isPositiveIntegerIssue(entry), + true, + `entry ${JSON.stringify(entry.key)} has a non-positive-integer issue: ${JSON.stringify(entry.issue)}`, + ); + } + }); + + // Forward guard, not a regression test: `version` and `smells` are set + // once by hand in this committed fixture and nothing in this suite + // mutates them, so this cannot fail today — it exists to catch a FUTURE + // change that corrupts the file's shape (e.g. a hand-edit that drops + // `version` or turns `smells` into an object). + test('forward guard: smell-baseline.json keeps its version:1 / smells-array shape', () => { + const baseline = readBaseline(); + assert.strictEqual(baseline.version, 1); + assert.ok(Array.isArray(baseline.smells)); + }); +}); diff --git a/tests/qa/oracles.cjs b/tests/qa/oracles.cjs index 0c1d6cec9..bcf0964a5 100644 --- a/tests/qa/oracles.cjs +++ b/tests/qa/oracles.cjs @@ -65,6 +65,12 @@ const nodePath = require('node:path'); const { KIND } = require('./result.cjs'); const { resolveForCompare, isUnderProjectDir } = require('./paths.cjs'); +const { + LIVE_COMMAND_TOKEN_PREFIXES, + getLiveCommandTokens, + firstToken, + isLiveCommandToken, +} = require('../helpers/live-command-registry.cjs'); /** Severity of a failed oracle outcome. A SMELL is evidence, not a verdict — it never fails a build. */ const SEVERITY = Object.freeze({ VIOLATION: 'violation', SMELL: 'smell' }); @@ -92,6 +98,20 @@ const SENTINEL_STRINGS = new Set(['undefined', 'null', 'NaN', '[object Object]'] */ const EXTERNAL_PATH_ALLOWED_KEYS = Object.freeze(new Set(['agents_dir'])); +/** + * Strip the walk()-root prefix (`"$."`) from a `walk()`-built path, e.g. + * `"$.next.command"` -> `"next.command"`, `"$.actions[0].command"` -> + * `"actions[0].command"` — the field-naming convention `routing-validity`'s + * `subject.key` has always used, kept stable across the switch to a generic + * `walk()`-based scan (#3913 P9 SEC-2). + * + * @param {string} walkPath + * @returns {string} + */ +function stripRootPrefix(walkPath) { + return walkPath.startsWith('$.') ? walkPath.slice(2) : walkPath; +} + /** * Extract the leaf key name from a `walk()`-built path (e.g. `"$.agents_dir"` * -> `"agents_dir"`, `"$.foo.bar[3]"` -> `"bar"` for the array element itself, @@ -313,8 +333,13 @@ const ORACLES = Object.freeze([ 'result.json must not contain a NaN number or a string exactly equal to a coercion-artifact sentinel ' + '(VIOLATION). When ctx.projectDir is supplied, an absolute-path string outside it is reported as a SMELL — ' + 'never a violation — UNLESS its leaf key name is in EXTERNAL_PATH_ALLOWED_KEYS (a field whose contract is to ' + - 'point outside the project, e.g. agents_dir), which is skipped entirely: not a smell, not a violation. Without ' + - 'ctx.projectDir the path check is skipped rather than guessed.', + 'point outside the project, e.g. agents_dir) or the string\'s FIRST WHITESPACE-DELIMITED WORD is an EXACT ' + + 'member of getLiveCommandTokens() (e.g. `/gsd:progress`, or `/gsd-plan-phase 2` where the token carries ' + + 'trailing arguments — see isLiveCommandToken in tests/helpers/live-command-registry.cjs, the single predicate ' + + 'shared with routing-validity), either of which is skipped entirely: not a smell, not a violation. Exact ' + + 'membership, never a `startsWith` prefix test with an unconstrained remainder — a string merely SHARING a ' + + 'prefix with a real token (e.g. `/gsd-x/../../../etc/passwd`) is not exempted and still surfaces as a SMELL. ' + + 'Without ctx.projectDir the path check is skipped rather than guessed.', check(ctx) { try { /** @type {{message: string, key: string, value: unknown}[]} */ @@ -327,6 +352,9 @@ const ORACLES = Object.freeze([ // Resolved once per runOracles call (this check runs exactly once per ctx), not once // per candidate string below — projectDir is the same value for every leaf in the walk. const resolvedProjectDir = projectDir !== null ? resolveForCompare(projectDir) : null; + // Resolved once per check() call, not once per leaf — getLiveCommandTokens() is itself + // memoized, but there is no reason to re-look-up the memo per candidate string. + const liveTokens = getLiveCommandTokens(); walk(json, (leaf, path) => { if (typeof leaf === 'number' && Number.isNaN(leaf)) { violations.push({ message: `NaN at ${path}`, key: path, value: leaf }); @@ -337,7 +365,8 @@ const ORACLES = Object.freeze([ typeof leaf === 'string' && nodePath.isAbsolute(leaf) && !isUnderProjectDir(resolveForCompare(leaf), resolvedProjectDir) && - !EXTERNAL_PATH_ALLOWED_KEYS.has(leafKeyOf(path)) + !EXTERNAL_PATH_ALLOWED_KEYS.has(leafKeyOf(path)) && + !isLiveCommandToken(leaf, liveTokens) ) { smells.push({ message: `absolute path ${JSON.stringify(leaf)} at ${path} is outside ctx.projectDir ${JSON.stringify(projectDir)}`, @@ -523,25 +552,44 @@ const ORACLES = Object.freeze([ Object.freeze({ id: 'routing-validity', describe: - 'A result.json.recommended / recommended_command token must name a command present in ctx.liveCommands.', + 'Every command token actually carried by the payload must name a command present in ctx.liveCommands. ' + + 'Rather than enumerating fixed field paths (`actions[].command` / `next.command`), this walks the ENTIRE ' + + 'payload for any string whose first whitespace-delimited word starts with one of LIVE_COMMAND_TOKEN_PREFIXES ' + + '(`/gsd-`, `/gsd:`, `$gsd-`) — catching the token wherever it is actually carried: `recommended_command`, a ' + + 'bare-string `next`, `next` as an array of `{command}`, `actions` as an object map, `steps[].command`, ' + + '`actions[].next.command`, etc (#3913 P9 SEC-2 — the fixed-path version missed all of these). A candidate ' + + 'passes only when its first word is an EXACT member of ctx.liveCommands (via the shared ' + + '`isLiveCommandToken` predicate, tests/helpers/live-command-registry.cjs — the SAME predicate ' + + 'value-hygiene\'s command-token exemption uses, so the two can never drift on what a live token is); a ' + + 'pleasing consequence is that value-hygiene therefore exempts exactly the strings routing-validity vouches ' + + 'for. A bare `recommended` id (no `/gsd-`-style prefix — e.g. `discuss-phase`, by design ' + + '`src/smart-entry.cts` `actions.find(a => a.recommended)?.id`, an action id, not a command token) is never a ' + + 'candidate. Engages only when at least one prefix-shaped string is present; a payload with no such string is ' + + 'not engaged (ok).', check(ctx) { try { const json = ctx && ctx.result ? ctx.result.json : undefined; if (json === null || typeof json !== 'object' || Array.isArray(json)) return { ok: true }; - const field = Object.prototype.hasOwnProperty.call(json, 'recommended') - ? 'recommended' - : Object.prototype.hasOwnProperty.call(json, 'recommended_command') - ? 'recommended_command' - : null; - if (field === null) return { ok: true }; - const value = json[field]; - if (typeof value !== 'string') return { ok: true }; const liveCommands = ctx && Array.isArray(ctx.liveCommands) ? ctx.liveCommands : []; - if (!liveCommands.includes(value)) { + const liveTokens = new Set(liveCommands); + const seen = new WeakSet(); + /** @type {{field: string, value: string} | null} */ + let violation = null; + walk(json, (leaf, path) => { + if (violation !== null || typeof leaf !== 'string') return; + const word = firstToken(leaf); + const looksLikeCommandToken = LIVE_COMMAND_TOKEN_PREFIXES.some((prefix) => word.startsWith(prefix)); + if (!looksLikeCommandToken) return; + if (!isLiveCommandToken(leaf, liveTokens)) { + violation = { field: stripRootPrefix(path), value: leaf }; + } + }, seen, '$'); + if (violation !== null) { return { ok: false, severity: SEVERITY.VIOLATION, - detail: `${field}=${JSON.stringify(value)} is not in ctx.liveCommands`, + subject: { key: violation.field, value: violation.value }, + detail: `${violation.field}=${JSON.stringify(violation.value)} is not in ctx.liveCommands`, }; } return { ok: true }; @@ -571,38 +619,14 @@ const ORACLES = Object.freeze([ }, }), - Object.freeze({ - id: 'soft-error-exit-zero', - describe: - 'SMELL, not a violation: result.kind === KIND.SOFT_ERROR means the operation reported failure through a ' + - 'payload key ({error:...}) while exiting 0. Every shell caller\'s `if ! cmd; then` is blind to that failure. ' + - 'This is legal today (42 call sites use output({error:...})) and is deliberately NOT changed here — the ' + - 'blast radius of changing it is CRITICAL — but it is reported so the trade stays visible instead of invisible.', - check(ctx) { - try { - const result = ctx && ctx.result; - if (!result || result.kind !== KIND.SOFT_ERROR) return { ok: true }; - const argv = Array.isArray(result.argv) ? result.argv : []; - const argvDisplay = argv.length ? argv.join(' ') : '(no argv)'; - const errorValue = result.json && typeof result.json === 'object' ? result.json.error : undefined; - return { - ok: false, - severity: SEVERITY.SMELL, - subject: { argv }, - detail: `command "${argvDisplay}" exited 0 but carries result.json.error = ${JSON.stringify(errorValue)}`, - }; - } catch (err) { - return { ok: false, severity: SEVERITY.SMELL, detail: `soft-error-exit-zero threw: ${err && err.message}` }; - } - }, - }), - Object.freeze({ id: 'untyped-success', describe: - 'SMELL, not a violation: result.kind === KIND.PROSE means the command only emits rendered text with no ' + - 'typed surface to assert on. CONTRIBUTING.md forbids asserting on rendered text, so a prose-only command is ' + - 'permanently unassertable by this harness — the gap is in the product, not the test.', + 'VIOLATION: result.kind === KIND.PROSE means the command only emits rendered text with no typed surface to ' + + 'assert on. CONTRIBUTING.md forbids asserting on rendered text, so a prose-only command is permanently ' + + 'unassertable by this harness. This is enforced, not merely tracked: the corpus PROSE count is 0 (#3913) — ' + + 'every executed step across every scenario emits a typed surface, so this oracle can now fire without ever ' + + 'having fired against the shipped corpus. A future prose-only step reddens the build immediately.', check(ctx) { try { const result = ctx && ctx.result; @@ -611,12 +635,12 @@ const ORACLES = Object.freeze([ const argvDisplay = argv.length ? argv.join(' ') : '(no argv)'; return { ok: false, - severity: SEVERITY.SMELL, + severity: SEVERITY.VIOLATION, subject: { argv }, detail: `command "${argvDisplay}" emits KIND.PROSE only; no typed surface exists to assert on`, }; } catch (err) { - return { ok: false, severity: SEVERITY.SMELL, detail: `untyped-success threw: ${err && err.message}` }; + return { ok: false, severity: SEVERITY.VIOLATION, detail: `untyped-success threw: ${err && err.message}` }; } }, }), diff --git a/tests/qa/paths.cjs b/tests/qa/paths.cjs index be9b28853..b01b25f4c 100644 --- a/tests/qa/paths.cjs +++ b/tests/qa/paths.cjs @@ -26,6 +26,24 @@ const fs = require('node:fs'); const path = require('node:path'); +/** + * Realpath implementation used by `realpathNearestAncestor` / `resolveForCompare` by default. + * Prefers `fs.realpathSync.native`, which — unlike plain `fs.realpathSync` — expands Windows + * DOS 8.3 short-name aliases (e.g. `RUNNER~1` -> `runneradmin`). Plain `fs.realpathSync` does + * NOT perform that expansion, so the two forms of the same directory compare as different paths + * (verified from real `windows-latest` CI output: `git_worktree_root` resolved to + * `.../runneradmin/...` while `ctx.projectDir` resolved to `.../RUNNER~1/...` — same directory, + * spelled two ways). Falls back to `fs.realpathSync` when `.native` is unavailable (older Node) + * so this stays a pure enhancement, never a hard dependency. + * + * @param {string} candidate + * @returns {string} + */ +function defaultRealpath(candidate) { + const native = fs.realpathSync && fs.realpathSync.native; + return typeof native === 'function' ? native(candidate) : fs.realpathSync(candidate); +} + /** * Realpath-resolves `candidate` by walking up to its nearest EXISTING ancestor and rejoining * the non-existent suffix, rather than requiring the whole path to exist. Plain @@ -38,18 +56,23 @@ const path = require('node:path'); * (verified: `.planning/NOT-YET.md` under an mkdtemp'd macOS `/var/...` dir was flagged as * outside a `/private/var/...`-resolved project root). Resolving the nearest existing ancestor * and rejoining the missing suffix keeps the comparison correct without requiring the leaf to - * exist. + * exist. `.native` throws ENOENT for a missing path exactly like plain `realpathSync`, so this + * walk-up applies identically regardless of which realpath implementation is injected. * * @param {string} candidate absolute path (may or may not exist on disk) + * @param {(p: string) => string} [realpathFn] injectable realpath implementation — defaults to + * `defaultRealpath` (`.native`-preferring). Tests use this seam to simulate the Windows 8.3 + * short-name case on any platform, since `.native`'s short-name expansion only actually + * occurs on a real Windows host. * @returns {string} realpath-resolved path, with any non-existent suffix rejoined */ -function realpathNearestAncestor(candidate) { +function realpathNearestAncestor(candidate, realpathFn = defaultRealpath) { try { - return fs.realpathSync(candidate); + return realpathFn(candidate); } catch { const parent = path.dirname(candidate); if (parent === candidate) return candidate; // reached a root that itself doesn't exist - return path.join(realpathNearestAncestor(parent), path.basename(candidate)); + return path.join(realpathNearestAncestor(parent, realpathFn), path.basename(candidate)); } } @@ -69,13 +92,15 @@ function realpathNearestAncestor(candidate) { * will regress it. * * @param {string} candidate absolute path (may or may not exist on disk) + * @param {(p: string) => string} [realpathFn] injectable realpath implementation, forwarded to + * `realpathNearestAncestor` — see that function's doc for why this seam exists. * @returns {string} realpath-resolved (nearest-ancestor fallback) path, backslash-normalized */ -function resolveForCompare(candidate) { +function resolveForCompare(candidate, realpathFn = defaultRealpath) { // Unconditional: backslash-separated path strings can arrive as *data* (e.g. a // Windows-style path embedded in JSON) even when running on Linux/macOS, not only when // `fs.realpathSync` itself returns a drive-letter path on native Windows. - return realpathNearestAncestor(candidate).replace(/\\/g, '/'); + return realpathNearestAncestor(candidate, realpathFn).replace(/\\/g, '/'); } /** @@ -224,4 +249,5 @@ module.exports = { isUnderProjectDir, isAbsoluteLike, hasTraversalSegment, + defaultRealpath, }; diff --git a/tests/qa/scenarios/greenfield-happy-path.json b/tests/qa/scenarios/greenfield-happy-path.json index 4e49c6a12..b1de83523 100644 --- a/tests/qa/scenarios/greenfield-happy-path.json +++ b/tests/qa/scenarios/greenfield-happy-path.json @@ -31,7 +31,7 @@ }, { "at": "plan:post", - "run": [["smart-entry"]] + "run": [["smart-entry", "--json"]] }, { "at": "execute:pre", diff --git a/tests/qa/smell-baseline.json b/tests/qa/smell-baseline.json index e8cb8693b..7e49d5a5d 100644 --- a/tests/qa/smell-baseline.json +++ b/tests/qa/smell-baseline.json @@ -1,40 +1,4 @@ { "version": 1, - "smells": [ - { - "key": "0cad2954807d::soft-error-exit-zero|greenfield-happy-path|state-snapshot|(subject-with-no-stable-discriminator)", - "id": "soft-error-exit-zero", - "scenario": "greenfield-happy-path", - "issue": 2980, - "reason": "state-snapshot uses the documented soft-failure idiom (output({error:...}) while exiting 0) when STATE.md does not exist yet on a greenfield project. 42 call sites across the engine use this idiom; oracles.cjs rates changing it CRITICAL blast radius — tracked as a known trade-off in #2980." - }, - { - "key": "373270350622::soft-error-exit-zero|perturbation-delete-artifact|roadmap get-phase 1|(subject-with-no-stable-discriminator)", - "id": "soft-error-exit-zero", - "scenario": "perturbation-delete-artifact", - "issue": 2980, - "reason": "`roadmap get-phase 1` reports ROADMAP.md not found via the same output({error:...})-exit-0 soft-failure idiom after the perturbation deletes the artifact — same known trade-off as the state-snapshot occurrences above, tracked in #2980." - }, - { - "key": "383aca0b3b13::soft-error-exit-zero|perturbation-unicode-headings|roadmap get-phase 1|(subject-with-no-stable-discriminator)", - "id": "soft-error-exit-zero", - "scenario": "perturbation-unicode-headings", - "issue": 2980, - "reason": "Same roadmap get-phase soft-failure idiom as perturbation-delete-artifact, fired here because the unicode-heading perturbation leaves the phase unresolvable — tracked in #2980." - }, - { - "key": "8ff1a5363a3f::untyped-success|greenfield-happy-path|smart-entry|(subject-with-no-stable-discriminator)", - "id": "untyped-success", - "scenario": "greenfield-happy-path", - "issue": 2979, - "reason": "smart-entry emits KIND.PROSE unconditionally, with no typed JSON surface for the harness to assert on — a real product gap tracked in #2979, not a QA-harness defect." - }, - { - "key": "92b6b4718571::soft-error-exit-zero|out-of-order|state-snapshot|(subject-with-no-stable-discriminator)", - "id": "soft-error-exit-zero", - "scenario": "out-of-order", - "issue": 2980, - "reason": "Same state-snapshot soft-failure idiom as greenfield-happy-path, fired here because the out-of-order walk reaches state-snapshot before STATE.md exists — tracked in #2980." - } - ] + "smells": [] }