Skip to content

Commit ea6d6ff

Browse files
committed
fix(observability-map): block the refused-swallow accusation on any own 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.
1 parent fac948a commit ea6d6ff

5 files changed

Lines changed: 108 additions & 9 deletions

File tree

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

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -172,11 +172,16 @@ export function usesBuilder(ep: EntryPoint): boolean {
172172
* throwing one needs types the scanner does not have. `dead-classifying-try-with-call` in the
173173
* mutation corpus is the open shape, running as an expected failure.
174174
*
175-
* The filter also has to run BEFORE nothing. Ordering the callback branch off `reachable` rather
176-
* than `ep.catches` accused a route that owns a real classifying catch of owning none, whenever
177-
* `canRaise` missed what that catch guarded: a destructuring declaration is not on its list, so
178-
* `try { const { a } = undefined; } catch (e) { ... }` beside a per-item `.map` catch failed with
179-
* "its only error handling sits in a callback the route does not own", which was simply untrue.
175+
* The refused-swallow arm reads the route's own deciding catches through `guardMayRaise`, never
176+
* through `guardCanRaise`. `canRaise` is a whitelist and misses real raising code (a destructuring
177+
* declaration is not on its list, and `const { a } = undefined` throws), so ordering the arm off
178+
* `reachable` accused a route that owns a real classifying catch of owning none, which was simply
179+
* untrue; `does not accuse a route that owns a catch of owning none` pins the verdict. The
180+
* containment read `guardMayRaise` is false only for the provably-inert `try { 0; }`, so the one
181+
* clause that must not block the accusation, the prepended dead classifier `dead-classifying-try`
182+
* refuses, still does not block it (`still fails a per-item swallow beside a deciding catch over a
183+
* dead guard`). The already-open residual is unchanged: `try { String(0); }` reads as may-raise
184+
* AND can-raise, which is `dead-classifying-try-with-call`, the corpus's expected failure.
180185
*/
181186
export const errorClassification = {
182187
id: ID,
@@ -209,8 +214,12 @@ export const errorClassification = {
209214
// `wrap-body-in-rethrow` adds to every route, must not lift a refused swallow out of the
210215
// verdict, or wrapping a per-item-swallow route in try/rethrow reads "every catch rethrows".
211216
// `fails a per-item swallow even when the route owns an inert rethrow catch` pins that.
217+
// "Nothing the route owns decides" is read off `ep.catches` under `guardMayRaise`, not off
218+
// `reachable`: a deciding catch `canRaise` cannot see still decides, and only the
219+
// provably-inert `try { 0; }` guard is excluded. See the `guardMayRaise` paragraph above.
212220
const reachableCb = ep.callbackCatches.filter((c) => c.guardCanRaise);
213-
if (!reachable.some(decides) && reachableCb.some(swallows)) {
221+
const ownDecides = ep.catches.some((c) => decides(c) && c.guardMayRaise);
222+
if (!ownDecides && reachableCb.some(swallows)) {
214223
return {
215224
id: ID,
216225
status: "fail",

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

Lines changed: 78 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -743,8 +743,11 @@ describe("error-classification", () => {
743743

744744
// I4. A route that owns a real classifying catch must never be told it owns none. `canRaise` does
745745
// not list destructuring, and `const { a } = undefined` throws, so the owned catch dropped out of
746-
// `reachable`; with the callback branch ordered off `reachable` the route was then accused of
747-
// having all its error handling in a callback, which was simply false.
746+
// `reachable`; with the refused-swallow arm ordered off `reachable` the route was then accused of
747+
// owning nothing that decides, which was simply false. The arm now reads own deciding catches
748+
// through `guardMayRaise`, so the accusation is withheld and the route sits out exactly as it did
749+
// before the arm existed. Asserted on `status`: an earlier version of this test asserted the
750+
// absence of a detail string no arm ever emits, which could not fail.
748751
it("does not accuse a route that owns a catch of owning none", () => {
749752
const r = run(
750753
"error-classification",
@@ -764,7 +767,79 @@ describe("error-classification", () => {
764767
return json({ ok: true });
765768
}`
766769
);
767-
expect(r.detail).not.toContain("callback the route does not own");
770+
expect(r.status).toBe("not-applicable");
771+
expect(r.detail).toContain("guards nothing that can throw");
772+
});
773+
774+
// I4 sibling: the same shape through `.filter` and a different `canRaise` miss (a plain
775+
// declaration is not on the whitelist either), so the fix is the rule and not the fixture.
776+
it("does not accuse a route whose deciding catch guards a declaration beside a filter swallow", () => {
777+
const r = run(
778+
"error-classification",
779+
"owned-filter.ts",
780+
`import { prisma } from "~/db.server";
781+
export async function action({ request }) {
782+
const items = await prisma.item.findMany();
783+
const kept = items.filter((item) => {
784+
try { return check(item); } catch { return false; }
785+
});
786+
try { const parsed = { ...raw }; } catch (e) {
787+
if (e instanceof TypeError) { return new Response(null, { status: 400 }); }
788+
throw e;
789+
}
790+
return json({ kept });
791+
}`
792+
);
793+
expect(r.status).toBe("not-applicable");
794+
});
795+
796+
// The blocking catch has to DECIDE: an own inert rethrow catch over the same invisible guard
797+
// still leaves the refused swallow in the verdict, or `wrap-body-in-rethrow` spelled with a
798+
// destructuring guard would lift every per-item swallow out of it.
799+
it("still fails a per-item swallow beside an inert catch over an invisible guard", () => {
800+
const r = run(
801+
"error-classification",
802+
"owned-inert.ts",
803+
`import { prisma } from "~/db.server";
804+
export async function action({ request }) {
805+
const items = await prisma.item.findMany();
806+
await Promise.all(
807+
items.map(async (item) => {
808+
try { await processItem(item); } catch { return null; }
809+
})
810+
);
811+
try { const { a } = undefined; } catch (e) { throw e; }
812+
return json({ ok: true });
813+
}`
814+
);
815+
expect(r.status).toBe("fail");
816+
expect(r.detail).toContain("nothing the route owns decides");
817+
});
818+
819+
// And the blocking catch has to guard something that MAY raise: the provably-inert `try { 0; }`
820+
// clause `dead-classifying-try` prepends decides and must still block nothing, or the prepend
821+
// would lift a refused-swallow fail to not-applicable at tree scale.
822+
it("still fails a per-item swallow beside a deciding catch over a dead guard", () => {
823+
const r = run(
824+
"error-classification",
825+
"owned-dead.ts",
826+
`import { prisma } from "~/db.server";
827+
export async function action({ request }) {
828+
const items = await prisma.item.findMany();
829+
await Promise.all(
830+
items.map(async (item) => {
831+
try { await processItem(item); } catch { return null; }
832+
})
833+
);
834+
try { 0; } catch (e) {
835+
if (e instanceof Error) { return new Response(null, { status: 400 }); }
836+
throw e;
837+
}
838+
return json({ ok: true });
839+
}`
840+
);
841+
expect(r.status).toBe("fail");
842+
expect(r.detail).toContain("nothing the route owns decides");
768843
});
769844

770845
// C2. A single-element array cannot iterate, so `[0].map(async () => { whole body })` is not a

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1752,6 +1752,7 @@ describe("scanFile: per-catch evidence", () => {
17521752
guardsParse: true,
17531753
awaitsOnlyParse: true,
17541754
guardCanRaise: true,
1755+
guardMayRaise: true,
17551756
tryStatementCount: 1,
17561757
});
17571758
expect(ep!.catches[1]).toMatchObject({
@@ -1856,6 +1857,7 @@ describe("scanFile: per-catch evidence", () => {
18561857
guardsParse: false,
18571858
awaitsOnlyParse: false,
18581859
guardCanRaise: true,
1860+
guardMayRaise: true,
18591861
tryStatementCount: 4,
18601862
});
18611863
});

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1641,6 +1641,7 @@ export function scanFile(fileName: string, source: string): EntryPoint | null {
16411641
throws: clause.throws,
16421642
branches: clause.branches,
16431643
...guardedWork(node.tryBlock),
1644+
guardMayRaise: tryBlockMayThrow(node.tryBlock),
16441645
tryStatementCount,
16451646
});
16461647
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,18 @@ export type CatchEvidence = {
5353
* declaration it misses.
5454
*/
5555
guardCanRaise: boolean;
56+
/**
57+
* The containment twin of `guardCanRaise`: false only when the guarded region provably cannot
58+
* raise, i.e. every statement is an expression over a bare literal, which is `try { 0; }` and
59+
* nothing else. Everything `canRaise`'s whitelist misses (a destructuring declaration, a
60+
* temporal-dead-zone read) stays true here, so `guardCanRaise` implies `guardMayRaise`. What the
61+
* refused-callback arm of `error-classification` reads: a route whose own classifying catch
62+
* `canRaise` cannot see must never be told nothing it owns decides
63+
* (`does not accuse a route that owns a catch of owning none`), while the provably dead
64+
* `try { 0; }` clause still blocks nothing (`still fails a per-item swallow beside a deciding
65+
* catch over a dead guard`).
66+
*/
67+
guardMayRaise: boolean;
5668
/**
5769
* Everything the guarded region waits for is one of those parses. What separates
5870
* `try { const body = await request.json(); } catch { 400 }` from

0 commit comments

Comments
 (0)