Skip to content

fix: pass globalSearchPath to command handler workflow discovery#1136

Closed
jonasvanderhaegen wants to merge 1 commit intocoleam00:mainfrom
jonasvanderhaegen:fix/command-handler-global-workflow-discovery
Closed

fix: pass globalSearchPath to command handler workflow discovery#1136
jonasvanderhaegen wants to merge 1 commit intocoleam00:mainfrom
jonasvanderhaegen:fix/command-handler-global-workflow-discovery

Conversation

@jonasvanderhaegen
Copy link
Copy Markdown
Contributor

Problem

The command handler (packages/core/src/handlers/command-handler.ts) calls discoverWorkflowsWithConfig without globalSearchPath. This means workflows in ~/.archon/.archon/workflows/ are invisible to webhook-triggered /workflow run and /workflow list commands.

The CLI (cli.ts:125) and orchestrator (orchestrator-agent.ts:385) already pass globalSearchPath: getArchonHome(). The command handler was the only caller that didn't.

Symptom: @archon /workflow run my-custom-workflow posted as a GitHub comment returns "Workflow not found", even though archon workflow run my-custom-workflow via CLI works.

Fix

Pass { globalSearchPath: getArchonHome() } to all three discoverWorkflowsWithConfig calls in command-handler.ts (list, reload, run).

Changes

  • Import getArchonHome from @archon/paths (already exported)
  • 3 call sites updated (lines 566, 612, 807)

4 lines changed.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 12, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 66d8eaa7-e732-461b-ad60-76d67bd9eaa4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

