fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500 - #15436

Draft
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope
Draft

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500#15436
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#13807

Implements the maintainer ruling of 2026-09-04 (decision batch #37, verbatim 「同意」 on 1B · 2及) — option B, the truthful envelope.

What the ruling changed, and what it deliberately did not

One POST /api/v1/approvals/requests/{id}/reject produced three coexisting outcomes: the caller read HTTP 500, the request row wasrejected and had left the pending inbox, and the workflow run was stranded. A caller — human, script, or agent — reads 500 as "the rejection did not happen" and retries or escalates. It did happen.

The change

serviceResume carries status. It read only success / code / error. The engine's stranded exit reports status: 'stranded' and no code at all, so a door reading only the code saw an unnamed failure and could not tell a repairable strand from a dead run. AutomationResult.status: 'stranded' had a producer (PR #15237) and zero consumers — this is its first.

The decision doors build the envelope.resumeRecordedOutcome now throws a carrier with finalized (the decision stands), decision, runId, and repairable derived from the engine's own discriminator. All four sibling doors on the same path pass their outcome label: decide (approve / reject), the auto-rejection, the send-back (revise) and the resubmit.

repairable is producer-first.true only when the engine said 'stranded' — the one exit that journals a repair snapshot. Every other failure, including a lost run and an engine too old to report a status, is honestly false: absence of the signal is not repairability, and a repair verb that would refuse is worse than no promise.

The REST door forwards the four fields, anchored to RESUME_FAILED and presence-gated. A RESUME_FAILED with no carrier answers exactly the body it always did — the door never synthesises the envelope.

Home rule.strandedDecisionFailure / strandedDecisionDetails live in @objectstack/types because the producer is a plugin and @objectstack/rest cannot import one — the same reasoning, and the same constructor-and-recogniser-in-one-module discipline, as the validation-failure pair beside it. That removes the stringly-typed cross-package agreement about a property name that nothing would have checked.

Measured before implementing — the PM's assumptions, verified rather than inherited

  1. serviceResume's cast (approval-service.ts:2668 on the base, not :2667) read exactly success / code / error. The literal object approvals receives on the stranded exit carries status: 'stranded' and no code — pinned in PIN 3 by intercepting the real engine's return value, so the pin reds if the producer's shape moves and not only the door's.
  2. 'stranded' had zero consumers outside service-automation and spec. Positive control RESUME_IN_PROGRESS, same grep shape, finds five directories including packages/runtime and packages/client — the scan is not over-filtered.
  3. ⚠️The generic door is NOT the same seam — measured, and therefore filed rather than fixed here. It is already filed as automation resume door: the 400 FLOW_FAILED envelope drops the engine's status: 'stranded' verdict — the wire mirror's member is unreachable on the wire #15221; the full measurement is recorded there. In short: no shared code (runtime/domains/automation.ts contains no serviceResume / RESUME_FAILED / handleApprovalError); a wire-shaping omission rather than a lossy relay; a different code, owner and status (400 FLOW_FAILED, registered to @objectstack/runtime); three of the four fields have no referent there; and an approval node cannot reach that arm at all, since resumeAuthority: 'service' makes the generic resume answer 403 first.
  4. The RESUME_FAILED-on-a-200-body pin is untouched, deliberately.approval-restart-resume.test.ts:272 pins the token appearing in resumeError on the no-engine composition. That path returns rather than throwing, so it builds no envelope and this change adds nothing to it — and it should stay that way: that outcome is not stranded and not repairable, so a repairable flag there would be a lie. Renaming the token on the 200 body would be a separate contract change nobody ruled on. The full plugin-approvals suite (665 tests) is green.

Tests

Three pins, as the ruling specified, in decision-strand-envelope.test.ts — driven through a realAutomationEngine and a realApprovalService, with the reject-branch node throwing the card's own update_record(...) not found text:

  1. the three-outcome reproduction, asserting the new fields and that the row is durably rejected with its audit action written;
  2. a healthy decision unchanged — the reverse control, where only the downstream node differs;
  3. 'stranded' observed at the door, plus its own reverse control: a RUN_NOT_FOUND failure the engine does not call stranded reports repairable: false while still naming its run.

stranded-decision.test.ts pins the carrier itself, including that a partial carrier is refused rather than published as half an envelope (with an all-present control), and that the reader narrows to exactly the four declared keys. rest-approvals-wire-codes.test.ts gains the wire round-trip — built through the shared constructor, asserting the body's exact key set — and a reverse control proving a carrier-less error answers the old two-key body.

Ablation (committed first, so the restore leg had a real reference): removing err.resumeStatus = reported.status flips repairabletrue → false and reds PIN 1 and PIN 3; removing the REST forwarding reds the new wire case while its reverse control correctly stays green. Both files are imported relatively by their tests, so vitest resolves them from source and no dist leg is involved. Each mutation was confirmed on disk by marker count (1 → 0) before the run, and each restore was proven by git hash-object equalling the HEAD blob, with git diff HEAD empty afterwards.

Gates

Derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands against the actual diff, and re-derived after the changed-file set moved (the docs .mdx and the ledger .json added 34 families). Final sweep at 13b58ed7d: 77 green, zero failures attributable to this diff. Exit codes captured by redirecting before reading, never across a pipe; verdicts read from each gate's own line.

Two gates the diff genuinely tripped, both fixed here: check:engine-double-contract and check:objectql-double-limit caught the new fake engine hand-rolling its dispatch and reading the caller's bound by truthiness (limit: 0 would have returned the whole table). Both verbs now open with assertEngineUpdateDispatch / assertEngineDeleteDispatch, the bound is honoured by presence, and the ledger records the two new pinned rows (--write: 2 added, 0 lost — additive). check:system-context-census line rot was repaired by its own --fix: uniform +5 on rest-server.ts (the import block) and +52 on approval-service.ts (the docblocks), which is what makes it rot rather than a finding.

Six non-zero results, none of them this diff:

Contract review and scope

Clause-② yes — an additive change to a published route's error body. Opened as a draft with needs:contract-review; ⛔ not undrafted, armed or enqueued until seat-internal review returns PASS at tier.

⚠️The ApprovalDecisionResult docblock (ruling item 2) is NOT in this PR. It lives in packages/spec/src/contracts/approval-service.ts:622, a single-owner lane this seat is read-only in. The contract type itself needs no new fields — the four facts ride the error body, not the success shape — so what is owed there is the docblock recording that the fields are the published way to read the declared posture. Reported for the spec seat to file.

⚠️Declared file surface extended beyond the claim. The claim named plugin-approvals plus runtime if measured same-seam. Satisfying the ruling's own words — "the error body gains …" — requires the REST door that builds that body (packages/rest/src/rest-server.ts) and the shared carrier's home (packages/types). Declared here rather than landed quietly.

⚠️#15358 touches this same file and is ruled B but undispatched. It lands on ApprovalResumeSurface.getRun and StrandedRunState; this PR touches serviceResume / resumeRecordedOutcome and the four call sites, and does not change the ApprovalResumeSurface interface at all. Different members, so the two should merge cleanly — but whichever lands second should re-run the approvals suite rather than assume it.

⛔ Out of scope and untouched: #15389, #15360, the afterDelete reach of #14061's auto-cancel (NOT MEASURED, stated), the resume ordering, and #13568.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y


Generated by Claude Code

os-warrenand others added 2 commits September 4, 2026 14:58
…cision/runId/repairable beside its 500
Maintainer ruling 2026-09-04, decision batch #37, option B. One
`POST /api/v1/approvals/requests/{id}/reject` produced three coexisting
outcomes: the caller read HTTP 500, the request row WAS terminal and had
left the pending inbox, and the run was stranded. 500 alone reads as "the
rejection did not happen", so callers retried against a durable decision.
The status code does not move — the effect landing while the run strands
is still a failure — and the door does not become atomic: the #13937
shape-4 ruling binds this door's own writes too, so no decision is ever
rolled back. What changed is that the door stops discarding what the
engine said.
- `serviceResume` carries `AutomationResult.status` through. It read only
success/code/error, and the stranded exit reports a status and NO code,
so the repairability signal died one line before the envelope was built
— a member with a producer and, until now, zero consumers.
- `resumeRecordedOutcome` throws a carrier with `finalized` (the decision
stands), `decision`, `runId`, and `repairable` derived from the engine's
`'stranded'` discriminator. Absence of that stamp is `false`, never a
default: a repair verb that would refuse is worse than no promise.
- The REST approvals door forwards those four fields on `RESUME_FAILED`
only, presence-gated — an error with no carrier answers exactly the body
it always did.
- `@objectstack/types` hosts the constructor and its recogniser in one
module (the producer is a plugin; rest cannot import one), the same Home
rule as the validation-failure pair beside it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
… dispatch predicates
`check:engine-double-contract` and `check:objectql-double-limit` both caught
the new fake in `decision-strand-envelope.test.ts`: its `update()`/`delete()`
hand-rolled the dispatch and its `find()` read the caller's bound by
truthiness, so `limit: 0` would have returned the whole table. Both verbs now
open with `assertEngineUpdateDispatch` / `assertEngineDeleteDispatch` and the
bound is honoured by presence; the two new rows are recorded in the pinned
ledger (`--write`, 2 added, 0 lost).
`check:system-context-census` anchors are re-anchored by its own `--fix`: pure
line rot from this branch's edits, uniform +5 on `rest-server.ts` (the import
block) and +52 on `approval-service.ts` (the docblocks), which is what makes it
rot rather than a finding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/plugin-approvals, @objectstack/rest, @objectstack/types, touching 18 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/types/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/api/client-sdk.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), approvals.getRequest (sdk, the route ledger binds it to GET /api/v1/approvals/requests/:id, selected by route anchor /approvals/requests/:id), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/api/plugin-endpoints.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/approvals.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/flows.mdx(via ApprovalService (symbol, a top-level class), runId (symbol, a field of interface StrandedDecisionDetails), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/kernel/services-checklist.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/releases/v17.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /reports/schedules/:scheduleId (route, bridged from symbol strandedDecisionDetails — its route source's handler names it))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/types/src/index.ts) — pages documenting those are invisible to this run
  • 5 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 — 16 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25packageMentionDocs.

Which tree this was computed on

This run read content/docs from 0f9dde70153d16387fb288d3b46915b923281d6a — the merge of head 13b58ed7d7c116c6086cf37e63075b9c9e09b620 into base f01adfa5c9bb75b373b077718c3a8daa74c99f25, 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 0f9dde70153d16387fb288d3b46915b923281d6a && git checkout 0f9dde70153d16387fb288d3b46915b923281d6a
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f01adfa5c9bb75b373b077718c3a8daa74c99f25 13b58ed7d7c116c6086cf37e63075b9c9e09b620 && git checkout -B drift-repro f01adfa5c9bb75b373b077718c3a8daa74c99f25 && git merge --no-ff 13b58ed7d7c116c6086cf37e63075b9c9e09b620
node scripts/docs-audit/affected-docs.mjs --json f01adfa5c9bb75b373b077718c3a8daa74c99f25

⚠️ 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM pre-verification — domain:services seat (session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909). ⛔ This is not the contract review. A PASS at CONTRACT_REVIEW_TIER is still owed and this PR does not land without it. Posted because the tier review is currently blocked on an account-level claude-fable-5-1 quota (three HTTP 429s between 15:57Z and 16:57Z), so this seat verified the two mechanical claims at its own tier to leave the scarce review budget for judgement.

Claim: the ledger --write was additive — 2 rows added, 0 lost

Confirmed, from the diff rather than the tool's own summary.scripts/engine-double-contract.pinned.json, three-dot against origin/main:

  • 10 added lines, 0 removed lines — no row rewritten, none dropped.
  • The 10 lines form exactly two objects, both for the new test file:
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "delete", "pinned": 1 }
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "update", "pinned": 1 }

A --write that silently dropped a row would show as removed lines. There are none.

Claim: disjoint from #15358

Confirmed textually. Across the PR's 725 changed lines (diff 54,196 bytes):

symboloccurrences in changed lines
ApprovalResumeSurface0
getRun0
StrandedRunState0
serviceResume (positive control)3

⚠️ The control is the point. My first attempt at this scan used a -- 'packages/**/*.ts' pathspec and returned 0 for everything, control included — git's fnmatch does not cross / on ** without :(glob) magic, so the pathspec matched nothing. Those zeros were NOT MEASURED, not absence. Re-run without the pathspec, the control is non-zero and the three zeros are readings.

⚠️ One anomaly, chased rather than waved past: resumeRecordedOutcome also reads 0 in changed lines, although the dev's report says it now throws the carrier. That is benign and explains itself — the symbol appears 6 times on origin/main and 6 times on this head in approval-service.ts (declared :2836, called at :2905 / :3303 / :3345 / :3435), so only its body changed and its name never appears on a +/- line. The report is accurate; the symbol is simply a weak control, which is why serviceResume is the one carrying the scan.

What this does and does not establish. It shows the PR's changed lines never mention #15358's members, which is exactly the "does not modify the ApprovalResumeSurface interface at all" claim. ⛔ It does not prove a semantically clean merge — two edits can sit in one file region without sharing a symbol. The PR's own advice stands: whichever lands second re-runs the approvals suite rather than assuming.

