Skip to content

feat(service-automation,qa): surface a failed try-region's steps so a caught failure leaves a trace (#7546) - #7699

Merged
os-help merged 1 commit into
mainfrom
claude/issue-7546-try-region-failed-steps
Aug 11, 2026
Merged

feat(service-automation,qa): surface a failed try-region's steps so a caught failure leaves a trace (#7546)#7699
os-help merged 1 commit into
mainfrom
claude/issue-7546-try-region-failed-steps

Conversation

@os-help

Copy link
Copy Markdown
Collaborator

Fixes#7546

Implements the maintainer ruling on #7546surface the failed try-region's steps, not the card's own Option C recommendation.

Ruling: implement — surface the failed try-region's steps. The catch path returns the failed attempt's childSteps (carrying regionKind: 'try' and failure status) so an operator can see what failed, how many attempts ran, and which node threw. The existing splice plumbing (#1479, per premise P3) is the vehicle — this connects an existing pipe, it does not build an observability system. Retry/throw semantics of try_catch are unchanged.

(Ruling comment of 2026-08-11, recorded from the maintainer's chat instruction 「接受你的全部建议,请更新 issue 的状态和标签」.)

Premise verification

Re-checked on this PR's base, origin/main @ 779ace4 — all five hold. Line numbers moved from the triage anchors taken at 34c01a5; nothing else did.

PremiseVerdictEvidence on this base
P1 — a failed region's partial steps are deliberately not surfacedholdsthe rationale comment is at engine.ts:5367 (triage said 5315), ending "a failed attempt's partial steps are not surfaced"
P2try-catch-node.ts returns childSteps only from a region that succeededholdsregionKind: 'try' tagging at try-catch-node.ts:109; the failed attempts' steps were dropped on the floor in the catch (err) arm
P3 — the engine splices childSteps into the parent logholdsengine.ts:5127, if (result.childSteps?.length) steps.push(...)
P4 — run summaries fold over the FLAT step logholdssummarizeRun is a pure fold in run-summary.ts
P5 — prior art on region interiors invisible to other subsystemsholds#4380, cited as analogy

One correction worth recording, because it changes what the fix had to build: the failing node's step already existed. executeNode pushes a status: 'failure' step into the region's array before it throws, so the material the card wanted was being assembled and then discarded as the region unwound. Nothing needed to be synthesised — only handed back.

What changed

engine.ts

  • runRegion() takes an optional partialSteps sink. On failure it tags the partial steps exactly as the success path does, pushes them into the sink, then rethrows unchanged. A sink rather than a return value because this path's contract is still "throw" — carrying steps out through the exception would either change what callers catch or need a bespoke error type, both larger seams. The two callers that do not opt in (loop, parallel) are untouched.
  • Tagging is factored into one local helper shared by both paths, so a failed attempt's steps are indistinguishable in shape from a successful one's; they differ only in their own status.
  • StepLogEntry gains retryAttempt. Not new vocabulary — the spec's ExecutionStepLogSchema has declared retryAttempt ("Retry attempt number (0 = first try)") since it was written, and had no producer anywhere in the engine. This gives the declared key its first writer, which is the direction declared-equals-enforced asks for. No packages/spec file is touched.

try-catch-node.ts — accumulates each failed attempt's steps across the retry ladder and folds them into childSteps ahead of the surviving region's steps, on both the catch path and the path where a retry eventually succeeds. The attempt index is tagged only when a retry policy is actually declared; otherwise every step would carry a constant retryAttempt: 0, which is noise rather than signal.

run-summary.ts — doc only, no code guard. See the P4 note below.

docs/qa/platform-checklist/areas/automation.jsonflow-error-handling rewritten, revision 1 to 2 with the matching history entry. The flow-node-type-matrix item (#7664) is untouched; verified by structural diff, exactly one item changed.

Two judgement calls, flagged for review

  1. Failed attempts are surfaced on the retry-succeeds path too, not only the catch path. The ruling's text names the catch path; the same mechanism covers both, and without it "how many attempts ran" is answerable only when the flow ultimately failed. Same pipe, no new vocabulary. Say the word and I will narrow it to the catch path alone.
  2. The no-catch failure path surfaces nothing, deliberately. The engine splices childSteps only on a successful node result, so attaching them to a failing one is dead weight; surfacing there would mean changing the splice for every container kind. That path is also not the gap this card is about — an unhandled failure already terminates the run failed with both run-level and step-level errors, which the QA run rated as passing.

P4: the fold needs no guard

The PM brief allowed a small guard in run-summary.ts if retried attempts inflated per-node runs misleadingly. They do not. A try node that failed twice before succeeding folds to runs: 3, failures: 2, status: 'failure' — every number true, and the same "worst outcome wins, runs/failures carry the nuance" rule a loop body has always folded under; a retry ladder is just another way for one node to run more than once. The node-level failure does not propagate to the run, so a recovered container still yields a completed run. Metrics get strictly more accurate: rows written by an attempt that then threw now reach selected/acted instead of vanishing. Documented in the fold's header and pinned by two tests, rather than guarded in code.

Container status was not touched — that was Option C's recovered idea, which the ruling did not adopt. A test pins it that way so it cannot drift.

Verification

  • pnpm --filter '@objectstack/service-automation^...' build — dependency closure built first in a fresh worktree.
  • pnpm --filter '@objectstack/service-automation' build — green (tsup DTS is this package's typecheck; it has no typecheck script).
  • pnpm --filter '@objectstack/service-automation' test75 files, 920 tests, all passing, including the 10 pre-existing try-catch-node.test.ts cases and nested-region-parity / nested-composition.
  • New pins: try-catch-failed-attempt-steps.test.ts, 9 cases — what failed, how many attempts (retryAttempt 0,1,2), which node threw, the whole partial attempt rather than only the throwing node, the retry-succeeds ladder, "a recovered container still reports success", no attempt index without a retry policy, a clean run staying clean, and the two run-summary fold assertions.
  • ESLint on all four changed source files — clean.

Gates, individually:

GateResult
node scripts/check-nul-bytes.mjsOK, 7095 files scanned, no raw control bytes
node scripts/check-platform-checklist.mjs1 problem, pre-existing and not minecoverage.json qa: UNCLASSIFIED (#7347). The edited item validates
node scripts/checklist-select.mjs --self-test17 cases pass
pnpm check:docs-audit-scope56 + 22 self-test cases pass; scope in sync
node scripts/check-changeset-fixed.mjsfixed group in sync, 69 packages

check:engine-double-contract is not implicated — no new fake or stub data engine; the test doubles here are flow node executors.

Reverse verification

Predicted before running: disabling the single seam (partialSteps?.push(...) in runRegion's catch arm) should turn 7 of the 9 new tests red and leave exactly two green — "a recovered container still reports success" and "a clean run stays clean", neither of which depends on a failed attempt being recorded — while all 10 pre-existing try-catch-node.test.ts cases stay green, since retry and throw semantics are untouched.

Measured: Tests 7 failed | 12 passed (19) — the 7 predicted failures, those exact 2 new tests green, and all 10 pre-existing cases green. Prediction matched exactly. Restored by inverse edit (no git stash; the marker line is absent and the push line is back, both confirmed by grep before committing).

The pre-existing suite staying green under the disable is the load-bearing half: it is what shows this change adds records without moving behaviour.

Notes

  • Changeset: @objectstack/service-automationminor — additive log records, no breaking change, so no ADR-0087 disposition line is required.
  • No packages/spec edits; no content/docs/releases/ edits.

Generated by Claude Code

… caught failure leaves a trace (#7546)
A caught `try_catch` failure used to record nothing about the failure. The run
log was `[start, container(success), catch-step]`: no step carried
`regionKind: 'try'`, none carried `status: 'failure'`, and the container's own
step read `success`, so a caught failure was indistinguishable from a clean run
that merely touched the catch path. The only evidence was the catch region's
side effects.
The steps were never structurally missing — a failing node pushes its own
`failure` step into the region's array before it throws, and the #1479
`childSteps` splice already carries region steps into the parent log. The failed
attempt's array was simply discarded as the region unwound.
`runRegion()` now hands a failed region's partial steps to the caller through an
opt-in sink before the throw propagates, tagged exactly as a successful region's
are; `try_catch` accumulates every failed attempt across the retry ladder and
folds them into `childSteps` ahead of the surviving region's steps. With a retry
policy declared they also carry `retryAttempt`, making the attempt count a value
in the log rather than an inference from elapsed time — a key the spec's
`ExecutionStepLogSchema` has declared all along with no producer until now.
Retry and throw semantics are unchanged, and a recovered container still reports
`success`; a distinct `recovered` status was considered in the decision and not
adopted. The run-summary fold needs no special case: a node that failed twice
then succeeded folds to `runs: 3, failures: 2`, all true, and rows written by an
abandoned attempt now reach `selected`/`acted` instead of vanishing.
Also rewrites the `flow-error-handling` checklist item (revision 2) — it asserted
the failing try step's presence, a contract the engine had never promised, which
is why QA run #7516 read a by-design silence as a FAIL. The ruling implemented
the clause rather than weakening it, so the item now pins the new contract, the
container's by-design `success`, and a retryAttempt count in place of the weak
duration-only retry oracle.
Ruling: #7546 (maintainer, 2026-08-11).
@vercel

vercelBot commented Aug 11, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 11, 2026 11:00am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-automation.

3 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/automation/flows.mdx(via @objectstack/service-automation)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/service-automation)
  • content/docs/plugins/packages.mdx(via @objectstack/service-automation)

2 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/service-automation)
  • content/docs/releases/v9.mdx(via @objectstack/service-automation)

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.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 11, 2026
@os-help
os-help marked this pull request as ready for review August 11, 2026 11:18
@os-help
os-help added this pull request to the merge queueAug 11, 2026
Merged via the queue into main with commit 8657957Aug 11, 2026
26 checks passed
@os-help
os-help deleted the claude/issue-7546-try-region-failed-steps branch August 11, 2026 11:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-help@claude