feat: declared workflow inputs - #27
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe PR adds typed, declared workflow inputs with validation, defaults, expression access, separate trigger payloads, sub-workflow forwarding, graph operations, and end-to-end coverage. Existing bare JSON run inputs remain compatible. ChangesWorkflow input execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Engine
participant InputResolver
participant RunState
Caller->>Engine: submit RunInput or bare trigger
Engine->>InputResolver: validate and resolve declared inputs
InputResolver-->>Engine: resolved input map
Engine->>RunState: store trigger and run.inputs
RunState-->>Caller: execute workflow
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
A WorkflowGraph now carries `inputs`: named, typed, optionally-defaulted parameters that form the workflow's public signature, independent of the trigger kind. `resolve_inputs` validates supplied values against the declarations, and `validate_all` rejects declarations an expression could not address or that contradict themselves.
Entry points now take `impl Into<RunInput>` — a struct carrying the trigger payload alongside values for the workflow's declared inputs. `From<Value>` keeps every existing caller compiling unchanged. `build_and_run` resolves the values against the graph's declarations before minting a run id, notifying the observer, or building the graph, so an input error means provably nothing ran. Resolved values are seeded at `run.inputs` and lifted to the `inputs` expression scope, so config reads `=inputs.<name>`. A `sub_workflow` node forwards values to its child via a new `inputs` config object, each field resolved against the parent's scope.
Adds GraphOp::SetWorkflowInputs (whole-list replace), documents the child `inputs` map on the sub_workflow contract, and points the trigger contract at the graph-level declarations so an authoring agent does not invent a trigger-config form schema. Collapses the transform node's hand-rolled expression scope onto a single build_expr_scope constructor. That drift is what made `=inputs.<name>` resolve to null in transform while working everywhere else — a node that builds its own scope silently loses keys, and the binding fails quietly rather than erroring. tests/inputs_e2e.rs covers the chain end to end: supplied values reaching node config, defaults, a rejected call running nothing, the bare-Value call shape still working, and a parent forwarding inputs to a sub_workflow child.
`inputs` is the one scope key that collides with a jq builtin — jq's own `inputs` reads further program inputs. `=inputs.repo` works because a simple dotted path is walked directly, but anything jq compiles (a concatenation, a conditional, a pipe) binds the builtin instead and silently yields nothing. Found by dry-running the new shipped example, which reported an empty agent prompt rather than an error. Documented at the scope, in the wiki, and pinned by a test that asserts both forms — including the failing one, so the warning cannot go stale without the test noticing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/inputs_e2e.rs (1)
283-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign this error assertion with the structured checks used elsewhere in this file.
This test checks
err.to_string().contains("repo"). The sibling negative-path tests (a_missing_required_input_runs_nothing,a_wrongly_typed_input_is_rejected_before_the_run,an_undeclared_input_is_rejected_rather_than_silently_dropped) all match onEngineError::Inputand assert.code()/.input_name()instead. A substring check on theDisplayoutput is weaker: it survives wording changes but can also mask the wrong root cause if an unrelated string containing "repo" appears in the error.Use the same structured pattern here for consistency and precision.
♻️ Proposed alignment with sibling tests
- let err = run(&compiled, json!({}), &caps) - .await - .expect_err("the child's requirement must be enforced across the boundary"); - assert!( - err.to_string().contains("repo"), - "the error should name the missing child input, got: {err}" - ); + let err = run(&compiled, json!({}), &caps) + .await + .expect_err("the child's requirement must be enforced across the boundary"); + match err { + EngineError::Input(inner) => { + assert_eq!(inner.input_name(), "repo"); + } + other => panic!("expected an input error naming `repo`, got: {other:?}"), + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/inputs_e2e.rs` around lines 283 - 316, Update a_parent_that_omits_a_required_child_input_fails to match the structured error assertions used by sibling negative-path tests: pattern-match the returned error as EngineError::Input and assert the expected error code and input_name() for "repo", rather than checking err.to_string(). Preserve the existing expectation that execution fails at the missing child input boundary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/nodes/integration/sub_workflow.rs`:
- Around line 97-99: Update the documentation comment near child_inputs to state
that only an absent inputs field yields an empty map; remove the claim that
non-object inputs are treated as empty, since child_inputs returns
EngineError::Capability for that case.
---
Nitpick comments:
In `@tests/inputs_e2e.rs`:
- Around line 283-316: Update a_parent_that_omits_a_required_child_input_fails
to match the structured error assertions used by sibling negative-path tests:
pattern-match the returned error as EngineError::Input and assert the expected
error code and input_name() for "repo", rather than checking err.to_string().
Preserve the existing expectation that execution fails at the missing child
input boundary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bfc5021-f0ad-4b28-b4c9-4571f6f8f469
📒 Files selected for processing (13)
src/catalog.rssrc/engine.rssrc/error.rssrc/graph_ops.rssrc/model/inputs.rssrc/model/mod.rssrc/nodes/control_flow/transform.rssrc/nodes/integration/sub_workflow.rssrc/nodes/mod.rssrc/validate.rstests/inputs_e2e.rswiki/Architecture.mdwiki/Node-Catalog.md
| /// An absent or non-object `inputs` config yields an empty map, so a | ||
| /// `sub_workflow` node authored before inputs existed keeps working against a | ||
| /// child that declares none. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the non-object inputs behavior in the documentation.
child_inputs returns EngineError::Capability when inputs is present and is not an object. Lines 97-99 state that a non-object value yields an empty map. State that only an absent inputs field yields an empty map.
Proposed fix
-/// An absent or non-object `inputs` config yields an empty map, so a
-/// `sub_workflow` node authored before inputs existed keeps working against a
-/// child that declares none.
+/// An absent `inputs` config yields an empty map, so a `sub_workflow` node
+/// authored before inputs existed keeps working against a child that declares
+/// none. A present non-object `inputs` config returns an error.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// An absent or non-object `inputs` config yields an empty map, so a | |
| /// `sub_workflow` node authored before inputs existed keeps working against a | |
| /// child that declares none. | |
| /// An absent `inputs` config yields an empty map, so a `sub_workflow` node | |
| /// authored before inputs existed keeps working against a child that declares | |
| /// none. A present non-object `inputs` config returns an error. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/nodes/integration/sub_workflow.rs` around lines 97 - 99, Update the
documentation comment near child_inputs to state that only an absent inputs
field yields an empty map; remove the claim that non-object inputs are treated
as empty, since child_inputs returns EngineError::Capability for that case.
Resolving the child `inputs` map inside `run_child` rather than at the call site means a `per_item` fan-out resolves each child's values against its own element, the same scope `workflow_id` already used. Resolving once outside would hand every child the first element's values.
e475a89 to
548dbe3
Compare
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 548dbe389e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[serde(default)] | ||
| pub inputs: Vec<WorkflowInput>, |
There was a problem hiding this comment.
When a graph containing this new field is loaded by an older schema-v1 runtime—such as during a rolling deployment—Serde silently ignores inputs, while the node expressions referencing inputs.<name> remain valid strings. The old engine can therefore execute the workflow without validating the required parameters and resolve those bindings to null, potentially issuing malformed external calls instead of rejecting the graph. Persist input-bearing graphs under a new schema version (with a v1→v2 migration that defaults this list to empty) so older runtimes reject them as too new.
Useful? React with 👍 / 👎.
1 participant
What
A
WorkflowGraphcan now declare typed parameters — its public signature — instead of being runnable only with an undocumented free-form payload.{ "name": "review-a-repo", "inputs": [ { "name": "repo", "type": "string", "required": true, "description": "Repo to review" }, { "name": "depth", "type": "number", "default": 3 } ], "nodes": [ ... ] }Node config reads them as
=inputs.repo. Declarations are trigger-kind agnostic, so a manual run, a scheduled one, and asub_workflowcall all expose the same parameters.Why here rather than in trigger config
TriggerKind::Formexists as an unimplemented discriminator, and the obvious move is to hang a field schema off it. Top-level wins because asub_workflowcaller can then see and satisfy its child's signature, and because a workflow's parameters do not change when an author switches it from manual to scheduled.Design notes worth reviewing
run.triggerstays whatever fired the run (a webhook body, a chat message);inputsis the declared, validated set. Conflating them would make a webhook field able to silently satisfy a declared parameter.RunInputinstead of a tenth positional argument. Every public entry point takesimpl Into<RunInput>, andFrom<Value>means all 14 entry points and 21 integration test files compile unchanged.build_and_runresolves before minting a run id, notifying the observer, or building the graph — so anInputerror provably means nothing ran.transform's hand-rolled scope is gone. It built its own scope object and so silently lost the new key —=inputs.xresolved to null there while working everywhere else. Both paths now go through onebuild_expr_scope, which is what stops the next added key from having the same bug.inputscollides with a jq builtin.=inputs.repoworks (dotted paths are walked, not compiled), but inside a real jq program bareinputsbinds jq's owninputsand yields nothing. Documented at the scope and in the wiki, and pinned by a test that asserts the failing form too, so the warning cannot go stale silently. Found by dry-running the example, not by reading the code.Validation
cargo test·cargo test --features mock(548 passing) ·cargo clippy --all-targets·cargo fmt --checkNew:
tests/inputs_e2e.rscovers a value reaching node config, defaults, a rejected call running nothing, the bare-Valuecall shape still working,sub_workflowforwarding, and the jq addressing rule.Summary by CodeRabbit
New Features
Documentation