fix(server): GET /api/workflows/:name now resolves global (~/.archon/workflows/) workflows#1366
fix(server): GET /api/workflows/:name now resolves global (~/.archon/workflows/) workflows#1366marciotoze wants to merge 8 commits intocoleam00:devfrom
Conversation
…be (coleam00#1359) The pre-flight binary smoke does a bare `bun build --compile` — it deliberately skips `scripts/build-binaries.sh` to stay fast. That means packages/paths/src/bundled-build.ts retains its dev defaults, including BUNDLED_IS_BINARY = false. version.ts branches on BUNDLED_IS_BINARY: when true it returns the embedded string; when false it calls getDevVersion(), which reads package.json at `SCRIPT_DIR/../../../../package.json`. Inside a compiled binary SCRIPT_DIR resolves under `$bunfs/root/`, the walk produces a CWD- relative path that doesn't exist, and the smoke aborts with "Failed to read version: package.json not found" — a false positive. Hit during the 0.3.8 release attempt: the real Pi lazy-load fix was working end-to-end; the smoke test was the only thing failing. Use --help instead. It exercises the same module-init graph (so it still catches the real failure modes the skill lists — Pi package.json init crash, Bun --bytecode bugs, CJS wrapper issues, circular imports under minify) but has no dev/binary branch, so no false positive. Also add a longer comment block explaining why --help is preferred, so this doesn't get "normalized" back to `version` by a future drive-by.
The brew path of /test-release runs `brew uninstall` in Phase 5 to leave the system in its pre-test state. For operators using the dual-homebrew pattern (renamed brew binary at `/opt/homebrew/bin/archon-stable` so it coexists with a `bun link` dev `archon`), that uninstall wipes the Cellar dir the `archon-stable` symlink points into → `archon-stable` becomes dangling → `brew cleanup` sweeps it away on the next brew op. Next time the operator wants stable, they have to manually re-run `brew-upgrade-archon`. Fix: make the skill aware of `archon-stable` and restore it transparently. - Phase 2 item 4: detect the `archon-stable` symlink before any brew op; export `ARCHON_STABLE_WAS_INSTALLED=yes` so Phase 5 knows to restore it. Only triggers for the brew path (curl-mac/curl-vps don't touch brew so they leave `archon-stable` alone). - Phase 5 brew path: after `brew uninstall + untap`, if the flag was set, re-tap + re-install + rename. Verifies the restored `archon-stable` reports a version and warns (non-fatal) if the rename target is missing. Documents the tradeoff: the restored version is "whatever the tap ships today", not necessarily the pre-test version — usually that's what the operator wants (the release they just tested becomes stable) but the back-version-QA case requires a manual `brew-upgrade-archon` after. - Phase 1 confirmation banner now mentions that `archon-stable` will be preserved so the operator isn't surprised by the reinstall during Phase 5. No changes to curl-mac/curl-vps paths. No changes to Phase 4 test suite.
… a compiled binary (coleam00#1360) v0.3.9 made Pi boot-safe: lazy-loading its imports meant `archon version` no longer crashed on `@mariozechner/pi-coding-agent/dist/config.js`'s module-init `readFileSync(getPackageJsonPath())`. That's what the `provider-lazy-load.test.ts` regression test guards. The fix was only half the problem though. When a Pi workflow actually runs, sendQuery() triggers the dynamic import — and Pi's config.js module-init fires then, hitting the exact same ENOENT on `dirname(process.execPath)/package.json`. Discovered by running `archon workflow run test-pi` against a locally-compiled 0.3.9 binary: [main] Failed: ENOENT: no such file or directory, open '/private/tmp/package.json' at readFileSync (unknown) at <anonymous> (/$bunfs/root/archon-providertest:184:7889) at init_config Boot-safe ≠ runtime-safe. The `/test-release` run for 0.3.9 passed because it only exercised `archon-assist` (Claude); Pi was never actually invoked on the released binary. Fix: before the dynamic `import('@mariozechner/pi-coding-agent')` in sendQuery, install a PI_PACKAGE_DIR shim. Pi's config.js checks `process.env.PI_PACKAGE_DIR` first in its `getPackageDir()` and short-circuits the `dirname(process.execPath)` walk. We write a minimal `{name, version, piConfig:{}}` stub to `tmpdir()/archon-pi-shim/package.json` (idempotent — existsSync check) and set the env var. Pi only reads `piConfig.name`, `piConfig.configDir`, and `version` from that file, all optional, so the stub surface is genuinely minimal. Localized to PiProvider: no global state, no mutation of any shared config, no upstream fork. Claude and Codex providers are unaffected (their SDKs don't have this class of module-init side effect). Verified end-to-end: built a compiled archon binary with this patch, ran `archon workflow run test-pi --no-worktree` (Pi workflow with model `anthropic/claude-haiku-4-5`), got a clean response. Before the patch, same binary crashed at `dag_node_started` with the ENOENT above. Regression test added: asserts `PI_PACKAGE_DIR` is set after sendQuery hits even its fast-fail "no model" path. Together with the existing `provider-lazy-load.test.ts` (boot-safe) this covers both halves.
… and Codex (coleam00#1361) Both binary resolvers previously stopped at env-var + explicit config and threw a "not found" error when neither was set. Users who followed the upstream-recommended install flow (Anthropic's `curl install.sh` for Claude, `npm install -g @openai/codex`) still had to manually set either `CLAUDE_BIN_PATH` / `CODEX_BIN_PATH` or the corresponding config field before any workflow could run. Add a tier-N autodetect step between the explicit config tier and the install-instructions throw. Purely additive: env and config still win when set (precedence covered by new tests). On autodetect miss, the same install-instructions error fires as before. Claude probe list (verified against docs.claude.com "Uninstall Claude Code → Native installation" section): - $HOME/.local/bin/claude (mac/linux native installer) - $USERPROFILE\.local\bin\claude.exe (Windows native installer) Codex probe list (verified against openai/codex README; npm global- install puts the binary at `{npm_prefix}/bin/<name>` on POSIX, `{npm_prefix}\<name>.cmd` on Windows): - $HOME/.npm-global/bin/codex (user-set `npm config set prefix`) - /opt/homebrew/bin/codex (mac arm64 with homebrew-node) - /usr/local/bin/codex (mac intel / linux system node) - %APPDATA%\npm\codex.cmd (Windows npm global default) - $HOME\.npm-global\codex.cmd (Windows user-set prefix) Not probed (explicit override still required): - Custom npm prefixes — `npm root -g` would need a subprocess per resolve, too much surface for a probe helper - `brew install --cask codex` — cask layout isn't a PATH binary - Manual GitHub Releases extracts — placement is user-determined - `~/.bun/bin/codex` — not documented in openai/codex README Pi provider intentionally has no equivalent change: the Pi SDK is bundled into the archon binary (no subprocess), so there's no "binary" to resolve. Pi auth lives at `~/.pi/agent/auth.json` which the SDK already finds by default, and the PR A shim (`PI_PACKAGE_DIR`) handles the package-dir case via Pi's own documented escape hatch. E2E verified: removed both config entries from ~/.archon/config.yaml, rebuilt compiled binary, ran `archon workflow run archon-assist` and a Codex workflow. Logs showed `source: 'autodetect'` for both, responses returned cleanly.
…workflows/) workflows The workflow builder (?edit=<name>) and any direct single-workflow fetch were returning 404 for global workflows because the GET /api/workflows/:name handler only checked the project directory and bundled defaults — it never looked at ~/.archon/workflows/. Add a second lookup step (between project and bundled) that reads from getHomeWorkflowsPath(), matching the three-tier priority already used by the list endpoint (discoverWorkflowsWithConfig): bundled < global < project. Add three tests: global workflow is returned with source:'global', global overrides a bundled default of the same name, and project takes priority over global when cwd is provided. Closes coleam00#1138 (complementary to coleam00#1257 which fixes the list endpoint).
📝 WalkthroughWalkthroughThe GET /api/workflows/:name handler now checks a home-scoped global workflows directory (ARCHON_HOME) (direct files and one-level subdirs) between project and bundled fallbacks; valid global YAML returns Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.5)
participant Client
end
rect rgba(220,255,200,0.5)
participant Server
end
rect rgba(255,240,200,0.5)
participant FS as "Filesystem (Project / ARCHON_HOME)"
end
rect rgba(240,220,255,0.5)
participant Bundled as "Bundled Workflows"
end
Client->>Server: GET /api/workflows/:name
Server->>FS: check project .archon/workflows/:name.yaml
alt project file found & valid
FS-->>Server: file contents
Server->>Client: 200 { source: "project", workflow }
else
Server->>FS: iterate ARCHON_HOME/workflows candidates (direct + 1-level subdirs)
alt global file found
FS-->>Server: file contents
alt parse succeeds
Server->>Client: 200 { source: "global", filename, workflow }
else parse fails
Server->>Client: 500 { error: "Global workflow file is invalid ..." }
end
else no global found
Server->>Bundled: load default workflow
Bundled-->>Server: bundled workflow
Server->>Client: 200 { source: "bundled", workflow }
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/routes/api.ts`:
- Around line 2264-2284: The global workflow lookup only checks
join(getHomeWorkflowsPath(), filename) and must also probe one-level subfolders
like join(getHomeWorkflowsPath(), folder, filename) to mirror discovery; update
the block that reads globalFilePath (and its error handling that uses
parseWorkflow, apiError, getLog().error and workflow.fetch_global_failed) to
first try the direct path and if missing iterate the immediate child directories
under getHomeWorkflowsPath() to look for filename, parse and return the workflow
with source: 'global' when found (or alternatively delegate to the shared
workflow resolver if available) before falling back to bundled defaults.
In `@packages/server/src/routes/api.workflows.test.ts`:
- Around line 256-343: Each test that sets process.env.ARCHON_HOME (the three
tests named "returns global workflow with source:global when file exists in
~/.archon/workflows/", "global workflow takes priority over bundled defaults",
and "project workflow takes priority over global workflow") should capture the
original value before mutating it and restore it in the finally block instead of
simply deleting it; specifically, store const previousArchonHome =
process.env.ARCHON_HOME before assigning process.env.ARCHON_HOME =
testArchonHome, and in each finally block restore by setting
process.env.ARCHON_HOME = previousArchonHome if it was defined or delete
process.env.ARCHON_HOME if previousArchonHome is undefined, keeping the existing
cleanup of test directories (testArchonHome/testProjectDir) unchanged.
🪄 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: 6142d8fd-6983-47b7-9b14-1d73975be5a4
📒 Files selected for processing (2)
packages/server/src/routes/api.tspackages/server/src/routes/api.workflows.test.ts
… tests Address CodeRabbit review comments on coleam00#1366: - api.ts: mirror discovery's 1-level subfolder support — after checking the direct path, iterate immediate child directories under getHomeWorkflowsPath() to find ~/.archon/workflows/group/foo.yaml. Adds readdir/stat imports from fs/promises. - api.workflows.test.ts: capture and restore the original ARCHON_HOME value in all three global-workflow tests instead of unconditionally deleting it. Adds a fourth test covering subfolder resolution.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/routes/api.ts`:
- Around line 2268-2281: The broad try/catch around readdir and the inner
try/catch around stat currently swallow all filesystem errors; change both to
inspect the caught Error (use err.code) and only ignore ENOENT (directory/not
found) while rethrowing any other errors (e.g., EACCES, EPERM, ENOTDIR) so they
surface; specifically update the outer block around readdir(globalBase) and the
inner block around stat(entryPath) that populate globalCandidates (referencing
globalBase, globalCandidates, filename, entryPath, readdir, stat) to check
error.code === 'ENOENT' and continue/return safely for that case, but throw the
original error for all other codes and add a brief comment explaining the
intentional ENOENT fallback.
🪄 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: c5a5b40c-b69b-4a5d-a43b-f6fd9868be1a
📒 Files selected for processing (2)
packages/server/src/routes/api.tspackages/server/src/routes/api.workflows.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/server/src/routes/api.ts (1)
2264-2308: Global lookup logic is correct — optional efficiency tweak available.Priority ordering (
bundled < global < project), 1-level subfolder support,ENOENT-only swallowing with clear inline comments, and thesource: 'global'label all match discovery semantics. Past concerns (subfolder support, silent error swallowing) are addressed.One optional refactor:
readdirwithwithFileTypes: trueremoves the per-entrystatround-trip and collapses the inner try/catch, sinceDirent.isDirectory()doesn't fail mid-iteration the way a separatestatcall can race against the entry being deleted.♻️ Optional refactor
- const entries = await readdir(globalBase); - for (const entry of entries) { - const entryPath = join(globalBase, entry); - try { - const entryStat = await stat(entryPath); - if (entryStat.isDirectory()) globalCandidates.push(join(entryPath, filename)); - } catch (err) { - if ((err as NodeJS.ErrnoException).code !== 'ENOENT') { - getLog().error({ err, entryPath, name }, 'workflow.global_entry_stat_failed'); - return apiError(c, 500, 'Failed to inspect global workflow directory'); - } - // Entry disappeared between readdir and stat — safe to skip. - } - } + const entries = await readdir(globalBase, { withFileTypes: true }); + for (const entry of entries) { + if (entry.isDirectory()) { + globalCandidates.push(join(globalBase, entry.name, filename)); + } + }If you keep this change, the
statimport on line 9 becomes unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/routes/api.ts` around lines 2264 - 2308, Replace the per-entry stat round-trip in the global workflow discovery loop by calling readdir(globalBase, { withFileTypes: true }) and use Dirent.isDirectory() to detect subfolders (update the loop that builds globalCandidates from globalBase/entries), removing the inner try/catch around stat/entryPath and eliminating the entryStat/entryPath variables; also remove the now-unused stat import if it becomes unused. Ensure the outer ENOENT handling for readdir stays the same and keep the subsequent readFile/parseWorkflow logic untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/server/src/routes/api.ts`:
- Around line 2264-2308: Replace the per-entry stat round-trip in the global
workflow discovery loop by calling readdir(globalBase, { withFileTypes: true })
and use Dirent.isDirectory() to detect subfolders (update the loop that builds
globalCandidates from globalBase/entries), removing the inner try/catch around
stat/entryPath and eliminating the entryStat/entryPath variables; also remove
the now-unused stat import if it becomes unused. Ensure the outer ENOENT
handling for readdir stays the same and keep the subsequent
readFile/parseWorkflow logic untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc3ee12b-2712-4be3-91d6-928b671c4326
📒 Files selected for processing (1)
packages/server/src/routes/api.ts
|
Hi @marciotoze — thanks for opening this PR. This repository uses a PR template at
Could you fill those out (even briefly)? The template helps reviewers understand scope, risk, and rollback — it speeds up review significantly. If a section genuinely doesn't apply, just write "N/A" in it rather than leaving it blank. |
|
@marciotoze related to #1138 — overlapping area or partial fix. |
Problem
The workflow builder (
/workflows/builder?edit=<name>) and any direct fetch of a single workflow by name return 404 for global workflows placed in~/.archon/workflows/.The
GET /api/workflows/:namehandler looked in two places only:<cwd>/.archon/workflows/<name>.yaml(project)It never checked
~/.archon/workflows/, so opening a global workflow in the builder always failed silently.This is complementary to #1257 (which fixes
GET /api/workflowslist + command-handler), but that PR does not touch the single-fetch endpoint — users whose workflows appear in the list still can't open them in the builder.Fix
Add a second lookup step between project and bundled that reads from
getHomeWorkflowsPath()(~/.archon/workflows/), matching the three-tier priority already enforced bydiscoverWorkflowsWithConfig:Files changed:
packages/server/src/routes/api.tsgetHomeWorkflowsPath; add global lookup step inGET /api/workflows/:namepackages/server/src/routes/api.workflows.test.tsTests
Three new test cases in
describe('GET /api/workflows/:name'):source:'global'— workflow in~/.archon/workflows/is found and returned with correct sourcecwdis providedValidation
bun run validate # type-check ✅ lint ✅ format ✅ tests ✅Linked issue
#1138 (complementary path — single-fetch endpoint not covered by #1257)
Summary by CodeRabbit
New Features
Tests