jonasvanderhaegen added a commit to skylence-be/test-sky-workflows that referenced this pull request Apr 12, 2026
Workflows are reusable across repos and live in ~/.archon/.archon/workflows/.
The Archon patch (coleam00/Archon#1136) enables webhook discovery from
the global path, so repo copies are no longer needed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jonasvanderhaegen added a commit to skylence-be/test-sky-workflows that referenced this pull request Apr 12, 2026
Workflows are reusable across repos and live in ~/.archon/.archon/workflows/.
The Archon patch (coleam00/Archon#1136) enables webhook discovery from
the global path, so repo copies are no longer needed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The command handler and web UI API endpoint call discoverWorkflowsWithConfig
without globalSearchPath, making workflows in ~/.archon/.archon/workflows/
invisible to webhook commands and the web UI.

Fix: pass { globalSearchPath: getArchonHome() } to:
- command-handler.ts: list, reload, run (3 calls)
- api.ts: GET /api/workflows (1 call)

The CLI and orchestrator already pass this correctly.
@jonasvanderhaegen jonasvanderhaegen force-pushed the fix/command-handler-global-workflow-discovery branch from 44b80c3 to f4b3163 Compare April 12, 2026 19:57
fcamblor added a commit to fcamblor/Archon that referenced this pull request Apr 16, 2026
Problem: Web UI was blind to workflows in ~/.archon/.archon/workflows/
because GET /api/workflows called discoverWorkflowsWithConfig without
a third argument, so the global search path was never consulted.

Fix: pass { globalSearchPath: getArchonHome() } as third argument,
aligning with CLI (cli/workflow.ts:123) and orchestrator
(orchestrator-agent.ts:386).

Also fix missing `resolve` import in isolation/worktree.ts that caused
type-check and lint errors (introduced in coleam00#1034).

Ref: completes coleam00#1138 (PR coleam00#1136 only covered command-handler/webhook).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Wirasm
Copy link
Copy Markdown
Collaborator

Wirasm commented Apr 17, 2026

Related to #1138 — overlapping area or partial fix.

@Wirasm
Copy link
Copy Markdown
Collaborator

Wirasm commented Apr 20, 2026

Superseded by #1315 — a broader primitive refactor that eliminates the globalSearchPath option entirely (instead of threading it through every call site). The refactor moves ~/.archon/.archon/workflows/ to ~/.archon/workflows/ (direct child, matching the workspaces/ / archon.db convention), adds home-scoped commands and scripts as first-class capabilities with the same discovery semantics, and fixes the latent bug where commands in subfolders were listed but unresolvable.

The specific bug this PR fixes — webhook /workflow run missing home-scoped workflows — is closed by #1315 since home-scope is now read unconditionally by every caller. Your tactical fix directly motivated the refactor; you're credited via Co-authored-by: on the replacement commit.

Closing this in favor of #1315. Thanks for surfacing the bug class.

@Wirasm Wirasm closed this Apr 20, 2026
Wirasm added a commit that referenced this pull request Apr 20, 2026
… drop globalSearchPath (closes #1136) (#1315)

* feat(paths,workflows): unify ~/.archon/{workflows,commands,scripts} + drop globalSearchPath

Collapses the awkward `~/.archon/.archon/workflows/` convention to a direct
`~/.archon/workflows/` child (matching `workspaces/`, `archon.db`, etc.), adds
home-scoped commands and scripts with the same loading story, and kills the
opt-in `globalSearchPath` parameter so every call site gets home-scope for free.

Closes #1136 (supersedes @jonasvanderhaegen's tactical fix — the bug was the
primitive itself: an easy-to-forget parameter that five of six call sites on
dev dropped).

Primitive changes:

- Home paths are direct children of `~/.archon/`. New helpers in `@archon/paths`:
  `getHomeWorkflowsPath()`, `getHomeCommandsPath()`, `getHomeScriptsPath()`,
  and `getLegacyHomeWorkflowsPath()` (detection-only for migration).
- `discoverWorkflowsWithConfig(cwd, loadConfig)` reads home-scope internally.
  The old `{ globalSearchPath }` option is removed. Chat command handler, Web
  UI workflow picker, orchestrator resolve path — all inherit home-scope for
  free without maintainer patches at every new site.
- `discoverScriptsForCwd(cwd)` merges home + repo scripts (repo wins on name
  collision). dag-executor and validator use it; the hardcoded
  `resolve(cwd, '.archon', 'scripts')` single-scope path is gone.
- Command resolution is now walked-by-basename in each scope. `loadCommand`
  and `resolveCommand` walk 1 subfolder deep and match by `.md` basename, so
  `.archon/commands/triage/review.md` resolves as `review` — closes the
  latent bug where subfolder commands were listed but unresolvable.
- All three (`workflows/`, `commands/`, `scripts/`) enforce a 1-level
  subfolder cap (matches the existing `defaults/` convention). Deeper
  nesting is silently skipped.
- `WorkflowSource` gains `'global'` alongside `'bundled'` and `'project'`.
  Web UI node palette shows a dedicated "Global (~/.archon/commands/)"
  section; badges updated.

Migration (clean cut — no fallback read):

- First use after upgrade: if `~/.archon/.archon/workflows/` exists, Archon
  logs a one-time WARN per process with the exact `mv` command:
  `mv ~/.archon/.archon/workflows ~/.archon/workflows && rmdir ~/.archon/.archon`
  The legacy path is NOT read — users migrate manually. Rollback caveat
  noted in CHANGELOG.

Tests:

- `@archon/paths/archon-paths.test.ts`: new helper tests (default HOME,
  ARCHON_HOME override, Docker), plus regression guards for the double-`.archon/`
  path.
- `@archon/workflows/loader.test.ts`: home-scoped workflows, precedence,
  subfolder 1-depth cap, legacy-path deprecation warning fires exactly once
  per process.
- `@archon/workflows/validator.test.ts`: home-scoped commands + subfolder
  resolution.
- `@archon/workflows/script-discovery.test.ts`: depth cap + merge semantics
  (repo wins, home-missing tolerance).
- Existing CLI + orchestrator tests updated to drop `globalSearchPath`
  assertions.

E2E smoke (verified locally, before cleanup):

- `.archon/workflows/e2e-home-scope.yaml` + scratch repo at /tmp
- Home-scoped workflow discovered from an unrelated git repo
- Home-scoped script (`~/.archon/scripts/*.ts`) executes inside a script node
- 1-level subfolder workflow (`~/.archon/workflows/triage/*.yaml`) listed
- Legacy path warning fires with actionable `mv` command; workflows there
  are NOT loaded

Docs: `CLAUDE.md`, `docs-web/guides/global-workflows.md` (full rewrite for
three-type scope + subfolder convention + migration), `docs-web/reference/
configuration.md` (directory tree), `docs-web/reference/cli.md`,
`docs-web/guides/authoring-workflows.md`.

Co-authored-by: Jonas Vanderhaegen <7755555+jonasvanderhaegen@users.noreply.github.com>

* test(script-discovery): normalize path separators in mocks for Windows

The 4 new tests in `scanScriptDir depth cap` and `discoverScriptsForCwd —
merge repo + home with repo winning` compared incoming mock paths with
hardcoded forward-slash strings (`if (path === '/scripts/triage')`). On
Windows, `path.join('/scripts', 'triage')` produces `\scripts\triage`, so
those branches never matched, readdir returned `[]`, and the tests failed.

Added a `norm()` helper at module scope and wrapped the incoming `path`
argument in every `mockImplementation` before comparing. Stored paths go
through `normalizeSep()` in production code, so the existing equality
assertions on `script.path` remain OS-independent.

Fixes Windows CI job `test (windows-latest)` on PR #1315.

* address review feedback: home-scope error handling, depth cap, and tests

Critical fixes:
- api.ts: add `maxDepth: 1` to all 3 findMarkdownFilesRecursive calls in
  GET /api/commands (bundled/home/project). Without this the UI palette
  surfaced commands from deep subfolders that the executor (capped at 1)
  could not resolve — silent "command not found" at runtime.
- validator.ts: wrap home-scope findMarkdownFilesRecursive and
  resolveCommandInDir calls in try/catch so EACCES/EPERM on
  ~/.archon/commands/ doesn't crash the validator with a raw filesystem
  error. ENOENT still returns [] via the underlying helper.

Error handling fixes:
- workflow-discovery.ts: maybeWarnLegacyHomePath now sets the
  "warned-once" flag eagerly before `await access()`, so concurrent
  discovery calls (server startup with parallel codebase resolution)
  can't double-warn. Non-ENOENT probe errors (EACCES/EPERM) now log at
  WARN instead of DEBUG so permission issues on the legacy dir are
  visible in default operation.
- dag-executor.ts: wrap discoverScriptsForCwd in its own try/catch so
  an EACCES on ~/.archon/scripts/ routes through safeSendMessage /
  logNodeError with a dedicated "failed to discover scripts" message
  instead of being mis-attributed by the outer catch's
  "permission denied (check cwd permissions)" branch.

Tests:
- load-command-prompt.test.ts (new): 6 tests covering the executor's
  command resolution hot path — home-scope resolves when repo misses,
  repo shadows home, 1-level subfolder resolvable by basename, 2-level
  rejected, not-found, empty-file. Runs in its own bun test batch.
- archon-paths.test.ts: add getHomeScriptsPath describe block to match
  the existing getHomeCommandsPath / getHomeWorkflowsPath coverage.

Comment clarity:
- workflow-discovery.ts: MAX_DISCOVERY_DEPTH comment now leads with the
  actual value (1) before describing what 0 would mean.
- script-discovery.ts: copy the "routing ambiguity" rationale from
  MAX_DISCOVERY_DEPTH to MAX_SCRIPT_DISCOVERY_DEPTH.

Cleanup:
- Remove .archon/workflows/e2e-home-scope.yaml — one-off smoke test that
  would ship permanently in every project's workflow list. Equivalent
  coverage exists in loader.test.ts.

Addresses all blocking and important feedback from the multi-agent
review on PR #1315.

---------

Co-authored-by: Jonas Vanderhaegen <7755555+jonasvanderhaegen@users.noreply.github.com>
@Wirasm Wirasm mentioned this pull request Apr 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants