feat(git): support configurable git remote name#1548
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:
📝 WalkthroughWalkthroughAdds repository-level git-remote support: introduces ChangesGit Remote Configurability
Sequence Diagram(s)sequenceDiagram
participant Config
participant Orchestrator
participant WorktreeProvider
participant GitModule
participant RepoFS
Config->>Orchestrator: loadRepoConfig(repoPath)
Orchestrator->>WorktreeProvider: request worktree create (repoPath, repoConf)
alt repoConf.worktree.remote present
WorktreeProvider->>WorktreeProvider: use configured remote (trimmed)
else repoConf.worktree.remote missing
WorktreeProvider->>GitModule: getDefaultRemote(repoPath)
GitModule-->>WorktreeProvider: remote name or null
alt remote null
WorktreeProvider-->>RepoFS: throw actionable "ambiguous remote" error
end
end
WorktreeProvider->>GitModule: syncWorkspace(workspacePath, baseBranch?, { remote })
GitModule->>GitModule: git fetch <remote> <branch>
GitModule->>GitModule: git reset --hard <remote>/<branch>
WorktreeProvider->>GitModule: create branch / set-upstream using <remote>/<branch>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/isolation/src/pr-state.ts (1)
42-54: ⚡ Quick winUse
getRemoteUrl()here instead of duplicating the git call.This PR already added a remote-aware helper in
@archon/git, but this path still shells out directly. Reusing the helper keeps the no-remote/error behavior centralized and avoids the two implementations drifting again.Suggested fix
-import { execFileAsync } from '@archon/git'; +import { execFileAsync, getRemoteUrl } from '@archon/git';- let remoteUrl = ''; - try { - const { stdout } = await execFileAsync('git', ['-C', repoPath, 'remote', 'get-url', remote], { - timeout: 10000, - }); - remoteUrl = stdout.trim(); - } catch (error) { + let remoteUrl = ''; + try { + remoteUrl = (await getRemoteUrl(repoPath, remote)) ?? ''; + } catch (error) { getLog().debug( { err: error as Error, repoPath, branch }, 'isolation.pr_state_remote_lookup_failed' ); cache?.set(branch, 'NONE'); return 'NONE'; }As per coding guidelines, "Use
@archon/gitfunctions for git operations; useexecFileAsync(notexec) when calling git directly".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/isolation/src/pr-state.ts` around lines 42 - 54, Replace the direct git shell-out using execFileAsync that sets remoteUrl with the shared helper getRemoteUrl from `@archon/git`: call getRemoteUrl(repoPath, remote) (or the helper's appropriate param order) instead of execFileAsync, assign its result to remoteUrl, and preserve the existing error handling path (log via getLog().debug with the caught error, set cache?.set(branch, 'NONE') and return 'NONE') so behavior remains identical; remove the duplicated git call and import getRemoteUrl where pr-state.ts currently references execFileAsync/remoteUrl.
🤖 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/git/src/repo.ts`:
- Around line 54-60: The remotes array may contain CRLF/trailing whitespace
(e.g., "origin\r") so remotes.includes('origin') can fail; update the
construction of remotes in repo.ts (the code that builds the remotes variable)
to normalize entries by trimming each item after split (e.g., .split('\n').map(r
=> r.trim()).filter(r => r.length > 0)) and then use the normalized remotes when
checking for 'origin' (and when returning remotes[0]) so CRLF or trailing spaces
won't break detection.
- Around line 62-66: The catch block in packages/git/src/repo.ts is swallowing
any git execution error and returning null (seen around the
get_default_remote_failed log), which incorrectly signals "no default remote";
change it to rethrow or propagate the git error except in the explicit case of
"no remote configured": in the catch in the function that queries the default
remote (references repoPath, getLog(), and the get_default_remote_failed log
key), inspect the error (err.stderr / exit code) and if it clearly indicates "no
remote" return null, otherwise rethrow the error so callers see the actual git
failure instead of the ambiguous null sentinel.
---
Nitpick comments:
In `@packages/isolation/src/pr-state.ts`:
- Around line 42-54: Replace the direct git shell-out using execFileAsync that
sets remoteUrl with the shared helper getRemoteUrl from `@archon/git`: call
getRemoteUrl(repoPath, remote) (or the helper's appropriate param order) instead
of execFileAsync, assign its result to remoteUrl, and preserve the existing
error handling path (log via getLog().debug with the caught error, set
cache?.set(branch, 'NONE') and return 'NONE') so behavior remains identical;
remove the duplicated git call and import getRemoteUrl where pr-state.ts
currently references execFileAsync/remoteUrl.
🪄 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: d656e48b-0636-4ddf-865b-01f8ac12f6dc
📒 Files selected for processing (10)
packages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/git/src/branch.tspackages/git/src/git.test.tspackages/git/src/index.tspackages/git/src/repo.tspackages/isolation/src/pr-state.tspackages/isolation/src/providers/worktree.test.tspackages/isolation/src/providers/worktree.tspackages/isolation/src/types.ts
|
@deepakrawat-dce related to #1419 — worktree.baseBranch config vs configurable remote name share git-config concerns. |
|
Hi @deepakrawat-dce — thanks for opening this PR. This repository uses a PR template at
Could you fill those out (even briefly)? The template If a section genuinely doesn't apply, just write "N/A" in |
|
🔗 Related issue: This PR is related to issue #1273 ("syncWorkspace always resets managed source clone to origin/main, ignoring configured default_branch"). Supporting configurable remote names complements the branch-detection fix for that issue. |
Review SummaryVerdict: blocking-issues Your PR adds a well-scoped, well-tested feature that removes the hardcoded Blocking issues
Suggested fixes
Minor / nice-to-have
ComplimentsThe end-to-end test coverage in Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality, docs-impact. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/orchestrator/orchestrator-agent.ts (1)
774-780:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSync notification always says "origin/" even when a different remote is configured.
remoteis scoped insidediscoverAllWorkflowsand is not surfaced throughDiscoverResult, so this user-facing message will be misleading wheneverworktree.remoteis set to something other than'origin'.🛠️ Proposed fix
Add
remotetoDiscoverResultand thread it through:interface DiscoverResult { workflows: WorkflowWithSource[]; errors: readonly WorkflowLoadError[]; syncResult?: WorkspaceSyncResult; syncError?: string; config?: MergedConfig; + remote?: string; }Inside
discoverAllWorkflows, return the resolved value:- return { workflows, errors: allErrors, syncResult, syncError, config }; + return { workflows, errors: allErrors, syncResult, syncError, config, remote };At the destructuring call site (lines 752–758):
const { workflows: workflowsWithSource, errors: workflowErrors, syncResult, syncError, config: discoveredConfig, + remote: syncRemote, } = await discoverAllWorkflows(conversation);In the notification template:
- content: `Synced with origin/${syncResult.branch} \u2014 updated ${syncResult.previousHead} \u2192 ${syncResult.newHead}`, + content: `Synced with ${syncRemote ?? 'origin'}/${syncResult.branch} \u2014 updated ${syncResult.previousHead} \u2192 ${syncResult.newHead}`,🤖 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 774 - 780, The message hardcodes "origin/" because the remote name is not included in DiscoverResult; update the discoverAllWorkflows return type (DiscoverResult) to include a remote string, set that remote from worktree.remote inside discoverAllWorkflows, thread it through to where syncResult is produced (ensure the destructuring/site that consumes discoverAllWorkflows includes remote), and replace the hardcoded "origin/" in the platform.sendStructuredEvent payload with the remote value from syncResult (e.g., use syncResult.remote/${syncResult.branch}) so the notification reflects the configured remote.packages/core/src/services/cleanup-service.ts (1)
587-611:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
remoteparameter to thecleanupMergedEnvironmentswrapper's options type.The wrapper at
isolation-operations.ts:168–174accepts{ includeClosed?: boolean }, but the underlyingcleanupMergedWorktreesfunction now accepts{ includeClosed?: boolean; remote?: string }. Without updating the wrapper's type, callers using the public API cannot pass theremoteoption.Fix in
isolation-operations.tsexport async function cleanupMergedEnvironments( codebaseId: string, mainPath: string, - options: { includeClosed?: boolean } = {} + options: { includeClosed?: boolean; remote?: string } = {} ): Promise<CleanupOperationResult> { return cleanupMergedWorktrees(codebaseId, mainPath, options); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/services/cleanup-service.ts` around lines 587 - 611, The wrapper function cleanupMergedEnvironments currently types its options as { includeClosed?: boolean } but calls cleanupMergedWorktrees which accepts { includeClosed?: boolean; remote?: string }, so update the wrapper's options type to include remote?: string and forward options.remote when invoking cleanupMergedWorktrees (ensure the function signature/parameter typing for cleanupMergedEnvironments and any related call site or exported type reflect remote?: string so callers can pass the remote option).
🧹 Nitpick comments (1)
packages/core/src/services/cleanup-service.ts (1)
308-316: Scheduled cleanup diverges from on-demand cleanup for non-origin remotes.
runScheduledCleanup(line 311) callsgetDefaultBranch(mainRepoPath)without aremote, hardcoding'origin'behavior. After this PR, the on-demandcleanupMergedWorktreesis remote-aware, but the periodic background cycle is not. For a repo configured withworktree.remote: upstream, the scheduler could fail to detect branches as merged (if they are merged intoupstream/mainbut haven't reachedorigin/main), leaving worktrees behind indefinitely.This is pre-existing behavior not changed by this PR, but the delta is now observable. A follow-up task to thread repo-config remote loading into
runScheduledCleanup's per-environment loop would close the gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/services/cleanup-service.ts` around lines 308 - 316, runScheduledCleanup currently calls getDefaultBranch(mainRepoPath) with the implicit 'origin' behavior and should be made remote-aware like cleanupMergedWorktrees; in the per-environment loop, load the repo config (repo-config/worktree.remote) for mainRepoPath and pass the configured remote down to getDefaultBranch and/or isBranchMerged (or call cleanupMergedWorktrees with the remote) so branch-merger checks use the correct remote (e.g., pass remote into getDefaultBranch, isBranchMerged, and any calls referencing env.branch_name) to ensure the scheduler detects merges against non-origin remotes such as upstream.
🤖 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/core/src/services/cleanup-service.ts`:
- Around line 448-452: The worktree remote argument isn't flowing through the
isolation cleanup path: update the IsolationResolverDeps.cleanup.getBreakdown
type to accept an optional third parameter (remote?: string), adjust the
IsolationResolver implementation (the cleanup.getBreakdown caller/implementer)
to forward the remote to the underlying getWorktreeStatusBreakdown logic, and
update the command handler call site that currently does
getWorktreeStatusBreakdown(codebase.id, codebase.default_cwd) to load the repo
config (worktree.remote) and pass that value as the third argument so merge
detection uses the correct remote.
---
Outside diff comments:
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 774-780: The message hardcodes "origin/" because the remote name
is not included in DiscoverResult; update the discoverAllWorkflows return type
(DiscoverResult) to include a remote string, set that remote from
worktree.remote inside discoverAllWorkflows, thread it through to where
syncResult is produced (ensure the destructuring/site that consumes
discoverAllWorkflows includes remote), and replace the hardcoded "origin/" in
the platform.sendStructuredEvent payload with the remote value from syncResult
(e.g., use syncResult.remote/${syncResult.branch}) so the notification reflects
the configured remote.
In `@packages/core/src/services/cleanup-service.ts`:
- Around line 587-611: The wrapper function cleanupMergedEnvironments currently
types its options as { includeClosed?: boolean } but calls
cleanupMergedWorktrees which accepts { includeClosed?: boolean; remote?: string
}, so update the wrapper's options type to include remote?: string and forward
options.remote when invoking cleanupMergedWorktrees (ensure the function
signature/parameter typing for cleanupMergedEnvironments and any related call
site or exported type reflect remote?: string so callers can pass the remote
option).
---
Nitpick comments:
In `@packages/core/src/services/cleanup-service.ts`:
- Around line 308-316: runScheduledCleanup currently calls
getDefaultBranch(mainRepoPath) with the implicit 'origin' behavior and should be
made remote-aware like cleanupMergedWorktrees; in the per-environment loop, load
the repo config (repo-config/worktree.remote) for mainRepoPath and pass the
configured remote down to getDefaultBranch and/or isBranchMerged (or call
cleanupMergedWorktrees with the remote) so branch-merger checks use the correct
remote (e.g., pass remote into getDefaultBranch, isBranchMerged, and any calls
referencing env.branch_name) to ensure the scheduler detects merges against
non-origin remotes such as upstream.
🪄 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: edd0d4b1-1289-49fb-ad66-af1eaed9ad68
📒 Files selected for processing (9)
CHANGELOG.mdpackages/core/src/config/config-loader.test.tspackages/core/src/config/config-types.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/services/cleanup-service.tspackages/docs-web/src/content/docs/reference/configuration.mdpackages/git/src/repo.tspackages/isolation/src/providers/worktree.test.tspackages/isolation/src/types.ts
✅ Files skipped from review due to trivial changes (3)
- CHANGELOG.md
- packages/core/src/config/config-loader.test.ts
- packages/isolation/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/isolation/src/providers/worktree.test.ts
- packages/git/src/repo.ts
Review SummaryVerdict: blocking-issues This PR adds configurable Blocking issues
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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/services/cleanup-service.ts (1)
312-314: ⚡ Quick winCache remote resolution per codebase in scheduled cleanup.
Line 313 loads/parses repo config for every environment. With many envs on the same repo, this repeats disk/YAML work and can duplicate identical failures.
Proposed diff
export async function runScheduledCleanup(): Promise<CleanupReport> { getLog().info('cleanup_started'); const report: CleanupReport = { removed: [], skipped: [], errors: [], sessionsDeleted: 0 }; + const remoteByRepoPath = new Map<string, string | undefined>(); try { @@ - const envRemote = - (await loadRepoConfig(env.codebase_default_cwd)).worktree?.remote?.trim() || undefined; + let envRemote: string | undefined; + if (remoteByRepoPath.has(env.codebase_default_cwd)) { + envRemote = remoteByRepoPath.get(env.codebase_default_cwd); + } else { + envRemote = + (await loadRepoConfig(env.codebase_default_cwd)).worktree?.remote?.trim() || undefined; + remoteByRepoPath.set(env.codebase_default_cwd, envRemote); + } const mainBranch = await getDefaultBranch(mainRepoPath, envRemote);🤖 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/core/src/services/cleanup-service.ts` around lines 312 - 314, The loop in cleanup-service calls loadRepoConfig(env.codebase_default_cwd) for every env, causing repeated disk/YAML parsing and duplicate failures; add a simple in-memory cache (e.g., Map keyed by env.codebase_default_cwd) before the call so you only load/parse each repo once, store the resolved worktree?.remote?.trim() (or undefined) in the cache, and then use the cached envRemote when invoking getDefaultBranch(mainRepoPath, envRemote); ensure errors from loadRepoConfig are cached as undefined to avoid repeated failing attempts.
🤖 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/core/src/services/cleanup-service.ts`:
- Around line 312-314: The loop in cleanup-service calls
loadRepoConfig(env.codebase_default_cwd) for every env, causing repeated
disk/YAML parsing and duplicate failures; add a simple in-memory cache (e.g.,
Map keyed by env.codebase_default_cwd) before the call so you only load/parse
each repo once, store the resolved worktree?.remote?.trim() (or undefined) in
the cache, and then use the cached envRemote when invoking
getDefaultBranch(mainRepoPath, envRemote); ensure errors from loadRepoConfig are
cached as undefined to avoid repeated failing attempts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e21559ed-64cc-4c94-b5a6-72b7f7332c4b
📒 Files selected for processing (4)
packages/core/src/config/config-loader.tspackages/core/src/handlers/command-handler.tspackages/core/src/operations/isolation-operations.tspackages/core/src/services/cleanup-service.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/config/config-loader.ts
Review SummaryVerdict: minor-fixes-needed This PR adds a Blocking issues
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. |
Archon hardcoded 'origin' as the git remote name in all fetch/push/reset operations. This breaks for repos that use non-standard remote names (e.g. enterprise monorepos with numbered remotes like '260', '262', '264'). Add `worktree.remote` config option in `.archon/config.yaml` and auto-detection via `getDefaultRemote()` when not configured: 1. 'origin' if it exists (standard convention) 2. The sole remote if only one is configured 3. Actionable error if ambiguous (multiple non-origin remotes) Thread the remote name through all git operations: syncWorkspace, syncRepository, getDefaultBranch, getRemoteUrl, worktree creation (issue/PR/ task/fork), branch tracking, and remote branch deletion. All existing callers default to 'origin' so this is fully backwards-compatible.
Use 'jan', 'feb', 'mar' as example remote names instead of numbered remotes that reference a specific organization.
- Split on /\r?\n/ instead of '\n' so Windows CRLF doesn't break
origin detection (e.g. 'origin\r' fails includes('origin'))
- Let git execution errors propagate instead of swallowing them as
null — callers now see the real failure instead of a misleading
"set worktree.remote" message
Address review feedback: - Load repo config before syncWorkspace in orchestrator-agent.ts and pass the resolved remote name - Thread remote through cleanupMergedWorktrees → isSafeToRemove → getPrState so PR-state detection works for non-origin remotes - Add getDefaultBranch remote param to getWorktreeStatusBreakdown - Add config-loader tests: propagates remote, trims whitespace, undefined when not configured - Add worktree test: fromBranch + custom remote (task workflow) - Document worktree.remote in configuration.md reference - Add CHANGELOG entry under [Unreleased] - Generalize comment examples (remove org-specific language) - Fix redundant docstring in getRemoteUrl
Address second review round: - cleanupToMakeRoom: accept and forward remote option - runScheduledCleanup: resolve remote per-environment via loadRepoConfig before calling getDefaultBranch - command-handler /worktree cleanup: resolve remote from repo config - command-handler /status: pass remote to getWorktreeStatusBreakdown - isolation-operations cleanupMergedEnvironments: forward remote option - Fix config-loader comment to describe purpose not feature name
- Add warn log in cleanupMergedWorktrees when isSafeToRemove fails (prevents silent swallowing of transient errors) - Wrap loadRepoConfig in runScheduledCleanup with try/catch to prevent filesystem errors from crashing scheduled cleanup - Restore safety comments: managed-clone reset invariant, same-repo vs fork PR rationale, SHA reproducibility, tracking branch - Fix config-loader tests: use mockFsReadFile (upstream renamed mock)
deepakrawat-dce
force-pushed
the
feat/configurable-git-remote
branch
from
ad8f8d4 to
a0e07c4
Compare
|
@deepakrawat-dce CI is failing, can you check? |
The orchestrator test mocked @archon/git without getDefaultRemote and ../config/config-loader without loadRepoConfig, causing CI failures. Also update syncWorkspace assertions to use objectContaining so they don't break when additional options (like remote) are passed.
Should be resolved now. Thanks for pointing this out. |
3 participants
Summary
'origin'as the git remote name in all fetch/push/reset operations, breaking repos with non-standard remotes.jan,feb,marfor release-based workflows) cannot use Archon's worktree isolation at all.worktree.remoteconfig option,getDefaultRemote()auto-detection, and threaded remote name through all git operations.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
remotein options${remote}/instead oforigin/worktree.remoteremoteparamLabel Snapshot
risk: lowsize: Mgit,isolation,configgit:repo,isolation:worktree,core:configChange Metadata
featuremulti(git + isolation + core)Linked Issue
Validation Evidence (required)
Security Impact (required)
NoNo(same git fetch/push, just configurable remote name)NoNoYes: N/ACompatibility / Migration
Yes— all new parameters have defaults matching prior behavior ('origin')Yes— new optionalworktree.remotefield in.archon/config.yamlNoworktree.remotebehave identically.Human Verification (required)
getDefaultRemote()correctly auto-detects sole remote; confirmedworktree.remoteconfig is respectedSide Effects / Blast Radius (required)
'origin'default. No behavior change for repos that already useorigin.resolveRemote()method logs at info level when auto-detecting a non-origin remoteRollback Plan (required)
worktree.remoteconfig field is the toggle — removing it reverts tooriginbehaviorRisks and Mitigations
Risk:
getDefaultRemote()now throws on git errors instead of returning null, which could surface unexpected errors in edge cases.resolveRemote()which is inside acreateWorktree()try/catch that already surfaces errors with actionable messages. The error path is intentional — better to fail fast with the real error than mislead the user.Risk: CRLF handling via
/\r?\n/regex could theoretically split differently on exotic git builds..trim()on each entry catches any trailing whitespace.Summary by CodeRabbit
New Features
Tests
Documentation