From ef43f5161f9d4cec0f735fe302210ff932388416 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 1 May 2026 16:14:39 -0400 Subject: [PATCH] fix(#2969): deterministic Step 5 verification gate for /gsd-reapply-patches (#2972) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2969): deterministic Step 5 verification gate for /gsd-reapply-patches The prior Step 5 "Hunk Verification Gate" was prescribed correctly in the workflow text — but executed laxly by the LLM, which filled in `verified: yes` without actually checking content presence. The reporter observed three distinct files (skills/gsd-discuss-phase/SKILL.md, skills/gsd-autonomous/ SKILL.md, get-shit-done/workflows/new-project.md) where archives contained substantive user-added blocks that did not survive into the merged result, yet the gate reported clean. Move verification from LLM-driven prose into a deterministic Node script the workflow calls. The script can't be shortcut. Changes: - scripts/verify-reapply-patches.cjs (new): pure Node, no external deps. For each file in the patches dir, computes user-added significant lines as the line-set diff between backup and pristine baseline (when available; falls back to "every significant backup line" when no pristine — over-broad but the safe direction for this bug class). Asserts each line appears literally in the merged installed file via String.prototype.includes. Filters trivial lines (length < 12 chars, pure punctuation, decorative comments) so harmless drift doesn't trigger false failures. Exits 0 on pass, 1 on any miss with per-file diagnostic, 2 on usage error. Supports --json for workflow consumption. - get-shit-done/workflows/reapply-patches.md: rewrite Step 5 to call the script and parse its JSON output. The Step 4 Hunk Verification Table remains as advisory Claude-readable summary, but the gate is now the script's exit code. - tests/bug-2969-verify-reapply-patches.test.cjs (new): 6 tests covering (a) pass when every line survives, (b) fail when a line is missing, (c) fail when the merged file is deleted entirely, (d) --json structured report shape, (e) backup-meta.json is correctly skipped as metadata, (f) no-pristine-dir fallback exercises the safe over-broad path. All pass. Out of scope: the manifest-baseline tightening described in #2969 Failure 1 (saveLocalPatches comparing against the wrong baseline so prior silent wipes poison subsequent updates). That's a separate, bigger architectural change involving pristine-content infrastructure; this PR addresses the gate fidelity half so users at least see the diagnostic when content goes missing. Closes #2969 (partial — Failure 2 only) * fix(#2969): preserve #1999 Hunk Verification Table assertions alongside new script gate CI failure on PR #2972 surfaced that tests/reapply-patches.test.cjs (the #1999 contract) asserts Step 5 references: - "Hunk Verification Table" - `verified: no` failure condition - explicit STOP/halt/abort directive - "table absent / missing" halt path My initial Step 5 rewrite for #2969 substituted the deterministic script for the table-based gate entirely, stripping those references. The script is the strictly stronger gate, but the existing #1999 test enforces the table-based safety net as a defense-in-depth contract. Restore both gates as a layered Step 5: - 5a (binding): deterministic verifier script — script gate, exits non-zero on any miss, cannot be shortcut by the LLM - 5b (advisory): Hunk Verification Table review — preserved as redundant safety net for the case where the script has a bug or the pristine baseline is unavailable Both gates must pass. Verified: tests/reapply-patches.test.cjs (5 tests in the #1999 suite) and tests/bug-2969-verify-reapply-patches.test.cjs (6 tests in the #2969 suite) all pass — 21/21 total in this fixture. * fix(#2969): address CodeRabbit findings on workflow + script Five CR findings on PR #2972, all valid; addressed in this commit: 1. (Major) Stderr was merged into VERIFY_OUTPUT via `2>&1`, so any Node warning, deprecation notice, or stack trace would corrupt the JSON parse downstream. Capture stdout only; stderr remains on the controlling terminal for operator visibility. 2. (Major) verifyFile() crashed with EISDIR/EACCES instead of producing a structured diagnostic when the installed path was a directory or unreadable. Wrap statSync/readFileSync in try/catch and emit a per-file fail row; the whole-run gate continues with structured output. Added test case asserting the directory-at-installed-path case fails with `not a regular file` diagnostic instead of crashing. 3. (Minor) PRISTINE_FLAG built as a single string + unquoted expansion would split paths with spaces. Switched to a bash array (VERIFY_ARGS) that preserves whitespace through expansion. 4. (Minor) Fenced code block missing language tag (markdownlint MD040). Added `text` tag to the error message block. 5. (Minor) Usage comment said pristine fallback was "backup-meta lookup" but the actual code path falls back to significant-line checks from backup content. Corrected the comment to match implementation. Verified all 21 tests in tests/reapply-patches.test.cjs (#1999 contract) + tests/bug-2969-verify-reapply-patches.test.cjs (now 7 tests with the new directory case) pass. * test(#2969): structured JSON assertions, no substring matching on script output Replace every assert.match(r.stdout, /pattern/) call with structured assertions on the parsed JSON report from the script's own --json mode. The script's --json contract IS the structured shape we test against — the test author should never depend on the human-readable formatter output, just as no test should depend on substring presence in source. Changes: - All 7 tests now run the verifier with --json (via a runVerifier() helper) and parse the resulting JSON document into { status, report, stderr }. Diagnostic stderr is preserved as a separate channel for debug output but is not used for assertions. - Each previously substring-matched diagnostic ("Failures: 1", "not a regular file", "installed file missing after merge", file path, dropped line) is now a deepEqual / equal / Array.includes against typed report fields: report.failures, report.results[i].status, report.results[i].reason, report.results[i].file, report.results[i].missing[]. - Added an explicit "documented shape" test asserting the JSON output has exactly the keys { file, missing, reason, status } per result — locks the public contract of the --json mode. - DRY'd up fixture reset into a resetFixture() helper since every test starts with a fresh patches/installed/pristine triple. Linter: scripts/lint-no-source-grep.cjs reports 0 violations across 348 test files. Combined run of bug-2969-...test.cjs (7 tests) + reapply-patches.test.cjs (5 tests in the #1999 suite) all pass — 22/22 in the relevant fixture. * fix(#2969): typed REASON enum + raw-text-matching rule shipped repo-wide This commit closes the loop on the no-source-grep discipline: 1. scripts/verify-reapply-patches.cjs: - Frozen REASON enum exposes the diagnostic surface as stable codes: OK_NO_USER_LINES_VS_PRISTINE, OK_NO_SIGNIFICANT_BACKUP_LINES, FAIL_INSTALLED_MISSING, FAIL_INSTALLED_NOT_REGULAR_FILE, FAIL_READ_ERROR, FAIL_USER_LINES_MISSING. - Each result.reason is now a code from this enum, not free text. Tests assert via REASON.X equality, not regex on prose. - REASON exported from module.exports. 2. tests/bug-2969-verify-reapply-patches.test.cjs: - Full rewrite. Every assertion on typed structured fields: report.results[0].status === 'fail', report.results[0].reason === REASON.FAIL_INSTALLED_NOT_REGULAR_FILE, report.results[0].missing.includes(droppedLine) (Array set membership, not String substring). - Locks the REASON enum surface via Object.keys(REASON).sort() deepEqual. - Locks the JSON report shape via Object.keys(report).sort() deepEqual. - Zero regex, zero String#includes, zero startsWith/endsWith on text. 3. CONTRIBUTING.md: - New section "Prohibited: Raw Text Matching on Test Outputs" with concrete BAD/GOOD examples (substring on file content; assert.match on stdout; "structured parser" hiding string ops; regex on free-form reason fields). - The rule statement: "Tests assert on typed structured values. If the code under test produces text, the code under test must also expose a structured intermediate representation, and the test must assert on that IR — never on the rendered text." - Required structured-surface table: file IR, --json mode, frozen enum, fs facts. - "Hiding grep behind a function is still grep" callout — the parser-wrapper anti-pattern. - New `pre-existing-text-matching` exemption category for the 8 grandfathered files. Marked Transitional; new tests cannot use it. 4. scripts/lint-no-source-grep.cjs: - Three new patterns enforced (in addition to the existing .cjs-source readFileSync rule): - assert.match/doesNotMatch on .stdout/.stderr - .stdout/.stderr.( - readFileSync(...).( - Aggregated violations per file (multiple findings now report together). - Updated diagnostic message references both CONTRIBUTING.md sections. 5. 8 pre-existing tests annotated with `// allow-test-rule: pre-existing-text-matching` so the lint passes on this commit; each carries the prose "Tracked for migration to typed-IR assertions; do not copy this pattern." Files: bug-2649, bug-2687, bug-2796, bug-2838, bug-2943, graphify, hooks-opt-in, security-scan. Verification: lint 0 violations across 348 test files; full suite passes. * fix(#2969): rename exemption category to pending-migration-to-typed-ir + cite tracking issue Per maintainer feedback: 1. "Grandfathered" / "legacy" framing is wrong — both terms imply permanent or condoned exemption. The 8 files are tracked for correction, not exempted. 2. Each annotated file must cite the tracking issue so the migration work is auditable. Changes: - CONTRIBUTING.md: rename exemption category from `pre-existing-text-matching` to `pending-migration-to-typed-ir`. Update prose to "Tracked for correction, not exempted" and require each annotation to cite the open migration issue (e.g. `// allow-test-rule: pending-migration-to-typed-ir [#NNNN]`). - 8 test files: update annotation to cite #2974 (the tracking issue opened for migrating these files to typed-IR assertions). --- CHANGELOG.md | 1 + CONTRIBUTING.md | 63 +++++ get-shit-done/workflows/reapply-patches.md | 81 +++++- scripts/lint-no-source-grep.cjs | 66 ++++- scripts/verify-reapply-patches.cjs | 247 ++++++++++++++++++ tests/bug-2649-sdk-fail-fast.test.cjs | 4 + ...g-2687-config-read-warning-parity.test.cjs | 4 + .../bug-2796-arg-parsing-regression.test.cjs | 4 + ...ummary-rescue-gitignored-planning.test.cjs | 4 + ...config-get-context-window-default.test.cjs | 4 + .../bug-2969-verify-reapply-patches.test.cjs | 205 +++++++++++++++ tests/graphify.test.cjs | 4 + tests/hooks-opt-in.test.cjs | 4 + tests/security-scan.test.cjs | 4 + 14 files changed, 673 insertions(+), 22 deletions(-) create mode 100755 scripts/verify-reapply-patches.cjs create mode 100644 tests/bug-2969-verify-reapply-patches.test.cjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 999db8227..1b78cd874 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - **`sketch --wrap-up` now dispatches correctly** — `/gsd-sketch --wrap-up` was silently no-oping because the flag dispatch wiring was omitted when the micro-skill entry point was absorbed in #2790. (#2949) - **`help.md` no longer advertises eight slash commands removed by the #2824 consolidation** — `/gsd-do`, `/gsd-note`, `/gsd-check-todos`, `/gsd-plant-seed`, `/gsd-research-phase`, `/gsd-list-phase-assumptions`, `/gsd-plan-milestone-gaps`, and `/gsd-join-discord` were removed when 86 skills were folded into 59. `help.md` was not updated alongside, so users typing the documented commands hit *Unknown command*. Each entry is now either rewritten to the surviving flag-based dispatcher (e.g., `/gsd-do …` → `/gsd-progress --do "…"`, `/gsd-note` → `/gsd-capture --note`, `/gsd-plant-seed` → `/gsd-capture --seed`, `/gsd-check-todos` → `/gsd-capture --list`) or removed for skills with no replacement. A regression test now asserts every `/gsd-*` reference in `help.md` has a matching `commands/gsd/*.md` stub. (#2954) - **`--sdk` install on Windows now writes a callable `gsd-sdk` shim** — `npx get-shit-done-cc@latest --claude --global --sdk` on Windows previously left `gsd-sdk` off PATH because `trySelfLinkGsdSdk` returned `null` unconditionally on `win32` (a missed gap from #2775's POSIX self-link, not an intentional deferral). The function now dispatches to a Windows counterpart that writes the standard npm shim triple (`gsd-sdk.cmd`, `gsd-sdk.ps1`, and a Bash wrapper) to npm's global bin, so `gsd-sdk` resolves in a fresh shell across cmd.exe, PowerShell, and Cygwin/MSYS/Git-Bash. A new regression guard in `tests/no-unconditional-win32-skip.test.cjs` blocks any future `if (process.platform === 'win32') return null;` skip-only branches in `bin/install.js`. (#2962) +- **`/gsd-reapply-patches` Step 5 gate is now deterministic — no more silent content drops** — the prior gate parsed a Claude-generated *Hunk Verification Table* whose `verified: yes` rows were filled in without actually checking content presence, leading to merged files that lost user-added blocks (e.g., a `` section, an `--execute-only` flag block) while the workflow reported success. The gate now invokes a Node script (`scripts/verify-reapply-patches.cjs`) that diffs each backup against the pristine baseline, computes the user-added significant lines, and asserts each one is present in the merged file. Exits non-zero with a per-file diagnostic on any miss; the workflow halts and surfaces the JSON output to the user. The verifier ignores low-signal lines (too short, pure whitespace, decorative comments) so trivial differences don't trigger false failures. Out of scope here: the manifest-baseline tightening described in #2969 Failure 1 — that's separate work. (#2969) ### Added — 1.40.0-rc.1 - **Six namespace meta-skills with keyword-tag descriptions** — replace the flat 86-skill diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b0a7525f6..9502633da 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -281,6 +281,7 @@ Some tests legitimately read source files. There are six recognized categories: | `docs-parity` | A reference doc must stay in sync with source-defined constants (e.g., `CONFIG_DEFAULTS`). The source is the canonical list; there is no runtime API to enumerate it. | | `integration-test-input` | A source file is used as a real fixture input to a transformation function under test — the file is not inspected for strings but passed as data. | | `structural-implementation-guard` | A feature's interception or wiring point is not reachable end-to-end via `runGsdTools`. Used temporarily until a behavioral path exists. | +| `pending-migration-to-typed-ir` | **Tracked for correction, not exempted.** Test was identified by the lint as carrying a raw-text-matching pattern that contradicts the rule above. Each annotated file MUST cite the open migration issue (e.g. `// allow-test-rule: pending-migration-to-typed-ir [#NNNN]`) so the tracking is auditable. New tests cannot use this category — they must refactor production to expose typed IR. The annotation is removed when the test is corrected. | Annotate with a standalone `//` comment before the file's opening block comment: @@ -296,6 +297,68 @@ Annotate with a standalone `//` comment before the file's opening block comment: The annotation **must** be a standalone `// allow-test-rule:` line, not inside a `/** */` block comment — the CI linter scans for the pattern `// allow-test-rule:`. +### Prohibited: Raw Text Matching on Test Outputs (file content, stdout, stderr) + +**Source-grep is not just `readFileSync` of a `.cjs` file.** The same anti-pattern shows up wherever a test pattern-matches against text that a system-under-test produced, regardless of whether that text came from a source file, a rendered shim, a child process's stdout, or a free-form `reason` string. **All forms are forbidden.** + +The following are all violations of the same rule: + +```javascript +// BAD — substring match on text written by the code under test +const cmdContent = fs.readFileSync(path.join(tmpDir, 'gsd-sdk.cmd'), 'utf8'); +assert.ok(cmdContent.includes(`@node ${jsonQuoted} %*`), '.cmd embeds shim path'); + +// BAD — regex match on a child process's human-readable stdout formatter +const r = cp.spawnSync(SCRIPT, ['--patches-dir', dir]); +assert.match(r.stdout, /Failures: 1/); +assert.match(r.stdout, /not a regular file/); + +// BAD — "structured parser" that hides string ops behind a function wrapper +function parseCmdShim(content) { + const lines = content.split('\r\n').filter((l) => l.length > 0); + return { header: lines[0], usesCRLF: content.includes('\r\n') }; +} + +// BAD — assert.match on a free-form `reason` string from a JSON report +assert.ok(/not a regular file/.test(report.results[0].reason)); +``` + +Each of these passes on accidental near-matches (a comment containing `@node` somewhere, a stack trace that happens to say `Failures: 1`, a mis-typed reason that still contains the substring you're matching) and fails on harmless reformatting (changing `Failures: 1` to `1 failure`, swapping CRLF rendering style, rewording the error prose). + +#### The rule + +> **Tests assert on typed structured values. If the code under test produces text, the code under test must also expose a structured intermediate representation, and the test must assert on that IR — never on the rendered text.** + +Concretely: for any system-under-test that produces text output (a file renderer, a CLI formatter, an error-message builder), the production code MUST expose a typed alternative that the test consumes: + +| Output kind | Required structured surface | What the test asserts on | +|---|---|---| +| Rendered file (shim, template, generated code) | A pure builder function returning the IR (`{ invocation, eol, fileNames, render }`) | `triple.invocation.target === expected`, `triple.eol.cmd === '\r\n'` | +| CLI human-formatter output | A `--json` mode that emits the same data structurally | `report.results[0].reason === REASON.FAIL_INSTALLED_NOT_REGULAR_FILE` | +| Error / status / reason | A frozen enum (`Object.freeze({ FAIL_X: 'fail_x', ... })`) | `assert.equal(result.reason, REASON.FAIL_X)` | +| File presence after a write | `fs.statSync().isFile()`, `.size > 0`, `.mtimeMs` advances | Filesystem facts; never read the file content back | + +#### Concrete examples from this repo + +`buildWindowsShimTriple(shimSrc)` in `bin/install.js` is the canonical IR pattern: pure function, no I/O, returns `{ invocation, eol, fileNames, render }`. `trySelfLinkGsdSdkWindows` calls it and writes `triple.render[kind]()` to disk. Tests assert on `triple.invocation.target`, `triple.eol.cmd`, `Object.keys(triple).sort()` — never on the rendered text. Filesystem-level tests assert `fs.statSync(target).size === Buffer.byteLength(triple.render.cmd())` to prove the writer writes what the renderer produces, **without comparing content**. + +`scripts/verify-reapply-patches.cjs` exposes a frozen `REASON` enum and emits it through `--json`. Tests assert `report.results[0].reason === REASON.FAIL_USER_LINES_MISSING`. The human formatter exists for operator console output only — tests must not depend on its prose. Adding a new reason code requires updating the `REASON` enum, the `--json` output, AND the test that locks `Object.keys(REASON).sort()` — three coordinated changes that prevent the code surface from drifting from the test surface. + +#### Hiding grep behind a function is still grep + +`parseCmdShim`, `parsePs1Invocation`, etc. that internally do `content.split(...)`, `lines[1].trim()`, `content.includes(...)` are still string manipulation. The fact that the entry point looks like a parser doesn't change what's happening underneath — the test is still asserting on the lexical shape of rendered text. The fix is not "wrap the grep in a function with a typed-looking return value." The fix is to **eliminate the rendered text from the test path entirely** by surfacing the IR. + +#### When you cannot eliminate text matching + +There are exactly two cases where text content is the legitimate object of a test, both already covered by the existing exemption matrix: + +1. `source-text-is-the-product` — workflow `.md` / agent `.md` / command `.md` files where the deployed text IS what the runtime loads. +2. `docs-parity` — a reference doc must mirror source-defined constants and there is no runtime enumeration API. + +For everything else, if a test reaches for `.includes()` / `.startsWith()` / `assert.match(text, /…/)`, the production code is missing a typed surface. **Add the typed surface; do not work around it.** + +**CI enforcement:** `scripts/lint-no-source-grep.cjs` is being extended (see issue tracker for the latest scope) to flag `String#includes`/`String#startsWith`/`String#endsWith`/`assert.match` on `readFileSync` results and on `cp.spawnSync` stdout/stderr in test files, with the same `// allow-test-rule:` exemption mechanism. + ### Node.js Version Compatibility **Node 22 is the minimum supported version.** Node 24 is the primary CI target. All tests must pass on both. diff --git a/get-shit-done/workflows/reapply-patches.md b/get-shit-done/workflows/reapply-patches.md index 2ea201b88..5938bda3e 100644 --- a/get-shit-done/workflows/reapply-patches.md +++ b/get-shit-done/workflows/reapply-patches.md @@ -269,17 +269,80 @@ After writing each merged file, verify that user modifications survived the merg ## Step 5: Hunk Verification Gate -Before proceeding to cleanup, evaluate the Hunk Verification Table produced in Step 4. +Two layered gates. Both must pass before proceeding to cleanup. -**If the Hunk Verification Table is absent** (Step 4 did not produce it), STOP immediately and report to the user: -``` -ERROR: Hunk Verification Table is missing. Post-merge verification was not completed. -Rerun /gsd-update --reapply to retry with full verification. +### 5a: Deterministic verifier (binding gate, #2969) + +Run the deterministic verifier script. Do NOT rely solely on the free-text `verified: yes/no` Hunk Verification Table from Step 4 — bug #2969 traced repeated false-positive `verified: yes` reports to that table being filled in without an actual content-presence check. The script performs the check structurally and exits non-zero on any miss. + +Run the verifier as a child process (the gsd-tools binary directory is not required — the script ships under `scripts/` in the source repo and is also exposed via the SDK at `sdk/dist/cli.js verify-reapply` when present): + +```bash +PRISTINE_DIR="${CONFIG_DIR}/gsd-pristine" + +# Build args as a bash array so paths with spaces survive expansion intact +# (string-concat + unquoted expansion would split incorrectly on whitespace). +VERIFY_ARGS=( + --patches-dir "$PATCHES_DIR" + --config-dir "$CONFIG_DIR" +) +if [ -d "$PRISTINE_DIR" ]; then + VERIFY_ARGS+=(--pristine-dir "$PRISTINE_DIR") +fi +VERIFY_ARGS+=(--json) + +# Capture stdout (the structured JSON report) separately from stderr so that +# Node warnings, deprecation notices, or stack traces do not corrupt the +# JSON parse downstream. Stderr is preserved on the controlling terminal +# for operator visibility. +VERIFY_OUTPUT="$(node "${GSD_HOME}/scripts/verify-reapply-patches.cjs" "${VERIFY_ARGS[@]}")" +VERIFY_STATUS=$? ``` -**If any row in the Hunk Verification Table shows `verified: no`**, STOP and report to the user: +**If `VERIFY_STATUS` is non-zero**, STOP and report to the user, parsing the JSON output: + +```text +ERROR: {failures} file(s) failed deterministic post-merge verification (#2969 gate). + +The verifier compared user-added lines (computed from the diff between +the backup and the pristine baseline) against the merged installed file. +Lines listed below are present in the backup but absent from the merged result. + +For each failed file: + {file} + missing: {first significant missing line, up to 5 per file} + backup: {patches_dir}/{file} + +Resolve before proceeding: + (a) Re-merge the missing content into the installed file by hand, or + (b) Restore from backup: cp {patches_dir}/{file} {installed_path} + +Then re-run /gsd-update --reapply to re-verify. ``` -ERROR: {N} hunk(s) failed verification — content may have been dropped during merge. + +Do not proceed to cleanup until the verifier exits 0. + +**Only when `VERIFY_STATUS` is 0** (or when all files had zero significant user-added lines, which the verifier reports as `Failures: 0`) may execution continue to gate 5b. + +### 5b: Hunk Verification Table review (advisory gate, #1999) + +The Hunk Verification Table produced in Step 4 must also be reviewed before proceeding. This is advisory after the script gate but is preserved as a defense-in-depth check — if the script ever has a bug or the pristine baseline is unavailable, the table-based gate still catches obvious regressions. + +**If the Hunk Verification Table is absent** (Step 4 silently produced nothing), STOP and report: + +``` +ERROR: Hunk Verification Table is missing — Step 4 did not produce it. +The deterministic verifier (5a) may still have passed, but a missing table +means post-merge verification was not fully completed. Rerun +/gsd-update --reapply to retry with full verification. +``` + +A missing table absent from the workflow output cannot bypass this gate. + +**If any row in the Hunk Verification Table shows `verified: no`**, STOP and report: + +``` +ERROR: {N} hunk(s) failed Step 5b verification — content may have been dropped during merge. Unverified hunks: {file} hunk {hunk_id}: signature line "{signature_line}" not found in merged output @@ -290,9 +353,9 @@ Review the merged file manually, then either: (b) Restore from backup: cp {patches_dir}/{file} {installed_path} ``` -Do not proceed to cleanup until the user confirms they have resolved all unverified hunks. +Do not proceed to cleanup until both gates (5a and 5b) pass. -**Only when all rows show `verified: yes`** (or when all files had zero user-added hunks) may execution continue to Step 6. +**Why both gates?** 5a (the script) is the binding gate — it does the actual substring check structurally and cannot be shortcut by the LLM. 5b (the table review) is the advisory gate — it provides a redundant safety net via the Step 4 prose summary, ensuring that even a script regression or absent pristine baseline cannot silently allow a `verified: no` row to slip past, nor can a missing table go unnoticed. Layered gates favour false-positive halts (recoverable) over silent successes on lost content (unrecoverable). ## Step 6: Cleanup option diff --git a/scripts/lint-no-source-grep.cjs b/scripts/lint-no-source-grep.cjs index 968030b04..16e29b2ed 100644 --- a/scripts/lint-no-source-grep.cjs +++ b/scripts/lint-no-source-grep.cjs @@ -39,6 +39,29 @@ const READ_WITH_CONST_RE = /readFileSync\s*\(\s*([A-Za-z_][A-Za-z0-9_]*)\s*,/gm; // Matches readFileSync with an inline path.join(.cjs) as first arg const READ_WITH_INLINE_CJS_RE = /readFileSync\s*\([^,)]*path\.join\s*\([^)]*(?:'bin'|"bin"|'lib'|"lib"|'get-shit-done'|"get-shit-done")[^)]*['"][^'"]*\.cjs['"]/; +/** + * #2962-class violations: raw text matching against process output or file + * content. The rule from CONTRIBUTING.md "Prohibited: Raw Text Matching on + * Test Outputs": tests assert on typed structured fields, never on rendered + * text. Patterns below are the obvious anti-patterns; subtler hidden forms + * (e.g. wrapping the same logic in a parser function) are still forbidden + * by the prose rule but cannot be detected lexically without an AST. + */ +const RAW_MATCH_PATTERNS = [ + { + re: /assert\.(?:match|doesNotMatch)\s*\(\s*[A-Za-z_$][A-Za-z0-9_$]*\.(?:stdout|stderr)\b/, + label: 'assert.match/doesNotMatch on .stdout/.stderr (emit --json from the SUT and assert on typed fields)', + }, + { + re: /\.(?:stdout|stderr)\.(?:includes|startsWith|endsWith)\s*\(/, + label: '.stdout/.stderr substring match (emit --json and assert on typed fields)', + }, + { + re: /readFileSync\s*\([^)]*\)\s*\.(?:includes|startsWith|endsWith)\s*\(/, + label: 'readFileSync(...). (expose an IR from production code; assert on its fields)', + }, +]; + function setFromMatches(content, re) { const found = new Set(); let m; @@ -53,13 +76,14 @@ function check(filepath) { if (ALLOW_ANNOTATION.test(content)) return null; + const violations = []; + // Pattern A: readFileSync(path.join(..., 'foo.cjs'), ...) if (READ_WITH_INLINE_CJS_RE.test(content)) { - return { - file: rel, + violations.push({ reason: 'readFileSync with inline .cjs path literal', fix: 'Replace with runGsdTools() behavioral test, or add // allow-test-rule: ', - }; + }); } // Pattern B: const FOO_PATH = path.join(..., 'foo.cjs') + readFileSync(FOO_PATH, ...) @@ -68,15 +92,26 @@ function check(filepath) { const readConsts = setFromMatches(content, READ_WITH_CONST_RE); const overlap = [...cjsConsts].filter(c => readConsts.has(c)); if (overlap.length > 0) { - return { - file: rel, + violations.push({ reason: `source .cjs path constant(s) used in readFileSync: ${overlap.join(', ')}`, fix: 'Replace with runGsdTools() behavioral test, or add // allow-test-rule: ', - }; + }); } } - return null; + // Patterns C..E: raw text matching against process output or file content. + // See CONTRIBUTING.md "Prohibited: Raw Text Matching on Test Outputs". + for (const { re, label } of RAW_MATCH_PATTERNS) { + if (re.test(content)) { + violations.push({ + reason: label, + fix: 'Expose typed IR from production code; assert on structured fields. Or add // allow-test-rule: ', + }); + } + } + + if (violations.length === 0) return null; + return { file: rel, violations }; } function findTestFiles(dir) { @@ -101,12 +136,17 @@ if (violations.length === 0) { process.exit(0); } -process.stderr.write(`\nERROR lint-no-source-grep: ${violations.length} violation(s) found\n\n`); -for (const v of violations) { - process.stderr.write(` ${v.file}\n`); - process.stderr.write(` Problem : ${v.reason}\n`); - process.stderr.write(` Fix : ${v.fix}\n\n`); +const totalIssues = violations.reduce((n, v) => n + v.violations.length, 0); +process.stderr.write(`\nERROR lint-no-source-grep: ${totalIssues} violation(s) across ${violations.length} file(s)\n\n`); +for (const f of violations) { + process.stderr.write(` ${f.file}\n`); + for (const v of f.violations) { + process.stderr.write(` Problem : ${v.reason}\n`); + process.stderr.write(` Fix : ${v.fix}\n`); + } + process.stderr.write('\n'); } -process.stderr.write('See CONTRIBUTING.md "Prohibited: Source-Grep Tests" for guidance.\n'); +process.stderr.write('See CONTRIBUTING.md "Prohibited: Source-Grep Tests" and\n'); +process.stderr.write('"Prohibited: Raw Text Matching on Test Outputs" for guidance.\n'); process.stderr.write('Structural tests that legitimately read source files: add // allow-test-rule: \n\n'); process.exit(1); diff --git a/scripts/verify-reapply-patches.cjs b/scripts/verify-reapply-patches.cjs new file mode 100755 index 000000000..9fe7b2105 --- /dev/null +++ b/scripts/verify-reapply-patches.cjs @@ -0,0 +1,247 @@ +#!/usr/bin/env node +'use strict'; + +/** + * Deterministic verifier for the /gsd-reapply-patches Step 5 "Hunk Verification + * Gate". For each backed-up patch file, asserts that the user's added lines + * (computed from a real diff against the pristine baseline, not from the + * LLM's prose summary) survive into the merged output. + * + * Usage: + * node scripts/verify-reapply-patches.cjs \ + * --patches-dir \ # gsd-local-patches/ + * --config-dir \ # ~/.claude (or runtime equivalent) + * [--pristine-dir ] # gsd-pristine/; if absent, falls back to + * # treating every significant backup line as + * # required (over-broad but safe for #2969: + * # false-positive halts beat silent successes + * # on lost content) + * [--json] # emit JSON report instead of human text + * + * Exit codes: + * 0 — every user-added line is present in the merged file (gate passes) + * 1 — at least one missing line in at least one file (gate fails) + * 2 — usage / structural error (e.g. patches dir missing) + * + * Bug #2969: the Step 5 gate previously trusted Claude's free-text "verified: + * yes/no" reporting per hunk. The LLM was filling in `yes` even when content + * had been silently dropped. Moving the check to a deterministic script is the + * durability fix. + */ + +const fs = require('node:fs'); +const path = require('node:path'); + +const SIGNIFICANT_MIN_CHARS = 12; + +function parseArgs(argv) { + const opts = { patchesDir: null, configDir: null, pristineDir: null, json: false }; + for (let i = 0; i < argv.length; i++) { + const arg = argv[i]; + if (arg === '--patches-dir') opts.patchesDir = argv[++i]; + else if (arg === '--config-dir') opts.configDir = argv[++i]; + else if (arg === '--pristine-dir') opts.pristineDir = argv[++i]; + else if (arg === '--json') opts.json = true; + else if (arg === '--help' || arg === '-h') { + process.stdout.write( + 'usage: verify-reapply-patches.cjs --patches-dir --config-dir [--pristine-dir ] [--json]\n', + ); + process.exit(0); + } else { + process.stderr.write(`unknown argument: ${arg}\n`); + process.exit(2); + } + } + return opts; +} + +function isSignificantLine(line) { + const trimmed = line.trim(); + if (trimmed.length < SIGNIFICANT_MIN_CHARS) return false; + // Pure punctuation / closing brackets carry too little structural info to + // reliably distinguish a survived hunk from incidental similarity. + if (/^[\s})\];,]+$/.test(trimmed)) return false; + // Generic decorative comments like `// ----` similarly fail the test. + if (/^[\s\-=#*/]+$/.test(trimmed)) return false; + return true; +} + +/** + * Walk a directory, returning every file's path relative to the root. + */ +function walk(rootDir, relPrefix = '') { + const out = []; + if (!fs.existsSync(rootDir)) return out; + for (const entry of fs.readdirSync(rootDir, { withFileTypes: true })) { + const rel = relPrefix ? path.join(relPrefix, entry.name) : entry.name; + const abs = path.join(rootDir, entry.name); + if (entry.isDirectory()) { + out.push(...walk(abs, rel)); + } else if (entry.isFile()) { + out.push(rel); + } + } + return out; +} + +/** + * Compute the set of "user-added" lines: lines present in the backup but + * absent from the pristine baseline. If no pristine is provided, falls back + * to using every significant line in the backup (over-broad but safe — favours + * false-positive failures over silent successes, which is the right side to + * err on for #2969). + */ +function computeUserAddedLines(backupContent, pristineContent) { + const backupLines = backupContent.split(/\r?\n/); + if (!pristineContent) { + return backupLines.filter(isSignificantLine); + } + const pristineSet = new Set(pristineContent.split(/\r?\n/)); + return backupLines.filter((line) => isSignificantLine(line) && !pristineSet.has(line)); +} + +/** + * Stable reason codes for the per-file result. Tests assert via + * `assert.equal(result.reason, REASON.X)` rather than regex-matching prose, + * so the diagnostic surface is a typed enum, not free text. + * + * Adding a new reason requires updating the REASON map AND the tests' + * shape assertion that locks the documented set of codes. + */ +const REASON = Object.freeze({ + OK_NO_USER_LINES_VS_PRISTINE: 'ok_no_user_lines_vs_pristine', + OK_NO_SIGNIFICANT_BACKUP_LINES: 'ok_no_significant_backup_lines', + FAIL_INSTALLED_MISSING: 'fail_installed_missing', + FAIL_INSTALLED_NOT_REGULAR_FILE: 'fail_installed_not_regular_file', + FAIL_READ_ERROR: 'fail_read_error', + FAIL_USER_LINES_MISSING: 'fail_user_lines_missing', +}); + +function verifyFile({ relPath, patchesDir, configDir, pristineDir }) { + const backupPath = path.join(patchesDir, relPath); + const installedPath = path.join(configDir, relPath); + const result = { file: relPath, status: 'ok', missing: [], reason: null }; + + if (!fs.existsSync(backupPath) || !fs.statSync(backupPath).isFile()) { + return result; // walked entry no longer exists — non-fatal + } + + // Installed path checks: must exist, must be a regular file, must be + // readable. Anything else is a fail-with-diagnostic, not a crash that + // aborts the whole gate run and drops structured output. + let installedStat; + try { + installedStat = fs.statSync(installedPath); + } catch { + result.status = 'fail'; + result.reason = REASON.FAIL_INSTALLED_MISSING; + return result; + } + if (!installedStat.isFile()) { + result.status = 'fail'; + result.reason = REASON.FAIL_INSTALLED_NOT_REGULAR_FILE; + return result; + } + + let backupContent; + let installedContent; + try { + backupContent = fs.readFileSync(backupPath, 'utf8'); + installedContent = fs.readFileSync(installedPath, 'utf8'); + } catch { + result.status = 'fail'; + result.reason = REASON.FAIL_READ_ERROR; + return result; + } + + let pristineContent = null; + if (pristineDir) { + const pristinePath = path.join(pristineDir, relPath); + try { + const stat = fs.statSync(pristinePath); + if (stat.isFile()) { + pristineContent = fs.readFileSync(pristinePath, 'utf8'); + } + } catch { + // Pristine missing or unreadable — fall through to over-broad mode. + } + } + + const userAdded = computeUserAddedLines(backupContent, pristineContent); + if (userAdded.length === 0) { + // Backup and pristine match exactly (or no significant content) — nothing + // to verify but also nothing to lose. Report as ok with diagnostic code. + result.reason = pristineContent + ? REASON.OK_NO_USER_LINES_VS_PRISTINE + : REASON.OK_NO_SIGNIFICANT_BACKUP_LINES; + return result; + } + + for (const line of userAdded) { + if (!installedContent.includes(line)) { + result.missing.push(line.trim()); + } + } + if (result.missing.length > 0) { + result.status = 'fail'; + result.reason = REASON.FAIL_USER_LINES_MISSING; + } + return result; +} + +function main() { + const opts = parseArgs(process.argv.slice(2)); + if (!opts.patchesDir || !opts.configDir) { + process.stderr.write('--patches-dir and --config-dir are required\n'); + process.exit(2); + } + if (!fs.existsSync(opts.patchesDir)) { + process.stderr.write(`patches dir not found: ${opts.patchesDir}\n`); + process.exit(2); + } + if (!fs.existsSync(opts.configDir)) { + process.stderr.write(`config dir not found: ${opts.configDir}\n`); + process.exit(2); + } + + const files = walk(opts.patchesDir).filter((f) => !f.endsWith('backup-meta.json')); + const results = files.map((relPath) => + verifyFile({ + relPath, + patchesDir: opts.patchesDir, + configDir: opts.configDir, + pristineDir: opts.pristineDir, + }), + ); + + const failures = results.filter((r) => r.status === 'fail'); + + if (opts.json) { + process.stdout.write(JSON.stringify({ checked: results.length, failures: failures.length, results }, null, 2) + '\n'); + } else { + process.stdout.write(`# Hunk Verification Gate (#2969)\n\n`); + process.stdout.write(`Checked: ${results.length} file(s)\n`); + process.stdout.write(`Failures: ${failures.length}\n\n`); + if (failures.length > 0) { + process.stdout.write(`## Files with missing user-added content\n\n`); + for (const r of failures) { + process.stdout.write(`- ${r.file}\n`); + if (r.reason) process.stdout.write(` reason: ${r.reason}\n`); + for (const line of r.missing.slice(0, 5)) { + process.stdout.write(` missing: ${line}\n`); + } + if (r.missing.length > 5) { + process.stdout.write(` …and ${r.missing.length - 5} more line(s)\n`); + } + } + } + } + + process.exit(failures.length > 0 ? 1 : 0); +} + +if (require.main === module) { + main(); +} + +module.exports = { computeUserAddedLines, isSignificantLine, verifyFile, walk, REASON }; diff --git a/tests/bug-2649-sdk-fail-fast.test.cjs b/tests/bug-2649-sdk-fail-fast.test.cjs index f6d3f0404..3478874ff 100644 --- a/tests/bug-2649-sdk-fail-fast.test.cjs +++ b/tests/bug-2649-sdk-fail-fast.test.cjs @@ -11,6 +11,10 @@ 'use strict'; +// allow-test-rule: pending-migration-to-typed-ir [#2974] +// Tracked in #2974 for migration to typed-IR assertions per CONTRIBUTING.md +// "Prohibited: Raw Text Matching on Test Outputs". Do not copy this pattern. + const { test, describe, before } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); diff --git a/tests/bug-2687-config-read-warning-parity.test.cjs b/tests/bug-2687-config-read-warning-parity.test.cjs index d1e3fbd23..b9a3a7d2d 100644 --- a/tests/bug-2687-config-read-warning-parity.test.cjs +++ b/tests/bug-2687-config-read-warning-parity.test.cjs @@ -1,5 +1,9 @@ 'use strict'; +// allow-test-rule: pending-migration-to-typed-ir [#2974] +// Tracked in #2974 for migration to typed-IR assertions per CONTRIBUTING.md +// "Prohibited: Raw Text Matching on Test Outputs". Do not copy this pattern. + /** * Regression test for #2687 — loadConfig must not emit "unknown config key" * warnings for keys that are registered in DYNAMIC_KEY_PATTERNS (e.g. review, diff --git a/tests/bug-2796-arg-parsing-regression.test.cjs b/tests/bug-2796-arg-parsing-regression.test.cjs index 32a5fd108..f9ffa25ec 100644 --- a/tests/bug-2796-arg-parsing-regression.test.cjs +++ b/tests/bug-2796-arg-parsing-regression.test.cjs @@ -17,6 +17,10 @@ 'use strict'; +// allow-test-rule: pending-migration-to-typed-ir [#2974] +// Tracked in #2974 for migration to typed-IR assertions per CONTRIBUTING.md +// "Prohibited: Raw Text Matching on Test Outputs". Do not copy this pattern. + const { describe, test, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); diff --git a/tests/bug-2838-summary-rescue-gitignored-planning.test.cjs b/tests/bug-2838-summary-rescue-gitignored-planning.test.cjs index 41b71c1cf..282dc3ab6 100644 --- a/tests/bug-2838-summary-rescue-gitignored-planning.test.cjs +++ b/tests/bug-2838-summary-rescue-gitignored-planning.test.cjs @@ -24,6 +24,10 @@ 'use strict'; +// allow-test-rule: pending-migration-to-typed-ir [#2974] +// Tracked in #2974 for migration to typed-IR assertions per CONTRIBUTING.md +// "Prohibited: Raw Text Matching on Test Outputs". Do not copy this pattern. + const { describe, test, before, after } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); diff --git a/tests/bug-2943-config-get-context-window-default.test.cjs b/tests/bug-2943-config-get-context-window-default.test.cjs index 8022e29a1..8ae55c138 100644 --- a/tests/bug-2943-config-get-context-window-default.test.cjs +++ b/tests/bug-2943-config-get-context-window-default.test.cjs @@ -12,6 +12,10 @@ 'use strict'; +// allow-test-rule: pending-migration-to-typed-ir [#2974] +// Tracked in #2974 for migration to typed-IR assertions per CONTRIBUTING.md +// "Prohibited: Raw Text Matching on Test Outputs". Do not copy this pattern. + const { describe, test, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); diff --git a/tests/bug-2969-verify-reapply-patches.test.cjs b/tests/bug-2969-verify-reapply-patches.test.cjs new file mode 100644 index 000000000..d3b8b28a4 --- /dev/null +++ b/tests/bug-2969-verify-reapply-patches.test.cjs @@ -0,0 +1,205 @@ +'use strict'; + +process.env.GSD_TEST_MODE = '1'; + +/** + * Bug #2969: /gsd-reapply-patches Step 5 hunk verification gate reports + * success on lost content because the LLM-driven workflow fills in + * "verified: yes" without actually checking content presence. + * + * Fix: deterministic verifier script (scripts/verify-reapply-patches.cjs) + * that the workflow calls. + * + * Per the repo's no-source-grep testing standard (CONTRIBUTING.md): + * tests must assert on TYPED structured fields — not regex/substring + * matching against script output, formatter prose, or file content. + * + * The script's --json mode emits a structured report whose `reason` + * field is a stable enum (exposed as REASON), and whose `missing` field + * is an array of typed strings (exact set membership, not substring). + * Every assertion below is a deepEqual / equal / Array.includes against + * those typed fields. Zero regex, zero String#includes on text. + */ + +const { test, describe, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const cp = require('node:child_process'); + +const ROOT = path.join(__dirname, '..'); +const SCRIPT = path.join(ROOT, 'scripts', 'verify-reapply-patches.cjs'); +const { REASON } = require(SCRIPT); + +let tmpRoot; +let patchesDir; +let configDir; +let pristineDir; + +function writeFile(absPath, content) { + fs.mkdirSync(path.dirname(absPath), { recursive: true }); + fs.writeFileSync(absPath, content); +} + +function resetFixture({ withPristine = true } = {}) { + for (const dir of [patchesDir, configDir, pristineDir]) { + fs.rmSync(dir, { recursive: true, force: true }); + } + fs.mkdirSync(patchesDir); + fs.mkdirSync(configDir); + if (withPristine) fs.mkdirSync(pristineDir); +} + +/** Runs the verifier with --json. Returns parsed structured report. */ +function runVerifier({ includePristine = true } = {}) { + const args = [ + SCRIPT, + '--patches-dir', patchesDir, + '--config-dir', configDir, + ...(includePristine ? ['--pristine-dir', pristineDir] : []), + '--json', + ]; + const r = cp.spawnSync(process.execPath, args, { encoding: 'utf8' }); + return { + status: r.status, + report: r.stdout && r.stdout.length ? JSON.parse(r.stdout) : null, + }; +} + +before(() => { + tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2969-')); + patchesDir = path.join(tmpRoot, 'patches'); + configDir = path.join(tmpRoot, 'installed'); + pristineDir = path.join(tmpRoot, 'pristine'); + resetFixture(); +}); + +after(() => { + fs.rmSync(tmpRoot, { recursive: true, force: true }); +}); + +describe('Bug #2969: deterministic Step 5 verification gate', () => { + test('REASON enum exposes the documented set of stable codes', () => { + // Locks the public diagnostic surface — adding a code requires updating + // this assertion, removing one breaks consumers that switch on the enum. + assert.deepEqual( + Object.keys(REASON).sort(), + [ + 'FAIL_INSTALLED_MISSING', + 'FAIL_INSTALLED_NOT_REGULAR_FILE', + 'FAIL_READ_ERROR', + 'FAIL_USER_LINES_MISSING', + 'OK_NO_SIGNIFICANT_BACKUP_LINES', + 'OK_NO_USER_LINES_VS_PRISTINE', + ], + ); + }); + + test('exits 0 with status=ok when every user-added line is present in the merged file', () => { + resetFixture(); + const pristine = 'line one of stock content here\nline two of stock content here\nline three of stock content here\n'; + const userAdded = 'a custom line the user added for behavior X\nanother substantial line that the user inserted\n'; + + writeFile(path.join(pristineDir, 'skills', 'foo', 'SKILL.md'), pristine); + writeFile(path.join(patchesDir, 'skills', 'foo', 'SKILL.md'), pristine + userAdded); + writeFile(path.join(configDir, 'skills', 'foo', 'SKILL.md'), pristine + userAdded); + + const { status, report } = runVerifier(); + assert.equal(status, 0); + assert.equal(report.failures, 0); + assert.equal(report.checked, 1); + assert.equal(report.results[0].status, 'ok'); + assert.deepEqual(report.results[0].missing, []); + }); + + test('reason=FAIL_USER_LINES_MISSING with the exact dropped line in .missing[]', () => { + resetFixture(); + const pristine = 'first stock line in the original file here\nsecond stock line in the original file here\n'; + const lostLine = 'this is the visual companion block that must survive'; + writeFile(path.join(pristineDir, 'skills', 'discuss-phase', 'SKILL.md'), pristine); + writeFile(path.join(patchesDir, 'skills', 'discuss-phase', 'SKILL.md'), `${pristine}${lostLine}\n`); + writeFile(path.join(configDir, 'skills', 'discuss-phase', 'SKILL.md'), pristine); + + const { status, report } = runVerifier(); + assert.equal(status, 1); + assert.equal(report.failures, 1); + const r0 = report.results[0]; + assert.equal(r0.file, 'skills/discuss-phase/SKILL.md'); + assert.equal(r0.status, 'fail'); + assert.equal(r0.reason, REASON.FAIL_USER_LINES_MISSING); + assert.ok( + r0.missing.includes(lostLine), + `dropped line should be in .missing[]; got ${JSON.stringify(r0.missing)}`, + ); + }); + + test('reason=FAIL_INSTALLED_NOT_REGULAR_FILE when installed path is a directory', () => { + resetFixture(); + writeFile(path.join(pristineDir, 'a.md'), 'pristine line of substantial content here\n'); + writeFile(path.join(patchesDir, 'a.md'), 'pristine line of substantial content here\nuser added line that is substantial\n'); + fs.mkdirSync(path.join(configDir, 'a.md')); // EISDIR trap + + const { status, report } = runVerifier(); + assert.equal(status, 1); + assert.equal(report.results[0].status, 'fail'); + assert.equal(report.results[0].reason, REASON.FAIL_INSTALLED_NOT_REGULAR_FILE); + }); + + test('reason=FAIL_INSTALLED_MISSING when the merged file has been deleted', () => { + resetFixture(); + const pristine = 'stock line one with substantial content for the test\n'; + writeFile(path.join(pristineDir, 'workflow.md'), pristine); + writeFile(path.join(patchesDir, 'workflow.md'), `${pristine}user line that should survive but does not\n`); + // configDir intentionally missing the file. + + const { status, report } = runVerifier(); + assert.equal(status, 1); + assert.equal(report.results[0].status, 'fail'); + assert.equal(report.results[0].reason, REASON.FAIL_INSTALLED_MISSING); + }); + + test('--json report has the documented shape: { checked, failures, results: [{ file, status, missing, reason }] }', () => { + resetFixture(); + const pristine = 'pristine line that is sufficiently long to be significant\n'; + const userAdded = 'extra line the user wrote for their workflow customisation'; + writeFile(path.join(pristineDir, 'a.md'), pristine); + writeFile(path.join(patchesDir, 'a.md'), `${pristine}${userAdded}\n`); + writeFile(path.join(configDir, 'a.md'), pristine); + + const { status, report } = runVerifier(); + assert.equal(status, 1); + assert.deepEqual(Object.keys(report).sort(), ['checked', 'failures', 'results']); + const r0 = report.results[0]; + assert.deepEqual(Object.keys(r0).sort(), ['file', 'missing', 'reason', 'status']); + assert.equal(typeof r0.file, 'string'); + assert.equal(typeof r0.status, 'string'); + assert.equal(typeof r0.reason, 'string'); + assert.ok(Array.isArray(r0.missing)); + }); + + test('ignores backup-meta.json — it is metadata, not a patched file', () => { + resetFixture(); + writeFile(path.join(patchesDir, 'backup-meta.json'), JSON.stringify({ files: [] })); + + const { status, report } = runVerifier(); + assert.equal(status, 0); + assert.equal(report.checked, 0); + assert.equal(report.failures, 0); + assert.deepEqual(report.results, []); + }); + + test('without --pristine-dir, treats every significant backup line as required (safe over-broad fallback)', () => { + resetFixture({ withPristine: false }); + const presentLine = 'this is a substantial line of user content here'; + const droppedLine = 'another substantial line that should survive'; + writeFile(path.join(patchesDir, 'b.md'), `${presentLine}\n${droppedLine}\n`); + writeFile(path.join(configDir, 'b.md'), `${presentLine}\n`); + + const { status, report } = runVerifier({ includePristine: false }); + assert.equal(status, 1); + assert.equal(report.results[0].reason, REASON.FAIL_USER_LINES_MISSING); + assert.ok(report.results[0].missing.includes(droppedLine)); + assert.ok(!report.results[0].missing.includes(presentLine)); + }); +}); diff --git a/tests/graphify.test.cjs b/tests/graphify.test.cjs index e1ec51d3d..db06883c2 100644 --- a/tests/graphify.test.cjs +++ b/tests/graphify.test.cjs @@ -1,5 +1,9 @@ 'use strict'; +// allow-test-rule: pending-migration-to-typed-ir [#2974] +// Tracked in #2974 for migration to typed-IR assertions per CONTRIBUTING.md +// "Prohibited: Raw Text Matching on Test Outputs". Do not copy this pattern. + /** * Tests for get-shit-done/bin/lib/graphify.cjs * diff --git a/tests/hooks-opt-in.test.cjs b/tests/hooks-opt-in.test.cjs index 16cc3477c..3bac1fe52 100644 --- a/tests/hooks-opt-in.test.cjs +++ b/tests/hooks-opt-in.test.cjs @@ -1,3 +1,7 @@ +// allow-test-rule: pending-migration-to-typed-ir [#2974] +// Tracked in #2974 for migration to typed-IR assertions per CONTRIBUTING.md +// "Prohibited: Raw Text Matching on Test Outputs". Do not copy this pattern. + /** * GSD Tools Tests - Community Hooks (opt-in) * diff --git a/tests/security-scan.test.cjs b/tests/security-scan.test.cjs index 9895b1a99..fba31af55 100644 --- a/tests/security-scan.test.cjs +++ b/tests/security-scan.test.cjs @@ -12,6 +12,10 @@ */ 'use strict'; +// allow-test-rule: pending-migration-to-typed-ir [#2974] +// Tracked in #2974 for migration to typed-IR assertions per CONTRIBUTING.md +// "Prohibited: Raw Text Matching on Test Outputs". Do not copy this pattern. + const { describe, test, before, after } = require('node:test'); const assert = require('node:assert/strict'); const { execFileSync, execSync } = require('child_process');