Skip to content

fix(approvals): recall tells its caller WHICH resume failure stranded the run - #17988

Merged
os-project-manager merged 4 commits into
mainfrom
claude/issue-15970-recall-resume-failure
Sep 13, 2026
Merged

os-project-manager merged 4 commits into
mainfrom
claude/issue-15970-recall-resume-failure

Conversation

@claude

@claude claude Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #15970

Clause-②: no

recall resumes its run DIRECTLY rather than through resumeRecordedOutcome, so the resumeCode / resumeStatus the engine already stamps on the error were discarded one line before the result was built. The caller got resumed: false plus one sentence of prose and no way to learn the run is repairable — while the identical strand reached through decide has carried { finalized, decision, runId, repairable: true } since #13807. The difference was the door, not the strand.

This fills ApprovalRecallResult.resumeFailure, a slot @objectstack/spec already declared (contracts/approval-service.ts) and left without a producer on this door. Zero packages/spec files touched.

The card's table, reproduced BEFORE the change

Real AutomationEngine + InMemorySuspendedRunStore + real ApprovalService + real registerApprovalNode, one live process, in-memory ObjectQL double for the approvals tables only. deal_approval parks at an approval node; mark_rejected throws the card's own text. Verbatim from the probe's stdout, worktree at c8a006fc4:

read result
the call no throw — returned normally
result.resumed false
result.resumeError "resume of run 'run_a2cb9106…' failed: update_record(crm_leave_request) failed: Record 9SEmlyRfw8D9-J7Z not found"
result.resumeFailure undefined
result.request.status "recalled"
strandedDecisionDetails(result) undefined
engine.hasSuspendedRun(runId) false
'resumeFailure' in result false
engine.restoreConsumedSuspension(runId) {"restored":true,"reason":"… the run is resumable again; re-issue the continuation"}
marks (downstream work) []

CONTROL in the SAME run — the identical strand through decide:

read result
the call THREW "RESUME_FAILED: the reject decision was recorded on request areq_7b2f3a61…, but its flow run 'run_f4601c41…' could not be resumed and is now stranded: …"
strandedDecisionDetails(err) {"finalized":true,"decision":"reject","runId":"run_f4601c41…","repairable":true}
engine.hasSuspendedRun(runId) false

