check:report-error: flag catches that never reach reportError - #497
Merged
TheGreatAxios merged 9 commits intoAug 30, 2026
Merged
TheGreatAxios merged 9 commits into
TheGreatAxios merged 9 commits into
Conversation
This was referenced Aug 30, 2026
TheGreatAxios
force-pushed
the
cl-7237-enforce-the-reporterror-convention-with-a-check-script
branch
2 times, most recently
from
August 30, 2026 21:45
2f16686 to
ea09503
Compare
AGENTS.md's "Conventions a check enforces" section names this rule but had no check backing it — the fourteen scripts in scripts/checks/ never mention reportError. This walks every catch clause in apps/packages/ workflows with the TypeScript compiler API and fails on one that neither calls reportError from @corbits/error-sink (via its actual import binding, so an aliased import still passes and an unrelated local function of the same name still fails) nor rethrows outside a nested function/class body. Running it against the repo surfaces 280 violations spread across most packages, well beyond the two already-ticketed clusters (CL-7197's bare catch in the workbench SSE stream, CL-7234's onboarding routes/bench- provisioning/credential-planting catches). Those seven catches are opted out here with a report-error-ignore comment naming their ticket, since fixing them collides with the lanes already assigned to them; the other 280 are real findings this check newly surfaces, not something this change silently suppresses.
Wiring this straight into check:structural without a way to record existing debt would either force fixing 280 pre-existing violations in this change (colliding with lanes already assigned to two of the named clusters) or leave check:structural red for the whole team on main. Following check:tool-package-freshness's own precedent for scoping a new invariant to what changed: a finding is checked against scripts/checks/report-error-baseline.txt, a debt ledger (not an allowlist) of every pre-existing violation, generated with `--write- baseline`. A finding not in the baseline always fails. A baselined finding still fails if the current change's diff touches its exact line — not merely its file, so a change that edits one function isn't forced to also clean up unrelated debt elsewhere in the same file (including this very PR's own edits, which only add report-error-ignore comments nowhere near the other debt already in those files). A stale baseline entry with no matching finding anymore also fails, so the ledger can only shrink. The seven catches already tracked on CL-7197 and CL-7234 keep their report-error-ignore comments rather than moving into the baseline — a violation in flight on a named ticket isn't the same kind of debt as the other 280, which have none yet.
…tches CL-7247: expects reportError from @corbits/error-sink on every catch in the OAuth/MCP-connection/credential-probe package that check:report-error flags in the debt ledger. Red against the current implementation.
CL-7247 batch 1: the connections package (OAuth token exchanges, MCP probes and OAuth, PKCE state, and the fixed-registry/tenant/MCP-server connect routes) is the credential/auth/secret-handling surface CL-7247 calls out to fix first, since a hand-rolled log here is where material reportError would redact is most likely to leak. 22 baselined catches addressed: - 18 now call reportError with an operation name and non-secret context (connector/slug/tenant identifiers only — never a token, code, verifier, or client secret, all of which are in scope at several of these sites). - 4 are genuine intentional swallows, marked report-error-ignore: pkce.ts's state-decrypt failure (indistinguishable from malicious probing by design), mcp-probe.ts's and oauth-routes.ts's malformed user-input parses (a paste-in typo, not a system fault), and mcp-oauth-routes.ts's best-effort error-body sniff (the classifier already falls back safely). Shrinks scripts/checks/report-error-baseline.txt by exactly these 22 lines — the debt they tracked is now either fixed or ticketed with its own ignore comment, never bulk-regenerated.
TheGreatAxios
force-pushed
the
cl-7237-enforce-the-reporterror-convention-with-a-check-script
branch
from
August 30, 2026 22:51
ea09503 to
c20a811
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scripts/checks/report-error.ts: walks everycatchclause inapps/,packages/,workflows/with the TypeScript compiler API and fails on one that neither callsreportErrorfrom@corbits/error-sink(matched against the file's actual import binding, so an alias still passes and an unrelated same-named local function still fails) nor rethrows outside a nested function/class body.check:structural.report-error-ignore: <reason>comment naming the ticket.scripts/checks/report-error-baseline.txt, a debt ledger (not an allowlist) of the 280 pre-existing violations found repo-wide, followingcheck:tool-package-freshness's own precedent for scoping a new invariant to what changed rather than retroactively flagging existing state. A finding not in the baseline always fails (new code is held to the rule immediately). A baselined finding still fails if the current diff touches its exact line — not merely its file, so an edit elsewhere in a large file doesn't force cleanup of unrelated debt (this PR's own edits, which only add opt-out comments, are the clearest example). A stale baseline entry with no matching finding fails too, so the ledger can only shrink. Regenerate withbun run scripts/checks/report-error.ts --write-baseline.Why this is green now
check:report-errorpasses cleanly: 161 compliant catches, 7 ticketed opt-outs, 280 baselined pre-existing violations, 0 new findings, 0 stale entries.check:structuralpasses end to end.The 280 baselined violations are not fixed — this PR does not touch any of them. They're a real, disclosed backlog (see the bucketed breakdown posted on CL-7237) that the ticket owner will burn down via a follow-up ticket, ratcheted so no new instance can be added going forward.
Test plan
bun test scripts/checks/test/report-error.test.ts— 25/25 passbun test scripts/— 223 pass, 0 fail (39 DB-backed e2e suites skip withoutDATABASE_URL, expected locally)bun run check:structural— passes end to end (exit 0)bunx prettier --checkon all touched files