Uh oh!
There was an error while loading. Please reload this page.
fix(service-automation): route every retry attempt through the node input-schema guard - #10024
Conversation
…nput-schema guard (#9889) executeWithoutRetry — the method retryExecution re-runs the flow through on every retry attempt — never called validateNodeInputSchemas, so a flow whose node config violates its own declared inputSchema under errorHandling.strategy: 'retry' was refused on attempt 1 and executed for real on attempts 2..N. Both attempt paths now call the same guard (the seedRunVariables chokepoint discipline from #9704); retry accounting is unchanged. Pins assert the side-effecting node never runs on any attempt, and that a valid flow still retries normally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
📓 Docs Drift CheckThis PR changes 1 package(s): ⛔ 2 release-owned page(s) name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin c48d37dd8bc1bf8f658c5465006c54508ad23b4b && git checkout c48d37dd8bc1bf8f658c5465006c54508ad23b4b
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 86d384bebfe17a56986d03c58d9ed5deab9b7713 194ab77dbd6a27fdfbb4951a4c3ddfc48ece02d3 && git checkout -B drift-repro 86d384bebfe17a56986d03c58d9ed5deab9b7713 && git merge --no-ff 194ab77dbd6a27fdfbb4951a4c3ddfc48ece02d3
node scripts/docs-audit/affected-docs.mjs --json 86d384bebfe17a56986d03c58d9ed5deab9b7713
|
Uh oh!
There was an error while loading. Please reload this page.
Fixes#9889
Defect
validateNodeInputSchemas— the guard that refuses to run a flow whose nodeconfigviolates its own declaredinputSchema— had exactly two sites inpackages/services/service-automation/src/engine.tsat the branch point (origin/main@1e050a5b1, re-measured on this branch): the call at line 3188 insideexecute(), and the definition at line 5261.executeWithoutRetry(line 6381) — the methodretryExecutionre-runs the flow through on every retry attempt — never called it.Because the retry handoff lives inside
execute()'s catch and the guard reports by throwing, a flow whose node config violates its declaredinputSchemaundererrorHandling.strategy: 'retry'was refused on attempt 1 (before any node executed) and then executed for real on attempts 2..N, with the config the guard rejected. A side-effecting node (a data write, an HTTP call, an email) behind a mis-declaredinputSchemawas reachable simply by declaringretry.Fix — the parity floor only
executeWithoutRetrynow calls the samevalidateNodeInputSchemasbefore dispatching the start node — the same chokepoint disciplineseedRunVariablescarries for the variable environment (#9704): one method holds the rules, both attempt paths call it. No second validation mechanism, no copy of the rules._variablesis deliberately unused), so re-running it per attempt cannot refuse anything attempt 1 would have allowed.executeWithoutRetry's generic failure arm, so each refused attempt still records afailedrun-log row and still consumes retry budget — retry accounting is unchanged. The stronger alternative (classifying definition-level failure as non-retryable soexecute()never hands off toretryExecutionat all) changes retry accounting and is deliberately NOT implemented; it is raised as an open question on the issue for a maintainer ruling.success: false+status: 'failed'+ the guard's message on both paths. It carries no ADR-0112code— pre-existing and deliberate per the automation: bothtriggerroutes still answer HTTP 200 wrapping an inner {success:false} — the same #3962 residue #8684 closed on resume #9378 classification (codes mark the never-dispatched exits;status: 'failed'marks dispatched-and-failed, which is the family a guard-refused attempt rides today). Whether a definition-level refusal deserves its own code is part of the same non-retryable question.Why this makes a sixth drift harder (this is instance five: #9378, #9415, #9414, #9510, #9704)
Honestly: adding a call in
executeWithoutRetryis a second call site, and a future edit could still delete it. What the PR changes about the class:validateNodeInputSchemas's doc now states the chokepoint contract explicitly — both attempt paths call it, and a repair to the validation rules belongs in the shared method, never inlined into a caller. Diverging on the rules now requires actively re-inlining a copy against a written prohibition at the exact place a rule-editor reads.input-schema-retry-parity.test.tsdrives a mis-declared flow understrategy: 'retry'with a spy executor and asserts the side-effecting node runs zero times across all attempts, plus onefailedrun-log row per attempt each carrying the guard's message. Removing theexecuteWithoutRetrycall flips the whole run to success (verified by ablation, below), so the deletion cannot pass the suite — a sixth drift on this guard now requires removing the call and deleting/weakening the pin in the same PR.execute()alone. That is the class-level residue the chokepoint pattern can only shrink one guard at a time; stated rather than claimed away.Tests
packages/services/service-automation/src/input-schema-retry-parity.test.ts, 3 pins:failedrows (1 initial +maxRetries: 2), each with the guard's message — pre-fix this run succeeded with the node executing for real;194ab77db:Test Files 83 passed (83),Tests 991 passed (991)—os-verify-lock: VERDICT command-exit 0.Ablation (mandatory lane clause)
executeWithoutRetryshould fail both negative pins at their first assertion —expect(result.success).toBe(false)receivingtrue— because attempt 2 runs unguarded and succeeds, i.e. the refusal evaporates entirely; the valid-flow pin stays green.Tests 2 failed | 1 passed (3), both failuresAssertionError: expected true to be falseon theresult.successassertion; valid-flow pin green.os-verify-lock: VERDICT command-exit 1.git hash-objectonengine.ts=35d3e3f51e66626bf7adcdc7bee995e91843b801both pre-ablation (at commit194ab77db) and post-restore; pins re-run green from the committed state (Tests 3 passed (3))../engine.jsrelative from source and vitest transforms the TS source directly (no package-exports/dist resolution, no alias involved), so the mutation and its restoration were both observable immediately;dist/was never in the loop.Gates — union re-derived with
node scripts/pm/dispatch-gates.mjs(no paths passed), run at final commit194ab77dbDispatch-named, all green (each gate's own verdict line):
pnpm check:cross-package-test-inputs/node scripts/check-cross-package-test-inputs.mjs— "OK: 12 package(s) read outside themselves, all declared, and turbo.json hashes every declared glob."pnpm check:slot-lookup— "baseline key set verified against 1e050a5: no files added."pnpm check:test-source-alias— "check-test-source-alias OK — 72 packages with tests scanned; …"pnpm check:type-source-resolution— "check-type-source-resolution OK — 76 packages with a tsconfig.json scanned; …"node scripts/docs-audit/check-affected-docs.mjs— exit 0, "✓ affected-docs self-test: 262 cases pass." (unreachable-rows table is its standing informational report)Added by re-derivation (changeset paths + new-test-file convention), all green:
pnpm check:changeset-gate-self-tests— "✓ check-changeset-no-major --self-test: 116 assertions …"pnpm check:objectui-changeset— "✓ objectui-range --self-test: all checks passed"node scripts/check-adr-0087-registration.mjs— "✓ … this PR adds no declared-breaking changeset (1 non-breaking changeset(s) seen)."node scripts/check-changeset-no-major.mjs— "✓ This diff introduces nomajorbump."node scripts/check-empty-changeset.mjs— "✓ No empty-frontmatter changeset introduced by this diff (1 declaring changeset(s) added)."pnpm check:query-options-erasure— "baseline key set verified against 1e050a5: no files added."pnpm check:engine-double-contract— "check-engine-double-contract: OK — 321 pinned, 133 in the DEBT ledger, 2 exempt."pnpm check:where-matcher— "baseline key set verified against 1e050a5: no files added."pnpm check:type-check-coverage— "check-type-check-coverage: OK — 64/77 workspace packages type-checked …"pnpm check:type-check-debt(after building the packages closure, VERDICT command-exit 0) — "check-type-check-coverage --re-measure: OK — 33 ledger entr(ies) re-measured in 348.3s, 1925 raw tsc error(s) total, none above its recorded number."pnpm check:nul-bytes— "check-nul-bytes: OK (scanned 6348 text file(s) … no raw ASCII control bytes)."The package declares no
typecheckscript; its type coverage rides the shared coverage/debt gates above, which re-measured green with the new test file in the tree.Changeset
.changeset/automation-input-schema-retry-parity.md— patch on@objectstack/service-automation, stating the new refusal plainly: flows that were running on retry with a mis-declaredinputSchemaare now refused on every attempt, and how an affected author repairs their flow.Generated by Claude Code