[alfredolopez80]

Summary

Two parts.

Design record (docs/design/workflow-step-change-policy.md, status Proposed):

• the principle that a step field exists only together with the code that consumes it, and the three layers that enforce it (layer 3 names both spec parsers: the built-in coordinator's ConfigLoader with parseRuntimeWorkflowSpec, and sdk.config.getSpec() for custom images);
• the flag policy for step execution changes and the proposed flag table;
• step tool-result spillover keeps results in mcp-host process memory, one store per step execution, released when the step ends (feat(mcp-host): spill oversized tool results of workflow steps out of the prompt #1454). Nothing is written to /output; the read tool only sees its own step execution's store;
• open decision 8: reducing status.steps[].toolsCalled[].args and .result to names, sizes and an artifact reference, after moving their current readers. It stays open until feat(mcp-host): spill oversized tool results of workflow steps out of the prompt #1454 lands.

Code (requested by the owners while answering review round 1):

• WRC sizes a managed output PVC from spec.output.storageSize, else from the new WRC env CLERUM_WORKFLOW_OUTPUT_STORAGE_SIZE, else 1Gi. The env accepts ^[1-9][0-9]*(Mi|Gi)$; any other value stops WRC at startup. deploy/base sets it to 1Gi.
• Changed default: a recipe without spec.output.storageSize gets a 1Gi PVC (was 256Mi). The size applies only when WRC creates the PVC; WRC never resizes an existing claim.
• CLERUM_WORKFLOW_OUTPUT_QUOTA_MB is added to MCP_HOST_ENV_PASSTHROUGH, so the artifact quota set on WRC reaches recipe mcp-host pods.
• The Control UI recipe templates no longer pin storageSize, so new recipes use the operator default.
• CRD storageSize description and docs/crds/workflowrecipe.md describe the precedence; the upgrade note is in docs/deploy/releases-and-upgrades.md.

Rollout

• The CRD change is description text only, but Helm does not update crds/: apply charts/clerum-crds/crds/workflowrecipe.yaml on each cluster.
• Environments outside deploy/base that set their own WRC env get the 1Gi built-in default until they set CLERUM_WORKFLOW_OUTPUT_STORAGE_SIZE. Not verified: the infra repository wiring and crd-guard.sh.

Testing

Head 372ce8363, Node 24.22.1.

• workflow-recipes: tsc -b EXIT=0; npm test EXIT=0, 154 files, 3371 passed, 29 skipped
• control-ui: tsc EXIT=0; tests EXIT=0, 261 files, 3106 passed
• Vacuity mutations, 9 of 9 killed: env ignored, validation removed, default changed, wiring hardcoded, reconciler literal default, spec ignored, passthrough entry removed, template storageSize restored, local templates off
• Review-round-1 tests run against this head: the only change in result is the frozen passthrough list, which now includes the quota variable (one control-ui OAuth test failed once under CPU contention and passed 16/16 four times in isolation)
• bash scripts/minikube/t0.sh: T0_EXIT=0, affected checks PASS (workflow-recipes 3371 passed, control-ui 3106 passed)
• bash scripts/ci/check-no-infra-identifiers.sh
• Prettier on the changed source files
• T2 not run (owner decision: no cluster lane for this PR)

Checklist

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

---

🤖 Generated with Claude Code

[zach88]

Went through this one and checked every path:line against 61f32ae. The record is accurate: every reference points where it says and every factual claim holds. The gaps I found are about what the spillover section leaves unsaid, mostly where persisted results live and how the read tool is scoped. They matter because people will implement from this, but none of them block a docs PR.

🟡 Approve with nits

• Must resolve before approval: 0
• Optional / non-blocking: 6 (3 Medium, 3 Low)

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

Optional (non-blocking)

These can merge as is. Treat them as follow-ups or as your call, though I'd fold the first two into the record before anyone builds spillover from it.

1. [R1-M1] Step spillover doesn't say where persisted results are stored (docs/design/workflow-step-change-policy.md:92-100, principle 5 at :45-46)

   • Spillover is on by default, but principle 5 only covers "reports and logs". Persisted tool results are payloads too.
   • In the recipe mcp-host pod the only writable volumes are /tmp (an emptyDir) and, when mounted, the recipe-output PVC at /output (workflow-recipes/src/workflow/podFactory.ts:834-840, :882-892).
   • That PVC is also mounted by the artifact-reader, which serves downloads (podFactory.ts:1060-1066), and by the snippet-runner, which runs user code (:1220-1227). An implementer looking for "the persistent dir" could pick /output and turn raw tool results into downloadable artifacts.
   • Agent spillover writes under ${workspacePath}/spillover/ (mcp-host/src/core/spillover/storage.ts:8), and recipe pods don't set a workspace path, so there's no safe default to inherit.
   • Suggestion: state it as a constraint:
     • pod-local, non-shared storage only, never /output;
     • persisted content falls under principle 5;
     • read-back goes through the sanitizer, like mcp-host/src/core/tools/spilloverRead.ts:56-60.

2. [R1-M2] "Limited to results of the same step" has no mechanism behind it (:97-98, open decision fix(control-api): repair the imagePolicy base↔overlay boundary test #2 at :113)

   • The agent's clerum__spillover_read loads whatever ref the model passes: storage.load(ref) at mcp-host/src/core/tools/spilloverRead.ts:89, with the task id parsed from the ref itself.
   • The recipe mcp-host is one long-lived pod per recipe serving many runs (podFactory.ts:654-656). If that tool gets reused, a prompt injection in one step's tool output can ask for refs from another run.
   • Decision fix(control-api): repair the imagePolicy base↔overlay boundary test #2 would also let the tool skip allowedTools.include, which today is the step's whole toolset (mcp-host/src/workflow/workflowService.ts:904-910, :1128-1129). Scoping would then be the only boundary left.
   • Suggestion: write in that access is keyed by server-side execution context (run, step, attempt), never by a model-supplied ref. Any ref outside the step's own set is rejected, and the agent tool isn't reused as is. Accept fix(control-api): repair the imagePolicy base↔overlay boundary test #2 only under that condition.

3. [R1-M3] Principle 5 vs what status carries today, and vs principle 3 (:45-46 vs :40-42)

   • Today the step status does carry content:
     • workflow-recipes/src/workflow/sdkRuntime.ts:300-307 reports output and toolsCalled;
     • ToolCallRecord has args and result (mcp-host/src/workflow/types.ts:71-77);
     • workflow-recipes/src/workflow/restEndpoints.ts:403-457 only truncates them to a preview before they land in status.steps[].toolsCalled[].
   • A core engine built for parity would have to keep emitting them, and the record doesn't say which principle wins.
   • Suggestion: add a line that acknowledges the current state and carves it out of parity, or add it to the open decisions in section 5.

4. [R1-L1] Spillover cleanup and limits have no backstop (:89, :99)

   • "Deleted when the step ends" doesn't cover crash, abort or timeout. The pod is long-lived, and an emptyDir survives container restarts.
   • /tmp has no sizeLimit (podFactory.ts:885), so a 16 MiB per-step budget times concurrent steps has no ceiling.
   • The agent path has TTL, boot sweep and GC (mcp-host/src/config.ts:1127-1135).
   • Suggestion: delete in a finally on every terminal path, plus a boot sweep, a TTL and a per-pod cap.

5. [R1-L2] Layer 3 and the contract test assume a single coordinator parser (:20, :58-59, :61-63)

   • spec.coordinatorImage allows custom images built on packages/workflow-sdk, which has its own config loader and never goes through parseRuntimeStepSpec (workflow-recipes/src/workflow/workflowReconciler.ts:1771-1777).
   • Suggestion: scope the rule to the built-in coordinator and say what applies to custom images.

6. [R1-L3] The snippet-runner worker filters env a second time (:70-73)

   • workflow-recipes/src/workflow/snippetRuntime.ts:6-9 and :80-89 filter the worker env again, so a var read by snippet code needs to be on two lists.
   • It doesn't affect the four proposed flags, which mcp-host reads. Half a sentence would cover it.

What checked out clean

• All 9 path:line refs match 61f32ae exactly:
  • the projection, the parser, and assertAllowedKeys inside run;
  • the {stepId, provider, model} injection payload;
  • the preflight call and the chart README lines;
  • the three passthrough allowlists;
  • the 8 KiB agent threshold and the separate step loop.
• "The step loop does not compact context" holds: results get appended in full at workflowService.ts:1310-1318.
• None of the four proposed flag names exist at 61f32ae, and CLERUM_TOOL_SPILLOVER_ENABLED does.
• spec.steps has no preserve-unknown-fields, so the pruning claim for older CRDs is right.
• Internal consistency is good (4 hops, 3 layers, principles 1-5, section 5), and "no new CRD fields" holds.
• The README table is aligned and the relative links resolve.
• check-no-infra-identifiers.sh is clean, with no infra names or secrets.
• The per-pod passthrough model is kept, and shipping the allowlist entry before the behavior means the kill switch actually works with default-on.

[alfredolopez80]

Thanks for the review. All six points are addressed in the design record; the PR also picks up three small code changes the owners asked for while answering R1-M1.

• R1-M1 (where persisted results live). Step tool-result spillover keeps results in mcp-host process memory, one store per step execution, released in a finally when the step ends (feat(mcp-host): spill oversized tool results of workflow steps out of the prompt #1454). Nothing is written to /output, so it does not touch the run PVC or the artifact quota, and it is not visible or downloadable. The bound is the per-step-execution budget; concurrent step executions each have their own budget, inside the container memory limit.
• R1-M2 (read-tool scoping). The read tool is created per step execution and only sees that execution's store, so a reference from another step or run resolves to nothing. The record now states this.
• R1-M3 (principle 5 vs current status). Principle 5 now applies to new status content; the existing bounded previews are named as the exception. Reducing status.steps[].toolsCalled[].args and .result to names, sizes and an artifact reference is recorded as open decision 8, including moving their current readers first.
• R1-L1 (cleanup and limits). Cleanup is the release at the end of the step execution; the limits are the threshold and the per-step-execution budget. No TTL is needed because nothing outlives the step.
• R1-L2 (custom coordinator images). Layer 3 now names both parsers: the built-in coordinator's ConfigLoader with parseRuntimeWorkflowSpec, and sdk.config.getSpec() for custom images.
• R1-L3 (snippet worker env). The record now states the snippet worker env filter.

On sanitizing: no sanitizer runs on step tool results today, inline or read back; only the <tool-result> delimiters wrap them. Spillover does not change that exposure: a result that stays inline and one read back from the store reach the model the same way.

Code in this PR:

• A WRC-managed output PVC for a recipe without spec.output.storageSize now requests CLERUM_WORKFLOW_OUTPUT_STORAGE_SIZE from the WRC Deployment, or 1Gi (was 256Mi). An invalid value stops WRC at startup. Existing claims are never resized.
• CLERUM_WORKFLOW_OUTPUT_QUOTA_MB is now forwarded to recipe mcp-host pods.
• The Control UI recipe templates no longer pin storageSize.
• The CRD storageSize description changes (text only), so each cluster needs the CRD applied; the upgrade note is in docs/deploy/releases-and-upgrades.md.