feat(observability-map): static observability scorer for webapp route entry points - #4455
Open
1stvamp wants to merge 106 commits into
Open
feat(observability-map): static observability scorer for webapp route entry points#44551stvamp wants to merge 106 commits into
1stvamp wants to merge 106 commits into
Conversation
|
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
Observability mapNothing in this pull request moves the report any more. The findings an earlier push reported are gone. Report only, nothing here gates the merge. The rules and their reasons: internal-packages/observability-map/README.md. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1stvamp
added a commit
that referenced
this pull request
Aug 2, 2026
CodeRabbit round on #4455, four findings in the mutation corpus. The baseline scan ran in the describe callback body, which Vitest executes during collection, where the suite's timeout option does not apply and a throw has no test name to attach to. It now runs in beforeAll with its own timeout. Collection of the enabled file drops from 9.7s to 3.7s, and the full corpus passes in 282s with no --testTimeout flag. readTree filtered with an inline copy of the scanner's file predicate, so the corpus could materialize files scanDirectory never reads and still count them towards the anti-vacuity thresholds. It uses the exported isScannableFile now. No behaviour change today: the two predicates were identical. The additive-coverage assertion reads no route tree and cost nothing, so gating it behind OBS_MAP_MUTATION_CORPUS only hid a stale list from the run people actually do. Moved next to the registry assertion. merge-comma-expressions is labelled preserving but would have merged a directive prologue into 'use client', foo(), which is no longer a directive. No route has that shape today; the guard and its test are there so the label stays true.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…nalysis
The scanner missed 30% of server entry points in apps/webapp/app/routes.
It found 299 of 427; it now finds all 427 with 0 parse failures.
- detect named export clauses (export { loader }, export { h as loader }),
resolving a local binding back to its declaration for the builder callee
- recurse one level into flat-route directories and key entry points by a
path relative to the scan root, so route.tsx files stay distinct
- count statements for the loader/action bodies only, recursing through
try, if, loop and switch blocks so a try-wrapped body reports its real size
- scope hasTryCatch and calleeNames to the entry-point bodies, leaving
importedNames file-wide
- resolve the initializer callee to the root of a call chain
- throw on parse diagnostics so parseFailures can actually fire
- scan .test.ts route files and exclude .d.ts instead
…r heuristics Follow-up to the adversarial review of the scanner fixes. - follow a call from a loader/action body to a same-file helper, one hop with a cycle guard, so a body that delegates reports the helper's statements, try/catch and callees rather than just the delegation. ph.$.ts goes from 2 statements and hasTryCatch false to 30 and true; 66 entry points gain statements, 6 gain hasTryCatch - only treat a ParseFailureError as a parse failure in scanDirectory and rethrow everything else, so an unreadable file is no longer reported as malformed source, and keep the diagnostic alongside the file name - match a builder handler only at the top of the config object or under methods.<HTTP method>.handler, not by name at any depth - read handler arguments from the root call of a builder chain only, so a callback given to a later decorator is not the route body - pin the loosened assertions and add negatives for the new resolution, the handler shapes and the chained builder
Adds error-classification, auth-boundary, request-context and audit-trail, plus the CHECKS registry. Every check is a pure function of an EntryPoint and reads body-scoped evidence only. Two rules differ from the design. error-classification uses hasTryCatch as its gate rather than a regex over ep.source, which is the whole file including the React component; EntryPoint carries no evidence about what a catch does with the error, so the check reports the hand-rolled catch and says it has not been read. request-context looks for an identity resolved in the body rather than grepping the file for identifier names, for the same reason. Both deviations, and the calibration run over the 427 webapp entry points, are written up in the task 5 report.
…ntry points The error-classification and request-context checks could not tell a rethrow from a swallow, or a database call from any method with a common name. Four additive fields, all body scoped through the existing one-hop helper resolution. - catchRethrows and catchBranches: whether a catch clause in the bodies contains a throw, or branches with if, switch or instanceof. Of the 190 routes that catch, 140 do one of those and 50 take one path out - calleeTexts: the full callee path (prisma.organization.findFirst), index aligned with calleeNames, which is unchanged - logCalls: logger.* and log.* calls with their object argument field names and whether the call sits in a catch, so a check can ask whether the failure path logs an identifier No existing field changes value on any of the 427 route entry points.
error-classification now reads catchRethrows and catchBranches instead of the
mere presence of a try. It fails only where every catch in the bodies takes one
way out regardless of what was thrown, which drops the finding count from 130
to 50. The swallow is read before the builder is credited: a swallow inside a
builder-wrapped handler never reaches the builder, and 18 of the 50 are that
shape.
request-context now asks whether a failure-path log names a tenant, using
logCalls with inCatch and the field names. The builder pass is gone, because
the builders log { error, url } at their boundary and the logger only attaches
http context ambiently, so a wrapped route is not attributed either. The check
no longer echoes auth-boundary: 17 entry points of 427 are scored by both, and
they disagree on 10 of those.
auth-boundary and audit-trail are unchanged.
…eration
error-classification cannot tell the deliberate narrow guard, e.g.
try { body = await request.json() } catch { 400 }, from a catch that
swallows the whole handler. Both take one path out.
catchesNarrowly is true when an entry point has at least one catch clause
and no try block with a catch holds more than two statements, counted in
the loader/action bodies and the same one-hop helpers as the other fields.
Every catch has to qualify: one broad catch anywhere makes it false, so a
route that guards a JSON.parse and also wraps its handler is still
reported. Two statements lets the guarded operation bind its result and
stops short of the three-statement try that covers a handler.
55 of the 427 route entry points, and 11 of the 32 error-classification
failures, all eleven hand-read as the deliberate idiom. No existing field
changes value.
… nothing Applicability keyed off the presence of a failure-path log, so a route that kept its errors and recorded nothing was not-applicable rather than reported, and deleting a log line took a route out of the report. Every non-trivial entry point is now judged: no catch at all passes, since the error reaches the central handler, and a catch has to name whose failure it was. 87 of the 169 failures are routes that record nothing, which is what the old gate was hiding. Verified over the real tree that removing logging cannot help: re-running all four checks against every entry point with log calls deleted, failure-path logs deleted, and log fields stripped moves 63 verdicts, none of them for the better. error-classification now uses catchesNarrowly to excuse the guard that wraps a single parse. Applied on its own the field also excuses a one-statement try around a service call, which passes the design's own swallow fixture and four findings that were hand-read as real, so the exemption also asks that the body parse something. That clears the nine verbatim request.json guards and keeps the rest: 50 failures become 35.
…p and unmeasured tracking
…see into The rendered fix list opened with three auth-boundary findings and all three were wrong. Two delegate to clearImpersonation, which authenticates and writes an audit row in a file the scanner never opens, and the third is a redirect stub flagged only because its path contains billing. A fail here says the route does privileged work with no guard, which is only supportable when the body is where a guard would have to be. A trivial body cannot hold a visible privileged operation, by the triviality rule's own definition, so either nothing privileged happens or the work sits behind an import along with any guard. Those now report not-applicable with a detail saying the guard could not be verified, rather than failing. Signature checks also count as guards now, which clears the HMAC-authenticated waitpoint callback. Three findings remain and all three are genuinely unauthenticated. request-context stops treating a parse guard as the route taking over its failure path, through the same shared reading of catchesNarrowly that error-classification uses. Re-ran the incentive sweep after the change: 57 verdicts move when logging is removed, none for the better.
Whole-entry catch booleans collapse when a route has a narrow parse guard and a broad handler catch, so a check cannot reason about either. 17 route entry points are in that state. - catches: one CatchEvidence per catch clause in the bodies and the one-hop helpers, carrying narrow, rethrows, branches, guardsParse and the try block statement count - guardsParse reads constructors as well as parse calls, so new URL(referer) is visible without touching calleeTexts, which other checks read - a try/finally now yields an empty catches list. hasTryCatch keeps its meaning, a try appears, so ask catches.length whether anything is caught - catchRethrows, catchBranches and catchesNarrowly are now derived from the list and keep their values on all 427 route entry points 242 catch clauses over 189 entry points, 9 of them swallowing outright. Clears all three false positives at the top of the report.
Both checks now read EntryPoint.catches instead of the aggregate booleans, so an entry point is only as good as its worst catch. 39 routes have more than one catch and 17 mix a narrow guard with a broad handler, and a single well-behaved catch used to speak for the swallow beside it. Neither check reads hasTryCatch any more. A try/finally leaves it true with no catch clause at all, which is what put runs-replication.status at the top of the first rendered fix list; the question is now catches.length. A parse guard is recognised when it covers less than half the body, which keeps otel.v1.logs reported, where the catch covers 15 of 18 statements and merely contains a request.json. The narrow limb of the proposed rule is left out. A one-statement try around an awaited service call is as narrow as one around a parse. Taking it clears eleven more routes and reading all eleven says six are real, including a silent run cancellation and two credential paths that report a database failure to the browser as a 400 with the internal message in it. FIX FIRST now reads account.tokens, api.v1.authorization-code and api.v1.token, all three genuine. Global score 83.
… smoke Adds pnpm run map (repo root and package script), single-entry inspection mode, and index.ts exports. The routes directory now resolves against the repo root found by walking up to pnpm-workspace.yaml, not process.cwd(), so the CLI works from both the repo root and the package directory. Single-entry mode notes when an entry has no applicable scored checks rather than printing a bare 100/100. Gitignores the generated observability-map.json artifact.
…likes Two catch-evidence fields matched shapes that resemble the thing they detect, which excused catches the checks exist to find. - guardsParse took any new X(), so new BranchesPresenter() or new Set() excused a catch over ordinary work. It now needs a parsing constructor, URL, URLSearchParams or RegExp, chosen from what the route tree actually constructs inside try blocks. 60 of 242 clauses change, 141 true to 81 - branches took an instanceof anywhere in the clause, including the error instanceof Error ? error.message : String(error) idiom, which words a message rather than picking a path. It now needs an if, a switch, or a conditional that is the whole return or throw. 29 of 242 clauses change, 134 true to 105. All 33 bare instanceof uses in the tree are the formatting idiom Clauses with no evidence at all go from 9 to 37. error-classification will need recalibrating: on this evidence it reports 64 routes rather than 28, and nothing it reported before stops being reported.
The build config had no include and no rootDir, so tsc inferred the package root because vitest.config.ts happened to be inside the compilation. That is the only reason dist/src/index.js landed where the package's main points, and excluding the config would have silently moved the entry point. Scope the build to src and pin rootDir so the layout is intentional. vitest was resolving from the root workspace by hoisting despite being the test runner and supplying the global types. Declare it at the version the other internal packages use.
Emptying every catch clause in the tree scored it 100. Both scored checks passed on the single fact that a route has no catch: error-classification credited it as propagating to the global handler, request-context treated it as having handed its failures over. So the gradient rewarded deleting error handling, and 222 of 412 entries scored 100 on that one shared fact. error-classification now reports not-applicable for a route with no catch, since there is no classification decision to judge, and no longer credits a builder wrapper for error handling the route does not do. request-context fails it instead: the global handler carries requestId, path, host and method and no tenant, so such a route genuinely cannot name whose request broke. Excusing it would reinstate the perverse incentive. Score falls from 76 to 22, which is the honest reading. Deleting all error handling now takes it to 7 rather than 100. The two checks decorrelate: kappa on error-classification against request-context moves from +0.231 to -0.032, and the other two pairs stay near zero.
1stvamp
force-pushed
the
feat/observability-map
branch
from
August 2, 2026 16:17
52a0a4c to
023bc03
Compare
1stvamp
marked this pull request as ready for review
August 2, 2026 17:13
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…after it
definitelyExits read a bare break or continue as leaving the statement list
wherever it found one, and both target the nearest enclosing construct of their
kind instead. A switch whose clauses all break falls through to the statement
written after it, so cutting that statement made
catch (e) { switch (e.code) { ...break } throw e; } read as a swallow, failing a
route that rethrows with a detail line saying it takes one way out regardless of
what was thrown. Same for a do body that breaks or continues.
definitelyExits now carries which bare jumps escape at that point in the
recursion: a switch clause drops break and inherits continue (continue targets
an enclosing loop, which the switch cannot be, so dropping it would stop a
genuinely dead throw being cut), a do body drops both, and a labelled jump always
counts. reachableStatements wraps its findIndex callback, which was passing an
index where the jumps record now goes.
The real route tree is byte-identical, report and clause evidence both:
nothing in apps/webapp writes this shape today.
dead-throw-after-switch-break is the corpus guard for the other direction, a
clause that returns and also breaks, which still has to be cut.
…wrapped exports
A route whose action is builder-wrapped and whose loader is a plain
export async function loader is judged on the action alone, and the pass detail
reads as a claim about the whole route. Ten routes in the tree mix the two
shapes and one of them is sensitive, so the check runs on exactly one of them.
Hand-read: its plain loader filters on members: { some: { userId } } and is
scoped, which nothing in the check saw.
Known limits only; the check is unchanged.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
catchClauseEvidence raised its exited flag off containsExit, which is true
of a provably dead statement itself, so prepending if (false) { throw e; }
to a deciding clause blinded the walk to the real classification below it:
78 real routes turned pass into a swallow verdict, the same set for all
eleven dead-* corpus spellings. The returns veto had the same defect via
containsReturn, regressing 11 rethrow-only routes from n/a to fail.
The walk's four evidence reads now go through a liveness fold
(literalTruth folding literal guards only) so a dead branch contributes
nothing, while an undecidable guard keeps the containment answer and
refusal stays intact. Real-tree report is byte-identical (global 19,
measured 412); the twelve dead-prepend corpus entries measure falls 0
rises 0 in the mirror direction. New corpus entry dead-if-false-return
covers the returns half at tree scale. containsReturn is deleted: its
only read site is now the live fold.
…ranteed The catch-evidence walk entered only a bare block and a do body, so relocating a clause's own statements inside if (true), a single-default switch, an if/else or a try/finally hid the branch evidence while the returns veto still saw the return: a deciding clause read as a swallow on 83 real routes per corpus entry, the generalisation of the switch-break bug fixed in 87e0822. The walk now enters the positions guaranteed to execute whenever the clause runs: a catchless try's tryBlock, the sole clause of a single-default switch, the then-arm of a keyword-exact if (true), and both arms of an if/else with isolated per-arm states merged by intersection (evidence in one arm only earns nothing). definitelyExits folds the literal true keyword so a trailing dead statement after if (true) { exit } is cut. The entry folds are keyword-exact while the liveness folds stay wide, deliberately: entry grants credit, liveness only withholds blindness. Refactors the walk's flags into a threaded state record (verified byte-identical on the real tree before the entries landed); the finished step is also byte-identical including per-clause evidence over all 427 entry points. The six mechanism-B corpus entries measure falls 0 rises 0. New corpus entry dead-classifier-one-arm pins the intersection: widening it to a union raises 80 routes and turns the entry red.
callbackCatches was a bare count and error-classification failed any route whose only catches were refused by the iteration boundary, on placement alone: wrapping a body in a non-array .map or .filter turned a passing route into a fail, a false accusation on 85 real routes per entry. The owner asked for the trade to be revisited. Refused catches now carry full CatchEvidence, built by the same catchClauseEvidence machinery as an own catch, and the check reads two arms off it: a refused swallow fails whenever nothing the route owns decides (deliberately not conditioned on the route owning no catches, so an own inert rethrow cannot lift a refused swallow out of the verdict, which closes a latent rise in the old code), and a route whose only catches are refused and none swallows sits out at not-applicable, never a pass. The ceiling is pinned at tree scale by the new dead-deciding-map corpus entry: any future crediting of refused catches raises ~261 catchless routes and turns it red. Real tree: global 19 -> 19, zero score or verdict changes, exactly two detail-only changes on the tree's two callback-catch routes, both genuine per-item swallows that keep failing. Mirror measurement: wrap-body-in-non-array-map and -filter at falls 77 rises 0 dropouts 0, every fall error-classification pass -> not-applicable; dead-deciding-map falls 0 rises 0.
…operty The corpus only failed on a score RISE, so it structurally could not catch a false accusation: 19 of 43 preserving entries were lowering 104 real routes' scores and nothing noticed. Every preserving entry now also asserts fallsIn, the mirror of risesIn: no route measured in both runs may score lower, over a comparison population pinned to the whole measured baseline so a shrunken population cannot pass vacuously. Exactly two entries carry a permanent lowers exemption, the reason on the entry itself (wrap-body-in-non-array-map and -filter, mechanism C: relocated deciding catches cap at not-applicable, 77 routes each). An exempted entry must still fall, and every fall must be exactly error-classification pass -> not-applicable with nothing moving to fail; anything else is a new defect hiding under the exemption. Red-capable in both directions: dropping the lowers field fails the entry on its 77 falls, and reverting the mechanism-A fix fails dead-if-false and dead-if-false-return on 78 falls each.
…ncels
A finally that leaves itself by break or continue cancels the try's
completion, so a throw or classifier in that tryBlock never escapes the
clause. The catchless-try walk entry credited it anyway: prepending
do { try { if (e instanceof Error) { throw e; } } finally { break; } }
while (false); to every catch raised 80 routes and took the global from
19 to 27. Entry now requires the finally to contain no escaping jump
(containment, since entry grants credit), and containsLiveWhere folds a
try dead when its finally provably completes abruptly, so the refused
statement cannot blind the classification after it either.
dead-throw-in-cancelled-try in the mutation corpus is the tree-scale
guard; the unit pins cover the break, continue, switch-hosted and
may-break spellings and the no-blinding identity.
…wn deciding catch that may raise
Arm c was ordered off reachable, own catches filtered by guardCanRaise,
so a route owning a real classifying catch that canRaise cannot see (a
destructuring guard) beside a per-item .map swallow was told nothing it
owns decides. canRaise is a whitelist and cannot carry that decision;
the new guardMayRaise is its containment twin, false only for the
provably inert try { 0; }, so the dead classifier dead-classifying-try
prepends still blocks nothing while the real catch does. The pin that
was meant to hold this asserted the absence of a detail string no arm
ever emits; it now asserts the verdict.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Round E, the six Devin threads on PR #4455. auth-scope credited any call handed an object with a caller-id property, so `logger.error("create failed", { userId: user.id })` in a builder-wrapped handler cleared the check for that export. Measured at tree scale: the new corpus entry log-caller-scope-userid raises the two routes auth-scope has ever found, settings.sso and settings.team, both confirmed cross-org exposures. The callee now also has to be something that could plausibly narrow a read. A denylist of sinks (loggers, console, response serializers) rather than an allowlist of query callees: 72 distinct callees earn credit on the real tree, and the allowlist was measured and rejected because it accuses regenerate-api-key, whose helper does members: { some: { userId } } and throws. Real tree unmoved, global stays 19, no route newly accused. Also in this round: - cap the delegated route list in the PR comment at 15, the section the file's own docstring wrongly claimed was bounded by construction. - drop the false "No audit helper exists in the webapp" sentence, and the branch that carried it, since the helper exists and AUDIT_SYMBOLS names it. - reword the README's "merge base" to the tip of the base branch. The checkout is a pull_request default, so the scanned tree is the test merge commit and base.sha is its parent, which makes base.sha the right base and the README the thing that was wrong. - gate the mutation corpus to the package's own paths plus a nightly, instead of four and a half minutes on every route PR. - narrow the obsmap paths filter to routes only, so the package's tests stop running twice on every PR touching it.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… filter `needs: changes` carries an implicit success() that outranks the event test in the `if`, so a failed or skipped filter job silently skipped the nightly mutation corpus and the tree-drift scan stopped without saying so. Adding a status-check function to the `if` drops the implicit success() and lets the event test decide alone. The filter job now runs only for pull requests, the sole path that reads its output. Pull request gating is unchanged.
The tests call String.prototype.matchAll, an ES2020 library feature, while `lib` said ES2019. Typecheck passed anyway because @types/node v24 declares `/// <reference lib="es2020" />`, so the program already contained es2020. No behaviour changes; the declaration now states the requirement instead of resting on a transitive reference.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The suite scans apps/webapp/app, packages/plugins/src, internal-packages/rbac/src and four files under .github/workflows, none of which turbo hashes for this package, so turbo run test replayed a pass recorded before those trees changed. Measured rather than argued: a route file with a syntax error fails the suite under vitest, and the same tree came back FULL TURBO in 301ms with the failure cached away as a success. inputs was tried and rejected rather than assumed unworkable. Turbo 1.x does accept .. in an input glob, and ../../apps/webapp/app/** did bust the cache on a route change, but it replaces the default file set instead of adding to it, so the same config silently dropped this package's own vitest.config.ts from the hash. The $TURBO_DEFAULT$ token that would add rather than replace is turbo 2.x only and matches nothing on 1.10.3. Costs about 23s per run and no CI job pays it: the dedicated workflow calls vitest without turbo, and unit-tests-internal.yml runs cold. Reported by Devin on #4455.
…ring stdout Both scan steps redirected pnpm --filter ... exec stdout into files the renderer JSON.parses. pnpm takes its recursive path under --filter and some versions announce 'Scope: N of M workspace projects' on it; one such line in head.json fails the parse and degrades every run to the stale-report comment, which is a permanent quiet failure rather than a loud one. It does not reproduce on the 10.33.2 the workflow pins, which was checked, so this closes the class rather than a reproduction: the scanner writes its own file and stdout is left to be log output. The -s guard keeps the partial dance honest now the redirect no longer creates the file, so a scanner that exits 0 without writing takes the stale-report branch instead of failing the mv and turning the job red. The render step still captures stdout, since prCommentCli has no --out and a banner there puts a stray line in a markdown comment rather than breaking a parse. Reported by Devin on #4455.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The filter watched apps/webapp/app/routes only, which was narrower than the suite's actual coupling. webappSymbols.test.ts walks all of apps/webapp/app and fails when a guard, sensitive or audit symbol stops resolving, so renaming e.g. requireUserId in app/services/session.server.ts matched the webapp filter and nothing else: no job ran this suite and the break landed on main, or on the next unrelated internal-packages PR. integration.test.ts also asserts on the text of observability-map.yml, which no filter watched at all, so editing the report workflow alone ran nothing. Derived the coupling set from the code rather than from the comment. Outside its own directory the suite reads apps/webapp/app (whole tree for symbols, the route subtree for the scan), packages/plugins/src, internal-packages/rbac/src, and four workflow files. packages/plugins/src and internal-packages/rbac/src stay out: internal already matches packages/** and internal-packages/**, and unit-tests-internal.yml runs the same suite, so listing them here would run it twice. A new test pins that reasoning. Cost, over the last 400 commits on main: 31% touch routes, 52% touch apps/webapp/app, so the job fires on roughly half of PRs instead of roughly a third. It is the cheap one, a single 4x runner with no containers and no database. Reported by Devin on #4455.
…tion The 30s and 60s per-test timeouts on the two real-tree tests were chosen on an idle machine, and the suite also runs inside unit-tests-internal.yml, which executes turbo run test --filter "@internal/*" as twelve concurrent shard processes on one runner. The 30s one does flake under that. Measured on an 8-core box. This file alone at load average 0.9: 6.3-6.4s for the scan, 10.8-11.2s for the sweep, both well above the 1.6-2.6s the old comment claimed. Two batches of twelve concurrent copies on those same 8 cores: 24.2-34.0s for the scan and 27.6-39.7s for the sweep, with one of the first twelve dying on "Test timed out in 30000ms". Twelve processes over 8 cores is 1.5 per core where the 32-vCPU runner is 0.375, so the reproduction is harsher than CI, which is why it is the thing to size against. Both now use one 120s constant, which is 3x the worst contended run measured. 60s was the other candidate and is not enough: the sweep already reached 39.7s. Neither test asserts anything about elapsed time, so the number is a hang detector rather than a performance budget, and the docstring says so. Reported by Devin on #4455.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Every input auth-boundary read was entry-point-wide, so one guarded export spoke for the whole file: calleeNames is the union of both bodies, checkedCallees was too, and usesBuilder was an OR over both initializer callees. A file whose loader called requireUser and whose action called nothing read as guarded in the body, and a createLoaderApiRoute loader authenticated a hand-written action beside it. This is the same defect auth-scope was fixed for a round earlier, in its sibling check. scanFile now splits calleeNames, calleeTexts, checkedCallees, statementCount and hasTryCatch per export, filled from one push site each so the union and the split cannot drift apart. usesBuilder had no other caller and is gone. routeExports is the single enumeration of a file's exports, shared with auth-scope, which had grown its own [loader, action] literal. Triviality had to follow, or the fix trades a false pass for a false accusation: naive per-export attribution moved auth.github.ts and auth.google.ts to fail, both being a one-line redirect-stub loader beside a guarded action that the entry-point-wide rule called non-trivial. isTrivial is now one rule over two views. The per-export view matches the side-effect hints against that export's own callee paths: the whole file is defeatable (the corpus's log-caller-scope-userid puts the word logger in the file and un-excuses the untouched loader) and nothing at all guts the check (five fixtures go from fail to not-applicable, because calleeNames keeps only a call's last segment and prisma.x.findMany reads as findMany). login.mfa's action verifies a TOTP or recovery code, which is a login-surface proof of possession like the verify* guards already listed, so it joins them rather than being accused once its loader stops speaking for it. Real tree unmoved: global 19, 62 auth-boundary applicable, 59 passing, no route changing any check. scan.ts also picks up routeModuleFiles here, shared with the corpus harness, because it sits in the same hunk as the per-export return shape.
mutations.ts entryBodies collected exported function declarations and
exported const identifiers only, so it missed the object binding pattern
(export const { action, loader } = createActionApiRoute(...)), the export
clause (const { action } = builder(...); export { action }), and
export const action = route.action. That is 36 of the tree's 427 entry
points, all of them API routes: every whole-body corpus entry skipped them
while the file count suggested otherwise. The scanner has read all four forms
since early on, so this was the harness lagging it.
No assertion could have noticed. A mutation that reaches fewer routes lowers
the score rather than raising it, which is exactly how the suppress-every-check
omission hid, so the answer is the same: assert the population. admin.tsx is
the one exclusion, named rather than counted, because its handler is a concise
arrow with no block for a block wrapper to wrap.
wrap-body-in-rethrow goes from 391 files with 36 entry points missed to 426
files, 1020 sites, 1 missed. Widening changes no entry's verdict: the full
corpus is 55 passed and 1 expected fail either way, and with the narrow
population the only failure is the new population assertion itself.
readTree now calls routeModuleFiles rather than keeping its own copy of the
directory walk. isScannableFile had already replaced the file half of that
copy; the directory half survived.
…rules Sweeping the package for the defect behind the two review threads: one question answered by two pieces of code, where only one copy gets fixed. Shared: - the scannable-file predicate, copied into integration.test.ts and webappSymbols.test.ts after it was exported to stop mutationCorpus.test.ts copying it - the FIX FIRST filter and sort, byte-identical in terminal.ts and prComment.ts, which already imports five helpers from it; failingIds is now scoredFailures plus a map - normalizeSegment, in the test that validates SENSITIVE_SEGMENTS against the real tree. It was splitting segments with /_+$/, the regex that function's own comment says not to use - the five bare-literal node kinds, written out in literalTruth three lines above the literalValue that already had them Pinned: - contextGap and auditGap, which redo by hand what checkContributions computes generically, on two headline figures with nothing saying they had to agree. Reverting either to a different denominator or numerator now goes red Left alone, with reasons recorded in the sweep report: canRaise vs tryBlockMayThrow, the two exact true-keyword folds, the two comment extractors, the three means, ratio vs globalWithout, and the eight AST helpers mutations.ts keeps its own copies of so the corpus can disagree with the scanner.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… fold
selectsADistinctPath decided whether an if or a switch in a catch clause
made a real classification decision by asking containsExit, a plain
containment walk. Containment is true of an exit that can never run, so
catch (e) { if (e instanceof Error) { if (false) { return null; } }
return json(x, { status: 500 }); } read as a decision while the same
clause without the if read as a swallow: 50 points a route for a
behaviour-preserving mechanical edit. Measured over apps/webapp/app/routes,
that shape took the tree from 19 to 27 and raised 80 of 412 routes.
An earlier wave had already moved catchClauseEvidence's exited flag onto
containsLiveExit for the same eleven dead spellings. The branch predicate
120 lines below it kept the containment read, so this is one rule fixed in
one place and left in its sibling.
The three exit reads now go through containsLiveExit and containsExit is
deleted, so there is one exit read in the file. The property that makes one
helper safe for two callers reading it for opposite purposes is now written
down on containsLiveWhere: it is strictly subtractive against containment,
so it only ever un-blinds the exited flag and only ever withholds a branch
grant. Conservatism is a property of the helper plus what the caller does
with a true, and auditing it at the definition is how this was missed.
Adds dead-armed-instanceof-if to the mutation corpus, additive class, and
dead-conjunction-instanceof-if under KNOWN_GAPS. The second is the sibling
this fix does not close: folding the arm does not fold a dead condition,
and if (e instanceof Error && false) reaches the same grant for the same 80
routes. literalTruth treats && and || as always null on purpose, so closing
it means widening that fold, a different rule with its own measurement.
Recorded and running rather than left to be rediscovered.
Requiring the arm to definitelyExits was measured and rejected: it accuses
admin.api.v1.orgs.$organizationId.environments.staging.ts, which classifies
Prisma's P2002 and rethrows everything else, of taking one way out
regardless of what was thrown. Pinned by 'still credits an arm guarded by a
condition that does not fold'.
The real tree does not move: every route's score and every check's status
are byte-identical before and after, global 19 either way.
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
2 participants
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.
A static observability scorer for the webapp's route entry points, Lighthouse-style. The idea comes from evlog's
mapcommand, but that tool has no Remix adapter and checks for its own logging API, so the idea is ported rather than the tool.It scans all 427 loader/action entry points in
apps/webapp/app/routeswith the TypeScript compiler API and scores each against five checks: error-classification, auth-boundary, auth-scope, request-context and audit-trail. Current output on the real tree is 19/100 over 412 measured entry points.The two findings at the top of the fix list are real:
/auth/ssoand/api/v1/authorization-codemint or exchange credentials unauthenticated, and/_app/orgs/:organizationSlug/settings/teamresolves its org from a URL slug and gates each mutating branch on an RBAC check alone, which perapps/webapp/CLAUDE.mdis not the tenant floor on self-hosted.Decisions worth knowing, all with the reasoning in the README:
catch (e) { throw e }was worth 50 points a route.try { String(0); }with a deciding catch is a known open hole worth 19 to 44, and it is disclosed rather than quietly excluded.audit-trailandrequest-contextare reported as headline figures rather than one finding repeated hundreds of times. Both still count in full where they should.CI: a report-only job posts a sticky comment when a PR moves the report, and says nothing when it does not. The package's own tests gate through
pr_checks.yml. The diff-scoped merge gate is still deferred until the report has been used in anger.524 tests plus the corpus. No runtime or dependency changes to anything that ships.