Harden IR and emitters against code injection from crafted pipelines - #4
Merged
Conversation
Node id / signal / impl references are interpolated verbatim into emitted source (def names, signal handlers, import targets) and into emitted filenames — so the IR now pins them to safe identifier shapes at the validation boundary, which every adapter inherits. Prose fields (descriptions) stay unrestricted but are escaped at splice sites: safe_docstring_line for Python docstrings, safe_block_comment_line for JSDoc, and resolve_within contains every emitted path inside the output directory as defense-in-depth. Originated from the correctness-review pass on PR #1 (the reviewer applied fixes to the working tree instead of reporting); ported onto the post-_ts_common/dbos-ts codebase and extended to the gaps it missed: JSON-schema description splices in dbos.py and all splice/path sites in the new dbos_ts.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VJdgd3mj2jk6dKCLUjdS48
trevhud
added a commit
that referenced
this pull request
Jul 30, 2026
Follow-up audit to the fan_out work. The recurring failure mode in this repo is a test that observes something *adjacent* to the behavior, so this measures the suite instead of trusting it: apply a semantic regression to an adapter, run the suite, and anything that still passes is an untested behavior. 18 mutations (retry policy, timeouts, step naming, payload threading, MCP allowlist narrowing, the ANTHROPIC_API_KEY scrub, invariants #4 and #7); 15 caught, 3 survived. **temporal: per-node `timeout:` was ignored and nothing noticed.** Making `_activity_timeout` return the default unconditionally left the whole suite green. A step declaring a 30s budget silently got 5m — five times too long on a wedged call, with nothing in the emitted code hinting why. **cloudflare: same gap on HITL gates**, and worse there, because `waitForEvent` THROWS on timeout: a gate inheriting the 7d default instead of its declared 1h turns a fast-fail approval window into a week-long hang, and the reverse fails a legitimately long wait. Both new tests carry a negative half — asserting only that the declared value appears would still pass if the adapter emitted it for every node, which is exactly how the original gap hid. Writing the cloudflare one immediately surfaced a real emitted-output bug: `1h` rendered as `"1 hours"`. Cloudflare's WorkflowDuration accepts either spelling so nothing broke, but emitted code is a reviewed artifact and "1 hours" reads as a defect. Now singularized. **The third survivor is an equivalent mutant, not a gap.** Swapping the tier order in `fan_out_element_param` cannot change the answer: fan_edge_sources ⊆ edge_sources, and a tier is accepted only when it singles out exactly one param, so if the broader tier singles one out it is necessarily the same one. Commented in place so the next sweep doesn't chase it. The precedence test added alongside is still real — verified by a mutation that IS observable (always take the alphabetically-first param), which it catches. **Also: the temporal e2e's fan was a no-op.** Its `exclusion_check_sequence` mock returned `passed: []`, so personalize_email never ran — the same vacuous-fixture bug just fixed in the three TS suites, missed because that suite passed and I only chased failures. Its `CAPTURED_PAYLOADS` dict is keyed by node and so keeps only the last call, structurally unable to observe a fan at all; an ordered CAPTURE_LOG now sits alongside it. Verified by mutating the snapshot the e2e actually imports back to batch dispatch: it fails with "expected 3 activity executions, got 1". That last point is worth knowing: the temporal e2e imports the committed `examples/.../runtimes/temporal/workflow.py` snapshot, not fresh emission, so mutating the adapter does not reach it. The snapshot is regenerated by the adapter tests' fixtures (which emit into the repo on purpose, so `git diff` shows emission changes at review). Audited two more patterns and found no open gaps: value-taking flags asserted only by presence (`--output-last-message`, `--append-system-prompt`) turn out to have their values checked elsewhere, and the remaining empty-collection fixtures are inert because nothing iterates them. CLAUDE.md gains a "Testing discipline" section with the five defects of this shape, the three habits that catch them, and the instruction to verify a new test by breaking the code rather than by inspection. 1162 fast + 27 slow pass; ruff, strict mypy, sanity-check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SBuSDL7Svm7iDzgmhTpDSW
trevhud
added a commit
that referenced
this pull request
Jul 30, 2026
* Dispatch fan_out nodes per element on all six runtimes A `fan_out` node means "invoke once per element of an upstream list". Only DBOS did that; python, temporal, cloudflare, dbos-ts and inngest passed the whole list in a single invocation. That was not a missing feature so much as a runtime leaking into node semantics. The same pipeline.yaml produced two incompatible contracts for the code a user fills in: BDR's `personalize_email` judge declares `contact` as one contact and its Zod/Pydantic schema says so, but five of six runtimes handed it the entire array. Fill in the stub against DBOS, re-target Temporal, and it breaks — which invariant #1 (the IR is runtime-agnostic) exists to prevent. Which input is the fanned list now resolves in the language-neutral `_common.fan_out_element_param`, not in a Python-only helper. Two adapters disagreeing about that would make one pipeline mean two different things, so it cannot live in a per-language module. Precedence is unchanged (fan_out edge marker > incoming-edge source > only node-bound param) and ambiguity is still an emit-time error, never a guess. Per runtime: DBOS enqueues a durable step per element (unchanged); python uses pool.map; temporal gathers one activity execution each; cloudflare and inngest Promise.all over per-element steps; dbos-ts allSettled + unwrap. All preserve input order. Two traps this surfaced, both documented in CLAUDE.md: - Cloudflare and Inngest key a durable step by NAME, so element steps are index-suffixed. A constant name makes Cloudflare serve element 0's cached result for all N — the run succeeds, the output has the right length, and every entry is identical. DBOS is exempt: it identifies a step by execution order. - A parkable (MCP-bound) fan_out node on Cloudflare runs its elements sequentially, same reason a parkable step leaves its parallel wave — waitForEvent inside a promise combinator is undocumented and its timeout throws, which would reject every sibling. Also collapses temporal's three dispatch shapes onto one `_execute_activity_expr`. The wave branch had previously been a separate copy that silently omitted retry_policy; one renderer means a field cannot be added to one shape and forgotten in the others. Emitted code routes the fanned list through a guard (`fanOutList` / `_fan_out_list`) naming the node and the IR reference. Found by running it: the first live fan_out run died with "Cannot read properties of undefined (reading 'map')" — no node, no reference, in generated code the user never wrote. Python's "'NoneType' object is not iterable" is no better, so both languages get it; diagnostics are part of the emitted contract and a guard on one runtime only is its own parity gap. Testing. tests/test_fan_out_parity.py asserts the contract across all six, and is built to fail against batch dispatch rather than merely pass against the new output: each check requires the element param to bind the loop variable AND the whole-list expression to be absent. Verified empirically — against origin/main the five broken runtimes fail every behavioral assertion while every dbos parametrization passes. The live e2e mocks were themselves masking this. Three TS suites had `exclusion_check_sequence` returning `passed: []`, so BDR's fan was a correct no-op that no assertion caught. Mock values now derive from the IR via `_helpers.fan_out_source_keys`, and the suites assert the fan ran once per element with each invocation receiving exactly one. Inngest's memoization check is now fan-aware (== FAN_OUT_ELEMENTS for fan_out nodes, == 1 otherwise), which is what catches a step-name collapse. 1159 fast + 27 slow pass; ruff, strict mypy, sanity-check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SBuSDL7Svm7iDzgmhTpDSW * Close three test gaps found by mutation-testing the adapters Follow-up audit to the fan_out work. The recurring failure mode in this repo is a test that observes something *adjacent* to the behavior, so this measures the suite instead of trusting it: apply a semantic regression to an adapter, run the suite, and anything that still passes is an untested behavior. 18 mutations (retry policy, timeouts, step naming, payload threading, MCP allowlist narrowing, the ANTHROPIC_API_KEY scrub, invariants #4 and #7); 15 caught, 3 survived. **temporal: per-node `timeout:` was ignored and nothing noticed.** Making `_activity_timeout` return the default unconditionally left the whole suite green. A step declaring a 30s budget silently got 5m — five times too long on a wedged call, with nothing in the emitted code hinting why. **cloudflare: same gap on HITL gates**, and worse there, because `waitForEvent` THROWS on timeout: a gate inheriting the 7d default instead of its declared 1h turns a fast-fail approval window into a week-long hang, and the reverse fails a legitimately long wait. Both new tests carry a negative half — asserting only that the declared value appears would still pass if the adapter emitted it for every node, which is exactly how the original gap hid. Writing the cloudflare one immediately surfaced a real emitted-output bug: `1h` rendered as `"1 hours"`. Cloudflare's WorkflowDuration accepts either spelling so nothing broke, but emitted code is a reviewed artifact and "1 hours" reads as a defect. Now singularized. **The third survivor is an equivalent mutant, not a gap.** Swapping the tier order in `fan_out_element_param` cannot change the answer: fan_edge_sources ⊆ edge_sources, and a tier is accepted only when it singles out exactly one param, so if the broader tier singles one out it is necessarily the same one. Commented in place so the next sweep doesn't chase it. The precedence test added alongside is still real — verified by a mutation that IS observable (always take the alphabetically-first param), which it catches. **Also: the temporal e2e's fan was a no-op.** Its `exclusion_check_sequence` mock returned `passed: []`, so personalize_email never ran — the same vacuous-fixture bug just fixed in the three TS suites, missed because that suite passed and I only chased failures. Its `CAPTURED_PAYLOADS` dict is keyed by node and so keeps only the last call, structurally unable to observe a fan at all; an ordered CAPTURE_LOG now sits alongside it. Verified by mutating the snapshot the e2e actually imports back to batch dispatch: it fails with "expected 3 activity executions, got 1". That last point is worth knowing: the temporal e2e imports the committed `examples/.../runtimes/temporal/workflow.py` snapshot, not fresh emission, so mutating the adapter does not reach it. The snapshot is regenerated by the adapter tests' fixtures (which emit into the repo on purpose, so `git diff` shows emission changes at review). Audited two more patterns and found no open gaps: value-taking flags asserted only by presence (`--output-last-message`, `--append-system-prompt`) turn out to have their values checked elsewhere, and the remaining empty-collection fixtures are inert because nothing iterates them. CLAUDE.md gains a "Testing discipline" section with the five defects of this shape, the three habits that catch them, and the instruction to verify a new test by breaking the code rather than by inspection. 1162 fast + 27 slow pass; ruff, strict mypy, sanity-check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SBuSDL7Svm7iDzgmhTpDSW --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A crafted pipeline.yaml could inject code at emit time: node ids/signals/impl refs are interpolated verbatim into emitted source and filenames, and description prose is spliced into docstrings/JSDoc where
"""or*/breaks out of the comment.safe_docstring_line/safe_block_comment_lineescape prose at every splice site (descriptions stay unrestricted prose)resolve_withincontains all emitted paths inside the output dirGate: 279 fast + 8 slow tests, mypy strict, ruff, sanity — green.
🤖 Generated with Claude Code
https://claude.ai/code/session_01VJdgd3mj2jk6dKCLUjdS48