test(workflows): guard against workflow-level schema fields being silently dropped at parse - #2459
Conversation
…ently dropped at parse parseWorkflow hand-assembles its result field by field, so a field added to workflowDefinitionSchema but not to that object literal is silently discarded: the YAML parses, the workflow loads, and the feature is inert. That already happened. `requires:` landed in workflowBaseSchema in ab81248 (2026-06-01) without touching the loader, and the assembly block only arrived in 2d7bf58 (2026-07-16) — six weeks in which the GitHub capability gate could never fire for a discovered workflow, fixed incidentally inside an unrelated PR. This is the third instance of one pattern: parallel enumerations that must agree with nothing enforcing agreement. The ref-surface enumerations carry a KEEP IN SYNC comment and were found broken anyway (#2450); the nested key sets are derived from each schema's .shape and cannot drift (#2455). This applies the derived form to the second case. The field list comes from workflowDefinitionSchema.shape, so a new schema field fails the test until it is given a fixture. Deliberately not solved by deriving the assembly itself — the hand assembly exists because of warn-and-drop, and schema.parse() would reject a bad field instead of logging and dropping it. The per-field assertion clears the mock logger first so it can tell the two failure causes apart: a warning means the fixture value is invalid (warn-and-drop working as designed), silence means a valid field was dropped (the actual bug). Verified by breaking it both ways: removing `requires` from the object literal reproduces the historical bug and fails with the right diagnosis, and adding a new schema key fails the ratchet until a fixture exists.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds schema-parity tests for workflow-level fields. The tests derive fields from ChangesWorkflow schema parity
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
PR Review SummaryPR: #2459 — Headline: the guard actually guards. Verified empirically by two independent agents via mutation testing — deleting a field from Everything below is polish: 2 cheap edits worth making, 4 optional. Critical Issues (0 found)None. Important Issues (2 found)
I1 — the warn-and-drop justification is factually overstated. The comment reads: "the hand assembly exists BECAUSE of warn-and-drop — a present-but-invalid field is logged and dropped rather than aborting the whole discovery pass." That is false for at least three fields, one of which (
Verified directly, not taken from the agent. The conclusion still holds — one warn-and-drop field is enough to make a blanket Suggested rewrite:
I2 —
All three schema shapes verified independently. Note the other 17 fixtures all do real equality or Suggestions (4 found)
S1 — the hand-assembly literal (with S2 — "a warning fired ⇒ your fixture value is wrong" is correct today: every field-specific S3 — the two long diagnostic strings inline in the ternary bury the assertion. Same behaviour, easier to scan: const message = warned ? `…fix the fixture, not the loader.` : `…add the field to that literal.`;
expect(fixture.present(result.workflow as WorkflowDefinition), message).toBe(true);S4 — Explicitly considered and NOT recommended (raised, then argued down):
Strengths
Documentation Issues
VerdictNEEDS FIXES (minor) — no defects in the guard; 4 one-line edits worth making before merge. Nothing found blocks correctness, and no agent requested changes to the mechanism. Both Important items are cheap and both make a precision-tool more precise, which is the whole point of the artifact. Recommended Actions
Reviewed by Claude — 4 specialist agents, findings independently re-verified against the code before inclusion. |
Addendum to the review aboveThe comment-analysis pass finished after I posted, with a field-by-field audit that sharpens I1 and promotes one suggestion to I3. Both re-verified against I1 is wider than I first reportedI said three fields contradict the "warn-and-drop" claim. The full audit of all 20 says only 13 actually warn-and-drop:
Verified there is no New — I3: the warn/no-warn discriminator is blind for exactly those 3 fieldsThe two-branch failure message is the best thing in this diff: a warn means "your fixture value is invalid", silence means "the loader dropped a valid field — that's the #2457 bug." Sound for 17 fields. Backwards for Write an invalid fixture for one of them — That is precisely the failure the PR body describes hitting during development (the bad Revised actions
Still no defect in the guard itself — all three items are comment/predicate precision on an artifact whose entire job is precision. Verdict unchanged: NEEDS FIXES (minor). |
Addresses I1, I2, I3 and S1, S3, S4 from the review on #2459. No change to what the guard catches; all six make a precision tool more precise. I1 — the docblock claimed warn-and-drop universally. Re-verified the field audit against loader.ts rather than taking it on faith: 4 of the 20 hard-reject (name, description, nodes, evidence_policy at :619-629), 13 warn-and-drop, and 3 coerce silently with no log at all (provider :423, model :425, persist_sessions :473 — there is no invalid_provider/invalid_model/invalid_persist_sessions warn event anywhere in the file). Rewritten to say most rather than all, and to point at loader.ts as the authority instead of restating a per-field table that would rot the moment a field changes category. I3 — the two-branch failure message was backwards for exactly those 3 silent fields: a bad `provider: 123` fixture is discarded with no warning, so the message confidently blamed the loader and sent the reader into parseWorkflow when the fixture was at fault. That is the same failure the message exists to prevent, and the one I hit during development with a bad `thinking: true` fixture. Fixed by ranking rather than verdict: a warning is still strong evidence the fixture is wrong, but silence now names both causes and points at the fixture first. Chosen over listing the three exceptions in a comment, which would duplicate loader.ts and rot. This subsumes S2's unstated-invariant concern. I2 — effort, thinking and sandbox used presence checks where the other 17 fixtures check values, and their schemas transform deterministically, so exact checks are available. Verified by mutation: returning effort:'low' and thinking:{type:'disabled'} from the loader now fails both round-trips, where before it left them green. S1 — the hand-assembly literal predates 2d7bf58; only the requires entry landed there. Reworded so it cannot be skimmed as "the mechanism didn't exist until then". S3 — the two diagnostic strings moved out of the assertion into a named message. S4 — nodes?.length, so a dropped nodes yields a clean false instead of a TypeError. Verified: full validate green (132 batches, 0 fail); the I3 message re-checked by running a deliberately invalid provider fixture; I2 re-checked by mutation.
|
Pushed
I1 — accurate, but deliberately not a tableThe suggested rewrite enumerated the categories with field names. I didn't take that shape: a three-way per-field table in a test docblock duplicates I3 — the real fix was the message, not a list of exceptionsYou're right that this is the sharpest finding, and it is the same failure the message exists to prevent — I hit it during development with a bad Rather than name the three silent fields in a comment (which rots identically), I removed the false verdict. A warning is still strong evidence the fixture is wrong. Silence is no longer treated as proof of the opposite — it now names both causes, ranked, and sends the reader to the cheaper one first:
That is the actual output, captured by running a deliberately invalid I2 — verified by mutation, not by inspectionMaking the loader return Verification
One thing from your review I'm acting on elsewhereThe Documentation Issue — |
Second pass on the accuracy audit. The first fixed false claims; this removes the
blocks that generate them — 1008 lines to 690, and 456 fenced lines to 128.
The test applied to each block: does a reader learn something here they could not
get faster by running the thing? Five failed it, and each was checked against the
docs site and the code before being cut.
CLI command dump (136 lines) -> the three rules --help omits
`archon --help` and reference/cli.md (640 lines) are both more current than
this block already was: it never mentioned chat, setup, workflow
search/install, continue, or doctor --full. What survives is what neither
tells you — the git-repo requirement, isolation-by-default, and the --json
approve/reject/resume semantics.
packages/ tree (99 lines) -> package roots in dependency order
Every dependency constraint it encoded is already stated, more precisely,
in Package Split a hundred lines below. Order verified against each
package.json: every package depends only on those above it. Inside a
package, ls and the file docblocks outlive any tree drawn here.
assistant defaults yaml (38) -> a paragraph
reference/configuration.md carries the full key set. Kept the two keys
worth knowing before you look: the binary paths, and settingSources.
~/.archon tree (20) -> a paragraph
reference/archon-directories.md carries the layout. Kept the rule that
matters: artifacts and logs live outside the repo and are never committed.
The repo-level .archon/ tree stays — eight lines, referenced constantly.
SDK type patterns, import patterns (40) -> rule lines
Paired correct/wrong examples where the rule is one sentence.
Also corrects a claim the #2459 review surfaced: the sqlite.test.ts parity test
no longer compares table names only. It checks columns in both directions against
tracked allowlists, so a column added to one dialect and forgotten in the other
now fails CI.
Kept deliberately: the logging idiom and the error-handling patterns, which
encode exact shapes meant to be copied, and every behavioural section — Core
Principles, Engineering Principles and Product Direction are byte-identical.
Verified no section was lost: the heading list is unchanged.
Second pass on the accuracy audit. The first fixed false claims; this removes the
blocks that generate them — 1008 lines to 690, and 456 fenced lines to 128.
The test applied to each block: does a reader learn something here they could not
get faster by running the thing? Five failed it, and each was checked against the
docs site and the code before being cut.
CLI command dump (136 lines) -> the three rules --help omits
`archon --help` and reference/cli.md (640 lines) are both more current than
this block already was: it never mentioned chat, setup, workflow
search/install, continue, or doctor --full. What survives is what neither
tells you — the git-repo requirement, isolation-by-default, and the --json
approve/reject/resume semantics.
packages/ tree (99 lines) -> package roots in dependency order
Every dependency constraint it encoded is already stated, more precisely,
in Package Split a hundred lines below. Order verified against each
package.json: every package depends only on those above it. Inside a
package, ls and the file docblocks outlive any tree drawn here.
assistant defaults yaml (38) -> a paragraph
reference/configuration.md carries the full key set. Kept the two keys
worth knowing before you look: the binary paths, and settingSources.
~/.archon tree (20) -> a paragraph
reference/archon-directories.md carries the layout. Kept the rule that
matters: artifacts and logs live outside the repo and are never committed.
The repo-level .archon/ tree stays — eight lines, referenced constantly.
SDK type patterns, import patterns (40) -> rule lines
Paired correct/wrong examples where the rule is one sentence.
Also corrects a claim the #2459 review surfaced: the sqlite.test.ts parity test
no longer compares table names only. It checks columns in both directions against
tracked allowlists, so a column added to one dialect and forgotten in the other
now fails CI.
Kept deliberately: the logging idiom and the error-handling patterns, which
encode exact shapes meant to be copied, and every behavioural section — Core
Principles, Engineering Principles and Product Direction are byte-identical.
Verified no section was lost: the heading list is unchanged.
…report better (#2464) * docs: cut CLAUDE.md blocks that transcribe what the tools already report Second pass on the accuracy audit. The first fixed false claims; this removes the blocks that generate them — 1008 lines to 690, and 456 fenced lines to 128. The test applied to each block: does a reader learn something here they could not get faster by running the thing? Five failed it, and each was checked against the docs site and the code before being cut. CLI command dump (136 lines) -> the three rules --help omits `archon --help` and reference/cli.md (640 lines) are both more current than this block already was: it never mentioned chat, setup, workflow search/install, continue, or doctor --full. What survives is what neither tells you — the git-repo requirement, isolation-by-default, and the --json approve/reject/resume semantics. packages/ tree (99 lines) -> package roots in dependency order Every dependency constraint it encoded is already stated, more precisely, in Package Split a hundred lines below. Order verified against each package.json: every package depends only on those above it. Inside a package, ls and the file docblocks outlive any tree drawn here. assistant defaults yaml (38) -> a paragraph reference/configuration.md carries the full key set. Kept the two keys worth knowing before you look: the binary paths, and settingSources. ~/.archon tree (20) -> a paragraph reference/archon-directories.md carries the layout. Kept the rule that matters: artifacts and logs live outside the repo and are never committed. The repo-level .archon/ tree stays — eight lines, referenced constantly. SDK type patterns, import patterns (40) -> rule lines Paired correct/wrong examples where the rule is one sentence. Also corrects a claim the #2459 review surfaced: the sqlite.test.ts parity test no longer compares table names only. It checks columns in both directions against tracked allowlists, so a column added to one dialect and forgotten in the other now fails CI. Kept deliberately: the logging idiom and the error-handling patterns, which encode exact shapes meant to be copied, and every behavioural section — Core Principles, Engineering Principles and Product Direction are byte-identical. Verified no section was lost: the heading list is unchanged. * docs: cut the remaining CLAUDE.md blocks that restate their own prose Second half of the shrink pass, under the inverted default: cut when in doubt, and justify what stays rather than what goes. logging (24 lines) -> two sentences The "Event naming rules" bullets immediately below already state the convention the block illustrated. What survives is the call shape — structured object first, event name second — and the three fields an error log must carry. error handling (29) -> two sentences Both blocks were generic try/catch. The git one's actual content was classifyIsolationError, and the sentence under it already said so. Kept the rule that matters: log the raw error AND send the classified message, because doing one of the two is the bug the pattern exists to prevent. running in worktrees (22) -> one sentence A transcript of `bun dev` plus three curl calls. The port rule is the content and it is stated in prose right below; the API calls are in the API Endpoints section. dev / testing / type-check / validate command blocks (35) -> four lines Script-name lists that package.json reports. Kept the ports (3090, 5173) and the generate:types ordering constraint, neither of which a script name tells you. Retained blocks and why each survives: packages/ root list (12) — dependency ORDER is the rule, and `ls` cannot tell you which package may depend on which. Verified against every package.json. repo-level .archon/ tree (6) — the canonical statement of what Archon reads from a repository, with no docs-site equivalent that is more current. CLAUDE.md is now 574 lines from 1008, and 22 fenced lines from 456. Section headings unchanged; Core Principles, Engineering Principles and Product Direction remain byte-identical.
…2523) * feat(workflows): structural workflow signature — inputs/returns/with on workflow: nodes (#2470) Adds a declarative signature surface to the workflow language (Signature Phase 2): a workflow declares `inputs:` (what it takes) and `returns:` (which node's output is its result); callers supply values via `with:` — now accepted on `workflow:` sub-run nodes, not just `include:`. This coordinates, it does not compute (Workflow Language Constitution — cited). Changes: - Schema: `inputs:`/`returns:` on workflowBaseSchema; `with:` on workflow nodes with include-style shape validation; reject `with`+`input`; `fan_out.as`↔`with` collision check; delete the dead `fan_out.as` placeholder rejection (#2224 merged); `inputEnvKey` - Loader: parse/validate inputs (warn-and-drop, reject required+default), returns top-level-id existence check, env-key mangling collision reject, scan `workflow:` `with:` values for dangling refs, four-surface KEEP-IN-SYNC comment - Include-expander: `returns:` drives primarySink only (sinks/depends_on unchanged, may be a non-sink); validate `with:` against declared `inputs:` (defaults/missing-required/ undeclared-key) only when inputs declared — undeclared blocks byte-for-byte unchanged - Runtime: strict `$INPUTS.<name>` substitution in the `!shellSafe` branch (did-you-mean hint, throws on unknown); `INPUTS_<UPPER_SNAKE>` env for bash/script sub-run nodes; persist `metadata.inputs` at spawn so `$INPUTS` reconstitutes on cold resume; fan-out static `with:` + per-item `$INPUTS.<as>` channel - `returns:` rebinds a child run's terminal output (parent_run_id-gated; blank → '' + WARN, no sink fallthrough); loop_group per-iteration scan deliberately unchanged - Bare-run guard: a required-input block still lists/loads but a top-level run fails before any worktree/clone/AI cost (CLI + orchestrator) - Validator: bundled-set-only `workflow:` target check via the runtime fuzzy resolver - Docs: authoring-guide Workflow Signature section + binding-time table + `INPUTS_*` env mangling; constitution admits inputs/returns and records cross-file schema checking rejected - Tests: signature parse/validate, returns→primarySink, with vs inputs, $INPUTS runtime, bundled-target check, bare-run guard, env-key mangling; #2459 parity ratchet fixtures added Closes #2470 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * simplify: reduce complexity in changed files Hoist resolveRunInputs(parentRun) out of the per-entry with: resolution loops in executeWorkflowNode and executeFanOutWorkflowNode — it re-parsed run metadata on every iteration. Computed once into a named local now. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: remove out-of-scope hyperframes default workflow Was accidentally included in this PR and caused check:bundled to fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R4KmwzY3iYy4ZBUw7RkwGY * fix(workflows): address review — with: expansion, runtime input contract, docs Addresses the changes requested on #2523. - include-expander: rewriteNodeOutputRefs() and applyInputsMacro() now walk every value in a workflow: node's with: map, so an included block that calls a child workflow can forward $INPUTS and reference its own nodes. Previously an included block's with: value referencing a child-local node failed with "references unknown node '$local.output'". - Extract the declared-input contract into workflow-inputs.ts and enforce it on the runtime workflow: path as well: declared defaults applied, unsupplied required inputs and undeclared keys rejected, resolved after child discovery and before isolation/worktree or run-row creation. Both call surfaces now go through one implementation, so include: and workflow: accept identical maps by construction. - Apply declared defaults to bare top-level runs, which have no parent to stamp metadata.inputs. - CLAUDE.md: with: is no longer rejected on workflow: nodes and fan_out.as is no longer reserved; document the workflow-level inputs:/returns: signature. Regression coverage: included block with a workflow: node whose with: values use both $INPUTS and a child-local node ref; sub-run defaults, missing-required and undeclared inputs; runtime $INPUTS delivery into AI surfaces; returns: rebinding on a sub-run; cold-resume metadata reconstitution. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R4KmwzY3iYy4ZBUw7RkwGY --------- Co-authored-by: Archon <archon@archon.dev> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Archon Maintainer Bot <rasmus.widing@gmail.com>
1 participant
Summary
parseWorkflowhand-assembles its result field by field, so a field added toworkflowDefinitionSchemabut not to that object literal is silently discarded — the YAML parses, the workflow loads, and the feature is simply inert. Nothing catches it.requires:was added toworkflowBaseSchemainab81248d(2026-06-01) without touching the loader; the assembly block only landed in2d7bf587(2026-07-16). Six weeks in which the GitHub capability gate could never fire for any discovered workflow — fixed incidentally inside an unrelated PR, not by anything noticing.workflowDefinitionSchema.shape, plus a per-field round-trip fixture. A new schema field fails the test until it is given a fixture.git diff --statis one test file.This is the third instance of one pattern
Parallel enumerations that must agree, with nothing enforcing agreement:
loader.tsscan /rewriteNodeOutputRefs/substituteNodeOutputRefssites)loader.ts:226-231when:shorthand escapes ref validationworkflowDefinitionSchema.shapevsparseWorkflow's literalrequires:, six weeks.shapeThe only difference in the third row is that it is derived rather than described. This PR applies that form to the second row.
Why a parity test and not deriving the assembly
Deriving the assembly (
workflowDefinitionSchema.parse(raw)) would eliminate the class outright, and it is the wrong trade here. The hand assembly exists because of warn-and-drop: a present-but-invalid field is logged and dropped rather than rejecting the whole workflow, so one typo cannot abort a discovery pass..parse()rejects instead. Restoring per-field warn-and-drop underneath a derived parse needs.catch()plus a logging hook on all 20 fields — a real refactor with behaviour-change risk on the discovery path, for the same protection.Note what #2455 actually derives: a set of names. Names are cheap to derive and carry no semantics. This PR derives the same thing.
UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
schemas/workflow.tsloader.test.tsObject.keys(workflowDefinitionSchema.shape)loader.ts(parseWorkflow)loader.test.tsschemas/workflow.tsloader.tsLabel Snapshot
risk: lowsize: Stestsworkflows:loaderChange Metadata
choreworkflowsLinked Issue
inputs:/returns:fields would hit exactly this trap)Validation Evidence (required)
bun run validate # wrapper exit 0Grepped the log rather than trusting the wrapper's exit code:
132 test batches reported, every one
0 fail, and the pass-line count matches the fail-line count so no batch silently failed to report.The guard was verified by breaking it, in both directions — a test that has never been red proves nothing:
...(requires !== undefined ? { requires } : {})fromparseWorkflow's literal:brand_new_field: z.string().optional()toworkflowBaseSchema, touching nothing else:Both breaks reverted; the diff is the test file alone.
Security Impact (required)
Test-only change.
Compatibility / Migration
Human Verification (required)
devbefore anything else (all 20 keys already present in the literal, so this lands as a ratchet rather than an unscoped bug hunt — worth knowing, since a red first run would have been a different-sized PR); both deliberate breaks above.thinking:fixture usedthinking: true, which is not a validthinkingConfigSchemavalue, so warn-and-drop correctly discarded it while the test blamed the loader. Fixed by clearing the mock logger before each parse and branching the message on whether a warning fired, so the two causes are told apart.bun test; there is no runtime component to exercise.Side Effects / Blast Radius (required)
@archon/workflowstest suite only.loader.test.ts, whichpackages/workflows/package.jsonalready runs in its ownbun testinvocation.Rollback Plan (required)
git revert <sha>— one test file, no production code.Risks and Mitigations
parseWorkflow, so a fixture cannot be satisfied without the literal carrying the field.presentis a survival predicate, not deep equality, so!== undefinedsuffices — which is exactly howthinkingandsandboxare handled today.Summary by CodeRabbit