⭐ One card row is corrected by measurement and stays corrected: strandedDecisionDetails(result) is undefined, not null — and that is not something this PR could have fixed. It is the ERROR-envelope reader for a door that THROWS; against a recall RESULT object it can only ever answer undefined, whatever the door does. The ruling put the success-answer carrier on resumeFailure for exactly that reason (the spec's own "⛔ Not StrandedDecisionDetails" note).

The same table AFTER, same harness, same lever

read result
the call no throw — returned normally (unchanged, and asserted so it stays unchanged)
result.resumed false
result.resumeError "resume of run 'run_d0bad676…' failed: update_record(crm_leave_request) failed: Record 9SEmlyRfw8D9-J7Z not found"
result.resumeFailure {"code":"RESUME_FAILED","runId":"run_d0bad676…","status":"stranded","repairable":true}
result.request.status "recalled"
strandedDecisionDetails(result) undefined (see above — correct, not unfixed)
engine.hasSuspendedRun(runId) false
'resumeFailure' in result true
engine.restoreConsumedSuspension(runId) {"restored":true,…} — the repair repairable: true promises, cashed in the pin
marks (downstream work) []

The decide control is byte-identical in the after run, which is the point: the harness discriminates the two doors rather than passing vacuously.

What changed

recall's non-revise-window arm, and nothing else:

Fences, each one measured rather than assumed

  • The no-throw stays. PIN 1 asserts the call returns normally; a fix that made recall fail would be the wrong fix, not a stricter one.
  • 🔴 RESUME_FAILED and only that code. PIN 1 asserts the literal. Two exits report; every other exit deliberately reports NOTHING, and PIN 3 pins that: a lost run's honest code is RESUME_TARGET_LOST and the tolerated duplicate's is RESUME_IN_PROGRESS, and this package's ledger row admits neither — its own comment spells out that this package stamps neither in a scanned position. Stamping RESUME_FAILED there would make the discriminator lie about WHICH failure it was, which is the defect being fixed, one field over. Absence is declared legitimate by the member's docblock ("An absent member means no report was made, never that no run is stranded").
  • Nothing stopped at the packages/spec fence and nothing needed to. The honest code for both exits this PR fills is RESUME_FAILED, which the ledger already admits. See Acceptance notes for the one adjacent observation.
  • The log line is untouched — same message, same error level, same three context keys, asserted in PIN 1. The ruling left logging alone; the report is a sibling of that line, not a replacement.
  • content/docs/releases/** untouched. packages/spec untouched (0 files).

Tests

New packages/plugins/plugin-approvals/src/recall-strand-discriminator.test.ts — PIN 1 (the card's table row for row), PIN 2 (a healthy recall, so PIN 1's presence is a reading and not the fixture), PIN 3 (the non-stranded exit that reports no code, with its reason), CONTROL (the identical strand through decide). subflow-hosted-approval-strand.test.ts gains the recall case for the #15556 shape.

Ablation — the pins go red on the defect. Mutation = restoring the pre-fix production file from c8a006fc4, hash-verified on disk both ways, under a trap … EXIT INT TERM with an absolute repo root.

HEAD blob fa932e35dd1f33848b8b8bf71bf2bcde1b67af65 / BASE blob 9a68d3685bb6aa29fd47718071dd9515c7c2a80b
LEG 0 clean    on-disk fa932e35 · marker 1 · pre-fix guard line 0 · 8 passed
LEG 1 mutated  on-disk 9a68d368 (== BASE) · marker 0 · pre-fix guard line 1
               vitest exit=1 — Test Files 2 failed (2), Tests 2 failed | 6 passed (8)
               AssertionError: expected undefined to deeply equal { code: 'RESUME_FAILED', …(3) }   x2
LEG 2 restored on-disk fa932e35 (== HEAD) · marker 1 · `git diff HEAD` empty · `git status --porcelain` empty
               vitest exit=0 — Test Files 2 passed (2), Tests 8 passed (8)

Predicted direction was RED and RED is what it did; exactly the two new resumeFailure assertions failed and the four controls stayed green. node scripts/ablation-dist-preflight.mjs @objectstack/plugin-approvals '…' --absent reported no dist/ in this package, which is the correct reading here rather than a void run: both test files import the mutated module RELATIVELY (./approval-service.js), so vitest resolves it from src and no dist sits on its resolution path. The hazard that script exists to stop is a false GREEN from a stale build; this ablation's observed colour is RED, which a stale build cannot produce.

Commands, all under the shared verify lock, VERDICT command-exit read rather than a bare $?:

  • pnpm --workspace-concurrency=2 --filter '@objectstack/plugin-approvals^...' build — exit 0 (dependency closure, built before any reading).
  • pnpm --filter @objectstack/plugin-approvals test — exit 0, 45 files / 738 tests pass (was 44 / 733 on main).
  • pnpm --filter @objectstack/plugin-approvals typecheck — exit 0; check:test-typecheck: OK — 8 file(s) / 324 error(s) / 27 pinned signature(s), the shrink-only ledger unchanged (the new test file is NOT in it; its two initial TS7006 were fixed by typing the recorder, not by expanding the ledger).
  • pnpm exec turbo run build --filter=!@objectstack/docs --concurrency=2 — exit 0, 73/73 tasks.
  • pnpm lint (eslint . --no-inline-config, the whole repo, no narrowing) — exit 0.

Gate families: 71 derived, 71 run, 0 NOT-MEASURED, 0 UNRUN, reconciled mechanically at 76ee9926e with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --ran FILE, every family recorded with its exit code:

✓ dispatch-gates --ran: 71 derived famil(ies) accounted for — 71 run,
  0 NOT-MEASURED (a DERIVED zero — all 71 recorded an exit code and none of them is 3).

Three of them answered exit 3 (PREREQUISITE NOT MET) on the first pass — check:dual-build-cjs-loads, check:i18n, check:type-check-debt — which is a statement about the tree and not a pass. The closure they name was built (the turbo run build line above) and all three were re-run to a real verdict: exit 0 each. check:engine-double-contract answered exit 1 with its own remedy (a new pinned engine double the ledger had not learned about); regenerated with --write, 0 seam rows added or grown, 0 lost, gate now exit 0.

The gate this lane was warned about, verbatim:

$ pnpm --filter @objectstack/spec check:error-code-provenance
> tsx scripts/check-error-code-provenance.ts --self-test && tsx scripts/check-error-code-provenance.ts

self-test OK — 3 patterns and every waiver direction pinned
bounds: packages/**/src non-test .ts/.tsx; ledger file excluded (mention, not stamping);
bounds: patterns = objlit, assign, constdef — blind to non-*_CODE constants,
bounds: helper/shorthand indirections, templates and class fields (see the header; sibling-gate shapes).
scanned 2304 files; 338 registered-code stamp site(s): 321 listed, 17 waived
OK — every registered-code stamp site is listed under its own owner key or carries a recorded waiver (10 waiver(s), all live)
exit=0

Clause-② — re-derived from the DELIVERED diff, not predicted

no. Five files: the production change, two test files, the changeset, and the gate ledger JSON.

  1. New exported symbols reachable from the published entry: zero. Every added line in the whole diff was searched for the word exportgit diff c8a006fc4..HEAD | grep '^+' | grep -w export returns nothing at all. src/index.ts is untouched (0 hits in the changed-file list) and so is package.json, whose exports map is a single . pointing at ./dist/index.*; the two test files are outside the src/index.ts build graph and outside the published files: ["dist", "README.md", "CHANGELOG.md"].
  2. New key on an already-published payload: none. resumeFailure was already declared, exported and type-pinned on ApprovalRecallResult in @objectstack/spec before this PR — resume-failure-report.pin.test.ts carries a type-level Eq assertion (named RecallCarriesTheReport) holding ApprovalRecallResult's resumeFailure member equal to ResumeFailureReport or undefined. This PR supplies its first producer. Per the dispatch skill's own boundary test, 「条款②只指已发布契约面,拉回已声明契约不触它」 — bringing an already-declared contract into effect does not touch Clause-②.

Neither the "an existing export's behaviour changed" reading nor wire compatibility is the floor, so neither was used. Changeset graded patch accordingly (@objectstack/plugin-approvals only).

Acceptance notes

Noted while working, not filed and not fixed here:

  • The RESUME_TARGET_LOST boundary is real but is not a defect. A recall whose run is already gone is still told in prose and carries no machine-readable half, because the honest code for it is one this package's ledger row does not admit. That is the row working as designed — its comment says so in as many words — and PIN 3 records the boundary so the next reader does not "fix" it by stamping the wrong code. If a consumer ever needs to branch on that exit, it is a packages/spec ledger event and belongs to the domain:spec seat, exactly as spec: ResumeFailureReport.code is a REQUIRED field naming RESUME_FAILED, but the provenance ledger owns that code under @objectstack/rest — so the only correct implementation is refused by check:error-code-provenance #17909 was routed. Nothing in this PR stopped at that fence.
  • recall's revise-window arm (cancelRun, ADR-0044) tolerates its own failure with prose and no report. A cancel is not a resume, so ResumeFailureReport has no referent there; left exactly as it was.
  • The missingRunCapability exit already spells [RESUME_FAILED] inside its prose for a composition with no engine, and resumeRecordedOutcome does not stamp a report there either. Left symmetric with the sibling door rather than diverging on this card.

Generated by Claude Code

… the run

`recall` resumes directly rather than through `resumeRecordedOutcome`, so the
`resumeCode` / `resumeStatus` the engine already stamps were dropped one line
before the result was built: the caller got `resumed: false` plus prose and no
way to learn the run is repairable. Fill the already-declared
`ApprovalRecallResult.resumeFailure` slot, on the two exits whose honest code
is `RESUME_FAILED` — this door's own strand, and a resume that succeeded while
the subflow parent above it stranded.

Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj
Co-authored-by: Claude <noreply@anthropic.com>
…h controls

The card's own table row for row (PIN 1), a healthy recall so the absence in
PIN 2 is a reading and not the fixture, the non-stranded exit that deliberately
reports no code (PIN 3), and the identical strand through `decide` as the
control that proves the harness discriminates the two doors. The #15556 subflow
fixture gains the recall case its spec docblock already declared.

Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj
Co-authored-by: Claude <noreply@anthropic.com>
…dger

`check:engine-double-contract` names its own remedy: a new test file that pins
its double to ObjectQL's dispatch predicates is new coverage the ledger has to
learn about, or it never protects that file. Regenerated with `--write`; 0 seam
rows added or grown, 0 lost.

Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj
Co-authored-by: Claude <noreply@anthropic.com>
`tsconfig.test.json` covers this file, so the two `filter` callbacks were
TS7006 implicit-any. Give `recordingLogger` a declared line shape instead of
annotating the parameters `any` — the debt ledger is shrink-only and a new
entry is a maintainer's call, not a way to get green.

Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-approvals, touching 4 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/automation/flows.mdx (via ApprovalService (symbol, a top-level class))
What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 6 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json eb9334915045aa197564bcf25d6dc3eff6746efepackageMentionDocs.

Which tree this was computed on

This run read content/docs from 96da2f18aaf746519ef18d1085f427c7a84bffb3 — the merge of head 76ee9926e554c84f15ce1638296973ea5b487773 into base eb9334915045aa197564bcf25d6dc3eff6746efe, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 96da2f18aaf746519ef18d1085f427c7a84bffb3 && git checkout 96da2f18aaf746519ef18d1085f427c7a84bffb3
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin eb9334915045aa197564bcf25d6dc3eff6746efe 76ee9926e554c84f15ce1638296973ea5b487773 && git checkout -B drift-repro eb9334915045aa197564bcf25d6dc3eff6746efe && git merge --no-ff 76ee9926e554c84f15ce1638296973ea5b487773

node scripts/docs-audit/affected-docs.mjs --json eb9334915045aa197564bcf25d6dc3eff6746efe

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs eb9334915045aa197564bcf25d6dc3eff6746efe → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 13, 2026
@claude

claude Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Contract review

Head reviewed: 76ee9926e554c84f15ce1638296973ea5b487773

Implemented-by: claude/issue-15970-recall-resume-failure (mode:subagent — the branch, not a session)
Reviewed-by: session_01URLHobLUJB9K1ABV6ofdjj (domain:services execution seat)

① Clause-② — re-derived in-seat, ⛔ not inherited and ⛔ not accepted from the report

reading result
added lines containing the token export (git diff | grep '^+' | grep -w export) 0
src/index.ts in the changed-file set absent ⇒ no re-export added
package.json in the changed-file set absent ⇒ the exports map did not move

Clause-②: no, changeset patch (correct — approval-service.ts compiles into the published dist, so skip-changeset would be wrong here, unlike #17982).

⭐ The delivery's framing of why is the part worth keeping: #17908 was minor because it ADDED AutomationEngine.takeSubflowParentStrand, the SubflowParentStrand interface and a new ApprovalResumeSurface member. This PR adds none of them — it CONSUMES them. And ApprovalRecallResult.resumeFailure was already declared, exported and type-pinned in packages/spec (contracts/resume-failure-report.pin.test.ts carries the RecallCarriesTheReport Eq assertion) before this PR existed. ⇒ supplying a declared contract's first producer is not a widening of the published surface. ⛔ Wire compatibility was not used as the floor, and neither was "an existing export's behaviour changed".

② 🔴 The trap this dispatch existed to prevent — navigated, ⛔ not stumbled into

The dispatch warned that the ADR-0112 ledger row admits exactly one code: #17924's cost-direction ablation measured that stamping RESUME_TARGET_LOST instead still reds the provenance guard.

Measured in-seat on the delivered diff:

  • two new stamp sites, and both are code: 'RESUME_FAILED';
  • RESUME_TARGET_LOST appears on 3 added lines — and 0 of them are a stamp (grep '^+' | grep RESUME_TARGET_LOST | grep -c 'code:'0). All three are prose: a changeset paragraph, a source comment, a test comment.

And the judgement behind that is the best call of the round. The RUN_NOT_FOUND exit's honest code would be RESUME_TARGET_LOST, which this package's row refuses. Rather than stamping RESUME_FAILED to fill the slot, the delivery reports nothing on that exit — because a wrong code there "would make the discriminator lie about WHICH failure it was, which is the defect this card fixes one field over." ⇒ filling a slot with a false value would have reproduced the very defect, one field across. PIN 3 pins that boundary so the next reader does not "fix" it with the wrong code, and the member's own docblock backs it: "an absent member means no report was made, never that no run is stranded."

✅ Provenance guard, verbatim on the final tree: 321 listed, 17 waivedOK, exit 0, captured by redirect before any pipe. ⛔ Nothing stopped at the packages/spec fence and nothing needed to; 0 spec files touched.

③ The ablation predicted its direction and got it

LEG 0 clean (on-disk blob fa932e35, 8 passed) → LEG 1 mutated back to the pre-fix file (9a68d368 == BASE, vitest exit 1: "Test Files 2 failed, Tests 2 failed | 6 passed", both failures expected undefined to deeply equal { code: 'RESUME_FAILED', … }) → LEG 2 restored (fa932e35 == HEAD, git diff HEAD empty, git status --porcelain empty, 8 passed). Under a trap ... EXIT INT TERM with an absolute repo root and a path-scoped git checkout HEAD -- PATH, ⛔ never a bare checkout.

Exactly the two new assertions failed and all four controls stayed green — a predicted direction, met. And the stale-dist hazard was addressed rather than skipped: the preflight reported "no dist/ in this package", and the delivery explains why that is the correct reading here (both test files import the mutated module relatively, so vitest resolves from src) and that the observed colour was RED, which a stale build cannot manufacture. ⛔ A tool answering "not applicable" was not treated as a pass.

④ Gates, and the exit-3 discipline held three times

71 derived, 71 run, 0 NOT-MEASURED, 0 UNRUN, reconciled mechanically with --ran so the zero is derived, ⛔ not claimed. Two families needed action rather than a pass:

  • check:engine-double-contract answered exit 1 naming its own remedy ⇒ regenerated with --write (0 added or grown, 0 lost) ⇒ exit 0. ✅ Survival proved by regenerating, ⛔ never by the merge exiting 0.
  • Three families answered exit 3 (PREREQUISITE NOT MET)check:dual-build-cjs-loads, check:i18n, check:type-check-debt. The closure they name was built (turbo run build, 73/73) and all three were re-run to a real verdict: exit 0 each. ✅ Exit 3 is a statement about the tree — ⛔ never a pass, ⛔ never an excuse.

⭐ The test-typecheck debt ledger is unchanged (8 files / 324 errors / 27 signatures): the new file's two initial TS7006 were fixed by typing the log recorder, ⛔ not by expanding a shrink-only, maintainer-only ledger.

Serial relay: this PR moves scripts/engine-double-contract.pinned.json (+11). Swept in-seat across every open PR — exactly one holder. One baton, no silent-loss exposure.

⭐ It corrected the card it was sent to implement

#15970's own table says strandedDecisionDetails(result)null. Measured: it is undefined — and more importantly the predicate reads a carrier off a thrown error, so against a recall result it can never discriminate, whatever this door does. ⇒ the row was not a bug to fix but a category error in the card, and it is recorded in PIN 1's comment rather than silently smoothed.

Verdict: PASS at 76ee9926e

Carrier gate: check-clause2-carriers --pair 17988exit 0, captured before any pipe. ⛔ recall's decision not to throw is untouched and now pinned; the error log line is untouched (same message, level and three context keys, asserted).

⚠️ Binds to the head it names. ⚠️ Landing waits on pre-check ③, measured at the workflow-run level per correction 162: CI and Lint & Type Check are still in progress.


Generated by Claude Code

@os-project-manager
os-project-manager marked this pull request as ready for review September 13, 2026 10:15
@os-project-manager
os-project-manager added this pull request to the merge queue Sep 13, 2026
Merged via the queue into main with commit 9fca8eb Sep 13, 2026
36 checks passed
@os-project-manager
os-project-manager deleted the claude/issue-15970-recall-resume-failure branch September 13, 2026 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

2 participants