fix(server): GET /api/workflows/:name missed home-scoped workflows#1405
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe single-workflow fetch endpoint now checks for home-scoped workflow files at ChangesWorkflow Discovery Logic
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant ProjectFS as Project FS
participant HomeFS as Home FS
participant Bundled as Bundled Defaults
Client->>Server: GET /api/workflows/:name (optional cwd)
alt cwd provided
Server->>ProjectFS: check for project-scoped workflow
alt project found
ProjectFS-->>Server: return project workflow (source: project)
Server-->>Client: 200 { workflow, source: "project" }
else project not found
Server->>HomeFS: read ~/.archon/workflows/:name.yaml
alt home found & parsed
HomeFS-->>Server: parsed workflow (source: global)
Server-->>Client: 200 { workflow, source: "global" }
else home not found
Bundled-->>Server: return bundled workflow
Server-->>Client: 200 { workflow, source: "bundled" }
end
end
else no cwd
Server->>HomeFS: read ~/.archon/workflows/:name.yaml
alt home found & parsed
HomeFS-->>Server: parsed workflow (source: global)
Server-->>Client: 200 { workflow, source: "global" }
else home not found
Bundled-->>Server: return bundled workflow
Server-->>Client: 200 { workflow, source: "bundled" }
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/server/src/routes/api.workflows.test.ts (1)
293-329: Shadow precedence test — consider strengthening the "home not attempted" assertion.The assertion
body.source === 'project'proves step 1 returned successfully, which implies step 2 was skipped given the current code path — but becauseparseWorkflowis globally mocked to return a fixed workflow regardless of input content, the test cannot actually distinguish whether the home file was opened. If a future refactor were to accidentally read both files, this test would still pass.Optional hardening: spy on
readFile(or inspect logger calls) and assert the home path was not accessed. Not a blocker — the current assertion is still correct for the current implementation.Optional: stricter assertion via readFile spy
+ const readFileSpy = spyOn(await import('fs/promises'), 'readFile'); mockListCodebases.mockImplementationOnce(async () => [{ default_cwd: testDir }]); const response = await app.request(`/api/workflows/shared?cwd=${testDir}`); expect(response.status).toBe(200); const body = (await response.json()) as { source: string }; - // Project must shadow home — home lookup should not even be attempted. expect(body.source).toBe('project'); + // Assert no readFile call targeted the home path + const homePath = join(homeWorkflowsDir, 'shared.yaml'); + expect(readFileSpy.mock.calls.some(args => String(args[0]) === homePath)).toBe(false); + readFileSpy.mockRestore();Requires adding
spyOnto thebun:testimport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/routes/api.workflows.test.ts` around lines 293 - 329, Test currently only checks body.source === 'project' but parseWorkflow is globally mocked so the test can’t detect if the home file was actually read; add a spy on the file reader and assert the home path was never accessed. Specifically, in the test 'project-scope shadows home-scope when same filename exists in both' (where parseWorkflow is mocked), create a spy (via spyOn from bun:test or your test framework) on fs/promises.readFile (or the file-read helper used by the workflows code) before calling the API, then assert the spy was not called with the home workflow path (the join(homeWorkflowsDir, 'shared.yaml') value) and that it was called with the project path if desired; keep restoring/removing the spy in the finally block. This ensures parseWorkflow’s mock can’t mask an unintended read of the home file.
🤖 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.workflows.test.ts`:
- Around line 293-329: Test currently only checks body.source === 'project' but
parseWorkflow is globally mocked so the test can’t detect if the home file was
actually read; add a spy on the file reader and assert the home path was never
accessed. Specifically, in the test 'project-scope shadows home-scope when same
filename exists in both' (where parseWorkflow is mocked), create a spy (via
spyOn from bun:test or your test framework) on fs/promises.readFile (or the
file-read helper used by the workflows code) before calling the API, then assert
the spy was not called with the home workflow path (the join(homeWorkflowsDir,
'shared.yaml') value) and that it was called with the project path if desired;
keep restoring/removing the spy in the finally block. This ensures
parseWorkflow’s mock can’t mask an unintended read of the home file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d8c8821-1f4f-4014-95d0-4835f99f3954
📒 Files selected for processing (2)
packages/server/src/routes/api.tspackages/server/src/routes/api.workflows.test.ts
Address CodeRabbit nitpick on coleam00#1405: `parseWorkflow` is globally mocked, so asserting `body.source === 'project'` alone cannot catch a regression that reads both files before deciding. Add a `spyOn(fs.readFile)` that fails if the home path was ever opened. Restores the spy in finally so other tests aren't affected.
|
Hi @lraphael — 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. |
|
Thanks for the review @Wirasm — added UX Journey, Architecture Diagram, Label Snapshot, and Change Metadata sections. Let me know if any of them need more detail. |
The single-workflow endpoint searched only project-scope, bundled, and filesystem defaults — skipping `~/.archon/workflows/` entirely. This made global workflows invisible to the web UI builder (which loads via this endpoint) even though `GET /api/workflows` (list) surfaced them correctly after PR coleam00#1315. Adds a home-scope lookup between project and bundled tiers, matching `discoverWorkflowsWithConfig`. Uses `source: 'global'` (already in the `WorkflowSource` union). Reproduces the bug: create a workflow in `~/.archon/workflows/`, then open `/workflows/builder?edit=<name>` → 404 "Workflow not found". After this fix: loads correctly with `source: "global"`. Related to coleam00#1138.
Two new tests for GET /api/workflows/:name: - home-scope hit: file in ~/.archon/workflows/, no project match → returns source: 'global' - shadow: same filename in both project and home → project wins, home is not even attempted Uses ARCHON_HOME env var to redirect home lookups to a tmpdir during the test. Restores the prior value in finally so test order doesn't leak state.
Address CodeRabbit nitpick on coleam00#1405: `parseWorkflow` is globally mocked, so asserting `body.source === 'project'` alone cannot catch a regression that reads both files before deciding. Add a `spyOn(fs.readFile)` that fails if the home path was ever opened. Restores the spy in finally so other tests aren't affected.
lraphael
force-pushed
the
fix/get-workflow-home-scope
branch
from
110df38 to
668afda
Compare
|
Hi @Wirasm @coleam00 — gentle bump on this one. I just hit the symptom in current Re: relation to #1138 / #1257: this is complementary, not overlapping.
Both are needed for the web UI to fully work with home-scoped workflows. The investigation in #1138 caught the list path but missed the singular Quick status update:
Thanks! |
Review SummaryVerdict: minor-fixes-needed Your PR cleanly completes the discovery-order mirror for Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality, docs-impact. |
- Shorten home-scope fallback comment to point at `discoverWorkflowsWithConfig` as the canonical discovery anchor; drop historical "previously skipped" and web-UI builder noise. - Drop "(project scope)" / bundled-defaults parentheticals — both restate WHAT the code already says. - Add 500-path test: malformed home-scoped YAML returns 500 with "Home workflow file is invalid" error message.
|
Thanks for the thorough review, @Wirasm — all four points addressed in f6620ea: Suggested fixes
Minor / nice-to-have
Test run: Appreciate the callout on the |
…oleam00#1405) * fix(server): GET /api/workflows/:name missed home-scoped workflows The single-workflow endpoint searched only project-scope, bundled, and filesystem defaults — skipping `~/.archon/workflows/` entirely. This made global workflows invisible to the web UI builder (which loads via this endpoint) even though `GET /api/workflows` (list) surfaced them correctly after PR coleam00#1315. Adds a home-scope lookup between project and bundled tiers, matching `discoverWorkflowsWithConfig`. Uses `source: 'global'` (already in the `WorkflowSource` union). Reproduces the bug: create a workflow in `~/.archon/workflows/`, then open `/workflows/builder?edit=<name>` → 404 "Workflow not found". After this fix: loads correctly with `source: "global"`. Related to coleam00#1138. * test(server): cover home-scope lookup + project/home precedence Two new tests for GET /api/workflows/:name: - home-scope hit: file in ~/.archon/workflows/, no project match → returns source: 'global' - shadow: same filename in both project and home → project wins, home is not even attempted Uses ARCHON_HOME env var to redirect home lookups to a tmpdir during the test. Restores the prior value in finally so test order doesn't leak state. * test(server): harden shadow test — assert home readFile is never called Address CodeRabbit nitpick on coleam00#1405: `parseWorkflow` is globally mocked, so asserting `body.source === 'project'` alone cannot catch a regression that reads both files before deciding. Add a `spyOn(fs.readFile)` that fails if the home path was ever opened. Restores the spy in finally so other tests aren't affected. * review(server): address Wirasm feedback on PR coleam00#1405 - Shorten home-scope fallback comment to point at `discoverWorkflowsWithConfig` as the canonical discovery anchor; drop historical "previously skipped" and web-UI builder noise. - Drop "(project scope)" / bundled-defaults parentheticals — both restate WHAT the code already says. - Add 500-path test: malformed home-scoped YAML returns 500 with "Home workflow file is invalid" error message. --------- Co-authored-by: Raphael Lechner <raphael.l@asix.pro>
2 participants
Summary
GET /api/workflows/:nameskipped the home scope (~/.archon/workflows/), making global workflows invisible to the Web UI builder (which loads through this endpoint).discoverWorkflowsWithConfig. Returnssource: 'global'(already in theWorkflowSourceunion).source: 'global'case.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
GET /api/workflows/:namehandlerreadFileGET /api/workflows/:namehandlergetHomeWorkflowsPath()GET /api/workflows/:namehandlerreadFile~/.archon/workflows/<sanitized-name>.yamlGET /api/workflows/:namehandlerparseWorkflow(home YAML)GET /api/workflows/:namehandlerisValidCommandNameguardLabel Snapshot
risk: lowsize: S(+116 / -3, two files)serverserver:workflowsChange Metadata
bugserverLinked Issue
Validation Evidence (required)
Evidence provided:
api.workflows.test.ts): home-scope hit (source: 'global') and project-over-home shadow precedence withspyOn(readFile)proving the home path is never opened when project wins.dev: placed a workflow in~/.archon/workflows/, hit/workflows/builder?edit=<name>in the UI. Before patch: "Failed to load workflow (404)". After patch: 12-node DAG renders,Validbadge green.Security Impact (required)
readFileon.yamlfiles, different path.readFileof~/.archon/workflows/<sanitized-name>.yaml. The name is already validated byisValidCommandName(path-traversal guard, covered by test at line 189). No directory traversal or user-controlled paths beyond the existing guard.Compatibility / Migration
bundledorprojectbefore keeps the samesourcevalue and body.Human Verification (required)
Verified scenarios:
~/.archon/workflows/→ API returnssource: 'global', UI builder loads it.readFileis never called (spy-asserted).archon-assist) still returnssource: 'bundled'when no project/home match...secret) still 400 before anyreadFile(existing test at line 189, still passes).Edge cases checked:
ENOENTfalls through to bundled (no 500).What was not verified:
/.archon) — only verified the macOS homedir path.getHomeWorkflowsPath()already handles both viagetArchonHome(), so this should be inherited correctly.Side Effects / Blast Radius (required)
GET /api/workflows/:name(single-workflow fetch). No change to LIST, PUT, or any workflow-run path.~/.archon/workflows/<name>.yamlfile with the same name as a bundled default, the home version now takes precedence over bundled (but still loses to project). This matchesdiscoverWorkflowsWithConfigand the LIST endpoint — users already rely on this precedence for the list view, so it aligns the two views.isValidCommandName) still fires first; parse errors return 500 with a distinct log key (workflow.fetch_home_failed).Rollback Plan (required)
api.ts(28 lines); the test additions are non-breaking and can stay or be reverted together.workflow.fetch_home_failedin logs would indicate malformed YAML in a home workflow — user-recoverable by fixing the file.Risks and Mitigations
workflow.fetch_home_failedlog key; existingisValidCommandNameguard unchanged.Summary by CodeRabbit
New Features
Bug Fixes
Tests