fix(observability-map): stop a switch's break cutting the statements 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.

Author: 1stvamp

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

@@ -741,6 +741,61 @@ describe("error-classification", () => {
     );
     expect(r.status).toBe("fail");
   });
+
+  // The verdict end of the `break and continue inside the construct they target` finding. A clause
+  // that sorts the error by code and then rethrows was failed, with a detail line asserting it
+  // takes one way out regardless of what was thrown, which is the opposite of what it does. Both
+  // spellings are here because the pair is the evidence: the switch must not change the verdict.
+  const SORTED_RETHROW = (sorter: string) => `import { prisma } from "~/db.server";
+     export async function action({ request, params }) {
+       try {
+         return json(await prisma.thing.update({ where: { id: params.id }, data: {} }));
+       } catch (e) {
+         ${sorter}
+         throw e;
+       }
+     }`;
+
+  it("does not accuse a clause that sorts the error by code and rethrows", () => {
+    const r = run(
+      "error-classification",
+      "api.v1.sorted.ts",
+      SORTED_RETHROW(
+        'switch (e.code) { case "P2025": handleNotFound(e); break; default: handleOther(e); break; }'
+      )
+    );
+    expect(r.status).toBe("not-applicable");
+    expect(r.detail).not.toContain("one way out");
+  });
+
+  it("reads the same clause written without the switch identically", () => {
+    const withSwitch = run(
+      "error-classification",
+      "api.v1.sorted.ts",
+      SORTED_RETHROW(
+        'switch (e.code) { case "P2025": handleNotFound(e); break; default: handleOther(e); break; }'
+      )
+    );
+    const without = run(
+      "error-classification",
+      "api.v1.sorted.ts",
+      SORTED_RETHROW("handleOther(e);")
+    );
+    expect(withSwitch).toEqual(without);
+  });
+
+  // The other direction, so the rule above is not just "a switch is ignored": the same sorter with
+  // a clause that answers the request is a decision, and still passes.
+  it("still passes a clause whose switch on the error code answers the request", () => {
+    const r = run(
+      "error-classification",
+      "api.v1.sorted.ts",
+      SORTED_RETHROW(
+        'switch (e.code) { case "P2025": return new Response(null, { status: 404 }); default: break; }'
+      )
+    );
+    expect(r.status).toBe("pass");
+  });
 });
 
 describe("auth-boundary", () => {

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

@@ -304,6 +304,11 @@ function containsLooseJump(node: ts.Node): boolean {
  * Only applied to a clause whose statements already end in a `return` or a `throw`, so the appended
  * throw really is unreachable, and never to one holding a loose `break` or `continue`, which a `do`
  * or a `switch` would capture.
+ *
+ * `dead-throw-after-switch-break` guards the opposite direction of the same rule. A `break` in a
+ * switch clause is no longer read as leaving the statement list the switch sits in, and the cheap
+ * way to write that is "a clause holding a break does not exit", which would take this whole family
+ * back: the clause here returns AND breaks, and the return is what has to win.
  */
 function deadThrowAfter(id: string, what: string, wrap: (body: string) => string): Mutation {
   return {
@@ -763,6 +768,11 @@ export const MUTATIONS: Mutation[] = [
     "wrap every catch body in a switch default and write throw e; after it",
     (body) => `switch (1) { default: {\n${body}\n} }`
   ),
+  deadThrowAfter(
+    "dead-throw-after-switch-break",
+    "wrap every catch body in a switch default that also breaks and write throw e; after it",
+    (body) => `switch (1) { default: {\n${body}\n}\nbreak; }`
+  ),
   deadThrowAfter(
     "dead-throw-after-try-finally",
     "wrap every catch body in try { ... } finally { } and write throw e; after it",
@@ -879,6 +889,7 @@ export const ADDITIVE_IDS = [
   "dead-throw-after-if-true",
   "dead-throw-after-if-else",
   "dead-throw-after-switch",
+  "dead-throw-after-switch-break",
   "dead-throw-after-try-finally",
   "wrap-body-in-rethrow",
   "wrap-body-in-same-arms-throw-ternary",

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

@@ -887,6 +887,121 @@ describe("scanFile: catch clause evidence", () => {
     });
   });
 
+  // S3. `definitelyExits` counted a bare `break` and a bare `continue` wherever it found one, and
+  // both of those target the nearest enclosing construct of their kind rather than the statement
+  // list the question is about. A `switch` whose clauses all break falls through to the statement
+  // written after it, so cutting that statement as unreachable accused a route of swallowing an
+  // error it rethrows, with a detail line saying it "takes one way out regardless of what was
+  // thrown" about a clause that takes the same way out it arrived by. This is the false-accusation
+  // direction, so both halves are pinned: what must now stay reachable, and what must still be cut.
+  describe("break and continue inside the construct they target", () => {
+    const clause = (body: string) => `
+      export async function loader() {
+        try {
+          return await prisma.thing.findMany();
+        } catch (e) {
+          ${body}
+        }
+      }
+    `;
+
+    // Reachable, so the throw after them is a real rethrow. Each jump targets the construct it is
+    // written in, and every one of these constructs falls through to the next statement.
+    const FALLS_THROUGH: Array<[string, string]> = [
+      [
+        "a switch whose clauses all break",
+        'switch (e.code) { case "P2025": handleNotFound(); break; default: handleOther(); break; }',
+      ],
+      ["a switch whose default is a bare break", "switch (e.code) { default: break; }"],
+      ["a do body that breaks", "do { break; } while (false);"],
+      ["a do body that continues", "do { continue; } while (false);"],
+      [
+        "a do body whose if/else both break",
+        "do { if (pick()) { break; } else { break; } } while (false);",
+      ],
+    ];
+
+    for (const [label, wrapped] of FALLS_THROUGH) {
+      it(`still sets rethrows for a throw written after ${label}`, () => {
+        const ep = scanFile("x.ts", clause(`${wrapped}\nthrow e;`));
+        expect(ep!.catches[0]!.rethrows).toBe(true);
+      });
+
+      it(`still credits an error test written after ${label}`, () => {
+        const ep = scanFile(
+          "x.ts",
+          clause(`${wrapped}\nif (e instanceof Error) { return json({ a: 1 }); }\nthrow e;`)
+        );
+        expect(ep!.catches[0]!.branches).toBe(true);
+      });
+    }
+
+    // The clause the whole finding was about, end to end: sorting the error by code and then
+    // rethrowing is a rethrow, which is `not-applicable`, and never a swallow.
+    it("reads a switch on the error code followed by a rethrow as a rethrow", () => {
+      const ep = scanFile(
+        "x.ts",
+        clause(
+          'switch (e.code) { case "P2025": handleNotFound(); break; default: handleOther(); break; }\nthrow e;'
+        )
+      );
+      expect(ep!.catches[0]).toMatchObject({ rethrows: true, throws: true, branches: false });
+    });
+
+    // Cut, so the throw after them is dead and must not be credited. The first two are the
+    // over-correction control: a clause that returns and also breaks still exits, and reading the
+    // break as "no exit" would take the whole `dead-throw-after-*` family back.
+    const EXITS: Array<[string, string]> = [
+      [
+        "a switch clause that returns before it breaks",
+        "switch (1) { default: { return null; } break; }",
+      ],
+      [
+        "a switch whose every clause returns",
+        "switch (e.code) { case 1: return null; default: return 0; }",
+      ],
+      ["a do body that returns before it breaks", "do { return null; break; } while (false);"],
+    ];
+
+    for (const [label, wrapped] of EXITS) {
+      it(`does not set rethrows for a throw written after ${label}`, () => {
+        const ep = scanFile("x.ts", clause(`${wrapped}\nthrow e;`));
+        expect(ep!.catches[0]!.rethrows).toBe(false);
+      });
+    }
+
+    // A `continue` in a switch clause targets the enclosing loop, not the switch, so it is
+    // inherited through the clause rather than dropped with the `break`. Dropping it would leave
+    // the throw below reachable, and it is not: the continue goes to the `do`'s condition.
+    // The labelled jumps beside it leave the `for` entirely, so they are exits wherever they are
+    // written. The bare `break` is the control that separates the three.
+    const IN_LOOP: Array<[string, boolean]> = [
+      ["break outer", false],
+      ["continue outer", false],
+      ["continue", false],
+      ["break", true],
+    ];
+
+    for (const [jump, rethrows] of IN_LOOP) {
+      it(`reads a switch clause that says ${jump} inside a labelled loop as rethrows=${rethrows}`, () => {
+        const ep = scanFile(
+          "x.ts",
+          `export async function loader() {
+             outer: for (const x of items) {
+               try { await service.call(x); }
+               catch (e) {
+                 switch (e.code) { case 1: ${jump}; default: ${jump}; }
+                 throw e;
+               }
+             }
+             return null;
+           }`
+        );
+        expect(ep!.catches[0]!.rethrows).toBe(rethrows);
+      });
+    }
+  });
+
   // S2. A clause whose try block cannot throw is unreachable, so it is not error handling and
   // nothing should be read off it. Crediting one was the largest hole ever found here: prepending
   // this to a body takes the real tree from 19 to 44 and raises 224 routes, because the routes

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

@@ -468,6 +468,21 @@ function selectsAnErrorPath(node: ts.ConditionalExpression, bindingName: string
   return normalizedText(unwrap(node.whenTrue)) !== normalizedText(unwrap(node.whenFalse));
 }
 
+/**
+ * Which bare (unlabelled) jumps, at this point in the recursion, leave the statement list the
+ * question is being asked about. A bare jump targets the nearest enclosing construct of its kind,
+ * so descending past one of those targets changes the answer for the jumps it captures.
+ */
+type BareJumps = { break: boolean; continue: boolean };
+
+/** A jump written directly in the list under question always leaves it: whatever it targets
+ * encloses the list. */
+const ESCAPES: BareJumps = { break: true, continue: true };
+
+/** A `do` body, asked about from the list the `do` sits in. `break` ends the loop and `continue`
+ * goes to the condition, and both of those reach the statement written after the `do`. */
+const IN_DO_BODY: BareJumps = { break: false, continue: false };
+
 /**
  * A statement that leaves the statement list it sits in on every path through itself, so anything
  * after it in the same list never runs.
@@ -477,52 +492,72 @@ function selectsAnErrorPath(node: ts.ConditionalExpression, bindingName: string
  * it was a block, a `do` body or an `if`/`else` that returned; `dead-throw-after-*` in the mutation
  * corpus is that family, and `scan.test.ts` has one case per construct.
  *
+ * A bare `break` or `continue` only counts where it actually leaves the list, which is what `jumps`
+ * carries. A `break` inside a switch clause targets the switch, so a switch whose clauses all break
+ * falls through to the statement after it and does NOT exit; reading that break as an exit accused
+ * `catch (e) { switch (e.code) { ... break; } throw e; }` of swallowing a rethrown error, which is
+ * both a false verdict and a detail line that says the opposite of what the route does. A `continue`
+ * inside a switch clause targets an enclosing loop instead, which the switch cannot be, so it is
+ * inherited rather than dropped: dropping it would stop
+ * `do { switch (x) { default: continue; } throw e; } while (c)` cutting a throw that really is dead.
+ * `break and continue inside the construct they target` in `scan.test.ts` holds both halves.
+ *
+ * A labelled `break`/`continue` always counts. Its target has to enclose the statement list, since
+ * nothing between the list and the jump can carry the label: `definitelyExits` answers false for a
+ * labelled statement, so the recursion never descends through one.
+ *
  * A sound under-approximation. `if` without an `else`, a labelled statement (a `break` to the label
  * escapes it) and every other loop form answer false, because none of them is guaranteed to run its
- * body. Saying false when the truth is true only leaves a later statement in the list, which is the
- * direction that withholds evidence rather than inventing it.
+ * body. That extends to a `do` that never falls through, `do { continue; } while (true)`, which is
+ * now false for the same reason `while (true) { }` always was: separating it from
+ * `do { continue; } while (c)` means folding the condition, which the dead-code defence deliberately
+ * does not do. Saying false when the truth is true only leaves a later statement in the list, which
+ * is the direction that withholds evidence rather than inventing it.
  */
-function definitelyExits(statement: ts.Statement): boolean {
-  if (
-    ts.isReturnStatement(statement) ||
-    ts.isThrowStatement(statement) ||
-    ts.isContinueStatement(statement) ||
-    ts.isBreakStatement(statement)
-  ) {
-    return true;
+function definitelyExits(statement: ts.Statement, jumps: BareJumps = ESCAPES): boolean {
+  if (ts.isReturnStatement(statement) || ts.isThrowStatement(statement)) return true;
+  if (ts.isBreakStatement(statement)) return statement.label !== undefined || jumps.break;
+  if (ts.isContinueStatement(statement)) return statement.label !== undefined || jumps.continue;
+  if (ts.isBlock(statement)) {
+    return statement.statements.some((s) => definitelyExits(s, jumps));
   }
-  if (ts.isBlock(statement)) return statement.statements.some(definitelyExits);
   // A `do` body runs before its condition is ever read.
-  if (ts.isDoStatement(statement)) return definitelyExits(statement.statement);
+  if (ts.isDoStatement(statement)) return definitelyExits(statement.statement, IN_DO_BODY);
   if (ts.isIfStatement(statement)) {
     return (
       statement.elseStatement !== undefined &&
-      definitelyExits(statement.thenStatement) &&
-      definitelyExits(statement.elseStatement)
+      definitelyExits(statement.thenStatement, jumps) &&
+      definitelyExits(statement.elseStatement, jumps)
     );
   }
   if (ts.isTryStatement(statement)) {
-    if (statement.finallyBlock && definitelyExits(statement.finallyBlock)) return true;
-    if (!definitelyExits(statement.tryBlock)) return false;
-    return statement.catchClause === undefined || definitelyExits(statement.catchClause.block);
+    if (statement.finallyBlock && definitelyExits(statement.finallyBlock, jumps)) return true;
+    if (!definitelyExits(statement.tryBlock, jumps)) return false;
+    return (
+      statement.catchClause === undefined || definitelyExits(statement.catchClause.block, jumps)
+    );
   }
   if (ts.isSwitchStatement(statement)) {
+    const inClause: BareJumps = { break: false, continue: jumps.continue };
     const clauses = statement.caseBlock.clauses;
     const last = clauses[clauses.length - 1];
     if (!clauses.some(ts.isDefaultClause) || last === undefined) return false;
     // An empty clause falls through to the next one, so it does not have to exit itself; the last
     // clause has nothing to fall through to and does.
     return (
-      clauses.every((c) => c.statements.length === 0 || c.statements.some(definitelyExits)) &&
-      last.statements.some(definitelyExits)
+      clauses.every(
+        (c) => c.statements.length === 0 || c.statements.some((s) => definitelyExits(s, inClause))
+      ) && last.statements.some((s) => definitelyExits(s, inClause))
     );
   }
   return false;
 }
 
 /** `statements` up to and including the first one that definitely exits. */
 function reachableStatements(statements: readonly ts.Statement[]): readonly ts.Statement[] {
-  const index = statements.findIndex(definitelyExits);
+  // The arrow matters: `findIndex` passes an index as the second argument, which `definitelyExits`
+  // would read as its `jumps` record.
+  const index = statements.findIndex((s) => definitelyExits(s));
   return index === -1 ? statements : statements.slice(0, index + 1);
 }