fix(server,workflows,web): surface bundled defaults on /api/workflows when no project context#1618
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughWhen no project is selected, the ChangesSupport bundled workflows when no project context is selected
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Possibly related issues
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/workflows/src/loader.test.ts (1)
2531-2539: ⚡ Quick winMake the “still loads bundled defaults” assertion explicit.
This test currently only re-checks that project scope is skipped. It should also assert bundled-sourced workflows exist, otherwise it can pass even if bundled loading breaks.
✅ Minimal assertion addition
const result = await discoverWorkflows(null, { loadDefaults: true }); // No project-source entries (project step skipped). const projectSourced = result.workflows.filter(w => w.source === 'project'); expect(projectSourced).toHaveLength(0); + const bundledSourced = result.workflows.filter(w => w.source === 'bundled'); + expect(bundledSourced.length).toBeGreaterThan(0);🤖 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/loader.test.ts` around lines 2531 - 2539, The test only asserts no project-sourced workflows but doesn't verify bundled defaults loaded; update the test that calls discoverWorkflows(null, { loadDefaults: true }) to also filter result.workflows for entries where w.source === 'bundled' (or the exact bundled source label used by the loader) and assert that array has length > 0 (e.g., expect(bundledSourced).toHaveLengthGreaterThan(0) or expect(bundledSourced.length).toBeGreaterThan(0)) so the bundled-defaults loader is explicitly validated.
🤖 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 `@packages/web/src/components/workflows/WorkflowList.tsx`:
- Around line 187-193: The empty-state hint in the WorkflowList component
currently tells users to check defaults.loadDefaultWorkflows which is misleading
when there is no project; update the JSX copy in WorkflowList so it clarifies
that when no project is present the app still attempts bundled defaults
automatically and that users should either create/select a project or check
global configuration (e.g., defaults.loadDefaultWorkflows) if bundled defaults
are missing—locate the message text in WorkflowList and replace it with wording
that explicitly differentiates the “no project” path from project-specific
config.
---
Nitpick comments:
In `@packages/workflows/src/loader.test.ts`:
- Around line 2531-2539: The test only asserts no project-sourced workflows but
doesn't verify bundled defaults loaded; update the test that calls
discoverWorkflows(null, { loadDefaults: true }) to also filter result.workflows
for entries where w.source === 'bundled' (or the exact bundled source label used
by the loader) and assert that array has length > 0 (e.g.,
expect(bundledSourced).toHaveLengthGreaterThan(0) or
expect(bundledSourced.length).toBeGreaterThan(0)) so the bundled-defaults loader
is explicitly validated.
🪄 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: 793baad8-8f8e-4c03-a45a-ab262adedd64
📒 Files selected for processing (5)
packages/server/src/routes/api.tspackages/server/src/routes/api.workflows.test.tspackages/web/src/components/workflows/WorkflowList.tsxpackages/workflows/src/loader.test.tspackages/workflows/src/workflow-discovery.ts
Review SummaryVerdict: minor-fixes-needed Your PR surfaces bundled and home-scoped workflows when no project is registered, fixing the broken first-run experience where users saw an empty workflow list before any codebase was added. The three changed files implement this cleanly and consistently. A few targeted additions will close the test and docs gaps. Blocking issues(none — no CRITICAL or HIGH findings) Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, test-coverage, comment-quality, docs-impact. |
- api.md: document cwd-omitted behavior so the empty-state case is discoverable - workflow-discovery.ts: docstring explains loadDefaults default rather than just naming the skipped branch - api.workflows.test.ts: mockDiscoverWorkflows accepts string | null to match the wider signature - api.ts: drop trailing period inside the multi-line inline comment - CHANGELOG.md: add the coleam00#1173 Fixed entry under [Unreleased] - loader.test.ts: add a regression test that asserts loadConfig is not invoked when cwd is null
|
Addressed in 018928c:
The "verify null-cwd path was exercised" sub-point of (1) was already covered: the existing test at L2531-2539 filters |
… when no project context (coleam00#1173) GET /api/workflows short-circuits to an empty array when there is no `cwd` query param and no registered codebases. The handler never reaches discovery, so bundled defaults are not surfaced and the UI renders a misleading "Add workflow definitions to .archon/workflows/" empty state on first run — even though the bundled YAML files are present on disk. This change: - Threads `cwd: string | null` through `discoverWorkflows` and `discoverWorkflowsWithConfig`. When `cwd` is `null` the discovery function loads bundled + home scopes and skips the project step cleanly (no path-join with an empty cwd, no read-error noise). - Removes the early-return in the GET handler. When no project context exists, it now calls `discoverWorkflowsWithConfig(null, ...)` so the response carries the bundled set instead of `[]`. - Distinguishes the empty-state copy in `WorkflowList` so the rare case where the list is genuinely empty reads correctly. With a project selected: "No workflows found in this project. Add workflow definitions to .archon/workflows/ in the project root." Without a project: "No workflows are available. Bundled defaults should appear here automatically; if they do not, check that `defaults.loadDefaultWorkflows` is enabled in your config." Tests cover both the API (new `falls back to null cwd when no cwd query and no codebases registered` case in `api.workflows.test.ts`) and the discovery layer (new `discoverWorkflows with null cwd` block in `loader.test.ts` asserting no project-source entries and no project-step read errors).
The second test in the null-cwd discovery group only verified that project-source workflows are absent. That assertion would still pass if the bundled-defaults loader silently regressed. Add an explicit `bundled` source-label assertion so the test catches that regression directly.
- api.md: document cwd-omitted behavior so the empty-state case is discoverable - workflow-discovery.ts: docstring explains loadDefaults default rather than just naming the skipped branch - api.workflows.test.ts: mockDiscoverWorkflows accepts string | null to match the wider signature - api.ts: drop trailing period inside the multi-line inline comment - CHANGELOG.md: add the coleam00#1173 Fixed entry under [Unreleased] - loader.test.ts: add a regression test that asserts loadConfig is not invoked when cwd is null
truffle-dev
force-pushed
the
fix/workflows-surface-bundled-defaults-no-cwd
branch
from
018928c to
889aec3
Compare
Review SummaryVerdict: ready-to-merge Solid bug fix. Your change plumbs 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. |
Address Wirasm's polish review on coleam00#1618: - loader.test.ts: assert result.workflows.length === 0 in the loadDefaults:false case so the test no longer passes if bundled defaults are accidentally loaded. - loader.test.ts: drop the inline (issue coleam00#1173) reference and the workflow-discovery.ts file+line pointer from the two comments that risk rotting on future refactor. - api.workflows.test.ts: drop the inline (issue coleam00#1173) reference. - CHANGELOG: append positive framing to the coleam00#1173 line so the entry reads as "what works now" not just "what no longer breaks".
|
|
Thanks for the merge. The two-round review caught two test-assertion gaps that were passing silently: |
… when no project context (coleam00#1618) * fix(server,workflows,web): surface bundled defaults on /api/workflows when no project context (coleam00#1173) GET /api/workflows short-circuits to an empty array when there is no `cwd` query param and no registered codebases. The handler never reaches discovery, so bundled defaults are not surfaced and the UI renders a misleading "Add workflow definitions to .archon/workflows/" empty state on first run — even though the bundled YAML files are present on disk. This change: - Threads `cwd: string | null` through `discoverWorkflows` and `discoverWorkflowsWithConfig`. When `cwd` is `null` the discovery function loads bundled + home scopes and skips the project step cleanly (no path-join with an empty cwd, no read-error noise). - Removes the early-return in the GET handler. When no project context exists, it now calls `discoverWorkflowsWithConfig(null, ...)` so the response carries the bundled set instead of `[]`. - Distinguishes the empty-state copy in `WorkflowList` so the rare case where the list is genuinely empty reads correctly. With a project selected: "No workflows found in this project. Add workflow definitions to .archon/workflows/ in the project root." Without a project: "No workflows are available. Bundled defaults should appear here automatically; if they do not, check that `defaults.loadDefaultWorkflows` is enabled in your config." Tests cover both the API (new `falls back to null cwd when no cwd query and no codebases registered` case in `api.workflows.test.ts`) and the discovery layer (new `discoverWorkflows with null cwd` block in `loader.test.ts` asserting no project-source entries and no project-step read errors). * test(workflows): assert bundled defaults surface when cwd is null The second test in the null-cwd discovery group only verified that project-source workflows are absent. That assertion would still pass if the bundled-defaults loader silently regressed. Add an explicit `bundled` source-label assertion so the test catches that regression directly. * fix(workflows): address review on coleam00#1618 - api.md: document cwd-omitted behavior so the empty-state case is discoverable - workflow-discovery.ts: docstring explains loadDefaults default rather than just naming the skipped branch - api.workflows.test.ts: mockDiscoverWorkflows accepts string | null to match the wider signature - api.ts: drop trailing period inside the multi-line inline comment - CHANGELOG.md: add the coleam00#1173 Fixed entry under [Unreleased] - loader.test.ts: add a regression test that asserts loadConfig is not invoked when cwd is null * test(workflows): tighten null-cwd assertions + drop rot-prone refs Address Wirasm's polish review on coleam00#1618: - loader.test.ts: assert result.workflows.length === 0 in the loadDefaults:false case so the test no longer passes if bundled defaults are accidentally loaded. - loader.test.ts: drop the inline (issue coleam00#1173) reference and the workflow-discovery.ts file+line pointer from the two comments that risk rotting on future refactor. - api.workflows.test.ts: drop the inline (issue coleam00#1173) reference. - CHANGELOG: append positive framing to the coleam00#1173 line so the entry reads as "what works now" not just "what no longer breaks".
2 participants
Summary
GET /api/workflowsreturns{"workflows":[]}when there is nocwdquery param and no registered codebase, even though bundled defaults exist on disk. The Workflows page then renders a misleading empty state telling users to add files to.archon/workflows/./app/.archon/workflows/defaults/.cwd: string | nullthrough the discovery functions; thenullcase loads bundled + home scopes and skips the project step. The GET handler now passesnullinstead of returning[]. The empty-state copy inWorkflowListnow distinguishes "no project selected" from "no workflows in this project".BUNDLED_WORKFLOWSonnamelookup when there is no cwd). The separategetArchonWorkspacesPath()issue mentioned in the issue's follow-up comment is a chat-flow bug, not a workflow-list bug, and is left for another change.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
api.tsGET handlerdiscoverWorkflowsWithConfignullinstead of returning[]earlydiscoverWorkflowscwd === nulldiscoverWorkflowsWithConfigloadConfig(cwd)cwd === null(no per-project config)WorkflowList.tsxlocalProjectIdcli/commands/{validate,workflow}.tsstringis assignable tostring | null— no caller changesLabel Snapshot
risk: lowsize: Sserver,workflows,webserver:routes/api,workflows:workflow-discovery,web:workflowsChange Metadata
bugmultiLinked Issue
Validation Evidence (required)
cli/doctor.test.tscheckWorkspaceWritablefailures (2) and 1workflowRunCommandflake (passes individually, fails in the full suite) reproduce on pristineupstream/devwithout these changes — verified viagit stashround-trip — so they are not caused by this PR.Security Impact (required)
NoNoNoNo—nullcwd skips the only cwd-derivedfs.accesscall (the project step), so file-system reach is reduced, not expanded.Compatibility / Migration
Yes—cwd: stringis assignable tocwd: string | null. All existing callers incli/commands/validate.ts,cli/commands/workflow.ts, and the rest ofapi.tscontinue to pass concrete strings.NoNoHuman Verification (required)
GET /api/workflowswith nocwdand no registered codebases returns the bundled set withsource: 'bundled'(new test).discoverWorkflows(null, { loadDefaults: false })produces zero project-source entries and zero project-step read errors (new test).discoverWorkflows(null, { loadDefaults: true })still loads bundled defaults and skips the project step (new test).cwd=still validates against registered codebases (unchanged 400 path); registered-codebases path still resolves to the first codebase'sdefault_cwd;discoverWorkflowsWithConfigno longer callsloadConfigwhencwd === null(per-project opt-out only makes sense when a project is actually selected).~/.archon/workflows/overrides — the home-scope step is unchanged, so this should behave the same as before.Side Effects / Blast Radius (required)
GET /api/workflows(handler),discoverWorkflowsanddiscoverWorkflowsWithConfig(signature widening),WorkflowListempty-state copy.[]now sees ~20 bundled defaults. That is the intended fix, but it does change the response shape for the no-project path.workflows_discovery_completedinfo-level log now includesscope: 'no_project_context'when the null path is taken, so dashboards can detect the new branch.Rollback Plan (required)
defaults.loadDefaultWorkflows: falseper project, but the no-project path always loads bundled (this is the bug being fixed).workflows_discovery_completedlog showsscope: 'no_project_context'more often than expected.Risks and Mitigations
discoverWorkflowsrequiring a non-nullcwd.cli/commands/validate.ts:90,cli/commands/workflow.ts:176, and the GET handler. All pass concrete strings. The signature widening is purely additive.defaults.loadDefaultWorkflows: falseper project expects the no-project path to also be empty.loadConfigbecause there is no project to read config from. This is documented in the function-level comment. If the user wants to suppress bundled defaults globally, the existingisBinaryBuild()/path mechanism already covers compiled-binary mode.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation