fix(workflows): surface unknown-key warnings where authors are, and correct the interactive hint

[Wirasm]

Builds on #2255 by @kagura-agent. Their four commits are cherry-picked here with authorship intact; this branch lives in the main repo rather than on their fork so their branch is not pushed to. Three commits are added on top. The maintainer decides afterwards whether this lands here or as a push to the fork.

The unrelated e2e YAML edits from #2255 (commit 71dddc2, four files fixing a different pre-existing validator warning and deleting an explanatory comment) are dropped — they are not this change.

Summary

• Problem: Zod strips unknown keys from workflow YAML silently. An author who writes interactive: true on a command node believing it creates a human gate gets no feedback, and the workflow runs unattended past what was meant to be an approval step (bug: workflow validator silently accepts unknown node keys (e.g. interactive: true on a command node) #2213). fix(workflows): warn on unknown/misplaced keys in workflow YAML (#2213) #2255 started emitting warnings for this, but parseWarnings had exactly one consumer in the tree — archon validate workflows, which is not part of bun run validate and which nothing requires an author to run. On every other surface the warning was degraded, silent, or dropped.
• Why it matters: Validation blessed a safety property that did not exist, and the fix reported it somewhere nobody looks.
• What changed: (1) warnings now reach every surface an author actually uses; (2) the run-path log line carries the prose, not just a payload and an event name; (3) the interactive hint was wrong and is corrected; (4) detection extended one level down, including into loop_group body nodes.
• What did not change (scope boundary): The posture stays warn, not reject — unknown keys are still stripped and the workflow still loads and runs. No change to how any key is parsed or executed. No new node fields, no YAML surface additions.

On the "warn rather than reject" justification

The original PR justified this as forward-compatibility. That is not Archon's posture — steps: hard-errors, hooks: is .strict(), with:/retry:/isolation all fail fast. The correct justification is precedent: archon validate workflows already carries a non-blocking warning framework for silently-ignored input ([bash] double-quoting a substitution that is already shell-quoted, [hooks] not supported by provider 'opencode' — this will be ignored, [denied_tools] Task was renamed to Agent). This adds one more class to that existing surface.

UX Journey

Before

  Author                     Archon                         Where it showed up
  ──────                     ──────                         ──────────────────
  writes `interactive: true`
  on a command node
                        ──▶  Zod strips the key
                             loader records a warning
                             string in parseWarnings

  archon validate workflows   ─────────────────────────────▶ full message ✓
                                                             (but not in `bun run validate`,
                                                              and nothing requires running it)
  archon workflow run         ─────────────────────────────▶ one log line: {id, key}
                                                             + event name. No prose, no hint.
  archon workflow run --json  ─────────────────────────────▶ SILENT (log level = silent)
                                                             ← the channel the chat agent
                                                               drives runs through
  /workflow list (chat)       ─────────────────────────────▶ nothing
  console / web               ─────────────────────────────▶ DROPPED
                                                             (api.ts mapped ws => {workflow, source})

  run proceeds unattended past the "gate" the author thought they wrote

After

  Author                     Archon                         Where it shows up
  ──────                     ──────                         ─────────────────
  writes `interactive: true`
  on a command node
                        ──▶  Zod strips the key
                             loader records the warning
                             [+ descends into nested blocks
                                and loop_group body nodes]

  archon validate workflows   ─────────────────────────────▶ full message ✓ [*corrected hint*]
  archon workflow list        ─────────────────────────────▶ [*inline under the workflow*]
  archon workflow list --json ─────────────────────────────▶ [*parseWarnings on the entry*]
  archon workflow run         ─────────────────────────────▶ [*stderr notice, scoped to THIS
                                                              workflow, before the run starts*]
                                                             + log line now carries the prose
  archon workflow run --json  ─────────────────────────────▶ [*stderr notice — stdout stays
                                                              exactly the JSON payload*]
  /workflow list (chat)       ─────────────────────────────▶ [*⚠ inline with the workflow*]
  console workflow picker     ─────────────────────────────▶ [*⚠ marker on the row,
                                                              full text in the tooltip*]

Architecture Diagram

Before

  loader.ts ──parseWarnings──▶ workflow-discovery.ts ──▶ WorkflowWithSource.parseWarnings
                                                              │
                                                              ├──▶ cli/validate.ts        ✓ ONLY consumer
                                                              ├──▶ cli/workflow.ts        (ignored)
                                                              ├──▶ core/command-handler   (ignored)
                                                              └──▶ server/api.ts          (dropped in map)
                                                                        │
                                                                        └──▶ web/console  (never sees it)

After

  schemas/dag-node.ts [~]  KNOWN_DAG_NODE_KEYS
                           [+] NestedKeySpec, KNOWN_NODE_NESTED_KEYS
                           [+] approvalConfigSchema (extracted so its .shape is nameable)
       │  derives from .shape of: approvalConfig, approvalOnReject, stepRetryConfig,
       │                          loopNodeConfig, loopControl, piNodeConfig, agentDefinition
       ▼
  schemas/workflow.ts [~]  KNOWN_WORKFLOW_KEYS, WORKFLOW_ONLY_KEYS
                           [+] KNOWN_WORKFLOW_NESTED_KEYS
       │                        (worktreePolicy, containerPolicy, evidencePolicy)
       ▼
  loader.ts [~] ═══ collectUnknownNodeKeys ──recurses──▶ loop_group.nodes[*] (full DAG nodes)
                ═══ collectUnknownConfigKeys ──recurses──▶ nested blocks, incl. agents.<id>.*
                ═══ unknownNodeKeyHint (interactive hint CORRECTED)
                ═══ pushUnknownKeyWarning (log line now carries the prose)
       │
       ▼
  workflow-discovery.ts (unchanged)  ──▶ WorkflowWithSource.parseWarnings
       │
       ├──▶ cli/validate.ts        ✓ unchanged
       ├──▶ cli/workflow.ts   [~] ═══ emitParseWarnings() on run (stderr) + list (inline/json)
       ├──▶ core/command-handler [~] ═══ /workflow list renders ⚠ inline
       └──▶ server/api.ts     [~] ═══ parseWarnings carried in the response
                 │                     (+ workflow.schemas.ts [~] response schema)
                 └──▶ web/console [~] ═══ primitives/workflow.ts maps it
                                    ═══ WorkflowPicker.tsx renders the ⚠ marker

Connection inventory

From	To	Status	Notes
schemas/dag-node.ts	schemas/loop.ts, schemas/retry.ts	unchanged	already imported; now also read for .shape
schemas/workflow.ts	schemas/dag-node.ts	modified	adds a type-only import of NestedKeySpec
loader.ts	schemas/dag-node.ts	modified	imports KNOWN_NODE_NESTED_KEYS + NestedKeySpec
loader.ts	schemas/workflow.ts	modified	imports KNOWN_WORKFLOW_NESTED_KEYS
cli/commands/workflow.ts	WorkflowWithSource.parseWarnings	new	run-path stderr notice + list rendering
core/handlers/command-handler.ts	WorkflowWithSource.parseWarnings	new	/workflow list
server/routes/api.ts	WorkflowWithSource.parseWarnings	new	previously dropped in the map
server/routes/schemas/workflow.schemas.ts	—	modified	optional parseWarnings on WorkflowListEntry
web/console/primitives/workflow.ts	/api/workflows	modified	maps the new field
web/console/components/WorkflowPicker.tsx	Workflow.parseWarnings	new	⚠ marker
workflows/test-utils.ts	—	modified	makeTestWorkflowWithSource takes optional parseWarnings

Label Snapshot

• Risk: risk: low
• Size: size: M
• Scope: workflows cli core server web docs
• Module: workflows:loader, workflows:schemas, cli:workflow, core:command-handler, server:routes, web:console

Change Metadata

• Change type: bug
• Primary scope: multi

Linked Issue

• Closes bug: workflow validator silently accepts unknown node keys (e.g. interactive: true on a command node) #2213
• Supersedes fix(workflows): warn on unknown/misplaced keys in workflow YAML (#2213) #2255 (builds on it; credit to @kagura-agent)

Validation Evidence (required)

bun run validate   # wrapper exit 0

The wrapper's exit code has masked a real failure on this repo before, so the log was grepped directly rather than trusted:

• check:bundled, check:bundled-skill, check:bundled-schema, check:pi-vendor-map (OK), check:capability-matrix (OK) — all pass
• type-check — all 11 packages Exited with code 0
• lint (--max-warnings 0) — clean, no output
• format:check — All matched files use Prettier code style!
• test — 133 batches, every one 0 fail

One pre-existing log line appears in the output and is not a failure: TypeError: Spread syntax requires ...iterable not be null or undefined at api.ts:4340, logged from inside api.auth.test.ts, whose batch reports 18 pass / 0 fail. api.ts:4340 is untouched by this PR (the single diff hunk in that file is at line 2969).

Tests fail before the fix. Verified by stashing only the source changes and re-running. Note this compares against the commit within this PR that precedes each fix, not against dev — at the base commit the warnings field does not exist at all, so a PR-vs-dev comparison would fail to compile rather than fail an assertion:

• Engine (loader.test.ts): 8 fail — the 7 new nested cases plus the strengthened interactive-hint assertion.
• Surfaces: CLI 3 fail, server 1 fail, chat 1 fail.

With the fix restored: 189 / 208 / 46 / 124 pass, 0 fail.

Security Impact (required)

• New permissions/capabilities? No
• New external network calls? No
• Secrets/tokens handling changed? No
• File system access scope changed? No

Read-only validation reporting. The one thing worth naming: warning strings are built from author-written YAML key names and are written to stderr / a chat message / a JSON field. Keys come from Object.keys() on the parsed YAML, never from secrets or env, and the console renders them as React text (escaped), not HTML.

Compatibility / Migration

• Backward compatible? Yes — unknown keys are still stripped and workflows still load and run exactly as before. Only reporting changed.
• Config/env changes? No
• Database migration needed? No

GET /api/workflows gains an optional response field; existing clients ignore it. archon workflow list --json gains an optional per-entry field. workflow run --json stdout is byte-identical — the notice goes to stderr.

Precision on that last point: workflow run only emits a JSON payload at all under --detach (pre-existing — CLAUDE.md's --json list correctly excludes run). Without --detach, --json suppresses logs but the command still prints human progress. The stdout-purity guarantee therefore applies to run --detach --json, and that is the context the test now exercises.

Human Verification (required)

Authored a real workflow with interactive: true on a command node and on a loop_group body node, in a scratch git repo, then checked each surface on a live build:

• archon validate workflows gated — both warnings, full prose and corrected hint.
• archon workflow list — both warnings inline under gated.
• archon workflow list --json — parseWarnings present on gated, absent on clean workflows; stdout parsed cleanly.
• archon workflow run gated --json --detach — stdout was exactly the {ok: true, ...} payload; both warnings on stderr. This is the channel that was previously completely silent.
• GET /api/workflows?cwd=… on a running server — parseWarnings present on gated only.
• Console workflow picker (browser, real server + vite) — ⚠ marker on the gated row only, full guidance in the tooltip/aria-label. Screenshot taken.

No-false-positive check on the real corpus: archon validate workflows --json over Archon's own 51 workflows still yields exactly 2 unknown_key warnings, both true positives in e2e-opencode-smoke.yaml (agent: general at workflow and node level — a live bug, silently dropped since April). The nested walk added zero noise. (An earlier revision of this description said 54; 51 is the correct count.) This is now pinned as a test rather than living only here — it fails if any workflow outside a known-bad allowlist starts warning.

Edge cases checked: clean nested blocks (a loop_group with valid interactive + gate_message, a body node with valid retry, an approval with valid on_reject) produce no warnings; free-form output_format keys produce no warnings; one workflow's warnings do not leak into another workflow's run.

Not verified: the include: interaction (see Side Effects); Slack/Telegram/Discord rendering of the /workflow list message (the message string is platform-agnostic and identical to how errors already renders there).

Side Effects / Blast Radius (required)

• Affected subsystems: workflow parsing (warning collection only), and the five reporting surfaces above.
• Potential unintended effects:
  • A repo with genuinely many unknown keys will now see more output than before, since detection reaches one level down. On Archon's own corpus the count is unchanged at 2.
  • The loader's per-key warn log fires during discovery, i.e. for every workflow discovered, not only the one being run. That was already true before this PR; the log line is now longer. The run-path notice is the scoped, targeted surface.
  • include: gap (pre-existing, not introduced here): warnings are keyed by the filename that was parsed. If workflow A include:s workflow B and B has unknown keys, the warnings attach to B's entry, not A's — so running A does not surface them. They still appear for B in validate and list. Now pinned by a test and stated in the authoring guide, so propagating them later is a visible, intentional change rather than a silent behaviour shift. The propagation itself is deferred: it is a behaviour change across an include boundary that deserves its own review, and include-expander.ts is being edited concurrently by feat(workflows): with: parameters on include: nodes — $INPUTS as a load-time macro (signature phase 1) #2466.
• Guardrails: the no-false-positive check above is the regression signal — if a future schema change makes a legitimate key look unknown, that count moves off 2.

Rollback Plan (required)

• Fast rollback: git revert the three commits on top of 5286dfa (or revert the whole branch). No data migration, no state change, no config.
• Feature flags: none — reporting only.
• Observable failure symptoms: spurious unknown key warnings on valid workflows (would mean a key set drifted from its schema), or workflow run --json stdout no longer parsing (would mean a warning leaked to stdout — there is a test asserting it does not).

Review round 2 (#2455 review, 2026-08-05)

Folded in: I1 (chat/console run path — the finding that contradicted this PR's own thesis), I2 (both derivation gaps), I3 (--json test now reaches writeJsonLine), I4 (a real bug this PR introduced — see below), S2, S4, S5, S6, S7, S8, S9-residual, and the cheap half of S1 (pin + document, propagation deferred).

I4 + S8 were done as one change. I4 was a filename-collision bug introduced by this PR: two parallel maps keyed by bare filename, one overwriting and one only ever adding, so a clean workflow could inherit a dropped file's warning. Rather than adding a second hand-written clearing loop, the warnings now live on the discovery entry — one Map.set() replaces both halves together, which removes the bug class structurally and deletes mergeWarnings plus its four call sites.

One correction to the review. I2's suggested one-liner — casting loopGroupNodeConfigSchema back to a ZodObject to recover .shape — is correct in substance but crashes at import where it was placed. Reading that shape fires the nodes getter, which builds z.array(dagNodeSchema); above dagNodeSchema in the module that is a temporal dead zone (ReferenceError: Cannot access 'dagNodeSchema' before initialization). tsc passes, so a compile-only check does not catch it — every test importing the module fails at load. The registry now sits at the end of the file with a comment pinning it there.

Deferred: S3 (unknown_key issues carry neither nodeId nor hint in validate --json). Making them structural means changing parseWarnings from string[] to an object array across the engine, the WorkflowWithSource contract, the OpenAPI response schema, the regenerated web types, and all seven renderers — a contract change several times the size of this whole round, for a Suggestion. It would also push message composition into each human surface, which is a quality regression risk in the prose. The information is present today, just not separately addressable.

CodeRabbit round. Four folded in: warning attribution across a same-name project/global shadow (the single-project auto-select branch RE-RESOLVES the workflow, so the caller's warnings could describe a file that is not running — the re-resolved entry now wins, with a regression test); role="img" on the picker glyph (a bare <span> has the implicit generic role, which prohibits an accessible name, so the aria-label was inert); "lists keys" → "warning messages" in three docs; and MD040 on a fence. One declined: warning when a mode-specific key appears on the wrong node type (with: on a command node, timeout: on a prompt node). Real gap, but it needs per-mode key sets and must be reconciled with the existing *_node_ai_fields_ignored warnings that cover the mirror-image case, or the two double-report — its own change, not a review fix.

Rebased twice (#2223, then #2224). The predicted dag-node.ts conflict landed exactly as expected and resolved as expected: fan_out: moved into dagNodeFlatSchema, so KNOWN_DAG_NODE_KEYS picks it up with nothing hand-listed. It also got a KNOWN_NODE_NESTED_KEYS entry, since fanOutConfigSchema strips unknown keys like every other nested block (fan_out.max_paralel would otherwise vanish silently).

A Windows CI failure worth recording. An unrelated SQLite upgrade test — 250 ms on dev — timed out twice at 5.3–6.0 s against Bun's 5000 ms per-test limit. This branch touches no DB code; the symptom is CPU starvation under bun --filter '*' --parallel test, and that test does real file I/O and is documented as Windows-fragile. The heaviest new test here (the corpus guardrail) now parses files directly instead of running full discovery — a tighter unit for a parser check and ~3× cheaper. On the green run that test came back at 1312 ms. Reported as correlation, not proof.

Partially done: S4. toWorkflow now has the co-located test its siblings have. The WorkflowPicker DOM is not unit-tested: this package has no DOM-rendering harness (RunStream.test.tsx, cited as the pattern, tests an exported pure function). The marker's condition is a direct parseWarnings.length > 0, and it was verified in a live browser with a screenshot.

Risks and Mitigations

• Risk: A nested key set drifts from its schema and produces false positives.
  • Mitigation: Every set is derived from the schema's .shape at module load — there is no hand-maintained list to drift. The one exception is loop_group, whose z.ZodType<…> annotation hides .shape from TS; it is rebuilt from loopControlSchema.shape plus its single nodes field, with a comment saying why.
• Risk: A future ProviderCapabilities-style addition adds a nested block nobody registers, so it silently goes unchecked.
  • Mitigation: Not fully mitigated. The block is documented in a comment listing exactly which node fields are deliberately absent and why (output_format, sandbox, hooks, thinking). A new nested config would need a new entry; this is a gap in coverage, not a source of false positives.
• Risk: Merge conflict with feat/subrun-fanout in packages/workflows/src/schemas/dag-node.ts — fix(workflows): warn on unknown/misplaced keys in workflow YAML (#2213) #2255 lifted the .extend({…}) block into dagNodeFlatSchema, and that branch inserts fan_out: into the same block.
  • Mitigation: Known and accepted. They are semantically compatible — KNOWN_DAG_NODE_KEYS derives from the shape, so fan_out becomes a known key automatically. Whoever merges second resolves it.

Summary by CodeRabbit

• New Features

  • Added warnings for unrecognized or misplaced workflow and node configuration keys.
  • Warnings appear in listings, validation results, command output, and workflow conversations without blocking execution.
  • JSON output keeps warnings separate from standard output for reliable parsing.
  • Added warning indicators and accessible details to the workflow picker.
  • API workflow listings include parse warnings when present and omit them for clean workflows.

• Documentation

  • Documented ignored keys, warning details, supported configuration areas, and warning behavior across interfaces.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Workflow parsing now records unknown-key warnings and carries them through discovery, validation, execution, API responses, and web display. Clean workflows keep the previous shape.

Changes

Workflow Parse Warnings

Layer / File(s)	Summary
Warning schema contracts packages/workflows/src/schemas/*, packages/workflows/src/test-utils.ts, packages/workflows/src/loader.ts, packages/workflows/src/loader.test.ts	Schemas expose accepted-key metadata, nested-key metadata, and optional parse-warning fields. The loader and its tests cover unknown keys, nested scopes, collisions, included workflows, and free-form fields.
Discovery warning propagation packages/workflows/src/workflow-discovery.ts	Discovery keeps warnings with workflow definitions, sources, and expanded results across directory, bundled, global, and project loading.
Execution and validation warning surfaces packages/cli/src/commands/*, packages/core/src/handlers/*, packages/core/src/orchestrator/*, packages/core/src/types/index.ts	Validation converts parse warnings into issues. CLI output uses JSON, human output, and stderr. Core handlers and orchestrator paths preserve warnings for the resolved workflow and emit them before execution.
API, web, and documentation surfaces packages/server/src/routes/*, packages/web/src/experiments/console/*, packages/docs-web/src/content/docs/*, CLAUDE.md	The API schema and response preserve warnings. The web workflow model and picker display them. The docs describe the warning behavior.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

• coleam00/Archon#2255: Both changes touch workflow parsing, validation, discovery, and warning propagation.
• coleam00/Archon#2129: Both changes modify packages/workflows/src/loader.ts and workflow parsing behavior.
• coleam00/Archon#1315: Both changes adjust workflow discovery and WorkflowSource-related plumbing.

Suggested labels: bug, area: workflows

Sequence Diagram(s)

sequenceDiagram
  participant WorkflowFile
  participant WorkflowLoader
  participant WorkflowDiscovery
  participant WorkflowRunner
  participant stderr
  WorkflowFile->>WorkflowLoader: parse YAML
  WorkflowLoader-->>WorkflowDiscovery: workflow and parseWarnings
  WorkflowDiscovery-->>WorkflowRunner: selected workflow entry
  WorkflowRunner->>stderr: emit selected workflow warnings
  WorkflowRunner->>WorkflowRunner: validate and execute workflow

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR addresses #2213 by warning on misplaced keys, including interactive, while preserving workflow loading and execution.
Out of Scope Changes check	✅ Passed	The changes remain within the stated objective and add only related detection, propagation, documentation, UI, API, and regression coverage.
Docstring Coverage	✅ Passed	Docstring coverage is 84.62% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title clearly summarizes the main changes: surfacing unknown-key warnings and correcting the interactive hint.
Description check	✅ Passed	The description covers the required template sections with detailed scope, UX, architecture, validation, security, compatibility, risks, and rollback information.

✨ Finishing Touches 💡 1

⚔️ Resolve merge conflicts 💡

• Resolve merge conflict in branch fix/2213-surface-unknown-key-warnings

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/2213-surface-unknown-key-warnings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

packages/workflows/src/loader.ts (1)

213-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the required structured event-name format.

node_unknown_key_ignored and workflow_unknown_key_ignored do not use the required {domain}.{action}_{state} format. Use names such as workflow.node_unknown_key_ignored and workflow.workflow_unknown_key_ignored.

As per coding guidelines, use Pino structured logging with {domain}.{action}_{state} event names.

🤖 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 213 - 218, Update the event
names passed to pushUnknownKeyWarning in packages/workflows/src/loader.ts at
lines 213-218 and 912-918 to use the required workflow domain prefix: change
node_unknown_key_ignored to workflow.node_unknown_key_ignored and
workflow_unknown_key_ignored to workflow.workflow_unknown_key_ignored,
preserving the surrounding warning behavior.

Source: Coding guidelines

🤖 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/docs-web/src/content/docs/guides/authoring-workflows.md`:
- Line 1093: Update the warning-output fenced code block near the affected
documentation content to specify the text language, changing the fence to use
```text while preserving the warning content unchanged.

In `@packages/web/src/experiments/console/components/WorkflowPicker.tsx`:
- Around line 341-349: Update the warning indicator span in WorkflowPicker’s
parseWarnings rendering so its warning details use a supported accessible naming
mechanism: either assign it role="img" while retaining the aria-label, or add a
visually hidden description and reference it via aria-describedby. Preserve the
existing warning text and tooltip behavior.

In `@packages/workflows/src/schemas/dag-node.ts`:
- Around line 607-649: Expose per-mode allowed-key sets alongside
KNOWN_DAG_NODE_KEYS in dag-node.ts, covering each node mode’s valid fields. In
loader.ts, after resolving the node mode, compare raw node keys against that
mode’s allowed set and warn for misplaced keys such as with, input, isolation,
runtime, deps, or timeout while retaining unknown-key warnings. Add
loader.test.ts regression cases covering with on a command node and another
mode-specific key used on an unrelated mode.

---

Nitpick comments:
In `@packages/workflows/src/loader.ts`:
- Around line 213-218: Update the event names passed to pushUnknownKeyWarning in
packages/workflows/src/loader.ts at lines 213-218 and 912-918 to use the
required workflow domain prefix: change node_unknown_key_ignored to
workflow.node_unknown_key_ignored and workflow_unknown_key_ignored to
workflow.workflow_unknown_key_ignored, preserving the surrounding warning
behavior.

🪄 Autofix

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: 4147351c-6b9d-43e5-8a2f-91b8152c7c2b

📥 Commits

Reviewing files that changed from the base of the PR and between 8704a65 and 8102527.

⛔ Files ignored due to path filters (1)

• packages/web/src/lib/api.generated.d.ts is excluded by !**/*.generated.*

📒 Files selected for processing (19)

• packages/cli/src/commands/validate.ts
• packages/cli/src/commands/workflow.test.ts
• packages/cli/src/commands/workflow.ts
• packages/core/src/handlers/command-handler.test.ts
• packages/core/src/handlers/command-handler.ts
• packages/docs-web/src/content/docs/guides/authoring-workflows.md
• packages/server/src/routes/api.ts
• packages/server/src/routes/api.workflows.test.ts
• packages/server/src/routes/schemas/workflow.schemas.ts
• packages/web/src/experiments/console/components/WorkflowPicker.tsx
• packages/web/src/experiments/console/lib/recommended.test.ts
• packages/web/src/experiments/console/primitives/workflow.ts
• packages/workflows/src/loader.test.ts
• packages/workflows/src/loader.ts
• packages/workflows/src/schemas/dag-node.ts
• packages/workflows/src/schemas/index.ts
• packages/workflows/src/schemas/workflow.ts
• packages/workflows/src/test-utils.ts
• packages/workflows/src/workflow-discovery.ts

[Wirasm]

---

pr: 2455
title: "fix(workflows): surface unknown-key warnings where authors are, and correct the interactive hint"
author: "Wirasm"
reviewed: 2026-08-05
recommendation: request-changes

PR Review: #2455 — surface unknown-key warnings where authors are

Branch: fix/2213-surface-unknown-key-warnings → dev (draft)
Files Changed: 20 (+1011/−68) · Head: 81025271
Review: 7 specialist agents + independent live-build verification

---

Summary

The feature works. Every headline claim in the PR description was verified empirically on a live build, and the engine mechanism is sound. The residue is not in the logic — it is in coverage of the run path, two narrow type-enforcement gaps, and a test that does not exercise the claim it backs.

One finding materially qualifies the PR's central thesis: warnings reach the surfaces where authors browse workflows, and the CLI surface where they run them — but not the chat/console path where a run is actually started. For a product framed as "driven remotely from Slack, Telegram, GitHub, Discord, the web UI, or the CLI," that is plausibly the majority path.

Nothing is a regression. Every gap below is "still as silent as before," never "newly broken."

---

Issues Found

Important

I1 — parseWarnings never reach the chat/console RUN path (packages/core/src/handlers/command-handler.ts:956)

Independently verified on three legs:

• command-handler.ts:920 is case 'run':; line 956 does const workflows = workflowEntries.map(ws => ws.workflow), discarding parseWarnings. The PR's only edit to this file is the /workflow list renderer.
• packages/core/src/orchestrator/orchestrator-agent.ts is untouched by the diff (0 hits). dispatchOrchestratorWorkflow takes a bare WorkflowDefinition; the natural-language auto-select path has resolvedEntry.parseWarnings in hand and never reads it.
• packages/server/src/routes/api.ts:3114 — the console's Start button synthesizes `/workflow run ${name} ${message}` and lands in that same discard path.

Failure scenario: a user in Slack, or clicking Start in the console, runs a workflow with interactive: true misplaced on a command node — the exact #2213 bug. The run proceeds unattended past the intended gate with nothing in the conversation. The console is the sharp case: it shows a ⚠ badge while browsing the picker, then goes silent at the moment of consequence.

Compounding: the PR's new doc section enumerates coverage as a complete-sounding list (validate, list, before a CLI run, chat /workflow list, console picker) that omits "when a workflow is actually run in chat/console" without flagging it as known.

Fix shape: WorkflowDispatchOptions (orchestrator-agent.ts:609) has room for parseWarnings?: readonly string[]; thread from the 3 fresh-dispatch call sites (1238 is a resume — skip) to a platform.sendMessage mirroring the CLI's emitParseWarnings. In command-handler.ts, keep the resolved WorkflowWithSource through resolveWorkflowName in the run case.

Reported as Critical by the silent-failure reviewer; downgraded to Important here because it is not a regression, crash, or data-loss path — pre-PR behavior was identically silent. It is nonetheless the top item, because it is the one finding that contradicts the PR's own stated goal.

---

I2 — Two spots where "no hand-maintained list to drift" is false (packages/workflows/src/schemas/dag-node.ts:702-704, :682; schemas/workflow.ts:209)

The risk section claims every key set derives from .shape so drift is structurally impossible. Verified true for ~90% of the surface. Two exceptions, both confirmed against live tsc:

• loop_group entry: new Set([...Object.keys(loopControlSchema.shape), 'nodes']). The loopControlSchema half is genuinely derived; 'nodes' is a bare literal with nothing forcing agreement with LoopGroupNodeConfig. Exact today (7+1=8 keys, verified), but a future field added to that .extend() compiles, runs, and silently false-positive-warns on a legitimate key.
  Fix (one line, verified): the z.ZodType<…> annotation hides .shape only at the type level; the runtime value is still the ZodObject. Cast back to recover it:

  const loopGroupShape = (loopGroupNodeConfigSchema as unknown as z.ZodObject<z.ZodRawShape>).shape;

• Registry Map key type: both KNOWN_NODE_NESTED_KEYS and KNOWN_WORKFLOW_NESTED_KEYS are ReadonlyMap<string, NestedKeySpec>. A typo'd registration ('aproval') compiles cleanly today — confirmed by a real tsc run against the package tsconfig — silently disabling that nested check forever, indistinguishable from "no spec needed."
  Fix (verified): type the key as keyof typeof dagNodeFlatSchema.shape (resp. workflowDefinitionSchema.shape). tsc then rejects the typo with a precise did-you-mean.

Either fix both, or downgrade the risk-section wording to name these two as the actual residual surface rather than asserting blanket derivation.

---

I3 — The test backing the headline --json claim does not exercise the claim (packages/cli/src/commands/workflow.test.ts:723)

The PR states "stdout is byte-identical — there is a test asserting it does not [leak]." That test only asserts spyOn(console, 'log') was not called with warning text. But the real --json payload is written via writeJsonLine() (packages/cli/src/utils/stdout.ts:50) — a different channel — and is only reachable on the --detach branch (workflow.ts:1069), which the test does not use. It never reaches the code that produces the JSON.

Confirmed from two independent directions: the test reviewer found it by reading; I found it by running the CLI, where run --json without --detach printed prose and I had to add --detach to get a payload at all.

What the test does prove (that emitParseWarnings uses console.warn, not console.log) is real but much narrower.

Fix: add a test in the --detach + --json context using spyOnJsonStdout(), asserting JSON.parse of captured stdout succeeds and equals the expected envelope while warnSpy fired.

---

Suggestions

S1 — include: warnings never reach the includer. Reproduced: a block with interactive: true on a node, included by another workflow → the target carries the warning, the includer carries none, so running the includer surfaces nothing on any of the five surfaces. Disclosed in the PR body's Side Effects section, but absent from the docs (the "Detection covers…" list omits include entirely) and untested — not even pinned. Fix direction is cheaper than the PR assumed: workflow-discovery.ts already contains resolveIncludeBlockCommandContents's visit(), which walks the include graph transitively for another purpose. (Criticality 5/10)

S2 — No automated regression for the corpus guardrail. The PR names "exactly 2 unknown_key warnings on the real corpus" as its own guardrail, but it exists only in the description. I re-ran it: 51 workflows (not 54 as stated), exactly 2 unknown_key warnings, both true positives in e2e-opencode-smoke.yaml. The nested walk added zero noise — the substance holds; the count in the PR body is wrong. Pin it as a test. (Criticality 7/10 — the highest-rated test gap)

S3 — unknown_key issues are thinner than sibling warnings on the same JSON surface. In validate --json, unknown_key issues carry neither nodeId nor hint, while sibling classes populate both (the bash warning has nodeId: 'fetch-issue' and a separate hint). Node attribution and the hint are fused into message prose, so machine consumers can't attribute or re-render them. Notable in a PR about machine-readable surfaces.

S4 — Console layer has zero test coverage. primitives/workflow.ts:32 (toWorkflow()'s normalization) and WorkflowPicker.tsx:341-349 (the ⚠ marker + aria-label) are untested. Every sibling primitives/*.ts has a co-located test; workflow.ts does not. RunStream.test.tsx proves component tests are the established pattern here. (Criticality 6/10)

S5 — The corrected hint is still imprecise. It ends "Workflow-level interactive: … forces foreground execution" — true only on web. The sole dispatch consumer is orchestrator-agent.ts:896, gated on platform.getPlatformType() === 'web'; Slack/Telegram/GitHub/CLI never read it. CLAUDE.md itself says "on web." A Slack or CLI author could believe the setting does something for them. Worth fixing while the text is being touched anyway — and it is now baked into the docs page too.

S6 — thinking: missing from the docs exemption list. The new guide section lists 3 of the 4 exempt fields and reads as exhaustive; thinking: is absent. It is also the only exempt field with no test (output_format has one). Found independently by three reviewers.

S7 — Documentation gaps.

• packages/docs-web/src/content/docs/reference/api.md:~208 — documented GET /api/workflows response omits new parseWarnings and pre-existing recommended.
• packages/docs-web/src/content/docs/reference/cli.md:187 — parseWarnings missing from the omitted-when-unset optional-fields list.
• reference/cli.md:~201 — workflow run section is silent about the new pre-run stderr notice, which matters to --json callers.
• CLAUDE.md:918 — same stale response shape.
• The guide's example uses the WARNING [unknown_key] prefix, which is specific to archon validate workflows; the other four surfaces format differently. Worth captioning.

S8 — Simplification: collapse the parallel allParseWarnings map (workflow-discovery.ts:326-339). workflowsByFile.set() already fully overwrites per filename, so attaching parseWarnings directly to the value makes the stale-clear behavior fall out for free — deleting the second map, mergeWarnings(), and its 4 call sites (~20 lines, near-zero risk). This is why that file needed +42 lines to thread one field.

S9 — Structured-log hygiene. pushUnknownKeyWarning's Pino call uses field node: carrying prose ("Node 'body' → loop_group node 'inner'", and "Workflow 'test'" for workflow-level warnings) rather than an id, and the warning field duplicates the full message. Against CLAUDE.md's logging conventions; cosmetic.

---

Validation Results

Check	Status	Details
Type Check	PASS	0 errors, all 11 packages
Lint	PASS	clean at --max-warnings 0
Format	PASS	all files match Prettier
Tests (changed files)	PASS	189 + 208 + 124 + 46 = 567 pass, 0 fail
Corpus no-false-positive	PASS	51 workflows → exactly 2 unknown_key, both true positives

Live-build behavior verified by me (scratch repo, isolated ARCHON_HOME):

Surface	Result
Corrected interactive hint	Accurate, matches runtime
loop_group body node, one level down	Detected, breadcrumb Node 'body' → loop_group node 'inner'
Nested config key (retry.backoff_secondz)	Detected with dotted path
run --json --detach	stdout pure JSON, warning on stderr
run (plain) and --quiet	Warning surfaces in both (broader than PR claims)
list (human)	Warnings inline
list --json	parseWarnings on exactly the 2 affected; key omitted on the other 21

---

Claims Corrected

PR claim	Reality
"54 workflows"	51. The "exactly 2 true positives" part is exact.
"Tests fail before the fix — 8 fail"	Compares two commits within the PR (5286dfa6→433c2718), not PR-vs-dev; at base 8704a65f no warnings field exists at all. 9 new it() blocks, not 7; 2 pass trivially at the earlier commit. Defensible, loosely framed.
"run --json stdout stays exactly the JSON payload"	Only with --detach. Without it, --json prints prose. Pre-existing (CLAUDE.md's --json list correctly excludes run) — a description fix, not a code fix.
"warnings now reach every surface an author actually uses"	Not the chat/console run path — see I1.

---

Verified Sound (no action)

• Scope boundary genuinely held — every new path pushes only to warnings/parseWarnings, never errors. Posture stays warn-not-reject.
• Malformed input structurally unreachable — the collectors run only after dagNodeSchema.safeParse() succeeds, so bad shapes fail the outer parse as errors. Internal guards are unreachable defense-in-depth. Confirmed by three reviewers.
• Recursion bounded — max depth 2 today (approval.on_reject).
• Run-path scoping correct and tested — a dedicated test plants warnings on a different workflow and asserts non-leakage.
• mock.module() merge trap does not fire — workflow-discovery.ts changed but added no exports; the 3 orchestrator factories that mock it stay safe. Confirmed independently twice.
• Optional-vs-empty-array is consistent across all 5 layers, using the same length > 0 guard as the existing errors field on the same route.
• Both "mitigation comments" are accurate, not fiction — 23+ object-shaped fields enumerated; each exemption justification independently verified (sandbox really is .passthrough(), hooks really is .strict(), thinking really is a preprocessed union).
• No shared formatter wanted — 7 renderings with genuinely different output contracts (stderr, JSON, structured issue object, markdown, DOM). The duplicable part (message + hint text) is already centralized in unknownNodeKeyHint.
• Nested detection earns its place — it is where the agents.<id>.disallowed_tools camelCase-slip case lives, a silent tool-restriction drop.
• No secret-leak risk — log lines carry key names only, never values.

---

Strengths

• The dagNodeFlatSchema extraction is structurally necessary, not polish: .transform() strips .shape, so a pre-refine flat schema is required to introspect. Correctly identified and handled.
• Surfaced an undocumented but load-bearing fact: in zod 4.4.3 .superRefine() preserves .shape (unlike v3), which is why loopControlSchema.shape works without extraction. Worth a comment.
• The interactive hint fix is real: the old text claimed loop.gate_message alone gates, which is false — the executor requires both. Verified against loopControlSchema.superRefine.
• Test quality is genuinely strong where it exists: explicit negative cases, cross-layer non-leakage assertions, and both empty-and-absent shapes asserted at the CLI and server layers.
• The no-false-positive discipline is real — the nested walk added zero noise to a 51-workflow corpus.

---

Recommendation

REQUEST CHANGES — though as a draft PR authored by the maintainer, this is a pre-merge fix list rather than a gate.

Before merge:

1. I1 — either wire warnings into the chat/console run path, or explicitly document it as a known limitation in the new doc section (which currently reads as exhaustive). The one thing not to do is leave a complete-sounding list that omits it.
2. I2 — both fixes are one-liners and verified; they make the risk section's claim true instead of aspirational.
3. I3 — the headline claim deserves a test that reaches the path.
4. Correct the PR body: 51 not 54, and the run --json / --detach distinction.

Cheap wins worth folding in: S5 (hint "on web"), S6 (thinking: in docs + a test), S7 (reference docs), S8 (~20-line simplification).

Reasonable to defer with an issue: S1 (include: — but at minimum pin current behavior in a test and mention it in the docs), S2, S3, S4.

---

Reviewed by Claude — 7 specialist agents plus independent live-build verification
Report: /Users/rasmus/.prp/archon-75601ef6/reviews/pr-2455-review.md

[Wirasm]

Follow-up: one more Important finding — reproduced

Post-review pass turned up a real bug that the first comment missed. Reproduced empirically, and it is introduced by this PR (not pre-existing).

I4 — Parse warnings misattribute across a root/subfolder filename collision

loadWorkflowsFromDir (packages/workflows/src/workflow-discovery.ts:98-164) keys both maps by bare filename, and the two merge sites are asymmetric:

Line	Operation	Behavior
119 / 130	workflows.set(...)	unconditional overwrite
122	parseWarnings.set(...)	sets, never clears
131-133	parseWarnings.set(...)	only if (result.warnings.length > 0) — never clears

So when a filename appears both at the root and in a 1-level subfolder (a documented, supported layout — "Subfolders: supported 1 level deep"), the workflow content is last-writer-wins while the warning entry is sticky. The two can end up describing different files.

Reproduced on a scratch repo at 81025271:

.archon/workflows/foo.yaml        → name: foo-root, CLEAN (no `interactive` anywhere)
.archon/workflows/zsub/foo.yaml   → name: foo-sub,  DIRTY (`interactive: true` on node `a`)

readdir() enumerated ['zsub', 'foo.yaml'], so the subfolder was merged first and the root file overwrote it. Result:

surviving workflow : foo-root   (grep -c interactive → 0)
warnings attached  : 1
  - Node 'a': unknown key 'interactive' will be ignored. Nothing on this node gates. …

A clean workflow is told it declares a key it does not contain. Symmetrically, in the opposite enumeration order a genuinely dirty file's warning is dropped when a clean same-named file shadows it.

Why it matters beyond the narrow repro: readdir() order is unspecified, so which way this falls is filesystem- and layout-dependent. That makes it a flaky, hard-to-attribute user report rather than a clean repro — and the failure mode is a false warning, which is precisely what the PR's own guardrail ("if a future schema change makes a legitimate key look unknown, that count moves off 2") is meant to catch. This variant is data-dependent rather than schema-dependent, so that guardrail would not catch it.

Attribution: parseWarnings does not exist at base — git show 8704a65f:…/workflow-discovery.ts | grep -c parseWarnings → 0. Both merge sites are + lines in this diff. The collision ambiguity for workflow content is pre-existing (last-wins by bare filename, its own latent issue); what is new is that warnings can now disagree with the workflow that survived.

Fix: the correct pattern already exists in this same file, added by this same PR — the top-level mergeWarnings (~line 330) deliberately clears a filename's stale warning when a higher-precedence scope overrides it cleanly. The inner sites just never got it. Either give lines 118-123 and 129-133 the same clear-on-clean-override logic, or centralize both under one helper so the two can't drift again.

Note this interacts with the simplification suggested as S8 in the main review (collapsing the parallel allParseWarnings map): attaching parseWarnings directly to the workflowsByFile value makes stale-clearing fall out of plain Map.set() for free, which would fix this class of bug structurally rather than by adding a second hand-written clearing loop. Worth doing S8 and I4 together as one change.

Corrections to the earlier comment

Two items in the first comment can be tightened now that they were chased further:

• include: is not worse than documented. expandIncludes() preserves the includer's own warnings correctly (keyed by its real filename); only the included block's warnings fail to propagate. No compounding failure — S1 stands exactly as written.
• The event names are fine. node_unknown_key_ignored / workflow_unknown_key_ignored are flat snake_case without a dotted domain prefix, but that matches siblings already in loader.ts (workflow_missing_name, workflow_missing_description). Not a deviation introduced here, so disregard that half of S9 — the node: field carrying prose instead of a stable id is the only part still worth a one-liner.

[coderabbitai]

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/core/src/orchestrator/orchestrator-agent.ts`:
- Line 2802: Update the options construction in handleWorkflowRunCommand to use
resolvedEntry?.parseWarnings as the authoritative value, rather than preferring
options.parseWarnings, so warnings match the workflow that will execute.
Preserve the fallback behavior only when the resolved entry has no warnings, and
add a regression test covering same-name global and project workflows.

In `@packages/docs-web/src/content/docs/reference/api.md`:
- Around line 209-213: Update the parseWarnings descriptions to state that they
contain warning messages identifying ignored keys and their node context, rather
than raw key names. Apply this wording in
packages/docs-web/src/content/docs/reference/api.md lines 209-213, CLAUDE.md
line 918, and packages/docs-web/src/content/docs/reference/cli.md line 187;
preserve the existing omission behavior for clean workflows.

🪄 Autofix

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: 5c6ad3bf-5b5c-43bb-825c-57082a9bbf4c

📥 Commits

Reviewing files that changed from the base of the PR and between 8102527 and 3fcf80d.

📒 Files selected for processing (16)

• CLAUDE.md
• packages/cli/src/commands/workflow.test.ts
• packages/core/src/handlers/command-handler.test.ts
• packages/core/src/handlers/command-handler.ts
• packages/core/src/orchestrator/orchestrator-agent.test.ts
• packages/core/src/orchestrator/orchestrator-agent.ts
• packages/core/src/types/index.ts
• packages/docs-web/src/content/docs/guides/authoring-workflows.md
• packages/docs-web/src/content/docs/reference/api.md
• packages/docs-web/src/content/docs/reference/cli.md
• packages/web/src/experiments/console/primitives/workflow.test.ts
• packages/workflows/src/loader.test.ts
• packages/workflows/src/loader.ts
• packages/workflows/src/schemas/dag-node.ts
• packages/workflows/src/schemas/workflow.ts
• packages/workflows/src/workflow-discovery.ts

🚧 Files skipped from review as they are similar to previous changes (6)

• packages/workflows/src/schemas/workflow.ts
• packages/docs-web/src/content/docs/guides/authoring-workflows.md
• packages/cli/src/commands/workflow.test.ts
• packages/workflows/src/loader.ts
• packages/workflows/src/schemas/dag-node.ts
• packages/workflows/src/workflow-discovery.ts

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[Wirasm]

What changed since the first review

The PR has grown from +1011/−68 in 20 files to +1608/−106 in 27 — enough that it is a different PR from the one reviewed, so this is a map of where to look rather than an invitation to re-read from the top.

Head at first review: 81025271. Everything below is on top of that, plus two rebases (onto #2223, then #2224).

The seven commits

Commit	Addresses	Where to look
e2a41bff	I4 + S8 as one change	workflow-discovery.ts — the interesting one
2d653718	I1 — chat/console run path	orchestrator-agent.ts, command-handler.ts
ab1713a6	I2, S5, S9-residual, S2	schemas/dag-node.ts, schemas/workflow.ts, loader.ts
54426aa9	I3, S4, S6, S1-pin, S7 docs	tests + docs-web/, CLAUDE.md
c4deb0d8	CodeRabbit round	orchestrator-agent.ts, WorkflowPicker.tsx, docs
b39e3524 + 22a66361	corpus-test cost, fan_out fold	loader.test.ts, schemas/dag-node.ts
510f87e4	TDZ hazard comment	schemas/dag-node.ts

Four things worth a reviewer's attention

1. I4 and S8 were done as one change, per the follow-up's suggestion. Warnings now live on the discovery entry rather than a map beside it, so a single Map.set() replaces the definition and its warnings together. That removes the bug class structurally and deletes mergeWarnings plus its four call sites. The two regression tests assert the order-independent invariant — whichever side of a root/subfolder filename collision wins, the surviving warnings describe the surviving workflow — so they do not depend on readdir() enumeration order. Verified failing without the fix.

2. I1 was wired, not documented-around. Threaded to dispatchOrchestratorWorkflow, the choke point every chat and console run funnels through, so one emit covers all of them. handleStreamMode / handleBatchMode / handleWorkflowInvocationResult now take the WorkflowWithSource[] discovery already produces instead of a pre-mapped definition list — one list, not a definition list plus a parallel lookup. Resume is excluded (the warning fired when the run began); delivery is best-effort so a failed send cannot stop the run. Verified live: /workflow run gated in a real conversation now posts the warning before the run starts.

3. One correction to the review — I2's suggested one-liner compiles and then dies at import. Casting loopGroupNodeConfigSchema back to recover .shape is right in substance, but reading that shape fires the schema's nodes getter, which builds z.array(dagNodeSchema). Above dagNodeSchema's declaration that is a temporal dead zone: ReferenceError: Cannot access 'dagNodeSchema' before initialization at import, taking all 226 tests in loader.test.ts from pass to a single load failure. tsc exits 0 either way, which is why a compile-only check does not see it. The registry now sits at the end of the file, and 510f87e4 documents the hazard at the cast itself — moving it up beside the type it belongs with is the natural tidy, and nothing else would stop it.

4. CodeRabbit found a real attribution bug in the I1 work above. The single-project auto-select branch re-resolves the workflow against that project's discovery, so the entry it lands on can differ from the one the caller resolved — a project workflow shadowing a same-named global one. Preferring the caller's warnings there described a workflow that was not running. Fixed to prefer the re-resolved entry, with a regression test asserting the stale set is not forwarded.

Two deferrals, with reasons

S3 — unknown_key issues carry neither nodeId nor hint in validate --json. Making them structural means changing parseWarnings from string[] to an object array across the engine, the WorkflowWithSource contract, the OpenAPI response schema, the regenerated web types and all seven renderers — several times the size of this entire round, for a Suggestion. It would also push message composition into each human surface, which is a quality regression risk in the prose. The information is present today, just not separately addressable.

S4 — half done. toWorkflow now has the co-located test every sibling primitive has. The WorkflowPicker DOM is not unit-tested: this package has no DOM-rendering harness (RunStream.test.tsx, cited as the pattern, tests an exported pure function). The marker's condition is a direct parseWarnings.length > 0, verified in a live browser with a screenshot.

S1 — as scoped: pinned by a test and stated in the authoring guide; propagation across the include boundary deferred.

Also declined, from CodeRabbit: warning when a mode-specific key appears on the wrong node type (with: on a command node, timeout: on a prompt node). A real gap, but it needs per-mode key sets and must be reconciled with the existing *_node_ai_fields_ignored warnings that cover the mirror-image case, or the two double-report. That is its own change, not a review fix.

Rebase notes

The predicted dag-node.ts conflict with the fan-out branch landed and resolved as expected: fan_out: moved into dagNodeFlatSchema, so KNOWN_DAG_NODE_KEYS derives it with nothing hand-listed. It also got a KNOWN_NODE_NESTED_KEYS entry, since fanOutConfigSchema strips unknown keys like every other nested block — fan_out.max_paralel would otherwise vanish silently.

Claims corrected in the description

51 workflows, not 54 — and that guardrail is now pinned as a test rather than living only in prose. The run --json stdout-purity claim now states that it applies to --detach (the only path that emits a payload). The "tests fail before the fix" line now says it compares against the preceding commit within the PR, not against dev.

Validation

bun run validate green — log grepped rather than trusting the wrapper exit code: 132 test batches, every one 0 fail; all generated-file checks, type-check, lint and format pass. All five CI checks green including Windows.

One Windows failure along the way is worth recording even though it is not this PR's: an unrelated SQLite event_order upgrade test that runs 250 ms on dev timed out twice at 5.3–6.0 s against Bun's 5000 ms per-test limit. This branch touches no DB code; the symptom is starvation under bun --filter '*' --parallel test, and that test does real file I/O with comments documenting it as Windows-fragile. The heaviest new test here (the corpus guardrail) now parses files directly rather than running full discovery — a tighter unit for a parser check, and ~3× cheaper — and the SQLite test came back at 1312 ms on the green run. Correlation, not proof.

[kagura-agent]

Thanks for carrying the four #2255 commits forward here. I verified they remain in the current branch; I’ll monitor #2455 for any reviewer feedback rather than duplicating the work.

[Wirasm]

---

pr: 2455
title: "fix(workflows): surface unknown-key warnings where authors are, and correct the interactive hint"
author: "Wirasm"
reviewed: 2026-08-05
recommendation: request-changes
scope: "delta only — f5280d0..22a6636 (7 commits authored after the round-2 review)"

PR Review round 3: #2455 — delta since the last review

Scope: f5280d05..22a66361 — the 7 commits authored after the round-2 comment (08:23:17Z). 17 files, +716/−156. Everything at or before f5280d05 was covered in round 2 and is not re-reviewed.
Agents: 6 specialists (code, types, errors, tests, docs, comments). simplify deliberately skipped — a fix round, not a polish round.
CI: green on 22a66361 and on current head 510f87e4 (Test Suite + Docs Build, incl. Windows).

---

Summary

Every fix this delta claims to make is real, complete, and introduces no logic defect. The general code review came back clean at 80+ confidence, type-check passes across 13 packages, and all four affected packages' suites are green (workflows 226/226, core 340/340, cli 209/209, web 39/39).

The residue is entirely in claims that overshoot what the code delivers — one user-facing doc, one interface docblock, one comment's stated reach. That matters more than usual here, because this PR exists to fix exactly that defect class: validation blessing a safety property that did not exist. Three text-level corrections, no logic changes.

One architectural gap deserves an explicit decision rather than a silent pass: parse warnings have no durable home, so a failed chat delivery does not delay the warning — it destroys it.

---

Verification of the round-2 findings

Finding	Status	Evidence
I1 — warnings never reached the chat/console RUN path	Fixed, verified	dispatchOrchestratorWorkflow is the single funnel for AI-routed invocation, /workflow run, and the console's POST /api/workflows/:name/run. The warning is sent before isolation resolution and before the foreground/background branch, so it fires even on web background dispatch.
I2 — key sets could drift from schemas	Fixed, verified empirically	keyof typeof …shape map-key typing rejects a typo'd registration under real tsc (TS2769/TS2820); the old Map<string, …> form compiled it clean. Confirmed by direct probe, not assertion.
I3 — --json test did not reach writeJsonLine	Fixed, verified	workflow.test.ts:3865 uses a real spyOn(process.stdout, 'write') and calls the real workflowRunCommand(…, {detach: true, json: true}); asserts JSON.parse() succeeds with no warning text in the payload. Genuinely closes the stub gap.
I4 — filename-collision misattribution	Fixed, verified	All 6 workflowsByFile.set() sites spread { ...parsed, source }; no site writes workflow and parseWarnings independently. mergeWarnings + 4 call sites fully deleted. The bug class is now structurally unrepresentable, not merely less likely.
CodeRabbit round (shadow re-resolution, role="img", doc wording)	Fixed, verified	orchestrator-agent.ts:2812 overrides with resolvedEntry?.parseWarnings; regression-tested with STALE/FRESH sentinels at orchestrator-agent.test.ts:2334-2404.
#2223/#2224 rebase fold	Fixed, verified	fan_out registered in both dagNodeFlatSchema and KNOWN_NODE_NESTED_KEYS. Closes a real narrow regression: fan_out existed on workflowNodeSchema at base but was missing from the flat schema, so it was silently stripped before superRefine could validate it.

---

Issues Found

Critical

None.

Important

I1 — cli.md promises a machine-readable stdout guarantee that only holds under --detach

packages/docs-web/src/content/docs/reference/cli.md:203:

"This matters to --json callers: --json silences all logging, so stderr is the only channel that carries it, and stdout stays exactly the machine-readable payload."

• Why it's wrong: the only writeJsonLine() call in workflowRunCommand is inside if (options.detach) (packages/cli/src/commands/workflow.ts:1069), which returns at line 1090. Lines 1092–1094 then console.log('Running workflow: …') / console.log('Working directory: …') unconditionally — not gated on options.json. So archon workflow run <name> --json without --detach writes plain prose to stdout and emits no JSON payload at all.
• Failure scenario: an integrator reads this sentence, pipes archon workflow run foo --json into a JSON parser, and gets a parse error on Running workflow: foo.
• Notable: the PR description states this correctly ("the stdout-purity guarantee therefore applies to run --detach --json"). The qualifier just didn't survive into the doc.
• Fix: scope the sentence to --detach --json, or move it into the --detach flag row.

I2 — the interface docblock's resume justification does not hold universally

packages/core/src/orchestrator/orchestrator-agent.ts:618 — "Omitted on resume: the warning fired when the run began."

• The structural half is true: all three resume-dispatch builders (orchestrator-agent.ts:1295, command-handler.ts:786-795, api.ts:3195-3248) leave parseWarnings unset.
• But the delivery mechanism exists only inside dispatchOrchestratorWorkflow, so a CLI-started run never fired a chat warning. Two concrete counterexamples where nothing fired: (a) a run started with archon workflow run, later resumed by typing /workflow resume <id> in chat — command-handler.ts's resume case has no originating-conversation guard, unlike the REST route which returns 400 at api.ts:3211 when !run.parent_conversation_id; (b) any run started before this feature shipped and resumed after upgrade.
• Fix: soften to "delivery happens once, at the run's original chat/console start (if any) — not guaranteed for every run."

I3 — a failed warning delivery destroys the warning rather than delaying it

packages/core/src/orchestrator/orchestrator-agent.ts:756-769 — the best-effort catch around platform.sendMessage.

• parseWarnings has no durable home: confirmed zero references across packages/core/src/db/, workflow_runs, and workflow_events. It exists only as an in-memory field threaded through function arguments (types/index.ts:71, orchestrator-agent.ts:620).
• Failure scenario: a Slack/Telegram API hiccup, revoked bot token, or rate-limit at exactly the moment a workflow with a dropped interactive: true is dispatched. The catch discards the error, the run starts, and the user sees nothing — reproducing bug: workflow validator silently accepts unknown node keys (e.g. interactive: true on a command node) #2213's exact failure mode one layer up. No /workflow status, no console run detail, and no workflow_events row will ever show it. The only trace is a WARN Pino line.
• Fairness: this catch-and-warn shape matches sibling handlers in the same file (error_notification_failed:1821, post_message_reminder_failed:1806). The distinction is that those are terminal — nothing follows them — whereas this one is immediately followed by starting a potentially mutating run.
• Not blocking, but decide explicitly. CLAUDE.md's bar is "document fallback when intentional and safe; otherwise throw." Either persist warnings onto the run before attempting delivery (consistent with treating workflow_events as the audit trail), or — the cheap option — document it as a known gap the way this PR already does for include:, and qualify the unconditional "Posted to the conversation before the run begins" row in the coverage table.

Suggestions

• S1 — keyof typeof …shape typo-protection was not extended one level down. dag-node.ts:1243's children: new Map<string, NestedKeySpec>([['on_reject', …]]) uses plain string, so 'on_rejct' would compile and silently disable that check. The narrow form was verified to work identically at that nesting level. Mitigated by a behavioral test that would fail. Either extend it (one-line) or scope the comment's "fails to compile" claim to top-level registrations.
• S2 — the include: known-gap note (authoring-workflows.md:1541) is accurate and test-pinned, but sits ~700 lines from the include: section (824-941) with no cross-link in either direction. Someone reading about composition won't meet it. One-line pointer under "How expansion works".
• S3 — the TDZ comment's "keep this block last" is stricter than the real constraint, which is "declared after dagNodeSchema." Currently equivalent only because nothing follows it. The precise rule won't needlessly block a future insertion.
• S4 — handleStreamMode/handleBatchMode widen workflows to WorkflowWithSource[] then immediately .map(ws => ws.workflow) two lines later. The reason (downstream needs parseWarnings/source off the matched entry) is only visible in handleWorkflowInvocationResult. One line at the signature would save the next reader the trip.
• S5 — test breadth gaps (criticality in parens). All three new orchestrator-agent.test.ts tests go through only the single-codebase auto-select branch:
  • (7) no negative test that resume does not re-send the warning — a refactor could silently start re-sending with nothing catching it
  • (6) the common path where conversation.codebase_id is already set (orchestrator-agent.ts:2732) is untested
  • (6) the natural-language /invoke-workflow path (:2384) is untested
  • (5) the sendMessage delivery-failure catch is untested — exactly the contract CLAUDE.md wants explicitly protected rather than implied by a comment
  • (5) the filename-collision pair relies on real readdir() order. On macOS only one of the two is a live regression test; the other passes identically against pre-fix code. Order is OS-dependent, so neither is guaranteed to catch the bug on every runner. Force both orderings deterministically.
  • (2) tests assert both sendMessage and dispatch were called, not that the warning came first
• S6 — two of the new before/after tests (should omit parse warnings for a clean workflow, sends no parse-warning message for a clean workflow) pass vacuously against pre-fix code. Inherent to negative-only assertions, not a defect, but they don't back the "fails before the fix" claim.
• S7 (pre-existing) — the comment at handleWorkflowInvocationResult says "supports partial name matching", but findWorkflow is workflows.find(w => w.name === name) — exact match only (router.ts:206-211). The partial-matching description belongs to findCodebaseByName. Predates this delta.

---

Validation

Check	Status	Detail
CI — Test Suite	pass	green on 22a66361 and head 510f87e4, incl. Windows (the prior flake did not recur)
CI — Docs Build	pass	both SHAs
type-check	pass	13 packages, 0 errors, at exactly 22a66361 in an isolated checkout
workflows tests	pass	226/226 new-area; ~1900 full package, 0 fail
core tests	pass	340/340; ~1400 full package, 0 fail
cli tests	pass	209/209; 513 full package, 0 fail
web tests	pass	39/39; 593 full package, 0 fail
mock.module() hazard	clear	delta adds no new runtime export to any mocked module — new fields are type-only additions, and NestedKeySpec/KNOWN_NODE_NESTED_KEYS were relocated, not introduced
Real-resource leakage	clear	ARCHON_HOME-isolated reruns of the new test files produced no archon.db

---

Strengths

• The I4 fix is the strongest work in the delta. Collapsing two parallel maps into one ParsedWorkflowFile converts a shipped desync bug into something the type system makes unrepresentable, and deletes 11 lines of compensating bookkeeping rather than adding a second clearing loop. Type scores: encapsulation 9, invariant expression 9, usefulness 9, enforcement 8.
• Comment accuracy is unusually high. All 8 load-bearing claims verified — several by direct runtime repro against the repo's own zod v4.4.3 install, including confirming that .shape fires getter-backed fields via object-spread (the mechanism the TDZ comment asserts). The interactive: hint correction is accurate and consistent with all seven other doc pages that already described it.
• The corpus-guardrail rewrite tests the same property, not less of it. Independently reproduced by removing denied_tools from dagNodeFlatSchema and confirming the test fails with three new false positives. It is arguably more thorough than the version it replaced (no override-collapsing), while being ~3× cheaper — a real answer to the Windows CI pressure.
• The fan_out fold is not just rebase hygiene; it closes a genuine narrow regression where fan_out: was stripped before its own superRefine could validate it.
• The include: gap is handled exactly right — documented, test-pinned, and named in the commit message. That is the model I3 above should follow.

---

Recommendation

REQUEST CHANGES — narrowly. The code is clean; nothing here needs a logic change.

Blocking on three text edits, because all three are the same over-claiming defect this PR was written to eliminate:

1. Scope the cli.md:203 --json guarantee to --detach.
2. Soften the orchestrator-agent.ts:618 resume docblock.
3. Either persist warnings before delivery, or document the best-effort delivery as a known gap and qualify the coverage-table row (I3).

Everything under Suggestions is fair to defer. Once (1)–(3) land, this is ready.

---

Reviewed by Claude — 6 specialist agents, delta-scoped, findings independently spot-verified.
Report: /Users/rasmus/.prp/archon-75601ef6/reviews/pr-2455-review-3.md

[Wirasm]

Round-3 delta — 159aa602 + 8af2bc9c

Two commits since the round-3 review. All five checks green, MERGEABLE/CLEAN.

The three blockers (159aa602)

I verified each against the code before editing; all three were correct.

#	Fix
I1 cli.md:203	Confirmed the only writeJsonLine sits inside if (options.detach) and the console.log('Running workflow: …') after it is ungated. Scoped the guarantee to --detach --json, and added an explicit sentence that plain run --json emits no payload — so nobody pipes it into a parser.
I2 dispatch docblock	Confirmed the CLI calls emitParseWarnings (stderr) and never reaches dispatchOrchestratorWorkflow. Now says delivery happens at most once, at the original chat/console start if any, and names the CLI-started-then-resumed case rather than implying a warning always fired.
S3 TDZ comment	"keep this block last" → "declared after dagNodeSchema", in both places — the section header at line 1179 carried the same over-claim.

Also folded S1, because it is the same defect rather than a Suggestion: the comment claimed a typo'd registration "fails to compile", which was false for children entries. Extended the keyof typeof …shape typing one level down instead of weakening the claim — 'on_rejct' now errors TS2769 where it previously compiled clean.

I3 — persisted, per Rasmus (8af2bc9c)

workflow_events already stores type + JSONB data, so no schema change: one entry in WORKFLOW_EVENT_TYPES, an emit, and the read paths.

The emit is in the engine, beside workflow_started — not at the chat dispatch site. Two consequences, both deliberate:

• Every run records it, whatever surface started it. A CLI- or REST-started run has no conversation to post into and now still carries its warnings — which closes the hole behind I2's counterexamples, where a run resumed with /workflow resume had never fired a chat warning and never could have.
• The record is written by a path the notification cannot touch. That is stronger than emitting before the send: no delivery outcome reaches it at all.

Read paths: archon workflow get <id> --verbose (human), --verbose --json (a named parseWarnings field rather than making callers scan raw events), and the console timeline.

Console: mapped, not audit-only. An unmapped type does not vanish here — it falls through to the text fallback and renders a raw {"warnings":[…]} dump, which is worse than either option on the table. Mapped as text, deliberately not system: system rows sit behind the System toggle, which is off by default, and a silently dropped interactive: gate is the last thing to put behind an opt-in.

Tested at both layers, each verified failing first — the contract the review named as untested (S5, criticality 5):

• executor writes the event; writes it even when the platform rejects every send; writes nothing for a clean workflow
• orchestrator still hands the warnings to the executor when the warning delivery itself throws

One note on that last test: failing every sendMessage doesn't exercise this path at all — an unrelated, pre-existing sendMessage earlier in the turn throws first and the run never starts. The test fails only the warning delivery, which is also the realistic case (a rate limit or over-length message on one call).

Verified end to end on a real CLI run: the row lands, and workflow get --verbose shows it for a run that never had a conversation.

One claim I introduced and then corrected

The first draft of this said the record was readable via archon workflow get <id>. It is not — the plain form returns the run row without reading the event log; I found this by running it rather than assuming. --verbose is required, and the doc, the code comment and the coverage table all say so now.

Coverage table

Persistence earns the strong version, so the row is no longer hedged:

Surface	Where
Any run that starts	Recorded on the run as workflow_parse_warnings — always
Chat / console (starting a run)	Also posted to the conversation, best-effort

Deferred

Everything else under Suggestions. S5's readdir-ordering gap is real — I saw it directly, 1 of the 2 collision tests fails pre-fix on macOS and the other passes vacuously — but "force both orderings deterministically" has no cheap implementation. The vacuous direction is inherent: when the dirty file wins, pre-fix and post-fix produce identical output, so no assertion can distinguish them. Forcing the order needs either a test seam in loadWorkflowsFromDir or mock.module('fs/promises'), which CLAUDE.md warns against and which would break the many real-I/O tests in that same file. Flagged for routing rather than half-done.

S7 is pre-existing and unrelated to this PR.

[Wirasm]

Follow-up filed so the one deferral here is tracked:

• feat(workflows): warn when a mode-exclusive key lands on the wrong node mode (with: on a command: node is dropped in silence) #2478 — CodeRabbit's Major finding at dag-node.ts:708, verified against the branch rather than taken on its word. This PR covers one direction thoroughly (AI fields on non-AI nodes, via the per-mode ignored-field lists at loader.ts:297-311) and leaves the inverse open: KNOWN_DAG_NODE_KEYS is the whole flat schema shape, so a mode-exclusive key on an AI node is accepted, dropped by the mode transform, and never mentioned. with: on a command: node is the live example, and feat: add load-time inputs for include nodes #2467 is about to make it reachable.

The other four CodeRabbit comments are addressed on ca1c08c1: the wrong-warnings bug at orchestrator-agent.ts now reads resolvedEntry?.parseWarnings (2840), the api.md description says "warning messages identifying the keys" rather than "lists keys", and the output fence carries text.

S5 stays as-is per your call.