Also already measured by this seat, do not re-derive

  • Three-dot origin/main...13b58ed7d = 10 files, +719, −11, matching the PR's own numbers.
  • node scripts/check-adr-0087-registration.mjs --base origin/main --head 13b58ed7dexit 0, captured by redirect rather than through a pipe. This changeset declares no BREAKING, so no adr-0087: marker is owed. (Contrast PR feat(service-automation): evaluate a value-role CEL envelope in the assignment executor and validate it at registerFlow (#15137) #15432, where the same gate was exit 1 — the two make a working pair of controls.)
  • CI green at 15:5xZ with Check Changeset success; one Test Core shard still in progress at that reading, neither passed nor failed.
  • No governed surface touched; nothing under content/docs/releases/.

⇒ Ruling compliance (is the generic resume door the same seam?), the finalized: true durability question, and repairable in the direction nobody checks are untouched by any of the above and remain the review's to decide.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationneeds:contract-reviewteststooling

Projects

None yet

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500 - #15436

Draft
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope
Draft

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500#15436
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#13807

Implements the maintainer ruling of 2026-09-04 (decision batch #37, verbatim 「同意」 on 1B · 2及) — option B, the truthful envelope.

What the ruling changed, and what it deliberately did not

One POST /api/v1/approvals/requests/{id}/reject produced three coexisting outcomes: the caller read HTTP 500, the request row wasrejected and had left the pending inbox, and the workflow run was stranded. A caller — human, script, or agent — reads 500 as "the rejection did not happen" and retries or escalates. It did happen.

The change

serviceResume carries status. It read only success / code / error. The engine's stranded exit reports status: 'stranded' and no code at all, so a door reading only the code saw an unnamed failure and could not tell a repairable strand from a dead run. AutomationResult.status: 'stranded' had a producer (PR #15237) and zero consumers — this is its first.

The decision doors build the envelope.resumeRecordedOutcome now throws a carrier with finalized (the decision stands), decision, runId, and repairable derived from the engine's own discriminator. All four sibling doors on the same path pass their outcome label: decide (approve / reject), the auto-rejection, the send-back (revise) and the resubmit.

repairable is producer-first.true only when the engine said 'stranded' — the one exit that journals a repair snapshot. Every other failure, including a lost run and an engine too old to report a status, is honestly false: absence of the signal is not repairability, and a repair verb that would refuse is worse than no promise.

The REST door forwards the four fields, anchored to RESUME_FAILED and presence-gated. A RESUME_FAILED with no carrier answers exactly the body it always did — the door never synthesises the envelope.

Home rule.strandedDecisionFailure / strandedDecisionDetails live in @objectstack/types because the producer is a plugin and @objectstack/rest cannot import one — the same reasoning, and the same constructor-and-recogniser-in-one-module discipline, as the validation-failure pair beside it. That removes the stringly-typed cross-package agreement about a property name that nothing would have checked.

Measured before implementing — the PM's assumptions, verified rather than inherited

  1. serviceResume's cast (approval-service.ts:2668 on the base, not :2667) read exactly success / code / error. The literal object approvals receives on the stranded exit carries status: 'stranded' and no code — pinned in PIN 3 by intercepting the real engine's return value, so the pin reds if the producer's shape moves and not only the door's.
  2. 'stranded' had zero consumers outside service-automation and spec. Positive control RESUME_IN_PROGRESS, same grep shape, finds five directories including packages/runtime and packages/client — the scan is not over-filtered.
  3. ⚠️The generic door is NOT the same seam — measured, and therefore filed rather than fixed here. It is already filed as automation resume door: the 400 FLOW_FAILED envelope drops the engine's status: 'stranded' verdict — the wire mirror's member is unreachable on the wire #15221; the full measurement is recorded there. In short: no shared code (runtime/domains/automation.ts contains no serviceResume / RESUME_FAILED / handleApprovalError); a wire-shaping omission rather than a lossy relay; a different code, owner and status (400 FLOW_FAILED, registered to @objectstack/runtime); three of the four fields have no referent there; and an approval node cannot reach that arm at all, since resumeAuthority: 'service' makes the generic resume answer 403 first.
  4. The RESUME_FAILED-on-a-200-body pin is untouched, deliberately.approval-restart-resume.test.ts:272 pins the token appearing in resumeError on the no-engine composition. That path returns rather than throwing, so it builds no envelope and this change adds nothing to it — and it should stay that way: that outcome is not stranded and not repairable, so a repairable flag there would be a lie. Renaming the token on the 200 body would be a separate contract change nobody ruled on. The full plugin-approvals suite (665 tests) is green.

Tests

Three pins, as the ruling specified, in decision-strand-envelope.test.ts — driven through a realAutomationEngine and a realApprovalService, with the reject-branch node throwing the card's own update_record(...) not found text:

  1. the three-outcome reproduction, asserting the new fields and that the row is durably rejected with its audit action written;
  2. a healthy decision unchanged — the reverse control, where only the downstream node differs;
  3. 'stranded' observed at the door, plus its own reverse control: a RUN_NOT_FOUND failure the engine does not call stranded reports repairable: false while still naming its run.

stranded-decision.test.ts pins the carrier itself, including that a partial carrier is refused rather than published as half an envelope (with an all-present control), and that the reader narrows to exactly the four declared keys. rest-approvals-wire-codes.test.ts gains the wire round-trip — built through the shared constructor, asserting the body's exact key set — and a reverse control proving a carrier-less error answers the old two-key body.

Ablation (committed first, so the restore leg had a real reference): removing err.resumeStatus = reported.status flips repairabletrue → false and reds PIN 1 and PIN 3; removing the REST forwarding reds the new wire case while its reverse control correctly stays green. Both files are imported relatively by their tests, so vitest resolves them from source and no dist leg is involved. Each mutation was confirmed on disk by marker count (1 → 0) before the run, and each restore was proven by git hash-object equalling the HEAD blob, with git diff HEAD empty afterwards.

Gates

Derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands against the actual diff, and re-derived after the changed-file set moved (the docs .mdx and the ledger .json added 34 families). Final sweep at 13b58ed7d: 77 green, zero failures attributable to this diff. Exit codes captured by redirecting before reading, never across a pipe; verdicts read from each gate's own line.

Two gates the diff genuinely tripped, both fixed here: check:engine-double-contract and check:objectql-double-limit caught the new fake engine hand-rolling its dispatch and reading the caller's bound by truthiness (limit: 0 would have returned the whole table). Both verbs now open with assertEngineUpdateDispatch / assertEngineDeleteDispatch, the bound is honoured by presence, and the ledger records the two new pinned rows (--write: 2 added, 0 lost — additive). check:system-context-census line rot was repaired by its own --fix: uniform +5 on rest-server.ts (the import block) and +52 on approval-service.ts (the docblocks), which is what makes it rot rather than a finding.

Six non-zero results, none of them this diff:

Contract review and scope

Clause-② yes — an additive change to a published route's error body. Opened as a draft with needs:contract-review; ⛔ not undrafted, armed or enqueued until seat-internal review returns PASS at tier.

⚠️The ApprovalDecisionResult docblock (ruling item 2) is NOT in this PR. It lives in packages/spec/src/contracts/approval-service.ts:622, a single-owner lane this seat is read-only in. The contract type itself needs no new fields — the four facts ride the error body, not the success shape — so what is owed there is the docblock recording that the fields are the published way to read the declared posture. Reported for the spec seat to file.

⚠️Declared file surface extended beyond the claim. The claim named plugin-approvals plus runtime if measured same-seam. Satisfying the ruling's own words — "the error body gains …" — requires the REST door that builds that body (packages/rest/src/rest-server.ts) and the shared carrier's home (packages/types). Declared here rather than landed quietly.

⚠️#15358 touches this same file and is ruled B but undispatched. It lands on ApprovalResumeSurface.getRun and StrandedRunState; this PR touches serviceResume / resumeRecordedOutcome and the four call sites, and does not change the ApprovalResumeSurface interface at all. Different members, so the two should merge cleanly — but whichever lands second should re-run the approvals suite rather than assume it.

⛔ Out of scope and untouched: #15389, #15360, the afterDelete reach of #14061's auto-cancel (NOT MEASURED, stated), the resume ordering, and #13568.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y


Generated by Claude Code

os-warrenand others added 2 commits September 4, 2026 14:58
…cision/runId/repairable beside its 500
Maintainer ruling 2026-09-04, decision batch #37, option B. One
`POST /api/v1/approvals/requests/{id}/reject` produced three coexisting
outcomes: the caller read HTTP 500, the request row WAS terminal and had
left the pending inbox, and the run was stranded. 500 alone reads as "the
rejection did not happen", so callers retried against a durable decision.
The status code does not move — the effect landing while the run strands
is still a failure — and the door does not become atomic: the #13937
shape-4 ruling binds this door's own writes too, so no decision is ever
rolled back. What changed is that the door stops discarding what the
engine said.
- `serviceResume` carries `AutomationResult.status` through. It read only
success/code/error, and the stranded exit reports a status and NO code,
so the repairability signal died one line before the envelope was built
— a member with a producer and, until now, zero consumers.
- `resumeRecordedOutcome` throws a carrier with `finalized` (the decision
stands), `decision`, `runId`, and `repairable` derived from the engine's
`'stranded'` discriminator. Absence of that stamp is `false`, never a
default: a repair verb that would refuse is worse than no promise.
- The REST approvals door forwards those four fields on `RESUME_FAILED`
only, presence-gated — an error with no carrier answers exactly the body
it always did.
- `@objectstack/types` hosts the constructor and its recogniser in one
module (the producer is a plugin; rest cannot import one), the same Home
rule as the validation-failure pair beside it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
… dispatch predicates
`check:engine-double-contract` and `check:objectql-double-limit` both caught
the new fake in `decision-strand-envelope.test.ts`: its `update()`/`delete()`
hand-rolled the dispatch and its `find()` read the caller's bound by
truthiness, so `limit: 0` would have returned the whole table. Both verbs now
open with `assertEngineUpdateDispatch` / `assertEngineDeleteDispatch` and the
bound is honoured by presence; the two new rows are recorded in the pinned
ledger (`--write`, 2 added, 0 lost).
`check:system-context-census` anchors are re-anchored by its own `--fix`: pure
line rot from this branch's edits, uniform +5 on `rest-server.ts` (the import
block) and +52 on `approval-service.ts` (the docblocks), which is what makes it
rot rather than a finding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/plugin-approvals, @objectstack/rest, @objectstack/types, touching 18 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/types/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/api/client-sdk.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), approvals.getRequest (sdk, the route ledger binds it to GET /api/v1/approvals/requests/:id, selected by route anchor /approvals/requests/:id), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/api/plugin-endpoints.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/approvals.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/flows.mdx(via ApprovalService (symbol, a top-level class), runId (symbol, a field of interface StrandedDecisionDetails), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/kernel/services-checklist.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/releases/v17.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /reports/schedules/:scheduleId (route, bridged from symbol strandedDecisionDetails — its route source's handler names it))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/types/src/index.ts) — pages documenting those are invisible to this run
  • 5 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 — 16 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25packageMentionDocs.

Which tree this was computed on

This run read content/docs from 0f9dde70153d16387fb288d3b46915b923281d6a — the merge of head 13b58ed7d7c116c6086cf37e63075b9c9e09b620 into base f01adfa5c9bb75b373b077718c3a8daa74c99f25, 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 0f9dde70153d16387fb288d3b46915b923281d6a && git checkout 0f9dde70153d16387fb288d3b46915b923281d6a
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f01adfa5c9bb75b373b077718c3a8daa74c99f25 13b58ed7d7c116c6086cf37e63075b9c9e09b620 && git checkout -B drift-repro f01adfa5c9bb75b373b077718c3a8daa74c99f25 && git merge --no-ff 13b58ed7d7c116c6086cf37e63075b9c9e09b620
node scripts/docs-audit/affected-docs.mjs --json f01adfa5c9bb75b373b077718c3a8daa74c99f25

⚠️ 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM pre-verification — domain:services seat (session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909). ⛔ This is not the contract review. A PASS at CONTRACT_REVIEW_TIER is still owed and this PR does not land without it. Posted because the tier review is currently blocked on an account-level claude-fable-5-1 quota (three HTTP 429s between 15:57Z and 16:57Z), so this seat verified the two mechanical claims at its own tier to leave the scarce review budget for judgement.

Claim: the ledger --write was additive — 2 rows added, 0 lost

Confirmed, from the diff rather than the tool's own summary.scripts/engine-double-contract.pinned.json, three-dot against origin/main:

  • 10 added lines, 0 removed lines — no row rewritten, none dropped.
  • The 10 lines form exactly two objects, both for the new test file:
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "delete", "pinned": 1 }
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "update", "pinned": 1 }

A --write that silently dropped a row would show as removed lines. There are none.

Claim: disjoint from #15358

Confirmed textually. Across the PR's 725 changed lines (diff 54,196 bytes):

symboloccurrences in changed lines
ApprovalResumeSurface0
getRun0
StrandedRunState0
serviceResume (positive control)3

⚠️ The control is the point. My first attempt at this scan used a -- 'packages/**/*.ts' pathspec and returned 0 for everything, control included — git's fnmatch does not cross / on ** without :(glob) magic, so the pathspec matched nothing. Those zeros were NOT MEASURED, not absence. Re-run without the pathspec, the control is non-zero and the three zeros are readings.

⚠️ One anomaly, chased rather than waved past: resumeRecordedOutcome also reads 0 in changed lines, although the dev's report says it now throws the carrier. That is benign and explains itself — the symbol appears 6 times on origin/main and 6 times on this head in approval-service.ts (declared :2836, called at :2905 / :3303 / :3345 / :3435), so only its body changed and its name never appears on a +/- line. The report is accurate; the symbol is simply a weak control, which is why serviceResume is the one carrying the scan.

What this does and does not establish. It shows the PR's changed lines never mention #15358's members, which is exactly the "does not modify the ApprovalResumeSurface interface at all" claim. ⛔ It does not prove a semantically clean merge — two edits can sit in one file region without sharing a symbol. The PR's own advice stands: whichever lands second re-runs the approvals suite rather than assuming.

Also already measured by this seat, do not re-derive

  • Three-dot origin/main...13b58ed7d = 10 files, +719, −11, matching the PR's own numbers.
  • node scripts/check-adr-0087-registration.mjs --base origin/main --head 13b58ed7dexit 0, captured by redirect rather than through a pipe. This changeset declares no BREAKING, so no adr-0087: marker is owed. (Contrast PR feat(service-automation): evaluate a value-role CEL envelope in the assignment executor and validate it at registerFlow (#15137) #15432, where the same gate was exit 1 — the two make a working pair of controls.)
  • CI green at 15:5xZ with Check Changeset success; one Test Core shard still in progress at that reading, neither passed nor failed.
  • No governed surface touched; nothing under content/docs/releases/.

⇒ Ruling compliance (is the generic resume door the same seam?), the finalized: true durability question, and repairable in the direction nobody checks are untouched by any of the above and remain the review's to decide.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationneeds:contract-reviewteststooling

Projects

None yet

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500 - #15436

Draft
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope
Draft

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500#15436
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#13807

Implements the maintainer ruling of 2026-09-04 (decision batch #37, verbatim 「同意」 on 1B · 2及) — option B, the truthful envelope.

What the ruling changed, and what it deliberately did not

One POST /api/v1/approvals/requests/{id}/reject produced three coexisting outcomes: the caller read HTTP 500, the request row wasrejected and had left the pending inbox, and the workflow run was stranded. A caller — human, script, or agent — reads 500 as "the rejection did not happen" and retries or escalates. It did happen.

The change

serviceResume carries status. It read only success / code / error. The engine's stranded exit reports status: 'stranded' and no code at all, so a door reading only the code saw an unnamed failure and could not tell a repairable strand from a dead run. AutomationResult.status: 'stranded' had a producer (PR #15237) and zero consumers — this is its first.

The decision doors build the envelope.resumeRecordedOutcome now throws a carrier with finalized (the decision stands), decision, runId, and repairable derived from the engine's own discriminator. All four sibling doors on the same path pass their outcome label: decide (approve / reject), the auto-rejection, the send-back (revise) and the resubmit.

repairable is producer-first.true only when the engine said 'stranded' — the one exit that journals a repair snapshot. Every other failure, including a lost run and an engine too old to report a status, is honestly false: absence of the signal is not repairability, and a repair verb that would refuse is worse than no promise.

The REST door forwards the four fields, anchored to RESUME_FAILED and presence-gated. A RESUME_FAILED with no carrier answers exactly the body it always did — the door never synthesises the envelope.

Home rule.strandedDecisionFailure / strandedDecisionDetails live in @objectstack/types because the producer is a plugin and @objectstack/rest cannot import one — the same reasoning, and the same constructor-and-recogniser-in-one-module discipline, as the validation-failure pair beside it. That removes the stringly-typed cross-package agreement about a property name that nothing would have checked.

Measured before implementing — the PM's assumptions, verified rather than inherited

  1. serviceResume's cast (approval-service.ts:2668 on the base, not :2667) read exactly success / code / error. The literal object approvals receives on the stranded exit carries status: 'stranded' and no code — pinned in PIN 3 by intercepting the real engine's return value, so the pin reds if the producer's shape moves and not only the door's.
  2. 'stranded' had zero consumers outside service-automation and spec. Positive control RESUME_IN_PROGRESS, same grep shape, finds five directories including packages/runtime and packages/client — the scan is not over-filtered.
  3. ⚠️The generic door is NOT the same seam — measured, and therefore filed rather than fixed here. It is already filed as automation resume door: the 400 FLOW_FAILED envelope drops the engine's status: 'stranded' verdict — the wire mirror's member is unreachable on the wire #15221; the full measurement is recorded there. In short: no shared code (runtime/domains/automation.ts contains no serviceResume / RESUME_FAILED / handleApprovalError); a wire-shaping omission rather than a lossy relay; a different code, owner and status (400 FLOW_FAILED, registered to @objectstack/runtime); three of the four fields have no referent there; and an approval node cannot reach that arm at all, since resumeAuthority: 'service' makes the generic resume answer 403 first.
  4. The RESUME_FAILED-on-a-200-body pin is untouched, deliberately.approval-restart-resume.test.ts:272 pins the token appearing in resumeError on the no-engine composition. That path returns rather than throwing, so it builds no envelope and this change adds nothing to it — and it should stay that way: that outcome is not stranded and not repairable, so a repairable flag there would be a lie. Renaming the token on the 200 body would be a separate contract change nobody ruled on. The full plugin-approvals suite (665 tests) is green.

Tests

Three pins, as the ruling specified, in decision-strand-envelope.test.ts — driven through a realAutomationEngine and a realApprovalService, with the reject-branch node throwing the card's own update_record(...) not found text:

  1. the three-outcome reproduction, asserting the new fields and that the row is durably rejected with its audit action written;
  2. a healthy decision unchanged — the reverse control, where only the downstream node differs;
  3. 'stranded' observed at the door, plus its own reverse control: a RUN_NOT_FOUND failure the engine does not call stranded reports repairable: false while still naming its run.

stranded-decision.test.ts pins the carrier itself, including that a partial carrier is refused rather than published as half an envelope (with an all-present control), and that the reader narrows to exactly the four declared keys. rest-approvals-wire-codes.test.ts gains the wire round-trip — built through the shared constructor, asserting the body's exact key set — and a reverse control proving a carrier-less error answers the old two-key body.

Ablation (committed first, so the restore leg had a real reference): removing err.resumeStatus = reported.status flips repairabletrue → false and reds PIN 1 and PIN 3; removing the REST forwarding reds the new wire case while its reverse control correctly stays green. Both files are imported relatively by their tests, so vitest resolves them from source and no dist leg is involved. Each mutation was confirmed on disk by marker count (1 → 0) before the run, and each restore was proven by git hash-object equalling the HEAD blob, with git diff HEAD empty afterwards.

Gates

Derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands against the actual diff, and re-derived after the changed-file set moved (the docs .mdx and the ledger .json added 34 families). Final sweep at 13b58ed7d: 77 green, zero failures attributable to this diff. Exit codes captured by redirecting before reading, never across a pipe; verdicts read from each gate's own line.

Two gates the diff genuinely tripped, both fixed here: check:engine-double-contract and check:objectql-double-limit caught the new fake engine hand-rolling its dispatch and reading the caller's bound by truthiness (limit: 0 would have returned the whole table). Both verbs now open with assertEngineUpdateDispatch / assertEngineDeleteDispatch, the bound is honoured by presence, and the ledger records the two new pinned rows (--write: 2 added, 0 lost — additive). check:system-context-census line rot was repaired by its own --fix: uniform +5 on rest-server.ts (the import block) and +52 on approval-service.ts (the docblocks), which is what makes it rot rather than a finding.

Six non-zero results, none of them this diff:

Contract review and scope

Clause-② yes — an additive change to a published route's error body. Opened as a draft with needs:contract-review; ⛔ not undrafted, armed or enqueued until seat-internal review returns PASS at tier.

⚠️The ApprovalDecisionResult docblock (ruling item 2) is NOT in this PR. It lives in packages/spec/src/contracts/approval-service.ts:622, a single-owner lane this seat is read-only in. The contract type itself needs no new fields — the four facts ride the error body, not the success shape — so what is owed there is the docblock recording that the fields are the published way to read the declared posture. Reported for the spec seat to file.

⚠️Declared file surface extended beyond the claim. The claim named plugin-approvals plus runtime if measured same-seam. Satisfying the ruling's own words — "the error body gains …" — requires the REST door that builds that body (packages/rest/src/rest-server.ts) and the shared carrier's home (packages/types). Declared here rather than landed quietly.

⚠️#15358 touches this same file and is ruled B but undispatched. It lands on ApprovalResumeSurface.getRun and StrandedRunState; this PR touches serviceResume / resumeRecordedOutcome and the four call sites, and does not change the ApprovalResumeSurface interface at all. Different members, so the two should merge cleanly — but whichever lands second should re-run the approvals suite rather than assume it.

⛔ Out of scope and untouched: #15389, #15360, the afterDelete reach of #14061's auto-cancel (NOT MEASURED, stated), the resume ordering, and #13568.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y


Generated by Claude Code

os-warrenand others added 2 commits September 4, 2026 14:58
…cision/runId/repairable beside its 500
Maintainer ruling 2026-09-04, decision batch #37, option B. One
`POST /api/v1/approvals/requests/{id}/reject` produced three coexisting
outcomes: the caller read HTTP 500, the request row WAS terminal and had
left the pending inbox, and the run was stranded. 500 alone reads as "the
rejection did not happen", so callers retried against a durable decision.
The status code does not move — the effect landing while the run strands
is still a failure — and the door does not become atomic: the #13937
shape-4 ruling binds this door's own writes too, so no decision is ever
rolled back. What changed is that the door stops discarding what the
engine said.
- `serviceResume` carries `AutomationResult.status` through. It read only
success/code/error, and the stranded exit reports a status and NO code,
so the repairability signal died one line before the envelope was built
— a member with a producer and, until now, zero consumers.
- `resumeRecordedOutcome` throws a carrier with `finalized` (the decision
stands), `decision`, `runId`, and `repairable` derived from the engine's
`'stranded'` discriminator. Absence of that stamp is `false`, never a
default: a repair verb that would refuse is worse than no promise.
- The REST approvals door forwards those four fields on `RESUME_FAILED`
only, presence-gated — an error with no carrier answers exactly the body
it always did.
- `@objectstack/types` hosts the constructor and its recogniser in one
module (the producer is a plugin; rest cannot import one), the same Home
rule as the validation-failure pair beside it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
… dispatch predicates
`check:engine-double-contract` and `check:objectql-double-limit` both caught
the new fake in `decision-strand-envelope.test.ts`: its `update()`/`delete()`
hand-rolled the dispatch and its `find()` read the caller's bound by
truthiness, so `limit: 0` would have returned the whole table. Both verbs now
open with `assertEngineUpdateDispatch` / `assertEngineDeleteDispatch` and the
bound is honoured by presence; the two new rows are recorded in the pinned
ledger (`--write`, 2 added, 0 lost).
`check:system-context-census` anchors are re-anchored by its own `--fix`: pure
line rot from this branch's edits, uniform +5 on `rest-server.ts` (the import
block) and +52 on `approval-service.ts` (the docblocks), which is what makes it
rot rather than a finding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/plugin-approvals, @objectstack/rest, @objectstack/types, touching 18 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/types/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/api/client-sdk.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), approvals.getRequest (sdk, the route ledger binds it to GET /api/v1/approvals/requests/:id, selected by route anchor /approvals/requests/:id), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/api/plugin-endpoints.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/approvals.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/flows.mdx(via ApprovalService (symbol, a top-level class), runId (symbol, a field of interface StrandedDecisionDetails), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/kernel/services-checklist.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/releases/v17.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /reports/schedules/:scheduleId (route, bridged from symbol strandedDecisionDetails — its route source's handler names it))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/types/src/index.ts) — pages documenting those are invisible to this run
  • 5 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 — 16 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25packageMentionDocs.

Which tree this was computed on

This run read content/docs from 0f9dde70153d16387fb288d3b46915b923281d6a — the merge of head 13b58ed7d7c116c6086cf37e63075b9c9e09b620 into base f01adfa5c9bb75b373b077718c3a8daa74c99f25, 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 0f9dde70153d16387fb288d3b46915b923281d6a && git checkout 0f9dde70153d16387fb288d3b46915b923281d6a
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f01adfa5c9bb75b373b077718c3a8daa74c99f25 13b58ed7d7c116c6086cf37e63075b9c9e09b620 && git checkout -B drift-repro f01adfa5c9bb75b373b077718c3a8daa74c99f25 && git merge --no-ff 13b58ed7d7c116c6086cf37e63075b9c9e09b620
node scripts/docs-audit/affected-docs.mjs --json f01adfa5c9bb75b373b077718c3a8daa74c99f25

⚠️ 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM pre-verification — domain:services seat (session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909). ⛔ This is not the contract review. A PASS at CONTRACT_REVIEW_TIER is still owed and this PR does not land without it. Posted because the tier review is currently blocked on an account-level claude-fable-5-1 quota (three HTTP 429s between 15:57Z and 16:57Z), so this seat verified the two mechanical claims at its own tier to leave the scarce review budget for judgement.

Claim: the ledger --write was additive — 2 rows added, 0 lost

Confirmed, from the diff rather than the tool's own summary.scripts/engine-double-contract.pinned.json, three-dot against origin/main:

  • 10 added lines, 0 removed lines — no row rewritten, none dropped.
  • The 10 lines form exactly two objects, both for the new test file:
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "delete", "pinned": 1 }
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "update", "pinned": 1 }

A --write that silently dropped a row would show as removed lines. There are none.

Claim: disjoint from #15358

Confirmed textually. Across the PR's 725 changed lines (diff 54,196 bytes):

symboloccurrences in changed lines
ApprovalResumeSurface0
getRun0
StrandedRunState0
serviceResume (positive control)3

⚠️ The control is the point. My first attempt at this scan used a -- 'packages/**/*.ts' pathspec and returned 0 for everything, control included — git's fnmatch does not cross / on ** without :(glob) magic, so the pathspec matched nothing. Those zeros were NOT MEASURED, not absence. Re-run without the pathspec, the control is non-zero and the three zeros are readings.

⚠️ One anomaly, chased rather than waved past: resumeRecordedOutcome also reads 0 in changed lines, although the dev's report says it now throws the carrier. That is benign and explains itself — the symbol appears 6 times on origin/main and 6 times on this head in approval-service.ts (declared :2836, called at :2905 / :3303 / :3345 / :3435), so only its body changed and its name never appears on a +/- line. The report is accurate; the symbol is simply a weak control, which is why serviceResume is the one carrying the scan.

What this does and does not establish. It shows the PR's changed lines never mention #15358's members, which is exactly the "does not modify the ApprovalResumeSurface interface at all" claim. ⛔ It does not prove a semantically clean merge — two edits can sit in one file region without sharing a symbol. The PR's own advice stands: whichever lands second re-runs the approvals suite rather than assuming.

Also already measured by this seat, do not re-derive

  • Three-dot origin/main...13b58ed7d = 10 files, +719, −11, matching the PR's own numbers.
  • node scripts/check-adr-0087-registration.mjs --base origin/main --head 13b58ed7dexit 0, captured by redirect rather than through a pipe. This changeset declares no BREAKING, so no adr-0087: marker is owed. (Contrast PR feat(service-automation): evaluate a value-role CEL envelope in the assignment executor and validate it at registerFlow (#15137) #15432, where the same gate was exit 1 — the two make a working pair of controls.)
  • CI green at 15:5xZ with Check Changeset success; one Test Core shard still in progress at that reading, neither passed nor failed.
  • No governed surface touched; nothing under content/docs/releases/.

⇒ Ruling compliance (is the generic resume door the same seam?), the finalized: true durability question, and repairable in the direction nobody checks are untouched by any of the above and remain the review's to decide.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationneeds:contract-reviewteststooling

Projects

None yet

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length \u003e 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500 - #15436

Draft
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope
Draft

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500#15436
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#13807

Implements the maintainer ruling of 2026-09-04 (decision batch #37, verbatim 「同意」 on 1B · 2及) — option B, the truthful envelope.

What the ruling changed, and what it deliberately did not

One POST /api/v1/approvals/requests/{id}/reject produced three coexisting outcomes: the caller read HTTP 500, the request row wasrejected and had left the pending inbox, and the workflow run was stranded. A caller — human, script, or agent — reads 500 as "the rejection did not happen" and retries or escalates. It did happen.

The change

serviceResume carries status. It read only success / code / error. The engine's stranded exit reports status: 'stranded' and no code at all, so a door reading only the code saw an unnamed failure and could not tell a repairable strand from a dead run. AutomationResult.status: 'stranded' had a producer (PR #15237) and zero consumers — this is its first.

The decision doors build the envelope.resumeRecordedOutcome now throws a carrier with finalized (the decision stands), decision, runId, and repairable derived from the engine's own discriminator. All four sibling doors on the same path pass their outcome label: decide (approve / reject), the auto-rejection, the send-back (revise) and the resubmit.

repairable is producer-first.true only when the engine said 'stranded' — the one exit that journals a repair snapshot. Every other failure, including a lost run and an engine too old to report a status, is honestly false: absence of the signal is not repairability, and a repair verb that would refuse is worse than no promise.

The REST door forwards the four fields, anchored to RESUME_FAILED and presence-gated. A RESUME_FAILED with no carrier answers exactly the body it always did — the door never synthesises the envelope.

Home rule.strandedDecisionFailure / strandedDecisionDetails live in @objectstack/types because the producer is a plugin and @objectstack/rest cannot import one — the same reasoning, and the same constructor-and-recogniser-in-one-module discipline, as the validation-failure pair beside it. That removes the stringly-typed cross-package agreement about a property name that nothing would have checked.

Measured before implementing — the PM's assumptions, verified rather than inherited

  1. serviceResume's cast (approval-service.ts:2668 on the base, not :2667) read exactly success / code / error. The literal object approvals receives on the stranded exit carries status: 'stranded' and no code — pinned in PIN 3 by intercepting the real engine's return value, so the pin reds if the producer's shape moves and not only the door's.
  2. 'stranded' had zero consumers outside service-automation and spec. Positive control RESUME_IN_PROGRESS, same grep shape, finds five directories including packages/runtime and packages/client — the scan is not over-filtered.
  3. ⚠️The generic door is NOT the same seam — measured, and therefore filed rather than fixed here. It is already filed as automation resume door: the 400 FLOW_FAILED envelope drops the engine's status: 'stranded' verdict — the wire mirror's member is unreachable on the wire #15221; the full measurement is recorded there. In short: no shared code (runtime/domains/automation.ts contains no serviceResume / RESUME_FAILED / handleApprovalError); a wire-shaping omission rather than a lossy relay; a different code, owner and status (400 FLOW_FAILED, registered to @objectstack/runtime); three of the four fields have no referent there; and an approval node cannot reach that arm at all, since resumeAuthority: 'service' makes the generic resume answer 403 first.
  4. The RESUME_FAILED-on-a-200-body pin is untouched, deliberately.approval-restart-resume.test.ts:272 pins the token appearing in resumeError on the no-engine composition. That path returns rather than throwing, so it builds no envelope and this change adds nothing to it — and it should stay that way: that outcome is not stranded and not repairable, so a repairable flag there would be a lie. Renaming the token on the 200 body would be a separate contract change nobody ruled on. The full plugin-approvals suite (665 tests) is green.

Tests

Three pins, as the ruling specified, in decision-strand-envelope.test.ts — driven through a realAutomationEngine and a realApprovalService, with the reject-branch node throwing the card's own update_record(...) not found text:

  1. the three-outcome reproduction, asserting the new fields and that the row is durably rejected with its audit action written;
  2. a healthy decision unchanged — the reverse control, where only the downstream node differs;
  3. 'stranded' observed at the door, plus its own reverse control: a RUN_NOT_FOUND failure the engine does not call stranded reports repairable: false while still naming its run.

stranded-decision.test.ts pins the carrier itself, including that a partial carrier is refused rather than published as half an envelope (with an all-present control), and that the reader narrows to exactly the four declared keys. rest-approvals-wire-codes.test.ts gains the wire round-trip — built through the shared constructor, asserting the body's exact key set — and a reverse control proving a carrier-less error answers the old two-key body.

Ablation (committed first, so the restore leg had a real reference): removing err.resumeStatus = reported.status flips repairabletrue → false and reds PIN 1 and PIN 3; removing the REST forwarding reds the new wire case while its reverse control correctly stays green. Both files are imported relatively by their tests, so vitest resolves them from source and no dist leg is involved. Each mutation was confirmed on disk by marker count (1 → 0) before the run, and each restore was proven by git hash-object equalling the HEAD blob, with git diff HEAD empty afterwards.

Gates

Derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands against the actual diff, and re-derived after the changed-file set moved (the docs .mdx and the ledger .json added 34 families). Final sweep at 13b58ed7d: 77 green, zero failures attributable to this diff. Exit codes captured by redirecting before reading, never across a pipe; verdicts read from each gate's own line.

Two gates the diff genuinely tripped, both fixed here: check:engine-double-contract and check:objectql-double-limit caught the new fake engine hand-rolling its dispatch and reading the caller's bound by truthiness (limit: 0 would have returned the whole table). Both verbs now open with assertEngineUpdateDispatch / assertEngineDeleteDispatch, the bound is honoured by presence, and the ledger records the two new pinned rows (--write: 2 added, 0 lost — additive). check:system-context-census line rot was repaired by its own --fix: uniform +5 on rest-server.ts (the import block) and +52 on approval-service.ts (the docblocks), which is what makes it rot rather than a finding.

Six non-zero results, none of them this diff:

Contract review and scope

Clause-② yes — an additive change to a published route's error body. Opened as a draft with needs:contract-review; ⛔ not undrafted, armed or enqueued until seat-internal review returns PASS at tier.

⚠️The ApprovalDecisionResult docblock (ruling item 2) is NOT in this PR. It lives in packages/spec/src/contracts/approval-service.ts:622, a single-owner lane this seat is read-only in. The contract type itself needs no new fields — the four facts ride the error body, not the success shape — so what is owed there is the docblock recording that the fields are the published way to read the declared posture. Reported for the spec seat to file.

⚠️Declared file surface extended beyond the claim. The claim named plugin-approvals plus runtime if measured same-seam. Satisfying the ruling's own words — "the error body gains …" — requires the REST door that builds that body (packages/rest/src/rest-server.ts) and the shared carrier's home (packages/types). Declared here rather than landed quietly.

⚠️#15358 touches this same file and is ruled B but undispatched. It lands on ApprovalResumeSurface.getRun and StrandedRunState; this PR touches serviceResume / resumeRecordedOutcome and the four call sites, and does not change the ApprovalResumeSurface interface at all. Different members, so the two should merge cleanly — but whichever lands second should re-run the approvals suite rather than assume it.

⛔ Out of scope and untouched: #15389, #15360, the afterDelete reach of #14061's auto-cancel (NOT MEASURED, stated), the resume ordering, and #13568.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y


Generated by Claude Code

os-warrenand others added 2 commits September 4, 2026 14:58
…cision/runId/repairable beside its 500
Maintainer ruling 2026-09-04, decision batch #37, option B. One
`POST /api/v1/approvals/requests/{id}/reject` produced three coexisting
outcomes: the caller read HTTP 500, the request row WAS terminal and had
left the pending inbox, and the run was stranded. 500 alone reads as "the
rejection did not happen", so callers retried against a durable decision.
The status code does not move — the effect landing while the run strands
is still a failure — and the door does not become atomic: the #13937
shape-4 ruling binds this door's own writes too, so no decision is ever
rolled back. What changed is that the door stops discarding what the
engine said.
- `serviceResume` carries `AutomationResult.status` through. It read only
success/code/error, and the stranded exit reports a status and NO code,
so the repairability signal died one line before the envelope was built
— a member with a producer and, until now, zero consumers.
- `resumeRecordedOutcome` throws a carrier with `finalized` (the decision
stands), `decision`, `runId`, and `repairable` derived from the engine's
`'stranded'` discriminator. Absence of that stamp is `false`, never a
default: a repair verb that would refuse is worse than no promise.
- The REST approvals door forwards those four fields on `RESUME_FAILED`
only, presence-gated — an error with no carrier answers exactly the body
it always did.
- `@objectstack/types` hosts the constructor and its recogniser in one
module (the producer is a plugin; rest cannot import one), the same Home
rule as the validation-failure pair beside it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
… dispatch predicates
`check:engine-double-contract` and `check:objectql-double-limit` both caught
the new fake in `decision-strand-envelope.test.ts`: its `update()`/`delete()`
hand-rolled the dispatch and its `find()` read the caller's bound by
truthiness, so `limit: 0` would have returned the whole table. Both verbs now
open with `assertEngineUpdateDispatch` / `assertEngineDeleteDispatch` and the
bound is honoured by presence; the two new rows are recorded in the pinned
ledger (`--write`, 2 added, 0 lost).
`check:system-context-census` anchors are re-anchored by its own `--fix`: pure
line rot from this branch's edits, uniform +5 on `rest-server.ts` (the import
block) and +52 on `approval-service.ts` (the docblocks), which is what makes it
rot rather than a finding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/plugin-approvals, @objectstack/rest, @objectstack/types, touching 18 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/types/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/api/client-sdk.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), approvals.getRequest (sdk, the route ledger binds it to GET /api/v1/approvals/requests/:id, selected by route anchor /approvals/requests/:id), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/api/plugin-endpoints.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/approvals.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/flows.mdx(via ApprovalService (symbol, a top-level class), runId (symbol, a field of interface StrandedDecisionDetails), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/kernel/services-checklist.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/releases/v17.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /reports/schedules/:scheduleId (route, bridged from symbol strandedDecisionDetails — its route source's handler names it))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/types/src/index.ts) — pages documenting those are invisible to this run
  • 5 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 — 16 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25packageMentionDocs.

Which tree this was computed on

This run read content/docs from 0f9dde70153d16387fb288d3b46915b923281d6a — the merge of head 13b58ed7d7c116c6086cf37e63075b9c9e09b620 into base f01adfa5c9bb75b373b077718c3a8daa74c99f25, 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 0f9dde70153d16387fb288d3b46915b923281d6a && git checkout 0f9dde70153d16387fb288d3b46915b923281d6a
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f01adfa5c9bb75b373b077718c3a8daa74c99f25 13b58ed7d7c116c6086cf37e63075b9c9e09b620 && git checkout -B drift-repro f01adfa5c9bb75b373b077718c3a8daa74c99f25 && git merge --no-ff 13b58ed7d7c116c6086cf37e63075b9c9e09b620
node scripts/docs-audit/affected-docs.mjs --json f01adfa5c9bb75b373b077718c3a8daa74c99f25

⚠️ 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM pre-verification — domain:services seat (session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909). ⛔ This is not the contract review. A PASS at CONTRACT_REVIEW_TIER is still owed and this PR does not land without it. Posted because the tier review is currently blocked on an account-level claude-fable-5-1 quota (three HTTP 429s between 15:57Z and 16:57Z), so this seat verified the two mechanical claims at its own tier to leave the scarce review budget for judgement.

Claim: the ledger --write was additive — 2 rows added, 0 lost

Confirmed, from the diff rather than the tool's own summary.scripts/engine-double-contract.pinned.json, three-dot against origin/main:

  • 10 added lines, 0 removed lines — no row rewritten, none dropped.
  • The 10 lines form exactly two objects, both for the new test file:
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "delete", "pinned": 1 }
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "update", "pinned": 1 }

A --write that silently dropped a row would show as removed lines. There are none.

Claim: disjoint from #15358

Confirmed textually. Across the PR's 725 changed lines (diff 54,196 bytes):

symboloccurrences in changed lines
ApprovalResumeSurface0
getRun0
StrandedRunState0
serviceResume (positive control)3

⚠️ The control is the point. My first attempt at this scan used a -- 'packages/**/*.ts' pathspec and returned 0 for everything, control included — git's fnmatch does not cross / on ** without :(glob) magic, so the pathspec matched nothing. Those zeros were NOT MEASURED, not absence. Re-run without the pathspec, the control is non-zero and the three zeros are readings.

⚠️ One anomaly, chased rather than waved past: resumeRecordedOutcome also reads 0 in changed lines, although the dev's report says it now throws the carrier. That is benign and explains itself — the symbol appears 6 times on origin/main and 6 times on this head in approval-service.ts (declared :2836, called at :2905 / :3303 / :3345 / :3435), so only its body changed and its name never appears on a +/- line. The report is accurate; the symbol is simply a weak control, which is why serviceResume is the one carrying the scan.

What this does and does not establish. It shows the PR's changed lines never mention #15358's members, which is exactly the "does not modify the ApprovalResumeSurface interface at all" claim. ⛔ It does not prove a semantically clean merge — two edits can sit in one file region without sharing a symbol. The PR's own advice stands: whichever lands second re-runs the approvals suite rather than assuming.

Also already measured by this seat, do not re-derive

  • Three-dot origin/main...13b58ed7d = 10 files, +719, −11, matching the PR's own numbers.
  • node scripts/check-adr-0087-registration.mjs --base origin/main --head 13b58ed7dexit 0, captured by redirect rather than through a pipe. This changeset declares no BREAKING, so no adr-0087: marker is owed. (Contrast PR feat(service-automation): evaluate a value-role CEL envelope in the assignment executor and validate it at registerFlow (#15137) #15432, where the same gate was exit 1 — the two make a working pair of controls.)
  • CI green at 15:5xZ with Check Changeset success; one Test Core shard still in progress at that reading, neither passed nor failed.
  • No governed surface touched; nothing under content/docs/releases/.

⇒ Ruling compliance (is the generic resume door the same seam?), the finalized: true durability question, and repairable in the direction nobody checks are untouched by any of the above and remain the review's to decide.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationneeds:contract-reviewteststooling

Projects

None yet

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500 - #15436

Draft
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope
Draft

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500#15436
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#13807

Implements the maintainer ruling of 2026-09-04 (decision batch #37, verbatim 「同意」 on 1B · 2及) — option B, the truthful envelope.

What the ruling changed, and what it deliberately did not

One POST /api/v1/approvals/requests/{id}/reject produced three coexisting outcomes: the caller read HTTP 500, the request row wasrejected and had left the pending inbox, and the workflow run was stranded. A caller — human, script, or agent — reads 500 as "the rejection did not happen" and retries or escalates. It did happen.

The change

serviceResume carries status. It read only success / code / error. The engine's stranded exit reports status: 'stranded' and no code at all, so a door reading only the code saw an unnamed failure and could not tell a repairable strand from a dead run. AutomationResult.status: 'stranded' had a producer (PR #15237) and zero consumers — this is its first.

The decision doors build the envelope.resumeRecordedOutcome now throws a carrier with finalized (the decision stands), decision, runId, and repairable derived from the engine's own discriminator. All four sibling doors on the same path pass their outcome label: decide (approve / reject), the auto-rejection, the send-back (revise) and the resubmit.

repairable is producer-first.true only when the engine said 'stranded' — the one exit that journals a repair snapshot. Every other failure, including a lost run and an engine too old to report a status, is honestly false: absence of the signal is not repairability, and a repair verb that would refuse is worse than no promise.

The REST door forwards the four fields, anchored to RESUME_FAILED and presence-gated. A RESUME_FAILED with no carrier answers exactly the body it always did — the door never synthesises the envelope.

Home rule.strandedDecisionFailure / strandedDecisionDetails live in @objectstack/types because the producer is a plugin and @objectstack/rest cannot import one — the same reasoning, and the same constructor-and-recogniser-in-one-module discipline, as the validation-failure pair beside it. That removes the stringly-typed cross-package agreement about a property name that nothing would have checked.

Measured before implementing — the PM's assumptions, verified rather than inherited

  1. serviceResume's cast (approval-service.ts:2668 on the base, not :2667) read exactly success / code / error. The literal object approvals receives on the stranded exit carries status: 'stranded' and no code — pinned in PIN 3 by intercepting the real engine's return value, so the pin reds if the producer's shape moves and not only the door's.
  2. 'stranded' had zero consumers outside service-automation and spec. Positive control RESUME_IN_PROGRESS, same grep shape, finds five directories including packages/runtime and packages/client — the scan is not over-filtered.
  3. ⚠️The generic door is NOT the same seam — measured, and therefore filed rather than fixed here. It is already filed as automation resume door: the 400 FLOW_FAILED envelope drops the engine's status: 'stranded' verdict — the wire mirror's member is unreachable on the wire #15221; the full measurement is recorded there. In short: no shared code (runtime/domains/automation.ts contains no serviceResume / RESUME_FAILED / handleApprovalError); a wire-shaping omission rather than a lossy relay; a different code, owner and status (400 FLOW_FAILED, registered to @objectstack/runtime); three of the four fields have no referent there; and an approval node cannot reach that arm at all, since resumeAuthority: 'service' makes the generic resume answer 403 first.
  4. The RESUME_FAILED-on-a-200-body pin is untouched, deliberately.approval-restart-resume.test.ts:272 pins the token appearing in resumeError on the no-engine composition. That path returns rather than throwing, so it builds no envelope and this change adds nothing to it — and it should stay that way: that outcome is not stranded and not repairable, so a repairable flag there would be a lie. Renaming the token on the 200 body would be a separate contract change nobody ruled on. The full plugin-approvals suite (665 tests) is green.

Tests

Three pins, as the ruling specified, in decision-strand-envelope.test.ts — driven through a realAutomationEngine and a realApprovalService, with the reject-branch node throwing the card's own update_record(...) not found text:

  1. the three-outcome reproduction, asserting the new fields and that the row is durably rejected with its audit action written;
  2. a healthy decision unchanged — the reverse control, where only the downstream node differs;
  3. 'stranded' observed at the door, plus its own reverse control: a RUN_NOT_FOUND failure the engine does not call stranded reports repairable: false while still naming its run.

stranded-decision.test.ts pins the carrier itself, including that a partial carrier is refused rather than published as half an envelope (with an all-present control), and that the reader narrows to exactly the four declared keys. rest-approvals-wire-codes.test.ts gains the wire round-trip — built through the shared constructor, asserting the body's exact key set — and a reverse control proving a carrier-less error answers the old two-key body.

Ablation (committed first, so the restore leg had a real reference): removing err.resumeStatus = reported.status flips repairabletrue → false and reds PIN 1 and PIN 3; removing the REST forwarding reds the new wire case while its reverse control correctly stays green. Both files are imported relatively by their tests, so vitest resolves them from source and no dist leg is involved. Each mutation was confirmed on disk by marker count (1 → 0) before the run, and each restore was proven by git hash-object equalling the HEAD blob, with git diff HEAD empty afterwards.

Gates

Derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands against the actual diff, and re-derived after the changed-file set moved (the docs .mdx and the ledger .json added 34 families). Final sweep at 13b58ed7d: 77 green, zero failures attributable to this diff. Exit codes captured by redirecting before reading, never across a pipe; verdicts read from each gate's own line.

Two gates the diff genuinely tripped, both fixed here: check:engine-double-contract and check:objectql-double-limit caught the new fake engine hand-rolling its dispatch and reading the caller's bound by truthiness (limit: 0 would have returned the whole table). Both verbs now open with assertEngineUpdateDispatch / assertEngineDeleteDispatch, the bound is honoured by presence, and the ledger records the two new pinned rows (--write: 2 added, 0 lost — additive). check:system-context-census line rot was repaired by its own --fix: uniform +5 on rest-server.ts (the import block) and +52 on approval-service.ts (the docblocks), which is what makes it rot rather than a finding.

Six non-zero results, none of them this diff:

Contract review and scope

Clause-② yes — an additive change to a published route's error body. Opened as a draft with needs:contract-review; ⛔ not undrafted, armed or enqueued until seat-internal review returns PASS at tier.

⚠️The ApprovalDecisionResult docblock (ruling item 2) is NOT in this PR. It lives in packages/spec/src/contracts/approval-service.ts:622, a single-owner lane this seat is read-only in. The contract type itself needs no new fields — the four facts ride the error body, not the success shape — so what is owed there is the docblock recording that the fields are the published way to read the declared posture. Reported for the spec seat to file.

⚠️Declared file surface extended beyond the claim. The claim named plugin-approvals plus runtime if measured same-seam. Satisfying the ruling's own words — "the error body gains …" — requires the REST door that builds that body (packages/rest/src/rest-server.ts) and the shared carrier's home (packages/types). Declared here rather than landed quietly.

⚠️#15358 touches this same file and is ruled B but undispatched. It lands on ApprovalResumeSurface.getRun and StrandedRunState; this PR touches serviceResume / resumeRecordedOutcome and the four call sites, and does not change the ApprovalResumeSurface interface at all. Different members, so the two should merge cleanly — but whichever lands second should re-run the approvals suite rather than assume it.

⛔ Out of scope and untouched: #15389, #15360, the afterDelete reach of #14061's auto-cancel (NOT MEASURED, stated), the resume ordering, and #13568.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y


Generated by Claude Code

os-warrenand others added 2 commits September 4, 2026 14:58
…cision/runId/repairable beside its 500
Maintainer ruling 2026-09-04, decision batch #37, option B. One
`POST /api/v1/approvals/requests/{id}/reject` produced three coexisting
outcomes: the caller read HTTP 500, the request row WAS terminal and had
left the pending inbox, and the run was stranded. 500 alone reads as "the
rejection did not happen", so callers retried against a durable decision.
The status code does not move — the effect landing while the run strands
is still a failure — and the door does not become atomic: the #13937
shape-4 ruling binds this door's own writes too, so no decision is ever
rolled back. What changed is that the door stops discarding what the
engine said.
- `serviceResume` carries `AutomationResult.status` through. It read only
success/code/error, and the stranded exit reports a status and NO code,
so the repairability signal died one line before the envelope was built
— a member with a producer and, until now, zero consumers.
- `resumeRecordedOutcome` throws a carrier with `finalized` (the decision
stands), `decision`, `runId`, and `repairable` derived from the engine's
`'stranded'` discriminator. Absence of that stamp is `false`, never a
default: a repair verb that would refuse is worse than no promise.
- The REST approvals door forwards those four fields on `RESUME_FAILED`
only, presence-gated — an error with no carrier answers exactly the body
it always did.
- `@objectstack/types` hosts the constructor and its recogniser in one
module (the producer is a plugin; rest cannot import one), the same Home
rule as the validation-failure pair beside it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
… dispatch predicates
`check:engine-double-contract` and `check:objectql-double-limit` both caught
the new fake in `decision-strand-envelope.test.ts`: its `update()`/`delete()`
hand-rolled the dispatch and its `find()` read the caller's bound by
truthiness, so `limit: 0` would have returned the whole table. Both verbs now
open with `assertEngineUpdateDispatch` / `assertEngineDeleteDispatch` and the
bound is honoured by presence; the two new rows are recorded in the pinned
ledger (`--write`, 2 added, 0 lost).
`check:system-context-census` anchors are re-anchored by its own `--fix`: pure
line rot from this branch's edits, uniform +5 on `rest-server.ts` (the import
block) and +52 on `approval-service.ts` (the docblocks), which is what makes it
rot rather than a finding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/plugin-approvals, @objectstack/rest, @objectstack/types, touching 18 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/types/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/api/client-sdk.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), approvals.getRequest (sdk, the route ledger binds it to GET /api/v1/approvals/requests/:id, selected by route anchor /approvals/requests/:id), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/api/plugin-endpoints.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/approvals.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/flows.mdx(via ApprovalService (symbol, a top-level class), runId (symbol, a field of interface StrandedDecisionDetails), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/kernel/services-checklist.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/releases/v17.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /reports/schedules/:scheduleId (route, bridged from symbol strandedDecisionDetails — its route source's handler names it))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/types/src/index.ts) — pages documenting those are invisible to this run
  • 5 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 — 16 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25packageMentionDocs.

Which tree this was computed on

This run read content/docs from 0f9dde70153d16387fb288d3b46915b923281d6a — the merge of head 13b58ed7d7c116c6086cf37e63075b9c9e09b620 into base f01adfa5c9bb75b373b077718c3a8daa74c99f25, 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 0f9dde70153d16387fb288d3b46915b923281d6a && git checkout 0f9dde70153d16387fb288d3b46915b923281d6a
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f01adfa5c9bb75b373b077718c3a8daa74c99f25 13b58ed7d7c116c6086cf37e63075b9c9e09b620 && git checkout -B drift-repro f01adfa5c9bb75b373b077718c3a8daa74c99f25 && git merge --no-ff 13b58ed7d7c116c6086cf37e63075b9c9e09b620
node scripts/docs-audit/affected-docs.mjs --json f01adfa5c9bb75b373b077718c3a8daa74c99f25

⚠️ 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM pre-verification — domain:services seat (session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909). ⛔ This is not the contract review. A PASS at CONTRACT_REVIEW_TIER is still owed and this PR does not land without it. Posted because the tier review is currently blocked on an account-level claude-fable-5-1 quota (three HTTP 429s between 15:57Z and 16:57Z), so this seat verified the two mechanical claims at its own tier to leave the scarce review budget for judgement.

Claim: the ledger --write was additive — 2 rows added, 0 lost

Confirmed, from the diff rather than the tool's own summary.scripts/engine-double-contract.pinned.json, three-dot against origin/main:

  • 10 added lines, 0 removed lines — no row rewritten, none dropped.
  • The 10 lines form exactly two objects, both for the new test file:
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "delete", "pinned": 1 }
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "update", "pinned": 1 }

A --write that silently dropped a row would show as removed lines. There are none.

Claim: disjoint from #15358

Confirmed textually. Across the PR's 725 changed lines (diff 54,196 bytes):

symboloccurrences in changed lines
ApprovalResumeSurface0
getRun0
StrandedRunState0
serviceResume (positive control)3

⚠️ The control is the point. My first attempt at this scan used a -- 'packages/**/*.ts' pathspec and returned 0 for everything, control included — git's fnmatch does not cross / on ** without :(glob) magic, so the pathspec matched nothing. Those zeros were NOT MEASURED, not absence. Re-run without the pathspec, the control is non-zero and the three zeros are readings.

⚠️ One anomaly, chased rather than waved past: resumeRecordedOutcome also reads 0 in changed lines, although the dev's report says it now throws the carrier. That is benign and explains itself — the symbol appears 6 times on origin/main and 6 times on this head in approval-service.ts (declared :2836, called at :2905 / :3303 / :3345 / :3435), so only its body changed and its name never appears on a +/- line. The report is accurate; the symbol is simply a weak control, which is why serviceResume is the one carrying the scan.

What this does and does not establish. It shows the PR's changed lines never mention #15358's members, which is exactly the "does not modify the ApprovalResumeSurface interface at all" claim. ⛔ It does not prove a semantically clean merge — two edits can sit in one file region without sharing a symbol. The PR's own advice stands: whichever lands second re-runs the approvals suite rather than assuming.

Also already measured by this seat, do not re-derive

  • Three-dot origin/main...13b58ed7d = 10 files, +719, −11, matching the PR's own numbers.
  • node scripts/check-adr-0087-registration.mjs --base origin/main --head 13b58ed7dexit 0, captured by redirect rather than through a pipe. This changeset declares no BREAKING, so no adr-0087: marker is owed. (Contrast PR feat(service-automation): evaluate a value-role CEL envelope in the assignment executor and validate it at registerFlow (#15137) #15432, where the same gate was exit 1 — the two make a working pair of controls.)
  • CI green at 15:5xZ with Check Changeset success; one Test Core shard still in progress at that reading, neither passed nor failed.
  • No governed surface touched; nothing under content/docs/releases/.

⇒ Ruling compliance (is the generic resume door the same seam?), the finalized: true durability question, and repairable in the direction nobody checks are untouched by any of the above and remain the review's to decide.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationneeds:contract-reviewteststooling

Projects

None yet

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500 - #15436

Draft
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope
Draft

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500#15436
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#13807

Implements the maintainer ruling of 2026-09-04 (decision batch #37, verbatim 「同意」 on 1B · 2及) — option B, the truthful envelope.

What the ruling changed, and what it deliberately did not

One POST /api/v1/approvals/requests/{id}/reject produced three coexisting outcomes: the caller read HTTP 500, the request row wasrejected and had left the pending inbox, and the workflow run was stranded. A caller — human, script, or agent — reads 500 as "the rejection did not happen" and retries or escalates. It did happen.

The change

serviceResume carries status. It read only success / code / error. The engine's stranded exit reports status: 'stranded' and no code at all, so a door reading only the code saw an unnamed failure and could not tell a repairable strand from a dead run. AutomationResult.status: 'stranded' had a producer (PR #15237) and zero consumers — this is its first.

The decision doors build the envelope.resumeRecordedOutcome now throws a carrier with finalized (the decision stands), decision, runId, and repairable derived from the engine's own discriminator. All four sibling doors on the same path pass their outcome label: decide (approve / reject), the auto-rejection, the send-back (revise) and the resubmit.

repairable is producer-first.true only when the engine said 'stranded' — the one exit that journals a repair snapshot. Every other failure, including a lost run and an engine too old to report a status, is honestly false: absence of the signal is not repairability, and a repair verb that would refuse is worse than no promise.

The REST door forwards the four fields, anchored to RESUME_FAILED and presence-gated. A RESUME_FAILED with no carrier answers exactly the body it always did — the door never synthesises the envelope.

Home rule.strandedDecisionFailure / strandedDecisionDetails live in @objectstack/types because the producer is a plugin and @objectstack/rest cannot import one — the same reasoning, and the same constructor-and-recogniser-in-one-module discipline, as the validation-failure pair beside it. That removes the stringly-typed cross-package agreement about a property name that nothing would have checked.

Measured before implementing — the PM's assumptions, verified rather than inherited

  1. serviceResume's cast (approval-service.ts:2668 on the base, not :2667) read exactly success / code / error. The literal object approvals receives on the stranded exit carries status: 'stranded' and no code — pinned in PIN 3 by intercepting the real engine's return value, so the pin reds if the producer's shape moves and not only the door's.
  2. 'stranded' had zero consumers outside service-automation and spec. Positive control RESUME_IN_PROGRESS, same grep shape, finds five directories including packages/runtime and packages/client — the scan is not over-filtered.
  3. ⚠️The generic door is NOT the same seam — measured, and therefore filed rather than fixed here. It is already filed as automation resume door: the 400 FLOW_FAILED envelope drops the engine's status: 'stranded' verdict — the wire mirror's member is unreachable on the wire #15221; the full measurement is recorded there. In short: no shared code (runtime/domains/automation.ts contains no serviceResume / RESUME_FAILED / handleApprovalError); a wire-shaping omission rather than a lossy relay; a different code, owner and status (400 FLOW_FAILED, registered to @objectstack/runtime); three of the four fields have no referent there; and an approval node cannot reach that arm at all, since resumeAuthority: 'service' makes the generic resume answer 403 first.
  4. The RESUME_FAILED-on-a-200-body pin is untouched, deliberately.approval-restart-resume.test.ts:272 pins the token appearing in resumeError on the no-engine composition. That path returns rather than throwing, so it builds no envelope and this change adds nothing to it — and it should stay that way: that outcome is not stranded and not repairable, so a repairable flag there would be a lie. Renaming the token on the 200 body would be a separate contract change nobody ruled on. The full plugin-approvals suite (665 tests) is green.

Tests

Three pins, as the ruling specified, in decision-strand-envelope.test.ts — driven through a realAutomationEngine and a realApprovalService, with the reject-branch node throwing the card's own update_record(...) not found text:

  1. the three-outcome reproduction, asserting the new fields and that the row is durably rejected with its audit action written;
  2. a healthy decision unchanged — the reverse control, where only the downstream node differs;
  3. 'stranded' observed at the door, plus its own reverse control: a RUN_NOT_FOUND failure the engine does not call stranded reports repairable: false while still naming its run.

stranded-decision.test.ts pins the carrier itself, including that a partial carrier is refused rather than published as half an envelope (with an all-present control), and that the reader narrows to exactly the four declared keys. rest-approvals-wire-codes.test.ts gains the wire round-trip — built through the shared constructor, asserting the body's exact key set — and a reverse control proving a carrier-less error answers the old two-key body.

Ablation (committed first, so the restore leg had a real reference): removing err.resumeStatus = reported.status flips repairabletrue → false and reds PIN 1 and PIN 3; removing the REST forwarding reds the new wire case while its reverse control correctly stays green. Both files are imported relatively by their tests, so vitest resolves them from source and no dist leg is involved. Each mutation was confirmed on disk by marker count (1 → 0) before the run, and each restore was proven by git hash-object equalling the HEAD blob, with git diff HEAD empty afterwards.

Gates

Derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands against the actual diff, and re-derived after the changed-file set moved (the docs .mdx and the ledger .json added 34 families). Final sweep at 13b58ed7d: 77 green, zero failures attributable to this diff. Exit codes captured by redirecting before reading, never across a pipe; verdicts read from each gate's own line.

Two gates the diff genuinely tripped, both fixed here: check:engine-double-contract and check:objectql-double-limit caught the new fake engine hand-rolling its dispatch and reading the caller's bound by truthiness (limit: 0 would have returned the whole table). Both verbs now open with assertEngineUpdateDispatch / assertEngineDeleteDispatch, the bound is honoured by presence, and the ledger records the two new pinned rows (--write: 2 added, 0 lost — additive). check:system-context-census line rot was repaired by its own --fix: uniform +5 on rest-server.ts (the import block) and +52 on approval-service.ts (the docblocks), which is what makes it rot rather than a finding.

Six non-zero results, none of them this diff:

Contract review and scope

Clause-② yes — an additive change to a published route's error body. Opened as a draft with needs:contract-review; ⛔ not undrafted, armed or enqueued until seat-internal review returns PASS at tier.

⚠️The ApprovalDecisionResult docblock (ruling item 2) is NOT in this PR. It lives in packages/spec/src/contracts/approval-service.ts:622, a single-owner lane this seat is read-only in. The contract type itself needs no new fields — the four facts ride the error body, not the success shape — so what is owed there is the docblock recording that the fields are the published way to read the declared posture. Reported for the spec seat to file.

⚠️Declared file surface extended beyond the claim. The claim named plugin-approvals plus runtime if measured same-seam. Satisfying the ruling's own words — "the error body gains …" — requires the REST door that builds that body (packages/rest/src/rest-server.ts) and the shared carrier's home (packages/types). Declared here rather than landed quietly.

⚠️#15358 touches this same file and is ruled B but undispatched. It lands on ApprovalResumeSurface.getRun and StrandedRunState; this PR touches serviceResume / resumeRecordedOutcome and the four call sites, and does not change the ApprovalResumeSurface interface at all. Different members, so the two should merge cleanly — but whichever lands second should re-run the approvals suite rather than assume it.

⛔ Out of scope and untouched: #15389, #15360, the afterDelete reach of #14061's auto-cancel (NOT MEASURED, stated), the resume ordering, and #13568.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y


Generated by Claude Code

os-warrenand others added 2 commits September 4, 2026 14:58
…cision/runId/repairable beside its 500
Maintainer ruling 2026-09-04, decision batch #37, option B. One
`POST /api/v1/approvals/requests/{id}/reject` produced three coexisting
outcomes: the caller read HTTP 500, the request row WAS terminal and had
left the pending inbox, and the run was stranded. 500 alone reads as "the
rejection did not happen", so callers retried against a durable decision.
The status code does not move — the effect landing while the run strands
is still a failure — and the door does not become atomic: the #13937
shape-4 ruling binds this door's own writes too, so no decision is ever
rolled back. What changed is that the door stops discarding what the
engine said.
- `serviceResume` carries `AutomationResult.status` through. It read only
success/code/error, and the stranded exit reports a status and NO code,
so the repairability signal died one line before the envelope was built
— a member with a producer and, until now, zero consumers.
- `resumeRecordedOutcome` throws a carrier with `finalized` (the decision
stands), `decision`, `runId`, and `repairable` derived from the engine's
`'stranded'` discriminator. Absence of that stamp is `false`, never a
default: a repair verb that would refuse is worse than no promise.
- The REST approvals door forwards those four fields on `RESUME_FAILED`
only, presence-gated — an error with no carrier answers exactly the body
it always did.
- `@objectstack/types` hosts the constructor and its recogniser in one
module (the producer is a plugin; rest cannot import one), the same Home
rule as the validation-failure pair beside it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
… dispatch predicates
`check:engine-double-contract` and `check:objectql-double-limit` both caught
the new fake in `decision-strand-envelope.test.ts`: its `update()`/`delete()`
hand-rolled the dispatch and its `find()` read the caller's bound by
truthiness, so `limit: 0` would have returned the whole table. Both verbs now
open with `assertEngineUpdateDispatch` / `assertEngineDeleteDispatch` and the
bound is honoured by presence; the two new rows are recorded in the pinned
ledger (`--write`, 2 added, 0 lost).
`check:system-context-census` anchors are re-anchored by its own `--fix`: pure
line rot from this branch's edits, uniform +5 on `rest-server.ts` (the import
block) and +52 on `approval-service.ts` (the docblocks), which is what makes it
rot rather than a finding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/plugin-approvals, @objectstack/rest, @objectstack/types, touching 18 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/types/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/api/client-sdk.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), approvals.getRequest (sdk, the route ledger binds it to GET /api/v1/approvals/requests/:id, selected by route anchor /approvals/requests/:id), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/api/plugin-endpoints.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/approvals.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/flows.mdx(via ApprovalService (symbol, a top-level class), runId (symbol, a field of interface StrandedDecisionDetails), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/kernel/services-checklist.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/releases/v17.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /reports/schedules/:scheduleId (route, bridged from symbol strandedDecisionDetails — its route source's handler names it))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/types/src/index.ts) — pages documenting those are invisible to this run
  • 5 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 — 16 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25packageMentionDocs.

Which tree this was computed on

This run read content/docs from 0f9dde70153d16387fb288d3b46915b923281d6a — the merge of head 13b58ed7d7c116c6086cf37e63075b9c9e09b620 into base f01adfa5c9bb75b373b077718c3a8daa74c99f25, 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 0f9dde70153d16387fb288d3b46915b923281d6a && git checkout 0f9dde70153d16387fb288d3b46915b923281d6a
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f01adfa5c9bb75b373b077718c3a8daa74c99f25 13b58ed7d7c116c6086cf37e63075b9c9e09b620 && git checkout -B drift-repro f01adfa5c9bb75b373b077718c3a8daa74c99f25 && git merge --no-ff 13b58ed7d7c116c6086cf37e63075b9c9e09b620
node scripts/docs-audit/affected-docs.mjs --json f01adfa5c9bb75b373b077718c3a8daa74c99f25

⚠️ 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM pre-verification — domain:services seat (session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909). ⛔ This is not the contract review. A PASS at CONTRACT_REVIEW_TIER is still owed and this PR does not land without it. Posted because the tier review is currently blocked on an account-level claude-fable-5-1 quota (three HTTP 429s between 15:57Z and 16:57Z), so this seat verified the two mechanical claims at its own tier to leave the scarce review budget for judgement.

Claim: the ledger --write was additive — 2 rows added, 0 lost

Confirmed, from the diff rather than the tool's own summary.scripts/engine-double-contract.pinned.json, three-dot against origin/main:

  • 10 added lines, 0 removed lines — no row rewritten, none dropped.
  • The 10 lines form exactly two objects, both for the new test file:
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "delete", "pinned": 1 }
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "update", "pinned": 1 }

A --write that silently dropped a row would show as removed lines. There are none.

Claim: disjoint from #15358

Confirmed textually. Across the PR's 725 changed lines (diff 54,196 bytes):

symboloccurrences in changed lines
ApprovalResumeSurface0
getRun0
StrandedRunState0
serviceResume (positive control)3

⚠️ The control is the point. My first attempt at this scan used a -- 'packages/**/*.ts' pathspec and returned 0 for everything, control included — git's fnmatch does not cross / on ** without :(glob) magic, so the pathspec matched nothing. Those zeros were NOT MEASURED, not absence. Re-run without the pathspec, the control is non-zero and the three zeros are readings.

⚠️ One anomaly, chased rather than waved past: resumeRecordedOutcome also reads 0 in changed lines, although the dev's report says it now throws the carrier. That is benign and explains itself — the symbol appears 6 times on origin/main and 6 times on this head in approval-service.ts (declared :2836, called at :2905 / :3303 / :3345 / :3435), so only its body changed and its name never appears on a +/- line. The report is accurate; the symbol is simply a weak control, which is why serviceResume is the one carrying the scan.

What this does and does not establish. It shows the PR's changed lines never mention #15358's members, which is exactly the "does not modify the ApprovalResumeSurface interface at all" claim. ⛔ It does not prove a semantically clean merge — two edits can sit in one file region without sharing a symbol. The PR's own advice stands: whichever lands second re-runs the approvals suite rather than assuming.

Also already measured by this seat, do not re-derive

  • Three-dot origin/main...13b58ed7d = 10 files, +719, −11, matching the PR's own numbers.
  • node scripts/check-adr-0087-registration.mjs --base origin/main --head 13b58ed7dexit 0, captured by redirect rather than through a pipe. This changeset declares no BREAKING, so no adr-0087: marker is owed. (Contrast PR feat(service-automation): evaluate a value-role CEL envelope in the assignment executor and validate it at registerFlow (#15137) #15432, where the same gate was exit 1 — the two make a working pair of controls.)
  • CI green at 15:5xZ with Check Changeset success; one Test Core shard still in progress at that reading, neither passed nor failed.
  • No governed surface touched; nothing under content/docs/releases/.

⇒ Ruling compliance (is the generic resume door the same seam?), the finalized: true durability question, and repairable in the direction nobody checks are untouched by any of the above and remain the review's to decide.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationneeds:contract-reviewteststooling

Projects

None yet

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500 - #15436

Draft
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope
Draft

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500#15436
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#13807

Implements the maintainer ruling of 2026-09-04 (decision batch #37, verbatim 「同意」 on 1B · 2及) — option B, the truthful envelope.

What the ruling changed, and what it deliberately did not

One POST /api/v1/approvals/requests/{id}/reject produced three coexisting outcomes: the caller read HTTP 500, the request row wasrejected and had left the pending inbox, and the workflow run was stranded. A caller — human, script, or agent — reads 500 as "the rejection did not happen" and retries or escalates. It did happen.

The change

serviceResume carries status. It read only success / code / error. The engine's stranded exit reports status: 'stranded' and no code at all, so a door reading only the code saw an unnamed failure and could not tell a repairable strand from a dead run. AutomationResult.status: 'stranded' had a producer (PR #15237) and zero consumers — this is its first.

The decision doors build the envelope.resumeRecordedOutcome now throws a carrier with finalized (the decision stands), decision, runId, and repairable derived from the engine's own discriminator. All four sibling doors on the same path pass their outcome label: decide (approve / reject), the auto-rejection, the send-back (revise) and the resubmit.

repairable is producer-first.true only when the engine said 'stranded' — the one exit that journals a repair snapshot. Every other failure, including a lost run and an engine too old to report a status, is honestly false: absence of the signal is not repairability, and a repair verb that would refuse is worse than no promise.

The REST door forwards the four fields, anchored to RESUME_FAILED and presence-gated. A RESUME_FAILED with no carrier answers exactly the body it always did — the door never synthesises the envelope.

Home rule.strandedDecisionFailure / strandedDecisionDetails live in @objectstack/types because the producer is a plugin and @objectstack/rest cannot import one — the same reasoning, and the same constructor-and-recogniser-in-one-module discipline, as the validation-failure pair beside it. That removes the stringly-typed cross-package agreement about a property name that nothing would have checked.

Measured before implementing — the PM's assumptions, verified rather than inherited

  1. serviceResume's cast (approval-service.ts:2668 on the base, not :2667) read exactly success / code / error. The literal object approvals receives on the stranded exit carries status: 'stranded' and no code — pinned in PIN 3 by intercepting the real engine's return value, so the pin reds if the producer's shape moves and not only the door's.
  2. 'stranded' had zero consumers outside service-automation and spec. Positive control RESUME_IN_PROGRESS, same grep shape, finds five directories including packages/runtime and packages/client — the scan is not over-filtered.
  3. ⚠️The generic door is NOT the same seam — measured, and therefore filed rather than fixed here. It is already filed as automation resume door: the 400 FLOW_FAILED envelope drops the engine's status: 'stranded' verdict — the wire mirror's member is unreachable on the wire #15221; the full measurement is recorded there. In short: no shared code (runtime/domains/automation.ts contains no serviceResume / RESUME_FAILED / handleApprovalError); a wire-shaping omission rather than a lossy relay; a different code, owner and status (400 FLOW_FAILED, registered to @objectstack/runtime); three of the four fields have no referent there; and an approval node cannot reach that arm at all, since resumeAuthority: 'service' makes the generic resume answer 403 first.
  4. The RESUME_FAILED-on-a-200-body pin is untouched, deliberately.approval-restart-resume.test.ts:272 pins the token appearing in resumeError on the no-engine composition. That path returns rather than throwing, so it builds no envelope and this change adds nothing to it — and it should stay that way: that outcome is not stranded and not repairable, so a repairable flag there would be a lie. Renaming the token on the 200 body would be a separate contract change nobody ruled on. The full plugin-approvals suite (665 tests) is green.

Tests

Three pins, as the ruling specified, in decision-strand-envelope.test.ts — driven through a realAutomationEngine and a realApprovalService, with the reject-branch node throwing the card's own update_record(...) not found text:

  1. the three-outcome reproduction, asserting the new fields and that the row is durably rejected with its audit action written;
  2. a healthy decision unchanged — the reverse control, where only the downstream node differs;
  3. 'stranded' observed at the door, plus its own reverse control: a RUN_NOT_FOUND failure the engine does not call stranded reports repairable: false while still naming its run.

stranded-decision.test.ts pins the carrier itself, including that a partial carrier is refused rather than published as half an envelope (with an all-present control), and that the reader narrows to exactly the four declared keys. rest-approvals-wire-codes.test.ts gains the wire round-trip — built through the shared constructor, asserting the body's exact key set — and a reverse control proving a carrier-less error answers the old two-key body.

Ablation (committed first, so the restore leg had a real reference): removing err.resumeStatus = reported.status flips repairabletrue → false and reds PIN 1 and PIN 3; removing the REST forwarding reds the new wire case while its reverse control correctly stays green. Both files are imported relatively by their tests, so vitest resolves them from source and no dist leg is involved. Each mutation was confirmed on disk by marker count (1 → 0) before the run, and each restore was proven by git hash-object equalling the HEAD blob, with git diff HEAD empty afterwards.

Gates

Derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands against the actual diff, and re-derived after the changed-file set moved (the docs .mdx and the ledger .json added 34 families). Final sweep at 13b58ed7d: 77 green, zero failures attributable to this diff. Exit codes captured by redirecting before reading, never across a pipe; verdicts read from each gate's own line.

Two gates the diff genuinely tripped, both fixed here: check:engine-double-contract and check:objectql-double-limit caught the new fake engine hand-rolling its dispatch and reading the caller's bound by truthiness (limit: 0 would have returned the whole table). Both verbs now open with assertEngineUpdateDispatch / assertEngineDeleteDispatch, the bound is honoured by presence, and the ledger records the two new pinned rows (--write: 2 added, 0 lost — additive). check:system-context-census line rot was repaired by its own --fix: uniform +5 on rest-server.ts (the import block) and +52 on approval-service.ts (the docblocks), which is what makes it rot rather than a finding.

Six non-zero results, none of them this diff:

Contract review and scope

Clause-② yes — an additive change to a published route's error body. Opened as a draft with needs:contract-review; ⛔ not undrafted, armed or enqueued until seat-internal review returns PASS at tier.

⚠️The ApprovalDecisionResult docblock (ruling item 2) is NOT in this PR. It lives in packages/spec/src/contracts/approval-service.ts:622, a single-owner lane this seat is read-only in. The contract type itself needs no new fields — the four facts ride the error body, not the success shape — so what is owed there is the docblock recording that the fields are the published way to read the declared posture. Reported for the spec seat to file.

⚠️Declared file surface extended beyond the claim. The claim named plugin-approvals plus runtime if measured same-seam. Satisfying the ruling's own words — "the error body gains …" — requires the REST door that builds that body (packages/rest/src/rest-server.ts) and the shared carrier's home (packages/types). Declared here rather than landed quietly.

⚠️#15358 touches this same file and is ruled B but undispatched. It lands on ApprovalResumeSurface.getRun and StrandedRunState; this PR touches serviceResume / resumeRecordedOutcome and the four call sites, and does not change the ApprovalResumeSurface interface at all. Different members, so the two should merge cleanly — but whichever lands second should re-run the approvals suite rather than assume it.

⛔ Out of scope and untouched: #15389, #15360, the afterDelete reach of #14061's auto-cancel (NOT MEASURED, stated), the resume ordering, and #13568.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y


Generated by Claude Code

os-warrenand others added 2 commits September 4, 2026 14:58
…cision/runId/repairable beside its 500
Maintainer ruling 2026-09-04, decision batch #37, option B. One
`POST /api/v1/approvals/requests/{id}/reject` produced three coexisting
outcomes: the caller read HTTP 500, the request row WAS terminal and had
left the pending inbox, and the run was stranded. 500 alone reads as "the
rejection did not happen", so callers retried against a durable decision.
The status code does not move — the effect landing while the run strands
is still a failure — and the door does not become atomic: the #13937
shape-4 ruling binds this door's own writes too, so no decision is ever
rolled back. What changed is that the door stops discarding what the
engine said.
- `serviceResume` carries `AutomationResult.status` through. It read only
success/code/error, and the stranded exit reports a status and NO code,
so the repairability signal died one line before the envelope was built
— a member with a producer and, until now, zero consumers.
- `resumeRecordedOutcome` throws a carrier with `finalized` (the decision
stands), `decision`, `runId`, and `repairable` derived from the engine's
`'stranded'` discriminator. Absence of that stamp is `false`, never a
default: a repair verb that would refuse is worse than no promise.
- The REST approvals door forwards those four fields on `RESUME_FAILED`
only, presence-gated — an error with no carrier answers exactly the body
it always did.
- `@objectstack/types` hosts the constructor and its recogniser in one
module (the producer is a plugin; rest cannot import one), the same Home
rule as the validation-failure pair beside it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
… dispatch predicates
`check:engine-double-contract` and `check:objectql-double-limit` both caught
the new fake in `decision-strand-envelope.test.ts`: its `update()`/`delete()`
hand-rolled the dispatch and its `find()` read the caller's bound by
truthiness, so `limit: 0` would have returned the whole table. Both verbs now
open with `assertEngineUpdateDispatch` / `assertEngineDeleteDispatch` and the
bound is honoured by presence; the two new rows are recorded in the pinned
ledger (`--write`, 2 added, 0 lost).
`check:system-context-census` anchors are re-anchored by its own `--fix`: pure
line rot from this branch's edits, uniform +5 on `rest-server.ts` (the import
block) and +52 on `approval-service.ts` (the docblocks), which is what makes it
rot rather than a finding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/plugin-approvals, @objectstack/rest, @objectstack/types, touching 18 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/types/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/api/client-sdk.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), approvals.getRequest (sdk, the route ledger binds it to GET /api/v1/approvals/requests/:id, selected by route anchor /approvals/requests/:id), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/api/plugin-endpoints.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/approvals.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/flows.mdx(via ApprovalService (symbol, a top-level class), runId (symbol, a field of interface StrandedDecisionDetails), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/kernel/services-checklist.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/releases/v17.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /reports/schedules/:scheduleId (route, bridged from symbol strandedDecisionDetails — its route source's handler names it))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/types/src/index.ts) — pages documenting those are invisible to this run
  • 5 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 — 16 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25packageMentionDocs.

Which tree this was computed on

This run read content/docs from 0f9dde70153d16387fb288d3b46915b923281d6a — the merge of head 13b58ed7d7c116c6086cf37e63075b9c9e09b620 into base f01adfa5c9bb75b373b077718c3a8daa74c99f25, 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 0f9dde70153d16387fb288d3b46915b923281d6a && git checkout 0f9dde70153d16387fb288d3b46915b923281d6a
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f01adfa5c9bb75b373b077718c3a8daa74c99f25 13b58ed7d7c116c6086cf37e63075b9c9e09b620 && git checkout -B drift-repro f01adfa5c9bb75b373b077718c3a8daa74c99f25 && git merge --no-ff 13b58ed7d7c116c6086cf37e63075b9c9e09b620
node scripts/docs-audit/affected-docs.mjs --json f01adfa5c9bb75b373b077718c3a8daa74c99f25

⚠️ 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM pre-verification — domain:services seat (session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909). ⛔ This is not the contract review. A PASS at CONTRACT_REVIEW_TIER is still owed and this PR does not land without it. Posted because the tier review is currently blocked on an account-level claude-fable-5-1 quota (three HTTP 429s between 15:57Z and 16:57Z), so this seat verified the two mechanical claims at its own tier to leave the scarce review budget for judgement.

Claim: the ledger --write was additive — 2 rows added, 0 lost

Confirmed, from the diff rather than the tool's own summary.scripts/engine-double-contract.pinned.json, three-dot against origin/main:

  • 10 added lines, 0 removed lines — no row rewritten, none dropped.
  • The 10 lines form exactly two objects, both for the new test file:
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "delete", "pinned": 1 }
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "update", "pinned": 1 }

A --write that silently dropped a row would show as removed lines. There are none.

Claim: disjoint from #15358

Confirmed textually. Across the PR's 725 changed lines (diff 54,196 bytes):

symboloccurrences in changed lines
ApprovalResumeSurface0
getRun0
StrandedRunState0
serviceResume (positive control)3

⚠️ The control is the point. My first attempt at this scan used a -- 'packages/**/*.ts' pathspec and returned 0 for everything, control included — git's fnmatch does not cross / on ** without :(glob) magic, so the pathspec matched nothing. Those zeros were NOT MEASURED, not absence. Re-run without the pathspec, the control is non-zero and the three zeros are readings.

⚠️ One anomaly, chased rather than waved past: resumeRecordedOutcome also reads 0 in changed lines, although the dev's report says it now throws the carrier. That is benign and explains itself — the symbol appears 6 times on origin/main and 6 times on this head in approval-service.ts (declared :2836, called at :2905 / :3303 / :3345 / :3435), so only its body changed and its name never appears on a +/- line. The report is accurate; the symbol is simply a weak control, which is why serviceResume is the one carrying the scan.

What this does and does not establish. It shows the PR's changed lines never mention #15358's members, which is exactly the "does not modify the ApprovalResumeSurface interface at all" claim. ⛔ It does not prove a semantically clean merge — two edits can sit in one file region without sharing a symbol. The PR's own advice stands: whichever lands second re-runs the approvals suite rather than assuming.

Also already measured by this seat, do not re-derive

  • Three-dot origin/main...13b58ed7d = 10 files, +719, −11, matching the PR's own numbers.
  • node scripts/check-adr-0087-registration.mjs --base origin/main --head 13b58ed7dexit 0, captured by redirect rather than through a pipe. This changeset declares no BREAKING, so no adr-0087: marker is owed. (Contrast PR feat(service-automation): evaluate a value-role CEL envelope in the assignment executor and validate it at registerFlow (#15137) #15432, where the same gate was exit 1 — the two make a working pair of controls.)
  • CI green at 15:5xZ with Check Changeset success; one Test Core shard still in progress at that reading, neither passed nor failed.
  • No governed surface touched; nothing under content/docs/releases/.

⇒ Ruling compliance (is the generic resume door the same seam?), the finalized: true durability question, and repairable in the direction nobody checks are untouched by any of the above and remain the review's to decide.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationneeds:contract-reviewteststooling

Projects

None yet

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500 - #15436

Draft
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope
Draft

fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500#15436
os-warren wants to merge 2 commits into
mainfrom
claude/issue-13807-truthful-reject-envelope

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#13807

Implements the maintainer ruling of 2026-09-04 (decision batch #37, verbatim 「同意」 on 1B · 2及) — option B, the truthful envelope.

What the ruling changed, and what it deliberately did not

One POST /api/v1/approvals/requests/{id}/reject produced three coexisting outcomes: the caller read HTTP 500, the request row wasrejected and had left the pending inbox, and the workflow run was stranded. A caller — human, script, or agent — reads 500 as "the rejection did not happen" and retries or escalates. It did happen.

The change

serviceResume carries status. It read only success / code / error. The engine's stranded exit reports status: 'stranded' and no code at all, so a door reading only the code saw an unnamed failure and could not tell a repairable strand from a dead run. AutomationResult.status: 'stranded' had a producer (PR #15237) and zero consumers — this is its first.

The decision doors build the envelope.resumeRecordedOutcome now throws a carrier with finalized (the decision stands), decision, runId, and repairable derived from the engine's own discriminator. All four sibling doors on the same path pass their outcome label: decide (approve / reject), the auto-rejection, the send-back (revise) and the resubmit.

repairable is producer-first.true only when the engine said 'stranded' — the one exit that journals a repair snapshot. Every other failure, including a lost run and an engine too old to report a status, is honestly false: absence of the signal is not repairability, and a repair verb that would refuse is worse than no promise.

The REST door forwards the four fields, anchored to RESUME_FAILED and presence-gated. A RESUME_FAILED with no carrier answers exactly the body it always did — the door never synthesises the envelope.

Home rule.strandedDecisionFailure / strandedDecisionDetails live in @objectstack/types because the producer is a plugin and @objectstack/rest cannot import one — the same reasoning, and the same constructor-and-recogniser-in-one-module discipline, as the validation-failure pair beside it. That removes the stringly-typed cross-package agreement about a property name that nothing would have checked.

Measured before implementing — the PM's assumptions, verified rather than inherited

  1. serviceResume's cast (approval-service.ts:2668 on the base, not :2667) read exactly success / code / error. The literal object approvals receives on the stranded exit carries status: 'stranded' and no code — pinned in PIN 3 by intercepting the real engine's return value, so the pin reds if the producer's shape moves and not only the door's.
  2. 'stranded' had zero consumers outside service-automation and spec. Positive control RESUME_IN_PROGRESS, same grep shape, finds five directories including packages/runtime and packages/client — the scan is not over-filtered.
  3. ⚠️The generic door is NOT the same seam — measured, and therefore filed rather than fixed here. It is already filed as automation resume door: the 400 FLOW_FAILED envelope drops the engine's status: 'stranded' verdict — the wire mirror's member is unreachable on the wire #15221; the full measurement is recorded there. In short: no shared code (runtime/domains/automation.ts contains no serviceResume / RESUME_FAILED / handleApprovalError); a wire-shaping omission rather than a lossy relay; a different code, owner and status (400 FLOW_FAILED, registered to @objectstack/runtime); three of the four fields have no referent there; and an approval node cannot reach that arm at all, since resumeAuthority: 'service' makes the generic resume answer 403 first.
  4. The RESUME_FAILED-on-a-200-body pin is untouched, deliberately.approval-restart-resume.test.ts:272 pins the token appearing in resumeError on the no-engine composition. That path returns rather than throwing, so it builds no envelope and this change adds nothing to it — and it should stay that way: that outcome is not stranded and not repairable, so a repairable flag there would be a lie. Renaming the token on the 200 body would be a separate contract change nobody ruled on. The full plugin-approvals suite (665 tests) is green.

Tests

Three pins, as the ruling specified, in decision-strand-envelope.test.ts — driven through a realAutomationEngine and a realApprovalService, with the reject-branch node throwing the card's own update_record(...) not found text:

  1. the three-outcome reproduction, asserting the new fields and that the row is durably rejected with its audit action written;
  2. a healthy decision unchanged — the reverse control, where only the downstream node differs;
  3. 'stranded' observed at the door, plus its own reverse control: a RUN_NOT_FOUND failure the engine does not call stranded reports repairable: false while still naming its run.

stranded-decision.test.ts pins the carrier itself, including that a partial carrier is refused rather than published as half an envelope (with an all-present control), and that the reader narrows to exactly the four declared keys. rest-approvals-wire-codes.test.ts gains the wire round-trip — built through the shared constructor, asserting the body's exact key set — and a reverse control proving a carrier-less error answers the old two-key body.

Ablation (committed first, so the restore leg had a real reference): removing err.resumeStatus = reported.status flips repairabletrue → false and reds PIN 1 and PIN 3; removing the REST forwarding reds the new wire case while its reverse control correctly stays green. Both files are imported relatively by their tests, so vitest resolves them from source and no dist leg is involved. Each mutation was confirmed on disk by marker count (1 → 0) before the run, and each restore was proven by git hash-object equalling the HEAD blob, with git diff HEAD empty afterwards.

Gates

Derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands against the actual diff, and re-derived after the changed-file set moved (the docs .mdx and the ledger .json added 34 families). Final sweep at 13b58ed7d: 77 green, zero failures attributable to this diff. Exit codes captured by redirecting before reading, never across a pipe; verdicts read from each gate's own line.

Two gates the diff genuinely tripped, both fixed here: check:engine-double-contract and check:objectql-double-limit caught the new fake engine hand-rolling its dispatch and reading the caller's bound by truthiness (limit: 0 would have returned the whole table). Both verbs now open with assertEngineUpdateDispatch / assertEngineDeleteDispatch, the bound is honoured by presence, and the ledger records the two new pinned rows (--write: 2 added, 0 lost — additive). check:system-context-census line rot was repaired by its own --fix: uniform +5 on rest-server.ts (the import block) and +52 on approval-service.ts (the docblocks), which is what makes it rot rather than a finding.

Six non-zero results, none of them this diff:

Contract review and scope

Clause-② yes — an additive change to a published route's error body. Opened as a draft with needs:contract-review; ⛔ not undrafted, armed or enqueued until seat-internal review returns PASS at tier.

⚠️The ApprovalDecisionResult docblock (ruling item 2) is NOT in this PR. It lives in packages/spec/src/contracts/approval-service.ts:622, a single-owner lane this seat is read-only in. The contract type itself needs no new fields — the four facts ride the error body, not the success shape — so what is owed there is the docblock recording that the fields are the published way to read the declared posture. Reported for the spec seat to file.

⚠️Declared file surface extended beyond the claim. The claim named plugin-approvals plus runtime if measured same-seam. Satisfying the ruling's own words — "the error body gains …" — requires the REST door that builds that body (packages/rest/src/rest-server.ts) and the shared carrier's home (packages/types). Declared here rather than landed quietly.

⚠️#15358 touches this same file and is ruled B but undispatched. It lands on ApprovalResumeSurface.getRun and StrandedRunState; this PR touches serviceResume / resumeRecordedOutcome and the four call sites, and does not change the ApprovalResumeSurface interface at all. Different members, so the two should merge cleanly — but whichever lands second should re-run the approvals suite rather than assume it.

⛔ Out of scope and untouched: #15389, #15360, the afterDelete reach of #14061's auto-cancel (NOT MEASURED, stated), the resume ordering, and #13568.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y


Generated by Claude Code

os-warrenand others added 2 commits September 4, 2026 14:58
…cision/runId/repairable beside its 500
Maintainer ruling 2026-09-04, decision batch #37, option B. One
`POST /api/v1/approvals/requests/{id}/reject` produced three coexisting
outcomes: the caller read HTTP 500, the request row WAS terminal and had
left the pending inbox, and the run was stranded. 500 alone reads as "the
rejection did not happen", so callers retried against a durable decision.
The status code does not move — the effect landing while the run strands
is still a failure — and the door does not become atomic: the #13937
shape-4 ruling binds this door's own writes too, so no decision is ever
rolled back. What changed is that the door stops discarding what the
engine said.
- `serviceResume` carries `AutomationResult.status` through. It read only
success/code/error, and the stranded exit reports a status and NO code,
so the repairability signal died one line before the envelope was built
— a member with a producer and, until now, zero consumers.
- `resumeRecordedOutcome` throws a carrier with `finalized` (the decision
stands), `decision`, `runId`, and `repairable` derived from the engine's
`'stranded'` discriminator. Absence of that stamp is `false`, never a
default: a repair verb that would refuse is worse than no promise.
- The REST approvals door forwards those four fields on `RESUME_FAILED`
only, presence-gated — an error with no carrier answers exactly the body
it always did.
- `@objectstack/types` hosts the constructor and its recogniser in one
module (the producer is a plugin; rest cannot import one), the same Home
rule as the validation-failure pair beside it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
… dispatch predicates
`check:engine-double-contract` and `check:objectql-double-limit` both caught
the new fake in `decision-strand-envelope.test.ts`: its `update()`/`delete()`
hand-rolled the dispatch and its `find()` read the caller's bound by
truthiness, so `limit: 0` would have returned the whole table. Both verbs now
open with `assertEngineUpdateDispatch` / `assertEngineDeleteDispatch` and the
bound is honoured by presence; the two new rows are recorded in the pinned
ledger (`--write`, 2 added, 0 lost).
`check:system-context-census` anchors are re-anchored by its own `--fix`: pure
line rot from this branch's edits, uniform +5 on `rest-server.ts` (the import
block) and +52 on `approval-service.ts` (the docblocks), which is what makes it
rot rather than a finding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/plugin-approvals, @objectstack/rest, @objectstack/types, touching 18 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/types/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/api/client-sdk.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), approvals.getRequest (sdk, the route ledger binds it to GET /api/v1/approvals/requests/:id, selected by route anchor /approvals/requests/:id), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/api/plugin-endpoints.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/approvals.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/automation/flows.mdx(via ApprovalService (symbol, a top-level class), runId (symbol, a field of interface StrandedDecisionDetails), /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))
  • content/docs/kernel/services-checklist.mdx(via /approvals/requests/:id (route, bridged from symbol sendBack — its route source's handler names it))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id))
  • content/docs/releases/v17.mdx(via runId (symbol, a field of interface StrandedDecisionDetails), getRequest (sdk, the bare tail of client method approvals.getRequest, bound to GET /api/v1/approvals/requests/:id), /reports/schedules/:scheduleId (route, bridged from symbol strandedDecisionDetails — its route source's handler names it))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/types/src/index.ts) — pages documenting those are invisible to this run
  • 5 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 — 16 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25packageMentionDocs.

Which tree this was computed on

This run read content/docs from 0f9dde70153d16387fb288d3b46915b923281d6a — the merge of head 13b58ed7d7c116c6086cf37e63075b9c9e09b620 into base f01adfa5c9bb75b373b077718c3a8daa74c99f25, 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 0f9dde70153d16387fb288d3b46915b923281d6a && git checkout 0f9dde70153d16387fb288d3b46915b923281d6a
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f01adfa5c9bb75b373b077718c3a8daa74c99f25 13b58ed7d7c116c6086cf37e63075b9c9e09b620 && git checkout -B drift-repro f01adfa5c9bb75b373b077718c3a8daa74c99f25 && git merge --no-ff 13b58ed7d7c116c6086cf37e63075b9c9e09b620
node scripts/docs-audit/affected-docs.mjs --json f01adfa5c9bb75b373b077718c3a8daa74c99f25

⚠️ 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 f01adfa5c9bb75b373b077718c3a8daa74c99f25 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM pre-verification — domain:services seat (session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909). ⛔ This is not the contract review. A PASS at CONTRACT_REVIEW_TIER is still owed and this PR does not land without it. Posted because the tier review is currently blocked on an account-level claude-fable-5-1 quota (three HTTP 429s between 15:57Z and 16:57Z), so this seat verified the two mechanical claims at its own tier to leave the scarce review budget for judgement.

Claim: the ledger --write was additive — 2 rows added, 0 lost

Confirmed, from the diff rather than the tool's own summary.scripts/engine-double-contract.pinned.json, three-dot against origin/main:

  • 10 added lines, 0 removed lines — no row rewritten, none dropped.
  • The 10 lines form exactly two objects, both for the new test file:
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "delete", "pinned": 1 }
{ "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "update", "pinned": 1 }

A --write that silently dropped a row would show as removed lines. There are none.

Claim: disjoint from #15358

Confirmed textually. Across the PR's 725 changed lines (diff 54,196 bytes):

symboloccurrences in changed lines
ApprovalResumeSurface0
getRun0
StrandedRunState0
serviceResume (positive control)3

⚠️ The control is the point. My first attempt at this scan used a -- 'packages/**/*.ts' pathspec and returned 0 for everything, control included — git's fnmatch does not cross / on ** without :(glob) magic, so the pathspec matched nothing. Those zeros were NOT MEASURED, not absence. Re-run without the pathspec, the control is non-zero and the three zeros are readings.

⚠️ One anomaly, chased rather than waved past: resumeRecordedOutcome also reads 0 in changed lines, although the dev's report says it now throws the carrier. That is benign and explains itself — the symbol appears 6 times on origin/main and 6 times on this head in approval-service.ts (declared :2836, called at :2905 / :3303 / :3345 / :3435), so only its body changed and its name never appears on a +/- line. The report is accurate; the symbol is simply a weak control, which is why serviceResume is the one carrying the scan.

What this does and does not establish. It shows the PR's changed lines never mention #15358's members, which is exactly the "does not modify the ApprovalResumeSurface interface at all" claim. ⛔ It does not prove a semantically clean merge — two edits can sit in one file region without sharing a symbol. The PR's own advice stands: whichever lands second re-runs the approvals suite rather than assuming.

Also already measured by this seat, do not re-derive

  • Three-dot origin/main...13b58ed7d = 10 files, +719, −11, matching the PR's own numbers.
  • node scripts/check-adr-0087-registration.mjs --base origin/main --head 13b58ed7dexit 0, captured by redirect rather than through a pipe. This changeset declares no BREAKING, so no adr-0087: marker is owed. (Contrast PR feat(service-automation): evaluate a value-role CEL envelope in the assignment executor and validate it at registerFlow (#15137) #15432, where the same gate was exit 1 — the two make a working pair of controls.)
  • CI green at 15:5xZ with Check Changeset success; one Test Core shard still in progress at that reading, neither passed nor failed.
  • No governed surface touched; nothing under content/docs/releases/.

⇒ Ruling compliance (is the generic resume door the same seam?), the finalized: true durability question, and repairable in the direction nobody checks are untouched by any of the above and remain the review's to decide.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationneeds:contract-reviewteststooling

Projects

None yet

1 participant

@os-warren