fix(orchestrator): use codebase cwd for direct chat provider execution#1233
fix(orchestrator): use codebase cwd for direct chat provider execution#1233
Conversation
📝 WalkthroughWalkthroughThe pull request implements provider Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Comprehensive PR ReviewPR: #1233 SummaryClean, minimal fix. The 9-line Verdict:
🟢 Low Issues (All Optional)1. Silent fallback with no log when codebase lookup fails (reported by all 3 agents)📍 When View suggested fixif (attachedCodebase) {
cwd = conversation.cwd ?? attachedCodebase.default_cwd;
} else {
// Intentional fallback: codebase may have been deleted; run with workspaces root.
getLog().warn(
{ codebaseId: conversation.codebase_id, conversationId },
'orchestrator.codebase_not_found_cwd_fallback'
);
}Matches the existing 2. Missing test for
|
⚡ Self-Fix Report (Aggressive)Status: COMPLETE Fixes Applied (4 total)
View all fixes
Tests Added
Skipped (0)(none — all findings addressed) Suggested Follow-up Issues(none) Validation✅ Type check | ✅ Lint | ✅ Tests (all packages, 0 fail) Self-fix by Archon · aggressive mode · fixes pushed to |
#1179) Direct chat with a codebase-scoped conversation was always passing the Archon workspaces root as cwd to the provider, ignoring the attached project. Now resolves cwd from conversation.cwd (worktree) or codebase.default_cwd, matching workflow execution behavior. Changes: - Resolve cwd from attached codebase in orchestrator-agent.ts - Add 3 tests covering codebase-scoped, worktree, and fallback paths Fixes #1179
…n cwd resolution - Add warn log when codebase lookup fails in the cwd fallback path, matching the pattern used for codebase_env_vars_load_failed one block below - Add test covering the codebase_id-set-but-not-found fallback branch - Reset mockListCodebases in beforeEach to prevent potential mock leaks - Rename misleading cleanup-service test that no longer injects errors
Wirasm
force-pushed
the
archon/task-fix-issue-1179
branch
from
5251cfa to
d2b3ba3
Compare
|
Rebased on current `dev` (the duplicate cleanup-service mock fix dropped out since #1232 landed that independently) and marked ready for review. Not merging yet — flagging a couple of design questions surfaced during review that are worth pinning somewhere before this lands as the accepted pattern. Fix itself: confirmed correct and scoped. Three-tier resolution (`conversation.cwd` → `codebase.default_cwd` → `getArchonWorkspacesPath()`) is the right shape. `codebases.find()` is free (already loaded at line 734 for prompt construction, no extra DB call). Tests cover all four branches (worktree, default_cwd, no codebase, codebase deleted). Design questions to discuss (not blocking this PR)1. `conversation.cwd` doing double duty`conversation.cwd` is set by workflow runs (as the worktree path) and now observably read by direct chat (as the provider cwd). That coupling is convenient — if a user runs `/workflow run implement --branch feature` and then follows up with direct chat "now add a test for this", the follow-up naturally continues in the same worktree. Coherent UX. But the lifecycle is leaky: if the user `/workflow abandon`s the run, `conversation.cwd` doesn't reset, the worktree gets deleted, and direct chat is now stuck pointing at a non-existent directory. Pre-existed this PR — but this PR makes it user-visible for the first time (before, the hardcoded `~/.archon/workspaces` fallback masked it). Options:
Leaning (a) — a single `existsSync` guard in the resolver is the minimum viable fix and keeps the primitive simple. (b) and (c) are overkill until the leaky abstraction bites in production. 2. Direct-chat tool surface — right default?This PR makes direct chat run provider tools (Read, Write, Edit, Bash, Grep, etc.) against the attached project's live directory. That's how Claude Code / Codex natively behave, and it matches user expectations coming from those CLIs. But it's a departure from Archon's "workflows provide structure (validation, retry, event emission, audit), direct chat just routes" model. A user running direct chat on their live repo can have the AI write/edit files without isolation — no worktree, no atomic revert, no per-step event emission, no retry on failure. Options:
(a) is what ships today (and what this PR makes work correctly). (b) is more conservative and aligns with the "workflows provide the safety rails" principle. (c) adds config surface we may not need. I'd hold on this one — let the direct-chat-on-live-repo pattern get some real usage after this fix lands and see if anyone actually wants the read-only mode. File an issue to track. 3. Follow-up I'll file regardless of (1) and (2)
None of these block this PR. The fix is the right incremental move and the primitives it inherits (`conversation.cwd` as direct-chat cwd hint) were already in place. Ready for CI + final look. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/core/src/orchestrator/orchestrator-agent.test.ts (2)
1049-1102: Nit:provider cwd resolutionis nested insidediscoverAllWorkflows — remote sync.These tests exercise cwd derivation in
handleMessage, notdiscoverAllWorkflows. Promoting the innerdescribeto a top-level suite (alongside the otherhandleMessagedescribes) would make the grouping match the behavior under test and avoid implicit coupling to the parent'sbeforeEach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/orchestrator/orchestrator-agent.test.ts` around lines 1049 - 1102, The "provider cwd resolution" describe block is nested under "discoverAllWorkflows — remote sync" but tests target handleMessage's cwd logic; move the entire describe('provider cwd resolution', ...) block out to top-level (alongside other handleMessage suites) so it isn't implicitly coupled to the parent's beforeEach; update placement only (retain tests and references to handleMessage, getCwdPassedToProvider, mockGetOrCreateConversation, mockListCodebases, mockGetCodebase) to ensure the suite runs with the correct setup.
1093-1101: Consider asserting the warn log on the not-found fallback branch.The new test verifies cwd falls back to the workspaces root when
codebase_idis set but the codebase isn't in the list, but it doesn't assert thatorchestrator.codebase_not_found_cwd_fallbackwas logged. Since the warn log is the primary observability signal for this (otherwise silent) fallback, an assertion here would prevent the log from silently regressing:Proposed addition
await handleMessage(makePlatform(), 'conv-1', 'Hello'); expect(getCwdPassedToProvider()).toBe('/home/test/.archon/workspaces'); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.objectContaining({ codebaseId: 'codebase-1', conversationId: 'conv-1' }), + 'orchestrator.codebase_not_found_cwd_fallback' + ); });Note: you may need
mockLogger.warn.mockClear()at the top of thisdescribeblock (or in its ownbeforeEach) since other tests in the parent suite also trigger warns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/orchestrator/orchestrator-agent.test.ts` around lines 1093 - 1101, The test "falls back to getArchonWorkspacesPath when codebase_id is set but codebase not in list" exercises the fallback but doesn't assert the warn observability; update the test to also assert that the warn log key orchestrator.codebase_not_found_cwd_fallback was emitted by checking mockLogger.warn was called with that key (or contains that message) after calling handleMessage (keep existing setup using mockGetOrCreateConversation and mockListCodebases and check getCwdPassedToProvider as before); also add a mockLogger.warn.mockClear() in the surrounding describe/beforeEach if needed to avoid cross-test pollution so the warn assertion is deterministic.packages/core/src/orchestrator/orchestrator-agent.ts (1)
812-827: LGTM — cwd resolution is clear, null-safe, and the fallback is explicitly documented.Precedence (
conversation.cwd→attachedCodebase.default_cwd→ workspaces root) matches the PR intent, the comment documents the intentional fallback (satisfies the "document fallback behavior with a comment when intentional and safe" guideline), and the warn eventorchestrator.codebase_not_found_cwd_fallbackfollows the{domain}.{action}_{state}convention.One small nit:
codebases.find(c => c.id === conversation.codebase_id)duplicates the same lookup already performed inbuildFullPrompt(line 477). If you want to tighten this further, you could resolvescopedCodebaseonce beforebuildFullPromptand reuse it for the cwd block — purely optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 812 - 827, The code performs the same lookup twice; extract the result of codebases.find(...) into a single variable (e.g., scopedCodebase or attachedCodebase) before calling buildFullPrompt and reuse that variable in the cwd resolution block instead of calling codebases.find again; update references in buildFullPrompt usage and the cwd fallback logic (the warn call and conversation.cwd ?? attachedCodebase.default_cwd) to use the single resolved variable to avoid duplicated lookups.
🤖 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/core/src/orchestrator/orchestrator-agent.test.ts`:
- Around line 1049-1102: The "provider cwd resolution" describe block is nested
under "discoverAllWorkflows — remote sync" but tests target handleMessage's cwd
logic; move the entire describe('provider cwd resolution', ...) block out to
top-level (alongside other handleMessage suites) so it isn't implicitly coupled
to the parent's beforeEach; update placement only (retain tests and references
to handleMessage, getCwdPassedToProvider, mockGetOrCreateConversation,
mockListCodebases, mockGetCodebase) to ensure the suite runs with the correct
setup.
- Around line 1093-1101: The test "falls back to getArchonWorkspacesPath when
codebase_id is set but codebase not in list" exercises the fallback but doesn't
assert the warn observability; update the test to also assert that the warn log
key orchestrator.codebase_not_found_cwd_fallback was emitted by checking
mockLogger.warn was called with that key (or contains that message) after
calling handleMessage (keep existing setup using mockGetOrCreateConversation and
mockListCodebases and check getCwdPassedToProvider as before); also add a
mockLogger.warn.mockClear() in the surrounding describe/beforeEach if needed to
avoid cross-test pollution so the warn assertion is deterministic.
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 812-827: The code performs the same lookup twice; extract the
result of codebases.find(...) into a single variable (e.g., scopedCodebase or
attachedCodebase) before calling buildFullPrompt and reuse that variable in the
cwd resolution block instead of calling codebases.find again; update references
in buildFullPrompt usage and the cwd fallback logic (the warn call and
conversation.cwd ?? attachedCodebase.default_cwd) to use the single resolved
variable to avoid duplicated lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26b1e926-1844-445a-8491-41945fe4fefb
📒 Files selected for processing (3)
packages/core/src/orchestrator/orchestrator-agent.test.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/services/cleanup-service.test.ts
1 participant
Summary
cwdpassed to the AI provider was always~/.archon/workspaces(the Archon root), ignoring the attached project.orchestrator-agent.tsnow resolvescwdfrom the attached codebase — usingconversation.cwd(worktree path) when set, otherwisecodebase.default_cwd, with a fallback togetArchonWorkspacesPath()for non-scoped conversations.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
handleMessagegetArchonWorkspacesPathhandleMessageconversation.cwd/codebase.default_cwdhandleStreamMode/handleBatchModeaiClient.sendQueryLabel Snapshot
risk: lowsize: XScorecore:orchestratorChange Metadata
bugcoreLinked Issue
Validation Evidence (required)
bun run type-check: ✅ 10 packages, 0 errorsbun run lint: ✅ 0 errors, 0 warningsbun run format:check: ✅ all files formattedbun run test: ✅ all passed, 0 failedAlso fixed a stale mock in
cleanup-service.test.tsuncovered during validation (stalemockExecFileAsynccalls replaced with propermockGetById/mockGetCodebasesetups).Security Impact (required)
Compatibility / Migration
Human Verification (required)
conversation.cwd = null(usesdefault_cwd),conversation.cwdset (uses worktree path), no codebase attached (uses Archon root fallback), codebase_id set but not in loaded list (falls back gracefully)Side Effects / Blast Radius (required)
handleMessage— workflow execution path is untouchedcodebase.default_cwdpoints to a path that no longer exists, provider tools will fail on filesystem ops — same risk exists for workflow execution todayRollback Plan (required)
let cwdblock inorchestrator-agent.ts(3 lines)Risks and Mitigations
conversation.cwdpoints to a deleted worktreeIsolationBlockedErrorhandling in the isolation layer addresses this separately.Summary by CodeRabbit
Bug Fixes
Tests