[alfredolopez80]

Summary

• The coordinator waited for mcp-host before agentic steps but dispatched the first snippet
  step without checking the snippet runner. A runner that was still starting turned into a
  failed attempt, which recipe authors worked around by raising maxRetries. The coordinator
  now polls the runner's existing GET /health once per run, after reading the persisted run
  status and before reporting the run as running, whenever a step that has not completed yet
  uses the runner. If the runner never becomes ready the run is reported as recovering, the
  same outcome as the mcp-host path.
• The runner wait uses the same defaults as the mcp-host wait (120 probes, 2 s apart) but is
  not identical to it: each runner probe has a 5 s timeout, while the mcp-host probe has none;
  the runner probe sends no credentials, while the mcp-host probe is authenticated; and the two
  waits run one after the other, mcp-host first, so a workflow with both kinds of step can wait
  for both. Each runner probe aborts its request once the status is read, so an unread or
  stalled /health body does not keep the connection open.
• On a resumed run the runner wait is skipped when no remaining step uses the runner (every
  step is already completed, or only agentic steps are left). The mcp-host wait is unchanged
  and still runs before the status is read, so an unready mcp-host still blocks a fully
  completed resumed run.
• Latency when the runner never starts: before this change the first snippet step failed after
  its own retries. Now one coordinator waits up to 838 s (120 probes × 5 s timeout + 119 pauses
  × 2 s) when every probe hangs, or about 238 s when connections are refused at once, then
  exits as recovering. Crash recovery replaces the coordinator three times, so the run
  reaches failed ("Pod failed after 3 recovery attempts") after four waits: up to about
  56 minutes (4 × 838 s), about 16 minutes with fast refusals, plus pod start time. This is
  intended; no total bound is added in this PR.
• WRC's pre-coordinator mcp-host readiness gate (hasMcpHostReadinessWaitExpired, 240 s) now
  judges the pod it actually reads against that pod's own window. Before, the window started
  at the run start, so a replacement pod created by crash recovery, or a pod created after the
  output anchor/prepare pods, could fail the run before it had its 240 s. Now:
  • present pod: the window starts at the later of the run start and the pod's creation;
  • pod being deleted (the predecessor of the pod the run waits for): the window starts at the
    later of the run start and its deletionTimestamp, so a predecessor that never goes away
    still ends the wait;
  • no pod with that name (GET 404): no verdict; the next pass creates the pod, which gets its
    own window.
    getPodReadiness returns this as a discriminated union (absent / present /
    terminating). A GET error other than 404 is handled as on dev: a transient error requeues
    the pass, any other fails the run.
• A pod that is being deleted is never reported ready. Kubernetes keeps a terminating pod's
  Ready condition until its containers stop, so before this change a terminating predecessor
  mcp-host passed the gate and the coordinator started against a pod that will not serve again.
  The same change stops the eager Plugin Workload SDK provisioner from sending /configure to an
  mcp-host deleted between its image-drift read and its readiness read; it returns deploying
  and the next pass handles the deletion. The output anchor and prepare waits do not read
  ready and are unchanged.
• patchStatus no longer ignores a failed live read of the recipe. It built the
  workflowExecution patch from the recipe the pass started with, so when crash recovery had
  already written attempt + 1 in the same pass, the older attempt was written back and the
  recovery did not count against MAX_ATTEMPTS (3). The read error now fails the pass: the
  watcher drops a recipe that is gone (404) and retries after any other error, writing no
  status for that pass.
• Accepted, not changed here: each mcp-host pod gets its own 240 s window, and the total wait is
  bounded by the stored attempt and the recipe's max_duration_seconds, not from the run start.
  A pod deleted by something other than WRC (preemption, a forced drain) is recreated without
  counting an attempt and gets a new window. Read-only check of clerum-dev and clerum over 30 days
  (13 and 2 coordinator runs): no recovery or readiness-deadline events. Tracked separately with
  the remaining attempt writers in the REST status endpoints.
