feat(conductor): god registry, webhook DAG node, n8n importer#2028
Open
Duskript wants to merge 9 commits into
Open
feat(conductor): god registry, webhook DAG node, n8n importer#2028Duskript wants to merge 9 commits into
Duskript wants to merge 9 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds GodDefinition interface (id, displayName, description, workflows?) to GlobalConfig, RepoConfig, and MergedConfig. Config-loader merges god arrays so repo gods override global gods by id, remainder appended. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… prompts Adds formatGodsSection(gods) to prompt-builder that renders a ## Known Specialists section listing each god's displayName, description, and dispatch workflows. Updates buildOrchestratorPrompt(), buildProjectScopedPrompt(), and buildOrchestratorSystemAppend() to accept an optional gods parameter (default []). Orchestrator passes config.gods through. 24 tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds the Conductor persona system prompt as a bundled default command. Covers: Conductor voice, answer-vs-dispatch decision doctrine, the ledger shape metaphor, and specialist dispatch rules. Regenerates bundled-defaults. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds webhookNodeConfigSchema (message?, timeout?) and webhookNodeSchema to dag-node.ts. WebhookNode pauses workflow execution until an external HTTP POST delivers a trigger payload. Updated DagNode union, superRefine mutual- exclusivity validation, and transform to handle the new type. isWebhookNode() type guard added; isPersistableNode() excludes webhook nodes. 8 new schema tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add 'webhook' to WorkflowNodeType in @archon/paths telemetry.ts - Extend ApprovalContext.type to include 'webhook' in workflow-run.ts - Add webhook_registered and webhook_triggered to WORKFLOW_EVENT_TYPES - Add executeWebhookNode() in dag-executor: pauses run with type 'webhook', sends trigger URL to platform, emits approval_pending for UI compat - Add POST /webhooks/workflow/:runId/:nodeId in api.ts (outside /api/* to bypass web auth gate — external callers like Zapier/n8n can't authenticate): validates run exists and is paused with type 'webhook', writes node_completed event with caller payload, writes webhook_triggered event, auto-resumes run Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add packages/workflows/src/n8n-converter.ts with convertN8nToArchon() pure function (no I/O, never throws on unknown node types) - Maps n8n-nodes-base.OpenAiChat → prompt:, Code(JS) → bash:, Code(Python) → script:/uv, HttpRequest → bash:/curl, Wait → approval: - Any unknown type produces a bash stub with TODO comment + warning entry - Translates n8n connections to depends_on arrays; deduplicates colliding kebab-case IDs using n8n UUID as primary key - 12-test suite covering: node count, name→kebab, depends_on edges, type mappings, unknown type warning+stub, deduplication, non-object input Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CLI:
- Add workflowImportN8nCommand() to packages/cli/src/commands/workflow.ts
reads n8n JSON file, calls convertN8nToArchon(), serializes with
Bun.YAML.stringify(), writes to <repoRoot>/.archon/workflows/<name>.yaml
- Wire as 'archon workflow import n8n <file> [--out <name>] [--cwd <path>]'
in packages/cli/src/cli.ts; --out option added to parseArgs config
- Error exits on missing file or invalid JSON; warnings printed to stderr
REST:
- Add POST /api/workflows/import/n8n to packages/server/src/routes/api.ts
(registered before GET /api/workflows/:name to avoid static/param clash)
- Registered via registerOpenApiRoute with Zod body schema importN8nBodySchema
- Returns { yaml, workflowName, warnings }; 400 on conversion error
- Lazy import of convertN8nToArchon keeps server startup cost neutral
- New schemas: importN8nBodySchema, importN8nResponseSchema in workflow.schemas.ts
- Export n8n-converter subpath added to @archon/workflows package.json exports
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…field configs The mock config in orchestrator.test.ts (and real configs without a 'gods' block) omit the 'gods' key entirely, making config.gods === undefined. Pass ?? [] to buildOrchestratorSystemAppend so it always receives an array. Also update the two orchestrator.test.ts assertions that check buildOrchestratorSystemAppend call signature to expect a 4th any(Array) argument (added in US-002 formatGodsSection integration). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds three extensions to Archon under a "Conductor" umbrella: a ChangesConductor Archon Extensions
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 149, 237, 0.5)
Note over ExternalCaller,tryAutoResumeAfterGate: Webhook node pause/resume lifecycle
end
participant DAGExecutor
participant executeWebhookNode
participant WorkflowStore
participant ExternalCaller
participant WebhookEndpoint as POST /webhooks/workflow/:runId/:nodeId
participant tryAutoResumeAfterGate
DAGExecutor->>executeWebhookNode: dispatch on isWebhookNode(node)
executeWebhookNode->>WorkflowStore: addWorkflowEvent(webhook_registered)
executeWebhookNode->>WorkflowStore: pauseWorkflowRun(type: webhook)
executeWebhookNode-->>DAGExecutor: placeholder completed output
ExternalCaller->>WebhookEndpoint: POST payload JSON
WebhookEndpoint->>WorkflowStore: validate run paused + gate type=webhook + nodeId
WebhookEndpoint->>WorkflowStore: addWorkflowEvent(node_completed)
WebhookEndpoint->>WorkflowStore: addWorkflowEvent(webhook_triggered, payload)
WebhookEndpoint->>WorkflowStore: updateRun status=failed
WebhookEndpoint->>tryAutoResumeAfterGate: tryAutoResumeAfterGate(run, approve, userId)
tryAutoResumeAfterGate-->>WebhookEndpoint: autoResumed
WebhookEndpoint-->>ExternalCaller: { received: true, autoResumed }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 349dbcb588
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+4290
to
+4293
| try { | ||
| payload = await c.req.json(); | ||
| } catch { | ||
| payload = await c.req.text(); |
There was a problem hiding this comment.
When an external trigger sends text/plain or otherwise invalid JSON, c.req.json() consumes the request body before throwing in Bun/Fetch, so the fallback c.req.text() throws Body already used and the outer catch returns 500. Since this route explicitly accepts raw text payloads, read the body once (for example as text, then JSON.parse when appropriate) so non-JSON webhooks can trigger the node.
Useful? React with 👍 / 👎.
Comment on lines
+2718
to
+2721
| message, | ||
| nodeId: node.id, | ||
| type: 'webhook', | ||
| captureResponse: true, // payload is always captured |
There was a problem hiding this comment.
For a webhook node that sets webhook.timeout, this pause context does not store a deadline or start time for the receiver to validate, and the webhook handler later only checks the paused node/type before accepting the POST. A trigger sent after the advertised timeout will still record node_completed and resume the workflow, so timed webhook gates never actually expire.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
packages/core/src/orchestrator/orchestrator.test.ts (1)
680-680: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the forwarded gods payload, not just “some array.”
expect.any(Array)only proves the new parameter exists. These assertions still pass ifhandleMessagealways sends[]or drops the merged config value entirely, so the new config-to-prompt wiring is effectively untested. Please assert the concrete gods array returned by the mocked config path here.Also applies to: 699-699
🤖 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/core/src/orchestrator/orchestrator.test.ts` at line 680, The assertions in orchestrator.test.ts are too weak because they only verify that an array was forwarded, not that the correct gods payload from the mocked config path was passed through. Update the expectations around handleMessage in the orchestrator test to assert the exact gods array value returned by the mocked config merge, using the existing orchestrator and handleMessage mocks so the config-to-prompt wiring is validated rather than just the parameter shape.
🤖 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 @.archon/ralph/conductor-archon-extensions/prd.md:
- Around line 17-24: Add the required blank lines before and after each markdown
table in the PRD so markdownlint MD058 passes; update the table sections in the
document (including the metrics tables and any other table blocks) to ensure
they are separated from surrounding text by empty lines while preserving the
table content and formatting.
- Around line 127-141: The fenced code block in the PRD section is missing a
language identifier, triggering the MD040 markdown lint issue. Update the
opening fence for the block containing the US-001 through US-007 dependency list
to include an explicit language tag, using the existing content under that
fence; keep the block structure unchanged and ensure the closing fence still
matches.
In `@packages/cli/src/commands/workflow.ts`:
- Around line 2199-2210: The import path in workflow.ts is exiting the process
directly on file-missing and JSON-parse failures, which prevents the top-level
cleanup in main()/packages/cli/src/cli.ts from running. Update the file-handling
logic around the absoluteFilePath check and the JSON.parse try/catch to throw an
Error or otherwise return a failure signal instead of calling process.exit(1),
and let the top-level CLI entrypoint decide when to terminate so telemetry
flushing and DB cleanup still execute.
- Around line 2231-2232: The workflow export path is using options.out directly
in the filename composition, which allows path traversal outside the intended
workflows directory. Update the outName/outFile logic in the workflow command to
sanitize or validate options.out before passing it to path.join, ensuring only a
safe basename is used and the destination always stays under .archon/workflows.
Use the workflow export path construction near the workflow name/default
handling to locate and fix this.
In `@packages/core/src/config/config-loader.ts`:
- Around line 105-108: The mergeGods helper currently appends the full overrides
array, so duplicate ids within overrides can survive and produce repeated
entries. Update mergeGods to deduplicate overrides by id before returning the
merged list, keeping only one entry per id while still replacing any matching
base entries. Use the existing mergeGods function and its overrideIds logic to
filter or collapse duplicates before concatenation.
In `@packages/server/src/routes/api.ts`:
- Around line 4260-4264: The webhook handler in
app.post('/webhooks/workflow/:runId/:nodeId') currently accepts external
triggers without any authentication, so add a per-gate secret token or HMAC
verification before processing or storing the payload. Update the webhook route
logic to validate the incoming request against a shared secret/capability token
tied to the gate, and reject requests that fail verification before the workflow
can resume.
- Around line 4288-4309: The webhook handler in api.ts stores unbounded request
bodies into workflow events, so add a payload size check before the
c.req.json/c.req.text read path in the webhook route logic. Use the existing
webhook payload handling around payload, outputStr, and
workflowEventDb.createWorkflowEvent to reject or truncate oversized bodies
before persistence, and ensure both node_completed and webhook_triggered writes
only receive bounded data.
- Around line 4268-4314: The webhook completion path in the API route is not
idempotent, so duplicate or concurrent deliveries can write duplicate completion
events and trigger multiple resumes. Update the webhook handler around the run
status check and the event writes to use an atomic conditional transition or
transaction in workflowDb/workflowEventDb before creating the node_completed and
webhook_triggered events. If the run has already been resolved by another
request, short-circuit with a success/no-op response instead of reprocessing,
and keep the logic centered on the webhook trigger flow and
tryAutoResumeAfterGate.
- Line 4314: The auto-resume path in tryAutoResumeAfterGate should not use
resolveWebUserId(c) on this public webhook route because it can be spoofed from
request headers. Update the call site in api.ts so the resume actor is omitted
or derived only from a verified webhook credential, and keep the change
localized around the tryAutoResumeAfterGate invocation used for the approve
path.
- Around line 4264-4320: The new webhook route handler on
app.post('/webhooks/workflow/:runId/:nodeId') is inferred too loosely, causing
unsafe-call/unsafe-return lint errors around c.req and c.json. Add explicit Hono
Context and Promise<Response> typing to the handler signature (or move the logic
into a fully typed helper called by the route) so all request/response calls are
type-safe. Keep the same workflowDb, workflowEventDb, and tryAutoResumeAfterGate
logic, but ensure the route handler itself is fully annotated.
In `@packages/workflows/src/dag-executor.ts`:
- Around line 2696-2722: The webhook-trigger flow in dag-executor currently
pauses the run unconditionally after calling safeSendMessage(), even when
delivery is suppressed or fails. Update the webhook branch around webhookMsg so
it checks the result of safeSendMessage() and, if delivery did not succeed, fail
the node/run instead of calling deps.store.pauseWorkflowRun(). Reuse the same
failure-handling pattern used by the interactive-loop gate logic in dag-executor
to keep behavior consistent.
In `@packages/workflows/src/n8n-converter.ts`:
- Around line 159-190: Duplicate n8n node names are currently collapsing through
nameToFirstId and can miswire depends_on during conversion. In n8n-converter.ts,
update the workflow import logic around the uuidToId/nameToFirstId mapping and
dependsOnMap construction to detect repeated n8nNode.name values before
resolving connections. Either fail fast with a clear error or emit a warning and
skip ambiguous connection mapping, so the conversion does not silently attach
all edges to the first-seen node.
- Around line 90-96: The generated bash in the n8n converter is directly
interpolating untrusted JSON values, so the HttpRequest conversion needs shell
escaping before composing the command. Update the `n8n-converter.ts` handlers
such as the `n8n-nodes-base.HttpRequest` mapper (and the other affected mappings
like the one noted in the comment) to sanitize `url`, `method`, and any
`n8nType`-derived text before inserting them into `bash`. Use a proper
shell-escaping approach or quote-safe construction so imported workflow fields
cannot break out of the command text.
- Around line 73-87: The n8n `Code` node converter currently maps the non-Python
path in `n8n-converter.ts` to a `bash` step, which breaks JavaScript `Code`
nodes. Update the `n8n-nodes-base.Code` handling so `node.parameters.language
!== 'python'` produces a JavaScript-capable `NodeKernel` using `script` with
`bun` rather than `bash`, while keeping the Python branch on `uv`. Preserve the
existing code extraction from `jsCode`/`pythonCode` and ensure the converted
wrapper comment remains intact.
In `@packages/workflows/src/schemas/dag-node.ts`:
- Around line 366-368: `webhookNodeSchema` is currently inheriting and accepting
base fields that `executeDagWorkflow()` never uses for webhook execution. Update
`webhookNodeSchema` (and the related transform/validation path around `retry`,
`provider`, `model`, and `persist_session`) to explicitly reject unsupported
fields or mark them as `never`, so invalid webhook config fails fast instead of
parsing silently. Make the same validation change in the matching webhook schema
definition referenced elsewhere in the diff so both entry points enforce the
same unsupported-field rules.
In `@packages/workflows/src/schemas/workflow-run.ts`:
- Around line 128-129: The new webhook pause type is being treated as a normal
approval in the approval flow, so update the pause/resume handling to keep
webhook runs off the natural-language chat path. In orchestrator-agent.ts,
adjust the branch that currently checks approval.type !== 'interactive_loop' so
it explicitly rejects or routes approval.type === 'webhook' to a distinct
non-chat-resumable path, and preserve the existing approval/interactive_loop
behavior only for those types. Use the ApprovalContext.type union in
workflow-run.ts and the approval handling logic in the orchestrator agent to
locate the change.
---
Nitpick comments:
In `@packages/core/src/orchestrator/orchestrator.test.ts`:
- Line 680: The assertions in orchestrator.test.ts are too weak because they
only verify that an array was forwarded, not that the correct gods payload from
the mocked config path was passed through. Update the expectations around
handleMessage in the orchestrator test to assert the exact gods array value
returned by the mocked config merge, using the existing orchestrator and
handleMessage mocks so the config-to-prompt wiring is validated rather than just
the parameter shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48d84dbd-ff4e-4a36-8be7-3aecb3d1a535
📒 Files selected for processing (24)
.archon/commands/defaults/conductor-persona.md.archon/ralph/conductor-archon-extensions/prd.json.archon/ralph/conductor-archon-extensions/prd.mdpackages/cli/src/cli.tspackages/cli/src/commands/workflow.tspackages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/orchestrator/orchestrator.test.tspackages/core/src/orchestrator/prompt-builder.test.tspackages/core/src/orchestrator/prompt-builder.tspackages/paths/src/telemetry.tspackages/server/src/routes/api.tspackages/server/src/routes/schemas/workflow.schemas.tspackages/workflows/package.jsonpackages/workflows/src/dag-executor.tspackages/workflows/src/defaults/bundled-defaults.generated.tspackages/workflows/src/n8n-converter.test.tspackages/workflows/src/n8n-converter.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/schemas/index.tspackages/workflows/src/schemas/workflow-run.tspackages/workflows/src/store.tspackages/workflows/src/webhook-node-schema.test.ts
Comment on lines
+17
to
+24
| | Metric | Target | How Measured | | ||
| |--------|--------|--------------| | ||
| | God registry visible in system prompt | Any god in config appears in prompt | Unit test `formatGodsSection()` | | ||
| | Webhook node pauses workflow | Run enters `paused` state on webhook node | Integration: workflow runs + event check | | ||
| | n8n converter round-trips | Reference n8n JSON produces valid Archon YAML | Unit test with fixture | | ||
| | Type-check clean | Zero tsc errors | `bun run type-check` | | ||
| | All tests pass | Zero failures | `bun run test` | | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add blank lines around tables to satisfy markdownlint.
Line 17, Line 61, and Line 116 start tables without required surrounding blank lines (MD058).
Suggested fix
### Success Metrics
+
| Metric | Target | How Measured |
|--------|--------|--------------|
| God registry visible in system prompt | Any god in config appears in prompt | Unit test `formatGodsSection()` |
| Webhook node pauses workflow | Run enters `paused` state on webhook node | Integration: workflow runs + event check |
| n8n converter round-trips | Reference n8n JSON produces valid Archon YAML | Unit test with fixture |
| Type-check clean | Zero tsc errors | `bun run type-check` |
| All tests pass | Zero failures | `bun run test` |
+Also applies to: 61-68, 116-124
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 17-17: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 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 @.archon/ralph/conductor-archon-extensions/prd.md around lines 17 - 24, Add
the required blank lines before and after each markdown table in the PRD so
markdownlint MD058 passes; update the table sections in the document (including
the metrics tables and any other table blocks) to ensure they are separated from
surrounding text by empty lines while preserving the table content and
formatting.
Source: Linters/SAST tools
Comment on lines
+127
to
+141
| ``` | ||
| US-001 (config types) | ||
| ↓ | ||
| US-002 (prompt-builder) | ||
|
|
||
| US-003 (persona file) — independent | ||
|
|
||
| US-004 (webhook schema) | ||
| ↓ | ||
| US-005 (webhook receiver + executor) | ||
|
|
||
| US-006 (n8n converter) | ||
| ↓ | ||
| US-007 (import CLI + REST) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify a language for the fenced code block.
The code fence opened at Line 127 is missing a language tag (MD040).
Suggested fix
-```
+```text
US-001 (config types)
↓
US-002 (prompt-builder)
...
US-006 (n8n converter)
↓
US-007 (import CLI + REST)</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 127-127: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 @.archon/ralph/conductor-archon-extensions/prd.md around lines 127 - 141, The
fenced code block in the PRD section is missing a language identifier,
triggering the MD040 markdown lint issue. Update the opening fence for the block
containing the US-001 through US-007 dependency list to include an explicit
language tag, using the existing content under that fence; keep the block
structure unchanged and ensure the closing fence still matches.
Source: Linters/SAST tools
Comment on lines
+2199
to
+2210
| if (!fs.existsSync(absoluteFilePath)) { | ||
| console.error(`Error: File not found: ${absoluteFilePath}`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| let n8nJson: unknown; | ||
| try { | ||
| const raw = fs.readFileSync(absoluteFilePath, 'utf-8'); | ||
| n8nJson = JSON.parse(raw); | ||
| } catch { | ||
| console.error(`Error: Could not parse ${path.basename(absoluteFilePath)} as JSON`); | ||
| process.exit(1); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Let main() own process exit and cleanup.
Calling process.exit(1) here bypasses the finally block in packages/cli/src/cli.ts, so telemetry flushing and DB cleanup never run on import failures. Throw an Error (or return an exit code) and let the top-level CLI decide when to terminate.
Suggested fix
if (!fs.existsSync(absoluteFilePath)) {
- console.error(`Error: File not found: ${absoluteFilePath}`);
- process.exit(1);
+ throw new Error(`File not found: ${absoluteFilePath}`);
}
...
try {
const raw = fs.readFileSync(absoluteFilePath, 'utf-8');
n8nJson = JSON.parse(raw);
} catch {
- console.error(`Error: Could not parse ${path.basename(absoluteFilePath)} as JSON`);
- process.exit(1);
+ throw new Error(`Could not parse ${path.basename(absoluteFilePath)} as JSON`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!fs.existsSync(absoluteFilePath)) { | |
| console.error(`Error: File not found: ${absoluteFilePath}`); | |
| process.exit(1); | |
| } | |
| let n8nJson: unknown; | |
| try { | |
| const raw = fs.readFileSync(absoluteFilePath, 'utf-8'); | |
| n8nJson = JSON.parse(raw); | |
| } catch { | |
| console.error(`Error: Could not parse ${path.basename(absoluteFilePath)} as JSON`); | |
| process.exit(1); | |
| if (!fs.existsSync(absoluteFilePath)) { | |
| throw new Error(`File not found: ${absoluteFilePath}`); | |
| } | |
| let n8nJson: unknown; | |
| try { | |
| const raw = fs.readFileSync(absoluteFilePath, 'utf-8'); | |
| n8nJson = JSON.parse(raw); | |
| } catch { | |
| throw new Error(`Could not parse ${path.basename(absoluteFilePath)} as JSON`); | |
| } |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 2205-2205: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(absoluteFilePath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/cli/src/commands/workflow.ts` around lines 2199 - 2210, The import
path in workflow.ts is exiting the process directly on file-missing and
JSON-parse failures, which prevents the top-level cleanup in
main()/packages/cli/src/cli.ts from running. Update the file-handling logic
around the absoluteFilePath check and the JSON.parse try/catch to throw an Error
or otherwise return a failure signal instead of calling process.exit(1), and let
the top-level CLI entrypoint decide when to terminate so telemetry flushing and
DB cleanup still execute.
Comment on lines
+2231
to
+2232
| const outName = options.out ?? workflow.name; | ||
| const outFile = path.join(outDir, `${outName}.yaml`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Sanitize --out before composing the destination filename.
options.out is joined directly into outDir, so --out ../../package.json escapes .archon/workflows and can overwrite arbitrary files. That breaks the command's “write into .archon/workflows/” contract.
Suggested fix
- const outName = options.out ?? workflow.name;
+ const rawOutName = options.out ?? workflow.name;
+ const outName = rawOutName.trim();
+ if (outName.length === 0 || outName !== path.basename(outName) || outName.includes(path.sep)) {
+ throw new Error(`Invalid output name: ${rawOutName}`);
+ }
const outFile = path.join(outDir, `${outName}.yaml`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const outName = options.out ?? workflow.name; | |
| const outFile = path.join(outDir, `${outName}.yaml`); | |
| const rawOutName = options.out ?? workflow.name; | |
| const outName = rawOutName.trim(); | |
| if (outName.length === 0 || outName !== path.basename(outName) || outName.includes(path.sep)) { | |
| throw new Error(`Invalid output name: ${rawOutName}`); | |
| } | |
| const outFile = path.join(outDir, `${outName}.yaml`); |
🤖 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/cli/src/commands/workflow.ts` around lines 2231 - 2232, The workflow
export path is using options.out directly in the filename composition, which
allows path traversal outside the intended workflows directory. Update the
outName/outFile logic in the workflow command to sanitize or validate
options.out before passing it to path.join, ensuring only a safe basename is
used and the destination always stays under .archon/workflows. Use the workflow
export path construction near the workflow name/default handling to locate and
fix this.
Comment on lines
+105
to
+108
| function mergeGods(base: GodDefinition[], overrides: GodDefinition[]): GodDefinition[] { | ||
| if (overrides.length === 0) return base; | ||
| const overrideIds = new Set(overrides.map(g => g.id)); | ||
| return [...base.filter(g => !overrideIds.has(g.id)), ...overrides]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Deduplicate repeated override ids before appending.
Line 107 collects override ids, but Line 108 still appends the entire overrides array unchanged. If a single config file contains the same id twice, the merged result keeps both entries, which breaks the documented “override by id” contract and will render duplicate specialists in the prompt.
Proposed fix
function mergeGods(base: GodDefinition[], overrides: GodDefinition[]): GodDefinition[] {
if (overrides.length === 0) return base;
- const overrideIds = new Set(overrides.map(g => g.id));
- return [...base.filter(g => !overrideIds.has(g.id)), ...overrides];
+ const dedupedOverridesById = new Map<string, GodDefinition>();
+ for (const god of overrides) {
+ dedupedOverridesById.set(god.id, god);
+ }
+ const dedupedOverrides = [...dedupedOverridesById.values()];
+ const overrideIds = new Set(dedupedOverrides.map(g => g.id));
+ return [...base.filter(g => !overrideIds.has(g.id)), ...dedupedOverrides];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function mergeGods(base: GodDefinition[], overrides: GodDefinition[]): GodDefinition[] { | |
| if (overrides.length === 0) return base; | |
| const overrideIds = new Set(overrides.map(g => g.id)); | |
| return [...base.filter(g => !overrideIds.has(g.id)), ...overrides]; | |
| function mergeGods(base: GodDefinition[], overrides: GodDefinition[]): GodDefinition[] { | |
| if (overrides.length === 0) return base; | |
| const dedupedOverridesById = new Map<string, GodDefinition>(); | |
| for (const god of overrides) { | |
| dedupedOverridesById.set(god.id, god); | |
| } | |
| const dedupedOverrides = [...dedupedOverridesById.values()]; | |
| const overrideIds = new Set(dedupedOverrides.map(g => g.id)); | |
| return [...base.filter(g => !overrideIds.has(g.id)), ...dedupedOverrides]; | |
| } |
🤖 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/core/src/config/config-loader.ts` around lines 105 - 108, The
mergeGods helper currently appends the full overrides array, so duplicate ids
within overrides can survive and produce repeated entries. Update mergeGods to
deduplicate overrides by id before returning the merged list, keeping only one
entry per id while still replacing any matching base entries. Use the existing
mergeGods function and its overrideIds logic to filter or collapse duplicates
before concatenation.
Comment on lines
+73
to
+87
| 'n8n-nodes-base.Code', | ||
| (node): NodeKernel => { | ||
| const code = | ||
| (node.parameters?.jsCode as string | undefined) ?? | ||
| (node.parameters?.pythonCode as string | undefined) ?? | ||
| '# TODO: add code'; | ||
| const isPython = node.parameters?.language === 'python'; | ||
| if (isPython) { | ||
| return { | ||
| script: `# Converted from n8n Code node\n${code}`, | ||
| runtime: 'uv', | ||
| }; | ||
| } | ||
| return { bash: `# Converted from n8n Code node\n${code}` }; | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Map JavaScript Code nodes to script/bun, not bash.
The non-Python branch turns n8n JavaScript like console.log(items) into a bash step, so imported workflows fail on first execution instead of preserving the original node semantics.
Suggested fix
[
'n8n-nodes-base.Code',
(node): NodeKernel => {
const code =
(node.parameters?.jsCode as string | undefined) ??
(node.parameters?.pythonCode as string | undefined) ??
'# TODO: add code';
const isPython = node.parameters?.language === 'python';
if (isPython) {
return {
script: `# Converted from n8n Code node\n${code}`,
runtime: 'uv',
};
}
- return { bash: `# Converted from n8n Code node\n${code}` };
+ return {
+ script: `// Converted from n8n Code node\n${code}`,
+ runtime: 'bun',
+ };
},
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 'n8n-nodes-base.Code', | |
| (node): NodeKernel => { | |
| const code = | |
| (node.parameters?.jsCode as string | undefined) ?? | |
| (node.parameters?.pythonCode as string | undefined) ?? | |
| '# TODO: add code'; | |
| const isPython = node.parameters?.language === 'python'; | |
| if (isPython) { | |
| return { | |
| script: `# Converted from n8n Code node\n${code}`, | |
| runtime: 'uv', | |
| }; | |
| } | |
| return { bash: `# Converted from n8n Code node\n${code}` }; | |
| }, | |
| 'n8n-nodes-base.Code', | |
| (node): NodeKernel => { | |
| const code = | |
| (node.parameters?.jsCode as string | undefined) ?? | |
| (node.parameters?.pythonCode as string | undefined) ?? | |
| '# TODO: add code'; | |
| const isPython = node.parameters?.language === 'python'; | |
| if (isPython) { | |
| return { | |
| script: `# Converted from n8n Code node\n${code}`, | |
| runtime: 'uv', | |
| }; | |
| } | |
| return { | |
| script: `// Converted from n8n Code node\n${code}`, | |
| runtime: 'bun', | |
| }; | |
| }, |
🤖 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/n8n-converter.ts` around lines 73 - 87, The n8n `Code`
node converter currently maps the non-Python path in `n8n-converter.ts` to a
`bash` step, which breaks JavaScript `Code` nodes. Update the
`n8n-nodes-base.Code` handling so `node.parameters.language !== 'python'`
produces a JavaScript-capable `NodeKernel` using `script` with `bun` rather than
`bash`, while keeping the Python branch on `uv`. Preserve the existing code
extraction from `jsCode`/`pythonCode` and ensure the converted wrapper comment
remains intact.
Comment on lines
+90
to
+96
| 'n8n-nodes-base.HttpRequest', | ||
| (node): NodeKernel => { | ||
| const url = (node.parameters?.url as string | undefined) ?? 'https://example.com'; | ||
| const method = (node.parameters?.method as string | undefined) ?? 'GET'; | ||
| return { | ||
| bash: `# Converted from n8n HttpRequest node\ncurl -s -X ${method.toUpperCase()} '${url}'`, | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not embed imported n8n fields into bash without escaping.
url, method, and n8nType come straight from the uploaded JSON and are spliced into generated shell text. A quote or shell metacharacter here produces a workflow that executes attacker-controlled commands later when someone runs the import.
Suggested fix
+function shellQuote(value: string): string {
+ return `'${value.replace(/'/g, `'\\''`)}'`;
+}
+
[
'n8n-nodes-base.HttpRequest',
(node): NodeKernel => {
const url = (node.parameters?.url as string | undefined) ?? 'https://example.com';
const method = (node.parameters?.method as string | undefined) ?? 'GET';
+ const safeMethod = /^(GET|POST|PUT|PATCH|DELETE|HEAD|OPTIONS)$/i.test(method)
+ ? method.toUpperCase()
+ : 'GET';
return {
- bash: `# Converted from n8n HttpRequest node\ncurl -s -X ${method.toUpperCase()} '${url}'`,
+ bash: `# Converted from n8n HttpRequest node\ncurl -s -X ${safeMethod} ${shellQuote(url)}`,
};
},
],
...
function stubKernel(n8nType: string): NodeKernel {
return {
- bash: `# TODO: map from n8n type ${n8nType}\necho "Unsupported n8n node type: ${n8nType}"`,
+ bash: `# TODO: map from n8n type ${n8nType}\necho ${shellQuote(`Unsupported n8n node type: ${n8nType}`)}`,
};
}Also applies to: 124-127
🤖 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/n8n-converter.ts` around lines 90 - 96, The generated
bash in the n8n converter is directly interpolating untrusted JSON values, so
the HttpRequest conversion needs shell escaping before composing the command.
Update the `n8n-converter.ts` handlers such as the `n8n-nodes-base.HttpRequest`
mapper (and the other affected mappings like the one noted in the comment) to
sanitize `url`, `method`, and any `n8nType`-derived text before inserting them
into `bash`. Use a proper shell-escaping approach or quote-safe construction so
imported workflow fields cannot break out of the command text.
Comment on lines
+159
to
+190
| // Assign deterministic Archon IDs keyed by n8n UUID, deduplicating name collisions. | ||
| // n8n connections reference nodes by *name*, so we also maintain a name→firstId map | ||
| // for connection resolution (name collisions in connections are unresolvable; we use | ||
| // first-seen, which is the same heuristic n8n itself applies). | ||
| const uuidToId = new Map<string, string>(); | ||
| const nameToFirstId = new Map<string, string>(); | ||
| const idCount = new Map<string, number>(); | ||
| for (const n8nNode of n8nWorkflow.nodes) { | ||
| const base = toNodeId(n8nNode.name); | ||
| const count = idCount.get(base) ?? 0; | ||
| idCount.set(base, count + 1); | ||
| const archonId = count === 0 ? base : `${base}-${count}`; | ||
| uuidToId.set(n8nNode.id, archonId); | ||
| if (!nameToFirstId.has(n8nNode.name)) { | ||
| nameToFirstId.set(n8nNode.name, archonId); | ||
| } | ||
| } | ||
|
|
||
| // Build depends_on from n8n connections | ||
| // connections[sourceName].main[outputIndex] = [{ node: targetName, ... }] | ||
| const dependsOnMap = new Map<string, string[]>(); | ||
| const connections = n8nWorkflow.connections ?? {}; | ||
| for (const [sourceName, conns] of Object.entries(connections)) { | ||
| const sourceId = nameToFirstId.get(sourceName); | ||
| if (!sourceId) continue; | ||
| for (const outputTargets of conns.main ?? []) { | ||
| for (const target of outputTargets ?? []) { | ||
| const targetId = nameToFirstId.get(target.node); | ||
| if (!targetId) continue; | ||
| const deps = dependsOnMap.get(targetId) ?? []; | ||
| if (!deps.includes(sourceId)) deps.push(sourceId); | ||
| dependsOnMap.set(targetId, deps); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Duplicate n8n node names silently corrupt depends_on.
IDs are deduplicated, but connection lookup still collapses everything through nameToFirstId. If an export contains two nodes with the same name, every edge for that name is attached to the first node and the imported DAG no longer matches the source workflow.
Fail fast or at least emit a warning before building depends_on when duplicate names are present.
🤖 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/n8n-converter.ts` around lines 159 - 190, Duplicate
n8n node names are currently collapsing through nameToFirstId and can miswire
depends_on during conversion. In n8n-converter.ts, update the workflow import
logic around the uuidToId/nameToFirstId mapping and dependsOnMap construction to
detect repeated n8nNode.name values before resolving connections. Either fail
fast with a clear error or emit a warning and skip ambiguous connection mapping,
so the conversion does not silently attach all edges to the first-seen node.
Comment on lines
+366
to
+368
| export const webhookNodeSchema = dagNodeBaseSchema.extend({ | ||
| webhook: webhookNodeConfigSchema, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject webhook fields that execution never honors.
webhookNodeSchema inherits base AI/session fields, and the transform still preserves retry, but executeDagWorkflow() handles webhook nodes before provider/model resolution and the common retry path. That means config like retry, provider, model, or persist_session can parse successfully even though webhook execution ignores it. Please fail validation for unsupported fields (or model them as never) instead of accepting dead config.
As per coding guidelines, "Prefer throwing early with a clear error for unsupported or unsafe states — never silently swallow errors or broaden permissions."
Also applies to: 681-683
🧰 Tools
🪛 ESLint
[error] 366-366: Unsafe call of a type that could not be resolved.
(@typescript-eslint/no-unsafe-call)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/workflows/src/schemas/dag-node.ts` around lines 366 - 368,
`webhookNodeSchema` is currently inheriting and accepting base fields that
`executeDagWorkflow()` never uses for webhook execution. Update
`webhookNodeSchema` (and the related transform/validation path around `retry`,
`provider`, `model`, and `persist_session`) to explicitly reject unsupported
fields or mark them as `never`, so invalid webhook config fails fast instead of
parsing silently. Make the same validation change in the matching webhook schema
definition referenced elsewhere in the diff so both entry points enforce the
same unsupported-field rules.
Source: Coding guidelines
Comment on lines
+128
to
+129
| /** Distinguishes approval-gate pauses, interactive-loop pauses, and webhook-trigger pauses. */ | ||
| type?: 'approval' | 'interactive_loop' | 'webhook'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't let webhook pauses fall into natural-language approval.
By widening ApprovalContext.type to 'webhook', the existing approval.type !== 'interactive_loop' branch in packages/core/src/orchestrator/orchestrator-agent.ts will now treat a paused webhook run like a normal approval: it writes node_completed and resumes on the next chat message. That bypasses the external POST contract entirely. Webhook pauses need a distinct non-chat-resumable path, or the approval handler must explicitly reject type === 'webhook'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/workflows/src/schemas/workflow-run.ts` around lines 128 - 129, The
new webhook pause type is being treated as a normal approval in the approval
flow, so update the pause/resume handling to keep webhook runs off the
natural-language chat path. In orchestrator-agent.ts, adjust the branch that
currently checks approval.type !== 'interactive_loop' so it explicitly rejects
or routes approval.type === 'webhook' to a distinct non-chat-resumable path, and
preserve the existing approval/interactive_loop behavior only for those types.
Use the ApprovalContext.type union in workflow-run.ts and the approval handling
logic in the orchestrator agent to locate the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GodDefinitionto config types +formatGodsSection()in prompt builder — operators can declare specialist personas in.archon/config.yaml'sgods:block and the chat system prompt gains a## Known Specialistssection describing each one and their dispatch workflowsconductor-persona.mdbundled default command — Conductor routing doctrine (answer directly vs dispatch to god workflow, ledger-shape metaphor, refusal to drift outside project scope)webhook:DAG node type inpackages/workflows/src/schemas/dag-node.ts— pauses workflow execution awaiting an external HTTP POST, with configurablemessageandtimeoutPOST /webhooks/workflow/:runId/:nodeIdreceiver + DAG executor support — mirrors the approval-gate pause/resume pattern; any external system (Zapier, n8n, GitHub Actions) can trigger continuation by POSTing to the printed URL; payload is captured as node outputpackages/workflows/src/n8n-converter.ts— pure functionconvertN8nToArchon()that converts n8n workflow JSON toWorkflowDefinition; maps HttpRequest→bash, Code→bash/script, OpenAiChat→prompt, Wait→approval; unknown types degrade to bash stubs with warningsarchon workflow import n8n <file.json>CLI +POST /api/workflows/import/n8nREST endpoint; CLI writes YAML to.archon/workflows/; REST returns YAML string for preview without writing to diskTest plan
bun run type-check— zero errorsbun run test— all tests pass (including new tests:webhook-node-schema.test.ts,n8n-converter.test.ts, updatedorchestrator.test.ts)bun run linton changed packages — zero warningsbun run check:bundled— bundled defaults up to date (conductor-persona included)archon workflow import n8n <n8n-export.json>and verify YAML written to.archon/workflows/webhook:node, run it, POST to the printed URL, verify it resumes🤖 Generated with Claude Code
Summary by CodeRabbit