fix(approvals): keep an undifferentiable stranded row in the report, and stop a malformed host verdict aborting the scan

.changeset/stranded-inspection-undifferentiated-rows.md

@@ -0,0 +1,12 @@
+---
+"@objectstack/plugin-approvals": patch
+---
+
+`inspectStrandedRequests` no longer drops a row it could not differentiate, and no longer lets one misbehaving host abort the whole scan (#16709, items 2 and 3).
+
+Both are the same mistake at two altitudes: the method exists to **enumerate** the terminal approval requests whose flow run cannot advance, so a failure to read the #15358 third oracle must never remove a row from the answer — and never remove the *other* rows either.
+
+- **A thrown third read now leaves its row in `stranded`, as `'failed'`.** It used to be counted `undetermined` and skipped, exactly as a thrown `hasSuspendedRun` or `getRun` is. Those two are not the same question: a throw from either leaves it unknown *whether* the row is stranded at all, and a storage outage must not be published as a lost run. By the time the third oracle is asked, both have answered — no live pause, terminal `failed` — and it is asked only *which* of the three shapes the row is. A read that could not be made is therefore the textbook "could not differentiate", which is what `'failed'` already means (`StrandedRunState`, #15358 ruling item 1). Dropping the row let `stranded: []` read as "nothing stranded" while a row was in fact stuck, with a log line as its only trace; for a report, fail-closed means showing the row.
+- **A host that violates `ApprovalResumeSurface` no longer aborts the scan.** `refineFailedRunState(verdict)` ran outside the `try` that wrapped the read, so an implementation resolving `undefined` where a verdict is declared threw a `TypeError` out of `inspectStrandedRequests` itself and the scan enumerated **nothing**. The refinement now runs inside that `try`; a malformed verdict costs its own row the differentiation, is counted `undetermined`, and costs every other row nothing.
+
+⛔ No new `StrandedRunState` member and no widened export: both cases map onto the existing undifferentiated `'failed'`. The `undetermined` counter is kept as telemetry and now **overlaps** `stranded` by design — a row can be both reported and counted — so neither number alone sizes the scan's blind spot.

packages/plugins/plugin-approvals/src/approval-service.ts

@@ -210,8 +210,11 @@ export interface ApprovalResumeSurface {
    * dead: the #15555 false-negative harm, one surface over) and ⛔ never
    * skipped (that hides the row). Absence of the discriminator is not
    * evidence of anything. Rejects when a store cannot be read; the inspection
-   * counts such a row `undetermined`, exactly as it does a thrown
-   * {@link hasSuspendedRun}.
+   * counts such a row `undetermined` — but ⛔ unlike a thrown
+   * {@link hasSuspendedRun} it does NOT drop the row, because this oracle is
+   * asked only WHICH shape a row already known to be stranded is (#16709).
+   * A host that resolves a malformed verdict is treated the same way, and
+   * costs no OTHER row its answer.
    */
   inspectConsumedSuspension?(runId: string): Promise<
     | { repairable: true }
@@ -495,13 +498,15 @@ function refineFailedRunState(verdict: ConsumedSuspensionVerdict): StrandedRunSt
  *    `NO_CONSUMED_SUSPENSION` covers all three and does not say which. The
  *    label is faithful to the verb: nothing re-arms it; the remedy is a new
  *    run, not a restore.
- *  - `failed` — the engine COULD NOT BE ASKED which of the three it is: the
- *    attached surface has no `inspectConsumedSuspension` (an engine build
- *    older than this plugin, or a test double). Today's undifferentiated
- *    label, kept on purpose as the fail-closed fallback (#15358 ruling, item
- *    1): absence of the discriminator is not evidence, so the row is reported
- *    and its repairability left unstated — ⛔ never `unrepairable`, ⛔ never
- *    dropped from the report.
+ *  - `failed` — the engine COULD NOT BE ASKED which of the three it is, or
+ *    was asked and could not answer. Three ways in: the attached surface has
+ *    no `inspectConsumedSuspension` (an engine build older than this plugin,
+ *    or a test double); the read THREW (a store outage); or the host resolved
+ *    a malformed verdict, violating its own declared surface (#16709). Today's
+ *    undifferentiated label, kept on purpose as the fail-closed fallback
+ *    (#15358 ruling, item 1): a failure to differentiate is not evidence, so
+ *    the row is reported and its repairability left unstated — ⛔ never
+ *    `unrepairable`, ⛔ never dropped from the report.
  *
  * ⚠️ This names the shapes for the REPORT only. It is not a run state: the
  * engine's own vocabulary is still `'completed' | 'paused' | 'failed'`
@@ -4394,7 +4399,10 @@ export class ApprovalService implements IApprovalService {
    * `'snapshot_dropped'` and `'unrepairable'` (see {@link StrandedRunState}).
    * A surface without that member leaves the row `'failed'` — reported,
    * undifferentiated — because absence of the discriminator is not evidence
-   * of anything; a thrown read counts `undetermined`, like the other two.
+   * of anything. So does a read that THREW or answered a malformed verdict
+   * (#16709): by the time this oracle is asked the row is already known to be
+   * stranded, so a failure to differentiate it is not a reason to drop it from
+   * a report — it is counted `undetermined` as telemetry AND reported.
    *
    * ⚠️ **What this can and cannot size.** It makes the condition *visible* in a
    * deployment; it is not itself a census, and it says nothing about this
@@ -4412,7 +4420,16 @@ export class ApprovalService implements IApprovalService {
   async inspectStrandedRequests(options?: { limit?: number }): Promise<{
     scanned: number;
     stranded: StrandedApprovalRequest[];
-    /** Rows skipped because the suspension store could not be read — NOT healthy, just unknown. */
+    /**
+     * Reads that could not be MADE — telemetry, ⛔ never a verdict and ⛔ never
+     * a "healthy" number. A thrown first or second oracle SKIPS its row
+     * (whether that row is stranded at all is then unknown, and a storage
+     * outage must not be published as a lost run); a thrown or malformed THIRD
+     * read leaves its row in `stranded` as the undifferentiated `'failed'` and
+     * is counted here as well — the row is known to be stranded, only its
+     * shape could not be told (#16709). So this counter and `stranded.length`
+     * overlap on purpose, and neither one alone sizes the scan's blind spot.
+     */
     undetermined: number;
   }> {
     const empty = { scanned: 0, stranded: [] as StrandedApprovalRequest[], undetermined: 0 };
@@ -4480,19 +4497,43 @@ export class ApprovalService implements IApprovalService {
       // the other two oracles. See `refineFailedRunState` and
       // `StrandedRunState` for the three answers and why none is folded.
       if (runState === 'failed' && typeof this.automation.inspectConsumedSuspension === 'function') {
-        let verdict: ConsumedSuspensionVerdict;
+        // ⚠️ [#16709 item 3] The REFINEMENT runs inside this `try`, with the
+        // read it refines. `refineFailedRunState` dereferences the verdict, so
+        // a host that violates the declared surface — resolving `undefined`
+        // where a verdict is declared — used to throw a `TypeError` out of
+        // `inspectStrandedRequests` itself, turning a PARTIAL answer into NO
+        // answer for every OTHER row in the scan. Enumerating the rows that
+        // cannot advance is this method's entire purpose, so a misbehaving
+        // implementation must cost at most the differentiation of its own row.
+        let refined: StrandedRunState | undefined;
+        let differentiated = true;
         try {
-          verdict = await this.automation.inspectConsumedSuspension(runId);
+          refined = refineFailedRunState(await this.automation.inspectConsumedSuspension(runId));
         } catch (err: any) {
+          // [#16709 item 2 — PM ruling, 2026-09-08] The row STAYS in the
+          // report, as the undifferentiated `'failed'`. This oracle is not
+          // asked WHETHER the row is stranded: the first two already answered
+          // that (no live pause, terminal `failed`). It is asked only WHICH of
+          // the three shapes it is — so a read that could not be made is the
+          // textbook "could not differentiate" case, which is exactly what
+          // `'failed'` is kept for (#15358 ruling, item 1).
+          //
+          // ⛔ Never dropped from the list. This is a REPORT of rows that
+          // cannot advance, and a row whose state we failed to determine is
+          // precisely the row an operator has to see; skipping it would make
+          // "nothing stranded" read TRUE while a row is in fact stuck, with
+          // the only trace a log line nobody is paging on. `undetermined`
+          // still counts it, as telemetry — never as a verdict.
+          differentiated = false;
           undetermined++;
           this.logger?.warn?.('[approvals] stranded-request scan could not read the consumed-suspension state', {
             request: raw?.id, run: runId, error: err?.message ?? String(err),
           });
-          continue;
         }
-        const refined = refineFailedRunState(verdict);
-        if (!refined) continue;   // re-armed between the two reads — alive after all
-        runState = refined;
+        if (differentiated) {
+          if (!refined) continue;   // re-armed between the two reads — alive after all
+          runState = refined;
+        }
       }
 
       // Neither suspended nor recoverable: the run this decision was supposed to

packages/plugins/plugin-approvals/src/stranded-request-inspection.test.ts

@@ -116,6 +116,15 @@ function automation(opts: {
   historyThrows?: boolean;
   repairability?: Record<string, Verdict>;
   repairabilityThrows?: boolean;
+  /** Runs whose third-oracle read THROWS — a store outage on those rows alone. */
+  repairabilityThrowsFor?: string[];
+  /**
+   * Runs whose host RESOLVES `undefined` — a contract-violating implementation
+   * of its own declared surface (#16709 item 3). ⛔ Deliberately outside
+   * `Verdict`: pinning what happens when a host lies is the whole point, and
+   * the cast that makes it expressible is confined to this double.
+   */
+  repairabilityMalformedFor?: string[];
 } = {}) {
   const inspectCalls: string[] = [];
   const surface: any = {
@@ -130,10 +139,16 @@ function automation(opts: {
       return opts.history?.[runId] ?? null;
     },
   };
-  if (opts.repairability !== undefined || opts.repairabilityThrows) {
+  if (
+    opts.repairability !== undefined || opts.repairabilityThrows
+    || opts.repairabilityThrowsFor || opts.repairabilityMalformedFor
+  ) {
     surface.inspectConsumedSuspension = async (runId: string): Promise<Verdict> => {
       inspectCalls.push(runId);
-      if (opts.repairabilityThrows) throw new Error('run history unreadable for the consumed suspension');
+      if (opts.repairabilityThrows || opts.repairabilityThrowsFor?.includes(runId)) {
+        throw new Error('run history unreadable for the consumed suspension');
+      }
+      if (opts.repairabilityMalformedFor?.includes(runId)) return undefined as unknown as Verdict;
       const v = opts.repairability?.[runId];
       if (!v) throw new Error(`test surface: no verdict scripted for ${runId}`);
       return v;
@@ -501,10 +516,26 @@ describe('#15358 — the third oracle splits `failed` three ways, and its ABSENC
     expect(out.undetermined).toBe(0);
   });
 
-  it('a THROWN read is `undetermined`, exactly like the other two oracles — never a verdict', async () => {
+  it('⭐ [#16709 item 2] a THROWN read keeps the row REPORTED as `failed` — it never leaves the list', async () => {
+    // ⚠️ This assertion USED TO READ `expect(out.stranded).toEqual([])`: a
+    // thrown third read was counted `undetermined` and the row dropped, as for
+    // the other two oracles. Ruled the other way (PM seat, 2026-09-08).
+    //
+    // The two earlier oracles and this one are not asked the same question. A
+    // thrown `hasSuspendedRun` or `getRun` leaves it unknown WHETHER the row is
+    // stranded at all, and a storage outage must not be published as a lost
+    // run. By the time this oracle is asked, both have already answered: no
+    // live pause, terminal `failed`. It is asked only WHICH of the three
+    // shapes — so a read that could not be made is the textbook "could not
+    // differentiate", which is exactly what `'failed'` is kept for (#15358
+    // ruling, item 1). Dropping the row would let "nothing stranded" read TRUE
+    // while a row is in fact stuck, with a log line as its only trace; for a
+    // REPORT, fail-closed means showing the row.
     svc.attachAutomation(automation({ ...failedRun, repairabilityThrows: true }));
     const out = await svc.inspectStrandedRequests();
-    expect(out.stranded).toEqual([]);
+    expect(out.stranded.map(s => [s.requestId, s.runState])).toEqual([['areq_1', 'failed']]);
+    // The counter is KEPT, as telemetry — it and `stranded` now overlap by
+    // design, and neither alone sizes the scan's blind spot.
     expect(out.undetermined).toBe(1);
   });
 
@@ -589,3 +620,133 @@ describe('#15358 — the third oracle splits `failed` three ways, and its ABSENC
     expect(JSON.stringify(engine._tables)).toBe(before);
   });
 });
+
+// ── #16709: a failure to DIFFERENTIATE never costs a row its place, and never
+//    costs another row its answer ─────────────────────────────────────────────
+//
+// Two residues of the #15358 contract review, ruled together (PM seat,
+// 2026-09-08):
+//
+//   item 2 — a thrown third read counted `undetermined` and DROPPED the row.
+//   item 3 — `refineFailedRunState(verdict)` ran OUTSIDE the `try`, so a host
+//            that violates its own declared surface by resolving `undefined`
+//            threw a `TypeError` out of `inspectStrandedRequests` and the scan
+//            enumerated NOTHING.
+//
+// Both are the same mistake at two altitudes: this method exists to enumerate
+// the rows that cannot advance, so a row it could not differentiate stays in
+// the report as the undifferentiated `'failed'`, and a row it could not read
+// at all costs no OTHER row its answer. ⛔ Neither is a new `StrandedRunState`
+// member: `'failed'` already means "reported, could not differentiate".
+
+describe('#16709 — a failure to differentiate keeps the row, and stays local to it', () => {
+  let engine: ReturnType<typeof makeFakeEngine>;
+  let svc: ApprovalService;
+
+  beforeEach(() => {
+    engine = makeFakeEngine();
+    svc = new ApprovalService({ engine: engine as any });
+    engine._tables['sys_approval_request'] = [requestRow()];
+  });
+
+  const failedRun = { history: { run_1: { status: 'failed' as const } } };
+
+  it('⭐ item 3 — a host resolving `undefined` is answered, not thrown out of the scan', async () => {
+    // The declared surface says this member resolves a verdict. A host that
+    // resolves `undefined` breaks that — and `refineFailedRunState` reads
+    // `verdict.repairable`, so the old code's `TypeError` escaped the method.
+    svc.attachAutomation(automation({ ...failedRun, repairabilityMalformedFor: ['run_1'] }));
+    await expect(svc.inspectStrandedRequests()).resolves.toMatchObject({ scanned: 1, undetermined: 1 });
+    const out = await svc.inspectStrandedRequests();
+    // Same disposition as a thrown read: reported, undifferentiated.
+    expect(out.stranded.map(s => [s.requestId, s.runState])).toEqual([['areq_1', 'failed']]);
+  });
+
+  it('⭐ items 2+3 — one bad row costs ITSELF a label and every other row nothing', async () => {
+    // The harm the two items share, measured on one population: before the
+    // fix the malformed row alone turned this whole call into a rejection, so
+    // `areq_ok` — a perfectly readable, perfectly repairable strand — was
+    // never enumerated either. A PARTIAL answer became NO answer.
+    engine._tables['sys_approval_request'] = [
+      requestRow({ id: 'areq_throw', flow_run_id: 'run_throw' }),
+      requestRow({ id: 'areq_malformed', flow_run_id: 'run_malformed' }),
+      requestRow({ id: 'areq_ok', flow_run_id: 'run_ok' }),
+      requestRow({ id: 'areq_missing', flow_run_id: 'run_missing' }),
+    ];
+    const auto = automation({
+      history: {
+        run_throw: { status: 'failed' },
+        run_malformed: { status: 'failed' },
+        run_ok: { status: 'failed' },
+        // `run_missing` absent on purpose — it never reaches the third oracle.
+      },
+      repairability: { run_ok: { repairable: true } },
+      repairabilityThrowsFor: ['run_throw'],
+      repairabilityMalformedFor: ['run_malformed'],
+    });
+    svc.attachAutomation(auto);
+
+    const out = await svc.inspectStrandedRequests();
+    expect(out.scanned).toBe(4);
+    expect(out.stranded.map(s => [s.requestId, s.runState])).toEqual([
+      ['areq_throw', 'failed'],
+      ['areq_malformed', 'failed'],
+      ['areq_ok', 'repairable'],
+      ['areq_missing', 'missing'],
+    ]);
+    // Both undifferentiated rows are counted, and only those two.
+    expect(out.undetermined).toBe(2);
+    // The third oracle really was reached for each `failed` row, and only
+    // those — so the labels above are its answers, not a skipped branch.
+    expect(auto.inspectCalls).toEqual(['run_throw', 'run_malformed', 'run_ok']);
+  });
+
+  it('⛔ item 2 does NOT widen to the two earlier oracles — those still SKIP their row', async () => {
+    // The control that makes the ruling legible. The distinction is not "a
+    // throw is fine now": it is WHICH question was being asked. A thrown first
+    // or second oracle leaves it unknown whether the row is stranded at all,
+    // and condemning on an outage is the harm those arms were written for.
+    engine._tables['sys_approval_request'] = [requestRow({ id: 'areq_h', flow_run_id: 'run_h' })];
+    svc.attachAutomation(automation({ suspendedThrows: true }));
+    expect(await svc.inspectStrandedRequests()).toMatchObject({ scanned: 1, stranded: [], undetermined: 1 });
+
+    svc.attachAutomation(automation({ historyThrows: true }));
+    expect(await svc.inspectStrandedRequests()).toMatchObject({ scanned: 1, stranded: [], undetermined: 1 });
+
+    // Positive control on the same row: with both stores readable and only the
+    // THIRD read failing, the row IS reported — so the empty lists above are
+    // those two oracles' posture, not a row that was never strandable.
+    svc.attachAutomation(automation({
+      history: { run_h: { status: 'failed' } }, repairabilityThrowsFor: ['run_h'],
+    }));
+    const out = await svc.inspectStrandedRequests();
+    expect(out.stranded.map(s => s.runState)).toEqual(['failed']);
+    expect(out.undetermined).toBe(1);
+  });
+
+  it('⛔ still no sixth `StrandedRunState`: the undifferentiated rows are literally `failed`', async () => {
+    // Item 2's ruling is a re-use of an existing member, not a new one — the
+    // reason it touches no barrel-exported type. Every label this scan can
+    // emit is one of the five, and both undifferentiated shapes emit the same
+    // string an ABSENT member emits.
+    engine._tables['sys_approval_request'] = [
+      requestRow({ id: 'areq_absent', flow_run_id: 'run_absent' }),
+      requestRow({ id: 'areq_throw', flow_run_id: 'run_throw' }),
+      requestRow({ id: 'areq_malformed', flow_run_id: 'run_malformed' }),
+    ];
+    const history = {
+      run_absent: { status: 'failed' }, run_throw: { status: 'failed' }, run_malformed: { status: 'failed' },
+    };
+    // The member is absent for `run_absent`'s scan…
+    svc.attachAutomation(automation({ history }));
+    const blind = await svc.inspectStrandedRequests();
+    // …and present-but-failing for the other two.
+    svc.attachAutomation(automation({
+      history, repairabilityThrowsFor: ['run_throw', 'run_absent'],
+      repairabilityMalformedFor: ['run_malformed'],
+    }));
+    const failing = await svc.inspectStrandedRequests();
+
+    expect(new Set([...blind.stranded, ...failing.stranded].map(s => s.runState))).toEqual(new Set(['failed']));
+  });
+});