* 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.<includes|startsWith|endsWith>( - readFileSync(...).<includes|startsWith|endsWith>( - 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).
This commit is contained in:
Tom Boucher
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user