fix(server): PUT/DELETE /api/workflows/{name} miss home-scoped (~/.archon/workflows/)#1525
fix(server): PUT/DELETE /api/workflows/{name} miss home-scoped (~/.archon/workflows/)#1525blankse wants to merge 1 commit into
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 workflow API endpoints now implement tiered workflow discovery: GET reads from project-scoped, then home-scoped (~/.archon/workflows/), returning ChangesWorkflow Route Implementation
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: 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.workflows.test.ts`:
- Around line 256-293: The test mutates process.env.ARCHON_HOME and the finally
block unconditionally deletes it; capture the original value before setting
ARCHON_HOME (e.g., const prevArchonHome = process.env.ARCHON_HOME), set
process.env.ARCHON_HOME = testArchonHome for the test, and in the finally block
restore it (process.env.ARCHON_HOME = prevArchonHome or delete if prevArchonHome
is undefined) instead of always deleting; apply the same pattern to the other
affected tests (the blocks around lines 295-334, 397-437, 537-569) to avoid
leaking environment state.
🪄 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: d109f173-1b47-4e93-b7d3-8e8bbd72cb68
📒 Files selected for processing (2)
packages/server/src/routes/api.tspackages/server/src/routes/api.workflows.test.ts
| test('returns home-scoped workflow with source:global when only ~/.archon/workflows/ has it', async () => { | ||
| // Regression for #1524: home-scoped workflows are listed by the | ||
| // dashboard but were unfetchable via GET, breaking the run detail page. | ||
| const testArchonHome = join(tmpdir(), `archon-home-get-${Date.now()}`); | ||
| const homeWorkflowDir = join(testArchonHome, 'workflows'); | ||
| await mkdir(homeWorkflowDir, { recursive: true }); | ||
| await writeFile( | ||
| join(homeWorkflowDir, 'team-shared.yaml'), | ||
| 'name: team-shared\ndescription: shared\nnodes:\n - id: n\n command: c\n' | ||
| ); | ||
| process.env.ARCHON_HOME = testArchonHome; | ||
|
|
||
| try { | ||
| const app = createTestApp(); | ||
| registerApiRoutes(app, {} as WebAdapter, {} as ConversationLockManager); | ||
|
|
||
| // No project-scoped match; falls through to home-scoped. | ||
| mockListCodebases.mockImplementationOnce(async () => []); | ||
| mockParseWorkflow.mockReturnValueOnce({ | ||
| workflow: makeTestWorkflow({ name: 'team-shared', description: 'shared' }), | ||
| error: null, | ||
| }); | ||
|
|
||
| const response = await app.request('/api/workflows/team-shared'); | ||
| expect(response.status).toBe(200); | ||
| const body = (await response.json()) as { | ||
| source: string; | ||
| filename: string; | ||
| workflow: { name: string }; | ||
| }; | ||
| expect(body.source).toBe('global'); | ||
| expect(body.filename).toBe('team-shared.yaml'); | ||
| expect(body.workflow.name).toBe('team-shared'); | ||
| } finally { | ||
| delete process.env.ARCHON_HOME; | ||
| await rm(testArchonHome, { recursive: true, force: true }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Restore any pre-existing ARCHON_HOME instead of always deleting it.
These tests mutate a process-global env var, but finally always deletes it. If the runner already had ARCHON_HOME set, later tests inherit the wrong environment and the suite becomes order-dependent.
Suggested fix
- process.env.ARCHON_HOME = testArchonHome;
+ const previousArchonHome = process.env.ARCHON_HOME;
+ process.env.ARCHON_HOME = testArchonHome;
try {
// ...
} finally {
- delete process.env.ARCHON_HOME;
+ if (previousArchonHome === undefined) {
+ delete process.env.ARCHON_HOME;
+ } else {
+ process.env.ARCHON_HOME = previousArchonHome;
+ }
await rm(testArchonHome, { recursive: true, force: true });
}As per coding guidelines, **/*.test.ts: "keep tests deterministic — no flaky timing or network dependence without guardrails".
Also applies to: 295-334, 397-437, 537-569
🤖 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 256 - 293, The
test mutates process.env.ARCHON_HOME and the finally block unconditionally
deletes it; capture the original value before setting ARCHON_HOME (e.g., const
prevArchonHome = process.env.ARCHON_HOME), set process.env.ARCHON_HOME =
testArchonHome for the test, and in the finally block restore it
(process.env.ARCHON_HOME = prevArchonHome or delete if prevArchonHome is
undefined) instead of always deleting; apply the same pattern to the other
affected tests (the blocks around lines 295-334, 397-437, 537-569) to avoid
leaking environment state.
5be4214 to
4ad107f
Compare
|
Rebased onto current Type-check + all 68 tests in |
…ingle-workflow REST endpoints
Background
`GET /api/workflows` (list) reads home-scoped workflows correctly via
`discoverWorkflowsWithConfig` — files placed under `~/.archon/workflows/`
appear in the dashboard, run successfully, and persist across projects
exactly as documented in CLAUDE.md (`bundled < global < project`).
The single-workflow REST endpoints (`GET`, `PUT`, `DELETE
/api/workflows/{name}`) never got the memo. Their lookup ladder skips
straight from project-scoped to bundled, so any home-scoped workflow
returns 404 from `GET`. The Web UI's run-detail page calls `getWorkflow`
to render the DAG graph; the 404 leaves it stuck on "Loading graph…"
forever.
`PUT` and `DELETE` had a related but distinct bug: when no codebase is
registered they fell back to `workingDir = getArchonHome()` and joined
the workflow folder as `<archonHome>/.archon/workflows/` — which is the
legacy pre-0.x location whose only remaining purpose is to emit the
migration warning. New workflows created from the dashboard with no
project landed there silently and stayed invisible to discovery.
Closes coleam00#1524.
Change
1. `GET /api/workflows/{name}` gains a step 2 between project-scoped and
bundled: read `getHomeWorkflowsPath()/<name>.yaml` and return
`source: 'global'` on hit. Mirrors the discovery layer's documented
priority. ENOENT falls through; any other error surfaces as 500.
2. `PUT /api/workflows/{name}`: when no `cwd` and no codebase exists,
write to `getHomeWorkflowsPath()` directly (`source: 'global'`)
instead of joining `getArchonHome()` with the project workflow
folder. Project-scoped saves are unchanged.
3. `DELETE /api/workflows/{name}`: same correction as PUT — operate on
`getHomeWorkflowsPath()` for the no-project case.
The `WorkflowSource` type already includes `'global'`; the discovery
code already labels home-scoped workflows that way. The endpoints now
agree with the rest of the system.
Tests
- New: GET returns `source: 'global'` for a home-scoped file.
- New: GET project-shadows-home (regression cover for the documented
priority).
- New: DELETE removes a real home-scoped file when no project is
registered, and verifies the file actually disappears from
`~/.archon/workflows/`.
- Updated: the existing "fall back to getArchonHome() when no cwd"
PUT test now asserts `source: 'global'` and verifies the file lands
at `~/.archon/workflows/<name>.yaml`, not at the legacy
`<archonHome>/.archon/workflows/...` path.
- Tests pull `ARCHON_HOME` via `process.env`, matching the existing
pattern in this file.
Out of scope
`WorkflowExecution.tsx` shows "Loading graph…" without a timeout or
error path when the GET returns anything other than 200 — flagged in
4ad107f to
d4d0394
Compare
Review SummaryVerdict: ready-to-merge Clean fix for the fallback-path regression in PUT and DELETE workflow routes. Both handlers were silently writing to Blocking issuesNone. Suggested fixesNone — all remaining items are optional. Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality. |
|
@blankse — first, real thank-you for this work. You correctly diagnosed that PUT and DELETE were missing the home-scoped (`~/.archon/workflows/`) layer that GET had been fixed for separately in #1405, and you kept this PR focused tightly on that gap with good test coverage and a rebase note that made the scope crystal-clear. I'm closing this in favor of #1557 (@guanghuang), which lands the same server fix plus the Web UI integration that lets users actually exercise the capability from the workflow builder (`source` query parameter end-to-end through the React side, not just the API). The two PRs converged on the same underlying problem but with different API contracts:
I went with #1557's explicit-parameter approach because (a) it matches the precedent for `source` in `GET /api/workflows` responses, and (b) the explicit contract avoids ambiguity when a workflow exists in both `project` and `global` locations (which one wins on PUT?). Your test coverage on the home-scoped GET fallback that landed via the rebase is in dev now. Thank you for the careful diagnosis, and please don't read this as wasted effort — your work confirmed the bug existed and motivated the wider fix. |
Summary
PUTandDELETE /api/workflows/{name}skip the home-scoped layer (~/.archon/workflows/). Saving a workflow from the dashboard with no project selected lands the YAML in~/.archon/.archon/workflows/— the legacy pre-0.x path the migration warning explicitly tells users to abandon. Deleting a home-scoped workflow via the API silently fails (404) because the handler looks in the wrong directory.bundled < global < project; the write endpoints were the last layer still walkingproject → bundledand missing the home tier.PUT /api/workflows/{name}: when nocwdand no registered codebases exist, write togetHomeWorkflowsPath()(=~/.archon/workflows/) instead of falling through to<archonHome>/.archon/workflows/. Response includessource: 'global'for the home-tier case ('project'unchanged).DELETE /api/workflows/{name}: mirrors PUT's resolution — home-scoped path when no project is registered, project path otherwise.api.workflows.test.tscovering: PUT falls back to home-scoped, PUT into project unchanged, DELETE removes home-scoped, DELETE returns 404 cleanly when missing.GEThandler (= fix(server): GET /api/workflows/:name missed home-scoped workflows #1405's territory), bundled-default fallback order,WorkflowSourcetype, discovery layer, any client-facing shape beyond the newsource: 'global'case on PUT.Closes #1524.
Test plan
bun --filter @archon/server type-checkpassesbun --filter @archon/server test packages/server/src/routes/api.workflows.test.ts— all 68 tests pass (4 new)~/.archon/workflows/<name>.yamlSummary by CodeRabbit