Skip to content

fix(ci): make check-cycles honest — real graph walk + cycles ratchet (#15159 G-01/G-02) - #15281

Merged
diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.52from
jonlwheat2-gif:fix/g01-check-cycles-blind-spots
Oct 2, 2026
Merged

diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.52from
jonlwheat2-gif:fix/g01-check-cycles-blind-spots

Conversation

@jonlwheat2-gif

Copy link
Copy Markdown
Contributor

Problem

npm run check:cycles printed:

[cycles] OK - no cycles detected across 450 files in: src/shared/components,
  src/lib/db, src/lib/compliance, open-sse/translator, open-sse/mcp-server

while being structurally unable to see most of the repository. Three independent
blinds, each sufficient on its own:

# Blind spot Consequence
1 defaultRoots was five directories, not the repo src/lib/config, src/app, open-sse/services, open-sse/handlers were never scanned. On a checkout without those five directories it scanned 0 files and still printed OK.
2 Regex matched only import|export … from every dynamic import("…") was invisible
3 if (!specifier.startsWith(".")) continue dropped every @/ and @omniroute/open-sse/ alias edge — i.e. most of the repo's own imports

This is the "gate that lies" class the audit names: a gate that prints green while
the defect is live converts unknown into verified, which is worse than no gate.

The lie had propagated into the docs. docs/architecture/QUALITY_GATES.md:520
cited it as intentional — "the green, curated check:cycles" — to justify
keeping it blocking while check:circular-deps (dpdm) stayed advisory at 91 cycles.

Live casualty

src/lib/db/settings.ts:15 imports ./readCache, which at :102 does
await import("@/lib/db/settings"). check-circular-deps (dpdm, which resolves
tsconfig paths) reports that as a real cycle. This gate could never have found it —
blinds 2 and 3 each independently hide the edge.

Fix

  • DEFAULT_ROOTS is now src + open-sse — 5023 files scanned, was 450.
  • Specifiers come from the TypeScript AST. import("…") counts as a runtime
    edge; type-position typeof import("…") does not. A regex cannot tell those
    apart, and counting erased imports would invent cycles that do not exist at
    runtime. This is why the extraction moved off regex rather than just adding an
    import( branch.
  • tsconfig paths are resolved for @/*, @omniroute/open-sse/* and
    baseUrl-relative specifiers, matching how the code actually resolves.
  • node_modules/build dirs and .d.ts files are skipped.

It finds 14 cycles, not 0. The largest is a 43-file SCC over src/lib/db
containing settings.ts.

Why this is one PR and not two (G-01 + G-02)

G-02 is inseparable. Fourteen pre-existing cycles cannot be fixed inside a gate PR,
and shipping G-01 alone would make npm run check:cycles exit 1 and block the whole
queue — a base-red inherited fix, which Hard Rules #18/#21 forbid.

So the honest measurement lands with the mechanism that makes it shippable:

  • config/quality/quality-baseline.json → metrics.cycles = 14, direction: down
  • npm run check:cycles:ratchet — blocks any regression, count can only fall
  • CI runs the ratcheting variant
  • Burn-down rides with A-01 (chatCore.ts split)

The baseline entry carries the full before/after reasoning inline, including an
explicit warning not to raise the ceiling to make a gate green — that is the exact
inverse of this ratchet.

Verification (release/v3.8.52 @ dbe703a)

Check Result
tests/unit/build/check-cycles-blind-spots.test.ts 21/21 pass
npm run check:cycles:ratchet exit 0, cycles=14
ratchet proven non-vacuous (ceiling lowered 14→13) exit 1, then restored → exit 0
npm run typecheck:core clean
npm run check:docs-all PASS, no new drift
npm run check:tracked-artifacts OK

Scan surface: 450 → 5023 files, 0 → 14 cycles.

The tests are honest about being blind-spot tests

A suite that only asserts "blind spot X is now detected" cannot distinguish a real
fix from a broken harness. So two GATE-SANITY cases pin that the fixture harness
itself detects a plain relative cycle — one of them would have failed outright if
the whole gate stopped working.

Three of the 21 are analyzeCycles unit tests on the newly exported surface, so
the graph walk is testable without a subprocess.

Pre-existing failures (not from this change)

12 failures in tests/unit/build/ (check-workflows,
check-ts7-diagnostics-ratchet, +10 others). Verified identical with
ci.yml reverted to its base value: 39 tests / 37 pass / 2 fail either way.

⚠️ base-red inherited: #15246 — release/v3.8.52 is not green (6 hard failures
in unit, vitest, integration, package-artifact and tarball jobs). None of those
failures originate here and none are fixed here.

Scope

scripts/check/check-cycles.mjs · tests/unit/build/check-cycles-blind-spots.test.ts (new) ·
config/quality/quality-baseline.json · .github/workflows/ci.yml (1 line) ·
package.json (1 script) · AGENTS.md + docs/architecture/QUALITY_GATES.md (the two
places that documented the false green as intentional).

⚠️ Touches AGENTS.md — flagging per the repo's review-focus rule: agent-instruction
surfaces are executed as authority by every AI session, so this needs explicit
operator approval before merge. The edit is two lines in Quick Start, documenting
check:cycles vs check:cycles:ratchet; no behavioural instruction changed.

Refs #15159 (G-01, G-02)

…iegosouzapw#15159 G-01/G-02)

check-cycles.mjs printed "[cycles] OK - no cycles detected across 450 files"
while being structurally unable to see most of the repository. Three
independent blinds:

  1. defaultRoots was five directories, not the repo. src/lib/config,
     src/app, open-sse/services, open-sse/handlers were never scanned —
     and on a checkout without those five directories it scanned 0 files
     and still printed OK.
  2. The specifier regex matched only `import|export … from`, so every
     dynamic `import("…")` was invisible.
  3. `if (!specifier.startsWith(".")) continue` dropped every `@/` and
     `@omniroute/open-sse/` alias edge — i.e. most of the repo's own
     imports.

Live casualty: src/lib/db/settings.ts:15 imports ./readCache, which at
:102 does `await import("@/lib/db/settings")`. check-circular-deps.mjs
(dpdm, which resolves tsconfig paths) reports that as a real cycle; this
gate could never have found it.

A gate that prints green while the defect is live converts "unknown" into
"verified" — worse than no gate. docs/architecture/QUALITY_GATES.md even
cited the false green as intentional ("the green, curated check:cycles")
to justify keeping it blocking.

The fix:

  - DEFAULT_ROOTS is now src + open-sse (5023 files, was 450).
  - Specifiers come from the TypeScript AST, so `import("…")` counts as a
    runtime edge while type-position `typeof import("…")` does not. A
    regex cannot tell those apart, and counting erased imports invents
    cycles that do not exist.
  - tsconfig `paths` are resolved for `@/*`, `@omniroute/open-sse/*` and
    baseUrl-relative specifiers.
  - build/vendor dirs and `.d.ts` files are skipped.

It finds 14 cycles, not 0. Fourteen pre-existing cycles cannot be fixed in
a gate PR, so G-02 lands with it: `metrics.cycles` = 14, direction `down`,
consumed by `--ratchet`. CI now runs `npm run check:cycles:ratchet`, which
blocks any regression and can only fall. Verified non-vacuous by lowering
the ceiling to 13 → exit 1, then restoring → exit 0.

Burn-down rides with A-01 (chatCore.ts split). Do not raise this ceiling
to make a gate green — that is the exact inverse of this ratchet.

Verification (release/v3.8.52 @ dbe703a):
  tests/unit/build/check-cycles-blind-spots.test.ts  21/21 pass
  check:cycles:ratchet                             exit 0, cycles=14
  typecheck:core                                   clean
  check:docs-all                                  PASS (no new drift)
  check:tracked-artifacts                          OK

The 21 tests drive the gate as a subprocess against synthetic trees, with
two GATE-SANITY cases pinning that the harness itself detects a plain
relative cycle — a blind-spot suite that cannot detect a cycle proves
nothing. The tests that import the module cover the exported
analyzeCycles surface directly.

Note: 12 pre-existing failures in tests/unit/build/ (check-workflows,
check-ts7-diagnostics-ratchet, and 10 others) are unrelated to this
change — verified identical with ci.yml reverted to its base value.
@jonlwheat2-gif

Copy link
Copy Markdown
Contributor Author

Ordering note (not a defect in this PR): the lint job will show red on this PR until #15283 (G-05) lands. The base branch carries six stale entries in config/quality/eslint-suppressions.json, which makes npm run lint exit 2 on every PR regardless of its contents. Merging #15283 first clears it. No code in this PR contributes to that failure.

@diegosouzapw
diegosouzapw merged commit 69cb18d into diegosouzapw:release/v3.8.52 Oct 2, 2026
15 of 16 checks passed
woodsonl added a commit to woodsonl/OmniRoute that referenced this pull request Oct 6, 2026
…-closed reality

diegosouzapw#15281 (69cb18d) shipped both the comments and the fail-closed exits in
the same commit: plain mode exits 1 on ANY cycle and --ratchet with a
missing/invalid baseline falls through to exit 1 (main() lines 464-497),
but the header and readBaselineCyclesValue docstring still described
plain mode as advisory with no ceiling. Comment-only; behavior unchanged.
woodsonl added a commit to woodsonl/OmniRoute that referenced this pull request Oct 6, 2026
- README/AGENTS/llm.txt (+ i18n mirrors): 193 to 194 SQL migrations,
  clearing the strict docs-counts drift inherited from release/v3.8.52
- QUALITY_GATES.md + RELEASE_CHECKLIST.md: bare `check:cycles` exits 1
  on any cycle since diegosouzapw#15281 (fail-closed, not advisory); the release
  checklist now runs `check:cycles:ratchet`
rafiknedir9-star pushed a commit to rafiknedir9-star/OmniRoute that referenced this pull request Oct 7, 2026
…ship guard

Upstream diegosouzapw#15281 moved `cycles` from the plain gates to ratchet_gates in quality.yml
but left the guard expecting the old line, so the test failed on the synced tip.
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.

2 participants