feat(workflow): support --input on workflow resume (swamp-club#467)

[stack72]

Summary

Adds --input/--input-file/--stdin support to swamp workflow resume (follow-up to swamp-club#369, manual_approval gates). Operators can supply or override input values that change between the suspend and resume phases — e.g. a freshly minted auth key issued during a verification gate.

Behaviour

• Inputs are persisted when a run suspends (not at run start), so post-gate steps can resolve inputs.* on resume. Runs that never suspend write no input values to disk.
• Resume inputs deep-merge over the persisted inputs: existing keys are preserved, and a resume --input wins on a key collision. The merge base is the full original input set, so a plain swamp workflow resume needs no inputs re-passed — you only type the deltas.
• Audit: the run record records the key names of resume-time inputs, never their values (so secrets aren't written to the persisted run).
• Evaluation stays strict — no change to how any workflow evaluates. A workflow must declare the inputs it references at run time; the pattern is declare-at-run, supply/override-at-resume. (Resolves the issue's two open questions: override-wins merge semantics + key-name audit.)

Mechanics

• WorkflowRun aggregate gains optional inputs and resumeInputs fields (backward compatible — legacy run records round-trip unchanged).
• deepMerge relocated from src/cli/input_parser.ts to a domain module (src/domain/inputs/input_merge.ts), re-exported so existing callers (workflow run, model method run) are unaffected.
• resume() merges inputs onto the expression context and records resume key names; the resume CLI command parses inputs and rejects multi-item stdin (resume targets a single run).

Notes for reviewers

• The only new on-disk behaviour: original input values are written to the run record when a run suspends. If a secret is passed as a raw --input (rather than a vault expression), that value lands in the suspended run YAML. Bounded to suspended runs; the audit field is key-names-only; vault expressions sidestep it.

Test Plan

• Unit: input_merge (merge/override/additive/nesting), WorkflowRun inputs + resumeInputs round-trip incl. legacy records lacking the fields.
• Service: execution_service suspend → approve → resume merge (override wins, original preserved); no inputs persisted for non-suspended runs.
• CLI integration: workflow resume --input end-to-end (resolved through CEL), multi-item stdin guard.
• Full suite: 6490 passed, 0 failed. deno check/lint/fmt clean.
• Live: verified on a compiled binary — declare→suspend→resume-override merges correctly; resume with no --input restores the original inputs; an undeclared referenced input fails fast at evaluation (strict, unchanged).
• UAT: systeminit/swamp-uat workflow suite 12/12 against the compiled binary; full uat:cli serial shows no regressions (the only failures reproduce identically on the unmodified main binary).
• Follow-ups filed: swamp-uat#242 (UAT resume coverage) and a swamp-club docs request for content/manual/reference/workflows.md.

🤖 Generated with Claude Code

[github-actions]

Adversarial Review

Critical / High

No critical or high severity findings.

Medium

1. Prototype pollution in deepMerge — src/domain/inputs/input_merge.ts:30-43

   deepMerge iterates Object.entries(overrides) without filtering dangerous keys like __proto__, constructor, or prototype. If an attacker supplies --input '__proto__.polluted=true' via CLI (which uses setNestedValue with dot-notation), the merge itself won't pollute prototypes because { ...base } creates a fresh object. However, the concern is that the isPlainObject check at line 33 would return true for Object.prototype (the value at __proto__), potentially causing the recursive merge to walk into the shared prototype object when the base already has such a key from a prior context.

   In practice, this is mitigated because: (a) CLI inputs go through parseKeyValueInputs which uses setNestedValue to create nested objects (not __proto__ injection), (b) YAML/JSON parsed inputs would need explicit __proto__ keys, and (c) the spread operator on the result creates plain objects. The risk is theoretical but worth a guard if this function is ever used on less-trusted data.

   Suggested fix: Add a key filter: if (key === "__proto__" || key === "constructor" || key === "prototype") continue;

2. Shallow copy of nested inputs in toData() — src/domain/workflows/workflow_run.ts:714-715

   data.inputs = { ...this._inputs } is a shallow copy. If inputs contain nested objects (which deepMerge explicitly supports and the round-trip test at line 1046 exercises with nested: { a: 1 }), the serialized data.inputs shares references to those nested objects with the live WorkflowRun. A downstream mutation of the serialized data would affect the in-memory run.

   In practice, the data goes directly to YAML serialization (which clones implicitly), so this is unlikely to cause real corruption. But it's inconsistent with the stated Readonly<Record<string, unknown>> return type of the inputs getter that signals immutability.

   Suggested fix: Use structuredClone(this._inputs) or a recursive copy, matching the pattern used elsewhere for data artifacts.

Low

1. stepExprContext?.inputs may be undefined at suspend — src/domain/workflows/execution_service.ts:2259

   run.suspend(stepExprContext?.inputs) passes undefined when stepExprContext is nullish or when expressionContext.inputs was never set (e.g., a workflow run with no --input flags). The suspend() method guards against this (if (inputs) { this._inputs = inputs; }), so _inputs stays {}. On resume, deepMerge({}, resumeInputs) produces just the resume inputs — which is the correct behavior. No bug here, but the flow relies on the undefined-means-empty convention across three call sites.

2. recordResumeInputs uses Array.includes for dedup — src/domain/workflows/workflow_run.ts:663-667

   Linear scan on each key. For any realistic number of resume input keys this is negligible, but Set would be more idiomatic. Not a bug.

3. parseStdinContent does not guard against YAML alias bombs — src/cli/input_parser.ts:361

   @std/yaml's parse is called on untrusted stdin content. Most YAML parsers in Deno/Node limit alias expansion, but a billion-laughs YAML payload on stdin could theoretically consume memory. This is a pre-existing concern not introduced by this PR.

Verdict

PASS — The implementation is clean and well-tested. The input merging logic is correct: deepMerge produces the right result for scalar overrides, additive keys, nested object merging, and array replacement. The audit trail correctly records only key names. Backward compatibility with legacy run records is explicitly tested. The toData shallow-copy issue (Medium #2) is worth addressing in a follow-up but does not produce wrong behavior in the current call paths.

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. --input description verbosity (src/cli/commands/workflow_resume.ts:78-81): The description "Override/additional input for the resumed run (key=value or JSON); merged over the original run inputs" is notably longer than the equivalent on workflow run ("Input values (key=value or JSON)"). The semicolon-joined clause and Override/additional slash construction are a bit awkward. A tighter alternative: "Input override for the resumed run (key=value or JSON), merged over original inputs" — or just match workflow run's phrasing and let the example/help text carry the merge semantics.

2. --input-file description inconsistency (workflow_resume.ts:83): Resume says "Override inputs from a YAML file" while workflow run says "Input values from YAML file". The leading word differs. Minor, but if these flags should feel like the same option in different contexts, matching the phrasing helps.

Verdict

PASS — New flags mirror the workflow run pattern exactly, error messages are clear and actionable, the override/addition warnings are security-conscious (key names only, no values), and JSON-mode behavior is consistent with existing commands.

[github-actions]

Code Review

Clean, well-designed feature. The architecture follows DDD principles correctly, test coverage is thorough, and the security-conscious decision to audit only key names (never values) is sound.

Blocking Issues

None.

Suggestions

1. Prototype pollution guard in deepMerge (src/domain/inputs/input_merge.ts): The function doesn't filter __proto__ or constructor keys. In practice this is safe here — V8's JSON.parse creates own properties (not prototype links), and the spread { ...base } produces plain objects — but adding a hasOwnProperty guard or key blocklist would be a defensive improvement for a function that merges untrusted user input recursively.

2. Shallow copy in suspend(inputs) (src/domain/workflows/workflow_run.ts:635-639): The method assigns the reference directly (this._inputs = inputs). A defensive copy (this._inputs = { ...inputs }) would prevent the caller from accidentally mutating the run's captured inputs. The current call site (execution_service.ts:2259, passing stepExprContext?.inputs) is safe in practice since the context is scoped per execution, but future callers might not be.

3. Minor: redundant "backward compatibility" comment (src/cli/input_parser.ts:27): The comment // Re-export deepMerge from domain layer for backward compatibility implies this re-export preserves an existing API surface, but deepMerge was only just moved in this PR. The comment on coerceInputTypes (line 25) makes sense since that was moved earlier; for deepMerge the comment could just say // Re-export deepMerge from domain layer since there are no external consumers to be backward-compatible with yet.

Overall: solid DDD layering (domain function used by both domain service and CLI layer via re-export), comprehensive test pyramid (unit → service → integration), correct libswamp import boundary compliance, and clean backward compatibility for legacy run records.