feat(gate-snapshot): preserve what the build already measured (Phase 1, v0.13.0) - #5
feat(gate-snapshot): preserve what the build already measured (Phase 1, v0.13.0)#5drjliddy-max wants to merge 3 commits into
Conversation
…1, v0.13.0)
The gates evaluate hundreds of facts per build - status, canonical, meta
robots, title, description, headings, image alt, JSON-LD, sitemap
membership, lastmod truthfulness, security headers, axe violations - and
discard every one at process exit. Only the exit code survives. That is
why no site accumulates a technical baseline: not a discipline failure,
a missing write call. This is that write call.
NOT a gate. Never fails a build, always exits 0.
ARCHITECTURE
Each gate runs as an isolated child process (bin/_run.mjs spawns tsx), so
no gate can see another's results. Gates write per-gate FRAGMENTS; a
merger binary combines them.
Gates terminate through many paths - gate-ada alone calls process.exit()
from three places, several gates set process.exitCode and fall through -
and process.exit() does NOT run pending finally blocks. A finalizer in
try/finally would therefore miss exactly the failures worth recording.
Instead beginFragment() registers one process.on("exit") handler: it
fires on every path including the top-level catch, and derives outcome
from the REAL exit code rather than a flag someone remembered to set.
Each gate integrates with one line at the top of main() and cannot forget
an exit path.
INVARIANTS (each has a test)
1. Inert unless GATE_SNAPSHOT_DIR is set. Proven end-to-end: a real gate
against a real consumer with no env var produced identical output,
exit 0, and zero files anywhere.
2. Emission can never fail a build. Every fs/serialization path wrapped;
failures warn to stderr. Tested by driving the actual bin with an
unwritable directory and asserting the gate's own exit status survives.
3. Missing is never zero. A gate with no fragment is not_run, never pass,
never omitted. A malformed fragment is error+malformed, never dropped.
4. Cross-mode and cross-environment comparisons return NOT_COMPARABLE.
gate-ada's html-snapshot fallback cannot evaluate color-contrast, so it
legitimately reports FEWER violations than browser mode; comparing
across modes would read measurement loss as improvement. scanMode is
recorded and the comparison is refused. Same for local vs production.
5. No secret VALUES serialized. Shape-based redaction, not field-name
denial, so it catches fields nobody anticipated - gate-ai-instrumentation
embeds a live G-XXXXXX measurement id in its consent-gated exception
message and it is redacted automatically. Env vars by NAME only.
process.env is never iterated. axe records rule id/impact/node COUNT,
never node.html or node.target (selectors embed customer content).
6. Non-finite numbers are dropped and named, not serialized. JSON.stringify
turns NaN/Infinity into null, and a null reads as a measured absence.
Real 0 and false are preserved.
7. Gate names cannot escape the fragments directory: allowlist plus
character check.
snapshotId is content-addressed and EXCLUDES capturedAt, so two merges of
one build are identical and an ingestion endpoint can treat a re-POST as a
no-op. gateConfigHash covers measurement SCOPE only (routes, expected
gates, scope-affecting config) so it is stable across runs and moves only
when what is measured moves.
The merger must be invoked in an always-run step: gate:all chains with &&
so a failing gate stops the chain, which is correct but means a merger at
the end never runs on the builds most worth recording. Documented in the
README with the CI pattern; gates that never ran come back as not_run.
VERIFIED (after rebase onto v0.12.0)
npm test 276/276 pass, 0 fail (241 inherited + 35 new)
npm run typecheck clean
negative control: real gate + real consumer, no env var -> 0 files
real fixture: snapshot captured from bwt-sample-site, scanned clean
of secret shapes and absolute paths, asserted partial
with 4 gates not_run
VERSION
Branched from b4b0c12 (then v0.11.3) deliberately NOT from the concurrent
conversion-contract lane, so the two stayed independent. That lane has
since merged and released v0.12.0, so this new gate takes the next minor:
v0.13.0. Rebased onto origin/main (eb0b674); README and package.json
auto-merged cleanly and the full combined suite is green.
Phase 2 (typed per-route facts) and Phase 3 (Site Monitor ingestion) are
deliberately NOT in this change. No consumer touched, no DDL, no endpoint,
nothing pushed or tagged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Staged so the fleet starts emitting BEFORE any ingestion or dashboard surface exists: history starts at first emission and cannot be reconstructed, so delay costs evidence that no later work recovers. Canary is bwt-sample-site - public showcase, no client impact, and currently 4 minor versions behind, so the canary bump also closes the most visible version drift in the fleet. That jump crosses the v0.10.0 conversion-gate breaking change and the v0.12.0 fail-closed GA4 contract, so it is a migration, not a bump - called out explicitly so a gate:all failure there is not misdiagnosed as a gate-snapshot defect. Flags that bmj-marketing has no test infrastructure at all (surfaced by the conversion-contract rollout lane) as separate work, not to be bundled into a dependency bump. Nothing executed. Operator-authorized steps marked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request adds an opt-in ChangesGate snapshot recording
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (12)
src/snapshot.ts (2)
286-312: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the doc comment: this module sets
error, not the merger.Lines 288-289 state that the merger surfaces
provenance({ errored: true })as outcomeerror. Line 312 already computesoutcome: errored ? "error" : ...here.mergeFragmentscopiesfrag.outcomeverbatim insrc/gate-snapshot.tsat Line 206 and only assignserroritself for malformed fragments. Update the comment so the invariant documentation matches the code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/snapshot.ts` around lines 286 - 312, Update the doc comment above beginFragment to state that this module maps provenance({ errored: true }) to outcome "error" when emitting the fragment; do not attribute that behavior to the merger, which only copies the fragment outcome.
137-144: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrecompute the global regexes once at module scope.
redactSecretsrebuilds 11RegExpobjects on every call.sanitizeValuecalls it for every string value and every object key, so a snapshot with many checks pays that cost repeatedly. Build the global variants once when the module loads.The static analysis hint about "regex from variable input" is a false positive here:
re.sourcecomes from module-local literals only.♻️ Proposed refactor
-const SECRET_SHAPES: Array<{ label: string; re: RegExp }> = [ +const SECRET_SHAPES: Array<{ label: string; re: RegExp }> = [ { label: "google-api-key", re: /\bAIza[0-9A-Za-z_-]{10,}/g }, ... ]; /** Replace any secret-shaped substring with a labelled marker. */ export function redactSecrets(input: string): string { let out = input; for (const { label, re } of SECRET_SHAPES) { - const global = new RegExp(re.source, re.flags.includes("g") ? re.flags : `${re.flags}g`); - out = out.replace(global, `[REDACTED:${label}]`); + re.lastIndex = 0; + out = out.replace(re, `[REDACTED:${label}]`); } return out; }If you keep the non-global literals, hoist the derived globals into a module-level constant instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/snapshot.ts` around lines 137 - 144, Precompute the global RegExp variants from SECRET_SHAPES at module scope instead of constructing them inside redactSecrets. Update redactSecrets to reuse the hoisted compiled patterns while preserving each label and replacement behavior; keep the module-local literal sources unchanged if retaining SECRET_SHAPES.Source: Linters/SAST tools
src/gate-snapshot.ts (1)
129-131: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueNested config objects are hashed in source key order.
productionSeo,sitemap, andaiInstrumentationare embedded verbatim from the parsedgate.config.json.JSON.stringifypreserves the source file's key order, so reordering keys without changing meaning produces a differentgateConfigHashand a false scope-drift report. The sibling arrays on Lines 127, 128, and 132 are sorted for exactly this reason. Apply a recursive key sort to these three values for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gate-snapshot.ts` around lines 129 - 131, Apply recursive key sorting to input.productionSeo, input.sitemap, and input.aiInstrumentation before storing them in the snapshot, matching the canonicalization used for the sibling arrays. Preserve null values and ensure nested object keys are sorted so equivalent configurations produce the same gateConfigHash regardless of source key order.src/__tests__/gate-snapshot.test.ts (7)
113-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests silently pass when the runner is root.
Both tests rely on
chmodSync(locked, 0o444)to make a directory unwritable. Root ignores the permission bits, somkdirSyncsucceeds and no failure path is exercised. Many CI images run as root by default, including GitHub Actions container jobs and Docker images with noUSERdirective. Invariant 2 would then be untested in the environment the rollout targets, and the tests would still report green.Skip these tests when
process.getuid?.() === 0so the gap is visible.💚 Proposed fix
-test("failure isolation: an unwritable snapshot dir does not throw", () => { +const asRoot = process.getuid?.() === 0; + +test("failure isolation: an unwritable snapshot dir does not throw", { skip: asRoot ? "chmod has no effect as root" : false }, () => {Apply the same option to the test at Line 138.
Also applies to: 138-161
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/gate-snapshot.test.ts` around lines 113 - 126, Update both failure-isolation tests around emitFragment, including the test at the referenced later range, to skip when process.getuid?.() === 0 before attempting the permission-based setup. Preserve their existing assertions and cleanup behavior for non-root runners.
84-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe first assertion in this test cannot fail.
diris never passed toemitFragment, andGATE_SNAPSHOT_DIRis unset for the call.emitFragmentreturns before it computes any path, so nothing could ever appear underdir. Line 89 would pass even if emission wrote to a different location. Only Line 90 tests anything.Assert against the location emission would actually use. Set the env var, capture the armed path, then re-run with the var removed and assert the file is absent.
💚 Proposed fix
test("inert: writes nothing when GATE_SNAPSHOT_DIR is unset", () => { const dir = tmp(); + // Prove the path is the one emission would use when armed. + withEnv(SNAPSHOT_DIR_ENV, dir, () => emitFragment(FRAGMENT)); + assert.ok(existsSync(path.join(fragmentsDir(dir), "gate-seo.json"))); + + const inert = tmp(); withEnv(SNAPSHOT_DIR_ENV, undefined, () => { emitFragment(FRAGMENT); }); - assert.equal(existsSync(path.join(dir, "fragments")), false); + assert.equal(existsSync(fragmentsDir(inert)), false); assert.equal(snapshotDir(), null); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/gate-snapshot.test.ts` around lines 84 - 91, Update the inert snapshot test around emitFragment and snapshotDir so it first sets GATE_SNAPSHOT_DIR, captures the armed snapshot directory, then unsets the variable before emitting. Assert the expected fragment file is absent from that captured directory and retain the snapshotDir null assertion, removing the ineffective check against the unrelated tmp() directory.
211-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe per-case assertion does not check that each credential was removed.
Line 213 only confirms that some
[REDACTED:marker appeared somewhere in the output. Line 214 checks forsecretpw, which appears in exactly one of the ten inputs, so it is vacuously true for the other nine. A pattern that matched one character and left the rest of the credential in place would still pass this loop.Assert that the input value itself is absent from the output, and assert the expected label.
💚 Proposed fix
for (const [raw, label] of cases) { const out = redactSecrets(raw); - assert.ok(out.includes("[REDACTED:"), `${label} was not redacted: ${out}`); - assert.ok(!out.includes("secretpw"), "a credential value survived redaction"); + assert.ok( + out.includes(`[REDACTED:${label}]`), + `${label} did not produce its own marker: ${out}`, + ); + assert.ok(!out.includes(raw), `the ${label} value survived redaction: ${out}`); }Note: the
[REDACTED:${label}]assertion requires each input to be matched by its intended pattern rather than by an earlier overlapping one. Adjust the expected label per case if any input matches two patterns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/gate-snapshot.test.ts` around lines 211 - 215, Update the per-case assertions in the redactSecrets test loop to verify that each raw credential value is absent from its corresponding output and that the output contains the expected [REDACTED:${label}] marker. Replace the shared secretpw check with case-specific assertions, and adjust expected labels for any overlapping patterns so each case validates its intended redaction.
492-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test covers
beginFragmentand its exit handler.The suite exercises
emitFragmentdirectly.beginFragmentis the function every gate calls, and its central design claim is that aprocess.on("exit")handler fires on all exit paths, includingprocess.exit(), which skipsfinallyblocks. That claim is untested. The outcome derivation atsrc/snapshot.tsLine 312, which maps the real exit code topassorfail, is also untested.Add a subprocess test that calls
beginFragment, records a check, exits with a chosen code, and then asserts the fragment on disk carries the matching outcome. Cover exit code 0, a non-zeroprocess.exit(), andprovenance({ errored: true }).As per path instructions: "Add tests under
src/__tests__/gate-<name>.test.tsthat test the positive path, the negative path, and at least one edge case".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/gate-snapshot.test.ts` around lines 492 - 543, Add subprocess coverage for beginFragment and its process.on("exit") handler, rather than testing only emitFragment. Exercise process.exit(0), a non-zero process.exit(), and provenance({ errored: true }) after recording a check, then read the emitted fragment and assert its outcome matches the exit code or errored provenance. Place the positive, negative, and edge-case tests in the gate test suite.Source: Path instructions
229-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name promises object-key redaction, but no assertion covers it.
Line 232 puts a credential-shaped string in an object key. Lines 235 and 236 only check the two values. Add an assertion that the key was redacted, since
sanitizeValueappliesredactSecretsto keys atsrc/snapshot.tsLine 178.Unrelated: Line 309 contains a stray character in the section comment,
the核 invariant.💚 Proposed fix
const serialized = JSON.stringify(clean); assert.ok(!serialized.includes("sk_live_abcdef123456")); assert.ok(!serialized.includes("G-ZZZ999")); + assert.ok( + !Object.keys(clean).some((k) => k.includes("Bearer abc12345")), + "a credential-shaped object key survived redaction", + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/gate-snapshot.test.ts` around lines 229 - 237, Update the test “secrets: redaction survives nesting, arrays and object keys” to assert that the sanitized object no longer contains the original credential-shaped key, while preserving the existing value assertions. Also remove the stray character from the section comment containing “the核 invariant.”
199-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSuppress the secret scanner on these test vectors.
Betterleaks reports
sk_live_abcdef123456on Lines 201, 231, and 235 as a Stripe access token. These are synthetic redaction fixtures, not live credentials, so the findings are false positives. Add a scanner ignore directive or move the vectors behind a generated prefix so the high-severity finding does not repeat on every run and mask a real one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/gate-snapshot.test.ts` around lines 199 - 210, Update the synthetic secret vectors in the gate-snapshot tests, especially the Stripe value in the cases array and matching fixtures around it, so Betterleaks no longer flags them. Use the repository’s established scanner-ignore directive or alter the test values with a generated/non-secret prefix while preserving each fixture’s intended detection category.Source: Linters/SAST tools
147-151: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the child-process execution time.
Set
timeout: 30_000inexecFileSync. The wrapper registerstsxitself, so no load-path change is needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/gate-snapshot.test.ts` around lines 147 - 151, Update the execFileSync invocation in the gate snapshot test to include a 30,000-millisecond timeout option, while preserving the existing cwd, env, and stdio settings.Source: Coding guidelines
schema/build-snapshot-v1.schema.json (1)
1-130: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNothing validates a snapshot against this schema.
src/__tests__/gate-snapshot.test.tsasserts individual fields by hand and never loads this file.buildSnapshotoutput and the fixture atsrc/__tests__/fixtures/snapshot/real-bwt-sample-site.snapshot.jsonare therefore free to drift from the contract they claim to satisfy.Add one test that validates both the fixture and a freshly built snapshot against this schema. That test would also catch the unvalidated fragment fields described in my comment on
src/gate-snapshot.tsLines 205-211.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@schema/build-snapshot-v1.schema.json` around lines 1 - 130, Add a schema-validation test alongside the existing gate-snapshot tests that loads build-snapshot-v1.schema.json and validates both the real-bwt-sample-site snapshot fixture and a freshly produced buildSnapshot result. Use a JSON Schema validator and assert both instances pass, ensuring fragment fields are checked against the declared contract rather than only individual fields.src/__tests__/fixtures/snapshot/real-bwt-sample-site.snapshot.json (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis fixture was captured with an older tools version.
toolsVersionis0.11.3, andsrc/__tests__/fixtures/snapshot/real-gate-sitemap-source.fragment.jsonLine 3 records the same version. The PR targets v0.13.0. The fixture therefore documents the output of an earlier build, not the merger in this change.The content is internally consistent:
gatesExpectedhas 7 entries,gatesRunhas 3,gatesNotRunhas 4, and the 5 checks matchsummary.checksTotal. Re-capture the fixture against the current implementation so it can serve as evidence for the code under review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/fixtures/snapshot/real-bwt-sample-site.snapshot.json` at line 17, Re-capture the real-bwt-sample-site snapshot fixture using the current v0.13.0 implementation, updating its toolsVersion and generated output while preserving the internally consistent gate and check counts. Also update the matching version recorded in real-gate-sitemap-source.fragment.json so both fixtures reflect the current build.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/GATE_SNAPSHOT_ROLLOUT.md`:
- Around line 68-79: Update the secret verification grep command in the snapshot
validation instructions to include Bearer tokens and -----BEGIN private-key
markers in its regular expression, while preserving the existing patterns and
expected zero-match result.
In `@README.md`:
- Around line 236-243: Update the Markdown fence in README.md lines 236-243 to
use the text language identifier for the snapshot tree, and update the fence in
docs/GATE_SNAPSHOT_ROLLOUT.md lines 90-94 to use the same text identifier for
the site list.
- Line 236: Update the opening fenced blocks at README.md lines 236-236 and
docs/GATE_SNAPSHOT_ROLLOUT.md lines 90-90 to include the text language
identifier, preserving their non-executable content.
In `@src/gate-ai-instrumentation-source.ts`:
- Around line 562-568: Replace slash-based cwd parsing with path.basename(cwd)
for projectDir in src/gate-ai-instrumentation-source.ts lines 562-568 and
src/gate-conversion-instrumentation-source.ts lines 578-588, ensuring both
provenance implementations produce portable basename-only values across
operating systems.
In `@src/gate-ai-instrumentation.ts`:
- Around line 417-423: Update the catch block for the execution flow in
beginFragment to call fragment.provenance({ errored: true }) before assigning
process.exitCode, so caught execution errors are recorded as errored rather than
merely failed.
In `@src/gate-seo.ts`:
- Around line 647-651: Update the checks payload in emitFragment so
fragment.checks stores only status data, counts, or approved categories, rather
than spreading raw page-derived check objects and names from allChecks. Preserve
route attribution using the existing route context while removing
customer-controlled detail fields before snapshot persistence.
In `@src/gate-snapshot.ts`:
- Around line 286-293: Clarify the intended treatment of check detail in the
snapshot identity derived by the snapshot canonicalization flow. If detail is
intentionally excluded from snapshotId, document the rationale in the nearby
snapshotId doc comment and update the schema description to state that the
content address covers only gate outcomes and check pass results; otherwise,
include detail in the check entries used to compute the canonical snapshot.
- Around line 205-211: Update buildSnapshot to validate parsed fragments before
casting or copying them: require outcome to be pass, fail, or error; checks to
be an array; and each check to contain a string name, boolean pass, and string
detail. Route invalid fragments through the existing malformed path so they
increment malformed and force a partial status, keeping the gate copies safe. In
the violationsBlocking, violationsMinor, scanMode, and colorContrastEvaluated
handling, replace unchecked casts with runtime type checks and return null for
invalid values. Add a test in gate-snapshot.test.ts for parseable JSON with an
invalid outcome, alongside the existing unparseable-fragment test.
- Around line 126-136: Replace every localeCompare-based sort used for
content-addressed data with a deterministic plain codepoint comparator, covering
the three sorts in the canonical hash construction, the sort in
expectedGatesFromScripts, and the sort in computeSnapshotId. Keep the existing
sorting and hashing behavior otherwise unchanged.
- Around line 75-86: Update the execFileSync call in gitOrNull to include a
short timeout option so stalled git subprocesses terminate and the existing
catch returns null. Keep the hardcoded argument arrays and non-shell execution
unchanged.
- Around line 492-499: Replace filesystem-path uses of new
URL(import.meta.url).pathname in the direct-invocation guard and package.json
resolution with fileURLToPath(import.meta.url), reusing the existing import
where available. Ensure invokedDirectly compares normalized filesystem paths
across Windows and paths containing spaces, and update the toolsVersion
resolution near the relevant snapshot logic; prefer the exported toolsVersion()
from src/snapshot.ts if that is the established implementation.
In `@src/snapshot.ts`:
- Line 127: Update sanitizeValue and its recursive callers to retain the current
property key, then make the ga4-api-secret rule redact credential-like keys such
as apiSecret regardless of value contents. Remove the lookahead so ordinary 20+
character tokens are not redacted solely because “secret” appears elsewhere, and
add regression tests covering both key-aware redaction and unrelated tokens.
---
Nitpick comments:
In `@schema/build-snapshot-v1.schema.json`:
- Around line 1-130: Add a schema-validation test alongside the existing
gate-snapshot tests that loads build-snapshot-v1.schema.json and validates both
the real-bwt-sample-site snapshot fixture and a freshly produced buildSnapshot
result. Use a JSON Schema validator and assert both instances pass, ensuring
fragment fields are checked against the declared contract rather than only
individual fields.
In `@src/__tests__/fixtures/snapshot/real-bwt-sample-site.snapshot.json`:
- Line 17: Re-capture the real-bwt-sample-site snapshot fixture using the
current v0.13.0 implementation, updating its toolsVersion and generated output
while preserving the internally consistent gate and check counts. Also update
the matching version recorded in real-gate-sitemap-source.fragment.json so both
fixtures reflect the current build.
In `@src/__tests__/gate-snapshot.test.ts`:
- Around line 113-126: Update both failure-isolation tests around emitFragment,
including the test at the referenced later range, to skip when
process.getuid?.() === 0 before attempting the permission-based setup. Preserve
their existing assertions and cleanup behavior for non-root runners.
- Around line 84-91: Update the inert snapshot test around emitFragment and
snapshotDir so it first sets GATE_SNAPSHOT_DIR, captures the armed snapshot
directory, then unsets the variable before emitting. Assert the expected
fragment file is absent from that captured directory and retain the snapshotDir
null assertion, removing the ineffective check against the unrelated tmp()
directory.
- Around line 211-215: Update the per-case assertions in the redactSecrets test
loop to verify that each raw credential value is absent from its corresponding
output and that the output contains the expected [REDACTED:${label}] marker.
Replace the shared secretpw check with case-specific assertions, and adjust
expected labels for any overlapping patterns so each case validates its intended
redaction.
- Around line 492-543: Add subprocess coverage for beginFragment and its
process.on("exit") handler, rather than testing only emitFragment. Exercise
process.exit(0), a non-zero process.exit(), and provenance({ errored: true })
after recording a check, then read the emitted fragment and assert its outcome
matches the exit code or errored provenance. Place the positive, negative, and
edge-case tests in the gate test suite.
- Around line 229-237: Update the test “secrets: redaction survives nesting,
arrays and object keys” to assert that the sanitized object no longer contains
the original credential-shaped key, while preserving the existing value
assertions. Also remove the stray character from the section comment containing
“the核 invariant.”
- Around line 199-210: Update the synthetic secret vectors in the gate-snapshot
tests, especially the Stripe value in the cases array and matching fixtures
around it, so Betterleaks no longer flags them. Use the repository’s established
scanner-ignore directive or alter the test values with a generated/non-secret
prefix while preserving each fixture’s intended detection category.
- Around line 147-151: Update the execFileSync invocation in the gate snapshot
test to include a 30,000-millisecond timeout option, while preserving the
existing cwd, env, and stdio settings.
In `@src/gate-snapshot.ts`:
- Around line 129-131: Apply recursive key sorting to input.productionSeo,
input.sitemap, and input.aiInstrumentation before storing them in the snapshot,
matching the canonicalization used for the sibling arrays. Preserve null values
and ensure nested object keys are sorted so equivalent configurations produce
the same gateConfigHash regardless of source key order.
In `@src/snapshot.ts`:
- Around line 286-312: Update the doc comment above beginFragment to state that
this module maps provenance({ errored: true }) to outcome "error" when emitting
the fragment; do not attribute that behavior to the merger, which only copies
the fragment outcome.
- Around line 137-144: Precompute the global RegExp variants from SECRET_SHAPES
at module scope instead of constructing them inside redactSecrets. Update
redactSecrets to reuse the hoisted compiled patterns while preserving each label
and replacement behavior; keep the module-local literal sources unchanged if
retaining SECRET_SHAPES.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f25e0d52-09e9-41aa-af48-2626aee32dbb
📒 Files selected for processing (18)
.gitignoreREADME.mdbin/gate-snapshot.mjsdocs/GATE_SNAPSHOT_ROLLOUT.mdpackage.jsonschema/build-snapshot-v1.schema.jsonsrc/__tests__/fixtures/snapshot/real-bwt-sample-site.snapshot.jsonsrc/__tests__/fixtures/snapshot/real-gate-sitemap-source.fragment.jsonsrc/__tests__/gate-snapshot.test.tssrc/gate-ada.tssrc/gate-ai-instrumentation-source.tssrc/gate-ai-instrumentation.tssrc/gate-conversion-instrumentation-source.tssrc/gate-dashboard-parity.tssrc/gate-seo.tssrc/gate-sitemap-source.tssrc/gate-snapshot.tssrc/snapshot.ts
| | Schema | validates against `schema/build-snapshot-v1.schema.json` | | ||
| | Secrets | no `G-`, `AIza`, `sk_live`, `postgres://`, `Bearer`, `-----BEGIN`, no `/Users/` paths | | ||
| | `build.environment` | `local` from a laptop, `production` only from a Vercel production build | | ||
| | `summary.comparability.adaScanMode` | present — `browser` locally, likely `html-snapshot` on Vercel | | ||
| | `completeness.status` | `complete` when the whole chain ran | | ||
| | `build.commitSha` | a real SHA, not null, not invented | | ||
| | `snapshotId` | stable across two merges of the same build | | ||
|
|
||
| ```bash | ||
| node -e 'const s=require("./.gate-snapshots/snapshot.json"); | ||
| console.log(s.build.environment, s.summary.comparability.adaScanMode, s.completeness.status, s.build.commitSha)' | ||
| grep -ciE "G-[A-Z0-9]{6,}|AIza|sk_live|postgres://|/Users/" .gate-snapshots/snapshot.json # expect 0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
sed -n '68,79p' docs/GATE_SNAPSHOT_ROLLOUT.md
python3 - <<'PY'
import re
text = "authorization: Bearer token\n-----BEGIN PRIVATE KEY-----\n"
pattern = re.compile(r"G-[A-Z0-9]{6,}|AIza|sk_live|postgres://|/Users/")
print("original_matches:", len(pattern.findall(text)))
pattern = re.compile(r"G-[A-Z0-9]{6,}|AIza|sk_live|postgres://|Bearer|-----BEGIN|/Users/")
print("expanded_matches:", len(pattern.findall(text)))
PYRepository: drjliddy-max/build-websites-tools
Length of output: 1057
Security Misconfiguration (CWE-693)
Reachability: Internal
Complete the secret verification command.
Add Bearer and -----BEGIN to the expression:
-grep -ciE "G-[A-Z0-9]{6,}|AIza|sk_live|postgres://|/Users/" .gate-snapshots/snapshot.json
+grep -ciE "G-[A-Z0-9]{6,}|AIza|sk_live|postgres://|Bearer|-----BEGIN|/Users/" .gate-snapshots/snapshot.json📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Schema | validates against `schema/build-snapshot-v1.schema.json` | | |
| | Secrets | no `G-`, `AIza`, `sk_live`, `postgres://`, `Bearer`, `-----BEGIN`, no `/Users/` paths | | |
| | `build.environment` | `local` from a laptop, `production` only from a Vercel production build | | |
| | `summary.comparability.adaScanMode` | present — `browser` locally, likely `html-snapshot` on Vercel | | |
| | `completeness.status` | `complete` when the whole chain ran | | |
| | `build.commitSha` | a real SHA, not null, not invented | | |
| | `snapshotId` | stable across two merges of the same build | | |
| ```bash | |
| node -e 'const s=require("./.gate-snapshots/snapshot.json"); | |
| console.log(s.build.environment, s.summary.comparability.adaScanMode, s.completeness.status, s.build.commitSha)' | |
| grep -ciE "G-[A-Z0-9]{6,}|AIza|sk_live|postgres://|/Users/" .gate-snapshots/snapshot.json # expect 0 | |
| | Schema | validates against `schema/build-snapshot-v1.schema.json` | | |
| | Secrets | no `G-`, `AIza`, `sk_live`, `postgres://`, `Bearer`, `-----BEGIN`, no `/Users/` paths | | |
| | `build.environment` | `local` from a laptop, `production` only from a Vercel production build | | |
| | `summary.comparability.adaScanMode` | present — `browser` locally, likely `html-snapshot` on Vercel | | |
| | `completeness.status` | `complete` when the whole chain ran | | |
| | `build.commitSha` | a real SHA, not null, not invented | | |
| | `snapshotId` | stable across two merges of the same build | | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/GATE_SNAPSHOT_ROLLOUT.md` around lines 68 - 79, Update the secret
verification grep command in the snapshot validation instructions to include
Bearer tokens and -----BEGIN private-key markers in its regular expression,
while preserving the existing patterns and expected zero-match result.
Additive repair commit. No history rewritten, no public commit amended.
F1 ARBITRARY FILESYSTEM WRITE (blocking)
GATE_SNAPSHOT_DIR was both the opt-in switch AND the caller-chosen
destination, so any non-blank value was treated as a directory. Confirmed
empirically before repair: an absolute path outside the repository wrote
snapshot.json there and exited 0.
Authorization and destination are now separate concerns:
AUTHORIZATION GATE_SNAPSHOT_ENABLED=1 (exact string "1", nothing else)
DESTINATION <git-toplevel>/.build-websites-tools/gate-snapshot/
fixed, repository-relative, never caller-supplied
GATE_SNAPSHOT_DIR is retained ONLY to be rejected: its presence is invalid
deprecated configuration, its value is never interpreted as a path, and the
merger exits 1 without writing. Confinement is proven at the write boundary,
not just the CLI: git toplevel canonicalized via realpath, containment
checked with path.relative (NOT a string prefix, which "/repo-evil" defeats
against "/repo"), nearest existing parent canonicalized, symlinked roots /
parents / destinations rejected, and the directory re-canonicalized AFTER
mkdir so a symlink planted between check and use is caught.
F2 ZERO EVIDENCE MARKED COMPLETE (blocking)
Confirmed before repair: zero expected gates + zero fragments returned
status "complete" WITH a reason that contradicted it. Completeness is now
mechanical. complete requires ALL of: valid config, available build
identity, a NON-EMPTY expected set, exactly one valid fragment per expected
gate, and no not_run / malformed / unknown gate. A gate outcome of "fail" is
compatible with complete - completeness describes evidence COVERAGE, not
success. reason is null if and only if status is complete.
F3 MISSING RUNTIME VALIDATION (blocking)
Parsed JSON was cast to Fragment after only a typeof check. New
src/snapshot-validate.ts validates schema version, gate name, outcome enum,
timestamps, checks container and per-check key types, provenance shape, and
rejects null / primitives / arrays / nested values. A 26-case invalid-input
matrix covers it. The final document is validated against the SHIPPED schema
before it can replace a valid snapshot.json. A dependency-free subset
validator is used rather than ajv (this package ships into nine consumer
builds); an unsupported schema keyword is an ERROR, never a silent pass.
F4 NON-ATOMIC FINAL WRITE (blocking)
snapshot.json was written directly. All writes now go through one boundary:
unique temp file in the SAME directory, fsync, atomic rename. Temp files are
cleaned up on every failure path. Concurrency policy is explicit and
documented - last validated writer wins; no lock, because a lock adds a
stale-lock failure mode to a tool whose job is to not disturb the build.
F5 PACKAGE CONTRACT DIVERGENCE (blocking)
Confirmed before repair: files was ["bin","src","README.md"] so the schema
the README documents did NOT ship while 19 test entries and 4 fixtures DID.
Now schema ships and tests/fixtures/internal docs do not. New
package-contract.test.ts asserts required entries and forbidden patterns,
AND packs, installs into a temp directory, and drives the installed binary
inside a real git repo - proving the artifact, not the working tree.
F6 PUBLIC DISCLOSURE + FIXTURE SAFETY
docs/GATE_SNAPSHOT_ROLLOUT.md removed from this PUBLIC repo: it carried
portfolio-operational inventory. The README now documents adoption
generically. Synthetic credentials in tests are assembled at runtime from
non-secret segments so no production-shaped literal is committed or packed.
NOTE: the bmj-marketing disclosure remains in the immutable commit message
of 9e200df and CANNOT be removed by an additive commit - operator decision
required, see the report.
F7 EXIT CODES
The CLI caught every failure and always exited 0, so an armed build could
silently produce nothing. Now: 0 when unauthorized (documented inert
default) or a snapshot was written; 1 for invalid config, non-repository
invocation, confinement failure, schema-validation failure, write failure,
or internal error. A partial snapshot still exits 0 - partial is valid
evidence; only an inability to PRODUCE evidence is fatal. Subprocess tests
assert exit code, stderr and generated files.
F8 CODERABBIT (12 comments, all dispositioned)
Fixed: new URL().pathname -> fileURLToPath (CRITICAL - percent-encoding and
/C:/ on Windows meant the direct-invocation guard could never fire and the
CLI silently wrote nothing); localeCompare -> byte comparator in all sort
sites (locale/ICU made a CONTENT ADDRESS machine-dependent); git subprocess
timeout; errored:true in the gate-ai-instrumentation catch; path.basename
instead of cwd.split("/"); key-aware secret redaction (the old lookahead
missed {apiSecret:"<opaque>"} and over-redacted unrelated tokens);
snapshotId now includes check detail, matching the schema description;
gate-seo no longer persists raw page-derived detail text into an artifact
that can be uploaded; markdown fence languages.
MUTATION CAMPAIGN: 14/14 caught, 0 escaped.
Three escaped on the first run and were REAL test gaps, now closed:
M3 destination read from env -> direct assertion that the artifact root
ignores all environment input
M9 final validation bypassed -> injectable schema proves runCli refuses
to replace a valid snapshot
M11 temp cleanup removed -> the directory pre-check short-circuited
before a temp file existed, so cleanup
was never exercised
The campaign also surfaced an order dependence: an escaping write from M4
landed in the shared temp root and broke a later run. makeRepo now nests
each repo in its own parent. Harness now asserts each patch applied, so a
non-applying mutation can no longer read as "caught".
VERIFIED (clean install)
npm ci ok
npm test 362/362 pass, 0 fail, 0 skipped, 0 todo
npm run typecheck clean
npm pack --dry-run 35 entries, schema in, tests/fixtures out
lint / build NO SUCH SCRIPT in this package - reported, not skipped
backward compat unarmed gate vs merge-base eb0b674: byte-identical
stdout, identical exit 0, zero files created
Nothing merged, tagged, published, deployed. No consumer touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/gate-snapshot.ts (2)
395-418: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
snapshotIdignores the measurement mode it is compared by.The canonical input covers
domain,commitSha,buildId,environment,gateConfigHash, and per-gateoutcomepluschecks. It excludesprovenance, and therefore excludessummary.comparability.adaScanMode.
gate-adacan run inbrowserorhtml-snapshotmode for the same commit and the samegateConfigHash. Both runs produce the samesnapshotIdwhilesummary.axeViolationsBlocking,summary.axeViolationsMinor, andsummary.comparability.adaScanModediffer. The doc comment at Lines 386-387 states that an ingestion endpoint can treat a re-POST as a no-op. Under that rule the endpoint keeps the first mode and discards the second, andassertComparableat Lines 446-454 then reads a mode that does not match the stored counts.Include the comparability axes in the address.
🔧 Proposed fix
export function computeSnapshotId(parts: { domain: string | null; commitSha: string | null; buildId: string | null; environment: string; gateConfigHash: string; + adaScanMode: string | null; gates: Record<string, MergedGate>; }): string { const canonical = JSON.stringify({ domain: parts.domain, commitSha: parts.commitSha, buildId: parts.buildId, environment: parts.environment, gateConfigHash: parts.gateConfigHash, + adaScanMode: parts.adaScanMode, gates: Object.keys(parts.gates)Pass
summary.comparability.adaScanModeat the call site inbuildSnapshot(Lines 628-635).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gate-snapshot.ts` around lines 395 - 418, Update computeSnapshotId and its buildSnapshot call site to include summary.comparability.adaScanMode in the canonical snapshot address. Ensure browser and html-snapshot runs produce distinct IDs while preserving the existing hashing behavior for all other fields.
118-132: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAn empty
VERCEL_*variable is treated as a resolved value.
??falls back only onundefinedandnull. Vercel exportsVERCEL_GIT_COMMIT_SHAandVERCEL_GIT_COMMIT_REFas empty strings when the deployment has no linked git metadata. In that casecommitShabecomes"", the git fallback never runs, andcommitSha ?? nullkeeps"".
buildSnapshotthen setsbuildIdentityAvailable: identity.commitSha !== null(Line 599), which istrue.completeness.statuscan becompletefor a snapshot with no usable commit SHA. The schema accepts""becausecommitShais["string", "null"].Treat a blank value as unresolved.
🔧 Proposed fix
+const nonBlank = (v: string | undefined): string | null => + typeof v === "string" && v.trim().length > 0 ? v : null; + export function resolveBuildIdentity( env: NodeJS.ProcessEnv = process.env, cwd: string = process.cwd(), ) { - const commitSha = env.VERCEL_GIT_COMMIT_SHA ?? gitOrNull(["rev-parse", "HEAD"], cwd); - const branch = - env.VERCEL_GIT_COMMIT_REF ?? gitOrNull(["rev-parse", "--abbrev-ref", "HEAD"], cwd); + const commitSha = nonBlank(env.VERCEL_GIT_COMMIT_SHA) ?? gitOrNull(["rev-parse", "HEAD"], cwd); + const branch = + nonBlank(env.VERCEL_GIT_COMMIT_REF) ?? gitOrNull(["rev-parse", "--abbrev-ref", "HEAD"], cwd); return { commitSha: commitSha ?? null, branch: branch ?? null, - buildId: env.VERCEL_DEPLOYMENT_ID ?? null, + buildId: nonBlank(env.VERCEL_DEPLOYMENT_ID),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gate-snapshot.ts` around lines 118 - 132, Update resolveBuildIdentity to treat blank VERCEL_GIT_COMMIT_SHA and VERCEL_GIT_COMMIT_REF values as unresolved, allowing gitOrNull to run and returning null when no fallback exists. Preserve non-blank environment values and the existing build identity fields.
🧹 Nitpick comments (5)
src/gate-snapshot.ts (1)
579-579: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThread the caller-supplied
envintoresolveBuildIdentity.
runCliaccepts anenvparameter and uses it forauthorizeat Line 662.buildSnapshotignores it and readsprocess.envhere. A caller that supplies a customenvgets a snapshot whosecommitSha,branch,buildId,environment, andcicome from the real process environment. The injection seam is then partial, and a test cannot exercise environment classification throughrunCli.Add an
envparameter tobuildSnapshotand pass it through fromrunCli.♻️ Proposed refactor
-export function buildSnapshot(cwd: string, artifactRoot: string): BuildOutcome { +export function buildSnapshot( + cwd: string, + artifactRoot: string, + env: NodeJS.ProcessEnv = process.env, +): BuildOutcome {- const identity = resolveBuildIdentity(process.env, cwd); + const identity = resolveBuildIdentity(env, cwd);And at Line 680:
- built = buildSnapshot(cwd, resolved.artifactRoot); + built = buildSnapshot(cwd, resolved.artifactRoot, env);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gate-snapshot.ts` at line 579, Update buildSnapshot to accept the caller-supplied env object and use it when invoking resolveBuildIdentity instead of process.env. Update runCli to pass its env argument into buildSnapshot, preserving the injected environment consistently for authorization and snapshot identity.src/__tests__/gate-snapshot.test.ts (3)
706-716: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test cannot fail; it does not exercise the write path.
The test seeds
snapshot.jsonwith"PRIOR-VALID", validates a hand-built object, then re-reads the same file. No code under test touches the file between the two steps, so line 715 always passes. The real behavior is covered at lines 1107-1134 by therunClitest. Delete this test, or route it throughrunCliwith an injected schema so the refusal-to-replace path is actually executed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/gate-snapshot.test.ts` around lines 706 - 716, Remove the non-exercising test around the hand-built validation result, or rewrite it to invoke runCli with an injected schema that rejects the snapshot. Ensure the test drives the actual snapshot write path and verifies that a previously valid snapshot.json remains unchanged.
411-423: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
chmodSyncdoes not block writes on every runner.
chmodSync(parent, 0o500)has no effect on Windows, and a root user in a container ignores the missing write bit. On those runners the test no longer creates an unwritable directory, and it passes without exercising the failure path. Skip the test when the mode change does not take effect, so the intent stays explicit.🔧 Proposed guard
test("emission failure never throws (unwritable artifact parent)", () => { const repo = makeRepo(); const parent = path.join(repo, ARTIFACT_DIR_SEGMENTS[0]); mkdirSync(parent, { recursive: true }); chmodSync(parent, 0o500); + const enforced = (() => { + try { + writeFileSync(path.join(parent, ".probe"), "x"); + rmSync(path.join(parent, ".probe")); + return false; + } catch { + return true; + } + })(); + if (!enforced) { + chmodSync(parent, 0o755); + return; // permissions are not enforced on this runner + } try {As per coding guidelines: "gates must be portable across operating systems and CI runners".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/gate-snapshot.test.ts` around lines 411 - 423, Update the “emission failure never throws (unwritable artifact parent)” test to verify that chmodSync actually makes the parent unwritable before calling emitFragment; skip the test when the permission change is ineffective, including on Windows or privileged runners, and always restore the original permissions in cleanup.Source: Coding guidelines
90-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemporary fixture directories are never removed.
tmp()creates a directory underos.tmpdir()for everymakeRepocall and for each escape root. Nothing deletes them. The suite creates dozens of git repositories per run, and each one leaks. Register cleanup witht.afterin each test, or track the created roots and remove them in a singleafterhook.Also applies to: 130-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/gate-snapshot.test.ts` around lines 90 - 95, Update the temporary-directory setup around tmp() and its callers, including makeRepo and escape-root creation, to register cleanup with each test’s t.after hook or a shared after hook. Recursively remove every created temporary root after the test completes while preserving the existing realpath-based fixture behavior.src/__tests__/package-contract.test.ts (1)
30-37: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
packedFiles()runsnpm packon every call.Four tests call
packedFiles(), sonpm pack --dry-runruns four times. Each invocation walks the whole working tree. Memoize the result, because the tarball contents do not change during one test run.♻️ Proposed refactor
-function packedFiles(): string[] { +let packedFilesCache: string[] | undefined; + +function packedFiles(): string[] { + if (packedFilesCache) return packedFilesCache; const out = execFileSync("npm", ["pack", "--dry-run", "--json"], { cwd: PKG_ROOT, encoding: "utf8", stdio: ["ignore", "pipe", "ignore"], }); - return (JSON.parse(out)[0].files as Array<{ path: string }>).map((f) => f.path); + packedFilesCache = (JSON.parse(out)[0].files as Array<{ path: string }>).map((f) => f.path); + return packedFilesCache; }Also applies to: 65-92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/package-contract.test.ts` around lines 30 - 37, Memoize the result of packedFiles() so npm pack --dry-run executes only once per test run; preserve the existing file-path parsing and return behavior for all callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@schema/build-snapshot-v1.schema.json`:
- Around line 208-229: Update validateFragment to reject check records
containing fields other than name, pass, and detail, matching the checks.items
schema and preventing unknown fields from reaching the merged snapshot. Preserve
validation of the existing required fields and ensure runCli handles the
rejected fragment without producing an invalid snapshot.
In `@src/__tests__/gate-snapshot.test.ts`:
- Around line 425-444: Update the subprocess invocation in the test “a real gate
subprocess keeps its own exit status when snapshots cannot be written” to use
cwd: repo instead of PKG_ROOT, ensuring resolveArtifactRoot targets the chmod-ed
fixture directory and no artifacts are written to the tools repository. Preserve
the assertion that the gate exits 0; if the fixture lacks required inputs, seed
it appropriately rather than changing gate behavior.
In `@src/__tests__/package-contract.test.ts`:
- Around line 139-143: Document the network or npm-cache requirement for the
package installation exercised by the package-contract test around the
execFileSync call. State that installing the tarball resolves runtime
dependencies such as playwright and jsdom from the registry or cache, and note
that offline runners must pre-cache them or use --offline.
- Around line 197-201: Update the package-contract test callback to be async,
dynamically import validateAgainstSchema from the installed
src/snapshot-validate.ts via pathToFileURL, and validate the complete produced
output against the installed schema. Assert result.valid and include serialized
diagnostics on failure, while guarding completeness property accesses so schema
changes yield assertion failures rather than TypeError.
In `@src/snapshot-validate.ts`:
- Around line 156-175: Remove propertyNames and maxItems from the
SUPPORTED_KEYWORDS allowlist in snapshot validation, since walk does not
evaluate either keyword. Leave the existing minItems handling and all
implemented keyword entries unchanged so unsupported constructs continue to
trigger validation errors.
- Around line 257-270: Update the property lookup in the object-walking logic
around walk so schema.properties keys are accepted only when they are own
properties, not inherited Object.prototype names. Use an own-property check
before calling walk; otherwise preserve the existing additionalProperties schema
validation and additionalProperties: false rejection behavior.
In `@src/snapshot.ts`:
- Around line 564-579: Update the snapshot outcome flow to recognize declared
skips: in the exit handler, derive a distinct skipped outcome when
provenance.skipped is true, while preserving error, pass, and fail behavior
otherwise. Add the skipped value to FRAGMENT_OUTCOMES and the gates.*.outcome
schema enum, and update computeCompleteness to represent skipped gates in the
completeness contract using the existing schema structure.
---
Outside diff comments:
In `@src/gate-snapshot.ts`:
- Around line 395-418: Update computeSnapshotId and its buildSnapshot call site
to include summary.comparability.adaScanMode in the canonical snapshot address.
Ensure browser and html-snapshot runs produce distinct IDs while preserving the
existing hashing behavior for all other fields.
- Around line 118-132: Update resolveBuildIdentity to treat blank
VERCEL_GIT_COMMIT_SHA and VERCEL_GIT_COMMIT_REF values as unresolved, allowing
gitOrNull to run and returning null when no fallback exists. Preserve non-blank
environment values and the existing build identity fields.
---
Nitpick comments:
In `@src/__tests__/gate-snapshot.test.ts`:
- Around line 706-716: Remove the non-exercising test around the hand-built
validation result, or rewrite it to invoke runCli with an injected schema that
rejects the snapshot. Ensure the test drives the actual snapshot write path and
verifies that a previously valid snapshot.json remains unchanged.
- Around line 411-423: Update the “emission failure never throws (unwritable
artifact parent)” test to verify that chmodSync actually makes the parent
unwritable before calling emitFragment; skip the test when the permission change
is ineffective, including on Windows or privileged runners, and always restore
the original permissions in cleanup.
- Around line 90-95: Update the temporary-directory setup around tmp() and its
callers, including makeRepo and escape-root creation, to register cleanup with
each test’s t.after hook or a shared after hook. Recursively remove every
created temporary root after the test completes while preserving the existing
realpath-based fixture behavior.
In `@src/__tests__/package-contract.test.ts`:
- Around line 30-37: Memoize the result of packedFiles() so npm pack --dry-run
executes only once per test run; preserve the existing file-path parsing and
return behavior for all callers.
In `@src/gate-snapshot.ts`:
- Line 579: Update buildSnapshot to accept the caller-supplied env object and
use it when invoking resolveBuildIdentity instead of process.env. Update runCli
to pass its env argument into buildSnapshot, preserving the injected environment
consistently for authorization and snapshot identity.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ced17b69-a9b5-4038-aae9-131afbdfdefe
📒 Files selected for processing (13)
.gitignoreREADME.mdpackage.jsonschema/build-snapshot-v1.schema.jsonsrc/__tests__/gate-snapshot.test.tssrc/__tests__/package-contract.test.tssrc/gate-ai-instrumentation-source.tssrc/gate-ai-instrumentation.tssrc/gate-conversion-instrumentation-source.tssrc/gate-seo.tssrc/gate-snapshot.tssrc/snapshot-validate.tssrc/snapshot.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/gate-conversion-instrumentation-source.ts
- package.json
- src/gate-seo.ts
- src/gate-ai-instrumentation-source.ts
- src/gate-ai-instrumentation.ts
- README.md
| "checks": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "object", | ||
| "required": [ | ||
| "name", | ||
| "pass", | ||
| "detail" | ||
| ], | ||
| "additionalProperties": false, | ||
| "properties": { | ||
| "name": { | ||
| "type": "string" | ||
| }, | ||
| "pass": { | ||
| "type": "boolean" | ||
| }, | ||
| "detail": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find check-record literals passed to the fragment recorder and inspect their fields.
set -euo pipefail
fd -e ts . src --exec rg -n -C6 '\.check\(|\.checks\(' {} \;
printf '%s\n' '--- CheckRecord definition ---'
ast-grep run --pattern 'export type CheckRecord = $$$' --lang typescript src || true
rg -n -C6 'CheckRecord' --type=ts srcRepository: drjliddy-max/build-websites-tools
Length of output: 5012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- snapshot validator ---'
cat -n src/snapshot-validate.ts | sed -n '90,145p'
printf '%s\n' '--- snapshot merge ---'
cat -n src/gate-snapshot.ts | sed -n '205,245p'
printf '%s\n' '--- fragment recorder and types ---'
rg -n -C8 'type CheckRecord|interface CheckRecord|checks\(|check\(' src --type=ts
printf '%s\n' '--- schema check definition ---'
cat -n schema/build-snapshot-v1.schema.json | sed -n '195,240p'
printf '%s\n' '--- CLI validation and write path ---'
rg -n -C8 'validateFragment|mergeFragments|runCli|writeFile|snapshot' src/snapshot-validate.ts src/gate-snapshot.ts src --type=ts | head -n 260Repository: drjliddy-max/build-websites-tools
Length of output: 35522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- fragment validation and emission ---'
rg -n -C12 'validateFragment\(|kind: "malformed"|malformed|emitFragment|beginFragment' src/snapshot.ts src/gate-snapshot.ts
printf '%s\n' '--- build and CLI outcome path ---'
cat -n src/gate-snapshot.ts | sed -n '503,650p'
rg -n -C10 'validateSnapshotDocument|return 1|process.exitCode|snapshot.json|writeConfinedFile' src/gate-snapshot.ts
printf '%s\n' '--- tests for schema/fragment contract and extra fields ---'
rg -n -C8 'additionalProperties|unknown.*field|extra.*field|check.*field|validateFragment|malformed fragment' src/__tests__ --type=ts
printf '%s\n' '--- deterministic source/schema verifier ---'
python3 - <<'PY'
import json
from pathlib import Path
schema = json.loads(Path("schema/build-snapshot-v1.schema.json").read_text())
check_schema = schema["properties"]["gates"]["additionalProperties"]["properties"]["checks"]["items"]
print("schema_check_additionalProperties:", check_schema.get("additionalProperties"))
print("schema_check_properties:", sorted(check_schema.get("properties", {})))
source = Path("src/snapshot-validate.ts").read_text()
block = source[source.index("f.checks.forEach"):source.index("f.checks.forEach") + 800]
print("fragment_check_block_has_unknown_field_check:",
"Object.keys" in block or "hasOwn" in block or "unknown" in block)
print("fragment_check_block_checks_required_fields:",
all(x in block for x in ("check.name", "check.pass", "check.detail")))
merge = Path("src/gate-snapshot.ts").read_text()
merge_block = merge[merge.index("gates[gate] = {", merge.index("for (const gate of expectedGates")):
merge.index("gates[gate] = {", merge.index("for (const gate of expectedGates")) + 500]
print("merge_copies_checks_verbatim:", "checks: frag.fragment.checks" in merge_block)
# Report object literals passed to check/checks in the source using a conservative
# lexical scan. This is only a call-site inventory, not a TypeScript parser.
for p in sorted(Path("src").glob("*.ts")):
text = p.read_text()
if ".check({" in text or ".checks(" in text:
print("call_site_file:", p)
PYRepository: drjliddy-max/build-websites-tools
Length of output: 34575
Reject unknown fields in check records before merging. validateFragment checks only name, pass, and detail, while the schema rejects extra fields. An extra runtime field is copied into the merged snapshot, causing final validation to fail. runCli then returns 1 without replacing snapshot.json. Reject unknown check fields in validateFragment, or remove "additionalProperties": false.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@schema/build-snapshot-v1.schema.json` around lines 208 - 229, Update
validateFragment to reject check records containing fields other than name,
pass, and detail, matching the checks.items schema and preventing unknown fields
from reaching the merged snapshot. Preserve validation of the existing required
fields and ensure runCli handles the rejected fragment without producing an
invalid snapshot.
| test("a real gate subprocess keeps its own exit status when snapshots cannot be written", () => { | ||
| const repo = makeRepo(); | ||
| const parent = path.join(repo, ARTIFACT_DIR_SEGMENTS[0]); | ||
| mkdirSync(parent, { recursive: true }); | ||
| chmodSync(parent, 0o500); | ||
| let status = -1; | ||
| try { | ||
| execFileSync(process.execPath, [path.join(PKG_ROOT, "bin", "gate-sitemap-source.mjs")], { | ||
| cwd: PKG_ROOT, | ||
| env: { ...process.env, [SNAPSHOT_ENABLED_ENV]: "1" }, | ||
| stdio: "pipe", | ||
| }); | ||
| status = 0; | ||
| } catch (err) { | ||
| status = (err as { status?: number }).status ?? -1; | ||
| } finally { | ||
| chmodSync(parent, 0o755); | ||
| } | ||
| assert.equal(status, 0, "a snapshot problem must never change a gate's exit status"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The test does not create the failure condition it claims, and it writes into the tools repository.
The test makes <repo>/.build-websites-tools read-only, then runs the gate subprocess with cwd: PKG_ROOT. resolveArtifactRoot derives the artifact root from the git toplevel of the working directory, so the subprocess resolves PKG_ROOT/.build-websites-tools, not the chmod-ed fixture directory. Two consequences follow:
- The snapshot write does not fail, so the assertion
status === 0passes for the wrong reason. - With
GATE_SNAPSHOT_ENABLED=1andcwd: PKG_ROOT, the gate emits a real fragment into the checked-out tools repository. That is a side effect outside the fixture.
Run the subprocess with cwd: repo so the chmod-ed directory is the one the gate resolves.
🔧 Proposed fix
execFileSync(process.execPath, [path.join(PKG_ROOT, "bin", "gate-sitemap-source.mjs")], {
- cwd: PKG_ROOT,
+ cwd: repo,
env: { ...process.env, [SNAPSHOT_ENABLED_ENV]: "1" },
stdio: "pipe",
});Note that gate-sitemap-source must still exit 0 inside the fixture repository. If it does not, seed the fixture accordingly or assert the gate's own status instead.
As per coding guidelines: "Do not introduce gate behavior that depends on filesystem paths outside the consuming site's working directory".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("a real gate subprocess keeps its own exit status when snapshots cannot be written", () => { | |
| const repo = makeRepo(); | |
| const parent = path.join(repo, ARTIFACT_DIR_SEGMENTS[0]); | |
| mkdirSync(parent, { recursive: true }); | |
| chmodSync(parent, 0o500); | |
| let status = -1; | |
| try { | |
| execFileSync(process.execPath, [path.join(PKG_ROOT, "bin", "gate-sitemap-source.mjs")], { | |
| cwd: PKG_ROOT, | |
| env: { ...process.env, [SNAPSHOT_ENABLED_ENV]: "1" }, | |
| stdio: "pipe", | |
| }); | |
| status = 0; | |
| } catch (err) { | |
| status = (err as { status?: number }).status ?? -1; | |
| } finally { | |
| chmodSync(parent, 0o755); | |
| } | |
| assert.equal(status, 0, "a snapshot problem must never change a gate's exit status"); | |
| }); | |
| test("a real gate subprocess keeps its own exit status when snapshots cannot be written", () => { | |
| const repo = makeRepo(); | |
| const parent = path.join(repo, ARTIFACT_DIR_SEGMENTS[0]); | |
| mkdirSync(parent, { recursive: true }); | |
| chmodSync(parent, 0o500); | |
| let status = -1; | |
| try { | |
| execFileSync(process.execPath, [path.join(PKG_ROOT, "bin", "gate-sitemap-source.mjs")], { | |
| cwd: repo, | |
| env: { ...process.env, [SNAPSHOT_ENABLED_ENV]: "1" }, | |
| stdio: "pipe", | |
| }); | |
| status = 0; | |
| } catch (err) { | |
| status = (err as { status?: number }).status ?? -1; | |
| } finally { | |
| chmodSync(parent, 0o755); | |
| } | |
| assert.equal(status, 0, "a snapshot problem must never change a gate's exit status"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/gate-snapshot.test.ts` around lines 425 - 444, Update the
subprocess invocation in the test “a real gate subprocess keeps its own exit
status when snapshots cannot be written” to use cwd: repo instead of PKG_ROOT,
ensuring resolveArtifactRoot targets the chmod-ed fixture directory and no
artifacts are written to the tools repository. Preserve the assertion that the
gate exits 0; if the fixture lacks required inputs, seed it appropriately rather
than changing gate behavior.
Source: Coding guidelines
| execFileSync("npm", ["install", "--no-audit", "--no-fund", "--loglevel", "error", tarball], { | ||
| cwd: consumer, | ||
| stdio: "ignore", | ||
| timeout: 150_000, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Report runtime dependencies of the package under test.
fd -t f -d 1 'package.json' | xargs -I{} jq '{name, version, dependencies, peerDependencies, optionalDependencies, bundleDependencies}' {}Repository: drjliddy-max/build-websites-tools
Length of output: 517
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- package contract test ---'
sed -n '1,210p' src/__tests__/package-contract.test.ts
printf '%s\n' '--- package manifests and lockfiles ---'
git ls-files '*package.json' '*package-lock.json' '*npm-shrinkwrap.json' '*yarn.lock' '*pnpm-lock.yaml' \
| sortRepository: drjliddy-max/build-websites-tools
Length of output: 8319
Document the network requirement for the installed-package test.
build-websites-tools declares runtime dependencies, including playwright and jsdom. npm install <tarball> must resolve these dependencies from the registry or npm cache. Offline or network-restricted runners can fail unless the dependencies are pre-cached or installed with --offline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/package-contract.test.ts` around lines 139 - 143, Document the
network or npm-cache requirement for the package installation exercised by the
package-contract test around the execFileSync call. State that installing the
tarball resolves runtime dependencies such as playwright and jsdom from the
registry or cache, and note that offline runners must pre-cache them or use
--offline.
| // Output validates against the schema shipped INSIDE the installed package. | ||
| assert.equal(produced.schemaVersion, schema.properties.schemaVersion.const); | ||
| assert.equal(produced.completeness.status, "complete"); | ||
| assert.equal(produced.completeness.reason, null); | ||
| assert.match(produced.snapshotId, new RegExp(schema.properties.snapshotId.pattern)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the installed package ships snapshot-validate and that the runner can load .ts from node_modules.
fd -t f 'package.json' -d 1 | head -1 | xargs -I{} jq '{files, exports, main, type, scripts}' {}
fd -t f 'snapshot-validate.ts' srcRepository: drjliddy-max/build-websites-tools
Length of output: 1022
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- test context ---'
sed -n '150,220p' src/__tests__/package-contract.test.ts
printf '%s\n' '--- validator ---'
cat -n src/snapshot-validate.ts
printf '%s\n' '--- schema references and validator usage ---'
rg -n "validateAgainstSchema|snapshot-validate|schemaVersion|snapshotId" src package.json tsconfig.json
printf '%s\n' '--- package metadata ---'
cat package.jsonRepository: drjliddy-max/build-websites-tools
Length of output: 19048
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- package-contract imports and test declaration ---'
sed -n '1,90p' src/__tests__/package-contract.test.ts
rg -n -A4 -B4 'test\\(|describe\\(' src/__tests__/package-contract.test.ts
printf '%s\n' '--- validator imports ---'
sed -n '1,90p' src/gate-snapshot.ts
printf '%s\n' '--- schema loading and final validation ---'
sed -n '460,515p' src/gate-snapshot.ts
sed -n '590,645p' src/gate-snapshot.ts
printf '%s\n' '--- package-contract references to installed source ---'
rg -n -C3 'installed|src/|pathToFileURL|execFileSync|package-contract' src/__tests__/package-contract.test.tsRepository: drjliddy-max/build-websites-tools
Length of output: 3562
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- test imports and callback ---'
sed -n '1,90p' src/__tests__/package-contract.test.ts
rg -n -A5 -B3 'test\\(|describe\\(' src/__tests__/package-contract.test.ts
printf '%s\n' '--- dynamic RegExp checks and analysis configuration ---'
rg -n -S 'new RegExp|RegExp\\(|eslint|semgrep|codeql|static.?analysis|security' . \
-g '!package-lock.json' -g '!pnpm-lock.yaml' -g '!yarn.lock'
printf '%s\n' '--- installed-source path assumptions ---'
rg -n -C4 'snapshot-validate|installed|package.json|npm pack|npm install|npm install|tsx' src/__tests__ package.jsonRepository: drjliddy-max/build-websites-tools
Length of output: 3542
🌐 Web query:
tsx Node.js loader import TypeScript files from node_modules node --import tsx
💡 Result:
When using tsx via node --import tsx (or node --loader tsx for older Node.js versions), Node.js generally refuses to process TypeScript files located inside node_modules [1][2]. By design, Node.js enforces this restriction to prevent package authors from publishing packages written in raw TypeScript, which would otherwise force consumers to perform type-stripping on dependencies, potentially impacting performance and reliability [3]. When Node.js encounters a .ts file inside node_modules, it will typically throw an error (such as ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING in newer versions) [3]. Key points regarding this behavior: 1. Restriction: Node.js explicitly blocks type-stripping for TypeScript files in node_modules to discourage the practice of publishing uncompiled TypeScript code [1][2][3]. 2. Alternative/Future Support: There are ongoing developments, such as the experimental flag --experimental-strip-types-in-node-modules-with-declarations, which aims to allow execution of .ts files in node_modules only when a co-located .d.ts declaration file is present [3]. 3. Recommended Practice: For dependencies, the standard approach is to use packages that have already been compiled to JavaScript (.js, .mjs, or .cjs) and include corresponding type declaration files (.d.ts). If you are attempting to use tsx to run your own application code, it will work for files in your project directory (outside node_modules) [4][5]. However, if you have local packages within node_modules (e.g., via npm link or yalc) that contain TypeScript, they will likely be subject to this restriction [3].
Citations:
- 1: https://nodejs.org/api/typescript.html
- 2: https://nodejs.org/docs/latest/api/typescript.html
- 3: module: allow .ts in node_modules when .d.ts is present nodejs/node#63936
- 4: https://github.com/esbuild-kit/tsx/blob/master/docs/dev-api/node-cli.md
- 5: https://github.com/esbuild-kit/tsx/blob/master/docs/dev-api/index.md
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- installation and installed path ---'
sed -n '90,175p' src/__tests__/package-contract.test.ts
printf '%s\n' '--- installed binary loading ---'
cat -n bin/gate-snapshot.mjs
cat -n bin/_run.mjs
printf '%s\n' '--- all test declarations ---'
rg -n -F 'test(' src/__tests__/package-contract.test.tsRepository: drjliddy-max/build-websites-tools
Length of output: 7471
Validate the complete output against the installed schema.
The current assertions can pass structurally invalid output. Make the test callback async, import validateAgainstSchema from the installed src/snapshot-validate.ts through pathToFileURL, and assert result.valid with serialized diagnostics. Guard the completeness accesses so schema changes produce assertion failures instead of TypeError.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 200-200: Do not use variable for regular expressions
Context: new RegExp(schema.properties.snapshotId.pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
[warning] 200-200: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(schema.properties.snapshotId.pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/package-contract.test.ts` around lines 197 - 201, Update the
package-contract test callback to be async, dynamically import
validateAgainstSchema from the installed src/snapshot-validate.ts via
pathToFileURL, and validate the complete produced output against the installed
schema. Assert result.valid and include serialized diagnostics on failure, while
guarding completeness property accesses so schema changes yield assertion
failures rather than TypeError.
| const SUPPORTED_KEYWORDS = new Set([ | ||
| "$schema", | ||
| "$id", | ||
| "title", | ||
| "description", | ||
| "type", | ||
| "const", | ||
| "enum", | ||
| "pattern", | ||
| "format", | ||
| "required", | ||
| "properties", | ||
| "additionalProperties", | ||
| "propertyNames", | ||
| "items", | ||
| "minimum", | ||
| "maximum", | ||
| "minItems", | ||
| "maxItems", | ||
| ]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Two allowlisted keywords are never evaluated.
SUPPORTED_KEYWORDS contains propertyNames and maxItems, but walk implements neither. walk checks minItems at Line 240 and has no maxItems or propertyNames branch. Both keywords therefore pass the unsupported-keyword gate at Lines 193-197 and are then ignored.
The header comment at Lines 24-26 states that an unknown construct is a validation error so the validator cannot silently under-check. An allowlisted-but-unimplemented keyword breaks that guarantee. The shipped schema does not use either keyword today, so no document is mis-validated yet. A later schema edit would be accepted and silently unchecked.
Remove both keywords from the allowlist, or implement them in walk.
🔧 Proposed fix (remove the unimplemented keywords)
"items",
"minimum",
"maximum",
"minItems",
- "maxItems",
]);- "propertyNames",
"items",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const SUPPORTED_KEYWORDS = new Set([ | |
| "$schema", | |
| "$id", | |
| "title", | |
| "description", | |
| "type", | |
| "const", | |
| "enum", | |
| "pattern", | |
| "format", | |
| "required", | |
| "properties", | |
| "additionalProperties", | |
| "propertyNames", | |
| "items", | |
| "minimum", | |
| "maximum", | |
| "minItems", | |
| "maxItems", | |
| ]); | |
| const SUPPORTED_KEYWORDS = new Set([ | |
| "$schema", | |
| "$id", | |
| "title", | |
| "description", | |
| "type", | |
| "const", | |
| "enum", | |
| "pattern", | |
| "format", | |
| "required", | |
| "properties", | |
| "additionalProperties", | |
| "items", | |
| "minimum", | |
| "maximum", | |
| "minItems", | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/snapshot-validate.ts` around lines 156 - 175, Remove propertyNames and
maxItems from the SUPPORTED_KEYWORDS allowlist in snapshot validation, since
walk does not evaluate either keyword. Leave the existing minItems handling and
all implemented keyword entries unchanged so unsupported constructs continue to
trigger validation errors.
| const props: Record<string, Schema> = schema.properties ?? {}; | ||
| for (const [k, v] of Object.entries(obj)) { | ||
| const child = path ? `${path}.${k}` : k; | ||
| if (props[k]) { | ||
| walk(v, props[k], child, errors); | ||
| } else if (schema.additionalProperties === false) { | ||
| errors.push({ path: child, message: "is not allowed by the schema" }); | ||
| } else if ( | ||
| schema.additionalProperties && | ||
| typeof schema.additionalProperties === "object" | ||
| ) { | ||
| walk(v, schema.additionalProperties, child, errors); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
props[k] reads the prototype chain and skips validation.
props comes from schema.properties ?? {}, so it inherits Object.prototype. A data key named toString, constructor, valueOf, or hasOwnProperty makes props[k] return an inherited function. The function is truthy, so walk recurses with that function as the schema. Object.keys(fn) is empty and every keyword branch is skipped, so the value receives no validation at all. The additionalProperties subschema and the additionalProperties: false rejection are both bypassed.
This path is reachable. In src/gate-snapshot.ts Lines 547-575, a fragment filename becomes the gates key. A file named toString.json produces gates["toString"], and schema/build-snapshot-v1.schema.json Lines 178-238 validates gates entries through additionalProperties. That entry is then written to snapshot.json without being checked against the gate-record contract.
Use an own-property check.
🛡️ Proposed fix
const props: Record<string, Schema> = schema.properties ?? {};
for (const [k, v] of Object.entries(obj)) {
const child = path ? `${path}.${k}` : k;
- if (props[k]) {
+ if (Object.prototype.hasOwnProperty.call(props, k)) {
walk(v, props[k], child, errors);
} else if (schema.additionalProperties === false) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const props: Record<string, Schema> = schema.properties ?? {}; | |
| for (const [k, v] of Object.entries(obj)) { | |
| const child = path ? `${path}.${k}` : k; | |
| if (props[k]) { | |
| walk(v, props[k], child, errors); | |
| } else if (schema.additionalProperties === false) { | |
| errors.push({ path: child, message: "is not allowed by the schema" }); | |
| } else if ( | |
| schema.additionalProperties && | |
| typeof schema.additionalProperties === "object" | |
| ) { | |
| walk(v, schema.additionalProperties, child, errors); | |
| } | |
| } | |
| const props: Record<string, Schema> = schema.properties ?? {}; | |
| for (const [k, v] of Object.entries(obj)) { | |
| const child = path ? `${path}.${k}` : k; | |
| if (Object.prototype.hasOwnProperty.call(props, k)) { | |
| walk(v, props[k], child, errors); | |
| } else if (schema.additionalProperties === false) { | |
| errors.push({ path: child, message: "is not allowed by the schema" }); | |
| } else if ( | |
| schema.additionalProperties && | |
| typeof schema.additionalProperties === "object" | |
| ) { | |
| walk(v, schema.additionalProperties, child, errors); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/snapshot-validate.ts` around lines 257 - 270, Update the property lookup
in the object-walking logic around walk so schema.properties keys are accepted
only when they are own properties, not inherited Object.prototype names. Use an
own-property check before calling walk; otherwise preserve the existing
additionalProperties schema validation and additionalProperties: false rejection
behavior.
| process.on("exit", (code) => { | ||
| const errored = provenance.errored === true; | ||
| emitFragment( | ||
| { | ||
| fragmentSchemaVersion: FRAGMENT_SCHEMA_VERSION, | ||
| gate, | ||
| version: toolsVersion(), | ||
| startedAt, | ||
| finishedAt: new Date().toISOString(), | ||
| outcome: errored ? "error" : code === 0 ? "pass" : "fail", | ||
| provenance, | ||
| checks, | ||
| ...(Object.keys(routes).length > 0 ? { routes } : {}), | ||
| }, | ||
| cwd, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
A declared skip is recorded as outcome: "pass".
The handler derives the outcome from the exit code only. Gates that declare a skip return early with exit code 0. src/gate-ai-instrumentation.ts (Lines 418-431) and src/gate-conversion-instrumentation-source.ts (Lines 551-557) set provenance({ skipped: true, skipReason }) and then return, so the recorder writes outcome: "pass".
The merger then counts that gate in gatesRun, and completeness.status can be complete. A consumer reading gates["gate-ai-instrumentation"].outcome === "pass" cannot distinguish a verified contract from a declared exception without knowing gate-specific provenance keys. schema/build-snapshot-v1.schema.json has no skipped outcome and no completeness.skipped array, and additionalProperties: false blocks adding one without a schema change.
Derive a distinct outcome from provenance.skipped, and extend the schema enum and the completeness contract to carry it.
🐛 Proposed direction
process.on("exit", (code) => {
const errored = provenance.errored === true;
+ const skipped = provenance.skipped === true;
emitFragment(
{
fragmentSchemaVersion: FRAGMENT_SCHEMA_VERSION,
gate,
version: toolsVersion(),
startedAt,
finishedAt: new Date().toISOString(),
- outcome: errored ? "error" : code === 0 ? "pass" : "fail",
+ outcome: errored ? "error" : skipped ? "skipped" : code === 0 ? "pass" : "fail",
provenance,
checks,FRAGMENT_OUTCOMES in src/snapshot-validate.ts Line 49, the gates.*.outcome enum in schema/build-snapshot-v1.schema.json Lines 189-194, and computeCompleteness in src/gate-snapshot.ts all need the same addition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/snapshot.ts` around lines 564 - 579, Update the snapshot outcome flow to
recognize declared skips: in the exit handler, derive a distinct skipped outcome
when provenance.skipped is true, while preserving error, pass, and fail behavior
otherwise. Add the skipped value to FRAGMENT_OUTCOMES and the gates.*.outcome
schema enum, and update computeCompleteness to represent skipped gates in the
completeness contract using the existing schema structure.
|
Superseded by #6, which contains the same feature with the review findings addressed and hosted CI passing. Closing this one unmerged; no changes from it were merged. |
1 participant
What this does
The gates already evaluate hundreds of facts per build — HTTP status, canonical, meta robots, title, description, headings, image alt, JSON-LD, sitemap membership,
lastmodtruthfulness, security headers, axe violations — and then discard every one of them at process exit. Only the exit code survives.That is why no site accumulates a technical baseline, and why "what did this page look like before we changed it" can only be answered by whoever happened to write it down. It isn't a discipline failure. It's a missing write call.
This is that write call.
gate-snapshotis not a gate. It never fails a build and always exits0.Architecture
Every gate runs as an isolated child process (
bin/_run.mjsspawns tsx), so no gate can see another's results. Gates write per-gate fragments; a merger binary combines them into one document.One design decision worth reviewing. Gates terminate through many paths —
gate-adaalone callsprocess.exit()from three places, and several gates setprocess.exitCodeand fall through.process.exit()does not run pendingfinallyblocks, so a finalizer intry/finallywould have missed exactly the failures most worth recording.Instead
beginFragment()registers a singleprocess.on("exit")handler. It fires on every path including the top-level catch, and derivesoutcomefrom the real exit code rather than a flag someone remembered to set. Each gate integrates with one line at the top ofmain()and cannot forget an exit path.Invariants (each has a test)
GATE_SNAPSHOT_DIRis set. Proven end-to-end, not just unit-tested: a real gate against a real consumer with no env var produced identical output, exit0, and zero files anywhere.not_run— neverpass, never omitted. A malformed fragment iserror+malformed, never silently dropped.NOT_COMPARABLE. This is the one that would have quietly corrupted the evidence:gate-ada'shtml-snapshotfallback cannot evaluatecolor-contrast, so it legitimately reports fewer violations than browser mode. Comparing across modes reads measurement loss as improvement.scanModeis recorded and the comparison is refused. Same for local vs production.gate-ai-instrumentationembeds a liveG-XXXXXXmeasurement ID in its consent-gated exception message, and that is redacted automatically. Env vars by name only;process.envis never iterated. axe records rule id / impact / node count, nevernode.htmlornode.target(selectors embed customer content on a real site).JSON.stringifyturnsNaN/Infinityintonull, and anullreads as a measured absence. Real0andfalseare preserved. Same reasoning as the conversion-relay scalar guard.Identity contract
snapshotIdis content-addressed and deliberately excludescapturedAt, so two merges of the same build are identical and a future ingestion endpoint can treat a re-POST as a no-op.gateConfigHashcovers measurement scope only (routes, expected gates, scope-affecting config), so it stays stable across runs and moves only when what is measured moves.The
&&chaingate:allchains with&&so a failing gate stops the chain — correct, a failed gate must stop a deploy. But it also means a merger placed at the end never runs on exactly the builds whose state is most worth recording.The merger is built to be invoked in an always-run step (CI
if: always(), or manually after a failed chain). Gates that never ran come back asnot_run. The README documents the CI pattern.Verification
bwt-sample-siterepo (output written outside the repo, consumer left untouched). Scanned clean of secret shapes and absolute paths; assertedpartialwith the four unrun gates correctlynot_run.Versioning
Branched from
b4b0c12(then v0.11.3), deliberately not from the concurrent conversion-contract lane, so the two stayed independent. That lane has since merged and released v0.12.0, so this new gate takes the next minor: v0.13.0. Rebased ontoorigin/main(eb0b674); README andpackage.jsonauto-merged cleanly and the combined suite is green.Scope
Phase 1 only. Deliberately not included: Phase 2 typed per-route facts, Site Monitor ingestion, the
build_snapshotstable or any DDL, the diff UI, and fleet rollout. No consumer repository was touched.Review focus
process.on("exit")recorder — is derivingoutcomefrom the exit code the right call, or should gates set it explicitly?src/snapshot.ts— anything missing?expectedGatesFromScripts()(deriving expected gates from the site's owngate:*scripts) is the right source of truth.Rollout plan:
docs/GATE_SNAPSHOT_ROLLOUT.md. Note the canary (bwt-sample-site) is on v0.9.0, so that step is a four-version migration, not a bump.Summary by CodeRabbit
gate-snapshotcommand that generates validated build snapshots from gate results.