Skip to content

Commit 78e6ad5

Browse files
committed
fix(observability-map): judge refused callback catches on their evidence
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.
1 parent 2cb4233 commit 78e6ad5

6 files changed

Lines changed: 227 additions & 31 deletions

File tree

internal-packages/observability-map/src/checks/errorClassification.ts

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -146,11 +146,19 @@ export function usesBuilder(ep: EntryPoint): boolean {
146146
* `admin.api.v1.runs-replication.status.ts` at the top of the first rendered fix list.
147147
*
148148
* `callbackCatches` is the third case, and it is what stops "no catch is not-applicable" from being
149-
* a payout. A route whose catches all sat inside a callback the scanner refused to attribute has
150-
* error handling, the scanner just could not read it as the route's; excusing that is worth 50
151-
* points to anyone who wraps a body in something the boundary rule refuses, which
152-
* `Promise.all([0].map(async () => { ... }))` did. It fails instead. The precision cost is a route
153-
* that genuinely only handles errors per item, which now fails rather than sitting out.
149+
* a payout. A refused catch is judged on its evidence, never on its placement: the same
150+
* `catchClauseEvidence` an own catch gets, with two arms reading it. A refused swallow fails the
151+
* route whenever nothing the route owns decides, and that arm is deliberately not conditioned on
152+
* the route owning no catches, so an own inert rethrow catch cannot lift a refused swallow out of
153+
* the verdict (`fails a per-item swallow even when the route owns an inert rethrow catch`). A
154+
* route whose only catches are refused and none of them swallows sits out, and never passes: the
155+
* not-applicable ceiling is what keeps a prepended dead deciding `.map` from minting a pass on the
156+
* 261 catchless routes, which `dead-deciding-map` in the mutation corpus holds at tree scale and
157+
* `sits out a catchless route with a prepended dead deciding map` pins on a fixture. What the old
158+
* blanket placement rule blocked, relocating a swallow behind the boundary, still fails
159+
* (`still fails a swallow wrapped in a non-array receiver's .map(...)`); what it wrongly accused,
160+
* a route whose only error handling genuinely is per item, now sits out instead of failing
161+
* (`sits out a route whose only catch is a deciding per-item boundary`).
154162
*
155163
* A clause whose try block holds nothing that could raise is read as no clause at all,
156164
* `guardCanRaise` on the evidence. Prepending `try { 0; } catch (e) { if (e instanceof Error) {
@@ -195,14 +203,35 @@ export const errorClassification = {
195203
: `catches its errors and chooses what to do without looking at what was thrown${which}`,
196204
};
197205
}
198-
// Read off `ep.catches`, not `reachable`: a route that owns a catch owns one, whether or not
199-
// `canRaise` could see what it guarded. Ordering this off `reachable` turned every `canRaise`
200-
// miss on a route that also has a per-item catch into an accusation that was flatly false.
201-
if (ep.catches.length === 0 && ep.callbackCatches > 0) {
206+
// A refused (iteration-callback) catch is judged on its evidence, never on its placement.
207+
// The fail arm first: a refused swallow fails whenever nothing the route owns decides.
208+
// Deliberately NOT conditioned on `ep.catches.length === 0`: an own inert catch, which
209+
// `wrap-body-in-rethrow` adds to every route, must not lift a refused swallow out of the
210+
// verdict, or wrapping a per-item-swallow route in try/rethrow reads "every catch rethrows".
211+
// `fails a per-item swallow even when the route owns an inert rethrow catch` pins that.
212+
const reachableCb = ep.callbackCatches.filter((c) => c.guardCanRaise);
213+
if (!reachable.some(decides) && reachableCb.some(swallows)) {
202214
return {
203215
id: ID,
204216
status: "fail",
205-
detail: "its only error handling sits in a callback the route does not own",
217+
detail:
218+
"a catch inside an iteration callback swallows what it caught, and nothing the route owns decides",
219+
};
220+
}
221+
// The ceiling: refused catches never reach the pass arm, so a route whose only catches are
222+
// refused and none of them swallows sits out of the denominator rather than collecting
223+
// anything. Read off `ep.catches`, not `reachable`: a route that owns a catch owns one,
224+
// whether or not `canRaise` could see what it guarded; ordering this off `reachable` turned
225+
// every `canRaise` miss on a route that also has a per-item catch into an accusation that was
226+
// flatly false. The detail asserts nothing about ownership or per-item-ness the scanner
227+
// cannot know: a once-invoked Result-style wrapper with a deciding inner catch reads the same
228+
// as its inline equivalent would.
229+
if (ep.catches.length === 0 && ep.callbackCatches.length > 0) {
230+
return {
231+
id: ID,
232+
status: "not-applicable",
233+
detail:
234+
"its only catches sit in iteration callbacks and none swallows, so the route itself classifies nothing",
206235
};
207236
}
208237
if (!reachable.some(decides)) {

internal-packages/observability-map/src/checks/index.test.ts

Lines changed: 105 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -507,11 +507,11 @@ describe("error-classification", () => {
507507
expect(r.status).toBe("not-applicable");
508508
});
509509

510-
// A7 as revised. A per-item error boundary inside a `.map()` callback is still not judged as the
511-
// route's own catch, so it never sets `catches` and never speaks for the route's `tryStatementCount`.
512-
// It is no longer excused either. Reading "no catch of its own" as not-applicable was worth 50
513-
// points to anything that could get the boundary rule to refuse the route's real catch, which
514-
// `[0].map(...)` did, so a refused catch now fails instead of sitting out.
510+
// A7 as revised twice. A per-item error boundary inside a `.map()` callback is still not judged
511+
// as the route's own catch, so it never sets `catches` and never speaks for the route's
512+
// `tryStatementCount`. This catch SWALLOWS what it caught, and nothing the route owns decides,
513+
// so the route still fails: judging refused catches on their evidence must not stop failing the
514+
// relocated swallow, which is the anti-laundering half of the rule.
515515
it("fails a route whose only catch is inside a Promise.all(items.map(...)) callback", () => {
516516
const source = `import { prisma } from "~/db.server";
517517
export async function action({ request }) {
@@ -529,10 +529,108 @@ describe("error-classification", () => {
529529
}`;
530530
const ep = scanFile("batch.process.ts", source)!;
531531
expect(ep.catches).toEqual([]);
532-
expect(ep.callbackCatches).toBe(1);
532+
expect(ep.callbackCatches).toHaveLength(1);
533533
const r = run("error-classification", "batch.process.ts", source);
534534
expect(r.status).toBe("fail");
535-
expect(r.detail).toContain("callback the route does not own");
535+
expect(r.detail).toContain("a catch inside an iteration callback swallows");
536+
});
537+
538+
// The evidence half of the mechanism-C rule: a refused catch that DECIDES caps at not-applicable
539+
// rather than failing (the old placement rule) or passing (the crediting rule `dead-deciding-map`
540+
// exists to refuse). The route's error handling is real and per item; the route itself decides
541+
// nothing, so out of the denominator is the honest place for it.
542+
it("sits out a route whose only catch is a deciding per-item boundary", () => {
543+
const r = run(
544+
"error-classification",
545+
"batch.decide.ts",
546+
`import { prisma } from "~/db.server";
547+
export async function action({ request }) {
548+
const results = await stream.map(async (item) => {
549+
try {
550+
await service.call(item);
551+
} catch (e) {
552+
if (e instanceof KnownError) { return new Response(e.code, { status: 400 }); }
553+
throw e;
554+
}
555+
});
556+
return json({ results });
557+
}`
558+
);
559+
expect(r.status).toBe("not-applicable");
560+
expect(r.detail).toContain("its only catches sit in iteration callbacks");
561+
});
562+
563+
// Same shape with an inert per-item rethrow: not a swallow, so it sits out too.
564+
it("sits out a route whose only catch is an inert per-item rethrow", () => {
565+
const r = run(
566+
"error-classification",
567+
"batch.rethrow.ts",
568+
`import { prisma } from "~/db.server";
569+
export async function action({ request }) {
570+
const results = await stream.map(async (item) => {
571+
try {
572+
await service.call(item);
573+
} catch (e) {
574+
throw e;
575+
}
576+
});
577+
return json({ results });
578+
}`
579+
);
580+
expect(r.status).toBe("not-applicable");
581+
expect(r.detail).toContain("its only catches sit in iteration callbacks");
582+
});
583+
584+
// The refused-swallow arm is deliberately not conditioned on the route owning no catches. An
585+
// own inert catch is what `wrap-body-in-rethrow`, a preserving corpus entry, adds to every
586+
// route: were the arm gated on `catches.length === 0`, wrapping a per-item-swallow route in
587+
// try/rethrow would read "every catch rethrows" and lift the fail to not-applicable, a rise
588+
// that existed in the pre-evidence code and was masked only by the affected routes scoring 0 on
589+
// every other check.
590+
it("fails a per-item swallow even when the route owns an inert rethrow catch", () => {
591+
const r = run(
592+
"error-classification",
593+
"batch.wrapped.ts",
594+
`import { prisma } from "~/db.server";
595+
export async function action({ request }) {
596+
try {
597+
const items = await prisma.item.findMany();
598+
await Promise.all(
599+
items.map(async (item) => {
600+
try {
601+
await processItem(item);
602+
} catch {
603+
return null;
604+
}
605+
})
606+
);
607+
return json({ ok: true });
608+
} catch (e) {
609+
throw e;
610+
}
611+
}`
612+
);
613+
expect(r.status).toBe("fail");
614+
expect(r.detail).toContain("a catch inside an iteration callback swallows");
615+
});
616+
617+
// The no-pass ceiling at the fixture scale: a catchless route with a prepended dead deciding
618+
// map sits out. A crediting rule would read pass here, which is 50 free points on the tree's
619+
// 261 catchless routes; the old placement rule read fail, a false accusation on a preserving
620+
// prepend. `dead-deciding-map` in the mutation corpus is the tree-scale version.
621+
it("sits out a catchless route with a prepended dead deciding map", () => {
622+
const r = run(
623+
"error-classification",
624+
"prepended-map.ts",
625+
`import { prisma } from "~/db.server";
626+
export async function action({ request }) {
627+
[0, 1].map((v) => { try { JSON.parse("0"); } catch (e) { if (e instanceof SyntaxError) { return null; } throw e; } return v; });
628+
const rows = await prisma.thing.findMany();
629+
return json({ rows });
630+
}`
631+
);
632+
expect(r.status).toBe("not-applicable");
633+
expect(r.detail).toContain("its only catches sit in iteration callbacks");
536634
});
537635

538636
// The same route with nothing caught anywhere stays not-applicable, so the fail above is

internal-packages/observability-map/src/mutations.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -828,6 +828,19 @@ export const MUTATIONS: Mutation[] = [
828828
},
829829
},
830830

831+
// The no-pass ceiling on refused (iteration-callback) catches, at tree scale. A two-element
832+
// array literal iterates, so the boundary rule refuses this catch; it decides and cannot run
833+
// its deciding arm (JSON.parse("0") never throws). Under any future rule that CREDITS refused
834+
// catches, the ~261 catchless routes rise from not-applicable to pass and this entry goes red.
835+
wrapEveryBody(
836+
"dead-deciding-map",
837+
"prepend a dead deciding per-item catch inside a two-element .map to every route body",
838+
'[0, 1].map((obsMapV) => { try { JSON.parse("0"); } catch (obsMapDead) {' +
839+
" if (obsMapDead instanceof SyntaxError) { return null; } throw obsMapDead; }" +
840+
" return obsMapV; });",
841+
""
842+
),
843+
831844
{
832845
id: "merge-declarations",
833846
kind: "preserving",
@@ -917,6 +930,7 @@ export const ADDITIVE_IDS = [
917930
"wrap-body-in-same-arms-throw-ternary",
918931
"empty-instanceof-if",
919932
"dead-classifier-one-arm",
933+
"dead-deciding-map",
920934
"registered-throw",
921935
"fake-require-guard",
922936
"fake-authenticated-lookup",

internal-packages/observability-map/src/scan.test.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1567,6 +1567,52 @@ describe("scanFile: catch clause evidence", () => {
15671567
);
15681568
expect(ep!.catches).toEqual([]);
15691569
});
1570+
1571+
// A refused catch keeps its evidence, built by the same machinery as an own catch, so
1572+
// `error-classification` can judge what it does rather than where it sits. Both flavours are
1573+
// pinned: the deciding per-item catch and the inert one.
1574+
it("populates evidence for a refused per-item catch that decides", () => {
1575+
const ep = scanFile(
1576+
"x.ts",
1577+
`
1578+
export async function action({ request }) {
1579+
const items = await load(request);
1580+
return items.map(async (item) => {
1581+
try {
1582+
return await process(item);
1583+
} catch (e) {
1584+
if (e instanceof KnownError) { return new Response(e.code, { status: 400 }); }
1585+
throw e;
1586+
}
1587+
});
1588+
}
1589+
`
1590+
);
1591+
expect(ep!.catches).toEqual([]);
1592+
expect(ep!.callbackCatches).toHaveLength(1);
1593+
expect(ep!.callbackCatches[0]).toMatchObject({ branches: true, guardCanRaise: true });
1594+
});
1595+
1596+
it("populates evidence for a refused per-item catch that only rethrows", () => {
1597+
const ep = scanFile(
1598+
"x.ts",
1599+
`
1600+
export async function action({ request }) {
1601+
const items = await load(request);
1602+
return items.map(async (item) => {
1603+
try {
1604+
return await process(item);
1605+
} catch (e) {
1606+
throw e;
1607+
}
1608+
});
1609+
}
1610+
`
1611+
);
1612+
expect(ep!.catches).toEqual([]);
1613+
expect(ep!.callbackCatches).toHaveLength(1);
1614+
expect(ep!.callbackCatches[0]).toMatchObject({ rethrows: true, branches: false });
1615+
});
15701616
});
15711617
});
15721618

internal-packages/observability-map/src/scan.ts

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1026,11 +1026,13 @@ function isAtMostSingletonArray(expr: ts.Expression): boolean {
10261026
* whole body })` collected it.
10271027
*
10281028
* Two things changed. A receiver that is an array literal of one element or none is refused here,
1029-
* because it cannot iterate. And the direction that used to pay no longer pays: `walkBody` counts
1030-
* the catches it refuses, and `error-classification` fails a route whose only catches were refused
1031-
* rather than excusing it. That is what makes the name list survivable, and it is why
1029+
* because it cannot iterate. And the direction that used to pay no longer pays: `walkBody` keeps
1030+
* the catches it refuses, evidence and all, and `error-classification` fails a route with a
1031+
* refused swallow when nothing the route owns decides, while a refused catch that decides caps at
1032+
* not-applicable and never a pass. That is what makes the name list survivable, and it is why
10321033
* `Result.map(...)`, which no name list can tell from `users.map(...)`, is a corpus entry that
1033-
* passes rather than a hole.
1034+
* passes rather than a hole: relocating a swallow behind the boundary still fails, and relocating
1035+
* a decision earns at most the route's exit from the denominator.
10341036
*
10351037
* The other direction still costs points and the earlier version of this comment said otherwise.
10361038
* A per-item callback under a callee the name list does not know, `pMap(items, cb)` or
@@ -1509,7 +1511,7 @@ export function scanFile(fileName: string, source: string): EntryPoint | null {
15091511

15101512
let statementCount = 0;
15111513
let hasTryCatch = false;
1512-
let callbackCatches = 0;
1514+
const callbackCatches: CatchEvidence[] = [];
15131515
const catches: CatchEvidence[] = [];
15141516
const calleeNames: string[] = [];
15151517
const logCalls: LogCall[] = [];
@@ -1540,8 +1542,9 @@ export function scanFile(fileName: string, source: string): EntryPoint | null {
15401542
// not: a per-item catch is not part of this body's own statement list, and `countStatement`
15411543
// already stops at a nested function boundary, so counting it here let `tryStatementCount`
15421544
// exceed the entry point's whole `statementCount` and judged a per-item error boundary as
1543-
// though it were the route's own. What is refused is counted in `callbackCatches` instead of
1544-
// dropped, so a route whose only error handling was refused is failed rather than excused.
1545+
// though it were the route's own. What is refused is kept in `callbackCatches` with its
1546+
// evidence instead of dropped, so `error-classification` can fail a refused swallow and sit
1547+
// out a refused catch that decides, without ever crediting either as the route's own.
15451548
//
15461549
// Only an iteration callback is a boundary, not every function-like node: a route's own body
15471550
// wrapped in `trace(async () => {...})`, `mutateWithFallback({ pgMutation: async (t) => {...} })`
@@ -1562,11 +1565,13 @@ export function scanFile(fileName: string, source: string): EntryPoint | null {
15621565
}
15631566
if (ts.isTryStatement(node)) {
15641567
hasTryCatch = true;
1565-
if (node.catchClause && inCallback) callbackCatches++;
1566-
if (node.catchClause && !inCallback) {
1568+
if (node.catchClause) {
1569+
// Built the same way for a refused catch as for an own one, so the dead-code defence
1570+
// and the walk's guaranteed-execution rules apply to both. Which list it lands in is
1571+
// walkBody's attribution decision alone.
15671572
const tryStatementCount = countStatements(node.tryBlock.statements);
15681573
const clause = catchClauseEvidence(node.catchClause);
1569-
catches.push({
1574+
(inCallback ? callbackCatches : catches).push({
15701575
rethrows: clause.rethrows,
15711576
throws: clause.throws,
15721577
branches: clause.branches,

internal-packages/observability-map/src/types.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -146,11 +146,15 @@ export type EntryPoint = {
146146
catches: CatchEvidence[];
147147
/**
148148
* Catch clauses the scan found but refused to attribute to the route, because they sit inside a
149-
* per-item iteration callback. Kept rather than dropped so `error-classification` can tell "this
150-
* route catches nothing" from "this route's only error handling was refused", which are 50 points
151-
* apart and used to read the same.
149+
* per-item iteration callback. Still refused for attribution: they never join `catches`, never
150+
* speak for the route's `tryStatementCount`, and never reach a pass. Kept WITH their evidence,
151+
* built by the same `catchClauseEvidence` machinery as an own catch, so `error-classification`
152+
* can judge what a refused catch does rather than where it sits: a refused swallow fails the
153+
* route (`fails a per-item swallow even when the route owns an inert rethrow catch`), a refused
154+
* catch that decides or rethrows caps at not-applicable (`sits out a route whose only catch is a
155+
* deciding per-item boundary`). The count the old field carried is `.length`.
152156
*/
153-
callbackCatches: number;
157+
callbackCatches: CatchEvidence[];
154158
/** Calls to a `logger.*` or `log.*` callee in those bodies, in source order. */
155159
logCalls: LogCall[];
156160
/**

0 commit comments

Comments
 (0)