fix(ci): make the two rg-based static checks actually run - #2006
Conversation
ripgrep is never installed on ubuntu-latest, so both `rg` assertions in
the Lint & Format job failed with "command not found" (exit 127) on
every run. `if rg ...; then ... fi` cannot distinguish that from "no
matches" (exit 1) — both read as false, so each step silently passed
without its assertion ever executing. The DI-seams check had 7 live
violations it never reported.
Rewrite both against `grep`, which every runner ships, with match/
no-match/error exit codes handled explicitly so a broken scan fails
the lane instead of reading as a pass, plus a zero-tracked-files guard
so a renamed directory can't quietly go uncovered.
The DI-seam pattern also gets narrower to drop two classes of false
positive surfaced by actually running it: `typeof fetch` (fetchImpl?/
fetch? seams inject the one global with no module boundary vi.mock can
intercept; auth-session.ts/cloud-profile.ts/daemon-proxy.ts exercise
the seam directly in their unit tests, while CLI-level tests use
vi.stubGlobal('fetch', ...) where the seam isn't reachable — a
deliberate, exercised seam) and `typeof SOME_CONSTANT` in
SCREAMING_SNAKE_CASE (derives a literal union type from a constant,
e.g. interaction-touch-response.ts's dispatchPath field — not an
injectable seam at all).
Fixes #1976
Size Report
npm unpacked components
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. Top changed packed files
|
|
Not ready. The scans now fail closed correctly, but the DI rule replaces one false-green with a name-based semantic allowlist: any optional |
…r-site one Review on PR #2006 (#1976): the previous revision fixed the exit-code handling but decided which `?: typeof X` matches to ban with a regex that exempted matches by the *spelling* of the typeof target (`typeof fetch` always passed, SCREAMING_SNAKE_CASE targets always passed). That's a name-based semantic allowlist, not ownership: a new, genuinely test-only `typeof fetch` seam anywhere in the tree would have silently passed, while an equally legitimate seam under any other name would still fail. Add scripts/di-seams: a small, tested TypeScript checker that judges each match against an explicit, typed, per-site allowlist (scripts/di-seams/approved.ts) keyed by (file, field name, typeof target) rather than by name. A triple is exempt only because it was individually reviewed and named — never because of how it's spelled — and the gate fails just as hard on a stale approval (one whose triple no longer matches anything, e.g. after a rename) as on an unapproved seam, so the list can't silently drift out of sync with the code it describes. Moves the DI-seams step in ci.yml to run after Setup toolchain (it's no longer a toolchain-free text scan); the Swift trailing-comma check stays where it was.
|
Fair point — the name-based exemption was exactly the failure mode you describe: an unreviewed Pushed 14d1c8d, which replaces it with Tests exercise the specific scenario you flagged directly: the same field/target seam is approved in one file and flagged as a violation in another; an unapproved field name is flagged even when its target is approved; an unapproved target is flagged even when its field name is approved. Since it's a real TypeScript checker now rather than a toolchain-free grep one-liner, I moved the step to run after Generated by Claude Code |
|
…pdir wrapper
CI caught two things the local (dependency-free) run couldn't:
- oxfmt formatting on the two new files.
- scripts/node-test-tmpdir.test.ts's repo-wide audit: every package.json
script that invokes `node --test` directly must route through
scripts/node-test-tmpdir.ts, or a crash/timeout mid-run leaks its
scratch TMPDIR. check:di-seams now does.
- check:gate-manifest: a package.json script that runs `node --test`
must be covered by a registered CHECK_CATALOG gate, or the audit
reports the test suite as run by no lane. Registered 'di-seams' in
scripts/check-affected/{model,checks}.ts and wired the CI step
through run-gate like every other structural guard in this job,
instead of invoking pnpm directly.
Verified locally with node_modules installed: check:di-seams,
check:gate-manifest, check:gate-manifest:test, check:affected:test,
check:layering, check:fallow (scoped to the changed files), format,
lint, and typecheck all pass.
|
Still blocked at |
…canner Review round 2 on PR #2006 (#1976): - findSeamMatches scanned line by line, so a declaration split across lines (`field?:` on one line, `typeof X` on the next) was invisible. Matching now runs against each file's whole source in one pass — `\s` matches a real newline in JavaScript regexes with no extra flag needed — with the line number derived from the match's character offset. - checkSeams keyed approval by (file, field, target) alone, so once one occurrence of a triple was approved, any further occurrence of that same triple anywhere in the file passed too. The key now includes the line the match starts on, so an approval names one specific declaration, not a recurring pattern. approved.ts expands from 5 collapsed entries to the 7 exact sites this closes down to. Added regression tests planting both gaps directly (a cross-line declaration, and a second unreviewed fetchImpl?: typeof fetch at a different line in an already-approved file) and verified both against the real tree with injected violations, restored cleanly afterward. Re-ran the full local gate suite (di-seams, gate-manifest, layering, fallow, format, lint, typecheck) — all green.
|
Both real gaps. Pushed 8d4eac1. Multiline: Duplicate site: approval identity now includes the line the match starts on — Added tests planting exactly the two scenarios you described (a cross-line declaration, and a second Generated by Claude Code |
|
Clean at |
thymikee
added
the
ready-for-human
Merging main (#2002) removed an unused import above the approved dispatchPath?: typeof MAESTRO_COORDINATE_FALLBACK_PATH declaration in interaction-touch-response.ts, shifting it from line 61 to line 60 — exactly the location-specific-approval staleness the gate is designed to catch, just triggered by an unrelated upstream edit rather than a change in this PR. Updated the approved line to match.
thymikee
removed
the
ready-for-human
|
Readiness retracted at |
|
Still blocked at |
…val marker Review round 3 on PR #2006 (#1976): CI proved the round-2 fix's core assumption wrong within one push. Keying approval by (file, line, field, target) made a line number the identity — an unrelated edit anywhere earlier in a file shifts every approval below it, and that's exactly what happened: merging main removed an unused import above the approved dispatchPath declaration, and the gate rejected an unchanged, already-reviewed line. Detection is now AST-based (oxc-parser, the same tool scripts/layering/*.ts already uses) instead of a source-text regex: any `{ optional: true, typeAnnotation: TSTypeQuery }` node — a property signature or a bare parameter — is a candidate, which finds a multiline `field?:\n typeof X` declaration for free instead of needing a special case for it. Approval is a `// di-seam-approved: <reason>` comment immediately above the declaration, matching this repo's own `// fallow-ignore-next-line complexity` convention: the marker precedes what it exempts. approved.ts (the external table) is deleted — there is nothing left to keep in sync, since the approval travels with the code it approves. A second, unmarked seam under the same field/target elsewhere still fails; reordering unrelated code around an approved declaration no longer touches it. Added the marker to the 7 real approved sites (fetch-global injection seams in auth-session.ts/cloud-profile.ts/daemon-proxy.ts; the literal-type-derivation false positive in interaction-touch-response.ts) and regression tests proving: a cross-line declaration is still found, a second unmarked occurrence of an approved field/target pair still fails, and an unrelated insertion above an approved declaration no longer breaks it. Verified against the real tree with an injected multi-line unrelated insertion before an approved site — still green. Re-ran the full local gate suite (di-seams, gate-manifest, layering, fallow, format, lint, typecheck, auth-session unit tests) — all green.
|
Agreed — a line number was never a real identity. Pushed ce5fbcd.
Verified: a second, unmarked occurrence of an approved field/target pair still fails; an unrelated multi-line insertion above an approved declaration — the exact shape of what broke last round — leaves it passing (tested against the real tree by injecting one, confirming green, and restoring cleanly). Multiline and cross-file regression tests from the last round still pass. Re-ran the full local gate suite (di-seams, gate-manifest, layering, fallow, format, lint, typecheck, auth-session unit tests) — all green. Generated by Claude Code |
|
The code-local AST marker fixes the line-drift design at |
Review round 4 on PR #2006 (#1976): approvalReason() returned '' (not null) for a bare `// di-seam-approved:` comment with nothing after it, and checkSeams() only filtered out null, so an empty marker silently approved a seam with zero justification — exactly the kind of unreviewed bypass this gate exists to prevent. approvalReason() now returns null when the joined reason text is empty after trimming, so a bare or whitespace-only marker is treated the same as no marker at all. Added tests for both the model-level behavior and the end-to-end checkSeams() result, plus verified against the real tree by injecting a bare-marker declaration and confirming it's flagged, then restored cleanly.
|
Real gap. Pushed 135a144.
Added both a model-level test ( Also merged Generated by Claude Code |
|
Ready for human review at |
thymikee
added
the
ready-for-human
2 participants
Summary
ripgrepis never installed onubuntu-latest, so bothrgassertions in theLint & Formatjob (Disallow trailing commas before closing parenthesis in SwiftandFail if test-only DI seams reappear in production code) failed withrg: command not found(exit 127) on every run.if rg ...; then ... ficannot distinguish "command not found" from "no matches" (exit 1) — both read as false, so each step silently passed without its assertion ever executing.grep, which every runner ships, with match/no-match/error exit codes handled explicitly (0 = violation found → fail, 1 = clean → pass, anything else = broken scan → fail loudly instead of reading as a pass) plus a zero-tracked-files guard so a future renamed/deleted directory can't silently make the check a no-op.main, none of which are the test-only DI seams the rule was written to ban, so the pattern is narrowed to drop both false-positive classes:typeof fetch: thefetchImpl?/fetch?seams insrc/cli/auth-session.ts,src/cli/connection/cloud-profile.ts, andsrc/remote/daemon-proxy.tsinject the one global with no module boundaryvi.mockcan intercept. Their own unit tests exercise the seam directly for exact per-call assertions, while the CLI-level tests (cloud-connect-auth.test.ts,cloud-connect-profile.test.ts) usevi.stubGlobal('fetch', ...)at a layer where the seam isn't reachable — a deliberate, exercised seam, not a leftover one.typeof SOME_CONSTANTinSCREAMING_SNAKE_CASE: e.g.dispatchPath?: typeof MAESTRO_COORDINATE_FALLBACK_PATHinsrc/daemon/handlers/interaction-touch-response.tsderives a literal union type from a constant — not an injectable seam at all, just a syntax coincidence the old pattern happened to match.Verified the rewritten scripts against the real repo tree and against synthetic injected violations (both a real trailing-comma-before-
)Swift case and a realdispatch?: typeof someFnseam), executed with the exactbash --noprofile --norc -eo pipefailinvocation GitHub Actions uses forrun:steps, confirming both checks pass on the current tree and correctly fail with diagnostic output when a genuine violation is present.Fixes #1976
Test plan
run:blocks from the workflow and executed them locally withbash --noprofile --norc -eo pipefail(GitHub Actions' actual bash invocation) against the current repo tree — both pass (exit 0).)violation into a tracked.swiftfile — the Swift check fails (exit 1) with the offending file and matched text, then confirmed the file was restored and the tree is clean.dispatch?: typeof someFn-shaped DI seam into a trackedsrc/file — the DI-seams check fails (exit 1) with file/line, then confirmed the file was restored and the tree is clean.fetchThing?: typeof fetchThingImpl(not excluded by thefetch-specific lookahead) while excluding all 7 currenttypeof fetch/typeof MAESTRO_COORDINATE_FALLBACK_PATHmatches onmain..github/workflows/ci.ymlparses as valid YAML./code-reviewin the session); no correctness issues found — the one findings besides a minor diagnostics nit (already addressed: switched Swift-check output from filenames-only to filename+matched-text) were left as-is as a deliberate, low-severity styling call.Generated by Claude Code