Skip to content

Commit 8c7cca1

Browse files
Trumpclaude
andauthored
fix(approvals): keep an undifferentiable stranded row in the report, and stop a malformed host verdict aborting the scan (#16739)
Three residues of the #15358 contract review (#16709). Item 1 (test-only) — the restore verb's drop of a stale hot consumed-suspension copy was unpinned package-wide: the reviewer's E2 ablation deleted the behaviour and left the whole service-automation suite green. Pinned where the drop is distinguishable from a no-op — after the durable row that proves the copy stale is evicted by run-history retention, a kept copy would be the only witness left and would offer an operator a restore of a run that already COMPLETED on another replica. Item 2 (PM ruling, 2026-09-08) — a thrown third read counted `undetermined` and dropped the row. By the time that oracle is asked the first two have already answered (no live pause, terminal `failed`); it is asked only WHICH of the three shapes the row is, so a read that could not be made is exactly the "could not differentiate" case `'failed'` already means. The row now stays in the report; `undetermined` is kept as telemetry. Item 3 — `refineFailedRunState(verdict)` ran outside the `try`, so a host resolving `undefined` threw a `TypeError` out of `inspectStrandedRequests` and the scan enumerated nothing. The refinement now runs inside that `try`: a malformed verdict costs its own row the differentiation and no other row anything. No new `StrandedRunState` member and no widened export. Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 Co-authored-by: Claude <noreply@anthropic.com>
1 parent f6480bc commit 8c7cca1

4 files changed

Lines changed: 457 additions & 21 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
"@objectstack/plugin-approvals": patch
3+
---
4+
5+
`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).
6+
7+
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.
8+
9+
- **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.
10+
- **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.
11+
12+
⛔ 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‎

Lines changed: 58 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -210,8 +210,11 @@ export interface ApprovalResumeSurface {
210210
* dead: the #15555 false-negative harm, one surface over) and ⛔ never
211211
* skipped (that hides the row). Absence of the discriminator is not
212212
* evidence of anything. Rejects when a store cannot be read; the inspection
213-
* counts such a row `undetermined`, exactly as it does a thrown
214-
* {@link hasSuspendedRun}.
213+
* counts such a row `undetermined` — but ⛔ unlike a thrown
214+
* {@link hasSuspendedRun} it does NOT drop the row, because this oracle is
215+
* asked only WHICH shape a row already known to be stranded is (#16709).
216+
* A host that resolves a malformed verdict is treated the same way, and
217+
* costs no OTHER row its answer.
215218
*/
216219
inspectConsumedSuspension?(runId: string): Promise<
217220
| { repairable: true }
@@ -495,13 +498,15 @@ function refineFailedRunState(verdict: ConsumedSuspensionVerdict): StrandedRunSt
495498
* `NO_CONSUMED_SUSPENSION` covers all three and does not say which. The
496499
* label is faithful to the verb: nothing re-arms it; the remedy is a new
497500
* run, not a restore.
498-
* - `failed` — the engine COULD NOT BE ASKED which of the three it is: the
499-
* attached surface has no `inspectConsumedSuspension` (an engine build
500-
* older than this plugin, or a test double). Today's undifferentiated
501-
* label, kept on purpose as the fail-closed fallback (#15358 ruling, item
502-
* 1): absence of the discriminator is not evidence, so the row is reported
503-
* and its repairability left unstated — ⛔ never `unrepairable`, ⛔ never
504-
* dropped from the report.
501+
* - `failed` — the engine COULD NOT BE ASKED which of the three it is, or
502+
* was asked and could not answer. Three ways in: the attached surface has
503+
* no `inspectConsumedSuspension` (an engine build older than this plugin,
504+
* or a test double); the read THREW (a store outage); or the host resolved
505+
* a malformed verdict, violating its own declared surface (#16709). Today's
506+
* undifferentiated label, kept on purpose as the fail-closed fallback
507+
* (#15358 ruling, item 1): a failure to differentiate is not evidence, so
508+
* the row is reported and its repairability left unstated — ⛔ never
509+
* `unrepairable`, ⛔ never dropped from the report.
505510
*
506511
* ⚠️ This names the shapes for the REPORT only. It is not a run state: the
507512
* engine's own vocabulary is still `'completed' | 'paused' | 'failed'`
@@ -4394,7 +4399,10 @@ export class ApprovalService implements IApprovalService {
43944399
* `'snapshot_dropped'` and `'unrepairable'` (see {@link StrandedRunState}).
43954400
* A surface without that member leaves the row `'failed'` — reported,
43964401
* undifferentiated — because absence of the discriminator is not evidence
4397-
* of anything; a thrown read counts `undetermined`, like the other two.
4402+
* of anything. So does a read that THREW or answered a malformed verdict
4403+
* (#16709): by the time this oracle is asked the row is already known to be
4404+
* stranded, so a failure to differentiate it is not a reason to drop it from
4405+
* a report — it is counted `undetermined` as telemetry AND reported.
43984406
*
43994407
* ⚠️ **What this can and cannot size.** It makes the condition *visible* in a
44004408
* deployment; it is not itself a census, and it says nothing about this
@@ -4412,7 +4420,16 @@ export class ApprovalService implements IApprovalService {
44124420
async inspectStrandedRequests(options?: { limit?: number }): Promise<{
44134421
scanned: number;
44144422
stranded: StrandedApprovalRequest[];
4415-
/** Rows skipped because the suspension store could not be read — NOT healthy, just unknown. */
4423+
/**
4424+
* Reads that could not be MADE — telemetry, ⛔ never a verdict and ⛔ never
4425+
* a "healthy" number. A thrown first or second oracle SKIPS its row
4426+
* (whether that row is stranded at all is then unknown, and a storage
4427+
* outage must not be published as a lost run); a thrown or malformed THIRD
4428+
* read leaves its row in `stranded` as the undifferentiated `'failed'` and
4429+
* is counted here as well — the row is known to be stranded, only its
4430+
* shape could not be told (#16709). So this counter and `stranded.length`
4431+
* overlap on purpose, and neither one alone sizes the scan's blind spot.
4432+
*/
44164433
undetermined: number;
44174434
}> {
44184435
const empty = { scanned: 0, stranded: [] as StrandedApprovalRequest[], undetermined: 0 };
@@ -4480,19 +4497,43 @@ export class ApprovalService implements IApprovalService {
44804497
// the other two oracles. See `refineFailedRunState` and
44814498
// `StrandedRunState` for the three answers and why none is folded.
44824499
if (runState === 'failed' && typeof this.automation.inspectConsumedSuspension === 'function') {
4483-
let verdict: ConsumedSuspensionVerdict;
4500+
// ⚠️ [#16709 item 3] The REFINEMENT runs inside this `try`, with the
4501+
// read it refines. `refineFailedRunState` dereferences the verdict, so
4502+
// a host that violates the declared surface — resolving `undefined`
4503+
// where a verdict is declared — used to throw a `TypeError` out of
4504+
// `inspectStrandedRequests` itself, turning a PARTIAL answer into NO
4505+
// answer for every OTHER row in the scan. Enumerating the rows that
4506+
// cannot advance is this method's entire purpose, so a misbehaving
4507+
// implementation must cost at most the differentiation of its own row.
4508+
let refined: StrandedRunState | undefined;
4509+
let differentiated = true;
44844510
try {
4485-
verdict = await this.automation.inspectConsumedSuspension(runId);
4511+
refined = refineFailedRunState(await this.automation.inspectConsumedSuspension(runId));
44864512
} catch (err: any) {
4513+
// [#16709 item 2 — PM ruling, 2026-09-08] The row STAYS in the
4514+
// report, as the undifferentiated `'failed'`. This oracle is not
4515+
// asked WHETHER the row is stranded: the first two already answered
4516+
// that (no live pause, terminal `failed`). It is asked only WHICH of
4517+
// the three shapes it is — so a read that could not be made is the
4518+
// textbook "could not differentiate" case, which is exactly what
4519+
// `'failed'` is kept for (#15358 ruling, item 1).
4520+
//
4521+
// ⛔ Never dropped from the list. This is a REPORT of rows that
4522+
// cannot advance, and a row whose state we failed to determine is
4523+
// precisely the row an operator has to see; skipping it would make
4524+
// "nothing stranded" read TRUE while a row is in fact stuck, with
4525+
// the only trace a log line nobody is paging on. `undetermined`
4526+
// still counts it, as telemetry — never as a verdict.
4527+
differentiated = false;
44874528
undetermined++;
44884529
this.logger?.warn?.('[approvals] stranded-request scan could not read the consumed-suspension state', {
44894530
request: raw?.id, run: runId, error: err?.message ?? String(err),
44904531
});
4491-
continue;
44924532
}
4493-
const refined = refineFailedRunState(verdict);
4494-
if (!refined) continue; // re-armed between the two reads — alive after all
4495-
runState = refined;
4533+
if (differentiated) {
4534+
if (!refined) continue; // re-armed between the two reads — alive after all
4535+
runState = refined;
4536+
}
44964537
}
44974538

44984539
// Neither suspended nor recoverable: the run this decision was supposed to

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

Lines changed: 165 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,15 @@ function automation(opts: {
116116
historyThrows?: boolean;
117117
repairability?: Record<string, Verdict>;
118118
repairabilityThrows?: boolean;
119+
/** Runs whose third-oracle read THROWS — a store outage on those rows alone. */
120+
repairabilityThrowsFor?: string[];
121+
/**
122+
* Runs whose host RESOLVES `undefined` — a contract-violating implementation
123+
* of its own declared surface (#16709 item 3). ⛔ Deliberately outside
124+
* `Verdict`: pinning what happens when a host lies is the whole point, and
125+
* the cast that makes it expressible is confined to this double.
126+
*/
127+
repairabilityMalformedFor?: string[];
119128
} = {}) {
120129
const inspectCalls: string[] = [];
121130
const surface: any = {
@@ -130,10 +139,16 @@ function automation(opts: {
130139
return opts.history?.[runId] ?? null;
131140
},
132141
};
133-
if (opts.repairability !== undefined || opts.repairabilityThrows) {
142+
if (
143+
opts.repairability !== undefined || opts.repairabilityThrows
144+
|| opts.repairabilityThrowsFor || opts.repairabilityMalformedFor
145+
) {
134146
surface.inspectConsumedSuspension = async (runId: string): Promise<Verdict> => {
135147
inspectCalls.push(runId);
136-
if (opts.repairabilityThrows) throw new Error('run history unreadable for the consumed suspension');
148+
if (opts.repairabilityThrows || opts.repairabilityThrowsFor?.includes(runId)) {
149+
throw new Error('run history unreadable for the consumed suspension');
150+
}
151+
if (opts.repairabilityMalformedFor?.includes(runId)) return undefined as unknown as Verdict;
137152
const v = opts.repairability?.[runId];
138153
if (!v) throw new Error(`test surface: no verdict scripted for ${runId}`);
139154
return v;
@@ -501,10 +516,26 @@ describe('#15358 — the third oracle splits `failed` three ways, and its ABSENC
501516
expect(out.undetermined).toBe(0);
502517
});
503518

504-
it('a THROWN read is `undetermined`, exactly like the other two oracles — never a verdict', async () => {
519+
it('⭐ [#16709 item 2] a THROWN read keeps the row REPORTED as `failed` — it never leaves the list', async () => {
520+
// ⚠️ This assertion USED TO READ `expect(out.stranded).toEqual([])`: a
521+
// thrown third read was counted `undetermined` and the row dropped, as for
522+
// the other two oracles. Ruled the other way (PM seat, 2026-09-08).
523+
//
524+
// The two earlier oracles and this one are not asked the same question. A
525+
// thrown `hasSuspendedRun` or `getRun` leaves it unknown WHETHER the row is
526+
// stranded at all, and a storage outage must not be published as a lost
527+
// run. By the time this oracle is asked, both have already answered: no
528+
// live pause, terminal `failed`. It is asked only WHICH of the three
529+
// shapes — so a read that could not be made is the textbook "could not
530+
// differentiate", which is exactly what `'failed'` is kept for (#15358
531+
// ruling, item 1). Dropping the row would let "nothing stranded" read TRUE
532+
// while a row is in fact stuck, with a log line as its only trace; for a
533+
// REPORT, fail-closed means showing the row.
505534
svc.attachAutomation(automation({ ...failedRun, repairabilityThrows: true }));
506535
const out = await svc.inspectStrandedRequests();
507-
expect(out.stranded).toEqual([]);
536+
expect(out.stranded.map(s => [s.requestId, s.runState])).toEqual([['areq_1', 'failed']]);
537+
// The counter is KEPT, as telemetry — it and `stranded` now overlap by
538+
// design, and neither alone sizes the scan's blind spot.
508539
expect(out.undetermined).toBe(1);
509540
});
510541

@@ -589,3 +620,133 @@ describe('#15358 — the third oracle splits `failed` three ways, and its ABSENC
589620
expect(JSON.stringify(engine._tables)).toBe(before);
590621
});
591622
});
623+
624+
// ── #16709: a failure to DIFFERENTIATE never costs a row its place, and never
625+
// costs another row its answer ─────────────────────────────────────────────
626+
//
627+
// Two residues of the #15358 contract review, ruled together (PM seat,
628+
// 2026-09-08):
629+
//
630+
// item 2 — a thrown third read counted `undetermined` and DROPPED the row.
631+
// item 3 — `refineFailedRunState(verdict)` ran OUTSIDE the `try`, so a host
632+
// that violates its own declared surface by resolving `undefined`
633+
// threw a `TypeError` out of `inspectStrandedRequests` and the scan
634+
// enumerated NOTHING.
635+
//
636+
// Both are the same mistake at two altitudes: this method exists to enumerate
637+
// the rows that cannot advance, so a row it could not differentiate stays in
638+
// the report as the undifferentiated `'failed'`, and a row it could not read
639+
// at all costs no OTHER row its answer. ⛔ Neither is a new `StrandedRunState`
640+
// member: `'failed'` already means "reported, could not differentiate".
641+
642+
describe('#16709 — a failure to differentiate keeps the row, and stays local to it', () => {
643+
let engine: ReturnType<typeof makeFakeEngine>;
644+
let svc: ApprovalService;
645+
646+
beforeEach(() => {
647+
engine = makeFakeEngine();
648+
svc = new ApprovalService({ engine: engine as any });
649+
engine._tables['sys_approval_request'] = [requestRow()];
650+
});
651+
652+
const failedRun = { history: { run_1: { status: 'failed' as const } } };
653+
654+
it('⭐ item 3 — a host resolving `undefined` is answered, not thrown out of the scan', async () => {
655+
// The declared surface says this member resolves a verdict. A host that
656+
// resolves `undefined` breaks that — and `refineFailedRunState` reads
657+
// `verdict.repairable`, so the old code's `TypeError` escaped the method.
658+
svc.attachAutomation(automation({ ...failedRun, repairabilityMalformedFor: ['run_1'] }));
659+
await expect(svc.inspectStrandedRequests()).resolves.toMatchObject({ scanned: 1, undetermined: 1 });
660+
const out = await svc.inspectStrandedRequests();
661+
// Same disposition as a thrown read: reported, undifferentiated.
662+
expect(out.stranded.map(s => [s.requestId, s.runState])).toEqual([['areq_1', 'failed']]);
663+
});
664+
665+
it('⭐ items 2+3 — one bad row costs ITSELF a label and every other row nothing', async () => {
666+
// The harm the two items share, measured on one population: before the
667+
// fix the malformed row alone turned this whole call into a rejection, so
668+
// `areq_ok` — a perfectly readable, perfectly repairable strand — was
669+
// never enumerated either. A PARTIAL answer became NO answer.
670+
engine._tables['sys_approval_request'] = [
671+
requestRow({ id: 'areq_throw', flow_run_id: 'run_throw' }),
672+
requestRow({ id: 'areq_malformed', flow_run_id: 'run_malformed' }),
673+
requestRow({ id: 'areq_ok', flow_run_id: 'run_ok' }),
674+
requestRow({ id: 'areq_missing', flow_run_id: 'run_missing' }),
675+
];
676+
const auto = automation({
677+
history: {
678+
run_throw: { status: 'failed' },
679+
run_malformed: { status: 'failed' },
680+
run_ok: { status: 'failed' },
681+
// `run_missing` absent on purpose — it never reaches the third oracle.
682+
},
683+
repairability: { run_ok: { repairable: true } },
684+
repairabilityThrowsFor: ['run_throw'],
685+
repairabilityMalformedFor: ['run_malformed'],
686+
});
687+
svc.attachAutomation(auto);
688+
689+
const out = await svc.inspectStrandedRequests();
690+
expect(out.scanned).toBe(4);
691+
expect(out.stranded.map(s => [s.requestId, s.runState])).toEqual([
692+
['areq_throw', 'failed'],
693+
['areq_malformed', 'failed'],
694+
['areq_ok', 'repairable'],
695+
['areq_missing', 'missing'],
696+
]);
697+
// Both undifferentiated rows are counted, and only those two.
698+
expect(out.undetermined).toBe(2);
699+
// The third oracle really was reached for each `failed` row, and only
700+
// those — so the labels above are its answers, not a skipped branch.
701+
expect(auto.inspectCalls).toEqual(['run_throw', 'run_malformed', 'run_ok']);
702+
});
703+
704+
it('⛔ item 2 does NOT widen to the two earlier oracles — those still SKIP their row', async () => {
705+
// The control that makes the ruling legible. The distinction is not "a
706+
// throw is fine now": it is WHICH question was being asked. A thrown first
707+
// or second oracle leaves it unknown whether the row is stranded at all,
708+
// and condemning on an outage is the harm those arms were written for.
709+
engine._tables['sys_approval_request'] = [requestRow({ id: 'areq_h', flow_run_id: 'run_h' })];
710+
svc.attachAutomation(automation({ suspendedThrows: true }));
711+
expect(await svc.inspectStrandedRequests()).toMatchObject({ scanned: 1, stranded: [], undetermined: 1 });
712+
713+
svc.attachAutomation(automation({ historyThrows: true }));
714+
expect(await svc.inspectStrandedRequests()).toMatchObject({ scanned: 1, stranded: [], undetermined: 1 });
715+
716+
// Positive control on the same row: with both stores readable and only the
717+
// THIRD read failing, the row IS reported — so the empty lists above are
718+
// those two oracles' posture, not a row that was never strandable.
719+
svc.attachAutomation(automation({
720+
history: { run_h: { status: 'failed' } }, repairabilityThrowsFor: ['run_h'],
721+
}));
722+
const out = await svc.inspectStrandedRequests();
723+
expect(out.stranded.map(s => s.runState)).toEqual(['failed']);
724+
expect(out.undetermined).toBe(1);
725+
});
726+
727+
it('⛔ still no sixth `StrandedRunState`: the undifferentiated rows are literally `failed`', async () => {
728+
// Item 2's ruling is a re-use of an existing member, not a new one — the
729+
// reason it touches no barrel-exported type. Every label this scan can
730+
// emit is one of the five, and both undifferentiated shapes emit the same
731+
// string an ABSENT member emits.
732+
engine._tables['sys_approval_request'] = [
733+
requestRow({ id: 'areq_absent', flow_run_id: 'run_absent' }),
734+
requestRow({ id: 'areq_throw', flow_run_id: 'run_throw' }),
735+
requestRow({ id: 'areq_malformed', flow_run_id: 'run_malformed' }),
736+
];
737+
const history = {
738+
run_absent: { status: 'failed' }, run_throw: { status: 'failed' }, run_malformed: { status: 'failed' },
739+
};
740+
// The member is absent for `run_absent`'s scan…
741+
svc.attachAutomation(automation({ history }));
742+
const blind = await svc.inspectStrandedRequests();
743+
// …and present-but-failing for the other two.
744+
svc.attachAutomation(automation({
745+
history, repairabilityThrowsFor: ['run_throw', 'run_absent'],
746+
repairabilityMalformedFor: ['run_malformed'],
747+
}));
748+
const failing = await svc.inspectStrandedRequests();
749+
750+
expect(new Set([...blind.stranded, ...failing.stranded].map(s => s.runState))).toEqual(new Set(['failed']));
751+
});
752+
});

0 commit comments

Comments
 (0)