Skip to content

feat(web): add fact-drift CI gate#3481

Merged
Hmbown merged 1 commit into
mainfrom
codex/finish-pr-3473
Jun 23, 2026
Merged

feat(web): add fact-drift CI gate#3481
Hmbown merged 1 commit into
mainfrom
codex/finish-pr-3473

Conversation

@Hmbown

@Hmbown Hmbown commented Jun 23, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #3415.
Supersedes #3473.

Verification

…ventory (#3415)

Extract shared derivation logic into web/scripts/facts-lib.mjs and
create a check-facts.mjs CI gate that fails when web/lib/facts.generated.ts
is stale relative to the workspace sources.

- facts-lib.mjs: single-source-of-truth derivation shared by
  derive-facts.mjs (prebuild) and check-facts.mjs (gate)
- derive-facts.mjs: slimmed to ~63 lines, imports from facts-lib.mjs
- check-facts.mjs: re-derives facts and compares against committed file;
  exits non-zero when version, providers, crates, sandboxBackends,
  defaultModel, nodeEngines, toolCount, or license have drifted
- web/package.json: add npm run check:facts script
- .github/workflows/web.yml: run check:facts after prebuild in lint job
- web/lib/check-facts.test.ts: 9 vitest tests covering stale version
  (0.8.62 vs 0.8.64), provider drift, multi-field detection, edge cases

Closes #3415
@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown

Claude finished @Hmbown's task in 4m 17s —— View job


Code Review — PR #3481

  • Gather context and understand the request
  • Read changed files
  • Analyze for correctness bugs and logic errors
  • Check provider/model safety invariants
  • Review for reuse/simplification opportunities
  • Assess test coverage
  • Check security concerns
  • Post final review

Overall: The extraction into facts-lib.mjs is the right move and the diffFacts test suite is solid. There is one correctness bug that makes the CI drift gate a no-op, a behavioral divergence between the build-time and runtime sandbox derivation, and a stale guidance string left in facts-drift.ts. No provider-inference safety violations.


[Bug — CI gate is a no-op]

.github/workflows/web.yml lines 36–44:

- name: Generate derived facts
  run: npm run prebuild          # ← OVERWRITES facts.generated.ts
- name: Check facts drift
  run: npm run check:facts       # ← compares freshly-written file to freshly-derived facts

prebuild runs derive-facts.mjs, which regenerates facts.generated.ts from the current workspace and writes it to disk. check:facts then reads that freshly-written file and compares it to the current workspace. They will always match — the committed version is never compared. If a developer bumps Cargo.toml but forgets to run prebuild and commit the updated facts.generated.ts, CI will silently pass.

Fix: swap the two steps so the drift check reads the committed file before prebuild overwrites it.

- name: Check facts drift        # sees the committed file
  run: npm run check:facts
- name: Generate derived facts   # regenerates for tsc
  run: npm run prebuild

Fix this →


[Moderate — deriveSandboxBackends diverges between build-time and runtime]

facts-lib.mjs:46-52 returns all unrecognised sandbox filenames as-is (after the exclusion list):

const map = { seatbelt: "seatbelt (macOS)", landlock: "landlock (Linux)" };
return files.map((f) => map[f] ?? f);   // bwrap, process_hardening, seccomp pass through

facts-drift.ts:126-135 only keeps names that appear in its own map:

return files.filter((n) => map[n]).sort().map((n) => map[n]);  // only seatbelt + landlock

The committed facts.generated.ts therefore has five backends (bwrap, landlock (Linux), process_hardening, seatbelt (macOS), seccomp), but when the Cloudflare cron runs it will derive only two (landlock (Linux), seatbelt (macOS)) and write that smaller list to KV, overriding the build-time value. The website would then show an incomplete sandbox list.

Either align facts-drift.ts to pass through unmapped names, or explicitly exclude bwrap/process_hardening/seccomp in facts-lib.mjs the same way facts-drift.ts implicitly does.


[Minor — stale guidance string in facts-drift.ts:115]

facts-drift.ts line 115 still says:

"Add them to labelMap here AND in web/scripts/derive-facts.mjs …"

Since this PR moved the authoritative map to facts-lib.mjs, that message should now point to web/scripts/facts-lib.mjs. A developer hitting this warning will look in the wrong file.

Fix this →


[Minor — parseCommittedFacts regex is fragile] (check-facts.mjs:44)

const m = src.match(/export const FACTS\s*:\s*\w+\s*=\s*([\s\S]*?);?\s*$/);

This works for the current generated format, but would silently fail to parse if: (a) a second export is added after FACTS, (b) as const appears before the ;, or (c) the type annotation spans more than one word (\w+ won't match RepoFacts & Foo). Since derive-facts.mjs fully controls the output format it's low-risk today, but worth a comment or a more deliberate extraction strategy (e.g., strip the header lines and JSON-parse the remainder).


Good choices

  • Extracting derivation logic into facts-lib.mjs so derive-facts.mjs and check-facts.mjs share one implementation is correct — any future bug in regex or path logic is fixed in one place.
  • The diffFacts test matrix (version, providers, tool count, null-to-value, multi-field, ignored fields) covers the meaningful cases and would have caught regressions.
  • The unmapped-variant warning in deriveProviders ensures a new enum variant in config.rs can't silently drop from the provider list.
  • facts.generated.ts being removed from .gitignore is the right call — without it the committed file can never be verified against the workspace.

@Hmbown Hmbown merged commit 38f9152 into main Jun 23, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v0.8.69: Add a website fact-drift gate for version, providers, and tool inventory

2 participants