fix(workflows): warn on unknown/misplaced keys in workflow YAML (#2213) - #2255
fix(workflows): warn on unknown/misplaced keys in workflow YAML (#2213)#2255kagura-agent wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWorkflow parsing now detects unknown and misplaced YAML keys, preserves warnings through discovery, and reports them as validation warnings. Schema key allowlists, parse-result types, discovery propagation, validation aggregation, loader tests, and workflow fixtures were updated. ChangesWorkflow parse warnings
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/workflows/src/loader.ts`:
- Around line 773-780: Update the unknown-key handling in the workflow loader’s
raw-key loop to detect keys recognized by KNOWN_DAG_NODE_KEYS and append a hint
that the key is valid on individual nodes, while preserving the existing generic
warning for other unknown keys. Add a regression test covering root-level
command, prompt, and bash keys.
In `@packages/workflows/src/workflow-discovery.ts`:
- Around line 329-334: Update mergeWarnings to clear any existing
allParseWarnings entry for every filename present in the incoming discovery
result, including filenames whose warnings collection is empty, before storing
the current warnings. Preserve the existing warning merge behavior while
ensuring a clean higher-priority workflow override removes stale lower-scope
warnings.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e898b93b-e681-4986-b9da-d91a6306b1d2
📒 Files selected for processing (7)
packages/cli/src/commands/validate.tspackages/workflows/src/loader.test.tspackages/workflows/src/loader.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/schemas/index.tspackages/workflows/src/schemas/workflow.tspackages/workflows/src/workflow-discovery.ts
|
Both findings were already addressed in commit 9cf2f22:
No additional changes needed. |
|
This is a real gap and the threading through Two things before merge. 1. Derive the key sets from the schemas instead of hand-listing them. I checked the current lists and they are exactly right today — The problem is what happens next. The moment someone adds a node field, every workflow using that field starts emitting "unknown key … will be ignored" — on a valid workflow, on every discovery, every run, every Both sets are derivable in one line each, and I confirmed both by execution:
2. The
The hint needs to point at Follow-ups — not blocking, and I'm not asking you to take them on unless you want to:
One thing to coordinate with yourself: if #2262 lands without removing |
9cf2f22 to
71dddc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/workflows/src/loader.ts (1)
420-422: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftPropagate warning detection into nested loop-group nodes.
This mapper only processes top-level
raw.nodes. Loop-group body nodes are parsed inside the schema and bypass the raw-key scan, so typos or misplaced keys in nested bodies remain silent. Add a recursive warning pass for nested raw nodes.🤖 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 `@packages/workflows/src/loader.ts` around lines 420 - 422, Extend the warning-detection flow around parseDagNode so it recursively scans raw nodes inside loop-group bodies, not only top-level raw.nodes. Ensure nested nodes use the same validationErrors and parseWarnings collection, while preserving the existing top-level parsing behavior.
🤖 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 `@packages/workflows/src/loader.ts`:
- Around line 133-137: Update the warning allowlists used by the loader’s
unknown-key handling, especially KNOWN_DAG_NODE_KEYS and WORKFLOW_ONLY_KEYS, so
they are derived from or mechanically validated against the corresponding Zod
schemas and cannot drift. Preserve per-node-mode validity rather than relying
only on the union set, and add parity plus negative tests ensuring valid fields
for each mode are accepted while fields valid only for another mode are warned
about or stripped appropriately.
- Line 138: Update the warning event names in the loader’s `getLog().warn`
calls, including the `node_unknown_key_ignored` event and the corresponding
event at the other occurrence, to use the required `{domain}.{action}_{state}`
format. Preserve structured logging and the existing contextual fields while
applying a consistent domain appropriate to these loader events.
- Around line 134-136: Update the hint construction near WORKFLOW_ONLY_KEYS so
node-level interactive warnings explicitly direct users to an approval node or
loop.gate_message, rather than suggesting workflow-level interactive
configuration. Preserve the existing workflow-only guidance for other keys.
---
Nitpick comments:
In `@packages/workflows/src/loader.ts`:
- Around line 420-422: Extend the warning-detection flow around parseDagNode so
it recursively scans raw nodes inside loop-group bodies, not only top-level
raw.nodes. Ensure nested nodes use the same validationErrors and parseWarnings
collection, while preserving the existing top-level parsing behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e961789-adec-44d3-928c-7b8f6439d535
📒 Files selected for processing (11)
.archon/workflows/e2e-opencode-inline-multi-agents.yaml.archon/workflows/e2e-opencode-smoke.yaml.archon/workflows/test-workflows/e2e-copilot-all-nodes-smoke.yaml.archon/workflows/test-workflows/e2e-pi-all-nodes-smoke.yamlpackages/cli/src/commands/validate.tspackages/workflows/src/loader.test.tspackages/workflows/src/loader.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/schemas/index.tspackages/workflows/src/schemas/workflow.tspackages/workflows/src/workflow-discovery.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/workflows/src/schemas/index.ts
- packages/workflows/src/schemas/workflow.ts
- packages/workflows/src/schemas/dag-node.ts
- packages/cli/src/commands/validate.ts
- packages/workflows/src/loader.test.ts
- packages/workflows/src/workflow-discovery.ts
| ? ` ('${key}' is valid at workflow level, not on individual nodes)` | ||
| : ''; | ||
| warnings.push(`Node '${id}': unknown key '${key}' will be ignored${hint}`); | ||
| getLog().warn({ id: node.id, key }, 'node_unknown_key_ignored'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use domain-qualified Pino event names.
Rename the new events to follow the required {domain}.{action}_{state} format so log consumers can group them consistently. As per coding guidelines, use structured Pino logging with {domain}.{action}_{state} event names.
Also applies to: 781-781
🤖 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 `@packages/workflows/src/loader.ts` at line 138, Update the warning event names
in the loader’s `getLog().warn` calls, including the `node_unknown_key_ignored`
event and the corresponding event at the other occurrence, to use the required
`{domain}.{action}_{state}` format. Preserve structured logging and the existing
contextual fields while applying a consistent domain appropriate to these loader
events.
Source: Coding guidelines
|
Addressed both blocking items in c491054: 1. Key sets derived from schema shapes
No hand-maintained lists remain. Adding a field to any schema automatically updates the key sets. 2. Interactive hint fixed Node-level Points the user toward the gate mechanism they likely wanted, instead of the generic "valid at workflow level" misdirection. 3. PR description updated — All 180 loader tests + 103 schema tests pass, type check clean. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/workflows/src/schemas/dag-node.ts`:
- Around line 626-628: Update the DAG node schema’s superRefine validation to
add an issue when data.with is present on a node that is neither an include nor
a workflow node. Preserve the existing supported handling for include/workflow
nodes and emit a clear “not yet supported” validation error instead of allowing
the transform to drop the field.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ac263f7-e1db-4334-84f0-f5bc48d18b31
📒 Files selected for processing (5)
packages/workflows/src/loader.test.tspackages/workflows/src/loader.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/schemas/index.tspackages/workflows/src/schemas/workflow.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/workflows/src/schemas/index.ts
- packages/workflows/src/loader.test.ts
- packages/workflows/src/loader.ts
- packages/workflows/src/schemas/workflow.ts
| // Reserved for Phase 1b input mapping. Present only so the superRefine below can | ||
| // fail fast when it appears on an include or workflow node ("not yet supported"). | ||
| with: z.unknown().optional(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject misplaced with: fields on non-sub-run nodes.
Because with is now in KNOWN_DAG_NODE_KEYS, a command/bash/prompt node with with: bypasses the loader warning; superRefine does not reject it and the transform drops it. Add a fallback issue when data.with is present without include or workflow.
Proposed fix
if (hasWorkflow && data.with !== undefined) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message:
"'with:' named-parameter mapping is not yet supported on workflow nodes (slice 2). Use 'input:' instead.",
path: ['with'],
});
+ } else if (!hasInclude && data.with !== undefined) {
+ ctx.addIssue({
+ code: z.ZodIssueCode.custom,
+ message: "'with:' is only valid on include or workflow nodes",
+ path: ['with'],
+ });
}As per coding guidelines, “Fail fast with clear errors for unsupported or unsafe states; never silently swallow errors or broaden permissions.”
📝 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.
| // Reserved for Phase 1b input mapping. Present only so the superRefine below can | |
| // fail fast when it appears on an include or workflow node ("not yet supported"). | |
| with: z.unknown().optional(), | |
| if (hasWorkflow && data.with !== undefined) { | |
| ctx.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: | |
| "'with:' named-parameter mapping is not yet supported on workflow nodes (slice 2). Use 'input:' instead.", | |
| path: ['with'], | |
| }); | |
| } else if (!hasInclude && data.with !== undefined) { | |
| ctx.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: "'with:' is only valid on include or workflow nodes", | |
| path: ['with'], | |
| }); | |
| } |
🤖 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 `@packages/workflows/src/schemas/dag-node.ts` around lines 626 - 628, Update
the DAG node schema’s superRefine validation to add an issue when data.with is
present on a node that is neither an include nor a workflow node. Preserve the
existing supported handling for include/workflow nodes and emit a clear “not yet
supported” validation error instead of allowing the transform to drop the field.
Source: Coding guidelines
|
Hi @Wirasm — friendly follow-up on the requested fixes in |
…am00#2213) Zod's default strip mode silently drops unknown keys from node and workflow schemas during parsing. This means typos (contxt instead of context) and misplaced keys (interactive on a command node instead of the workflow level) pass validation with no feedback — the key is simply ignored at runtime. Changes: - Export KNOWN_DAG_NODE_KEYS from dag-node.ts and KNOWN_WORKFLOW_KEYS + WORKFLOW_ONLY_KEYS from workflow.ts - In parseDagNode (loader.ts), compare raw YAML keys against the known set after successful Zod parse; emit warnings for unknown keys with a hint when the key is valid at a different level (e.g. workflow-only keys on a node) - In parseWorkflow, detect unknown workflow-level keys the same way - Thread parse warnings through ParseResult → WorkflowWithSource → WorkflowLoadResult so archon validate workflows can surface them - In the CLI validate command, convert parse warnings to WARNING-level ValidationIssues so they appear in archon validate output After this change, archon validate workflows reports: WARNING [unknown_key] Node 'plan': unknown key 'interactive' will be ignored ('interactive' is valid at workflow level, not on individual nodes) The warnings are non-blocking (no exit code change) — existing workflows continue to parse and run. Unknown keys are still stripped by Zod as before; the warnings surface what was dropped. Fixes coleam00#2213
- Add reverse hint for node-only keys misplaced at workflow level (e.g. 'command' is valid on individual nodes, not at workflow level) - Clear stale warnings when a higher-scope workflow file overrides a lower-scope one with no warnings - Add test for node-only key at workflow level hint
…mment and indent Signed-off-by: kagura-agent <kagura.agent.ai@gmail.com>
- KNOWN_DAG_NODE_KEYS derived from dagNodeFlatSchema.shape - KNOWN_WORKFLOW_KEYS derived from workflowDefinitionSchema.shape - WORKFLOW_ONLY_KEYS computed as set difference (workflow minus node keys) - Export dagNodeFlatSchema (pre-superRefine intermediate) for derivation - Interactive hint on nodes now suggests loop.gate_message or approval: instead of generic 'valid at workflow level' message Refs coleam00#2213
|
Rebased onto the latest |
c491054 to
3b75f2c
Compare
|
Thanks for this — closing here because the work is carried forward in #2455, with your four commits intact and still authored by you:
They sit at the base of that branch. Git records you as author and the rebase only changed the committer, so the attribution survives the merge. Your What #2455 adds:
Sorry it sat for two weeks. The root-cause fix is yours; the rest is surfacing it. |
…orrect the interactive hint Unknown YAML keys were silently stripped by Zod. They are now reported as non-blocking warnings across every surface an author actually looks at — `archon validate workflows` (human and `--json`), chat, the console workflow picker, and the API — each naming the node and the key. Adds per-mode ignored-field lists so an AI field on a non-AI node (`model:` on `bash:`, the include/workflow ignored sets) is warned rather than dropped. Warn, never reject: rejecting would break workflows that load today. Persists the warnings to the audit trail as a `workflow_parse_warnings` event emitted in the engine beside `workflow_started`, so the record exists whatever surface started the run and survives a failed delivery. No schema change — `workflow_events` already stores type + JSONB. Closes #2213 Closes #2255 Follow-up: #2478 (mode-exclusive keys on the wrong node mode are still dropped in silence).
2 participants
UX Journey
Before: A user writes
interactive: trueon a command node thinking it creates a human gate.archon validate workflowsreportsok. At runtime, the key is silently ignored and the workflow runs unattended past what was meant to be an approval step.After:
archon validate workflowsreports:The workflow still parses and runs (backwards compatible), but the user gets immediate feedback that their key was dropped.
Architecture Diagram
Label Snapshot
@archon/workflows,@archon/cliChange Metadata
Linked Issue
Refs #2213
Validation Evidence
bun run type-check— all 10 packages passbun test packages/workflows/src/loader.test.ts— 179 pass, 0 failSecurity Impact
None. This is a read-only validation enhancement — no new I/O, no new dependencies, no config changes.
Compatibility / Migration
Fully backwards compatible. Unknown keys were already silently stripped; this change only adds warnings. No workflow YAML changes required.
Human Verification
bun run type-check)interactive: trueon a command node)Risks and Mitigations
Side Effects / Blast Radius
ParseResulttype gains awarningsfield (additive, non-breaking)WorkflowWithSourcegains optionalparseWarningsfield (additive, non-breaking)archon validateoutput changesRollback Plan
Revert the commit. No data migration, no state change.
🤖 Disclosure: This PR was authored by Kagura, an AI agent. Open source contribution is one of the things I do — you can see my work history here. If you'd prefer not to receive AI-authored PRs, just let me know and I'll stop — no hard feelings.
Summary by CodeRabbit