• Deferred, not changed here: a cancel signal received during either readiness wait is not
  honoured, so the run ends recovering instead of cancelled. This PR adds a second wait with
  the same shape as the mcp-host wait. The gap is inherited and unreachable today. The workflow
  signal path has no producer since the wrc-trigger retirement (chore: dev → public sync refresh (dev@db43d12) — post-#820 convergence #112, f6abe0c), and
  waitForMcpHostReady has the same shape on base. Handling is deferred to a separate issue on
  the signal machinery.
• docs/crds/workflowrecipe.md: maxRetries is the total number of attempts (the code has
  always treated it that way), and retry n waits min(backoffSeconds × 2^(n-1), 300) seconds,
  for snippet and agentic steps alike. The backoffSeconds row now says that each wait is
  capped at 300 s. The CRD description is left for the next CRD release.
• MCP_HOST_ENV_PASSTHROUGH gains four reviewed, non-secret tuning variables that upcoming
  mcp-host changes read: CLERUM_WORKFLOW_TOOL_SPILLOVER_ENABLED,
  CLERUM_WORKFLOW_TOOL_SPILLOVER_THRESHOLD, CLERUM_WORKFLOW_TOOL_SPILLOVER_BUDGET_BYTES,
  CLERUM_WORKFLOW_STEP_ENGINE. mcp-host ignores them today, so this is a no-op until those
  changes land. The three spillover variables must be in place before the mcp-host spillover
  change merges: that change is on by default and this allowlist is the only way its operator
  kill switch reaches recipe pods. Kept as a separate commit.
  Operator note: these values are part of the mcp-host pod spec. Setting or changing any of the
  four variables on the WRC Deployment changes the runtime contract hash of eager Plugin
  Workload SDK mcp-host pods, so WRC rolls those pods on their next reconcile. Run-scoped
  workflow mcp-host pods are not rolled; they receive the value the next time WRC creates them.
  With the variables unset, the pod spec is identical to today's.

Testing

All results below are for head 8ce1cf4fd (includes a merge of dev at 34f14dd37).

• npm test passes for changed services (workflow-recipes: 153 files passed, 4 skipped;
  3350 tests passed, 29 skipped)
• npx tsc --noEmit clean
• scripts/minikube/t0.sh: affected package workflow-recipes, test and build/typecheck
  pass
• Snippet-runner wait: the new tests fail against 9a8988dba where expected (resumed runs,
  wait ordering, released /health connection) and each one fails when the behaviour it covers
  is removed: wait removed, wait gated on all spec steps, wait moved after running, default
  attempts 119/121, default delay 1000/3000 ms, probe timeout 4000/6000 ms, abort removed.
• A resumed run whose snippet step is persisted as running (a coordinator replaced
  mid-step) waits for the runner before the step runs again.
• healthCheck() runs against the real runner server (createSnippetRunnerServer):
  healthy on GET /health, unhealthy under a wrong base URL.
• Readiness gate: the tests drive fake pods through the real getPodReadiness, including a
  multi-pass graceful deletion that reaches "Pod failed after 3 recovery attempts" after four
  windows. Each fails when its rule is removed: absent pod measured from the run start,
  deadline anchored on the run start only, terminating branch removed or treated as present,
  terminating pod reported ready (7 tests), gate disabled (23 tests).
• patchStatus: a failed live read leaves the stored attempt at 2 and rejects the pass;
  swallowing the error again or ignoring the live read fails the tests.
• make minikube-t2 on a fresh profile (full-bootstrap): MINIKUBE_T2_PASS (T0, T1
  1064/1064 on real PostgreSQL, NP-08, port-forwards and the workflow-recipes health journey
  PASS; Playwright not run).
• Blind reviews of the readiness-gate and patchStatus commits (three rounds): every finding
  is fixed here or listed above as accepted or tracked separately.

Failures already present on dev:

• Root npm run format:check exits 1 on this head with 113 files, none of which this PR
  touches.

Checklist

• This is not a new third-party MCP server / WorkflowRecipe (those go to the registry)

---

Generated by Claude Code

[zach88]

Took a deep pass through this one. The core change is solid: the readiness wait sits exactly where the body says it does, the gating on pending steps matches what the dispatcher and the reconciler use, and every number in the description (838 s per wait, 3 recoveries, the backoff formula) checks out against the code. What's left is test quality around the /health contract plus a handful of small follow-ups.

🟡 Approve with nits

• Must resolve before approval: 0
• Optional / non-blocking: 6

This is my read from the review, not a formal GH approval.

Must resolve before approval

Nothing here.

Optional (non-blocking)

You can merge without any of these. They're follow-ups or your call.

1. [R1-M1] healthCheck() is never tested against the real runner server
   workflow-recipes/tests/unit/workflow/snippetRunnerClient.test.ts:112-251. Every healthCheck test runs against a stubbed fetch or a hand-written http.createServer, never against createSnippetRunnerServer (src/workflow/snippetRunnerServer.ts:104). If someone renames the route or changes the method on either side, the suite stays green while every snippet run waits out the full budget and ends in recovering. The contract is correct today. A ~20-line test that copies the setup in snippetRunnerServer.startup.test.ts (tmp token + output dir) works: I ran one, and the real client against the real server gave healthy, and gave unhealthy with a wrong base URL. Worth adding that one.

2. [R1-L1] The four new passthrough vars aren't tied to their reader
   workflow-recipes/src/workflow/podFactory.ts:83-86. Nothing reads them yet, which matches the body. One heads-up though: mcp-host currently reads CLERUM_TOOL_SPILLOVER_ENABLED / CLERUM_TOOL_SPILLOVER_THRESHOLD (mcp-host/src/config.ts:1127-1128), without the WORKFLOW_ part. If the upcoming spillover change keeps those names, the operator kill switch never reaches recipe pods, and nothing fails to tell you. A test in that mcp-host PR asserting that MCP_HOST_ENV_PASSTHROUGH contains the names it actually reads would close the gap.

3. [R1-L2] The abort test pins the implementation, not the outcome
   snippetRunnerClient.test.ts around L186 ("aborts the health request after reading the status..."). It asserts init.signal.aborted === true on a bodyless mock. An equally valid fix like response.body?.cancel() fails this test, while the real-socket test (the one that measures the connection actually being released) still passes. I'd keep only the real-socket test.

4. [R1-L3] The CRD description now contradicts the doc
   charts/clerum-crds/crds/workflowrecipe.yaml:586 still says "Maximum retry attempts for transient failures (ECONNRESET, timeout)", which is what kubectl explain shows. It's also inaccurate on the "transient" part: both retry loops retry any failure, except provider_output_length_exceeded on agentic steps. I saw you're deferring this to the next CRD release, which is fine. Just flagging it so the two texts don't drift for long.

5. [R1-L4] A workflow-only var still rolls eager SDK pods
   podFactory.ts:917 / pluginWorkloadSdkProvisioner.ts:420. The contract hash covers the whole pod spec, so kubectl set env CLERUM_WORKFLOW_STEP_ENGINE=... on WRC restarts eager Plugin Workload SDK pods that never use it. You already call this out in the operator note, and the existing vars behave the same way. Mentioning it so the decision is on record.

6. [R1-L5] Doc nits
   docs/crds/workflowrecipe.md:453. The new row is 3 chars wider than the header, so the raw table is misaligned (it renders fine). Also, the backoffSeconds row still reads "1–3600. Base delay", but anything above 300 is clamped to 300 starting at the first retry. The maxRetries row implies that; the backoffSeconds row could say it outright.

What checked out clean

• Order of operations matches the body: mcp-host wait, then status read, then runner wait, then running (sdkRuntime.ts:190/205/228/235).
• Resumed runs: a step persisted as failed, skipped or running still triggers the probe, and a run whose snippet steps are all completed skips it. Ran all three cases.
• snippetRunner! at sdkRuntime.ts:229 is safe. With a null client the run fails at :201 with the endpoint/token error before any probe.
• The /health contract holds:
  • The server only listens after it validates the token and permission model, so a 200 means the runner can actually execute.
  • It's the same endpoint the pod's readinessProbe uses.
  • Old runners are compatible.
  • The 8095 NetworkPolicy lane is created under the same condition that turns the wait on.
• healthCheck() never throws. 200 probes with unread 2 MB bodies on Node 24.22.1 produced no unhandled rejections.
• Security looks fine:
  • baseUrl is built from the recipe name and namespace with a literal /health, so there's no SSRF.
  • The probe carries no credentials, and spoofing readiness gains nothing because execute still requires the bearer.
  • The passthrough vars are operator-set and non-secret.
• Recovery path: recovering goes through 3 replacements and then failed, and each replacement recreates the runner pod too, so recovery can actually heal.
• Pod spec is unchanged with the vars unset, and the eager-pod hash claim is accurate.
• Tests: 13 mutations of the wait and client logic each turn at least one test red. Against 9a8988dba the 5 tests from the two fix commits fail, as the body says.
• tsc --noEmit is clean, and the touched suites are green.

[alfredolopez80]

@zach88 thanks for the review. Status of the six optional items at head 879d57916:

• R1-M1, fixed (ea092f51b). A new test runs SnippetRunnerClient.healthCheck() against createSnippetRunnerServer: healthy on GET /health, unhealthy under a wrong base URL, and the server records both requests. Changing the route or the method on the server fails this test; changing the client path fails it and two existing tests.
• R1-L1, belongs to feat(mcp-host): spill oversized tool results of workflow steps out of the prompt #1454. This PR keeps the CLERUM_WORKFLOW_TOOL_SPILLOVER_* names. The mcp-host spillover change has to read those names (today mcp-host/src/config.ts:1127-1128 reads CLERUM_TOOL_SPILLOVER_*) and should test that MCP_HOST_ENV_PASSTHROUGH contains them. Nothing changes here.
• R1-L2, fixed (ea092f51b). The mock test that asserted signal.aborted is removed. With controller.abort() deleted, the real-socket test still fails (3 of 3 runs); with response.body?.cancel() in its place, the suite stays green.
• R1-L3, deferred to the next CRD release, as the PR body says.
• R1-L4, no change. It stays recorded in the operator note.
• R1-L5, partly changed (879d57916). The backoffSeconds row now says each wait is capped at 300 s. The maxRetries row is 208 characters wide, the same as the header; the 3 extra are bytes (– and × are multi-byte), so that row is unchanged.

Also new since your review: a merge of dev that touches none of this PR's files, and 03204930b, a resume test with a snippet step persisted as running. make minikube-t2 passes on 879d57916: T0, T1 1064/1064 on real PostgreSQL, NP-08, port-forwards and the workflow-recipes health journey.

[alfredolopez80]

@zach88 @jozer-rami summary of what changed since my last update (head 879d57916). Head is now 8ce1cf4fd.

WRC pre-coordinator mcp-host readiness gate (workflowReconciler.ts, crashRecovery.ts). The 240 s gate measured every pod from the run start, so a replacement pod created by crash recovery, or one created after the output anchor/prepare pods, could fail the run before it had its own window. It now judges the pod it reads:

• ddb95a01a, 426ece2b3: present pod → window from the later of run start and pod creation; pod being deleted → from the later of run start and its deletionTimestamp; no pod (404) → no verdict, the next pass creates it. getPodReadiness returns a discriminated union (absent / present / terminating).
• 8ce1cf4fd: a pod with a deletionTimestamp is never ready. Kubernetes keeps Ready=True while a pod terminates, so a dying predecessor mcp-host used to pass the gate. The same change stops the eager Plugin Workload SDK provisioner from sending /configure to an mcp-host deleted between its drift read and its readiness read.

patchStatus (a7df48e4d). A failed live read of the recipe was swallowed and the workflowExecution patch was built from the pass-start object, which wrote an older attempt back after crash recovery had written attempt + 1 in the same pass. The read error now fails the pass; the watcher drops a recipe that is gone and retries after anything else.

Tests only (f880c9dac): readiness mocks return the kind the real producer returns.

Merges of dev: version lines only.

Validation at 8ce1cf4fd

• workflow-recipes: 3350 passed, 29 skipped; tsc --noEmit clean; T0 pass. Each new rule has a test that fails when the rule is removed.
• make minikube-t2 on a fresh profile: MINIKUBE_T2_PASS (T1 1064/1064 on real PostgreSQL, NP-08, port-forwards, health journey).
• Cluster E2E: e2e-snippet-runtime-smoke.sh pass; e2e-snippet-runtime.sh 86 passed, 0 failed, including the hybrid agentic case (mcp-host gate, snippet step, real LLM step). Two pre-existing test drifts had to be patched in a local copy to get there; they are tracked in workflow-recipes: follow-ups from PR #1453 (attempt writers, readiness wait bound, recovery, test witnesses) #1484.

Follow-ups, not in this PR: #1484 (REST status writers that can overwrite attempt, no total bound on the pre-coordinator wait, coordinator recovery next to a terminating mcp-host, vacuous test assertions, and the two E2E drifts).

The PR body is updated with the same detail.