fix(server/workflows): resolve project workflows with .yml extension#1711
fix(server/workflows): resolve project workflows with .yml extension#1711jasonjack-jay wants to merge 1 commit into
Conversation
`GET /api/workflows/:name` and `DELETE /api/workflows/:name` constructed
the on-disk filename as `${name}.yaml` and only tried that one extension.
The discovery scanner in `workflow-discovery.ts` (the source for
`GET /api/workflows`) accepts both `.yaml` and `.yml`, so a project that
named its workflow `phase-0-spike.yml` would appear in the list endpoint
but 404 on the detail endpoint and could not be deleted via the API.
This is the same scope-mismatch class as the trailing-newline / case-sensitivity
bugs that crop up whenever two endpoints derive paths independently — fix it
once at the read site rather than forcing every author to rename files.
Changes:
- GET /api/workflows/:name now probes `${name}.yaml` then `${name}.yml`
in each scope (project, home, default). Extracted as a small inline
helper so the same logic isn't duplicated three times.
- DELETE /api/workflows/:name probes both extensions before returning 404.
- PUT (save) is unchanged: writes continue to use `.yaml` as the
canonical extension for newly-created workflows. Existing `.yml` files
remain readable and deletable.
- Bundled defaults are still keyed on `${name}.yaml` (the suffix is a
synthetic in-memory key there, not a filesystem path).
Tests:
- Added regression: `phase-0-spike.yml` in `<cwd>/.archon/workflows/`
returns 200 with `source: project` and `filename: phase-0-spike.yml`.
- All 36 tests in `api.workflows.test.ts` pass (was 35).
- `tsc --noEmit` on `packages/server` is clean.
📝 WalkthroughWalkthroughThe PR updates workflow file handling to recognize both ChangesWorkflow File Extension Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![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)
280-307: ⚡ Quick winAdd a matching
.ymlregression for DELETE to lock the full fix surface.This PR fixes both GET and DELETE extension handling, but only GET
.ymlis asserted. Add a sibling DELETE.ymltest to prevent regressions.As per coding guidelines, "**/*.{ts,tsx,test.ts}: ... keep tests deterministic with no flaky timing or network dependence without guardrails."Proposed test addition
describe('DELETE /api/workflows/:name', () => { + test('removes existing .yml workflow file and returns deleted:true', async () => { + const testDir = join(tmpdir(), `wf-del-yml-test-${Date.now()}`); + const workflowDir = join(testDir, '.archon', 'workflows'); + await mkdir(workflowDir, { recursive: true }); + await writeFile( + join(workflowDir, 'to-delete-yml.yml'), + 'name: to-delete-yml\ndescription: y\nnodes:\n - id: z\n command: z\n' + ); + + try { + const app = createTestApp(); + registerApiRoutes(app, {} as WebAdapter, {} as ConversationLockManager); + mockListCodebases.mockImplementationOnce(async () => [{ default_cwd: testDir }]); + + const response = await app.request(`/api/workflows/to-delete-yml?cwd=${testDir}`, { + method: 'DELETE', + }); + expect(response.status).toBe(200); + const body = (await response.json()) as { deleted: boolean; name: string }; + expect(body.deleted).toBe(true); + expect(body.name).toBe('to-delete-yml'); + } finally { + await rm(testDir, { recursive: true, force: true }); + } + });🤖 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/server/src/routes/api.workflows.test.ts` around lines 280 - 307, Add a sibling test mirroring the GET `.yml` case that exercises DELETE handling: create a test named like "returns project workflow when file uses .yml extension (matches discovery) - DELETE", reuse the same setup steps (create testDir, workflowDir, write phase-0-spike.yml), instantiate the app with createTestApp() and registerApiRoutes(app, {} as WebAdapter, {} as ConversationLockManager), mockListCodebases.mockImplementationOnce to return [{ default_cwd: testDir }], call app.request('/api/workflows/phase-0-spike?cwd=' + testDir, { method: 'DELETE' }), and assert the response status is 200 and the response body contains source === 'project' and filename === 'phase-0-spike.yml'; finally, cleanup testDir with rm(...).
🤖 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.
Nitpick comments:
In `@packages/server/src/routes/api.workflows.test.ts`:
- Around line 280-307: Add a sibling test mirroring the GET `.yml` case that
exercises DELETE handling: create a test named like "returns project workflow
when file uses .yml extension (matches discovery) - DELETE", reuse the same
setup steps (create testDir, workflowDir, write phase-0-spike.yml), instantiate
the app with createTestApp() and registerApiRoutes(app, {} as WebAdapter, {} as
ConversationLockManager), mockListCodebases.mockImplementationOnce to return [{
default_cwd: testDir }], call app.request('/api/workflows/phase-0-spike?cwd=' +
testDir, { method: 'DELETE' }), and assert the response status is 200 and the
response body contains source === 'project' and filename ===
'phase-0-spike.yml'; finally, cleanup testDir with rm(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1f98667-c627-4091-870b-3cc8bb220683
📒 Files selected for processing (2)
packages/server/src/routes/api.tspackages/server/src/routes/api.workflows.test.ts
Review SummaryVerdict: minor-fixes-needed Extends the workflow CRUD API to support Blocking issuesNone — no CRITICAL findings. 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. |
2 participants
Summary
GET /api/workflows/:nameandDELETE /api/workflows/:nameonly probed${name}.yamlon disk, but the discovery scanner that backsGET /api/workflowsaccepts both.yamland.yml. A project workflow namedphase-0-spike.ymltherefore appeared in the list endpoint but 404'd on the detail endpoint and could not be deleted via the API.Repro
.archon/workflows/, createphase-0-spike.yml(note the extension) with any valid workflow.GET /api/workflows?cwd=<project>— workflow appears in the list.GET /api/workflows/phase-0-spike?cwd=<project>— 404./workflows/runs/<id>) fetches this endpoint and renders broken for any run whose workflow file uses.yml.Fix
GET /api/workflows/:namenow probes${name}.yamlthen${name}.ymlin each scope (project, home, default). Extracted as a small inline helper to avoid duplicating the same logic three times.DELETE /api/workflows/:nameprobes both extensions before returning 404.PUT(save) is unchanged — writes continue to use.yamlas the canonical extension for newly-created workflows. Existing.ymlfiles remain readable and deletable.${name}.yaml(synthetic in-memory key, not a filesystem path).Tests
phase-0-spike.ymlreturns 200 with `source: project` and `filename: phase-0-spike.yml`.api.workflows.test.tspass (was 35).Summary by CodeRabbit
New Features
.yamland.ymlfile extensions, providing greater flexibility in file naming conventions.Tests
.ymlextensions.