From 9723d2b7e0a2c8607b930fcd7ef19afbc83ac042 Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 5 Aug 2026 16:44:07 -0400 Subject: [PATCH 01/11] test(#3090): assert which value, not which type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine assertions drove a real fault through a real seam and then checked only that the call did not throw, or that a result was a string, a boolean, an array. Each passed whether the code was right or wrong. #3050 is the canonical instance of the shape: its counter-test asserted effectiveRoot was a string and never which root, so a silent misroute passed it. All nine now assert the exact verdict, derived from the production branch each one reaches and traced back to source rather than taken from the survey. One was worse than a weak assertion. The test targeting resolveWorktreeLinkage's main_worktree path used createTempGitProject, which always seeds .planning/ — so the reason was always has_local_planning and the git-dir comparison the test appears to exercise was unreachable from its own fixture. It was not asserting loosely, it was pointed at the wrong path. The fixture now builds a git project without .planning (projectDoc had to be disabled too, since it defaults to git and would have re-seeded it), and the test reaches the branch it names. Another had no reason assertion anywhere in the file while its four siblings all pinned theirs — the odd one out rather than a convention. The last is mine. The parity guard shipped in #3077 checked typeof and doesNotThrow across four ExecGitFn seams, and that PR described it as failing "the moment any site re-grows its own shape". It could not: a site returning a different value of the same type passed it. All four benign-passthrough outputs are derivable exact values, so it now asserts them and the claim is true. Test names that promised more than their assertions established are corrected to match what they prove. Refs #3057 Co-Authored-By: Claude Opus 5 --- tests/faulty-deps.test.cjs | 53 +++++++++++++++----- tests/worktree-safety.test.cjs | 91 ++++++++++++++++++++-------------- 2 files changed, 95 insertions(+), 49 deletions(-) diff --git a/tests/faulty-deps.test.cjs b/tests/faulty-deps.test.cjs index ba36aebf3..f6728404f 100644 --- a/tests/faulty-deps.test.cjs +++ b/tests/faulty-deps.test.cjs @@ -144,31 +144,58 @@ describe('A. execGit normalization (#3071)', () => { const sharedStub = makeFaultyGit(); // worktree-safety.cts:33 — via resolveWorktreeContext, the exported - // entry point that threads deps.execGit. + // entry point that threads deps.execGit. Derived: existsSync:()=>false + // skips the has_local_planning shortcut, so resolveWorktreeLinkage runs; + // the shared stub's benign passthrough (exitCode:0, empty stdout) for + // both --git-dir and --git-common-dir makes them resolve to the SAME + // path (tmpDir), which is the main_worktree branch + // (src/worktree-safety.cts:196-200) — a `typeof === 'string'` check + // would still pass if a shape regrowth returned e.g. reason:'not_a_git_repo'. const wsResult = worktreeSafety.resolveWorktreeContext(tmpDir, { execGit: sharedStub, existsSync: () => false, }); - assert.strictEqual(typeof wsResult.effectiveRoot, 'string'); - assert.strictEqual(typeof wsResult.mode, 'string'); - assert.strictEqual(typeof wsResult.reason, 'string'); + assert.deepStrictEqual(wsResult, { + effectiveRoot: tmpDir, + mode: 'current_directory', + reason: 'main_worktree', + }); // git-base-branch.cts:32 — trySymbolicRef takes execGit directly as its - // second positional argument (no deps wrapper). - assert.doesNotThrow(() => trySymbolicRef(tmpDir, sharedStub)); + // second positional argument (no deps wrapper). Derived: the stub + // answers `git symbolic-ref ...` with exitCode:0 but empty stdout, and + // trySymbolicRef treats an empty stdout as "unset" regardless of exit + // code (src/git-base-branch.cts:105) — it must return null, not merely + // return without throwing. + assert.strictEqual(trySymbolicRef(tmpDir, sharedStub), null); // worktree-base-ref.cts:88 — evaluateWorktreeBaseDegrade threads - // deps.execGit. + // deps.execGit. Derived: `git rev-parse HEAD` answers exitCode:0 with + // empty stdout, which is the explicit exit0-empty-stdout branch + // (src/worktree-base-ref.cts:401-403) — shouldDegrade:false, + // reason:'no-head', headAbsenceVerified:false (NOT the exit-128 + // "definitive no-head" case, which would report headAbsenceVerified:true). + // A `typeof shouldDegrade === 'boolean'` check would still pass on a + // fail-closed flip to shouldDegrade:true. const wbrResult = evaluateWorktreeBaseDegrade({ execGit: sharedStub, cwd: tmpDir }); - assert.strictEqual(typeof wbrResult.shouldDegrade, 'boolean'); - assert.strictEqual(typeof wbrResult.reason, 'string'); + assert.deepStrictEqual(wbrResult, { + shouldDegrade: false, + reason: 'no-head', + message: null, + headSha: null, + forkRef: null, + forkSha: null, + headAbsenceVerified: false, + }); // verification.cts:226 — defaultPhaseCleanCommitTimesMs takes execGitFn // directly as its third positional argument, typed `= typeof execGit`. - assert.doesNotThrow(() => { - const map = defaultPhaseCleanCommitTimesMs(tmpDir, ['a.md', 'b.md'], sharedStub); - assert.ok(map instanceof Map); - }); + // Derived: `git log ...` answers exitCode:0 with empty stdout, which + // trips the early `logRes.stdout.length === 0` return (empty Map) at + // src/verification.cts:247 — `map instanceof Map` alone would also pass + // for a non-empty map. + const map = defaultPhaseCleanCommitTimesMs(tmpDir, ['a.md', 'b.md'], sharedStub); + assert.deepStrictEqual(map, new Map()); }); }); diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index f19b1dd0b..7d70fd635 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -20,7 +20,8 @@ const assert = require('node:assert/strict'); const path = require('node:path'); const childProcess = require('node:child_process'); const fc = require('fast-check'); -const { createTempGitProject, createTempDir, cleanup } = require('./helpers.cjs'); +const { createTempDir, cleanup } = require('./helpers.cjs'); +const { createFixture } = require('./fixtures/index.cjs'); const { makeFaultyGit } = require('./helpers/faulty-deps.cjs'); const WORKTREE_SAFETY_PATH = path.join( @@ -127,8 +128,8 @@ describe('resolveWorktreeContext', () => { assert.strictEqual(context.mode, 'current_directory'); }); - // Counter-test: timeout returns object with effectiveRoot (Contract 6) - test('returns valid result on timeout, not throw', () => { + // Counter-test: timeout returns the canonical degraded shape, not just an object (Contract 6) + test('returns effectiveRoot=cwd, mode=current_directory, reason=git_timed_out on timeout, not throw', () => { let threw = false; let result; try { @@ -137,11 +138,11 @@ describe('resolveWorktreeContext', () => { threw = true; } assert.strictEqual(threw, false, 'must not throw on timeout'); - assert.strictEqual(typeof result, 'object'); - assert.ok( - typeof result.effectiveRoot === 'string', - 'must return effectiveRoot string even on timeout' - ); + assert.deepStrictEqual(result, { + effectiveRoot: '/tmp', + mode: 'current_directory', + reason: 'git_timed_out', + }); }); // ─── #3050 DEFECT 2: timeout must be distinguishable from not_git_repo ───── @@ -439,7 +440,7 @@ describe('planWorktreePrune', () => { }); // Counter-test: timeout path (Contract 6) - test('returns action=skip when execGit times out', () => { + test('returns action=skip, reason=git_timed_out when execGit times out', () => { let threw = false; let result; try { @@ -450,9 +451,10 @@ describe('planWorktreePrune', () => { assert.strictEqual(threw, false, 'must not throw on timeout'); assert.strictEqual(typeof result, 'object'); assert.strictEqual(result.action, 'skip'); - assert.ok( - typeof result.reason === 'string' && result.reason.length > 0, - 'must return a non-empty reason when git times out' + assert.strictEqual( + result.reason, + 'git_timed_out', + 'must surface the specific git_timed_out reason, not a generic non-empty string' ); }); @@ -524,11 +526,15 @@ describe('executeWorktreePrunePlan', () => { }); // Counter-test: timeout path (Contract 6) - test('returns ok:false when plan is skip (timeout path)', () => { + test('returns {ok:false, action:skip, reason:git_timed_out, pruned:[]} when plan is skip (timeout path)', () => { const plan = planWorktreePrune('/tmp', {}, { execGit: makeTimeoutStub() }); const result = executeWorktreePrunePlan(plan, { execGit: makeTimeoutStub() }); - assert.strictEqual(typeof result, 'object'); - assert.strictEqual(result.ok, false, 'must return ok:false on timeout'); + assert.deepStrictEqual(result, { + ok: false, + action: 'skip', + reason: 'git_timed_out', + pruned: [], + }); }); // AC4 strict: timedOut must be surfaced as a first-class field @@ -578,7 +584,7 @@ describe('listLinkedWorktreePaths', () => { }); // Counter-test: failure path (Contract 6) - test('returns ok:false on timeout, not throw', () => { + test('returns ok:false, reason:git_timed_out on timeout, not throw', () => { let threw = false; let result; try { @@ -588,9 +594,10 @@ describe('listLinkedWorktreePaths', () => { } assert.strictEqual(threw, false, 'must not throw on timeout'); assert.strictEqual(result.ok, false); - assert.ok( - typeof result.reason === 'string' && result.reason.length > 0, - 'must return non-empty reason on timeout' + assert.strictEqual( + result.reason, + 'git_timed_out', + 'must surface the specific git_timed_out reason, not a generic non-empty string' ); }); @@ -641,8 +648,11 @@ describe('inspectWorktreeHealth', () => { ]); }); - // Counter-test: timeout path (Contract 6) - test('returns ok:false when git times out', () => { + // Counter-test: timeout path (Contract 6). This function's own reason on + // timeout is not pinned by any sibling test in this file (unlike + // planWorktreePrune/listLinkedWorktreePaths/snapshotWorktreeInventory, + // which each have a dedicated "reason is git_timed_out" test) — pin it here. + test('returns {ok:false, reason:git_timed_out, findings:[]} when git times out', () => { let threw = false; let result; try { @@ -651,13 +661,16 @@ describe('inspectWorktreeHealth', () => { threw = true; } assert.strictEqual(threw, false, 'must not throw on timeout'); - assert.strictEqual(typeof result, 'object'); - assert.strictEqual(result.ok, false); + assert.deepStrictEqual(result, { + ok: false, + reason: 'git_timed_out', + findings: [], + }); }); - test('findings is empty array (not undefined) on timeout', () => { + test('findings is an empty array, not undefined, on timeout', () => { const result = inspectWorktreeHealth('/tmp', {}, { execGit: makeTimeoutStub() }); - assert.strictEqual(Array.isArray(result.findings), true, 'findings must be an array even when ok:false'); + assert.deepStrictEqual(result.findings, [], 'findings must be [] (not undefined) even when ok:false'); }); // Counter-test (B5, #3057): a statSync throw must surface as its own @@ -734,7 +747,7 @@ describe('snapshotWorktreeInventory', () => { }); // Counter-test: timeout path (Contract 6) - test('returns ok:false with reason on timeout, not throw', () => { + test('returns ok:false, reason:git_timed_out on timeout, not throw', () => { let threw = false; let result; try { @@ -745,9 +758,10 @@ describe('snapshotWorktreeInventory', () => { assert.strictEqual(threw, false, 'must not throw on timeout'); assert.strictEqual(typeof result, 'object'); assert.strictEqual(result.ok, false); - assert.ok( - typeof result.reason === 'string' && result.reason.length > 0, - 'must return non-empty reason on timeout' + assert.strictEqual( + result.reason, + 'git_timed_out', + 'must surface the specific git_timed_out reason, not a generic non-empty string' ); }); @@ -3536,15 +3550,20 @@ describe('worktree-safety: resolveWorktreeRoot and pruneOrphanedWorktrees reloca describe('worktree-safety: resolveWorktreeRoot behaviour', () => { const worktreeSafety = require(WORKTREE_SAFETY_PATH); - test('resolveWorktreeRoot(createTempGitProject()) returns {root, reason} with a non-empty root', (t) => { - const dir = createTempGitProject('gsd-wt-root-'); + // NOTE: createTempGitProject() always seeds .planning/ (createFixture's + // planning:true default), which makes resolveWorktreeContext short-circuit + // on the has_local_planning branch (src/worktree-safety.cts:203-216) BEFORE + // ever calling git — so a fixture built with it can never reach the + // git-based main_worktree path this test's name is about. Use a git fixture + // WITHOUT .planning/ (and without the projectDoc PROJECT.md, which — since + // projectDoc defaults to `git` in createFixture — would otherwise silently + // recreate the .planning/ directory it's writing into) so resolveWorktreeRoot + // actually reaches resolveWorktreeLinkage's real git rev-parse comparison. + test('resolveWorktreeRoot(git repo with no local .planning) reaches the git-based main_worktree path, returns {root: dir, reason: main_worktree}', (t) => { + const dir = createFixture({ prefix: 'gsd-wt-root-', planning: false, git: true, projectDoc: false }); t.after(() => cleanup(dir)); const result = worktreeSafety.resolveWorktreeRoot(dir); - assert.ok(result && typeof result === 'object', 'must return an object, not a bare string'); - assert.ok(typeof result.root === 'string' && result.root.length > 0, - `Expected non-empty root string, got: ${JSON.stringify(result)}`); - assert.ok(typeof result.reason === 'string' && result.reason.length > 0, - `Expected non-empty reason string, got: ${JSON.stringify(result)}`); + assert.deepStrictEqual(result, { root: dir, reason: 'main_worktree' }); }); test('resolveWorktreeRoot propagates git_timed_out via the injected execGit seam (#3050)', () => { From 7dd9e59f6b5ac2d9ffb1b33c04f32b2451dceb4b Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 5 Aug 2026 17:20:56 -0400 Subject: [PATCH 02/11] test(#3090): stop exempting violations under categories that do not fit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An allow-test-rule annotation citing a category that does not apply is worse than no annotation, because it reads as reviewed. Eight were confirmed by reading the assertions each one covered, and auditing the rest found five more plus one refutation — a converter test whose wording described the wrong mechanism while the covered assertion genuinely was deployed-text. The instructive one used the CANONICAL string for the same mistake: STATE.md command output labelled as a deployed artifact. A canonical string is not evidence the category fits, which is why normalising strings alone would have laundered the problem rather than fixed it. Every mapping the audit had inferred rather than code-verified was spot-checked before rewriting, and the ones that turned out not to fit were re-annotated rather than relabelled. Fourteen STATE.md assertions had a typed extractor available all along and now use it; their annotations came out because nothing needs exempting. Eight assertions genuinely need a production change first — CLI stdout and stderr with no structured mode — and are tagged pending-migration-to-typed-ir citing #3090, which is what that category is for. It had zero real uses before this, while one file carried a real citation to migration issue #2974 under a non-canonical tag. Six annotations covered assertions that do no text matching at all. An exemption for a violation that does not exist is noise that makes the real ones harder to audit; those are removed. atomic-write-coverage gains the annotation it always warranted — its own docstring describes a structural-regression-guard while the file carried none. Fifty-nine non-canonical strings across roughly thirty files are normalised, and the allow-test-rule allowlist is regenerated to match. 472 annotations became 463: every one now uses a canonical category, and the two remaining non-canonical strings are ESLint RuleTester fixtures, not annotations. Refs #3057 Co-Authored-By: Claude Opus 5 --- .../lint-allow-test-rule-refs.allowlist.json | 43 ++---- tests/adr-230-pr-target-policy.test.cjs | 4 +- tests/agent-classification-parity.test.cjs | 2 +- tests/atomic-write-coverage.test.cjs | 6 + tests/changeset-cli.test.cjs | 9 +- tests/ci-test-scope.test.cjs | 8 +- tests/claude-imperative-reference.test.cjs | 2 +- tests/cline-imperative-reference.test.cjs | 2 +- tests/clock-seam.test.cjs | 12 +- tests/codex-declarative-reference.test.cjs | 2 +- tests/config-loader.test.cjs | 14 +- tests/config.test.cjs | 4 +- tests/cursor-imperative-reference.test.cjs | 2 +- tests/debug-session-manager-commit.test.cjs | 2 +- tests/docs-parity-live-registry.test.cjs | 10 +- tests/edge-probe-docs-fixtures.test.cjs | 2 +- tests/edge-probe-planner-contract.test.cjs | 2 +- tests/edge-probe-spec-phase-contract.test.cjs | 2 +- tests/execute-phase-wave.test.cjs | 7 +- tests/execute-phase-worktree-guard.test.cjs | 2 +- tests/executor-mvp-tdd-section.test.cjs | 4 +- ...fix-2257-debug-nonterminal-resume.test.cjs | 6 +- ...2598-opencode-background-dispatch.test.cjs | 2 +- tests/fix-2603-kimi-code-host-matrix.test.cjs | 2 +- ...-2615-effortsurface-matrix-parity.test.cjs | 2 +- tests/git-base-branch.test.cjs | 4 +- tests/golden-parity-single-source.test.cjs | 3 +- ...check-update-worker-platform-gate.test.cjs | 6 +- tests/hermes-imperative-reference.test.cjs | 2 +- tests/install-minimal-hooks.test.cjs | 14 +- tests/install.test.cjs | 4 +- tests/inventory-headings-countfree.test.cjs | 2 +- ...ue-2828-flat-roadmap-total-phases.test.cjs | 1 - tests/issue-498-identity-drift-lint.test.cjs | 4 +- ...sue-498-update-backup-runtime-dir.test.cjs | 8 +- ...issue-57-runtime-install-no-drift.test.cjs | 9 +- tests/issue-815-update-next-channel.test.cjs | 8 +- tests/kimi-variant-disambiguation.test.cjs | 11 +- tests/markdown-table.test.cjs | 2 +- tests/model-omit-when-inherit-guard.test.cjs | 2 +- tests/no-phantom-issue-refs.test.cjs | 2 +- tests/opencode-imperative-reference.test.cjs | 2 +- tests/phase.test.cjs | 143 +++++++++++------- tests/planner-decomposition.test.cjs | 2 +- ...policy-138-nyquist-config-default.test.cjs | 2 +- .../prohibition-probe.docs-fixtures.test.cjs | 2 +- ...rohibition-probe.planner-contract.test.cjs | 2 +- tests/prohibition-probe.schema.test.cjs | 2 +- ...ibition-probe.spec-phase-contract.test.cjs | 2 +- tests/prohibition-probe.validators.test.cjs | 2 +- tests/prohibition-probe.verify-tier.test.cjs | 2 +- .../project-instruction-file-parity.test.cjs | 3 +- tests/prompt-budget-cli.test.cjs | 16 +- tests/repo-layout.test.cjs | 5 +- tests/research-agent-profiles.test.cjs | 2 +- tests/resolve-dispatch-type.test.cjs | 5 - tests/roadmap-parser.test.cjs | 4 +- tests/roadmapper-granularity.test.cjs | 2 +- tests/run-tests-harness.test.cjs | 11 +- tests/runtime-converters.test.cjs | 25 ++- tests/runtime-homes-descriptor-drive.test.cjs | 4 +- tests/runtime-launcher-parity.test.cjs | 12 +- tests/runtime-name-policy.test.cjs | 2 +- tests/secure-phase.test.cjs | 6 +- tests/state.test.cjs | 109 ++++++------- ...consideration-probe-docs-fixtures.test.cjs | 2 +- tests/verify.test.cjs | 4 +- tests/workflow-shell-pinning.test.cjs | 6 +- tests/worktree-safety.test.cjs | 7 +- 69 files changed, 365 insertions(+), 256 deletions(-) diff --git a/scripts/lint-allow-test-rule-refs.allowlist.json b/scripts/lint-allow-test-rule-refs.allowlist.json index 4fff797c9..e3686eb7b 100644 --- a/scripts/lint-allow-test-rule-refs.allowlist.json +++ b/scripts/lint-allow-test-rule-refs.allowlist.json @@ -1,5 +1,5 @@ [ - "tests/agent-classification-parity.test.cjs :: runtime-contract-is-the-product — docs/AGENTS.md section layout + docs/INVENTORY.md table ARE the classification surface being validated", + "tests/agent-classification-parity.test.cjs :: source-text-is-the-product — docs/AGENTS.md section layout + docs/INVENTORY.md table ARE the classification surface being validated", "tests/agent-frontmatter.test.cjs :: source-text-is-the-product", "tests/agent-required-reading-consistency.test.cjs :: source-text-is-the-product", "tests/agent-size-budget.test.cjs :: source-text-is-the-product", @@ -15,16 +15,13 @@ "tests/autonomous-interactive.test.cjs :: source-text-is-the-product", "tests/autonomous-to-flag.test.cjs :: source-text-is-the-product", "tests/chain-flag-plan-phase.test.cjs :: source-text-is-the-product", - "tests/changeset-cli.test.cjs :: reads a product workflow .md file (not CJS source) to verify", + "tests/changeset-cli.test.cjs :: source-text-is-the-product", "tests/check-update-config-dir.test.cjs :: structural-regression-guard", - "tests/ci-test-scope.test.cjs :: CLI usage banner presence is a user-facing contract.", - "tests/ci-test-scope.test.cjs :: CLI usage failure text is user-facing contract for this parser guard.", "tests/claude-md.test.cjs :: source-text-is-the-product", "tests/claude-skills-migration.test.cjs :: source-text-is-the-product", "tests/cleanup-branch-pruning.test.cjs :: source-text-is-the-product", "tests/cline-install.test.cjs :: source-text-is-the-product", "tests/cline-support.test.cjs :: source-text-is-the-product", - "tests/clock-seam.test.cjs :: line 159 reads the STATE.md temp file written by readModifyWriteStateMd — this is a runtime output file assertion, not a source-grep; the API returns void so a file read-back is the only way to verify the transform was applied", "tests/code-review-agent-skills.test.cjs :: source-text-is-the-product", "tests/code-review-command.test.cjs :: source-text-is-the-product", "tests/code-review-pipeline-regression.test.cjs :: source-text-is-the-product", @@ -47,18 +44,15 @@ "tests/discuss-phase-power.test.cjs :: source-text-is-the-product", "tests/docs-parity-live-registry.test.cjs :: source-text-is-the-product", "tests/drift-detection.test.cjs :: source-text-is-the-product", - "tests/edge-probe-docs-fixtures.test.cjs :: runtime-contract-is-the-product — the rendered reference/SPEC/ADR vocab surfaces are the runtime contract; this pins their bijection to the code (docs-parity)", - "tests/edge-probe-planner-contract.test.cjs :: runtime-contract-is-the-product — plan-phase.md's planner prompt is the deployed runtime contract under assertion", - "tests/edge-probe-spec-phase-contract.test.cjs :: runtime-contract-is-the-product — spec-phase.md Step 5.5 is the deployed workflow runtime contract under assertion", + "tests/edge-probe-docs-fixtures.test.cjs :: source-text-is-the-product — the rendered reference/SPEC/ADR vocab surfaces are the runtime contract; this pins their bijection to the code (docs-parity)", + "tests/edge-probe-planner-contract.test.cjs :: source-text-is-the-product — plan-phase.md's planner prompt is the deployed runtime contract under assertion", + "tests/edge-probe-spec-phase-contract.test.cjs :: source-text-is-the-product — spec-phase.md Step 5.5 is the deployed workflow runtime contract under assertion", "tests/edit-phase.test.cjs :: source-text-is-the-product", "tests/eslint-rules.test.cjs :: must still error", "tests/eslint-rules.test.cjs :: pending migration", "tests/eslint-rules.test.cjs :: source-text-is-the-product", "tests/execute-phase-active-flags.test.cjs :: source-text-is-the-product", "tests/execute-phase-step-5-5-deviation-doc.test.cjs :: source-text-is-the-product", - "tests/execute-phase-wave.test.cjs :: behavioral — calls gsd-tools and asserts structured output", - "tests/execute-phase-wave.test.cjs :: behavioral — exercises config-set validation, not source text", - "tests/execute-phase-wave.test.cjs :: behavioral — exercises gsd-tools wave-defaulting logic", "tests/execute-phase-wave.test.cjs :: source-text-is-the-product", "tests/execute-phase-worktree-artifacts.test.cjs :: source-text-is-the-product", "tests/explore-command.test.cjs :: source-text-is-the-product", @@ -66,9 +60,9 @@ "tests/forensics.test.cjs :: source-text-is-the-product", "tests/frontmatter-cli.test.cjs :: source-text-is-the-product", "tests/gates-taxonomy.test.cjs :: source-text-is-the-product", - "tests/git-base-branch.test.cjs :: runtime-contract-is-the-product", - "tests/git-base-branch.test.cjs :: runtime-contract-is-the-product — the workflow .md content IS", - "tests/gsd-check-update-worker-platform-gate.test.cjs :: structural assertion on spawn-options shape; the behavior", + "tests/git-base-branch.test.cjs :: source-text-is-the-product", + "tests/git-base-branch.test.cjs :: source-text-is-the-product — the workflow .md content IS", + "tests/gsd-check-update-worker-platform-gate.test.cjs :: structural-regression-guard", "tests/gsd-researcher-app-aware.test.cjs :: source-text-is-the-product", "tests/gsd-researcher-flow-diagram.test.cjs :: source-text-is-the-product", "tests/gsd-settings-advanced.test.cjs :: source-text-is-the-product", @@ -80,21 +74,19 @@ "tests/install-minimal-hooks.test.cjs :: source-text-is-the-product", "tests/install-nested-layout.test.cjs :: source-text-is-the-product", "tests/install-runtime-artifacts.test.cjs :: source-text-is-the-product", - "tests/install.test.cjs :: runtime-contract-is-the-product", "tests/install.test.cjs :: source-text-is-the-product", "tests/intel.test.cjs :: source-text-is-the-product — agents/gsd-intel-updater.md IS the", "tests/intel.test.cjs :: source-text-is-the-product — readFileSync assertions target API-SURFACE.md, which is the generated product of intelApiSurface; asserting on its text content is the only way to verify correct generation.", - "tests/inventory-headings-countfree.test.cjs :: runtime-contract-is-the-product — INVENTORY.md heading format is the shipped doc surface being locked", + "tests/inventory-headings-countfree.test.cjs :: source-text-is-the-product — INVENTORY.md heading format is the shipped doc surface being locked", "tests/ios-scaffold-safety.test.cjs :: source-text-is-the-product", "tests/issue-2639-codex-toml-neutralization.test.cjs :: source-text-is-the-product", "tests/issue-429-comment-text-gate.test.cjs :: source-text-is-the-product", - "tests/issue-498-update-backup-runtime-dir.test.cjs :: structural assertion on the deployed update.md backup bash;", - "tests/issue-57-runtime-install-no-drift.test.cjs :: delegation-presence guard. Catches wholesale removal of the registry", - "tests/issue-57-runtime-install-no-drift.test.cjs :: structural guard over bin/install.js source. Behavioral assertions", + "tests/issue-498-update-backup-runtime-dir.test.cjs :: source-text-is-the-product", + "tests/issue-57-runtime-install-no-drift.test.cjs :: structural-regression-guard", "tests/issue-607-installer-dry-run.install.test.cjs :: integration-test-input", "tests/issue-607-legacy-cleanup.test.cjs :: integration-test-input", "tests/issue-787-cline-hooks-agents.test.cjs :: source-text-is-the-product", - "tests/issue-815-update-next-channel.test.cjs :: reads product workflow/command markdown to verify the --next RC channel contract — not a source-grep test", + "tests/issue-815-update-next-channel.test.cjs :: source-text-is-the-product", "tests/locking-bugs-1909-1916-1925-1927.test.cjs :: architectural-invariant", "tests/mcp-tool-inheritance.test.cjs :: source-text-is-the-product", "tests/milestone-summary.test.cjs :: source-text-is-the-product", @@ -111,7 +103,6 @@ "tests/path-replacement.test.cjs :: source-text-is-the-product", "tests/phase-dependency-levels.test.cjs :: source-text-is-the-product", "tests/phase.test.cjs :: source-text-is-the-product", - "tests/phase.test.cjs :: state-md-is-the-runtime-contract — regression tests for", "tests/phase6-capability-docs.test.cjs :: source-text-is-the-product", "tests/phase6-capstone-conformance.test.cjs :: source-text-is-the-product", "tests/phase6-planning-capabilities.test.cjs :: source-text-is-the-product", @@ -127,7 +118,6 @@ "tests/product-name-purity.test.cjs :: source-text-is-the-product", "tests/profile-output.test.cjs :: source-text-is-the-product", "tests/progress-forensic.test.cjs :: source-text-is-the-product", - "tests/prompt-budget-cli.test.cjs :: prompt-content-is-the-product", "tests/prompt-thinning.test.cjs :: source-text-is-the-product", "tests/quick-session-management.test.cjs :: source-text-is-the-product", "tests/qwen-skills-migration.test.cjs :: source-text-is-the-product", @@ -137,12 +127,11 @@ "tests/release-coverage-scope.test.cjs :: source-text-is-the-product", "tests/release-tarball-smoke-workflow.test.cjs :: source-text-is-the-product", "tests/release-tarball-smoke.install.test.cjs :: integration-test-input", - "tests/research-agent-profiles.test.cjs :: research agent .md content is the governed surface", + "tests/research-agent-profiles.test.cjs :: source-text-is-the-product research agent .md content is the governed surface", "tests/review-default-reviewers-workflow.test.cjs :: source-text-is-the-product", "tests/roadmap.test.cjs :: source-text-is-the-product", - "tests/run-tests-harness.test.cjs :: run-tests.cjs is a CLI test harness whose only IR is its", - "tests/runtime-launcher-parity.test.cjs :: structural parity/drift guard — asserts literal presence/absence of the canonical gsd_run launcher and the retired $GSD_SDK / `/gsd-tools` tokens across workflow markdown; there is no typed IR for \"this source file does not contain substring X\".", - "tests/runtime-name-policy.test.cjs :: runtime-contract-is-the-product — FALLBACK_ALIASES source text IS the", + "tests/runtime-launcher-parity.test.cjs :: structural-regression-guard", + "tests/runtime-name-policy.test.cjs :: source-text-is-the-product — FALLBACK_ALIASES source text IS the", "tests/scan-command.test.cjs :: source-text-is-the-product", "tests/secret-scan-lint.security.test.cjs :: source-text-is-the-product", "tests/secure-phase.test.cjs :: source-text-is-the-product", @@ -166,7 +155,7 @@ "tests/workflow-compat.test.cjs :: source-text-is-the-product", "tests/workflow-guard-registration.test.cjs :: structural-regression-guard", "tests/workflow-maintainer-skip.test.cjs :: source-text-is-the-product", - "tests/workflow-shell-pinning.test.cjs :: file-scope prefilter, not a test assertion — we need to", + "tests/workflow-shell-pinning.test.cjs :: source-text-is-the-product", "tests/workflow-size-budget.test.cjs :: source-text-is-the-product", "tests/workspace.test.cjs :: source-text-is-the-product", "tests/worktree-cleanup.test.cjs :: source-text-is-the-product", diff --git a/tests/adr-230-pr-target-policy.test.cjs b/tests/adr-230-pr-target-policy.test.cjs index 7ef4e4de3..9846b96c9 100644 --- a/tests/adr-230-pr-target-policy.test.cjs +++ b/tests/adr-230-pr-target-policy.test.cjs @@ -1,6 +1,8 @@ 'use strict'; -// allow-test-rule: reads workflow YAML source as the security artifact under test #1190 +// allow-test-rule: source-text-is-the-product (#1190) +// Reads the shipped .github/workflows YAML — the deployed text GitHub +// Actions executes — as the security artifact under test. /** * ADR-230 regression guard: PR target-branch policy. diff --git a/tests/agent-classification-parity.test.cjs b/tests/agent-classification-parity.test.cjs index 8264091e8..f66cd2e83 100644 --- a/tests/agent-classification-parity.test.cjs +++ b/tests/agent-classification-parity.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product — docs/AGENTS.md section layout + docs/INVENTORY.md table ARE the classification surface being validated +// allow-test-rule: source-text-is-the-product — docs/AGENTS.md section layout + docs/INVENTORY.md table ARE the classification surface being validated 'use strict'; /** diff --git a/tests/atomic-write-coverage.test.cjs b/tests/atomic-write-coverage.test.cjs index e5901f8ff..83be53bc2 100644 --- a/tests/atomic-write-coverage.test.cjs +++ b/tests/atomic-write-coverage.test.cjs @@ -1,3 +1,9 @@ +// allow-test-rule: structural-regression-guard (#1972) +// Reads milestone.cjs/phase.cjs/frontmatter.cjs source and parses for bare +// fs.writeFileSync call sites — a specific code pattern that must not exist +// to prevent partial-write corruption. Behavioral tests cannot distinguish +// platformWriteSync from a bare fs.writeFileSync; only source inspection can. + /** * Structural regression guard for atomic write usage (#1972). * diff --git a/tests/changeset-cli.test.cjs b/tests/changeset-cli.test.cjs index ba3b532e3..03247aa0c 100644 --- a/tests/changeset-cli.test.cjs +++ b/tests/changeset-cli.test.cjs @@ -327,7 +327,8 @@ describe('changeset cli extract: version-range changelog extraction (#3496)', () }); // F1: workflows/update.md must reference the extract subcommand invocation. - // allow-test-rule: reads a product workflow .md file (not CJS source) to verify + // allow-test-rule: source-text-is-the-product + // Reads a product workflow .md file (not CJS source) to verify // the user-facing instruction was wired; there is no behavioural runtime to invoke. test('F1: workflows/update.md contains concrete extract subcommand invocation', (_t) => { const workflowPath = path.join(ROOT, 'gsd-core', 'workflows', 'update.md'); @@ -359,7 +360,8 @@ describe('changeset cli extract: version-range changelog extraction (#3496)', () // NOT the old broken path ($GSD_DIR/gsd-core/scripts/changeset/cli.cjs). // The installer copies scripts/changeset/ into /scripts/changeset/, // so the runtime path is $GSD_DIR/scripts/changeset/cli.cjs (#935). - // allow-test-rule: reads a product workflow .md file (not CJS source) to verify + // allow-test-rule: source-text-is-the-product + // Reads a product workflow .md file (not CJS source) to verify // the runtime install path contract; there is no behavioural runtime to invoke. test('F2: update.md CLI path is $GSD_DIR/scripts/changeset/cli.cjs (not gsd-core/scripts/…) (#935)', (_t) => { const workflowPath = path.join(ROOT, 'gsd-core', 'workflows', 'update.md'); @@ -377,7 +379,8 @@ describe('changeset cli extract: version-range changelog extraction (#3496)', () }); // F3: update.md must guard against the CLI being missing (not pure silent-swallow) - // allow-test-rule: reads a product workflow .md file (not CJS source) to verify + // allow-test-rule: source-text-is-the-product + // Reads a product workflow .md file (not CJS source) to verify // the guard is present; there is no behavioural runtime to invoke. test('F3: update.md has an explicit guard when changeset CLI is missing (#935)', (_t) => { const workflowPath = path.join(ROOT, 'gsd-core', 'workflows', 'update.md'); diff --git a/tests/ci-test-scope.test.cjs b/tests/ci-test-scope.test.cjs index 52f4c848a..c39839836 100644 --- a/tests/ci-test-scope.test.cjs +++ b/tests/ci-test-scope.test.cjs @@ -176,9 +176,12 @@ describe('ci-test-scope.cjs', () => { encoding: 'utf8', }); assert.notStrictEqual(r.status, 0); - // allow-test-rule: CLI usage failure text is user-facing contract for this parser guard. + // allow-test-rule: pending-migration-to-typed-ir [#3090] + // Regex-matches the CLI's human-readable stderr formatter (usage banner + + // arg-parser Error#message) — CONTRIBUTING's own BAD example verbatim. + // scripts/ci-test-scope.cjs has no --json / frozen-reason-enum error mode + // yet; adding one is a production change out of scope here. Tracked under #3090. assert.match(r.stderr, /--files requires a value/); - // allow-test-rule: CLI usage banner presence is a user-facing contract. assert.match(r.stderr, /Usage:/); }); @@ -211,7 +214,6 @@ describe('ci-test-scope.cjs', () => { // A plain source file that matches no RULES entry but is under gsd-core/ (code path) const result = scopeFor(['gsd-core/src/some-util.js']); assert.strictEqual(result.code_changed, true); - // allow-test-rule: the unit-fallback contract is the exact subject of bug #408. assert.deepStrictEqual(result.targeted_tests, ['unit'], 'targeted_tests must be [\'unit\'] when code changed but no rule matched'); }); diff --git a/tests/claude-imperative-reference.test.cjs b/tests/claude-imperative-reference.test.cjs index 3cb3245ef..ab1f1da69 100644 --- a/tests/claude-imperative-reference.test.cjs +++ b/tests/claude-imperative-reference.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: AC2 requires asserting no `runtime === 'claude'` string-equality branch remains in bin/install.js — the descriptor-migration contract is a property of the source text, so a source-grep is the only faithful check (#2086) +// allow-test-rule: structural-regression-guard — AC2 requires asserting no `runtime === 'claude'` string-equality branch remains in bin/install.js — the descriptor-migration contract is a property of the source text, so a source-grep is the only faithful check (#2086) 'use strict'; /** diff --git a/tests/cline-imperative-reference.test.cjs b/tests/cline-imperative-reference.test.cjs index a54a4fa8c..9ca29aad2 100644 --- a/tests/cline-imperative-reference.test.cjs +++ b/tests/cline-imperative-reference.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: AC2 requires asserting no `runtime === 'cline'` string-equality branch remains in bin/install.js/src — the descriptor-migration contract is a property of the source text, so a source-grep is the only faithful check (#2090) +// allow-test-rule: structural-regression-guard — AC2 requires asserting no `runtime === 'cline'` string-equality branch remains in bin/install.js/src — the descriptor-migration contract is a property of the source text, so a source-grep is the only faithful check (#2090) 'use strict'; /** diff --git a/tests/clock-seam.test.cjs b/tests/clock-seam.test.cjs index 3b4e0e925..48a8f905a 100644 --- a/tests/clock-seam.test.cjs +++ b/tests/clock-seam.test.cjs @@ -1,5 +1,12 @@ 'use strict'; -// allow-test-rule: line 159 reads the STATE.md temp file written by readModifyWriteStateMd — this is a runtime output file assertion, not a source-grep; the API returns void so a file read-back is the only way to verify the transform was applied +// allow-test-rule: pending-migration-to-typed-ir [#3090] +// This file's assert.throws(fn, /regex/) sites (acquireStateLock / +// readModifyWriteStateMd / withPlanningLock / acquireInstallMigrationLock +// timeout and error-propagation checks) regex-match the human-readable +// thrown Error#message — CONTRIBUTING's "Error / status / reason" BAD +// pattern; the fix is a frozen-enum REASON code on the thrown error, which +// requires a production change to those locking functions that is out of +// scope for this test-only change. Tracked for correction under #3090. /** * Deterministic clock-seam tests for acquireStateLock / withPlanningLock (issue #453). @@ -40,6 +47,7 @@ const stateMod = require('../gsd-core/bin/lib/state.cjs'); const { acquireStateLock, releaseStateLock, readModifyWriteStateMd } = stateMod; const { withPlanningLock } = require('../gsd-core/bin/lib/planning-workspace.cjs'); const { createTempProject, cleanup, runGsdTools } = require('./helpers.cjs'); +const { stateExtractField } = require('../gsd-core/bin/lib/state-document.cjs'); // ───────────────────────────────────────────────────────────────────────────── // 1. Fake-clock proof: acquireStateLock accepts and uses the clock seam @@ -822,7 +830,7 @@ describe('readModifyWriteStateMd lock cleanup on error', () => { const clock = makeFakeClock(0); readModifyWriteStateMd(statePath, (c) => c + '\n**Patched:** yes\n', tmpDir, undefined, clock); const content = fs.readFileSync(statePath, 'utf-8'); - assert.ok(content.includes('**Patched:** yes'), 'transform must be applied'); + assert.strictEqual(stateExtractField(content, 'Patched'), 'yes', 'transform must be applied'); assert.strictEqual(clock.sleepCalls.length, 0, 'no sleep when lock is immediately available'); }); }); diff --git a/tests/codex-declarative-reference.test.cjs b/tests/codex-declarative-reference.test.cjs index 619b1e113..057b6d9a9 100644 --- a/tests/codex-declarative-reference.test.cjs +++ b/tests/codex-declarative-reference.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: AC2 requires asserting no `runtime === 'codex'` string-equality and no positive `isCodex` branch remain in bin/install.js/src — the descriptor-migration contract is a property of the source text, so a source-grep is the only faithful check (#2088) +// allow-test-rule: structural-regression-guard — AC2 requires asserting no `runtime === 'codex'` string-equality and no positive `isCodex` branch remain in bin/install.js/src — the descriptor-migration contract is a property of the source text, so a source-grep is the only faithful check (#2088) 'use strict'; /** diff --git a/tests/config-loader.test.cjs b/tests/config-loader.test.cjs index e0a545528..6398724d9 100644 --- a/tests/config-loader.test.cjs +++ b/tests/config-loader.test.cjs @@ -707,8 +707,6 @@ describe('bug #2638 — sub_repos canonical location', () => { __foldDescribe("folded:bug-3523-cjs-loadconfig-branching-strategy-warning (consolidation epic #1969 B6 #1975)", () => { 'use strict'; -// allow-test-rule: validates runtime CLI stdout/stderr warning behavior, not source grep (see #3523) - /** * Regression tests for #3523 — CJS loadConfig must not emit a false * "unknown config key(s)" warning for `branching_strategy` when that key @@ -833,7 +831,7 @@ describe('bug-3523 — no warning for legacy top-level branching_strategy', () = ); // After migration write-back, config-get should find git.branching_strategy. - const result = runWithStderr(['config-get', 'git.branching_strategy'], tmpDir); + const result = runWithStderr(['config-get', 'git.branching_strategy', '--raw'], tmpDir); assert.equal( result.status, @@ -845,8 +843,9 @@ describe('bug-3523 — no warning for legacy top-level branching_strategy', () = '', `No error should fire when reading migrated branching_strategy (#3523) — got: ${result.stderr}` ); - assert.ok( - result.stdout.includes('milestone'), + assert.equal( + result.stdout.trim(), + 'milestone', `Expected git.branching_strategy to be 'milestone' but got: ${result.stdout}` ); }); @@ -880,6 +879,11 @@ describe('bug-3523 — double-emission reduced to single-emission', () => { const result = runWithStderr(['resolve-model', 'planner'], tmpDir); + // allow-test-rule: pending-migration-to-typed-ir [#3090] + // Counts occurrences of a sentinel substring in the CLI's human-readable + // stderr warning text — no structured "warning count"/warning-list API is + // exposed yet; adding one is a production change out of scope here. + // Tracked under #3090. // Count how many times the sentinel key appears in warnings const warningLines = result.stderr .split('\n') diff --git a/tests/config.test.cjs b/tests/config.test.cjs index 7a8e1b926..2f05699bd 100644 --- a/tests/config.test.cjs +++ b/tests/config.test.cjs @@ -1808,7 +1808,7 @@ describe('#3197 — gsd-tools.cjs config-set workflow._auto_chain_active', () => { const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-3086-git-create-tag-config-gate (consolidation epic #1969 B2 #1971)", () => { -// allow-test-rule: workflow-markdown-is-the-runtime-contract (see #3086) +// allow-test-rule: source-text-is-the-product (see #3086) // Justification: complete-milestone.md IS the runtime — the agent reads and // follows it directly. Asserting the block is present in the // markdown is the only way to verify the gate is wired. Per CONTEXT.md L611. @@ -2285,7 +2285,7 @@ describe('feat-3210 / H5: enum validation for code_quality.fallow.scope and .pro __foldDescribe("folded:bug-3212-execute-phase-stall-safe-resume (consolidation epic #1969 B3 #1972)", () => { 'use strict'; -// allow-test-rule: source-text-is-product [#3212] +// allow-test-rule: source-text-is-the-product [#3212] // The bug is in workflow/config contracts consumed by agents at runtime. const { describe, test } = require('node:test'); diff --git a/tests/cursor-imperative-reference.test.cjs b/tests/cursor-imperative-reference.test.cjs index e0ef266a8..a56d5fe61 100644 --- a/tests/cursor-imperative-reference.test.cjs +++ b/tests/cursor-imperative-reference.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: AC2 requires asserting no `runtime === 'cursor'` string-equality branch remains in bin/install.js/src — the descriptor-migration contract is a property of the source text, so a source-grep is the only faithful check (#2089) +// allow-test-rule: structural-regression-guard — AC2 requires asserting no `runtime === 'cursor'` string-equality branch remains in bin/install.js/src — the descriptor-migration contract is a property of the source text, so a source-grep is the only faithful check (#2089) 'use strict'; /** diff --git a/tests/debug-session-manager-commit.test.cjs b/tests/debug-session-manager-commit.test.cjs index 03fcc2240..acd494d0e 100644 --- a/tests/debug-session-manager-commit.test.cjs +++ b/tests/debug-session-manager-commit.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product see #2568 +// allow-test-rule: source-text-is-the-product see #2568 // agents/gsd-debug-session-manager.md is executed instruction text: the orchestrator // follows it verbatim, so WHERE the commit step sits relative to the terminal vs // non-terminal summary shapes IS the contract. The commit_docs gate it relies on is diff --git a/tests/docs-parity-live-registry.test.cjs b/tests/docs-parity-live-registry.test.cjs index c821e420c..85ed92d32 100644 --- a/tests/docs-parity-live-registry.test.cjs +++ b/tests/docs-parity-live-registry.test.cjs @@ -914,11 +914,11 @@ describe('bug #2950: stale deleted-command references removed from workflow file * integration. (Test-enforced via concept-mapping audit.) */ -// allow-test-rule: structural-IR parser for a docs guide. The .includes() (see #2840) -// calls below build a typed record (commandsPresent flags, conceptPairs -// flags, nonGoalFlags, safetyFlags); assertions run on those booleans, not -// on raw text. This is the documented escape hatch in -// scripts/lint-no-source-grep.cjs for doc-shape tests. +// allow-test-rule: source-text-is-the-product (see #2840) +// docs/issue-driven-orchestration.md's deployed prose IS the product being +// validated for required-concept coverage. The .includes() calls below build +// a typed record (commandsPresent flags, conceptPairs flags, nonGoalFlags, +// safetyFlags); assertions run on those booleans, not on raw text. 'use strict'; diff --git a/tests/edge-probe-docs-fixtures.test.cjs b/tests/edge-probe-docs-fixtures.test.cjs index 61654e665..e64f5d3e2 100644 --- a/tests/edge-probe-docs-fixtures.test.cjs +++ b/tests/edge-probe-docs-fixtures.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product — the rendered reference/SPEC/ADR vocab surfaces are the runtime contract; this pins their bijection to the code (docs-parity) +// allow-test-rule: source-text-is-the-product — the rendered reference/SPEC/ADR vocab surfaces are the runtime contract; this pins their bijection to the code (docs-parity) // Asserts the portable reference doc (gsd-core/references/edge-probe.md) keeps its // worked-example JSON blocks in sync with the source-of-truth fixture files under // gsd-core/references/edge-probe-fixtures/. The fixtures are the canonical data; the diff --git a/tests/edge-probe-planner-contract.test.cjs b/tests/edge-probe-planner-contract.test.cjs index 59bf36094..dd8168ff8 100644 --- a/tests/edge-probe-planner-contract.test.cjs +++ b/tests/edge-probe-planner-contract.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product — plan-phase.md's planner prompt is the deployed runtime contract under assertion +// allow-test-rule: source-text-is-the-product — plan-phase.md's planner prompt is the deployed runtime contract under assertion // plan-phase.md is the deployed planning workflow contract; these checks lock // the SPEC path wiring and quality-gate that the edge-probe review (RR-01/02/03) // requires — assertions scope to extracted sub-blocks to avoid false positives. diff --git a/tests/edge-probe-spec-phase-contract.test.cjs b/tests/edge-probe-spec-phase-contract.test.cjs index 4933cbe71..05b3616f8 100644 --- a/tests/edge-probe-spec-phase-contract.test.cjs +++ b/tests/edge-probe-spec-phase-contract.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product — spec-phase.md Step 5.5 is the deployed workflow runtime contract under assertion +// allow-test-rule: source-text-is-the-product — spec-phase.md Step 5.5 is the deployed workflow runtime contract under assertion // spec-phase.md is the deployed spec workflow contract; these checks lock // the Step 5.5 wiring so the edge-probe.cjs runtime invocation cannot // silently rot the way the original plan-phase no-op did (reviewer finding RR-11). diff --git a/tests/execute-phase-wave.test.cjs b/tests/execute-phase-wave.test.cjs index 38579dfa8..a2ae73a86 100644 --- a/tests/execute-phase-wave.test.cjs +++ b/tests/execute-phase-wave.test.cjs @@ -268,7 +268,6 @@ describe('execute-phase docs: user-facing wave flag', () => { describe('phase-plan-index: wave grouping behavior', () => { test('phase-plan-index groups plans by wave (DAG-bucketing: P002 depends on P001)', () => { - // allow-test-rule: behavioral — calls gsd-tools and asserts structured output const fs = require('fs'); const path = require('path'); const tmpDir = createTempProject(); @@ -334,7 +333,6 @@ describe('phase-plan-index: wave grouping behavior', () => { }); test('phase-plan-index defaults missing wave frontmatter to wave 1', () => { - // allow-test-rule: behavioral — exercises gsd-tools wave-defaulting logic const fs = require('fs'); const path = require('path'); const tmpDir = createTempProject(); @@ -419,7 +417,6 @@ describe('use_worktrees config: cross-workflow structural coverage', () => { }); test('config-set accepts workflow.use_worktrees', () => { - // allow-test-rule: behavioral — exercises config-set validation, not source text const tmpDir = createTempProject(); try { const result = runGsdTools('config-set workflow.use_worktrees true', tmpDir); @@ -802,7 +799,9 @@ describe('execute-phase: between-wave manifest reset (#1369, #3384)', () => { const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-3096-ai-integration-phase-parallel-race (consolidation epic #1969 B4 #1973)", () => { 'use strict'; -// allow-test-rule: reads product workflow markdown (ai-integration-phase.md) to verify structural ordering contract — not a source-grep test (see #3096) +// allow-test-rule: source-text-is-the-product (see #3096) +// Reads product workflow markdown (ai-integration-phase.md) to verify +// structural ordering contract. // Regression guard for bug #3096. // diff --git a/tests/execute-phase-worktree-guard.test.cjs b/tests/execute-phase-worktree-guard.test.cjs index de6ddeb2c..9c3756149 100644 --- a/tests/execute-phase-worktree-guard.test.cjs +++ b/tests/execute-phase-worktree-guard.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product see #1856 +// allow-test-rule: source-text-is-the-product see #1856 // The orchestrator cwd-drift guard (#48) is shell EMBEDDED in execute-phase.md. // The shipped text IS the runtime contract, so these tests extract the block and // EXECUTE it against real git fixtures rather than asserting on its characters — diff --git a/tests/executor-mvp-tdd-section.test.cjs b/tests/executor-mvp-tdd-section.test.cjs index f1b022703..86763ec9c 100644 --- a/tests/executor-mvp-tdd-section.test.cjs +++ b/tests/executor-mvp-tdd-section.test.cjs @@ -105,7 +105,9 @@ describe('gsd-executor — state.* calls use the named-only router form (#1863 r const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-3097-3099-executor-worktree-path-safety (consolidation epic #1969 B7 #1976)", () => { 'use strict'; -// allow-test-rule: reads markdown product files (gsd-executor.md, worktree-path-safety.md) to verify structural protocol — not source-grep (see #3097) +// allow-test-rule: source-text-is-the-product (see #3097) +// Reads markdown product files (gsd-executor.md, worktree-path-safety.md) to +// verify structural protocol. // Regression guards for bug #3097 and #3099. // diff --git a/tests/fix-2257-debug-nonterminal-resume.test.cjs b/tests/fix-2257-debug-nonterminal-resume.test.cjs index 775d4442b..bb9c2f715 100644 --- a/tests/fix-2257-debug-nonterminal-resume.test.cjs +++ b/tests/fix-2257-debug-nonterminal-resume.test.cjs @@ -46,9 +46,11 @@ const DEBUG_MD = path.join(__dirname, '..', 'gsd-core', 'workflows', 'debug.md') const SESSION_MANAGER_MD = path.join(__dirname, '..', 'agents', 'gsd-debug-session-manager.md'); describe('#2257 debug non-terminal session-manager return contract', () => { - // allow-test-rule: workflow/agent prose IS the runtime contract under test #2257 + // allow-test-rule: source-text-is-the-product (#2257) + // workflow/agent prose IS the runtime contract under test const debugContent = fs.readFileSync(DEBUG_MD, 'utf-8'); - // allow-test-rule: workflow/agent prose IS the runtime contract under test #2257 + // allow-test-rule: source-text-is-the-product (#2257) + // workflow/agent prose IS the runtime contract under test const managerContent = fs.readFileSync(SESSION_MANAGER_MD, 'utf-8'); const section4Start = debugContent.indexOf('## 4. Session Management'); diff --git a/tests/fix-2598-opencode-background-dispatch.test.cjs b/tests/fix-2598-opencode-background-dispatch.test.cjs index 82fe1f128..652470bb4 100644 --- a/tests/fix-2598-opencode-background-dispatch.test.cjs +++ b/tests/fix-2598-opencode-background-dispatch.test.cjs @@ -24,7 +24,7 @@ * silently re-assert an unsupported capability. */ -// allow-test-rule: runtime-contract-is-the-product #2598 — the descriptor JSON and the +// allow-test-rule: source-text-is-the-product #2598 — the descriptor JSON and the // host-integration matrix ARE the negotiated contract; asserting their values is behavioral. 'use strict'; diff --git a/tests/fix-2603-kimi-code-host-matrix.test.cjs b/tests/fix-2603-kimi-code-host-matrix.test.cjs index d7b53f26b..962d1369e 100644 --- a/tests/fix-2603-kimi-code-host-matrix.test.cjs +++ b/tests/fix-2603-kimi-code-host-matrix.test.cjs @@ -29,7 +29,7 @@ * disagreement is how the gap survived. */ -// allow-test-rule: runtime-contract-is-the-product #2603 — the descriptor JSON and the +// allow-test-rule: source-text-is-the-product #2603 — the descriptor JSON and the // host-integration matrix ARE the negotiated contract; asserting their values is behavioral. 'use strict'; diff --git a/tests/fix-2615-effortsurface-matrix-parity.test.cjs b/tests/fix-2615-effortsurface-matrix-parity.test.cjs index 0fe485a6f..91f1db443 100644 --- a/tests/fix-2615-effortsurface-matrix-parity.test.cjs +++ b/tests/fix-2615-effortsurface-matrix-parity.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product #2615 — the host-integration matrix +// allow-test-rule: source-text-is-the-product #2615 — the host-integration matrix // IS the cited source of truth for every descriptor axis (ADR-1239); asserting that a // shipped axis value appears there, and matches, is a contract assertion. diff --git a/tests/git-base-branch.test.cjs b/tests/git-base-branch.test.cjs index dcbde474f..c64da3d85 100644 --- a/tests/git-base-branch.test.cjs +++ b/tests/git-base-branch.test.cjs @@ -13,11 +13,11 @@ * G. Anti-regression guard: five affected workflows must NOT contain the * duplicated bare `:-main` / `:-master` fallback pattern that was the root cause. * They must call `gsd_run query git.base-branch` instead. - * (allow-test-rule: runtime-contract-is-the-product — the workflow .md content IS + * (allow-test-rule: source-text-is-the-product — the workflow .md content IS * the runtime surface; the absence of the bad pattern is what ships to agents.) */ -// allow-test-rule: runtime-contract-is-the-product +// allow-test-rule: source-text-is-the-product // Justification: the workflow .md files ARE the product surface — agents read and // execute them directly. Guard G asserts that the resolved command appears in all five // workflows, which requires reading those workflow files. Per TESTING-STANDARDS.md §6. diff --git a/tests/golden-parity-single-source.test.cjs b/tests/golden-parity-single-source.test.cjs index fd23a2b7d..46bf4a4a7 100644 --- a/tests/golden-parity-single-source.test.cjs +++ b/tests/golden-parity-single-source.test.cjs @@ -92,7 +92,8 @@ const CONSUMERS = [ for (const consumer of CONSUMERS) { test(`${consumer.name} does not re-declare an inline buildParityManifest or any exclusion constant (#2266)`, () => { - // allow-test-rule: source text is the product for this anti-divergence check, see #2266 + // allow-test-rule: source-text-is-the-product (see #2266) + // Source text is the product for this anti-divergence check. const content = fs.readFileSync(path.join(ROOT, ...consumer.rel), 'utf8'); for (const { label, re } of FORBIDDEN_INLINE) { assert.ok( diff --git a/tests/gsd-check-update-worker-platform-gate.test.cjs b/tests/gsd-check-update-worker-platform-gate.test.cjs index f3938b066..2dd930574 100644 --- a/tests/gsd-check-update-worker-platform-gate.test.cjs +++ b/tests/gsd-check-update-worker-platform-gate.test.cjs @@ -22,7 +22,8 @@ * is the minimum-cost contract. */ -// allow-test-rule: structural assertion on spawn-options shape; the behavior +// allow-test-rule: structural-regression-guard +// structural assertion on spawn-options shape; the behavior // (Windows-only shell resolution) is platform-gated at runtime and cannot be // reached on POSIX CI without a Windows lane. @@ -295,7 +296,8 @@ describe('Issue #815: --next dist-tag support', () => { * contract for the worker, the same rationale #378 carried. */ -// allow-test-rule: structural assertion on hook delegation; the behavior being (see #378) +// allow-test-rule: structural-regression-guard (see #378) +// structural assertion on hook delegation; the behavior being // tested (correct package name → no E404) only manifests at runtime against the // live npm registry, which CI does not call. diff --git a/tests/hermes-imperative-reference.test.cjs b/tests/hermes-imperative-reference.test.cjs index 889789df6..111af4ea2 100644 --- a/tests/hermes-imperative-reference.test.cjs +++ b/tests/hermes-imperative-reference.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: AC2 requires asserting no `runtime === 'hermes'` string-equality branch remains in bin/install.js — the descriptor-migration contract is a property of the source text, so a source-grep is the only faithful check (#2091) +// allow-test-rule: structural-regression-guard — AC2 requires asserting no `runtime === 'hermes'` string-equality branch remains in bin/install.js — the descriptor-migration contract is a property of the source text, so a source-grep is the only faithful check (#2091) 'use strict'; /** diff --git a/tests/install-minimal-hooks.test.cjs b/tests/install-minimal-hooks.test.cjs index c31492f1e..4e6306f64 100644 --- a/tests/install-minimal-hooks.test.cjs +++ b/tests/install-minimal-hooks.test.cjs @@ -2652,7 +2652,7 @@ describe('enh-770: gsd-config-reload.js hook script', () => { }); test('gsd-config-reload.js contains the gsd-hook-version stamp', () => { - // allow-test-rule: runtime-contract-is-the-product — the stamp template token (see #770) + // allow-test-rule: source-text-is-the-product — the stamp template token (see #770) // IS the product surface that the installer must find and replace with the // real version at copy time; asserting its presence is required. const content = fs.readFileSync(reloadScript, 'utf8'); @@ -2663,7 +2663,7 @@ describe('enh-770: gsd-config-reload.js hook script', () => { }); test('gsd-config-reload.js reads from stdin and emits JSON output', () => { - // allow-test-rule: runtime-contract-is-the-product — the stdin-read and (see #770) + // allow-test-rule: source-text-is-the-product — the stdin-read and (see #770) // JSON-emit pattern IS the hook contract; asserting its presence is required. const content = fs.readFileSync(reloadScript, 'utf8'); assert.ok( @@ -2673,7 +2673,7 @@ describe('enh-770: gsd-config-reload.js hook script', () => { }); test('gsd-config-reload.js targets the FileChanged hook event', () => { - // allow-test-rule: runtime-contract-is-the-product — the hookEventName is (see #770) + // allow-test-rule: source-text-is-the-product — the hookEventName is (see #770) // the protocol surface; asserting its presence verifies the contract. const content = fs.readFileSync(reloadScript, 'utf8'); assert.ok( @@ -2693,7 +2693,7 @@ describe('enh-770: hooks/hooks.json plugin manifest includes new hook events', ( }); test('hooks.json contains SubagentStop event', () => { - // allow-test-rule: runtime-contract-is-the-product — hooks.json IS the (see #770) + // allow-test-rule: source-text-is-the-product — hooks.json IS the (see #770) // plugin manifest surface that Claude Code reads at plugin load time. const content = JSON.parse(fs.readFileSync(hooksJsonPath, 'utf8')); assert.ok( @@ -2703,7 +2703,7 @@ describe('enh-770: hooks/hooks.json plugin manifest includes new hook events', ( }); test('hooks.json contains Stop event', () => { - // allow-test-rule: runtime-contract-is-the-product — hooks.json IS the (see #770) + // allow-test-rule: source-text-is-the-product — hooks.json IS the (see #770) // plugin manifest surface that Claude Code reads at plugin load time. const content = JSON.parse(fs.readFileSync(hooksJsonPath, 'utf8')); assert.ok( @@ -2713,7 +2713,7 @@ describe('enh-770: hooks/hooks.json plugin manifest includes new hook events', ( }); test('hooks.json contains PreCompact event', () => { - // allow-test-rule: runtime-contract-is-the-product — hooks.json IS the (see #770) + // allow-test-rule: source-text-is-the-product — hooks.json IS the (see #770) // plugin manifest surface that Claude Code reads at plugin load time. const content = JSON.parse(fs.readFileSync(hooksJsonPath, 'utf8')); assert.ok( @@ -2723,7 +2723,7 @@ describe('enh-770: hooks/hooks.json plugin manifest includes new hook events', ( }); test('hooks.json contains FileChanged event', () => { - // allow-test-rule: runtime-contract-is-the-product — hooks.json IS the (see #770) + // allow-test-rule: source-text-is-the-product — hooks.json IS the (see #770) // plugin manifest surface that Claude Code reads at plugin load time. const content = JSON.parse(fs.readFileSync(hooksJsonPath, 'utf8')); assert.ok( diff --git a/tests/install.test.cjs b/tests/install.test.cjs index 74322766e..cb127e548 100644 --- a/tests/install.test.cjs +++ b/tests/install.test.cjs @@ -1153,7 +1153,7 @@ describe('readCmdNames() — tolerates missing commands/gsd directory (#1223)', }); // ─── Section N: Antigravity .agents canonical workspace dir (#791) ───────────── -// allow-test-rule: runtime-contract-is-the-product +// allow-test-rule: source-text-is-the-product // Reads deployed agent .md files whose text IS the product surface the // Antigravity runtime loads at startup (path references, command names). @@ -1289,7 +1289,7 @@ describe('install — --devin-desktop CLI flag routes to windsurf runtime (#792) }); }); // ─── Section N: Windsurf workflow slash-command install (#1615) ───────────── -// allow-test-rule: runtime-contract-is-the-product +// allow-test-rule: source-text-is-the-product // Reads deployed workflow .md files whose text IS the product surface the // Windsurf runtime loads at startup (path references, command names). diff --git a/tests/inventory-headings-countfree.test.cjs b/tests/inventory-headings-countfree.test.cjs index 722bd676f..75c0f18a4 100644 --- a/tests/inventory-headings-countfree.test.cjs +++ b/tests/inventory-headings-countfree.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product — INVENTORY.md heading format is the shipped doc surface being locked +// allow-test-rule: source-text-is-the-product — INVENTORY.md heading format is the shipped doc surface being locked 'use strict'; /** diff --git a/tests/issue-2828-flat-roadmap-total-phases.test.cjs b/tests/issue-2828-flat-roadmap-total-phases.test.cjs index f3aec51d7..e89de566d 100644 --- a/tests/issue-2828-flat-roadmap-total-phases.test.cjs +++ b/tests/issue-2828-flat-roadmap-total-phases.test.cjs @@ -1,4 +1,3 @@ -// allow-test-rule: behavioral-fs-fixture (#2828) 'use strict'; // Regression guard for #2828: on a flat unmilestoned roadmap (no versioned milestone diff --git a/tests/issue-498-identity-drift-lint.test.cjs b/tests/issue-498-identity-drift-lint.test.cjs index c5dcf2131..705aef82b 100644 --- a/tests/issue-498-identity-drift-lint.test.cjs +++ b/tests/issue-498-identity-drift-lint.test.cjs @@ -76,7 +76,9 @@ describe('Issue #498: the live repo passes the drift lint', () => { const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-170-workflow-fallback-install-hint (consolidation epic #1969 B4 #1973)", () => { 'use strict'; -// allow-test-rule: workflow markdown is shipped product text; this test validates fallback hint literals across all workflow files (see #170) +// allow-test-rule: source-text-is-the-product (see #170) +// Workflow markdown is shipped product text; this test validates fallback +// hint literals across all workflow files. const { test } = require('node:test'); const assert = require('node:assert/strict'); diff --git a/tests/issue-498-update-backup-runtime-dir.test.cjs b/tests/issue-498-update-backup-runtime-dir.test.cjs index bb06df9f7..454b85a9d 100644 --- a/tests/issue-498-update-backup-runtime-dir.test.cjs +++ b/tests/issue-498-update-backup-runtime-dir.test.cjs @@ -18,9 +18,11 @@ * program; asserting their shape is asserting on the deployed contract. */ -// allow-test-rule: structural assertion on the deployed update.md backup bash; -// the data-loss behavior only manifests against a real install during a clean -// reinstall, which CI does not perform. +// allow-test-rule: source-text-is-the-product +// update.md's bash blocks ARE the deployed /gsd:update program; asserting +// their shape is asserting on the deployed contract. The data-loss behavior +// only manifests against a real install during a clean reinstall, which CI +// does not perform. 'use strict'; diff --git a/tests/issue-57-runtime-install-no-drift.test.cjs b/tests/issue-57-runtime-install-no-drift.test.cjs index 2a06b4af5..626b7d246 100644 --- a/tests/issue-57-runtime-install-no-drift.test.cjs +++ b/tests/issue-57-runtime-install-no-drift.test.cjs @@ -159,7 +159,8 @@ describe('issue-57 AC2 — config-mutation dispatch is closed over the explicit } }); - // allow-test-rule: structural guard over bin/install.js source. Behavioral assertions + // allow-test-rule: structural-regression-guard + // structural guard over bin/install.js source. Behavioral assertions // cannot observe inline `runtime === '...'` config branching, so this enforces that // every inline per-runtime branch references a runtime the adapter registry knows // about — a NEW branch against an unregistered runtime name fails here. It matches @@ -184,7 +185,8 @@ describe('issue-57 AC2 — config-mutation dispatch is closed over the explicit ); }); - // allow-test-rule: structural guard over bin/install.js source (#2103). VS Code + // allow-test-rule: structural-regression-guard (#2103) + // structural guard over bin/install.js source. VS Code // (capabilities/vscode/capability.json) is a registry runtime (role:runtime, for // validator/host-integration coverage) but is NEVER CLI-installed — it is a // Marketplace/VSIX extension with no --vscode flag and no allRuntimes membership @@ -214,7 +216,8 @@ describe('issue-57 AC2 — config-mutation dispatch is closed over the explicit ); }); - // allow-test-rule: delegation-presence guard. Catches wholesale removal of the registry + // allow-test-rule: structural-regression-guard + // delegation-presence guard. Catches wholesale removal of the registry // dispatch (a regression to scattered per-runtime config branching). Presence-style, not // absence-grep, so it does not bite on incidental non-config `runtime === '...'` checks. test('bin/install.js requires the config adapter registry and dispatches through it', () => { diff --git a/tests/issue-815-update-next-channel.test.cjs b/tests/issue-815-update-next-channel.test.cjs index da853f91f..e82aefbee 100644 --- a/tests/issue-815-update-next-channel.test.cjs +++ b/tests/issue-815-update-next-channel.test.cjs @@ -1,5 +1,7 @@ 'use strict'; -// allow-test-rule: reads product workflow/command markdown to verify the --next RC channel contract — not a source-grep test +// allow-test-rule: source-text-is-the-product +// Reads product workflow/command markdown to verify the --next RC channel +// contract. // Issue #815: `/gsd-update --next` (alias `--rc`) must thread the @next dist-tag // through the whole update flow (version check + install) while leaving the @@ -102,7 +104,9 @@ describe('update.md — no bare ~.claude path references (#2470)', () => { const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-3130-update-npx-robust-invocation (consolidation epic #1969 B4 #1973)", () => { 'use strict'; -// allow-test-rule: reads product workflow markdown (update.md) to verify structural invocation contract — not a source-grep test (see #3130) +// allow-test-rule: source-text-is-the-product (see #3130) +// Reads product workflow markdown (update.md) to verify structural +// invocation contract. // Regression guard for bug #3130. // diff --git a/tests/kimi-variant-disambiguation.test.cjs b/tests/kimi-variant-disambiguation.test.cjs index 4d39066a1..37240e8c0 100644 --- a/tests/kimi-variant-disambiguation.test.cjs +++ b/tests/kimi-variant-disambiguation.test.cjs @@ -1,8 +1,9 @@ -// allow-test-rule: behavioral-subprocess-test — see #2505 — Phase 5 kimi-variant -// disambiguation is verified via install.js subprocess output capture, since -// the disambiguateKimiVariant function is inline in bin/install.js (not -// exported). The test sets a disposable HOME, creates the probe config files, -// and asserts on the printed notices. +// allow-test-rule: pending-migration-to-typed-ir [#3090] +// Phase 5 kimi-variant disambiguation is verified via install.js subprocess +// output capture (regex on printed notices), since disambiguateKimiVariant +// is inline in bin/install.js (not exported) and emits no structured/JSON +// output. Exporting the function and/or adding a --json notice mode is a +// production change out of scope here. Tracked under #3090. process.env.GSD_TEST_MODE = '1'; const { test, describe, before, after } = require('node:test'); diff --git a/tests/markdown-table.test.cjs b/tests/markdown-table.test.cjs index e7f3e8b73..f3d5ed37f 100644 --- a/tests/markdown-table.test.cjs +++ b/tests/markdown-table.test.cjs @@ -930,7 +930,7 @@ describe('TABLE_SCHEMAS parity: registry headers must appear verbatim in their s */ function assertHeaderInFile(relPath, variant) { const fullPath = path.join(ROOT, relPath); - const content = fs.readFileSync(fullPath, 'utf8'); // allow-test-rule: runtime-contract-is-the-product — template/registry parity (#2242) + const content = fs.readFileSync(fullPath, 'utf8'); // allow-test-rule: source-text-is-the-product — template/registry parity (#2242) const expectedHeader = buildHeader(variant); const normalizedExpected = normalize(expectedHeader); const found = content diff --git a/tests/model-omit-when-inherit-guard.test.cjs b/tests/model-omit-when-inherit-guard.test.cjs index 4982defa0..16198daec 100644 --- a/tests/model-omit-when-inherit-guard.test.cjs +++ b/tests/model-omit-when-inherit-guard.test.cjs @@ -1,5 +1,5 @@ // allow-test-rule: structural-regression-guard see #2517 -// allow-test-rule: runtime-contract-is-the-product see #2684 +// allow-test-rule: source-text-is-the-product see #2684 // Guards the omit-when-inherit fix: workflow orchestrators must instruct the agent to // OMIT the model= param from Agent() calls when the *_model var is "inherit" or empty. // Without it, model="" is passed verbatim and 404s on non-Claude runtimes diff --git a/tests/no-phantom-issue-refs.test.cjs b/tests/no-phantom-issue-refs.test.cjs index fc56fcffd..e0ad727dd 100644 --- a/tests/no-phantom-issue-refs.test.cjs +++ b/tests/no-phantom-issue-refs.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product (see #1073) — this guard asserts the +// allow-test-rule: source-text-is-the-product (see #1073) — this guard asserts the // ABSENCE of phantom pre-migration issue references in repo text (docs, tests, // workflows). The file *content* is the product surface here (#1073): dangling // refs that don't exist in open-gsd/gsd-core mislead triage and manufacture diff --git a/tests/opencode-imperative-reference.test.cjs b/tests/opencode-imperative-reference.test.cjs index 982b5f8d7..2894aff35 100644 --- a/tests/opencode-imperative-reference.test.cjs +++ b/tests/opencode-imperative-reference.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: AC2 requires asserting no `runtime === 'opencode'` string-equality branch remains in bin/install.js/src — the descriptor-migration contract is a property of the source text, so a source-grep is the only faithful check (#2087) +// allow-test-rule: structural-regression-guard — AC2 requires asserting no `runtime === 'opencode'` string-equality branch remains in bin/install.js/src — the descriptor-migration contract is a property of the source text, so a source-grep is the only faithful check (#2087) 'use strict'; /** diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index b50cde6ad..cfadc479a 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -1,10 +1,6 @@ // allow-test-rule: source-text-is-the-product // Reads .md/.json/.yml product files whose deployed text IS what the // runtime loads — testing text content tests the deployed contract. -// allow-test-rule: state-md-is-the-runtime-contract — regression tests for -// bug #3517 assert the exact STATE.md fields written by phase.complete; -// STATE.md IS the product surface being verified, not source code. -// Migration to typed-IR parser tracked in #2974. /** * GSD Tools Tests - Phase @@ -5757,6 +5753,13 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c // ───────────────────────────────────────────────────────────────────────────── { + // Typed STATE.md surfaces (#3090) — replaces raw regex/substring matching + // on STATE.md content written by phase.complete in this bug-#3517 block. + const { stateExtractField } = require('../gsd-core/bin/lib/state-document.cjs'); + const { extractFrontmatter } = require('../gsd-core/bin/lib/frontmatter.cjs'); + const { parseMarkdownTable } = require('../gsd-core/bin/lib/markdown-table.cjs'); + const { parsePhaseFromProse } = require('../gsd-core/bin/lib/phase-id.cjs'); + function runSdkQuery(args, cwd) { if (Array.isArray(args) && args[0] === 'phase.complete') { writePassedVerificationForPhase(cwd, args[1]); @@ -5986,24 +5989,24 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c assert.ok(r1.success, `first call failed: ${r1.error}`); const stateAfter1 = fs.readFileSync(statePath, 'utf8'); - const match1 = stateAfter1.match(/completed_phases:\s*(\d+)/); - assert.ok(match1, 'completed_phases not found in frontmatter after first call'); + const progress1 = extractFrontmatter(stateAfter1).progress; + assert.ok(progress1, 'progress not found in frontmatter after first call'); assert.equal( - Number(match1[1]), + Number(progress1.completed_phases), 2, - `After first call: completed_phases should be 2 (derived from ROADMAP: phases 4 and 5 complete), got ${match1[1]}`, + `After first call: completed_phases should be 2 (derived from ROADMAP: phases 4 and 5 complete), got ${progress1.completed_phases}`, ); const r2 = runSdkQuery(['phase.complete', '5'], tmpDir); assert.ok(r2.success, `second call failed: ${r2.error}`); const stateAfter2 = fs.readFileSync(statePath, 'utf8'); - const match2 = stateAfter2.match(/completed_phases:\s*(\d+)/); - assert.ok(match2, 'completed_phases not found in frontmatter after second call'); + const progress2 = extractFrontmatter(stateAfter2).progress; + assert.ok(progress2, 'progress not found in frontmatter after second call'); assert.equal( - Number(match2[1]), + Number(progress2.completed_phases), 2, - `After second call (same phase): completed_phases must remain 2 (idempotent), got ${match2[1]}`, + `After second call (same phase): completed_phases must remain 2 (idempotent), got ${progress2.completed_phases}`, ); }); @@ -6015,16 +6018,16 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c assert.ok(r.success, `call failed: ${r.error}`); const state = fs.readFileSync(statePath, 'utf8'); - const stoppedMatch = state.match(/stopped_at:\s*(.+)/); - assert.ok(stoppedMatch, 'stopped_at not found in frontmatter'); + const stoppedAt = extractFrontmatter(state).stopped_at; + assert.ok(stoppedAt, 'stopped_at not found in frontmatter'); assert.ok( - !stoppedMatch[1].includes('05-03-PLAN.md'), - `stopped_at should not still say "Completed 05-03-PLAN.md" — got: ${stoppedMatch[1]}`, + !stoppedAt.includes('05-03-PLAN.md'), + `stopped_at should not still say "Completed 05-03-PLAN.md" — got: ${stoppedAt}`, ); assert.ok( - stoppedMatch[1].toLowerCase().includes('phase 5') || - stoppedMatch[1].toLowerCase().includes('complete'), - `stopped_at should reference phase 5 completion, got: ${stoppedMatch[1]}`, + stoppedAt.toLowerCase().includes('phase 5') || + stoppedAt.toLowerCase().includes('complete'), + `stopped_at should reference phase 5 completion, got: ${stoppedAt}`, ); }); @@ -6036,10 +6039,8 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c assert.ok(r.success, `call failed: ${r.error}`); const state = fs.readFileSync(statePath, 'utf8'); - const lastUpdatedMatch = state.match(/last_updated:\s*(.+)/); - assert.ok(lastUpdatedMatch, 'last_updated not found in frontmatter'); - - const raw = lastUpdatedMatch[1].trim().replace(/^"(.*)"$/, '$1'); + const raw = extractFrontmatter(state).last_updated; + assert.ok(raw, 'last_updated not found in frontmatter'); // Must have been refreshed — not the stale seed value from setupPhase3517Project assert.notEqual( @@ -6073,10 +6074,10 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c assert.ok(r.success, `call failed: ${r.error}`); const state = fs.readFileSync(statePath, 'utf8'); - const match = state.match(/total_plans:\s*(\d+)/); - assert.ok(match, 'total_plans not found in frontmatter'); - const totalPlans = Number(match[1]); - assert.ok(Number.isFinite(totalPlans) && totalPlans > 0, `total_plans must be a positive number, got: ${match[1]}`); + const progress = extractFrontmatter(state).progress; + assert.ok(progress && progress.total_plans !== undefined, 'total_plans not found in frontmatter'); + const totalPlans = Number(progress.total_plans); + assert.ok(Number.isFinite(totalPlans) && totalPlans > 0, `total_plans must be a positive number, got: ${progress.total_plans}`); }); test('frontmatter completed_plans is updated from SUMMARY file count after phase.complete', () => { @@ -6087,9 +6088,9 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c assert.ok(r.success, `call failed: ${r.error}`); const state = fs.readFileSync(statePath, 'utf8'); - const match = state.match(/completed_plans:\s*(\d+)/); - assert.ok(match, 'completed_plans not found in frontmatter'); - const completedPlans = Number(match[1]); + const progress = extractFrontmatter(state).progress; + assert.ok(progress && progress.completed_plans !== undefined, 'completed_plans not found in frontmatter'); + const completedPlans = Number(progress.completed_plans); assert.equal( completedPlans, 10, @@ -6105,9 +6106,9 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c assert.ok(r.success, `call failed: ${r.error}`); const state = fs.readFileSync(statePath, 'utf8'); - const match = state.match(/percent:\s*(\d+)/); - assert.ok(match, 'percent not found in frontmatter'); - assert.equal(Number(match[1]), 67, `percent should be 67 (2/3 phases), got: ${match[1]}`); + const progress = extractFrontmatter(state).progress; + assert.ok(progress && progress.percent !== undefined, 'percent not found in frontmatter'); + assert.equal(Number(progress.percent), 67, `percent should be 67 (2/3 phases), got: ${progress.percent}`); }); test('state frontmatter and numeric phase line reflect next phase after phase.complete', () => { @@ -6118,8 +6119,19 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c assert.ok(r.success, `call failed: ${r.error}`); const state = fs.readFileSync(statePath, 'utf8'); - assert.match(state, /completed_phases:\s*2/, 'completed_phases must be updated in frontmatter'); - assert.match(state, /Phase:\s*0?6\b/, 'numeric Phase line should advance to phase 6'); + const progress = extractFrontmatter(state).progress; + assert.equal( + Number(progress && progress.completed_phases), + 2, + `completed_phases must be updated in frontmatter, got: ${progress && progress.completed_phases}`, + ); + const phaseLine = stateExtractField(state, 'Phase'); + const { phase: nextPhase } = parsePhaseFromProse(phaseLine); + assert.equal( + Number(nextPhase), + 6, + `numeric Phase line should advance to phase 6, got Phase line: ${phaseLine}`, + ); }); test('prose-block STATE keeps next phase name without field-miss warnings (#1316)', () => { @@ -6143,16 +6155,27 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c ); const state = fs.readFileSync(path.join(planningDir, 'STATE.md'), 'utf8'); - assert.match(state, /current_phase:\s*"?33"?/, 'current_phase frontmatter must advance to 33'); - assert.match( - state, - /^Phase:\s*33\s+—\s+Follow Up Implementation\b/m, - `Current Position Phase line must keep the next phase name; state:\n${state}`, + const currentPhase = extractFrontmatter(state).current_phase; + assert.equal( + String(currentPhase), + '33', + `current_phase frontmatter must advance to 33, got: ${currentPhase}`, ); + + const phaseLine = stateExtractField(state, 'Phase'); + const { phase: nextPhase, name: nextPhaseName } = parsePhaseFromProse(phaseLine); + assert.equal(Number(nextPhase), 33, `Current Position Phase line must advance to 33; got Phase line: ${phaseLine}`); + assert.equal( + nextPhaseName, + 'Follow Up Implementation', + `Current Position Phase line must keep the next phase name; got Phase line: ${phaseLine}`, + ); + + const lastActivity = stateExtractField(state, 'Last activity'); assert.match( - state, - /^Last activity:\s*\d{4}-\d{2}-\d{2}\s+—\s+Phase 32 complete/m, - `Last activity line must use the template em-dash delimiter with narrative; state:\n${state}`, + lastActivity || '', + /^\d{4}-\d{2}-\d{2}\s+—\s+Phase 32 complete/, + `Last activity line must use the template em-dash delimiter with narrative; got: ${lastActivity}`, ); }); @@ -6164,10 +6187,14 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c assert.ok(r.success, `call failed: ${r.error}`); const state = fs.readFileSync(statePath, 'utf8'); - assert.match( - state, - /\|\s*5\s*\|\s*7\s*\|/, - `By Phase table should have a row for phase 5 with 7 summaries.\nState:\n${state}`, + const table = parseMarkdownTable(state); + assert.ok(table.ok, `By Phase table must parse; reason: ${table.ok ? '' : table.reason}`); + const row = table.value.rows.find((r) => r.Phase.trim() === '5'); + assert.ok(row, `By Phase table should have a row for phase 5.\nState:\n${state}`); + assert.equal( + row.Plans.trim(), + '7', + `By Phase table row for phase 5 should show 7 summaries, got row: ${JSON.stringify(row)}`, ); }); @@ -6181,10 +6208,24 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c const state = fs.readFileSync(statePath, 'utf8'); - assert.match(state, /completed_phases:\s*2/, 'completed_phases must be 2 (4 and 5 complete)'); - assert.match(state, /percent:\s*67/, 'percent must be 67%'); - const hasPhase6 = /Phase:\s*0?6/.test(state) || /current_phase:\s*0?6/.test(state); - assert.ok(hasPhase6, `STATE.md must reference Phase 6 as current after completing Phase 5.\nState:\n${state}`); + const fm = extractFrontmatter(state); + assert.equal( + Number(fm.progress && fm.progress.completed_phases), + 2, + `completed_phases must be 2 (4 and 5 complete), got: ${fm.progress && fm.progress.completed_phases}`, + ); + assert.equal( + Number(fm.progress && fm.progress.percent), + 67, + `percent must be 67%, got: ${fm.progress && fm.progress.percent}`, + ); + const phaseLine = stateExtractField(state, 'Phase'); + const { phase: bodyPhase } = parsePhaseFromProse(phaseLine); + const hasPhase6 = Number(bodyPhase) === 6 || Number(fm.current_phase) === 6; + assert.ok( + hasPhase6, + `STATE.md must reference Phase 6 as current after completing Phase 5. body Phase line: ${phaseLine}, frontmatter current_phase: ${fm.current_phase}`, + ); }); }); } diff --git a/tests/planner-decomposition.test.cjs b/tests/planner-decomposition.test.cjs index de42eb391..2894b9d79 100644 --- a/tests/planner-decomposition.test.cjs +++ b/tests/planner-decomposition.test.cjs @@ -148,7 +148,7 @@ describe('reference files contain key content from original mode sections', () = __foldDescribe("folded:bug-3320-planner-deep-work-rules (consolidation epic #1969 B4 #1973)", () => { 'use strict'; -// allow-test-rule: source-text-is-product [#3320] +// allow-test-rule: source-text-is-the-product [#3320] // The bug is a contradiction in prompt/workflow source text. These assertions // intentionally pin the contract words that planner agents consume. diff --git a/tests/policy-138-nyquist-config-default.test.cjs b/tests/policy-138-nyquist-config-default.test.cjs index 72f5a11dc..afacb332b 100644 --- a/tests/policy-138-nyquist-config-default.test.cjs +++ b/tests/policy-138-nyquist-config-default.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product — asserts GSD workflow/template markdown prose, the executable contract (#138, #2117) +// allow-test-rule: source-text-is-the-product — asserts GSD workflow/template markdown prose, the executable contract (#138, #2117) 'use strict'; // Policy regression test for issue #138: diff --git a/tests/prohibition-probe.docs-fixtures.test.cjs b/tests/prohibition-probe.docs-fixtures.test.cjs index 3481a1c4a..e08e5ca0f 100644 --- a/tests/prohibition-probe.docs-fixtures.test.cjs +++ b/tests/prohibition-probe.docs-fixtures.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product (see #644) — the rendered reference doc's worked-example +// allow-test-rule: source-text-is-the-product (see #644) — the rendered reference doc's worked-example // vocab surface is the runtime contract; this pins its bijection to the source-of-truth fixtures (docs-parity). // // RED-first parity contract: the portable reference doc (gsd-core/references/prohibition-probe.md) keeps diff --git a/tests/prohibition-probe.planner-contract.test.cjs b/tests/prohibition-probe.planner-contract.test.cjs index 0daeb73cf..ffe8a32b8 100644 --- a/tests/prohibition-probe.planner-contract.test.cjs +++ b/tests/prohibition-probe.planner-contract.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product (see #644) — plan-phase.md's planner prompt is the deployed +// allow-test-rule: source-text-is-the-product (see #644) — plan-phase.md's planner prompt is the deployed // runtime contract under assertion (the workflow PROSE is the product). // // RED-first PROSE-PRESENCE contract for the plan-phase lift of confirmed prohibitions. plan-phase.md diff --git a/tests/prohibition-probe.schema.test.cjs b/tests/prohibition-probe.schema.test.cjs index 78172f0d1..bef484a14 100644 --- a/tests/prohibition-probe.schema.test.cjs +++ b/tests/prohibition-probe.schema.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product (see #644) — the must_haves.prohibitions: block is the +// allow-test-rule: source-text-is-the-product (see #644) — the must_haves.prohibitions: block is the // runtime plan-contract surface; this pins its parse/round-trip/projection bijection to the code. // // RED-first schema contract for the `must_haves.prohibitions:` SIBLING block (ADR-550 Decision 3 — diff --git a/tests/prohibition-probe.spec-phase-contract.test.cjs b/tests/prohibition-probe.spec-phase-contract.test.cjs index d95b546cd..0423d5db9 100644 --- a/tests/prohibition-probe.spec-phase-contract.test.cjs +++ b/tests/prohibition-probe.spec-phase-contract.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product (see #644) — spec-phase.md Step 5.6 is the deployed workflow +// allow-test-rule: source-text-is-the-product (see #644) — spec-phase.md Step 5.6 is the deployed workflow // runtime contract under assertion (the workflow PROSE is the product; ADR-550 D5 forbids a JS engine here). // // RED-first PROSE-PRESENCE contract for the prohibition probe's Step 5.6 (ADR-550 D1 DIVERGENCE, diff --git a/tests/prohibition-probe.validators.test.cjs b/tests/prohibition-probe.validators.test.cjs index f64961753..69d0f5f05 100644 --- a/tests/prohibition-probe.validators.test.cjs +++ b/tests/prohibition-probe.validators.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product (see #644) — the prohibition validators and the verify-time +// allow-test-rule: source-text-is-the-product (see #644) — the prohibition validators and the verify-time // disposition are the deployed safety contract; this pins them against the CANONICAL fixture corpus // and the ADR-550 D4 "judgment is never silently green" invariant so the code can never drift from // its own documented intent again. diff --git a/tests/prohibition-probe.verify-tier.test.cjs b/tests/prohibition-probe.verify-tier.test.cjs index ef9ac82c6..ed38c57a0 100644 --- a/tests/prohibition-probe.verify-tier.test.cjs +++ b/tests/prohibition-probe.verify-tier.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product (see #644) — the verify-time disposition of a test-tier +// allow-test-rule: source-text-is-the-product (see #644) — the verify-time disposition of a test-tier // prohibition is the deployed safety contract; this pins its fail-closed default to the code. // // RED-first SAFETY HALF of ADR-550 Decision 5(d) [maintainer decision 2026-06-12 "B-with-guard"]. diff --git a/tests/project-instruction-file-parity.test.cjs b/tests/project-instruction-file-parity.test.cjs index 53de6d8f3..bae91ae4e 100644 --- a/tests/project-instruction-file-parity.test.cjs +++ b/tests/project-instruction-file-parity.test.cjs @@ -89,7 +89,8 @@ describe('bug #1529: getProjectInstructionFile ↔ gsd-tools query parity', () = }); describe('bug #1529: new-project.md workflow uses the shared policy query', () => { - // allow-test-rule: structural drift guard for #1529 — the workflow's bash block MUST invoke the + // allow-test-rule: structural-regression-guard (#1529) + // the workflow's bash block MUST invoke the // shared `gsd_run query project-instruction-file` query rather than a hardcoded // codex-only `if/else` branch; there is no typed IR for "this bash block calls a // specific gsd-tools query instead of a hardcoded mapping". diff --git a/tests/prompt-budget-cli.test.cjs b/tests/prompt-budget-cli.test.cjs index 34d64c1bb..b1e6ed8b4 100644 --- a/tests/prompt-budget-cli.test.cjs +++ b/tests/prompt-budget-cli.test.cjs @@ -1,12 +1,14 @@ 'use strict'; -// allow-test-rule: prompt-content-is-the-product -// The prompt-budget CLI writes an assembled, trimmed prompt string to disk. -// Testing that the prompt omits a dropped section (research) requires a -// content assertion on the output file — the file content IS the product. -// Structured metadata (omitted[], hardFailed, etc.) is always the primary -// assertion; text content checks are secondary and only used to verify the -// trim policy was applied correctly to the assembled output. +// allow-test-rule: pending-migration-to-typed-ir [#3090] +// The prompt-budget CLI writes an assembled, trimmed prompt string to disk — +// a computed/assembled output, not shipped source text, so this is not +// source-text-is-the-product (same mistake as the STATE.md-as-deployed- +// artifact miscategorization). Structured metadata (omitted[], hardFailed, +// etc.) is the primary assertion; the secondary raw substring check that the +// dropped research content is actually absent from the assembled prompt has +// no typed IR to convert to — prompt text is unstructured by nature. +// Tracked under #3090. /** * prompt-budget-cli.test.cjs diff --git a/tests/repo-layout.test.cjs b/tests/repo-layout.test.cjs index 61a95c8d0..e54f37523 100644 --- a/tests/repo-layout.test.cjs +++ b/tests/repo-layout.test.cjs @@ -1,5 +1,8 @@ 'use strict'; -// allow-test-rule: structural guard-placement verification in bin/install.js requires source-text analysis; install.js is a non-exportable CLI script and the guard must be in a specific lexical scope which require()+behavior cannot verify #1188 +// allow-test-rule: structural-regression-guard (#1188) +// Guard-placement verification in bin/install.js requires source-text +// analysis; install.js is a non-exportable CLI script and the guard must be +// in a specific lexical scope which require()+behavior cannot verify. /** * Governance tests for the gsd-core repository root layout. diff --git a/tests/research-agent-profiles.test.cjs b/tests/research-agent-profiles.test.cjs index bab307129..27ad21435 100644 --- a/tests/research-agent-profiles.test.cjs +++ b/tests/research-agent-profiles.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: research agent .md content is the governed surface +// allow-test-rule: source-text-is-the-product research agent .md content is the governed surface // The 7 researcher agent .md files are the deployed AI agent definitions — their // frontmatter and @-includes ARE what the runtime loads. Asserting on their content // is asserting on the deployed contract, not the test author's source code. diff --git a/tests/resolve-dispatch-type.test.cjs b/tests/resolve-dispatch-type.test.cjs index 289386491..47ade08dd 100644 --- a/tests/resolve-dispatch-type.test.cjs +++ b/tests/resolve-dispatch-type.test.cjs @@ -1,8 +1,3 @@ -// allow-test-rule: behavioral-query-coverage — see #2505 — this test exercises the -// resolve-dispatch-type query end-to-end (subprocess) AND the pure -// resolveDispatchType function (require), covering the runtime-aware dispatch -// contract from epic #2505 Phase 4 (#2508). The query is the workflow-facing -// surface; the function is the pure projection. process.env.GSD_TEST_MODE = '1'; const { test, describe } = require('node:test'); diff --git a/tests/roadmap-parser.test.cjs b/tests/roadmap-parser.test.cjs index 4bb37a815..df91773fb 100644 --- a/tests/roadmap-parser.test.cjs +++ b/tests/roadmap-parser.test.cjs @@ -1529,7 +1529,9 @@ describe('bug #730 — milestone (Phase Details) section scope resolution', () = const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-3128-roadmap-plan-count-slug-layout (consolidation epic #1969 B3 #1972)", () => { 'use strict'; -// allow-test-rule: reads roadmap.cjs source to verify isPlanFile pattern was adopted — structural contract prevents silent regression to old filter (see #3128) +// allow-test-rule: structural-regression-guard (see #3128) +// Reads roadmap.cjs source to verify isPlanFile pattern was adopted — +// structural contract prevents silent regression to the old filter. // Regression guard for bug #3128. // diff --git a/tests/roadmapper-granularity.test.cjs b/tests/roadmapper-granularity.test.cjs index 61bdd9b2f..e5c0c5f31 100644 --- a/tests/roadmapper-granularity.test.cjs +++ b/tests/roadmapper-granularity.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product agent .md instruction surface see #1205 +// allow-test-rule: source-text-is-the-product agent .md instruction surface see #1205 // agents/gsd-roadmapper.md is the deployed agent — the Granularity Calibration table // AND the phase_id_convention instructions ARE the deployed behavior. Asserting on // their prose asserts what runs in production (#163, #1205). diff --git a/tests/run-tests-harness.test.cjs b/tests/run-tests-harness.test.cjs index adaec76c7..d0970a73b 100644 --- a/tests/run-tests-harness.test.cjs +++ b/tests/run-tests-harness.test.cjs @@ -1,7 +1,10 @@ -// allow-test-rule: run-tests.cjs is a CLI test harness whose only IR is its -// stable stderr line `run-tests: suite="X" files=N: name1 name2 ...` plus its -// exit code. No typed IR is exposable from a shell script; the printed line -// IS the contract this test pins. See docs/TESTING-SUITES.md and issue #3597. +// allow-test-rule: pending-migration-to-typed-ir [#3090] +// run-tests.cjs is a CLI test harness with no --json/structured output mode; +// these tests regex/substring-match its human-readable stderr (usage errors, +// the `run-tests: suite="X" files=N: name1 name2 ...` selection line) instead +// of a frozen typed IR. Adding a structured output mode is a production +// change out of scope here. See docs/TESTING-SUITES.md and issue #3597. +// Tracked under #3090. // // Tests for scripts/run-tests.cjs --suite filtering (issue #3597). // diff --git a/tests/runtime-converters.test.cjs b/tests/runtime-converters.test.cjs index 0992e3289..17d012130 100644 --- a/tests/runtime-converters.test.cjs +++ b/tests/runtime-converters.test.cjs @@ -973,7 +973,11 @@ test('claude runtime does NOT rewrite the runtime default — stamping is non-cl // --------------------------------------------------------------------------- test('regression: every edited workflow gets codex-stamped (source↔engine parity, all surfaces) (#1515)', () => { - // allow-test-rule: emitted workflow runtime-resolution shell block is the runtime contract surface (#1515) — asserts on engine-transformed output of the real source + // allow-test-rule: pending-migration-to-typed-ir [#3090] + // `out` is _applyRuntimeRewrites's engine-transformed shell text, not shipped + // source — substring-matching it is the "Rendered file" row CONTRIBUTING + // requires a typed-IR builder for. No such IR exists yet for the shell + // rewrite output; production change out of scope here. Tracked under #3090. const WORKFLOWS = ['execute-phase.md', 'autonomous.md', 'manager.md', 'diagnose-issues.md', 'quick.md']; const CLAUDE_RUNTIME = 'config-get runtime --default claude --raw 2>/dev/null || echo "claude"'; const CODEX_RUNTIME = 'config-get runtime --default codex --raw 2>/dev/null || echo "codex"'; @@ -1065,7 +1069,11 @@ const FALSE_WT_LINE = 'config-get workflow.use_worktrees --default false --raw 2 // --------------------------------------------------------------------------- test('parity: every non-Claude runtime stamps its own runtime default and use_worktrees=false on all workflows (#1521)', () => { - // allow-test-rule: emitted workflow runtime-resolution shell block is the runtime contract surface (#1521) + // allow-test-rule: pending-migration-to-typed-ir [#3090] + // `out` is _applyRuntimeRewrites's engine-transformed shell text, not shipped + // source — substring-matching it is the "Rendered file" row CONTRIBUTING + // requires a typed-IR builder for. No such IR exists yet for the shell + // rewrite output; production change out of scope here. Tracked under #3090. for (const rt of NON_CLAUDE) { for (const wf of WORKFLOWS) { const src = fs.readFileSync( @@ -1181,7 +1189,10 @@ test('property: _stampNonClaudeRuntimeDefaults is idempotent (#1521)', () => { // --------------------------------------------------------------------------- test('execute-phase.md, quick.md, and diagnose-issues.md guards are generalized to != "claude" (not Codex-specific) (#1521)', () => { - // allow-test-rule: emitted workflow runtime-resolution shell block is the runtime contract surface (#1521) + // allow-test-rule: source-text-is-the-product (#1521) + // Reads the raw shipped workflow .md source directly (not engine-transformed + // output) and asserts on its literal guard-clause text — the deployed prose + // IS the runtime contract here. // #2584 Phase 3 (#2627): execute-phase.md graduated PAST the `!= "claude"` // guard — worktree isolation there is now keyed on the negotiated // `dispatch.isolation` capability, so no runtime name appears in its guard at @@ -1226,7 +1237,9 @@ test('execute-phase.md, quick.md, and diagnose-issues.md guards are generalized // --------------------------------------------------------------------------- test('manager.md and autonomous.md gate run_in_background on FLATTEN=false, not a runtime name (#1521, graduated by #1708)', () => { - // allow-test-rule: orchestration dispatch gating in manager/autonomous .md is the runtime contract surface (#1521/#1708) + // allow-test-rule: source-text-is-the-product (#1521/#1708) + // Reads the raw shipped manager.md/autonomous.md workflow prose directly — + // the deployed orchestration dispatch gating text IS the runtime contract. const manager = fs.readFileSync( path.join(__dirname, '..', 'gsd-core', 'workflows', 'manager.md'), 'utf8', @@ -1263,7 +1276,9 @@ test('manager.md and autonomous.md gate run_in_background on FLATTEN=false, not }); test('manager.md and autonomous.md no longer contain old "not claude" background-dispatch gating (#1521)', () => { - // allow-test-rule: orchestration dispatch gating in manager/autonomous .md is the runtime contract surface (#1521) + // allow-test-rule: source-text-is-the-product (#1521) + // Reads the raw shipped manager.md/autonomous.md workflow prose directly — + // the deployed orchestration dispatch gating text IS the runtime contract. const manager = fs.readFileSync( path.join(__dirname, '..', 'gsd-core', 'workflows', 'manager.md'), 'utf8', diff --git a/tests/runtime-homes-descriptor-drive.test.cjs b/tests/runtime-homes-descriptor-drive.test.cjs index e59afb949..7fcf57307 100644 --- a/tests/runtime-homes-descriptor-drive.test.cjs +++ b/tests/runtime-homes-descriptor-drive.test.cjs @@ -881,7 +881,9 @@ describe('descriptor-driven parity: 13 non-probe registry runtimes × no-env-var const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-3126-global-skills-base-runtime-path (consolidation epic #1969 B3 #1972)", () => { 'use strict'; -// allow-test-rule: last three tests read init.cjs source to verify delegation contract to runtime-homes.cjs — structural guard, no behavioral IR exposed (see #3126) +// allow-test-rule: structural-implementation-guard (see #3126) +// Last three tests read init.cjs source to verify delegation contract to +// runtime-homes.cjs — no behavioral IR exposed yet for this wiring point. // Regression guard for bug #3126. // diff --git a/tests/runtime-launcher-parity.test.cjs b/tests/runtime-launcher-parity.test.cjs index 02b4bec96..cc54cdb60 100644 --- a/tests/runtime-launcher-parity.test.cjs +++ b/tests/runtime-launcher-parity.test.cjs @@ -21,7 +21,8 @@ * can satisfy gsd_run for Codex shim-only installs. */ -// allow-test-rule: structural parity/drift guard — asserts literal presence/absence of the canonical gsd_run launcher and the retired $GSD_SDK / `/gsd-tools` tokens across workflow markdown; there is no typed IR for "this source file does not contain substring X". +// allow-test-rule: structural-regression-guard +// structural parity/drift guard — asserts literal presence/absence of the canonical gsd_run launcher and the retired $GSD_SDK / `/gsd-tools` tokens across workflow markdown; there is no typed IR for "this source file does not contain substring X". const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); @@ -790,7 +791,8 @@ describe('runtime-launcher-parity — agents (#1041)', () => { * When all three miss, exit non-zero. */ -// allow-test-rule: structural/behavioral regression for the ~/.claude fallback arm in (see #211) +// allow-test-rule: structural-regression-guard (see #211) +// structural/behavioral regression for the ~/.claude fallback arm in // the gsd_run launcher snippet -- asserts literal substring presence and exercises the // bash resolution path via execFileSync; there is no typed IR for "snippet contains arm X". @@ -998,7 +1000,8 @@ describe('bug-211: launcher ~/.claude home fallback', () => { * (sync-runtime-launcher.cjs was re-run after editing the snippet). */ -// allow-test-rule: structural/behavioral regression for non-Claude runtime-home (see #891) +// allow-test-rule: structural-regression-guard (see #891) +// structural/behavioral regression for non-Claude runtime-home // fallback arms in the gsd_run launcher snippet -- asserts literal substring // presence for each runtime-home probe and exercises the bash resolution paths // via execFileSync; there is no typed IR for "snippet contains arm X". @@ -1606,7 +1609,8 @@ describe('bug-3668: workflow SDK resolver supports installed user projects', { s * (C) Precedence: repo-local .claude/ wins over $HOME/.claude/ when both exist. */ -// allow-test-rule: structural/behavioral regression for the repo-local .claude/ install (see #444) +// allow-test-rule: structural-regression-guard (see #444) +// structural/behavioral regression for the repo-local .claude/ install // arm in the gsd_run launcher snippet -- asserts literal substring presence and exercises // the bash resolution path via execFileSync; there is no typed IR for "snippet contains arm X". diff --git a/tests/runtime-name-policy.test.cjs b/tests/runtime-name-policy.test.cjs index 2fb0e5f43..5c81cd6f1 100644 --- a/tests/runtime-name-policy.test.cjs +++ b/tests/runtime-name-policy.test.cjs @@ -54,7 +54,7 @@ describe('runtime-name-policy windsurf alias parity — manifest vs FALLBACK_ALI test('manifest and FALLBACK_ALIASES windsurf alias sets are identical', () => { // Read FALLBACK_ALIASES from source to detect manual drift before a build. const srcPath = path.join(ROOT, 'src', 'runtime-name-policy.cts'); - // allow-test-rule: runtime-contract-is-the-product — FALLBACK_ALIASES source text IS the + // allow-test-rule: source-text-is-the-product — FALLBACK_ALIASES source text IS the // product contract for runtimes that can't load the manifest at runtime; verifying // both surfaces contain the same windsurf aliases catches manual-mirror drift. const src = fs.readFileSync(srcPath, 'utf8'); diff --git a/tests/secure-phase.test.cjs b/tests/secure-phase.test.cjs index 526f249d2..3758cd206 100644 --- a/tests/secure-phase.test.cjs +++ b/tests/secure-phase.test.cjs @@ -634,7 +634,7 @@ describe('SECURE: threat-model-anchored behaviour', () => { }); // ─── 8. Regression: security config variables resolved before use (#1625) ──── -// allow-test-rule: runtime-contract-is-the-product — secure-phase.md prose is the executed contract (#1625) +// allow-test-rule: source-text-is-the-product — secure-phase.md prose is the executed contract (#1625) describe('SECURE: security config variables resolved before use (#1625)', () => { const wfPath = path.join(WORKFLOWS_DIR, 'secure-phase.md'); @@ -718,7 +718,9 @@ describe('SECURE: security config variables resolved before use (#1625)', () => const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-3120-secure-phase-empty-register (consolidation epic #1969 B4 #1973)", () => { 'use strict'; -// allow-test-rule: reads product workflow markdown (secure-phase.md) to verify structural guard contract — not a source-grep test (see #3120) +// allow-test-rule: source-text-is-the-product (see #3120) +// Reads product workflow markdown (secure-phase.md) to verify structural +// guard contract. // Regression guard for bug #3120. // diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 8605438b5..8dd769293 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -6918,10 +6918,6 @@ describe('bug #3454: state mutation must preserve literal $N amounts', () => { __foldDescribe("folded:bug-3489-complete-phase-idempotent (consolidation epic #1969 B2 #1971)", () => { 'use strict'; -// allow-test-rule: source-text-is-the-product (see #3489) -// State.md is the deployed artifact; asserting on its literal text content -// tests the deployed contract. - /** * Regression test for #3489 * @@ -6941,6 +6937,7 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); const { createTempProject, cleanup, runGsdTools } = require('./helpers.cjs'); +const { stateExtractField } = require('../gsd-core/bin/lib/state-document.cjs'); describe('bug #3489: state complete-phase must be idempotent', () => { let tmpDir; @@ -7030,8 +7027,9 @@ describe('bug #3489: state complete-phase must be idempotent', () => { assert.ok(result.success, `command failed: ${result.error || result.output}`); const after = fs.readFileSync(statePath, 'utf8'); - assert.ok( - after.includes('**Status:** Phase 03 complete'), + assert.equal( + stateExtractField(after, 'Status'), + 'Phase 03 complete', `expected Status updated to "Phase 03 complete", got:\n${after}`, ); @@ -7051,7 +7049,6 @@ describe('bug #3489: state complete-phase must be idempotent', () => { const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-397-state-preserve-executor-authored (consolidation epic #1969 B2 #1971)", () => { 'use strict'; -// allow-test-rule: reads runtime STATE.md written to temp dir — behavioral output test, not source-grep (see #397) // Regression tests for bug #397. // @@ -7086,6 +7083,8 @@ const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); const { cleanup } = require('./helpers.cjs'); +const { stateExtractField } = require('../gsd-core/bin/lib/state-document.cjs'); +const { collectSection } = require('../gsd-core/bin/lib/markdown-sectionizer.cjs'); const ROOT = path.join(__dirname, '..'); const TOOLS_PATH = path.join(ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs'); @@ -7279,12 +7278,12 @@ describe('bug #397: executor-authored STATE.md fields must be preserved', () => const r = runGsdState(['record-session', '--stopped-at', 'Plan 1 complete'], dir); assert.ok(r.success, `record-session failed: ${r.error}`); const after = readState(dir); - const rfMatch = after.match(/Resume File:\s*(.+)/i); - assert.ok(rfMatch, 'Resume File field not found in STATE.md after record-session'); + const resumeFile = stateExtractField(after, 'Resume File'); + assert.ok(resumeFile, 'Resume File field not found in STATE.md after record-session'); assert.strictEqual( - rfMatch[1].trim(), + resumeFile, '/home/user/my-custom-context.md', - `record-session overwrote executor-authored Resume File with '${rfMatch[1].trim()}'`, + `record-session overwrote executor-authored Resume File with '${resumeFile}'`, ); } finally { cleanup(dir); @@ -7298,12 +7297,12 @@ describe('bug #397: executor-authored STATE.md fields must be preserved', () => const r = runGsdState(['record-session', '--stopped-at', 'Plan complete'], dir); assert.ok(r.success, `record-session failed: ${r.error}`); const after = readState(dir); - const rfMatch = after.match(/Resume File:\s*(.+)/i); - assert.ok(rfMatch, 'Resume File field not found in STATE.md after record-session'); + const resumeFile = stateExtractField(after, 'Resume File'); + assert.ok(resumeFile, 'Resume File field not found in STATE.md after record-session'); assert.strictEqual( - rfMatch[1].trim(), + resumeFile, 'None', - `Expected 'None' to remain when it was already 'None', got: ${rfMatch[1].trim()}`, + `Expected 'None' to remain when it was already 'None', got: ${resumeFile}`, ); } finally { cleanup(dir); @@ -7317,12 +7316,12 @@ describe('bug #397: executor-authored STATE.md fields must be preserved', () => const r = runGsdState(['record-session', '--resume-file', '/tmp/new-resume.md'], dir); assert.ok(r.success, `record-session failed: ${r.error}`); const after = readState(dir); - const rfMatch = after.match(/Resume File:\s*(.+)/i); - assert.ok(rfMatch, 'Resume File field not found in STATE.md after record-session'); + const resumeFile = stateExtractField(after, 'Resume File'); + assert.ok(resumeFile, 'Resume File field not found in STATE.md after record-session'); assert.strictEqual( - rfMatch[1].trim(), + resumeFile, '/tmp/new-resume.md', - `Expected explicit --resume-file value to be written, got: ${rfMatch[1].trim()}`, + `Expected explicit --resume-file value to be written, got: ${resumeFile}`, ); } finally { cleanup(dir); @@ -7338,12 +7337,12 @@ describe('bug #397: executor-authored STATE.md fields must be preserved', () => assert.ok(r.success, `advance-plan failed: ${r.error}`); const after = readState(dir); // The Configuration-level Status must not be clobbered - const statusMatch = after.match(/^Status:\s*(.+)/m); - assert.ok(statusMatch, 'Status field not found after advance-plan'); + const status = stateExtractField(after, 'Status'); + assert.ok(status, 'Status field not found after advance-plan'); assert.strictEqual( - statusMatch[1].trim(), + status, 'Awaiting QA sign-off before proceeding', - `advance-plan overwrote executor-authored Status: got '${statusMatch[1].trim()}'`, + `advance-plan overwrote executor-authored Status: got '${status}'`, ); } finally { cleanup(dir); @@ -7358,17 +7357,17 @@ describe('bug #397: executor-authored STATE.md fields must be preserved', () => const r = runGsdState(['advance-plan'], dir); assert.ok(r.success, `advance-plan failed: ${r.error}`); const after = readState(dir); - const statusMatch = after.match(/^Status:\s*(.+)/m); - assert.ok(statusMatch, 'Status field not found after advance-plan'); + const status = stateExtractField(after, 'Status'); + assert.ok(status, 'Status field not found after advance-plan'); // 'Ready to execute' is a known default and should be replaced assert.notStrictEqual( - statusMatch[1].trim(), + status, 'Ready to execute', `Status should have been updated from 'Ready to execute' after phase-complete, but was not`, ); assert.ok( - statusMatch[1].includes('Phase complete') || statusMatch[1].includes('ready for verification'), - `Expected phase-complete Status text, got: '${statusMatch[1].trim()}'`, + status.includes('Phase complete') || status.includes('ready for verification'), + `Expected phase-complete Status text, got: '${status}'`, ); } finally { cleanup(dir); @@ -7384,12 +7383,12 @@ describe('bug #397: executor-authored STATE.md fields must be preserved', () => assert.ok(r.success, `advance-plan failed: ${r.error}`); const after = readState(dir); // The top-level Last Activity (in Configuration section) must be preserved - const laMatch = after.match(/^Last Activity:\s*(.+)/im); - assert.ok(laMatch, 'Last Activity field not found after advance-plan'); + const lastActivity = stateExtractField(after, 'Last Activity'); + assert.ok(lastActivity, 'Last Activity field not found after advance-plan'); assert.strictEqual( - laMatch[1].trim(), + lastActivity, 'Unblocked after infra fix — merged PR #88 manually', - `advance-plan overwrote executor-authored Last Activity: got '${laMatch[1].trim()}'`, + `advance-plan overwrote executor-authored Last Activity: got '${lastActivity}'`, ); } finally { cleanup(dir); @@ -7404,21 +7403,21 @@ describe('bug #397: executor-authored STATE.md fields must be preserved', () => const r = runGsdState(['advance-plan'], dir); assert.ok(r.success, `advance-plan failed: ${r.error}`); const after = readState(dir); - const posMatch = after.match(/##\s*Current Position\s*\n([\s\S]*?)(?=\n##|$)/i); - assert.ok(posMatch, 'Current Position section not found after advance-plan'); - const posBody = posMatch[1]; - const posStatusMatch = posBody.match(/^Status:\s*(.+)/m); - assert.ok(posStatusMatch, 'Status field not found in Current Position section'); + const section = collectSection(after, (h) => h.text.trim() === 'Current Position'); + assert.ok(section, 'Current Position section not found after advance-plan'); + const posBody = section.body; + const posStatus = stateExtractField(posBody, 'Status'); + assert.ok(posStatus, 'Status field not found in Current Position section'); assert.strictEqual( - posStatusMatch[1].trim(), + posStatus, 'On hold — waiting for upstream dependency merge', - `advance-plan overwrote executor-authored Current Position Status: got '${posStatusMatch[1].trim()}'`, + `advance-plan overwrote executor-authored Current Position Status: got '${posStatus}'`, ); - const posActivityMatch = posBody.match(/^Last activity:\s*(.+)/im); - assert.ok(posActivityMatch, 'Last activity field not found in Current Position section'); + const posActivity = stateExtractField(posBody, 'Last activity'); + assert.ok(posActivity, 'Last activity field not found in Current Position section'); assert.ok( - posActivityMatch[1].includes('blocked by infra'), - `advance-plan overwrote executor-authored Current Position Last activity: got '${posActivityMatch[1].trim()}'`, + posActivity.includes('blocked by infra'), + `advance-plan overwrote executor-authored Current Position Last activity: got '${posActivity}'`, ); } finally { cleanup(dir); @@ -9978,7 +9977,6 @@ describe('buildStateFrontmatter cache invalidation (#1967)', () => { const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-3127-state-begin-phase-idempotent (consolidation epic #1969 B3 #1972)", () => { 'use strict'; -// allow-test-rule: reads runtime STATE.md written to temp dir — behavioral output test, not source-grep (see #3127) // Regression tests for bug #3127. // @@ -10003,6 +10001,7 @@ const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); const { cleanup } = require('./helpers.cjs'); +const { stateExtractField } = require('../gsd-core/bin/lib/state-document.cjs'); const ROOT = path.join(__dirname, '..'); @@ -10079,9 +10078,9 @@ describe('bug #3127: state.begin-phase idempotency guard', () => { cmdStateBeginPhase(dir, '5', 'test-phase', 8, false); const after = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8'); // Current Plan must not have been reset to 1 - const planMatch = after.match(/^Current Plan:\s*(\S+)/m); - if (planMatch) { - assert.notStrictEqual(planMatch[1], '1', + const currentPlan = stateExtractField(after, 'Current Plan'); + if (currentPlan !== null) { + assert.notStrictEqual(currentPlan, '1', 'begin-phase reset Current Plan to 1 on a mid-flight phase — idempotency guard not applied'); } } finally { @@ -10098,9 +10097,10 @@ describe('bug #3127: state.begin-phase idempotency guard', () => { cmdStateBeginPhase(dir, '5', 'test-phase', 8, false); const after = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8'); // The rich stopped_at narrative must be preserved + const stoppedAt = stateExtractField(after, 'stopped_at'); assert.ok( - after.includes('Plan 02 SHIPPED') || after.includes('Wave 2 GREEN'), - 'begin-phase overwrote stopped_at narrative on a mid-flight phase', + stoppedAt && (stoppedAt.includes('Plan 02 SHIPPED') || stoppedAt.includes('Wave 2 GREEN')), + `begin-phase overwrote stopped_at narrative on a mid-flight phase; got: ${stoppedAt}`, ); } finally { cleanup(dir); @@ -10116,9 +10116,9 @@ describe('bug #3127: state.begin-phase idempotency guard', () => { cmdStateBeginPhase(dir, '5', 'test-phase', 8, false); const after = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8'); // Normal path: Current Plan should become 1 (or stay 1) - const planMatch = after.match(/^Current Plan:\s*(\S+)/m); - if (planMatch) { - assert.strictEqual(planMatch[1], '1', + const currentPlan = stateExtractField(after, 'Current Plan'); + if (currentPlan !== null) { + assert.strictEqual(currentPlan, '1', 'begin-phase should set Current Plan to 1 on a fresh phase'); } } finally { @@ -10142,9 +10142,10 @@ describe('bug #3127: state.begin-phase idempotency guard', () => { try { cmdStateBeginPhase(dir, '5', 'test-phase', 8, false); const after = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8'); + const lastActivity = stateExtractField(after, 'Last activity'); assert.ok( - after.includes(PINNED_DATE), - `begin-phase must update Last Activity date to the pinned date ${PINNED_DATE} even on resume (safe field)`, + lastActivity && lastActivity.includes(PINNED_DATE), + `begin-phase must update Last Activity date to the pinned date ${PINNED_DATE} even on resume (safe field); got: ${lastActivity}`, ); } finally { // Restore env vars before cleanup to avoid leaking state to other tests. diff --git a/tests/ui-consideration-probe-docs-fixtures.test.cjs b/tests/ui-consideration-probe-docs-fixtures.test.cjs index c93414681..8178e8f41 100644 --- a/tests/ui-consideration-probe-docs-fixtures.test.cjs +++ b/tests/ui-consideration-probe-docs-fixtures.test.cjs @@ -1,4 +1,4 @@ -// allow-test-rule: runtime-contract-is-the-product (see #1867) — the rendered reference doc's taxonomy table IS the runtime contract; this pins its bijection to the code (docs-parity, ADR-456 exception matrix) +// allow-test-rule: source-text-is-the-product (see #1867) — the rendered reference doc's taxonomy table IS the runtime contract; this pins its bijection to the code (docs-parity, ADR-456 exception matrix) // Asserts gsd-core/references/ui-consideration-probe.md keeps its taxonomy id column in // sync with the source-of-truth UI_TAXONOMY (built .cjs), and that the closed compiled // taxonomy stays DISJOINT from the open-prose domain-probes.md bank (the mixed-axis boundary, diff --git a/tests/verify.test.cjs b/tests/verify.test.cjs index daf3bd66b..96e06ac74 100644 --- a/tests/verify.test.cjs +++ b/tests/verify.test.cjs @@ -1742,14 +1742,14 @@ describe('bug-967 verify key-links strict file-path contract', () => { // This test reads the canonical docs file and asserts the example is consistent // with the strict-path contract. // - // allow-test-rule: the plan-md.md reference (see #967) + // allow-test-rule: source-text-is-the-product the plan-md.md reference (see #967) // example IS the documented authoring surface for key_links; asserting it uses // a file path (not an endpoint) directly tests the documented contract. test('docs/reference/plan-md.md key_links example uses a relative file path for to:, not an HTTP endpoint', () => { // Locate plan-md.md relative to this test file's repo root const docPath = path.join(__dirname, '..', 'docs', 'reference', 'plan-md.md'); assert.ok(fs.existsSync(docPath), `plan-md.md not found at ${docPath}`); - const content = fs.readFileSync(docPath, 'utf-8'); // allow-test-rule: the plan-md.md reference example IS the documented authoring surface for key_links; asserting it uses a file path (not an endpoint) directly tests the documented contract. (see #967) + const content = fs.readFileSync(docPath, 'utf-8'); // allow-test-rule: source-text-is-the-product the plan-md.md reference example IS the documented authoring surface for key_links; asserting it uses a file path (not an endpoint) directly tests the documented contract. (see #967) // Find the key_links block in the annotated example (the first YAML frontmatter fence) // The bad old value was: to: "/api/feed" diff --git a/tests/workflow-shell-pinning.test.cjs b/tests/workflow-shell-pinning.test.cjs index 7d1e8a3a1..64f58d729 100644 --- a/tests/workflow-shell-pinning.test.cjs +++ b/tests/workflow-shell-pinning.test.cjs @@ -56,8 +56,10 @@ function listWorkflowFiles() { .map((e) => path.join(WORKFLOWS_DIR, e)); // Filter to files that have at least one Windows hosted-runner reference. - // allow-test-rule: file-scope prefilter, not a test assertion — we need to - // detect whether a workflow file targets Windows runners at all. The pwsh + // allow-test-rule: source-text-is-the-product + // Reads the shipped .github/workflows/*.yml — the deployed text GitHub + // Actions executes — to detect whether a workflow targets Windows runners + // at all. The pwsh // stderr-swallow class is windows-only, so files that never mention // windows-hosted labels are out of scope. Exposing a typed IR from production // code is not appropriate here because the source-of-truth is the YAML diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index 7d70fd635..917bf8774 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -4299,7 +4299,12 @@ describe('bug-3707: reapOrphanWorktrees — adversarial edge cases', () => { const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-3129-validate-commit-git-bypass (consolidation epic #1969 B5 #1974)", () => { 'use strict'; -// allow-test-rule: reads hook shell script to verify delegation pattern — structural contract test, not source-grep (see #3129) +// allow-test-rule: structural-regression-guard (see #3129) +// Reads the gsd-validate-commit.sh hook source to verify it delegates to +// git-cmd.js isGitSubcommand() rather than the old regex — a specific code +// pattern that must (and must not) exist; behavioral tests of tokenize()/ +// isGitSubcommand() cannot observe which detection strategy the hook itself +// calls. // Regression tests for bug #3129. // From a4560c3669977819f3528929709e5db8cc220565 Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 5 Aug 2026 17:54:32 -0400 Subject: [PATCH 03/11] test(#3090): read the body Status, not the frontmatter key that shadows it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit stateExtractField tries bold **Field:**, then a plain ^Field: line, first match wins. STATE.md's frontmatter carries a `status:` key that appears before the body's plain `Status:` line, so extracting "Status" from unstripped content returns the frontmatter value and never the body prose the test was written against. Scoping the lookup to stripFrontmatter() fixes it. The tempting fix was to assert the frontmatter enum instead, on the reasoning that a typed value beats matching prose. That would have been wrong and would have weakened the test: normalizeStateStatus maps both "Phase complete — ready for verification" and "Verifying Phase N" onto the same 'verifying' enum, so the enum cannot tell phase-complete from mid-verification, which is precisely the distinction this case exists to prove. Typed is not automatically stronger when the type conflates the cases under test. Case 4 carried the same collision and was passing only because normalizeStateStatus falls through to the raw text when no known pattern matches, so frontmatter and body happened to agree. Fixed alongside it rather than left for the next person to trip over. All fourteen conversions on this branch were audited against the same failure mode. The collision can only arise where the frontmatter key and the body field name are identical case-insensitively — Status is the only such field, since every other frontmatter key is snake_case against a Title Case body label. Refs #3057 Co-Authored-By: Claude Opus 5 --- tests/state.test.cjs | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 8dd769293..cb7edb15a 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -7085,6 +7085,7 @@ const path = require('node:path'); const { cleanup } = require('./helpers.cjs'); const { stateExtractField } = require('../gsd-core/bin/lib/state-document.cjs'); const { collectSection } = require('../gsd-core/bin/lib/markdown-sectionizer.cjs'); +const { stripFrontmatter } = require('../gsd-core/bin/lib/frontmatter.cjs'); const ROOT = path.join(__dirname, '..'); const TOOLS_PATH = path.join(ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs'); @@ -7336,8 +7337,14 @@ describe('bug #397: executor-authored STATE.md fields must be preserved', () => const r = runGsdState(['advance-plan'], dir); assert.ok(r.success, `advance-plan failed: ${r.error}`); const after = readState(dir); - // The Configuration-level Status must not be clobbered - const status = stateExtractField(after, 'Status'); + // The Configuration-level Status must not be clobbered. Scope extraction + // to the body (frontmatter stripped): the frontmatter `status:` key and + // the body `Status:` field name collide case-insensitively under + // stateExtractField's plain-line pattern (#1255's landmine — see + // state-transition.cts), and frontmatter's `status` is a normalized + // enum (normalizeStateStatus), not the executor-authored prose this + // assertion is proving was preserved. + const status = stateExtractField(stripFrontmatter(after), 'Status'); assert.ok(status, 'Status field not found after advance-plan'); assert.strictEqual( status, @@ -7357,7 +7364,14 @@ describe('bug #397: executor-authored STATE.md fields must be preserved', () => const r = runGsdState(['advance-plan'], dir); assert.ok(r.success, `advance-plan failed: ${r.error}`); const after = readState(dir); - const status = stateExtractField(after, 'Status'); + // Scope extraction to the body (frontmatter stripped): stateExtractField's + // plain-line pattern matches the frontmatter `status:` key before the body + // `Status:` field (case-insensitive collision — #1255). Frontmatter status + // is normalizeStateStatus's coarser enum (e.g. both "Verifying Phase N" + // and "Phase complete — ready for verification" normalize to the SAME + // 'verifying' value), so asserting it here would be a WEAKER check than + // the body prose this test exists to prove was replaced. + const status = stateExtractField(stripFrontmatter(after), 'Status'); assert.ok(status, 'Status field not found after advance-plan'); // 'Ready to execute' is a known default and should be replaced assert.notStrictEqual( From 6128f730039eeca04bd0f4cac7bbff8190b21c7d Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 5 Aug 2026 18:28:04 -0400 Subject: [PATCH 04/11] test(#3090): normalize the whole reason line, not just the category token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The allow-test-rule gate keys on identity, and the identity it records is everything after the colon on the annotation line — not the category token. Ten annotations carried the canonical category plus a trailing justification on the same line, so the recorded identity was a prose blob, and where the prose wrapped it was a sentence fragment: `source-text-is-the-product — the workflow .md content IS`. Seven of those ten are ones this branch already rewrote. That pass renamed the token and left the prose, which is the same error this wave exists to correct, one level down: the label was fixed without checking what the machine reads. Justifications move to the following comment line, which the scanner ignores because it lacks the token. No annotation gains or loses an issue reference, so no exemption changes compliance status; the allowlist goes 161 to 159 as two files' duplicate identities collapse. git-base-branch.test.cjs carried the token twice — once as the real annotation, once echoed in docblock prose that the line scanner parsed as a second exemption with a truncated identity. The echo is reworded to drop the literal token. intel.test.cjs:1360 was cut off mid-clause with an issue ref appended after the break; its sentence is restored and the ref kept on the annotation line so it stays compliant. Every remaining non-canonical identity is an ESLint RuleTester fixture inside a `code:` template literal, which the line scanner cannot tell apart from an annotation. Those two stay grandfathered. Refs #3057 Co-Authored-By: Claude Opus 5 --- .../lint-allow-test-rule-refs.allowlist.json | 20 +++++++++---------- tests/agent-classification-parity.test.cjs | 3 ++- tests/command-contract.test.cjs | 5 +++-- tests/edge-probe-docs-fixtures.test.cjs | 3 ++- tests/edge-probe-planner-contract.test.cjs | 3 ++- tests/edge-probe-spec-phase-contract.test.cjs | 3 ++- tests/git-base-branch.test.cjs | 5 +++-- tests/intel.test.cjs | 13 ++++++------ tests/inventory-headings-countfree.test.cjs | 3 ++- tests/research-agent-profiles.test.cjs | 3 ++- tests/runtime-name-policy.test.cjs | 4 ++-- 11 files changed, 36 insertions(+), 29 deletions(-) diff --git a/scripts/lint-allow-test-rule-refs.allowlist.json b/scripts/lint-allow-test-rule-refs.allowlist.json index e3686eb7b..c3b457443 100644 --- a/scripts/lint-allow-test-rule-refs.allowlist.json +++ b/scripts/lint-allow-test-rule-refs.allowlist.json @@ -1,5 +1,5 @@ [ - "tests/agent-classification-parity.test.cjs :: source-text-is-the-product — docs/AGENTS.md section layout + docs/INVENTORY.md table ARE the classification surface being validated", + "tests/agent-classification-parity.test.cjs :: source-text-is-the-product", "tests/agent-frontmatter.test.cjs :: source-text-is-the-product", "tests/agent-required-reading-consistency.test.cjs :: source-text-is-the-product", "tests/agent-size-budget.test.cjs :: source-text-is-the-product", @@ -28,7 +28,7 @@ "tests/code-review.test.cjs :: source-text-is-the-product", "tests/codebuddy-install.test.cjs :: source-text-is-the-product", "tests/codex-config.test.cjs :: source-text-is-the-product", - "tests/command-contract.test.cjs :: source-text-is-the-product — commands/gsd/*.md files ARE the", + "tests/command-contract.test.cjs :: source-text-is-the-product", "tests/commands.test.cjs :: source-text-is-the-product", "tests/concurrency-safety.test.cjs :: source-text-is-the-product", "tests/config-field-docs.test.cjs :: docs-parity", @@ -44,9 +44,9 @@ "tests/discuss-phase-power.test.cjs :: source-text-is-the-product", "tests/docs-parity-live-registry.test.cjs :: source-text-is-the-product", "tests/drift-detection.test.cjs :: source-text-is-the-product", - "tests/edge-probe-docs-fixtures.test.cjs :: source-text-is-the-product — the rendered reference/SPEC/ADR vocab surfaces are the runtime contract; this pins their bijection to the code (docs-parity)", - "tests/edge-probe-planner-contract.test.cjs :: source-text-is-the-product — plan-phase.md's planner prompt is the deployed runtime contract under assertion", - "tests/edge-probe-spec-phase-contract.test.cjs :: source-text-is-the-product — spec-phase.md Step 5.5 is the deployed workflow runtime contract under assertion", + "tests/edge-probe-docs-fixtures.test.cjs :: source-text-is-the-product", + "tests/edge-probe-planner-contract.test.cjs :: source-text-is-the-product", + "tests/edge-probe-spec-phase-contract.test.cjs :: source-text-is-the-product", "tests/edit-phase.test.cjs :: source-text-is-the-product", "tests/eslint-rules.test.cjs :: must still error", "tests/eslint-rules.test.cjs :: pending migration", @@ -61,7 +61,6 @@ "tests/frontmatter-cli.test.cjs :: source-text-is-the-product", "tests/gates-taxonomy.test.cjs :: source-text-is-the-product", "tests/git-base-branch.test.cjs :: source-text-is-the-product", - "tests/git-base-branch.test.cjs :: source-text-is-the-product — the workflow .md content IS", "tests/gsd-check-update-worker-platform-gate.test.cjs :: structural-regression-guard", "tests/gsd-researcher-app-aware.test.cjs :: source-text-is-the-product", "tests/gsd-researcher-flow-diagram.test.cjs :: source-text-is-the-product", @@ -75,9 +74,8 @@ "tests/install-nested-layout.test.cjs :: source-text-is-the-product", "tests/install-runtime-artifacts.test.cjs :: source-text-is-the-product", "tests/install.test.cjs :: source-text-is-the-product", - "tests/intel.test.cjs :: source-text-is-the-product — agents/gsd-intel-updater.md IS the", - "tests/intel.test.cjs :: source-text-is-the-product — readFileSync assertions target API-SURFACE.md, which is the generated product of intelApiSurface; asserting on its text content is the only way to verify correct generation.", - "tests/inventory-headings-countfree.test.cjs :: source-text-is-the-product — INVENTORY.md heading format is the shipped doc surface being locked", + "tests/intel.test.cjs :: source-text-is-the-product", + "tests/inventory-headings-countfree.test.cjs :: source-text-is-the-product", "tests/ios-scaffold-safety.test.cjs :: source-text-is-the-product", "tests/issue-2639-codex-toml-neutralization.test.cjs :: source-text-is-the-product", "tests/issue-429-comment-text-gate.test.cjs :: source-text-is-the-product", @@ -127,11 +125,11 @@ "tests/release-coverage-scope.test.cjs :: source-text-is-the-product", "tests/release-tarball-smoke-workflow.test.cjs :: source-text-is-the-product", "tests/release-tarball-smoke.install.test.cjs :: integration-test-input", - "tests/research-agent-profiles.test.cjs :: source-text-is-the-product research agent .md content is the governed surface", + "tests/research-agent-profiles.test.cjs :: source-text-is-the-product", "tests/review-default-reviewers-workflow.test.cjs :: source-text-is-the-product", "tests/roadmap.test.cjs :: source-text-is-the-product", "tests/runtime-launcher-parity.test.cjs :: structural-regression-guard", - "tests/runtime-name-policy.test.cjs :: source-text-is-the-product — FALLBACK_ALIASES source text IS the", + "tests/runtime-name-policy.test.cjs :: source-text-is-the-product", "tests/scan-command.test.cjs :: source-text-is-the-product", "tests/secret-scan-lint.security.test.cjs :: source-text-is-the-product", "tests/secure-phase.test.cjs :: source-text-is-the-product", diff --git a/tests/agent-classification-parity.test.cjs b/tests/agent-classification-parity.test.cjs index f66cd2e83..59deba498 100644 --- a/tests/agent-classification-parity.test.cjs +++ b/tests/agent-classification-parity.test.cjs @@ -1,4 +1,5 @@ -// allow-test-rule: source-text-is-the-product — docs/AGENTS.md section layout + docs/INVENTORY.md table ARE the classification surface being validated +// allow-test-rule: source-text-is-the-product +// docs/AGENTS.md section layout + docs/INVENTORY.md table ARE the classification surface being validated 'use strict'; /** diff --git a/tests/command-contract.test.cjs b/tests/command-contract.test.cjs index e9fd20281..cf124132e 100644 --- a/tests/command-contract.test.cjs +++ b/tests/command-contract.test.cjs @@ -1,5 +1,6 @@ -// allow-test-rule: source-text-is-the-product — commands/gsd/*.md files ARE the -// deployed skill surface. Testing their contract tests the runtime behaviour. +// allow-test-rule: source-text-is-the-product +// commands/gsd/*.md files ARE the deployed skill surface. Testing their +// contract tests the runtime behaviour. 'use strict'; diff --git a/tests/edge-probe-docs-fixtures.test.cjs b/tests/edge-probe-docs-fixtures.test.cjs index e64f5d3e2..a155445be 100644 --- a/tests/edge-probe-docs-fixtures.test.cjs +++ b/tests/edge-probe-docs-fixtures.test.cjs @@ -1,4 +1,5 @@ -// allow-test-rule: source-text-is-the-product — the rendered reference/SPEC/ADR vocab surfaces are the runtime contract; this pins their bijection to the code (docs-parity) +// allow-test-rule: source-text-is-the-product +// the rendered reference/SPEC/ADR vocab surfaces are the runtime contract; this pins their bijection to the code (docs-parity) // Asserts the portable reference doc (gsd-core/references/edge-probe.md) keeps its // worked-example JSON blocks in sync with the source-of-truth fixture files under // gsd-core/references/edge-probe-fixtures/. The fixtures are the canonical data; the diff --git a/tests/edge-probe-planner-contract.test.cjs b/tests/edge-probe-planner-contract.test.cjs index dd8168ff8..6e79b672f 100644 --- a/tests/edge-probe-planner-contract.test.cjs +++ b/tests/edge-probe-planner-contract.test.cjs @@ -1,4 +1,5 @@ -// allow-test-rule: source-text-is-the-product — plan-phase.md's planner prompt is the deployed runtime contract under assertion +// allow-test-rule: source-text-is-the-product +// plan-phase.md's planner prompt is the deployed runtime contract under assertion // plan-phase.md is the deployed planning workflow contract; these checks lock // the SPEC path wiring and quality-gate that the edge-probe review (RR-01/02/03) // requires — assertions scope to extracted sub-blocks to avoid false positives. diff --git a/tests/edge-probe-spec-phase-contract.test.cjs b/tests/edge-probe-spec-phase-contract.test.cjs index 05b3616f8..b07f3e779 100644 --- a/tests/edge-probe-spec-phase-contract.test.cjs +++ b/tests/edge-probe-spec-phase-contract.test.cjs @@ -1,4 +1,5 @@ -// allow-test-rule: source-text-is-the-product — spec-phase.md Step 5.5 is the deployed workflow runtime contract under assertion +// allow-test-rule: source-text-is-the-product +// spec-phase.md Step 5.5 is the deployed workflow runtime contract under assertion // spec-phase.md is the deployed spec workflow contract; these checks lock // the Step 5.5 wiring so the edge-probe.cjs runtime invocation cannot // silently rot the way the original plan-phase no-op did (reviewer finding RR-11). diff --git a/tests/git-base-branch.test.cjs b/tests/git-base-branch.test.cjs index c64da3d85..d67ff38bc 100644 --- a/tests/git-base-branch.test.cjs +++ b/tests/git-base-branch.test.cjs @@ -13,8 +13,9 @@ * G. Anti-regression guard: five affected workflows must NOT contain the * duplicated bare `:-main` / `:-master` fallback pattern that was the root cause. * They must call `gsd_run query git.base-branch` instead. - * (allow-test-rule: source-text-is-the-product — the workflow .md content IS - * the runtime surface; the absence of the bad pattern is what ships to agents.) + * (see the source-text-is-the-product exemption declared below this docblock — + * the workflow .md content IS the runtime surface; the absence of the bad + * pattern is what ships to agents.) */ // allow-test-rule: source-text-is-the-product diff --git a/tests/intel.test.cjs b/tests/intel.test.cjs index 43ab68929..32cd2b250 100644 --- a/tests/intel.test.cjs +++ b/tests/intel.test.cjs @@ -4,7 +4,8 @@ * Covers: query, status, diff, validate, snapshot, patch-meta, * extract-exports, enabled/disabled gating, and CLI routing via gsd-tools. */ -// allow-test-rule: source-text-is-the-product — readFileSync assertions target API-SURFACE.md, which is the generated product of intelApiSurface; asserting on its text content is the only way to verify correct generation. +// allow-test-rule: source-text-is-the-product +// readFileSync assertions target API-SURFACE.md, which is the generated product of intelApiSurface; asserting on its text content is the only way to verify correct generation. 'use strict'; @@ -1107,8 +1108,8 @@ describe('intelApiSurface', () => { }); describe('#1000 regression: gsd-intel-updater emits canonical intel filenames', () => { - // allow-test-rule: source-text-is-the-product — agents/gsd-intel-updater.md IS the - // system prompt the intel-updater agent runs under; asserting its filename references + // allow-test-rule: source-text-is-the-product + // agents/gsd-intel-updater.md IS the system prompt the intel-updater agent runs under; asserting its filename references // verifies the deployed agent surface contract matches the INTEL_FILES the CLI reads. const agentPromptPath = path.join(__dirname, '..', 'agents', 'gsd-intel-updater.md'); const agentPrompt = fs.readFileSync(agentPromptPath, 'utf8'); @@ -1357,9 +1358,9 @@ describe('bug #2351: intel updater kilo layout support', () => { { const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-3290-intel-updater-layout-block (consolidation epic #1969 B7 #1976)", () => { -// allow-test-rule: source-text-is-the-product — agents/gsd-intel-updater.md IS (see #3290) -// the deployed agent instruction set. Asserting its text content tests the -// deployed behaviour contract, not internal implementation. +// allow-test-rule: source-text-is-the-product (see #3290) +// agents/gsd-intel-updater.md IS the deployed agent instruction set. Asserting +// its text content tests the deployed behaviour contract, not internal implementation. 'use strict'; diff --git a/tests/inventory-headings-countfree.test.cjs b/tests/inventory-headings-countfree.test.cjs index 75c0f18a4..e42c3a2ae 100644 --- a/tests/inventory-headings-countfree.test.cjs +++ b/tests/inventory-headings-countfree.test.cjs @@ -1,4 +1,5 @@ -// allow-test-rule: source-text-is-the-product — INVENTORY.md heading format is the shipped doc surface being locked +// allow-test-rule: source-text-is-the-product +// INVENTORY.md heading format is the shipped doc surface being locked 'use strict'; /** diff --git a/tests/research-agent-profiles.test.cjs b/tests/research-agent-profiles.test.cjs index 27ad21435..329c6893c 100644 --- a/tests/research-agent-profiles.test.cjs +++ b/tests/research-agent-profiles.test.cjs @@ -1,4 +1,5 @@ -// allow-test-rule: source-text-is-the-product research agent .md content is the governed surface +// allow-test-rule: source-text-is-the-product +// research agent .md content is the governed surface // The 7 researcher agent .md files are the deployed AI agent definitions — their // frontmatter and @-includes ARE what the runtime loads. Asserting on their content // is asserting on the deployed contract, not the test author's source code. diff --git a/tests/runtime-name-policy.test.cjs b/tests/runtime-name-policy.test.cjs index 5c81cd6f1..53b00b818 100644 --- a/tests/runtime-name-policy.test.cjs +++ b/tests/runtime-name-policy.test.cjs @@ -54,8 +54,8 @@ describe('runtime-name-policy windsurf alias parity — manifest vs FALLBACK_ALI test('manifest and FALLBACK_ALIASES windsurf alias sets are identical', () => { // Read FALLBACK_ALIASES from source to detect manual drift before a build. const srcPath = path.join(ROOT, 'src', 'runtime-name-policy.cts'); - // allow-test-rule: source-text-is-the-product — FALLBACK_ALIASES source text IS the - // product contract for runtimes that can't load the manifest at runtime; verifying + // allow-test-rule: source-text-is-the-product + // FALLBACK_ALIASES source text IS the product contract for runtimes that can't load the manifest at runtime; verifying // both surfaces contain the same windsurf aliases catches manual-mirror drift. const src = fs.readFileSync(srcPath, 'utf8'); const match = src.match(/windsurf:\s*\[([^\]]+)\]/); From fba501836bf0fedddb88b1948088a460a68e1118 Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 5 Aug 2026 18:39:51 -0400 Subject: [PATCH 05/11] test(#3090): let the destructive call ask the safety question the function already answers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cleanup() computed `isTmpPath` — the exact predicate for "is this path safe to delete" — and consulted it only inside the catch block, to classify a transient Windows error. The rmSync above it ran unconditionally against whatever path it was handed, with `recursive: true, force: true`. The guard existed and the destructive call never asked it. The chdir at the top of the function makes the failure mode worse rather than better: a wrong target first moves the process out of the tree, then deletes it. So the predicate moves above both, and an out-of-tmpdir path is refused before either can run. The refusal throws and names the path; returning quietly would reproduce the fail-open shape this wave exists to remove. The catch keeps its `&& isTmpPath` term. It is now always true, but it states the condition the swallow depends on rather than inheriting it from a check twenty lines up, and it stays correct if the guard is ever relaxed. All 300+ call sites resolve under os.tmpdir() today, including the two files that override TMPDIR — both create their override root through the real os.tmpdir() first — so nothing legitimate is refused. The regression test targets tests/ itself: a real directory that must never be deleted, so nothing is created and nothing needs tearing down. It asserts the throw names the path, that cwd is unchanged (the chdir hazard), and that a known file inside still exists — proving the directory was not emptied rather than merely still present. Whether tests/ is outside os.tmpdir() is environment-dependent: os.tmpdir() is /tmp on Linux, so a checkout under /tmp would put it inside, and there a correct guard would delete this directory rather than refuse it. The test asserts that precondition before calling cleanup, so that environment fails loudly instead of destructively. Refs #3057 Co-Authored-By: Claude Opus 5 --- tests/helpers-cleanup.test.cjs | 61 ++++++++++++++++++++++++++++++++++ tests/helpers.cjs | 10 +++++- 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/tests/helpers-cleanup.test.cjs b/tests/helpers-cleanup.test.cjs index b876c5e9a..15d3f29a5 100644 --- a/tests/helpers-cleanup.test.cjs +++ b/tests/helpers-cleanup.test.cjs @@ -10,6 +10,8 @@ const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); +const os = require('os'); + const { cleanup, createTempDir } = require('./helpers.cjs'); // ─── Test 1: Real-FS happy path ────────────────────────────────────────────── @@ -98,3 +100,62 @@ test('cleanup does not throw when cwd is inside the target dir, and removes the assert.strictEqual(fs.existsSync(dir), false, 'temp dir should not exist after cleanup'); }); + +// ─── Test 4: out-of-tmpdir refusal ─────────────────────────────────────────── + +test('cleanup throws and does not chdir or delete when target is outside os.tmpdir()', () => { + // __dirname (this repo's tests/ directory) must never be deleted. Whether + // it is actually outside os.tmpdir() is environment-dependent: on Linux + // os.tmpdir() is /tmp, and a CI container that checks the repo out under + // /tmp would put __dirname INSIDE tmpdir, in which case a correctly-working + // cleanup() would not refuse it -- it would delete this directory. The + // precondition assertion below verifies the "outside tmpdir" assumption + // before cleanup() is ever called, so that situation fails loudly and + // safely instead of destructively. No scratch directory is created, so + // there is nothing to tear down. + const outsideDir = __dirname; + const knownFile = path.join(outsideDir, 'helpers-cleanup.test.cjs'); + + // Mirror cleanup()'s own out-of-tmpdir predicate (tests/helpers.cjs) so + // this test cannot run on a target it does not actually control. + const tmpRoot = path.resolve(os.tmpdir()); + const resolvedOutsideDir = path.resolve(outsideDir); + const isInsideTmpdir = + resolvedOutsideDir === tmpRoot || resolvedOutsideDir.startsWith(`${tmpRoot}${path.sep}`); + assert.strictEqual( + isInsideTmpdir, + false, + `this test cannot run safely when the repo lives under os.tmpdir(): ` + + `outsideDir (${resolvedOutsideDir}) is inside os.tmpdir() (${tmpRoot})` + ); + + const cwdBefore = process.cwd(); + + assert.throws( + () => cleanup(outsideDir), + (err) => err instanceof Error && err.message.includes(outsideDir), + 'cleanup must throw an Error whose message names the offending path' + ); + + assert.strictEqual( + process.cwd(), + cwdBefore, + 'cleanup must refuse before chdir, so cwd is unchanged' + ); + assert.strictEqual(fs.existsSync(outsideDir), true, 'target directory must still exist after refusal'); + assert.strictEqual( + fs.existsSync(knownFile), + true, + 'a known file inside the target must still exist, proving the dir was not emptied' + ); +}); + +// ─── Test 5: control — a real os.tmpdir()-rooted path still cleans up ─────── + +test('cleanup still removes a real os.tmpdir()-rooted directory (control)', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cleanup-control-')); + + cleanup(dir); + + assert.strictEqual(fs.existsSync(dir), false, 'os.tmpdir()-rooted directory should be removed'); +}); diff --git a/tests/helpers.cjs b/tests/helpers.cjs index 19e5983e2..ef5d720e2 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -211,6 +211,15 @@ function cleanup(tmpDir) { const target = path.resolve(tmpDir); const cwd = path.resolve(process.cwd()); const tmpRoot = path.resolve(os.tmpdir()); + // isTmpPath was previously computed only inside the catch block below, so it + // classified a transient Windows error but was never consulted by the + // destructive rmSync call itself — a wrong `target` would still chdir out of + // its own tree and get force-deleted. Hoisted above both the chdir and the + // rmSync so an out-of-tmpdir path is refused before either can run. + const isTmpPath = target === tmpRoot || target.startsWith(`${tmpRoot}${path.sep}`); + if (!isTmpPath) { + throw new Error(`cleanup() refused to remove a path outside os.tmpdir(): ${target}`); + } if (cwd === target || cwd.startsWith(`${target}${path.sep}`)) { // Windows cannot remove a directory that is the current working directory. process.chdir(path.dirname(target)); @@ -225,7 +234,6 @@ function cleanup(tmpDir) { } catch (error) { // After retries, Windows can still briefly hold temp dirs open after a timed-out // child exits. Ignore that teardown-only flake for temp roots, but rethrow everything else. - const isTmpPath = target === tmpRoot || target.startsWith(`${tmpRoot}${path.sep}`); const isTransientWinErr = process.platform === 'win32' && isTmpPath && ['EBUSY', 'ENOTEMPTY', 'EPERM'].includes(error && error.code); From 164076b6acb60d0712a533a697541173f79b2ac9 Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 5 Aug 2026 19:16:31 -0400 Subject: [PATCH 06/11] test(#3090): compare both canonical forms of the temp root, not just the unresolved one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard added in the previous commit refused legitimate temp directories on macOS. os.tmpdir() returns a path under /var/folders/..., /var is a symlink to /private/var, and path.resolve() does not resolve symlinks. So a caller that passed the realpath'd form — via fs.realpathSync(), or via process.cwd() after chdir-ing into a temp dir, which returns the resolved path — produced /private/var/folders/... and failed a check written against /var/folders/... Measured on this machine before the fix: mkdtemp path /var/folders/.../probe-XXX allowed fs.realpathSync of the same dir /private/var/folders/.../probe-XXX REFUSED process.cwd() after chdir to it /private/var/folders/.../probe-XXX REFUSED The accepted-roots set is now built from both the resolved and realpath'd forms of os.tmpdir(), deduped — on Linux they are identical, so the set collapses to one entry and nothing changes there. realpathSync is wrapped because it throws if the root is momentarily missing, and a safety check must not become a new crash. The target itself is deliberately NOT realpath'd: cleanup() is called on already-deleted directories, where realpathSync raises ENOENT. The roots are computed per call rather than hoisted to module scope, because two test files override TMPDIR and a hoisted value would go stale for them. Worth naming: the remote matrix runs linux-node22 and linux-node24 only, and it passed 30544/30544 on the broken commit. os.tmpdir() is /tmp on Linux with no symlink indirection, so that matrix could not have caught this at any sample size. The regression test added here branches on whether realpath differs from the original path, so it exercises the real case on macOS and stays meaningful rather than vacuous on Linux. Refs #3057 Co-Authored-By: Claude Opus 5 --- tests/helpers-cleanup.test.cjs | 26 ++++++++++++++++++++++++++ tests/helpers.cjs | 23 +++++++++++++++++++++-- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/tests/helpers-cleanup.test.cjs b/tests/helpers-cleanup.test.cjs index 15d3f29a5..3e63c82e1 100644 --- a/tests/helpers-cleanup.test.cjs +++ b/tests/helpers-cleanup.test.cjs @@ -159,3 +159,29 @@ test('cleanup still removes a real os.tmpdir()-rooted directory (control)', () = assert.strictEqual(fs.existsSync(dir), false, 'os.tmpdir()-rooted directory should be removed'); }); + +// ─── Test 6: realpath'd os.tmpdir() form is not refused (regression) ──────── + +test('cleanup accepts a realpath()d temp dir even when it differs from the raw path (macOS /var -> /private/var)', (t) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cleanup-realpath-')); + const realPath = fs.realpathSync(dir); + + if (realPath !== dir) { + // macOS (and any other symlinked-tmpdir platform): the realpath'd form + // diverges from the raw mkdtempSync() path. This is exactly the shape a + // caller gets from fs.realpathSync() or from process.cwd() after + // chdir-ing into a realpath'd dir — assert the guard does NOT refuse it. + cleanup(realPath); + assert.strictEqual(fs.existsSync(realPath), false, 'realpath()d form should be removed, not refused'); + assert.strictEqual(fs.existsSync(dir), false, 'raw path should also be gone (same directory)'); + } else { + // Linux and any platform with no tmpdir symlink indirection: realpath() + // equals the raw path, so this branch exercises the ordinary path and + // keeps the test meaningful (non-vacuous) on both platforms. + t.after(() => { + if (fs.existsSync(dir)) cleanup(dir); + }); + cleanup(dir); + assert.strictEqual(fs.existsSync(dir), false, 'temp dir should be removed'); + } +}); diff --git a/tests/helpers.cjs b/tests/helpers.cjs index ef5d720e2..6e63de0a8 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -210,13 +210,32 @@ function cleanup(tmpDir) { if (typeof tmpDir !== 'string' || tmpDir.length === 0) return; const target = path.resolve(tmpDir); const cwd = path.resolve(process.cwd()); - const tmpRoot = path.resolve(os.tmpdir()); // isTmpPath was previously computed only inside the catch block below, so it // classified a transient Windows error but was never consulted by the // destructive rmSync call itself — a wrong `target` would still chdir out of // its own tree and get force-deleted. Hoisted above both the chdir and the // rmSync so an out-of-tmpdir path is refused before either can run. - const isTmpPath = target === tmpRoot || target.startsWith(`${tmpRoot}${path.sep}`); + // + // Two acceptable roots, not one: on macOS os.tmpdir() returns a path under + // /var/folders/... but /var is a symlink to /private/var, and path.resolve() + // does not resolve symlinks. A caller that passed the REALPATH'd form of a + // temp dir (e.g. via fs.realpathSync(), or via process.cwd() after chdir-ing + // into a realpath'd dir) would resolve to /private/var/folders/... and get + // wrongly refused by a check against only path.resolve(os.tmpdir()). Build + // the accepted-roots set from both the raw and realpath'd forms of + // os.tmpdir() — realpathSync is wrapped in try/catch because it throws if + // the temp root is momentarily missing, and this guard must never crash + // cleanup() over that. Do NOT realpath `target` itself: cleanup() is + // legitimately called on already-deleted or never-created dirs, and + // realpathSync throws ENOENT on a missing path. + const tmpRoots = [path.resolve(os.tmpdir())]; + try { + const realTmpRoot = fs.realpathSync(os.tmpdir()); + if (!tmpRoots.includes(realTmpRoot)) tmpRoots.push(realTmpRoot); + } catch (_) { /* temp root unreadable — fall back to the resolved form only */ } + const isTmpPath = tmpRoots.some( + (root) => target === root || target.startsWith(`${root}${path.sep}`) + ); if (!isTmpPath) { throw new Error(`cleanup() refused to remove a path outside os.tmpdir(): ${target}`); } From b375d76a100d78bd26c2d8a8c97a537bdc9139d0 Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 5 Aug 2026 19:37:17 -0400 Subject: [PATCH 07/11] test(#3090): accept every canonical spelling of the temp root, not one platform's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows CI refused legitimate temp directories for the same reason macOS did, one commit earlier: cleanup() refused to remove a path outside os.tmpdir(): C:\Users\runneradmin\AppData\Local\Temp\bug-3491-7zisup GitHub's windows runners report os.tmpdir() in the 8.3 SHORT form (C:\Users\RUNNER~1\AppData\Local\Temp) while callers hold the expanded LONG form. fs.realpathSync() does not reliably expand 8.3 names; only fs.realpathSync.native() does. Casing can differ independently (C:\ vs c:\). Two platform-specific breakages from the same check is a sign the check was written against one spelling rather than the concept, so this stops patching symptoms. tmpRootCandidates() collects every variant derivable from os.tmpdir() — resolved, realpath'd, and native-realpath'd — each probe isolated in its own try/catch so an unavailable variant contributes nothing instead of crashing teardown, then deduped. A target is accepted under any of them, and the comparison folds case on win32 only, where casing genuinely varies. The error message still prints the original-case path. On this machine three variants collapse to two: /var/folders/.../T and /private/var/folders/.../T. Both spellings of a real temp dir are accepted and removed. The Linux matrix passed every one of these broken states — 30544 and then 30545, both lanes green — because /tmp has neither symlink indirection nor short names. The platform CI shards are the only thing that has caught any of it, which is worth stating plainly given what this branch is about. Refs #3057 Co-Authored-By: Claude Opus 5 --- tests/helpers-cleanup.test.cjs | 15 ++++++++ tests/helpers.cjs | 69 ++++++++++++++++++++++++---------- 2 files changed, 64 insertions(+), 20 deletions(-) diff --git a/tests/helpers-cleanup.test.cjs b/tests/helpers-cleanup.test.cjs index 3e63c82e1..267a425a6 100644 --- a/tests/helpers-cleanup.test.cjs +++ b/tests/helpers-cleanup.test.cjs @@ -160,6 +160,21 @@ test('cleanup still removes a real os.tmpdir()-rooted directory (control)', () = assert.strictEqual(fs.existsSync(dir), false, 'os.tmpdir()-rooted directory should be removed'); }); +// ─── Test 6b: accepted-roots set always accepts a freshly-minted tmpdir ───── + +test('cleanup accepts a freshly-created os.tmpdir()-rooted dir on any platform (accepted-roots invariant)', (t) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cleanup-roots-')); + t.after(() => { + if (fs.existsSync(dir)) cleanup(dir); + }); + + assert.doesNotThrow( + () => cleanup(dir), + 'cleanup must accept a directory created directly under os.tmpdir(), regardless of platform-specific canonical spelling (8.3 short/long names, /var symlink, drive-letter case)' + ); + assert.strictEqual(fs.existsSync(dir), false, 'temp dir should be removed, not refused'); +}); + // ─── Test 6: realpath'd os.tmpdir() form is not refused (regression) ──────── test('cleanup accepts a realpath()d temp dir even when it differs from the raw path (macOS /var -> /private/var)', (t) => { diff --git a/tests/helpers.cjs b/tests/helpers.cjs index 6e63de0a8..cbe70d7e6 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -206,6 +206,40 @@ function createTempGitProject(prefix = 'gsd-test-') { return createFixture({ prefix, planning: true, git: true, projectDoc: true }); } +// The OS temp root has several canonical spellings, and a path a caller +// legitimately passes to cleanup() may arrive in any of them: macOS resolves +// os.tmpdir() under /var/folders/... but /var is a symlink to /private/var; +// Windows CI runners report os.tmpdir() in the 8.3 SHORT form +// (C:\Users\RUNNER~1\AppData\Local\Temp) while the caller's path is the +// expanded LONG form, and fs.realpathSync() does not reliably expand 8.3 +// short names there — only fs.realpathSync.native() does; drive-letter and +// path casing can also differ (C:\ vs c:\). Collect every variant we can +// derive from os.tmpdir() and accept a target under any of them. Each probe +// is wrapped in its own try/catch — none of them may throw and crash +// cleanup(), they just contribute nothing if unavailable. +function tmpRootCandidates() { + const roots = []; + try { + roots.push(path.resolve(os.tmpdir())); + } catch (_) { /* os.tmpdir() unavailable — skip this variant */ } + try { + roots.push(fs.realpathSync(os.tmpdir())); + } catch (_) { /* temp root unreadable — skip this variant */ } + try { + roots.push(fs.realpathSync.native(os.tmpdir())); + } catch (_) { /* native realpath unavailable/unreadable — skip this variant */ } + const isWindows = process.platform === 'win32'; + const seen = new Set(); + const deduped = []; + for (const root of roots) { + const key = isWindows ? root.toLowerCase() : root; + if (seen.has(key)) continue; + seen.add(key); + deduped.push(root); + } + return deduped; +} + function cleanup(tmpDir) { if (typeof tmpDir !== 'string' || tmpDir.length === 0) return; const target = path.resolve(tmpDir); @@ -216,26 +250,21 @@ function cleanup(tmpDir) { // its own tree and get force-deleted. Hoisted above both the chdir and the // rmSync so an out-of-tmpdir path is refused before either can run. // - // Two acceptable roots, not one: on macOS os.tmpdir() returns a path under - // /var/folders/... but /var is a symlink to /private/var, and path.resolve() - // does not resolve symlinks. A caller that passed the REALPATH'd form of a - // temp dir (e.g. via fs.realpathSync(), or via process.cwd() after chdir-ing - // into a realpath'd dir) would resolve to /private/var/folders/... and get - // wrongly refused by a check against only path.resolve(os.tmpdir()). Build - // the accepted-roots set from both the raw and realpath'd forms of - // os.tmpdir() — realpathSync is wrapped in try/catch because it throws if - // the temp root is momentarily missing, and this guard must never crash - // cleanup() over that. Do NOT realpath `target` itself: cleanup() is - // legitimately called on already-deleted or never-created dirs, and - // realpathSync throws ENOENT on a missing path. - const tmpRoots = [path.resolve(os.tmpdir())]; - try { - const realTmpRoot = fs.realpathSync(os.tmpdir()); - if (!tmpRoots.includes(realTmpRoot)) tmpRoots.push(realTmpRoot); - } catch (_) { /* temp root unreadable — fall back to the resolved form only */ } - const isTmpPath = tmpRoots.some( - (root) => target === root || target.startsWith(`${root}${path.sep}`) - ); + // Do NOT realpath `target` itself: cleanup() is legitimately called on + // already-deleted or never-created dirs, and realpathSync throws ENOENT on + // a missing path. Comparison is case-insensitive on Windows (drive-letter + // and path casing vary there) and case-sensitive everywhere else; the + // error message below always prints the original-case target. + const isWindows = process.platform === 'win32'; + const tmpRoots = tmpRootCandidates(); + const targetForCompare = isWindows ? target.toLowerCase() : target; + const isTmpPath = tmpRoots.some((root) => { + const rootForCompare = isWindows ? root.toLowerCase() : root; + return ( + targetForCompare === rootForCompare || + targetForCompare.startsWith(`${rootForCompare}${path.sep}`) + ); + }); if (!isTmpPath) { throw new Error(`cleanup() refused to remove a path outside os.tmpdir(): ${target}`); } From cc0a4685f14b3b7d9c7076383509b031e148935d Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 5 Aug 2026 19:51:11 -0400 Subject: [PATCH 08/11] test(#3090): close the symlink escape, and stop the test from mirroring the guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An isolated review found the safety precondition was not safe. Test 4 uses the repo's own tests/ directory as a path that must never be deleted, and asserted beforehand that it sits outside the temp root — but it computed that root as path.resolve(os.tmpdir()) alone, while cleanup() accepts a target under any of several root spellings. For a checkout under the realpath'd temp root the precondition reads "outside, safe to proceed" while the guard reads "inside, delete it", and rmSync runs on tests/ before the assertion fails. The commit that added it claimed it would fail loudly instead of destructively; it would have done the opposite. The fix is not a second root in the test. tmpRootCandidates() is exported and the precondition calls it, so there is one source of truth and nothing left to drift. A mirror was the defect, not its contents. The same review bounded what the refusal actually guarantees: the check is a string prefix test, so a symlink living under tmpdir but pointing outside it passes while rmSync follows the link and deletes the real directory. When the target exists its real path is now checked too, against the same predicate — factored into one function so the two comparisons cannot diverge the way the test's copy did. A realpath failure refuses rather than proceeds; a safety check that cannot verify must not report safe, which is the whole subject of this branch. Missing targets are skipped, since rmSync with force no-ops on them and realpathSync would only throw ENOENT. `const isTmpPath = true` is gone. It survived the previous commit as a way to keep the catch's `&& isTmpPath` reading as a real condition, but a constant dressed as a test states nothing; the guard clauses above throw, so the catch comment now says the invariant in words instead. The new coverage does not depend on the platform the bug lives on. The root list is asserted directly — non-empty, absolute, deduped, and containing a freshly created temp dir — and the symlink refusal runs everywhere, skipping only where symlink creation is unavailable. The previous realpath test was coverage-identical to the control on Linux, so the only lanes the matrix runs could not have verified the fix it was written for. Refs #3057 Co-Authored-By: Claude Opus 5 --- tests/helpers-cleanup.test.cjs | 123 ++++++++++++++++++++++++++++++--- tests/helpers.cjs | 74 +++++++++++++++----- 2 files changed, 171 insertions(+), 26 deletions(-) diff --git a/tests/helpers-cleanup.test.cjs b/tests/helpers-cleanup.test.cjs index 267a425a6..a337ddee1 100644 --- a/tests/helpers-cleanup.test.cjs +++ b/tests/helpers-cleanup.test.cjs @@ -12,7 +12,7 @@ const path = require('path'); const os = require('os'); -const { cleanup, createTempDir } = require('./helpers.cjs'); +const { cleanup, createTempDir, tmpRootCandidates } = require('./helpers.cjs'); // ─── Test 1: Real-FS happy path ────────────────────────────────────────────── @@ -116,17 +116,31 @@ test('cleanup throws and does not chdir or delete when target is outside os.tmpd const outsideDir = __dirname; const knownFile = path.join(outsideDir, 'helpers-cleanup.test.cjs'); - // Mirror cleanup()'s own out-of-tmpdir predicate (tests/helpers.cjs) so - // this test cannot run on a target it does not actually control. - const tmpRoot = path.resolve(os.tmpdir()); + // Use cleanup()'s own tmpRootCandidates() as the single source of truth + // for "is this path inside tmpdir" instead of a hand-mirrored predicate. + // The old copy here checked only path.resolve(os.tmpdir()), one of + // SEVERAL root spellings cleanup() actually accepts (it also accepts the + // realpath'd and native-realpath'd forms) — a repo checked out under a + // root this precondition didn't know about would compute "outside, safe" + // while cleanup() computed "inside, delete", turning this refusal test + // into the very destructive scenario it exists to prevent. Importing the + // real function removes that drift class entirely. const resolvedOutsideDir = path.resolve(outsideDir); - const isInsideTmpdir = - resolvedOutsideDir === tmpRoot || resolvedOutsideDir.startsWith(`${tmpRoot}${path.sep}`); + const roots = tmpRootCandidates(); + const isWindows = process.platform === 'win32'; + const outsideDirForCompare = isWindows ? resolvedOutsideDir.toLowerCase() : resolvedOutsideDir; + const isInsideTmpdir = roots.some((root) => { + const rootForCompare = isWindows ? root.toLowerCase() : root; + return ( + outsideDirForCompare === rootForCompare || + outsideDirForCompare.startsWith(`${rootForCompare}${path.sep}`) + ); + }); assert.strictEqual( isInsideTmpdir, false, - `this test cannot run safely when the repo lives under os.tmpdir(): ` + - `outsideDir (${resolvedOutsideDir}) is inside os.tmpdir() (${tmpRoot})` + `this test cannot run safely when the repo lives under any tmpdir root ` + + `cleanup() accepts: outsideDir (${resolvedOutsideDir}) is inside one of ${JSON.stringify(roots)}` ); const cwdBefore = process.cwd(); @@ -200,3 +214,96 @@ test('cleanup accepts a realpath()d temp dir even when it differs from the raw p assert.strictEqual(fs.existsSync(dir), false, 'temp dir should be removed'); } }); + +// ─── Test 7: tmpRootCandidates() shape invariants (platform-independent) ──── +// +// Test 6 above is coverage-identical to Test 5's control on any platform +// where fs.realpathSync(tmpdir) does not diverge from the raw path (Linux, +// notably — the platform the remote matrix actually runs on), so it cannot +// verify the macOS-specific fix there. These assertions hold the exported +// tmpRootCandidates() to a contract that is meaningful on every platform. + +test('tmpRootCandidates() returns a well-formed, deduplicated list of absolute paths', () => { + const roots = tmpRootCandidates(); + + assert.ok(Array.isArray(roots) && roots.length > 0, 'must return a non-empty array'); + + for (const root of roots) { + assert.strictEqual(path.isAbsolute(root), true, `root must be absolute: ${root}`); + } + + const isWindows = process.platform === 'win32'; + const keys = roots.map((r) => (isWindows ? r.toLowerCase() : r)); + const uniqueKeys = new Set(keys); + assert.strictEqual(uniqueKeys.size, keys.length, `roots must be deduplicated: ${JSON.stringify(roots)}`); + + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cleanup-roots-contract-')); + try { + const resolvedDir = path.resolve(dir); + const dirForCompare = isWindows ? resolvedDir.toLowerCase() : resolvedDir; + const isUnderSomeRoot = roots.some((root) => { + const rootForCompare = isWindows ? root.toLowerCase() : root; + return ( + dirForCompare === rootForCompare || + dirForCompare.startsWith(`${rootForCompare}${path.sep}`) + ); + }); + assert.strictEqual( + isUnderSomeRoot, + true, + `a freshly mkdtempSync'd dir under os.tmpdir() must be under at least one returned root: ` + + `${resolvedDir} vs ${JSON.stringify(roots)}` + ); + } finally { + cleanup(dir); + } +}); + +// ─── Test 8: symlink-escape refusal, all platforms ─────────────────────────── + +test('cleanup refuses a symlink under tmpdir that resolves outside tmpdir, and the victim survives', (t) => { + const symlinkParent = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cleanup-symlink-')); + t.after(() => { + if (fs.existsSync(symlinkParent)) cleanup(symlinkParent); + }); + + // The victim must live OUTSIDE tmpdir (that's the whole point of the + // scenario) — put it under this worktree's gitignored .gsd/ directory + // rather than os.homedir() so the test has no dependency on the runner's + // home layout. cleanup() will (correctly, by design) refuse a path there, + // so teardown cannot route through cleanup() either; the raw rmSync below + // is narrowly scoped and justified inline. + const victimParent = path.join(__dirname, '..', '.gsd'); + fs.mkdirSync(victimParent, { recursive: true }); + const victim = fs.mkdtempSync(path.join(victimParent, 'cleanup-symlink-victim-')); + t.after(() => { + if (fs.existsSync(victim)) { + // eslint-disable-next-line local/no-raw-rmsync-in-tests -- victim is deliberately outside os.tmpdir() (that's the scenario under test), so cleanup() refuses it by design and cannot be used for its own teardown. + fs.rmSync(victim, { recursive: true, force: true }); + } + }); + fs.writeFileSync(path.join(victim, 'marker.txt'), 'victim'); + + const symlinkPath = path.join(symlinkParent, 'escape-link'); + let symlinkCreated = true; + try { + fs.symlinkSync(victim, symlinkPath, 'dir'); + } catch (error) { + symlinkCreated = false; + t.skip(`symlink creation not permitted in this environment (${error.code || error.message}) — likely Windows without developer mode`); + } + + if (symlinkCreated) { + assert.throws( + () => cleanup(symlinkPath), + (err) => err instanceof Error && err.message.includes(symlinkPath), + 'cleanup must throw and name the symlink path when it resolves outside os.tmpdir()' + ); + assert.strictEqual(fs.existsSync(victim), true, 'victim directory must survive the refusal'); + assert.strictEqual( + fs.existsSync(path.join(victim, 'marker.txt')), + true, + 'victim contents must survive the refusal' + ); + } +}); diff --git a/tests/helpers.cjs b/tests/helpers.cjs index cbe70d7e6..4d16f8cd0 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -244,30 +244,67 @@ function cleanup(tmpDir) { if (typeof tmpDir !== 'string' || tmpDir.length === 0) return; const target = path.resolve(tmpDir); const cwd = path.resolve(process.cwd()); - // isTmpPath was previously computed only inside the catch block below, so it - // classified a transient Windows error but was never consulted by the + // The temp-root check below was previously done only inside the catch block, + // so it classified a transient Windows error but was never consulted by the // destructive rmSync call itself — a wrong `target` would still chdir out of // its own tree and get force-deleted. Hoisted above both the chdir and the // rmSync so an out-of-tmpdir path is refused before either can run. // - // Do NOT realpath `target` itself: cleanup() is legitimately called on - // already-deleted or never-created dirs, and realpathSync throws ENOENT on - // a missing path. Comparison is case-insensitive on Windows (drive-letter - // and path casing vary there) and case-sensitive everywhere else; the - // error message below always prints the original-case target. + // `target` itself is only realpath'd below when it exists (fs.existsSync + // guard on the symlink-escape check) — realpathSync throws ENOENT on a + // missing path, and cleanup() is legitimately called on already-deleted or + // never-created dirs. Comparison is case-insensitive on Windows + // (drive-letter and path casing vary there) and case-sensitive everywhere + // else; the error message below always prints the original-case target. const isWindows = process.platform === 'win32'; const tmpRoots = tmpRootCandidates(); - const targetForCompare = isWindows ? target.toLowerCase() : target; - const isTmpPath = tmpRoots.some((root) => { - const rootForCompare = isWindows ? root.toLowerCase() : root; - return ( - targetForCompare === rootForCompare || - targetForCompare.startsWith(`${rootForCompare}${path.sep}`) - ); - }); - if (!isTmpPath) { + // Shared by the literal-target check and the symlink-realpath check below + // so the two cannot drift apart — a second, independently written copy of + // this comparison is how the matching precondition in the tests came to + // disagree with the guard it was meant to mirror. + function isUnderRoots(p) { + const pForCompare = isWindows ? p.toLowerCase() : p; + return tmpRoots.some((root) => { + const rootForCompare = isWindows ? root.toLowerCase() : root; + return ( + pForCompare === rootForCompare || + pForCompare.startsWith(`${rootForCompare}${path.sep}`) + ); + }); + } + if (!isUnderRoots(target)) { throw new Error(`cleanup() refused to remove a path outside os.tmpdir(): ${target}`); } + // Symlink-escape guard: the check above is a string prefix test on `target` + // alone, so a symlink physically located under tmpdir but pointing OUTSIDE + // it would pass that check while rmSync follows the link and deletes the + // real, out-of-tree directory. When the target exists, also resolve its + // real path and require that to be under a root too. + // + // Only when it exists: cleanup() is legitimately called on already-deleted + // or never-created dirs, and fs.realpathSync throws ENOENT on those — a + // missing target is harmless anyway since rmSync({force: true}) no-ops on it. + // + // Any realpath error (including a non-ENOENT failure) fails CLOSED: refuse + // rather than silently proceed. This is a safety check, not a best-effort + // probe — an unreadable/looping symlink must not be treated as safe. + if (fs.existsSync(target)) { + let real; + try { + real = fs.realpathSync(target); + } catch (error) { + throw new Error( + `cleanup() refused to remove a path it could not verify via realpath ` + + `(${(error && error.code) || (error && error.message) || 'unknown error'}): ${target}` + ); + } + if (!isUnderRoots(real)) { + throw new Error( + `cleanup() refused to remove a path that resolves via symlink to a ` + + `location outside os.tmpdir(): ${target} -> ${real}` + ); + } + } if (cwd === target || cwd.startsWith(`${target}${path.sep}`)) { // Windows cannot remove a directory that is the current working directory. process.chdir(path.dirname(target)); @@ -282,8 +319,9 @@ function cleanup(tmpDir) { } catch (error) { // After retries, Windows can still briefly hold temp dirs open after a timed-out // child exits. Ignore that teardown-only flake for temp roots, but rethrow everything else. + // By this point target is guaranteed under a temp root: the guard clauses above throw + // for any other path, so this swallow doesn't need to re-test that. const isTransientWinErr = process.platform === 'win32' - && isTmpPath && ['EBUSY', 'ENOTEMPTY', 'EPERM'].includes(error && error.code); if (!isTransientWinErr) throw error; } @@ -693,4 +731,4 @@ function clearSessionEnv() { for (const k of SESSION_ENV_KEYS) delete process.env[k]; } -module.exports = { runGsdTools, createTempDir, createTempProject, createTempGitProject, cleanup, readFileNormalized, readWorkflowCombined, parseFrontmatter, isUsageOutput, captureConsole, toPosixPath, absPlanningPath, runNpm, isolatedNpmEnv, withIsolatedProcessState, delay, waitFor, resetRuntimeWarningCaches, SESSION_ENV_KEYS, saveSessionEnv, restoreSessionEnv, clearSessionEnv, TOOLS_PATH }; +module.exports = { runGsdTools, createTempDir, createTempProject, createTempGitProject, cleanup, tmpRootCandidates, readFileNormalized, readWorkflowCombined, parseFrontmatter, isUsageOutput, captureConsole, toPosixPath, absPlanningPath, runNpm, isolatedNpmEnv, withIsolatedProcessState, delay, waitFor, resetRuntimeWarningCaches, SESSION_ENV_KEYS, saveSessionEnv, restoreSessionEnv, clearSessionEnv, TOOLS_PATH }; From 2e81516f1f9bfbb0d6850f56f9a99edc4cd83255 Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 5 Aug 2026 20:05:45 -0400 Subject: [PATCH 09/11] test(#3090): accept the temp roots the suite actually uses, and say which ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fourth failure of the same guard, found by review before it reached CI: the config-schema property suite builds fixtures through a getWritableTmp() helper that returns the first writable of /private/tmp, /tmp, os.tmpdir(). On macOS that is /private/tmp while os.tmpdir() is /var/folders/.../T, so five cleanup() calls in that file were refused outright. The three earlier breakages were all spellings of one root. This one is not: the suite legitimately uses more than one temp root, so the premise was wrong rather than the encoding. tmpRootCandidates() now probes the conventional system temp dirs alongside os.tmpdir(), each included only if it exists on the host, so the accepted set stays a bounded explicit list instead of growing a patch per platform. Two corrections that follow from the same review: A root that is itself a filesystem root already ends in a separator, and appending another built `//`, which only the literal `/` satisfies — TMPDIR=/ would have refused every descendant. The separator is only appended when it is not already there. The refusal messages named os.tmpdir(), which stopped being the boundary. They now name the roots actually compared against. Every failure of this guard so far was diagnosed from that message in a CI log, so it should show what was checked rather than a stale approximation of it. The symlink-escape check keeps its refusal but drops its claim. fs.rmSync does not follow a top-level symlink — it unlinks the link and leaves the target alone — so that check was never closing a live escape, and the test asserting the victim survived would have passed with the guard removed. Both now say what is true: a defense-in-depth boundary against a future change to the deletion mechanism, with only the refusal itself load-bearing. The QA path helpers were evaluated for reuse rather than keeping a third copy of path containment. They resolve against one project directory and have no multi-root or Windows short-name handling, so they are not a drop-in; noted rather than forced. Refs #3057 Co-Authored-By: Claude Opus 5 --- tests/helpers-cleanup.test.cjs | 17 +++++++-- tests/helpers.cjs | 67 +++++++++++++++++++++++++++------- 2 files changed, 67 insertions(+), 17 deletions(-) diff --git a/tests/helpers-cleanup.test.cjs b/tests/helpers-cleanup.test.cjs index a337ddee1..0c078aa03 100644 --- a/tests/helpers-cleanup.test.cjs +++ b/tests/helpers-cleanup.test.cjs @@ -261,7 +261,7 @@ test('tmpRootCandidates() returns a well-formed, deduplicated list of absolute p // ─── Test 8: symlink-escape refusal, all platforms ─────────────────────────── -test('cleanup refuses a symlink under tmpdir that resolves outside tmpdir, and the victim survives', (t) => { +test('cleanup refuses a symlink under tmpdir that resolves outside tmpdir', (t) => { const symlinkParent = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cleanup-symlink-')); t.after(() => { if (fs.existsSync(symlinkParent)) cleanup(symlinkParent); @@ -294,16 +294,27 @@ test('cleanup refuses a symlink under tmpdir that resolves outside tmpdir, and t } if (symlinkCreated) { + // The assert.throws below is the load-bearing assertion for this test: + // it proves the guard refuses a symlink whose target resolves outside + // every accepted tmp root. assert.throws( () => cleanup(symlinkPath), (err) => err instanceof Error && err.message.includes(symlinkPath), 'cleanup must throw and name the symlink path when it resolves outside os.tmpdir()' ); - assert.strictEqual(fs.existsSync(victim), true, 'victim directory must survive the refusal'); + // These two assertions document the end state only — they are NOT proof + // the guard prevented a deletion that would otherwise have happened. A + // probe on node v26.5.1 showed fs.rmSync({recursive:true, force:true}) + // does not follow a top-level symlink target regardless: it unlinks the + // symlink itself and leaves whatever it points at (the victim, here) + // untouched either way. So the victim would survive with or without this + // guard; the guard's own correctness is established solely by the + // assert.throws above. + assert.strictEqual(fs.existsSync(victim), true, 'victim directory still exists (rmSync would not have followed the symlink either way)'); assert.strictEqual( fs.existsSync(path.join(victim, 'marker.txt')), true, - 'victim contents must survive the refusal' + 'victim contents still exist (rmSync would not have followed the symlink either way)' ); } }); diff --git a/tests/helpers.cjs b/tests/helpers.cjs index 4d16f8cd0..7cd90d541 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -213,10 +213,11 @@ function createTempGitProject(prefix = 'gsd-test-') { // (C:\Users\RUNNER~1\AppData\Local\Temp) while the caller's path is the // expanded LONG form, and fs.realpathSync() does not reliably expand 8.3 // short names there — only fs.realpathSync.native() does; drive-letter and -// path casing can also differ (C:\ vs c:\). Collect every variant we can -// derive from os.tmpdir() and accept a target under any of them. Each probe -// is wrapped in its own try/catch — none of them may throw and crash -// cleanup(), they just contribute nothing if unavailable. +// path casing can also differ (C:\ vs c:\). This function collects the full +// set of accepted temp roots — os.tmpdir()'s several spellings are one part +// of that set, not the whole of it (see below). Each probe is wrapped in its +// own try/catch — none of them may throw and crash cleanup(), they just +// contribute nothing if unavailable. function tmpRootCandidates() { const roots = []; try { @@ -229,6 +230,28 @@ function tmpRootCandidates() { roots.push(fs.realpathSync.native(os.tmpdir())); } catch (_) { /* native realpath unavailable/unreadable — skip this variant */ } const isWindows = process.platform === 'win32'; + // os.tmpdir() alone is too narrow: it honors $TMPDIR, but some tests + // (e.g. tests/config-schema.property.test.cjs's getWritableTmp()) create + // fixtures directly under the conventional system temp roots instead of + // through $TMPDIR — on macOS that is /private/tmp, which can differ from + // os.tmpdir()'s /var/folders/.../T. Probe the well-known non-Windows temp + // roots too, each independently and only if it actually exists on this + // host, so the accepted set stays a bounded, explicit list rather than an + // open-ended patch list. macOS additionally exposes /tmp as a symlink to + // /private/tmp, so both the unprefixed and /private-prefixed spellings — + // and each one's realpath — are collected. + if (!isWindows) { + for (const candidate of ['/tmp', '/private/tmp']) { + try { + if (fs.existsSync(candidate)) { + roots.push(path.resolve(candidate)); + try { + roots.push(fs.realpathSync(candidate)); + } catch (_) { /* exists but unreadable via realpath — skip this variant */ } + } + } catch (_) { /* existsSync itself should not throw, but fail closed if it does */ } + } + } const seen = new Set(); const deduped = []; for (const root of roots) { @@ -248,7 +271,7 @@ function cleanup(tmpDir) { // so it classified a transient Windows error but was never consulted by the // destructive rmSync call itself — a wrong `target` would still chdir out of // its own tree and get force-deleted. Hoisted above both the chdir and the - // rmSync so an out-of-tmpdir path is refused before either can run. + // rmSync so an out-of-temp-root path is refused before either can run. // // `target` itself is only realpath'd below when it exists (fs.existsSync // guard on the symlink-escape check) — realpathSync throws ENOENT on a @@ -266,20 +289,36 @@ function cleanup(tmpDir) { const pForCompare = isWindows ? p.toLowerCase() : p; return tmpRoots.some((root) => { const rootForCompare = isWindows ? root.toLowerCase() : root; - return ( - pForCompare === rootForCompare || - pForCompare.startsWith(`${rootForCompare}${path.sep}`) - ); + if (pForCompare === rootForCompare) return true; + // A root that is itself a filesystem root (`/`, or `C:\` reachable via + // TMPDIR=/) already ends with path.sep — appending a second one would + // build `//`, which only the literal string `/` satisfies, refusing + // every real descendant. Only append the separator when it is not + // already there. + const prefix = rootForCompare.endsWith(path.sep) + ? rootForCompare + : `${rootForCompare}${path.sep}`; + return pForCompare.startsWith(prefix); }); } if (!isUnderRoots(target)) { - throw new Error(`cleanup() refused to remove a path outside os.tmpdir(): ${target}`); + throw new Error( + `cleanup() refused to remove a path outside the known temp roots ` + + `(${tmpRoots.join(', ')}): ${target}` + ); } // Symlink-escape guard: the check above is a string prefix test on `target` // alone, so a symlink physically located under tmpdir but pointing OUTSIDE - // it would pass that check while rmSync follows the link and deletes the - // real, out-of-tree directory. When the target exists, also resolve its - // real path and require that to be under a root too. + // it would pass that check on `target`'s own path. NOTE: as of this + // writing, `fs.rmSync` itself does not follow a top-level symlink target + // (it unlinks the symlink and leaves whatever it points at untouched), so + // this guard is not closing a live escape against the current rmSync + // behavior — it is defense-in-depth against a future change to the + // deletion mechanism (a different rm implementation, a recursive walk that + // does follow links, etc.) that would make `target` being a symlink + // dangerous. When the target exists, resolve its real path and require + // that to be under a root too, so the guard holds regardless of how the + // eventual delete is implemented. // // Only when it exists: cleanup() is legitimately called on already-deleted // or never-created dirs, and fs.realpathSync throws ENOENT on those — a @@ -301,7 +340,7 @@ function cleanup(tmpDir) { if (!isUnderRoots(real)) { throw new Error( `cleanup() refused to remove a path that resolves via symlink to a ` + - `location outside os.tmpdir(): ${target} -> ${real}` + `location outside the known temp roots (${tmpRoots.join(', ')}): ${target} -> ${real}` ); } } From 7ef9945adbf82ad3efbcde513c3164380ee0b3c4 Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 5 Aug 2026 21:39:53 -0400 Subject: [PATCH 10/11] test(#3090): stop paying for the guard on every teardown The Windows node-24 scoped-test job hit its 15-minute ceiling twice on this branch and GitHub reported both as cancelled, which is how a job timeout surfaces. The same job finishes in about two minutes twenty on next, across four consecutive runs. cleanup() is the only hot-path change here. It was doing up to seven filesystem calls per invocation: three probes of the temp root, two more for each conventional temp dir, then an existence check and a realpath of the target. On Windows fs.realpathSync.native opens a file handle and Defender charges for each one, and this runs in the teardown of effectively every test. The root candidates are now memoized on the live os.tmpdir() value. The key matters: two files in the suite override TMPDIR mid-run and restore it, so a plain module-level hoist would go stale for them, while re-reading os.tmpdir() costs an env lookup. Measured over 20,000 calls: 546ms unmemoized, 10ms memoized. The symlink-escape check is removed rather than optimized, because it was guarding something that cannot happen. Verified directly on node v26.5.1: fs.rmSync(link, {recursive: true, force: true}) link exists false victim exists true file exists true rmSync unlinks a top-level symlink and leaves its target alone, and a symlink nested inside a tree being recursively removed is also unlinked rather than followed. The check cost two filesystem calls per teardown on the slowest platform in the matrix and bought nothing. Its test asserted the victim survived, which was true with or without the guard. What closed the original defect is untouched: a target outside the known temp roots is still refused, before the chdir and before the rmSync, with the roots named in the message. Refs #3057 Co-Authored-By: Claude Opus 5 --- tests/helpers-cleanup.test.cjs | 59 -------------------------- tests/helpers.cjs | 75 ++++++++++++---------------------- 2 files changed, 27 insertions(+), 107 deletions(-) diff --git a/tests/helpers-cleanup.test.cjs b/tests/helpers-cleanup.test.cjs index 0c078aa03..47e28ba17 100644 --- a/tests/helpers-cleanup.test.cjs +++ b/tests/helpers-cleanup.test.cjs @@ -259,62 +259,3 @@ test('tmpRootCandidates() returns a well-formed, deduplicated list of absolute p } }); -// ─── Test 8: symlink-escape refusal, all platforms ─────────────────────────── - -test('cleanup refuses a symlink under tmpdir that resolves outside tmpdir', (t) => { - const symlinkParent = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cleanup-symlink-')); - t.after(() => { - if (fs.existsSync(symlinkParent)) cleanup(symlinkParent); - }); - - // The victim must live OUTSIDE tmpdir (that's the whole point of the - // scenario) — put it under this worktree's gitignored .gsd/ directory - // rather than os.homedir() so the test has no dependency on the runner's - // home layout. cleanup() will (correctly, by design) refuse a path there, - // so teardown cannot route through cleanup() either; the raw rmSync below - // is narrowly scoped and justified inline. - const victimParent = path.join(__dirname, '..', '.gsd'); - fs.mkdirSync(victimParent, { recursive: true }); - const victim = fs.mkdtempSync(path.join(victimParent, 'cleanup-symlink-victim-')); - t.after(() => { - if (fs.existsSync(victim)) { - // eslint-disable-next-line local/no-raw-rmsync-in-tests -- victim is deliberately outside os.tmpdir() (that's the scenario under test), so cleanup() refuses it by design and cannot be used for its own teardown. - fs.rmSync(victim, { recursive: true, force: true }); - } - }); - fs.writeFileSync(path.join(victim, 'marker.txt'), 'victim'); - - const symlinkPath = path.join(symlinkParent, 'escape-link'); - let symlinkCreated = true; - try { - fs.symlinkSync(victim, symlinkPath, 'dir'); - } catch (error) { - symlinkCreated = false; - t.skip(`symlink creation not permitted in this environment (${error.code || error.message}) — likely Windows without developer mode`); - } - - if (symlinkCreated) { - // The assert.throws below is the load-bearing assertion for this test: - // it proves the guard refuses a symlink whose target resolves outside - // every accepted tmp root. - assert.throws( - () => cleanup(symlinkPath), - (err) => err instanceof Error && err.message.includes(symlinkPath), - 'cleanup must throw and name the symlink path when it resolves outside os.tmpdir()' - ); - // These two assertions document the end state only — they are NOT proof - // the guard prevented a deletion that would otherwise have happened. A - // probe on node v26.5.1 showed fs.rmSync({recursive:true, force:true}) - // does not follow a top-level symlink target regardless: it unlinks the - // symlink itself and leaves whatever it points at (the victim, here) - // untouched either way. So the victim would survive with or without this - // guard; the guard's own correctness is established solely by the - // assert.throws above. - assert.strictEqual(fs.existsSync(victim), true, 'victim directory still exists (rmSync would not have followed the symlink either way)'); - assert.strictEqual( - fs.existsSync(path.join(victim, 'marker.txt')), - true, - 'victim contents still exist (rmSync would not have followed the symlink either way)' - ); - } -}); diff --git a/tests/helpers.cjs b/tests/helpers.cjs index 7cd90d541..9560b8f9c 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -218,7 +218,27 @@ function createTempGitProject(prefix = 'gsd-test-') { // of that set, not the whole of it (see below). Each probe is wrapped in its // own try/catch — none of them may throw and crash cleanup(), they just // contribute nothing if unavailable. +// Memoization cache for tmpRootCandidates(), keyed on the LIVE os.tmpdir() +// value (not hoisted to a plain module-level constant) — two test files in +// this suite mutate TMPDIR/TEMP/TMP mid-run and restore them afterward, so +// caching on the current os.tmpdir() read is what keeps a stale cache from +// leaking across that override instead of a one-time computation baked in +// at module load. +let _tmpRootCandidatesCacheKey; +let _tmpRootCandidatesCache; + function tmpRootCandidates() { + const cacheKey = os.tmpdir(); + if (cacheKey === _tmpRootCandidatesCacheKey && _tmpRootCandidatesCache) { + return _tmpRootCandidatesCache; + } + const deduped = _computeTmpRootCandidates(); + _tmpRootCandidatesCacheKey = cacheKey; + _tmpRootCandidatesCache = Object.freeze(deduped); + return _tmpRootCandidatesCache; +} + +function _computeTmpRootCandidates() { const roots = []; try { roots.push(path.resolve(os.tmpdir())); @@ -272,19 +292,11 @@ function cleanup(tmpDir) { // destructive rmSync call itself — a wrong `target` would still chdir out of // its own tree and get force-deleted. Hoisted above both the chdir and the // rmSync so an out-of-temp-root path is refused before either can run. - // - // `target` itself is only realpath'd below when it exists (fs.existsSync - // guard on the symlink-escape check) — realpathSync throws ENOENT on a - // missing path, and cleanup() is legitimately called on already-deleted or - // never-created dirs. Comparison is case-insensitive on Windows - // (drive-letter and path casing vary there) and case-sensitive everywhere - // else; the error message below always prints the original-case target. + // Comparison is case-insensitive on Windows (drive-letter and path casing + // vary there) and case-sensitive everywhere else; the error message below + // always prints the original-case target. const isWindows = process.platform === 'win32'; const tmpRoots = tmpRootCandidates(); - // Shared by the literal-target check and the symlink-realpath check below - // so the two cannot drift apart — a second, independently written copy of - // this comparison is how the matching precondition in the tests came to - // disagree with the guard it was meant to mirror. function isUnderRoots(p) { const pForCompare = isWindows ? p.toLowerCase() : p; return tmpRoots.some((root) => { @@ -307,43 +319,10 @@ function cleanup(tmpDir) { `(${tmpRoots.join(', ')}): ${target}` ); } - // Symlink-escape guard: the check above is a string prefix test on `target` - // alone, so a symlink physically located under tmpdir but pointing OUTSIDE - // it would pass that check on `target`'s own path. NOTE: as of this - // writing, `fs.rmSync` itself does not follow a top-level symlink target - // (it unlinks the symlink and leaves whatever it points at untouched), so - // this guard is not closing a live escape against the current rmSync - // behavior — it is defense-in-depth against a future change to the - // deletion mechanism (a different rm implementation, a recursive walk that - // does follow links, etc.) that would make `target` being a symlink - // dangerous. When the target exists, resolve its real path and require - // that to be under a root too, so the guard holds regardless of how the - // eventual delete is implemented. - // - // Only when it exists: cleanup() is legitimately called on already-deleted - // or never-created dirs, and fs.realpathSync throws ENOENT on those — a - // missing target is harmless anyway since rmSync({force: true}) no-ops on it. - // - // Any realpath error (including a non-ENOENT failure) fails CLOSED: refuse - // rather than silently proceed. This is a safety check, not a best-effort - // probe — an unreadable/looping symlink must not be treated as safe. - if (fs.existsSync(target)) { - let real; - try { - real = fs.realpathSync(target); - } catch (error) { - throw new Error( - `cleanup() refused to remove a path it could not verify via realpath ` + - `(${(error && error.code) || (error && error.message) || 'unknown error'}): ${target}` - ); - } - if (!isUnderRoots(real)) { - throw new Error( - `cleanup() refused to remove a path that resolves via symlink to a ` + - `location outside the known temp roots (${tmpRoots.join(', ')}): ${target} -> ${real}` - ); - } - } + // No symlink-escape check here: fs.rmSync does not follow a top-level + // symlink — it unlinks the link itself and leaves the target intact — so + // there is no live hazard for the root-membership check above to guard + // against. That check is the one closing an actual defect. if (cwd === target || cwd.startsWith(`${target}${path.sep}`)) { // Windows cannot remove a directory that is the current working directory. process.chdir(path.dirname(target)); From 5039d499248af08519c248d44fcdfb0dfa59aeb9 Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 5 Aug 2026 23:22:59 -0400 Subject: [PATCH 11/11] ci(#3057): shard the scoped Windows lane, the last unsharded one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scoped Windows lane reached exactly 15m05s and was cancelled on four consecutive shas of PR #3094. A job that exceeds timeout-minutes reports as CANCELLED rather than FAILURE, which is why it first read as infrastructure noise; the giveaway is that the duration equals the cap. The Required tests rollup fans that job in, so it red-blocked merge while every other lane — including all three sharded full-Windows shards — was green. The trigger was a change to the shared test helper, which scopes the install-heavy suites into the selected list. The lane normally runs about eight minutes; with that list it does not fit. It was the only unsharded lane left in this job, so it was the only one without headroom to absorb a large scoped list. Issue #869 hit this exact cliff on the sibling lane and named the durable answer in its own follow-up: a timeout bump moves the cliff, sharding removes it. #2952 then sharded the full lane. This finishes that work. The runner already supports it — the shard partition is applied after scope selection, so it composes with a selected file list rather than only with a suite, and the partition is cost-weighted from the measured timings table. The job name template already renders a shard suffix when one is present, so the three entries name themselves. No individual matrix job is a required status check; the rollup is, and it is name-independent, so renaming these jobs does not touch branch protection. timeout-minutes stays at 15. Each shard now does roughly a third of the work, so the cap goes from binding to backstop without being raised. The lane-shape tests were generalized rather than relaxed: the complete-shard-set invariant now runs per sharded scope instead of only over the full lane, and "only the full lane is sharded" became "targeted is the only unsharded lane". A new assertion pins the shard through to the runner — without it the three shards would each run the entire selected list, triple the cost and no speedup, and every check would stay green. No LANE_COSTS entry is added for the new shards. The only recorded cost for that lane is the pre-sharding run that hit the cap, and inventing a post-sharding number would be exactly the kind of unmeasured claim the rest of that table avoids. The estimate and the reason are written down instead, to be replaced by a real measurement. Refs #3057 Co-Authored-By: Claude Opus 5 --- .github/workflows/test.yml | 39 ++++++++-- tests/ci-full-lane-sharding.test.cjs | 87 +++++++++++++++-------- tests/ci-test-job-timeout-budget.test.cjs | 12 ++++ 3 files changed, 101 insertions(+), 37 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f716b8ebb..24d8e8912 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -116,16 +116,25 @@ jobs: needs: changes if: needs.changes.outputs.product_changed == 'true' runs-on: ${{ matrix.os }} - # #2952: this lane is sharded three ways (see the matrix below), so the - # budget covers ONE shard, not the whole unit suite. Measured on run - # 30677442953: shard 1/3 7m12s, 2/3 4m32s, 3/3 3m59s. Shard 1 is the long - # pole because the unsharded aux suites (integration/security/install/slow) - # ride along on it — that is deliberate; they total ~1m35s and sharding them - # would cost more than it saves. + # #2952: the `scope: full` lane is sharded three ways (see the matrix + # below), so the budget covers ONE shard, not the whole unit suite. + # Measured on run 30677442953: shard 1/3 7m12s, 2/3 4m32s, 3/3 3m59s. Shard + # 1 is the long pole because the unsharded aux suites + # (integration/security/install/slow) ride along on it — that is + # deliberate; they total ~1m35s and sharding them would cost more than it + # saves. # # 15 is ~1.9x the slowest measured shard. Before sharding this same lane ran # 15m20s against a 15-minute cap and was killed mid-run, which is the whole # of #2952 — the number is unchanged, the work behind it is a third the size. + # + # The `scope: windows` lane is sharded three ways for the same reason + # (see #3057): on PR #3094 it reached 15m05s against this same cap and was + # CANCELLED, four shas in a row — a change to tests/helpers.cjs scoped in + # the install-heavy suites and pushed the single Windows lane over the top. + # Per #869, a timeout bump only moves that cliff; sharding removes it. The + # cap stays at 15 unchanged: each shard now does roughly a third of the + # work, so headroom improves rather than needing a raise. # tests/ci-test-job-timeout-budget.test.cjs holds every lane here to a # headroom factor over its own measured cost. timeout-minutes: 15 @@ -158,6 +167,13 @@ jobs: # gave the `test-full` lane, which measures 0.0% spread across 3 bins on # the current table. The aux suites (integration/security/install/slow) # total ~1m35s and are not worth sharding; they run on shard 1 only. + # + # #3057: the `scope: windows` lane is now sharded three ways too, the + # same fix applied to the same cliff (#869's stated durable follow-up). + # It hit exactly 15m05s and was CANCELLED on PR #3094, four shas + # straight, after a change to tests/helpers.cjs scoped in the + # install-heavy suites. Its shards use the same `--shard i/n` + # flag on scripts/run-tests.cjs, applied AFTER scope selection. - os: ubuntu-latest node-version: 22 scope: targeted @@ -176,6 +192,15 @@ jobs: - os: windows-latest node-version: 24 scope: windows + shard: 1/3 + - os: windows-latest + node-version: 24 + scope: windows + shard: 2/3 + - os: windows-latest + node-version: 24 + scope: windows + shard: 3/3 steps: # Windows lane on checkout v5.0.1 (drops includeIf; no auth flake, uses Node 24 natively). @@ -255,7 +280,7 @@ jobs: - name: Run scoped tests if: matrix.scope != 'full' - run: node scripts/run-tests.cjs --files-from .ci-selected-tests.txt + run: node scripts/run-tests.cjs --files-from .ci-selected-tests.txt${{ matrix.shard && format(' --shard {0}', matrix.shard) || '' }} # #2952: each shard runs its slice of the unit suite under c8 but renders # NO report and enforces NO gate — it only leaves raw V8 dumps in diff --git a/tests/ci-full-lane-sharding.test.cjs b/tests/ci-full-lane-sharding.test.cjs index 2721d82b6..7b3a425ce 100644 --- a/tests/ci-full-lane-sharding.test.cjs +++ b/tests/ci-full-lane-sharding.test.cjs @@ -1,13 +1,20 @@ 'use strict'; /** - * The full test lane is sharded, and the coverage gate that sharding displaced - * is still wired in — .github/workflows/test.yml (#2952). + * The full test lane and the scoped Windows lane are both sharded, and the + * coverage gate that sharding displaced is still wired in — + * .github/workflows/test.yml (#2952, #3057). * * The `scope: full` lane was the only unsharded lane in this file. It ran the * entire unit suite under c8 on a single runner, grew past a 15-minute cap, and * reddened `next` (#2952). Raising the cap treated the symptom; sharding is the - * shape fix, and it is the same answer #1212 reached for the Windows lane. + * shape fix, and it is the same answer #1212 reached for the Windows lane at + * the time. + * + * The `scope: windows` lane then hit the identical cliff itself: it reached + * exactly 15m05s and was CANCELLED on PR #3094, four shas in a row. Per #869's + * stated durable follow-up, it is now sharded three ways too (#3057), leaving + * `scope: targeted` as the only lane in this job with no shard. * * Sharding introduces two failure modes that stay GREEN while being wrong, so * both are pinned here: @@ -69,44 +76,64 @@ test('the full test lane is sharded and complete (#2952)', async (t) => { const workflow = loadWorkflow('test.yml'); const include = workflow.jobs.test.strategy.matrix.include; const fullLanes = include.filter((e) => e.scope === 'full'); + const windowsLanes = include.filter((e) => e.scope === 'windows'); + // The only lane in this job with no shard is `scope: targeted` — the fast, + // single-runner default lane. Both `full` and `windows` are sharded. + const shardedScopes = { full: fullLanes, windows: windowsLanes }; - await t.test('the full lane is actually sharded, not a single runner', () => { - assert.ok(fullLanes.length > 0, 'expected at least one `scope: full` matrix entry'); - assert.ok( - fullLanes.length > 1, - 'the `scope: full` lane is back to a single unsharded entry. That is the ' - + '#2952 regression: the whole unit suite under c8 on one runner grew past ' - + 'its cap and reddened `next`.', - ); - for (const lane of fullLanes) { + for (const [scope, lanes] of Object.entries(shardedScopes)) { + await t.test(`the \`scope: ${scope}\` lane is actually sharded, not a single runner`, () => { + assert.ok(lanes.length > 0, `expected at least one \`scope: ${scope}\` matrix entry`); assert.ok( - lane.shard !== undefined, - `a \`scope: full\` matrix entry declares no shard: ${JSON.stringify(lane)}`, + lanes.length > 1, + `the \`scope: ${scope}\` lane is back to a single unsharded entry. That is ` + + 'the #2952/#3057 regression: the whole suite on one runner grows past ' + + 'its cap and reddens `next`.', ); - } - }); + for (const lane of lanes) { + assert.ok( + lane.shard !== undefined, + `a \`scope: ${scope}\` matrix entry declares no shard: ${JSON.stringify(lane)}`, + ); + } + }); - await t.test('the declared shards form one complete set', () => { - const specs = fullLanes.map((e) => e.shard); - assert.ok( - isCompleteShardSet(specs), - `the full lane's shards ${JSON.stringify(specs)} are not a complete set. ` - + 'Every entry must share one denominator N and the numerators must be ' - + 'exactly 1..N — a missing numerator silently stops running that slice of ' - + 'the unit suite while every check stays green.', - ); - }); + await t.test(`the \`scope: ${scope}\` lane's declared shards form one complete set`, () => { + const specs = lanes.map((e) => e.shard); + assert.ok( + isCompleteShardSet(specs), + `the \`scope: ${scope}\` lane's shards ${JSON.stringify(specs)} are not a ` + + 'complete set. Every entry must share one denominator N and the ' + + 'numerators must be exactly 1..N — a missing numerator silently stops ' + + 'running that slice of the suite while every check stays green.', + ); + }); + } - await t.test('only the full lane is sharded', () => { - for (const lane of include.filter((e) => e.scope !== 'full')) { + await t.test('no other lane is sharded', () => { + for (const lane of include.filter((e) => e.scope !== 'full' && e.scope !== 'windows')) { assert.equal( lane.shard, undefined, - `non-full lane ${JSON.stringify(lane)} declares a shard; the scoped and ` - + 'Windows lanes run a selected file list, not a partition.', + `lane ${JSON.stringify(lane)} declares a shard but is neither \`scope: full\` ` + + 'nor `scope: windows` — the targeted lane runs a selected file list, not ' + + 'a partition.', ); } }); + await t.test('the scoped Windows shards are passed through to run-tests.cjs', () => { + const scopedStep = workflow.jobs.test.steps.find( + (s) => s.name === 'Run scoped tests', + ); + assert.ok(scopedStep, 'no "Run scoped tests" step in the test job'); + assert.match( + scopedStep.run, /matrix\.shard/, + 'the "Run scoped tests" step does not reference matrix.shard, so the ' + + 'windows lane\'s three shards would each run the entire selected file ' + + 'list — N times the cost, no speedup.', + ); + }); + await t.test('each shard runs its own slice, not the whole suite', () => { const unitStep = workflow.jobs.test.steps.find( (s) => typeof s.run === 'string' && s.run.includes('test:coverage:unit:raw'), diff --git a/tests/ci-test-job-timeout-budget.test.cjs b/tests/ci-test-job-timeout-budget.test.cjs index 4c52de419..cedce1415 100644 --- a/tests/ci-test-job-timeout-budget.test.cjs +++ b/tests/ci-test-job-timeout-budget.test.cjs @@ -59,6 +59,18 @@ const LANE_COSTS = [ // whole unit suite. Run 30677442953: shard 1/3 7m12s, 2/3 4m32s, 3/3 3m59s. // Shard 1 is the long pole because the unsharded aux suites ride on it. // Before sharding the same lane cost 15m20s and blew a 15-minute cap. + // + // This one `timeout-minutes` also covers the `scope: windows` matrix + // entries — GitHub applies a single job-level budget across every matrix + // combination, not one per entry. That lane is now sharded three ways too + // (#3057), but no post-sharding per-shard measurement exists yet: its only + // recorded cost is the PRE-sharding whole-suite run that hit 15m05s and was + // CANCELLED on PR #3094. Each of its three shards should now cost roughly a + // third of that (~5m), which is already comfortably under the 8m/12m this + // entry requires — so no separate LANE_COSTS entry is added on a number + // that has not actually been measured. Replace this estimate with a real + // measured shard cost once one exists, the same discipline every other + // entry here follows. evidence: 'run 30677442953 — 7m12s slowest shard', }, {