feat(observability-map): static observability scorer for webapp route entry points - #4455
Open
1stvamp wants to merge 95 commits into
Open
feat(observability-map): static observability scorer for webapp route entry points#44551stvamp wants to merge 95 commits into
1stvamp wants to merge 95 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.
…a workflow nobody waits for The previous attempt put a test job in observability-map.yml. It was targeted, and it gated nothing: pr_checks.yml's all-checks aggregate needs an explicit list of jobs and cannot see another workflow, so the job would have reported a result no merge was required to wait for. Fixing that needs repository settings neither this branch nor a reviewer can change. Follows the pattern already in the repo instead. The suite moves into unit-tests-observability-map.yml as a workflow_call workflow, pr_checks.yml's changes job grows an obsmap filter covering internal-packages/observability-map/** and apps/webapp/app/routes/**, and a job calls the workflow behind that filter and joins the all-checks needs list. That keeps the cost proportionate, one package rather than eighteen with postgres, clickhouse, redis and electric, while actually gating. The duplicate run in observability-map.yml is gone, since the same suite now runs from pr_checks on the same paths. Adds four text checks over the two workflow files, each of which fails when its half of the wiring is removed: the call, the filter contents, the all-checks entry and the absence of a second run. Whether GitHub agrees is still something only a pull request can answer.
…a name prefix standing in for a guard
Round C: what the tool looks at. Five items, all interlocking through the
sensitive cohort, so they land together.
auth-boundary matched three patterns and all three over-matched. /^(require|
authenticate)/ passed any callee beginning "require": live in the tree,
requireSsoEntitlement is a plan check and it cleared the org SSO settings route.
/Authenticated/ passed resolveAuthenticatedEnv on ten routes, which is a
findFirst by environment id and authenticates nothing, and commitAuthenticatedSession
on six. Both are now an explicit list of the helpers this webapp has, including
the non-throwing getUser/getUserId that invite-accept and the login page act on.
A builder authenticates a request; it does not necessarily scope it.
authorization is optional on all nine and the RBAC gate runs inside
if (authorization), which is the cross-org IDOR class apps/webapp/CLAUDE.md
names. auth-scope is a new scored check that says "authenticated but not scoped"
in its own right, applicable only where it is answerable: sensitive,
builder-wrapped, not delegating. 19 apply on the real tree and all 19 pass.
Sensitivity covered tokens, billing, impersonation and envvars and missed
membership, invites, roles, the login surface, API keys and the two billing
settings the bare "billing" segment does not match. Half of SENSITIVE_SYMBOLS
named nothing at all: setImpersonation, createJWT, signJWT and updateEnvVars are
exported nowhere, while startImpersonation was absent. The cohort goes from 26 to
67 and test/webappSymbols.test.ts now fails if a name or a segment stops
resolving.
Two ways a route left the denominator unnoticed. A destructured export
declaration produced no entry point at all, so export const { action } =
builder(...) was simply absent from n. And a route whose body is in another
module read as a redirect stub: zero statements, every check not-applicable, a
placeholder 100. Those are different facts, so delegating routes are counted and
warned about separately, the way parse failures are.
Finally, disclosure rather than weighting. request-context applies to 412 entry
points and is the only applicable scored check for 223 of them, so the report
now prints applicability, pass rate, how many entries rest on one check alone,
and what the global would be without each check.
Global 15 to 19. Reverting the sensitivity vocabulary alone takes it back to 15,
auth-scope is worth 1 and the guard list a net 1.
…orpus fake-require-guard prepends an invented requireObsMapValidRequest() call to every route body, and fake-authenticated-lookup prepends resolveAuthenticatedEnv(). Under the prefix patterns both took the tree from 18 to 19 and raised five routes; with the accept-list both leave every route where it was.
… prints now Re-derives every figure from a run on the current tree: global 19, cohort 67, 75 in the fix list, 329 collapsed into CONTEXT with 43 sensitive, audit 49 of 49, corpus 42 entries with 41 defended, the gaming-boundary codemod 19 to 29 over 139 in-catch log calls. Every invariant now names the test that holds it, or is gone. Three reviewers each found claims here the code did not honour, and in every case a prior review had passed the file, so a claim with no test behind it is now written as a measurement with its script or not written at all. Known limits are the residuals this wave leaves rather than the ones it fixed: guards matched by name and not by behaviour, an ability gate credited as scoping when CLAUDE.md says only the membership-scoped query is the tenant floor, auth-scope's property-assignment-only caller filter, the three login-flow routes that fail auth-boundary correctly and unhelpfully, and a route that delegates one of its two exports. Adds sections for what the score is made of, for a route whose body is in another module, and for how the sensitivity vocabulary and the guard list are held to naming things that exist.
… stop crediting a guard whose answer is ignored
apps/webapp/CLAUDE.md is explicit that the OSS RBAC fallback ability is
permissive, so ability.can(...) alone does not reject a non-member on
self-hosted: the gate enforces the role and the membership-scoped query is the
tenant floor. auth-scope was reading an ability call as scoping, which is the
opposite of what that says.
So the ability limb is gone, and the caller filter is now attributed to the
export it was written in rather than to the file. That second half is what
actually finds anything: both failing routes scope themselves in one export and
not the other, and the file-wide reading passed them on the wrong half.
Two routes now fail, both hand-read, and both resolve their target organization
from the URL slug with no membership filter:
- settings.sso, in the loader. resolveOrg is findFirst({ where: { slug } }) and
the loader has only ability.can("manage", { type: "sso" }) in front of the
connection list. Its action carries the userId, which is what used to clear
the file.
- settings.team, in the action. The org id comes from resolveOrgIdFromSlug and
each mutating branch gates on ability.can alone. Its loader calls
TeamPresenter.call({ userId: user.id }), which is what used to clear the file.
The other three the file-wide reading passed are correct and still pass:
ApiKeysPresenter, regenerateApiKey and DeleteOrganizationService each run
members: { some: { userId } } internally and throw, and settings._index writes
the membership query inline.
Separately, getUser and getUserId answer with null instead of throwing, so
calling one was never evidence of a boundary. They move to SOFT_GUARDS and are
credited only when the body binds the result and a condition reads it, which the
scan now records as checkedCallees. The two routes using them still pass, on
evidence rather than on a hand-read. Dropping the names instead was measured: it
gives two false accusations.
Finally, the symbol resolution test no longer reads a path inside
apps/webapp/node_modules to resolve remix-auth's authenticate and
isAuthenticated. That fails confusingly on a hoisting or version change, and a
test that fails for environmental reasons trains people to ignore it. The two
names are listed as deliberately unchecked, bounded, and the test still rejects
a guard that is neither first-party nor listed.
Global stays 19; the two new fails move the mean 18.704 to 18.583.
… it marginal The whole-tree integration test runs a ts.Program per file for the parse diagnostics and walks the tree a second time to count candidates. That is 1.6 to 2.6 seconds idle and it went past the suite's 10s default once under parallel load. Measured against the round C signals with them stubbed out: no detectable difference, so the test was already marginal rather than newly slow. Given an explicit 30s budget with the reason, matching the exhaustive sweep beside it.
AGENTS.md's convention puts a test beside the file it covers; this package put all of them in test/, reaching back through ../src/. Moves fourteen test files plus the mutation-corpus helper next to their sources with no content change, so this diff reads as a pure rename. Import paths and the docstring-reference walker follow in a separate commit.
…ests Content-only follow-up to the pure-rename commit: fixes relative import paths broken by the move, adjusts tsconfig.build.json to exclude *.test.ts and the mutation-corpus helper from the emitted dist tree, and teaches docstringReferences.test.ts to keep scanning production source only now that tests live in src/ (previously guaranteed by test/ and src/ being separate directories). Also updates README and in-source comments that named the old test/ paths. Verified: dist emits no test or mutation-corpus files; default suite still 514 passed; OBS_MAP_MUTATION_CORPUS=1 corpus still 43 passed, 1 expected fail (run via the README's documented --testTimeout=120000 invocation; an initial run without it hit the suite's 10s default and timed out on whichever entry was slow that pass, which reproduced identically before this round's changes and is unrelated to the move).
…ement clearing auth-scope Four defects from the whole-branch review, all of them the same shape: a list or a corpus that stopped keeping up with the code. AUDIT_SYMBOLS named nothing. auditLog, recordAudit and writeAuditEvent are exported nowhere in apps/webapp, packages/core or internal-packages, so the pass branch could never fire, and both renderers printed "No audit helper exists in the webapp" while models/admin.server.ts has been writing prisma.impersonationAuditLog.create rows on two paths all along. The list is now the three helpers that reach that write, and the AUDIT figure goes from 0 of 49 to 3 of 49. The pass branch had never been exercised against a real name either: its only test imported auditLog from a module that does not exist. It rotted because webappSymbols.test.ts covered every other name list in the package and not this one. That came first, and it fails on the old list. audit-trail was also the one check that did not follow the visibility rule the README states as universal. It gated on sensitivity and hasAction alone, so on resources.impersonation.ts, a four-statement body, auth-boundary declined to judge because any guard would be behind the import while audit-trail accused the route over an audit write behind that same import. It now takes the same exemption, with a known writer read before it so presence still counts where absence does not. auth-scope was defeated by a dead statement. The predicate fired on any property at all whose value was a caller id, wherever it sat, so prepending an unused object holding user.id under an arbitrary key to every body raised settings.sso and settings.team, the only two findings the check has ever produced and both confirmed cross-org exposures. The property name now has to be an identity field and the object has to be handed to a call. Two corpus entries cover both halves. And suppress-every-check emitted directives for four checks, not five: auth-scope was added a round after that entry was written. The corpus could not catch its own omission, because leaving a check out of the sweep lowers the score rather than raising it, so there is now an ungated registry assertion instead. Adding a check without extending the corpus turns pnpm test red.
…and correct the stale figures
Five disclosure and accuracy items from the whole-branch review, no behaviour
change.
auth-scope is applicable only when a route uses a builder, and auth-boundary
passes any route that uses a builder, so auth-scope applicable structurally
implies auth-boundary pass: all 19 carry the same detail string. That free point
is a third to a quarter of each of their scores. The 19 average 59.7 as scored
and 44.6 without auth-boundary, and settings.team, a confirmed cross-org
exposure, scores 25 rather than 0 because of it. Said in the README where the
check is explained, since the finding is the thing to read there, not the score.
Then the stale numbers. Eight docstrings still quoted the pre-round-C baseline of
15, including the code-side disclosure of the largest parked residual, where the
README had it right and the code understated it. Each is re-derived where it can
be, from the live corpus run, and dated where it cannot: two of them measure
rejected variants that would have to be reimplemented to re-measure, so they now
say which tree they were taken against. One of them read "takes the tree back to
15 to 42", which was garbled as well as stale.
terminal.ts said the audit finding would repeat 52 times where it is 46. The
round A forging residual was in the ledger only and is now a Known limits bullet
beside try { String(0); }, with both figures live. The workflow said the corpus
takes roughly three minutes where it is about four and a half for 44 entries.
And score.ts now says outright that it answers the delegating case for every
check, because two checks tested ep.delegating themselves and both branches were
unreachable behind that short-circuit.
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.
The CLI sent the warning to stderr for the whole-report path and renderTerminal embedded the same lines in the stdout report body, so a plain run showed every warning twice. Stderr now carries it only when stdout is JSON a caller parses, which is the reason that loop existed. The terminal report keeps it in the body, so redirecting stdout to a file loses nothing. The single-route path is unchanged: its terminal output never carried the warning, so it never duplicated.
…DME fence dead-if-empty-string said append where prependToEveryCatch splices at the head of the clause. That string is printed in the corpus table and in the Vitest test name, so it described the wrong position. The report-output block in the README had no language tag (markdownlint MD040).
…rror
catch (e) { throw e instanceof Response ? e : new ServerError(e); } read as
inert, while the same clause written with return passed. The walk set rethrows
and cut the path before the shared branch check ran, so the throw arm of that
condition was unreachable and a thrown conditional was never offered to
selectsAnErrorPath. That contradicted the CatchEvidence.branches contract in
types.ts, which names return and throw alike.
The check now runs in the throw arm, before the path is cut, through the same
selectsAnErrorPath predicate the return path uses. The same-arms rule comes
with it, so throw e instanceof Error ? e : e is refused for the reason
return x ? A : A is, and the asymmetry closes without a new laundering surface.
The shared check below narrows to return statements, since the throw case is
handled above.
wrap-body-in-same-arms-throw-ternary is the tree-scale guard, the A/B partner
of wrap-body-in-rethrow with the ternary as the only difference. Weakening the
arms test takes the tree from 19 to 44 and raises 224 routes, and the entry
fails; with the rule in place it reaches 391 files and 924 sites, raises none.
No published figure moves. The whole JSON report over apps/webapp/app/routes is
byte-identical before and after, global 19, measured 412, per-entry diffs 0,
because no route in the tree writes a thrown ternary.
…an unapplied mutation
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.
Comment on lines
+317
to
+318
| const delegated = delegatedLines(head); | ||
| lines.push(...delegated); |
Contributor
There was a problem hiding this comment.
🟡 Pull request comment can be rejected when many routes move their code into another file
The list of routes whose code lives elsewhere is written into the pull request comment in full with no limit (report.delegating.join(", ") at internal-packages/observability-map/src/report/terminal.ts:93) while every neighbouring section is capped, so a wide-reaching refactor can push the comment past the size the platform accepts and nothing gets posted.
Impact: On a pull request that moves many route bodies into separate files, the automated report silently fails to appear.
Why this section is the one that is unbounded
renderPrComment caps every other section by construction: the changed-entries table at MAX_CHANGED_ROWS (15), the fix list at 3, the check contributions at one line per check, and the unknown-suppression lines at MAX_UNKNOWN_SUPPRESSION_LINES (10) — the last of which was added, per the comment at internal-packages/observability-map/src/report/prComment.ts:18-24, precisely because a tree-wide directive typo produced 87,938 characters against GitHub's 65,536 limit and a swallowed 422.
delegatedLines has the same shape: one entry per route, joined into a single line, with no cap. A codemod that moves route bodies into .server.ts modules — the exact refactor the delegating feature exists to notice — is what produces a large report.delegating, and the resulting line is appended verbatim at internal-packages/observability-map/src/report/prComment.ts:317-318. The comment at the top of the file claiming "Every other section of this comment is bounded by construction" is therefore not true today.
The terminal renderer has no such constraint, so the cap belongs at the pull-request-comment call site (slice plus an "and N more" tail) rather than in delegatedLines itself.
Prompt for agents
In internal-packages/observability-map/src/report/prComment.ts, renderPrComment appends delegatedLines(head) unbounded. delegatedLines (internal-packages/observability-map/src/report/terminal.ts) renders every delegating file name into a single joined line, so the size of that section grows with the number of routes that delegate their body. Every other section of the PR comment is explicitly capped — MAX_CHANGED_ROWS for the table, three entries for the fix list, MAX_UNKNOWN_SUPPRESSION_LINES for the typo warnings — and the file's own docstring claims all other sections are bounded by construction. Cap the delegated list the same way the unknown-suppression list is capped (a slice plus an "and N more" tail), doing it at the PR-comment call site so the terminal report keeps printing the full list. Also correct the docstring so it no longer claims every other section is bounded.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+58
to
+64
| // The closing sentence is a claim about the codebase, so it is only made when the figure in | ||
| // front of it supports it. It was printed unconditionally, including next to a non-zero count. | ||
| const gap = | ||
| withAudit === 0 | ||
| ? " No audit helper exists in the webapp." | ||
| : ` ${sensitiveMutations - withAudit} without one.`; | ||
| return `AUDIT ${withAudit} of ${sensitiveMutations} sensitive mutations record an actor.${gap}`; |
Contributor
There was a problem hiding this comment.
🟡 Report claims the app has no way to record who performed an action, which is untrue
The summary line asserts that no audit helper exists whenever the scan finds zero audited sensitive changes (the withAudit === 0 branch at internal-packages/observability-map/src/report/terminal.ts:60-63), even though such a helper does exist, so readers are told a false fact about the codebase instead of a count of what is missing.
Impact: A reader of the report can conclude the product has no audit-logging facility at all and go on to build a second one.
Why the claim is false and when it is reached
The docstring on internal-packages/observability-map/src/checks/auditTrail.ts:7-30 documents this exact sentence as the symptom of the bug that round fixed: the audit symbol list previously named nothing, so the pass branch could never fire, "every applicable route failed, and both renderers printed 'No audit helper exists in the webapp' while models/admin.server.ts was writing prisma.impersonationAuditLog.create(...)". The symbol list was corrected (AUDIT_SYMBOLS now names redirectWithImpersonation, clearImpersonation, startImpersonation, all of which resolve — webappSymbols.test.ts proves it) but the sentence was left in place.
On the full tree withAudit is 3, so the branch is not taken today. It is still reachable: any scan of a subset (--routes=<dir>) that contains sensitive mutations but no impersonation route takes it, as does the full tree the day the three impersonation routes change shape. The existing test does not claim no audit helper exists when one is in use only pins the withAudit > 0 case.
The honest wording for zero is the same shape as the non-zero branch: state how many are missing, not that the facility is absent.
Suggested change
| // The closing sentence is a claim about the codebase, so it is only made when the figure in | |
| // front of it supports it. It was printed unconditionally, including next to a non-zero count. | |
| const gap = | |
| withAudit === 0 | |
| ? " No audit helper exists in the webapp." | |
| : ` ${sensitiveMutations - withAudit} without one.`; | |
| return `AUDIT ${withAudit} of ${sensitiveMutations} sensitive mutations record an actor.${gap}`; | |
| // The closing sentence is a claim about the gap, not about the webapp: an audit helper does | |
| // exist (`AUDIT_SYMBOLS`), so a zero here means nothing reached it, not that nothing is there. | |
| const gap = ` ${sensitiveMutations - withAudit} without one.`; | |
| return `AUDIT ${withAudit} of ${sensitiveMutations} sensitive mutations record an actor.${gap}`; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+99
to
+108
| - name: 🔎 Scan base with the head's scanner | ||
| run: | | ||
| if git worktree add /tmp/base-tree ${{ github.event.pull_request.base.sha }} \ | ||
| && pnpm --filter @internal/observability-map exec tsx src/cli.ts --json --no-write \ | ||
| --routes=/tmp/base-tree/apps/webapp/app/routes > /tmp/base.json; then | ||
| : | ||
| else | ||
| echo "-" > /tmp/base.json || true | ||
| echo "base scan failed or the worktree could not be added; falling back to no base" >&2 | ||
| fi |
Contributor
There was a problem hiding this comment.
🔍 The base comparison uses the base branch tip, not the merge base the README describes
The base scan resolves its tree from github.event.pull_request.base.sha, which GitHub sets to the tip of the base branch as of the last PR update, not to the merge base between head and base. internal-packages/observability-map/README.md:31-32 says the comment scans "head against the PR's merge base".
The practical consequence is that unrelated commits landing on main after the PR was opened move the base report, so the comment can attribute a score change (or an added/removed entry row) to a pull request that did not cause it. The merge base is available in a pull_request checkout with fetch-depth: 0 via git merge-base HEAD ${{ github.event.pull_request.base.sha }}, which would match the documented behaviour. Since the job is report-only this is a precision issue rather than a gate failure, but the delta table is the section the whole comment exists for.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+152
to
+173
| function scopesByCallerIn(fns: Iterable<EntryFunction>): boolean { | ||
| let found = false; | ||
| const visit = (node: ts.Node) => { | ||
| if (found) return; | ||
| if ( | ||
| ts.isPropertyAssignment(node) && | ||
| ts.isIdentifier(node.name) && | ||
| CALLER_ID_FIELD.test(node.name.text) | ||
| ) { | ||
| const path = propertyPath(node.initializer); | ||
| if (path !== null && CALLER_ID_PATH.test(path) && isHandedToACall(node)) { | ||
| found = true; | ||
| return; | ||
| } | ||
| } | ||
| ts.forEachChild(node, visit); | ||
| }; | ||
| for (const fn of fns) { | ||
| if (fn.body) visit(fn.body); | ||
| } | ||
| return found; | ||
| } |
Contributor
There was a problem hiding this comment.
🔍 auth-scope credits any call handed an object with a caller-id property, including a log call
scopesByCallerIn requires two things: the property name is in CALLER_ID_FIELD and the object literal holding it reaches a call argument (isHandedToACall). Neither condition constrains what the callee is, so logger.error("create failed", { userId: user.id }) written anywhere in a builder-wrapped handler satisfies the check and clears auth-scope for that export.
This is the same class as the disclosed actor-argument residual on internal-packages/observability-map/src/checks/authScope.ts:83-86 (generatePortalLink({ organizationId, userId: user.id })), so it is not undisclosed. Worth noting anyway because it is cheaper to reach than the documented case, and the mutation corpus does not cover it: dead-caller-scope-object and dead-caller-scope-userid both prepend a dead object, which isHandedToACall correctly refuses, so the corpus would not notice if a future edit widened the handed-to-a-call side. A corpus entry that prepends logger.error("obs-map", { userId: user.id }); to every body would close that gap, or a rise would show and the entry would join KNOWN_GAPS honestly.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+18
to
+25
| # The tree-scale mutation corpus: every known laundering shape applied to the whole route tree, | ||
| # asserting the score does not rise. Roughly four and a half minutes for 44 entries, which is why | ||
| # it is gated out of the package's default `pnpm test` and run here instead. Unlike the report | ||
| # job below it has no token to lose, so it runs for fork PRs too, and unlike the report job it is | ||
| # allowed to fail the build. | ||
| mutation-corpus: | ||
| name: 🧬 Mutation corpus | ||
| runs-on: warp-ubuntu-latest-x64-4x |
Contributor
There was a problem hiding this comment.
🔍 Report and mutation-corpus jobs both scan the full tree on every qualifying PR
observability-map.yml runs two full scans in the report job (head and base, each a ts.Program per route file for the syntactic diagnostics, ~1.5s each per the note at internal-packages/observability-map/src/scan.ts:1105-1111) plus the mutation corpus, which the header comment puts at roughly four and a half minutes for 44 entries, each entry rescanning the whole tree. The corpus job runs on every pull request touching apps/webapp/app/routes/**, which is a large share of webapp PRs, and it is allowed to fail the build while not being part of the all-checks aggregate.
That combination is worth confirming with maintainers: a red X on a non-required job that fires on most route changes and takes ~5 minutes of a 4x runner is the kind of thing that gets muted. The pnpm test gate through pr_checks.yml is the required one and is fast, so the split is deliberate; the question is whether the corpus should be scheduled (nightly / on changes to the package only) rather than per route PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
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.
Comment on lines
+164
to
+167
| obsmap: | ||
| needs: changes | ||
| if: needs.changes.outputs.obsmap == 'true' | ||
| uses: ./.github/workflows/unit-tests-observability-map.yml |
Contributor
There was a problem hiding this comment.
🔍 New package's test suite will run twice on any PR that touches it
pr_checks.yml's pre-existing internal filter already matches internal-packages/** (line 76), and unit-tests-internal.yml runs turbo run test --filter "@internal/*" across 12 shards. @internal/observability-map matches that filter, so a PR touching this package now triggers both the new lightweight obsmap job and the heavy 32-vCPU internal job (which also builds the package via turbo run build --filter "@internal/*..."). The new filter is still worth having, because it is the only thing that covers apps/webapp/app/routes/**, but the duplication is worth knowing about: it costs a full container-backed internal run plus a 30s real-tree scan inside a shard for every change to this package. If you want to avoid it, the internal filter would need a negation for internal-packages/observability-map/**, or the reusable internal workflow would need to exclude the package from its turbo filter.
Was this helpful? React with 👍 or 👎 to provide feedback.
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.