fix(core): pass configured default_branch to syncWorkspace to prevent hard-reset to wrong branch#1274
fix(core): pass configured default_branch to syncWorkspace to prevent hard-reset to wrong branch#1274
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 a nullable Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CloneHandler as Clone Handler
participant Git
participant DB as Database
participant Orchestrator
participant Sync as Workspace Sync
User->>CloneHandler: request clone/register
CloneHandler->>Git: git clone <repo>
Git-->>CloneHandler: clone complete
CloneHandler->>Git: git rev-parse --abbrev-ref HEAD (timeout)
Git-->>CloneHandler: branch name / HEAD / error
CloneHandler->>DB: createCodebase(..., default_branch=detected?)
DB-->>CloneHandler: stored
User->>Orchestrator: trigger workflow (message)
Orchestrator->>DB: fetch codebase (includes default_branch)
DB-->>Orchestrator: codebase with default_branch
Orchestrator->>Sync: syncWorkspace(path, baseBranch=configuredBranch, {resetAfterFetch})
Sync->>Git: git fetch origin
Sync->>Git: git reset --hard origin/<configuredBranch>
Git-->>Sync: reset complete
Sync-->>Orchestrator: sync complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/handlers/clone.ts (1)
314-380:⚠️ Potential issue | 🟡 Minor
registerRepositoryskips default-branch detection — managed-clone fix won't apply to locally registered repos.
cloneRepositorynow detects the actual checked-out branch and persists it, butregisterRepositorystill callsregisterRepoAtPath(localPath, name, remoteUrl)without adefaultBranch(Line 380). Local repos already have a valid working tree atlocalPath, so the samegit -C <localPath> rev-parse --abbrev-ref HEADdetection is straightforward here. Without it, any locally registered repo whose actual branch isn'tmainfalls back to the schema default'main', which is precisely the failure mode#1273aims to prevent oncesyncWorkspacestarts trustingcodebase.default_branch.Proposed fix
// default_cwd is the real local path (not the symlink) - return registerRepoAtPath(localPath, name, remoteUrl); + let detectedBranch: string | undefined; + try { + const { stdout } = await execFileAsync( + 'git', + ['-C', localPath, 'rev-parse', '--abbrev-ref', 'HEAD'], + { timeout: 5000 } + ); + const branch = stdout.trim(); + if (branch && branch !== 'HEAD') { + detectedBranch = branch; + } + } catch { + // Non-fatal — leave undefined so DB default applies + } + return registerRepoAtPath(localPath, name, remoteUrl, detectedBranch);You could also hoist the detection into a small helper to avoid duplicating the block from
cloneRepository.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/handlers/clone.ts` around lines 314 - 380, registerRepository currently doesn't detect the repo's checked-out branch so local repos get the schema default 'main'; add the same branch-detection used in cloneRepository: run execFileAsync('git', ['-C', localPath, 'rev-parse', '--abbrev-ref', 'HEAD']) (handle errors) to get defaultBranch and pass that defaultBranch into the existing registerRepoAtPath(localPath, name, remoteUrl, defaultBranch) call; optionally factor the execFileAsync branch lookup into a small helper shared with cloneRepository to avoid duplication and reference the functions registerRepository, registerRepoAtPath, and cloneRepository when making the change.
🧹 Nitpick comments (4)
migrations/000_combined.sql (1)
32-32: Consider mirroring the idempotent ALTER at the bottom.Other per-migration columns (e.g.,
allow_env_keysfrom migration 021 at Line 313-315) are both in the CREATE TABLE and re-stated asALTER TABLE ... ADD COLUMN IF NOT EXISTSat the file's tail to handle upgrades from earlier combined-baseline snapshots. For consistency, add:-- From migration 022: default_branch on codebases ALTER TABLE remote_agent_codebases ADD COLUMN IF NOT EXISTS default_branch TEXT DEFAULT 'main';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/000_combined.sql` at line 32, Add an idempotent ALTER that mirrors the CREATE TABLE column for default_branch: in the migrations file append an "ALTER TABLE remote_agent_codebases ADD COLUMN IF NOT EXISTS default_branch TEXT DEFAULT 'main';" (referencing the default_branch column and remote_agent_codebases table introduced by migration 022) so upgrades from earlier combined-baseline snapshots will succeed without errors.packages/core/src/db/codebases.ts (1)
24-31: Explicitnullbind overrides the columnDEFAULT 'main'— intentional? Document it.Passing
data.default_branch ?? nullalways binds a value, so Postgres never applies theDEFAULT 'main'from migrations 000/022. In practice this is desirable for this PR (so/register-projectcallers that don't pass a branch get NULL rather than a misleading'main', aligning with thestring | nulltype), but it means the column default is effectively dead code for new inserts.Two small asks:
- Add a brief comment here explaining the NULL-means-"unknown, auto-detect" contract so a future contributor doesn't "fix" this back to using the DB default.
- Consider dropping
DEFAULT 'main'from the migration in favor of NULL (see the migration review) so the DB and application agree on the semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/db/codebases.ts` around lines 24 - 31, The INSERT binds data.default_branch with the expression `data.default_branch ?? null`, which intentionally sends NULL to Postgres (so the app treats NULL as "unknown/auto-detect" rather than letting the DB default 'main' apply); add a short clarifying comment next to this binding in packages/core/src/db/codebases.ts (referencing the SQL INSERT for remote_agent_codebases and the `data.default_branch ?? null` expression) that documents the NULL-means-"unknown/auto-detect" contract so future contributors don't revert it to using the DB default, and also note in the comment to consider removing `DEFAULT 'main'` from the migration so DB and app semantics match.packages/core/src/db/codebases.test.ts (1)
58-59: Missing test for the non-nulldefault_branchinsert path.Both
createCodebasetests currently exercise only thedefault_branch → nullbranch. Given this PR's core purpose is to persist the detected branch fromcloneRepository, please add a test that passesdefault_branch: 'main'(or a non-main value) and asserts it appears in the bind params. This also guards against accidental?? nullregressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/db/codebases.test.ts` around lines 58 - 59, Add a new unit test in packages/core/src/db/codebases.test.ts that covers the non-null default_branch insertion path: call the same helper that invokes createCodebase (or the test helper that triggers the SQL shown: the INSERT INTO remote_agent_codebases...) with default_branch: 'main' (or another non-null value) and assert the executed query's bind params include 'main' in the fourth position (matching the array ['test-project', 'https://github.com/user/repo', '/workspace/test-project', 'main', 'claude']); this mirrors the existing null-branch test but passes a non-null default_branch to guard against accidental ?? null regressions.packages/core/src/handlers/clone.ts (1)
284-299: Consider a debug log on detection failure.Silent catch is documented, which satisfies the guideline, but swallowing without any signal makes future triage harder if
default_branchends up unexpectedly at the DB default. A singlegetLog().debugon the catch path would preserve the non-fatal behavior while giving operators a breadcrumb. Minor nit — feel free to ignore.Proposed tweak
- } catch { + } catch (error) { // Non-fatal — leave undefined so DB default applies + getLog().debug({ path: targetPath, err: error }, 'default_branch_detect_failed'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/handlers/clone.ts` around lines 284 - 299, The catch block swallowing errors when detecting the checked-out branch should emit a non-fatal debug log so operators get a breadcrumb; modify the try/catch around execFileAsync (the block that sets detectedBranch) to catch the error as a variable (e.g., err) and call getLog().debug with a short message like "failed to detect repo branch, falling back to DB default" and include the error details and targetPath/command context; preserve the non-fatal behavior by leaving detectedBranch undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@migrations/022_add_default_branch_to_codebases.sql`:
- Around line 5-6: The migration currently adds default_branch with DEFAULT
'main' which will materialize 'main' for all existing rows and can cause
destructive hard resets; change the ALTER TABLE in remote_agent_codebases (the
ADD COLUMN IF NOT EXISTS default_branch TEXT DEFAULT 'main') to add the column
without a non-NULL default (e.g., no DEFAULT or DEFAULT NULL) so existing rows
remain NULL, and instead implement a safe backfill step outside the migration
that reads each repository's current branch from disk and updates default_branch
only where appropriate; ensure application logic handles NULL by falling back to
local git branch detection (e.g., use the existing codepath that calls git
rev-parse --abbrev-ref HEAD) or skips resets.
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 413-418: When codebase.default_branch is null we must avoid
falling back to origin/HEAD for managed clones; change the logic around
configuredBranch (the variable computed from codebase.default_branch before
calling syncWorkspace) so that if isManagedClone is true and
codebase.default_branch is null you run a local branch detection (git rev-parse
--abbrev-ref HEAD via the existing git helper or a small helper) and set
configuredBranch to that local branch; if local branch disagrees with
origin/HEAD, disable or refuse the hard reset (e.g., flip resetAfterFetch to
false or return an error) before calling syncWorkspace(toRepoPath(...),
configuredBranch, { resetAfterFetch: isManagedClone }); alternatively also
populate default_branch during handleRegisterProject so new rows never persist
null.
In `@packages/core/src/types/index.ts`:
- Line 61: Update the test helper functions named makeCodebase used in
orchestrator-agent.test.ts and orchestrator-isolation.test.ts to return complete
Codebase objects by adding the missing required fields: include default_branch:
string | null (e.g., "main" or null), repository_url: string | null (or an
actual repo URL), and ai_assistant_type: string | null (or appropriate
enum/string value) alongside the existing properties so the returned object
satisfies the Codebase type signature and fixes TypeScript compilation errors.
---
Outside diff comments:
In `@packages/core/src/handlers/clone.ts`:
- Around line 314-380: registerRepository currently doesn't detect the repo's
checked-out branch so local repos get the schema default 'main'; add the same
branch-detection used in cloneRepository: run execFileAsync('git', ['-C',
localPath, 'rev-parse', '--abbrev-ref', 'HEAD']) (handle errors) to get
defaultBranch and pass that defaultBranch into the existing
registerRepoAtPath(localPath, name, remoteUrl, defaultBranch) call; optionally
factor the execFileAsync branch lookup into a small helper shared with
cloneRepository to avoid duplication and reference the functions
registerRepository, registerRepoAtPath, and cloneRepository when making the
change.
---
Nitpick comments:
In `@migrations/000_combined.sql`:
- Line 32: Add an idempotent ALTER that mirrors the CREATE TABLE column for
default_branch: in the migrations file append an "ALTER TABLE
remote_agent_codebases ADD COLUMN IF NOT EXISTS default_branch TEXT DEFAULT
'main';" (referencing the default_branch column and remote_agent_codebases table
introduced by migration 022) so upgrades from earlier combined-baseline
snapshots will succeed without errors.
In `@packages/core/src/db/codebases.test.ts`:
- Around line 58-59: Add a new unit test in
packages/core/src/db/codebases.test.ts that covers the non-null default_branch
insertion path: call the same helper that invokes createCodebase (or the test
helper that triggers the SQL shown: the INSERT INTO remote_agent_codebases...)
with default_branch: 'main' (or another non-null value) and assert the executed
query's bind params include 'main' in the fourth position (matching the array
['test-project', 'https://github.com/user/repo', '/workspace/test-project',
'main', 'claude']); this mirrors the existing null-branch test but passes a
non-null default_branch to guard against accidental ?? null regressions.
In `@packages/core/src/db/codebases.ts`:
- Around line 24-31: The INSERT binds data.default_branch with the expression
`data.default_branch ?? null`, which intentionally sends NULL to Postgres (so
the app treats NULL as "unknown/auto-detect" rather than letting the DB default
'main' apply); add a short clarifying comment next to this binding in
packages/core/src/db/codebases.ts (referencing the SQL INSERT for
remote_agent_codebases and the `data.default_branch ?? null` expression) that
documents the NULL-means-"unknown/auto-detect" contract so future contributors
don't revert it to using the DB default, and also note in the comment to
consider removing `DEFAULT 'main'` from the migration so DB and app semantics
match.
In `@packages/core/src/handlers/clone.ts`:
- Around line 284-299: The catch block swallowing errors when detecting the
checked-out branch should emit a non-fatal debug log so operators get a
breadcrumb; modify the try/catch around execFileAsync (the block that sets
detectedBranch) to catch the error as a variable (e.g., err) and call
getLog().debug with a short message like "failed to detect repo branch, falling
back to DB default" and include the error details and targetPath/command
context; preserve the non-fatal behavior by leaving detectedBranch undefined.
🪄 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: 4a5ffa44-0020-478c-bd43-a72da3a80c9a
📒 Files selected for processing (9)
migrations/000_combined.sqlmigrations/022_add_default_branch_to_codebases.sqlpackages/core/src/db/codebases.test.tspackages/core/src/db/codebases.tspackages/core/src/handlers/clone.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/orchestrator/orchestrator.test.tspackages/core/src/types/index.tspackages/server/src/routes/schemas/codebase.schemas.ts
| const configuredBranch = codebase.default_branch | ||
| ? toBranchName(codebase.default_branch) | ||
| : undefined; | ||
| syncResult = await syncWorkspace(toRepoPath(codebase.default_cwd), configuredBranch, { | ||
| resetAfterFetch: isManagedClone, | ||
| }); |
There was a problem hiding this comment.
Null default_branch silently falls back to auto-detect — same root bug for unset rows.
When codebase.default_branch is null, configuredBranch is undefined and syncWorkspace re-enters the auto-detect path (git symbolic-ref refs/remotes/origin/HEAD → typically main). For managed clones (resetAfterFetch: true) this will still do git reset --hard origin/main — the exact behavior issue #1273 is trying to avoid.
This works for codebases created via the updated clone path (branch is detected and persisted), but two scenarios still hit the bug:
- Rows that existed before migration 022 (defaulted to
'main'— see my comment on the migration). - Codebases created via
/register-project(handleRegisterProject at Line 1300 doesn't passdefault_branch, so null is persisted).
Consider either (a) detecting HEAD at /register-project time too, or (b) when default_branch is null on a managed clone, detecting the local branch via git rev-parse --abbrev-ref HEAD before syncing rather than resolving via origin/HEAD, and refusing the reset if they disagree.
🤖 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 413 - 418,
When codebase.default_branch is null we must avoid falling back to origin/HEAD
for managed clones; change the logic around configuredBranch (the variable
computed from codebase.default_branch before calling syncWorkspace) so that if
isManagedClone is true and codebase.default_branch is null you run a local
branch detection (git rev-parse --abbrev-ref HEAD via the existing git helper or
a small helper) and set configuredBranch to that local branch; if local branch
disagrees with origin/HEAD, disable or refuse the hard reset (e.g., flip
resetAfterFetch to false or return an error) before calling
syncWorkspace(toRepoPath(...), configuredBranch, { resetAfterFetch:
isManagedClone }); alternatively also populate default_branch during
handleRegisterProject so new rows never persist null.
🔍 Comprehensive PR ReviewPR: #1274 — fix(core): pass configured default_branch to syncWorkspace SummaryThe core fix is correct and well-scoped: Verdict:
🟠 High Issues (Should Fix Before Merge)1.
|
| Issue | Location | Suggestion |
|---|---|---|
Comments misdescribe fallback as "DB default applies" — actual fallback is NULL → syncWorkspace auto-detect |
clone.ts:285,298 |
Update to: "store NULL so syncWorkspace falls back to auto-detection" |
makeCodebase mock helper missing default_branch field |
clone.test.ts:128-148 |
Add default_branch: null to returned object and Partial<> type |
findCodebaseByDefaultCwd inline mock missing default_branch |
codebases.test.ts:295-308 |
Add default_branch: null to inline mock row |
createCodebase test never passes non-null default_branch |
codebases.test.ts:46-61 |
Add a test with default_branch: 'develop' asserting it reaches SQL params |
✅ What's Good
- Correct null-guard:
codebase.default_branch ? toBranchName(...) : undefinedavoids callingtoBranchName('')which would throw - Safe migration:
ADD COLUMN IF NOT EXISTShandles environments where the column was manually added - Timeout on detection:
{ timeout: 5000 }prevents branch detection from stalling the clone flow 'HEAD'filter: Correctly handles detached HEAD —'HEAD'is not stored as a branch nameexecFileAsynccompliance: New git call follows CLAUDE.md's requirement (notexec)sanitizeErrorpreserved: Credential sanitization in clone failure path is intact- TypeScript strict: No
anyintroduced;default_branch: string | nulltyped correctly throughout - Well-scoped: No speculative changes — YAGNI followed
Next Steps
- ⚡ Add tests for the 2 HIGH issues (branch detection logic +
syncWorkspaceforwarding) - 📝 Consider the MEDIUM debug log (one-liner)
- 🎯 Merge when tests are green
Reviewed by Archon comprehensive-pr-review workflow
Artifacts: /.archon/workspaces/coleam00/Archon/artifacts/runs/eb39ea1b-10b9-4475-9772-5644ad14839b/review/
Fixed: - Add debug log to branch detection catch block in clone.ts (observability) - Update comments to accurately describe NULL → syncWorkspace auto-detect fallback - Add `default_branch` field to `makeCodebase` helper in clone.test.ts - Add `default_branch: null` to findCodebaseByDefaultCwd inline mock in codebases.test.ts - Add test for createCodebase with explicit non-null default_branch Tests added: - 4 branch detection tests in clone.test.ts (happy path, detached HEAD, failure, empty) - 2 syncWorkspace branch forwarding tests in orchestrator.test.ts (non-null + null branch) - Mock @archon/git in orchestrator.test.ts to verify syncWorkspace call arguments Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
⚡ Self-Fix Report (Aggressive)Status: COMPLETE Fixes Applied (7 total)
View all fixes
Tests Added
Skipped (0)(none — all findings addressed) Suggested Follow-up Issues(none) Validation✅ Type check | ✅ Lint | ✅ Tests (48 clone + 70 orchestrator + 28 codebases = 146 passed)
Self-fix by Archon · aggressive mode · fixes pushed to |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/orchestrator/orchestrator.test.ts (1)
671-693: Nit: typocodbaseWithBranch→codebaseWithBranch.- const codbaseWithBranch: Codebase = { + const codebaseWithBranch: Codebase = { ...mockCodebase, default_branch: 'develop', default_cwd: '/home/test/.archon/workspaces/owner/repo/source', }; mockGetOrCreateConversation.mockResolvedValue({ ...mockConversationWithProject, codebase_id: 'codebase-789', }); - mockGetCodebase.mockResolvedValue(codbaseWithBranch); + mockGetCodebase.mockResolvedValue(codebaseWithBranch);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/orchestrator/orchestrator.test.ts` around lines 671 - 693, The test declares a variable misspelled as codbaseWithBranch; rename it to codebaseWithBranch everywhere in the test (the variable declaration and its use in mockGetCodebase.mockResolvedValue) so references are consistent; ensure the object still matches the Codebase shape used by the test and update any other occurrences of codbaseWithBranch within this test (e.g., the local variable passed to mockGetCodebase) to the corrected identifier.
🤖 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/orchestrator/orchestrator.test.ts`:
- Around line 134-138: Current use of mock.module('@archon/git', ...) replaces
the module globally and can clash with spy-based mocks in clone.test.ts; replace
this with spyOn-based mocks: spyOn the specific exported functions used in
orchestrator.test.ts (e.g., syncWorkspace, toRepoPath, toBranchName) to return
mockSyncWorkspace, mockToRepoPath, mockToBranchName respectively, and
restore/clear those spies in afterEach; alternatively, if changing to spyOn is
not possible, move this suite to run in its own bun test invocation to avoid
cross-test pollution.
---
Nitpick comments:
In `@packages/core/src/orchestrator/orchestrator.test.ts`:
- Around line 671-693: The test declares a variable misspelled as
codbaseWithBranch; rename it to codebaseWithBranch everywhere in the test (the
variable declaration and its use in mockGetCodebase.mockResolvedValue) so
references are consistent; ensure the object still matches the Codebase shape
used by the test and update any other occurrences of codbaseWithBranch within
this test (e.g., the local variable passed to mockGetCodebase) to the corrected
identifier.
🪄 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: 67aa4020-b7d1-4f59-8d8e-333d2376d534
📒 Files selected for processing (4)
packages/core/src/db/codebases.test.tspackages/core/src/handlers/clone.test.tspackages/core/src/handlers/clone.tspackages/core/src/orchestrator/orchestrator.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/db/codebases.test.ts
PR Review — #1274: fix(core): pass configured default_branch to syncWorkspaceReviewed: 2026-04-17 | Status: ✅ Auto-fixes applied and pushed Pre-Review Status
SummaryThis PR correctly identifies and fixes the root cause: Four issues were found and auto-fixed directly on the PR branch (commit Issues Found and Fixed🔴 CRITICAL — Migration backfills 'main' for all existing rowsFile:
Fix applied: Removed - ADD COLUMN IF NOT EXISTS default_branch TEXT DEFAULT 'main';
+ ADD COLUMN IF NOT EXISTS default_branch TEXT;
🟠 HIGH —
|
| Area | Verdict |
|---|---|
| Root cause fix | ✅ Correct — toBranchName(codebase.default_branch) passed to syncWorkspace |
| Branch detection | ✅ Correct — uses git rev-parse --abbrev-ref HEAD post-clone, non-fatal |
| DB layer | ✅ createCodebase now accepts and persists default_branch |
| API schema | ✅ codebase.schemas.ts updated; OpenAPI spec will reflect default_branch |
| Test coverage | ✅ New tests cover: detected branch stored, detached HEAD, rev-parse failure, syncWorkspace branch forwarding |
| Error handling | ✅ Branch detection is non-fatal (falls back to NULL/auto-detect) |
Verification
After pushing fixes, type-check and tests pass:
bun run type-check✅ All 10 packages passbun test packages/core/src/orchestrator/orchestrator.test.ts✅ 70/70 passbun test packages/core/src/orchestrator/orchestrator-agent.test.ts✅ 97/97 passbun test packages/core/src/orchestrator/orchestrator-isolation.test.ts✅ 2/2 pass
… hard-reset to wrong branch (#1273) syncWorkspace was always called with baseBranch=undefined, causing it to auto-detect origin/HEAD (typically main) and run git reset --hard on every message for managed clones, silently destroying feature-branch work. - Add default_branch to Codebase TypeScript interface - Update createCodebase to accept and persist default_branch - Detect actual post-clone branch in cloneRepository and store it - Pass toBranchName(codebase.default_branch) to syncWorkspace - Add default_branch to codebase API schema (OpenAPI) - Add PostgreSQL migration 022 and update 000_combined.sql baseline Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixed: - Add debug log to branch detection catch block in clone.ts (observability) - Update comments to accurately describe NULL → syncWorkspace auto-detect fallback - Add `default_branch` field to `makeCodebase` helper in clone.test.ts - Add `default_branch: null` to findCodebaseByDefaultCwd inline mock in codebases.test.ts - Add test for createCodebase with explicit non-null default_branch Tests added: - 4 branch detection tests in clone.test.ts (happy path, detached HEAD, failure, empty) - 2 syncWorkspace branch forwarding tests in orchestrator.test.ts (non-null + null branch) - Mock @archon/git in orchestrator.test.ts to verify syncWorkspace call arguments Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…anch Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…base type - Migration 022: remove DEFAULT 'main' so existing rows stay NULL instead of being silently set to 'main', which would trigger the same hard-reset-to-main bug for managed clones on non-main branches during upgrade. - orchestrator-agent.test.ts: add default_branch: null to makeCodebase() and makeCodebaseForSync() to satisfy the updated Codebase interface (type-check would fail without this). - orchestrator-isolation.test.ts: add default_branch, repository_url, and ai_assistant_type to makeCodebase() for the same reason. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
leex279
force-pushed
the
fix/issue-1273-sync-workspace-respects-default-branch
branch
from
3df8d06 to
abb6481
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/handlers/clone.ts (1)
288-293: Consider aligning the timeout withgetDefaultBranch(10s).
getDefaultBranchinpackages/git/src/branch.tsuses a 10s timeout for a similar local git metadata query.rev-parse --abbrev-ref HEADis a cheap local read, but on cold I/O (CI, spinning disks, constrained containers) 5s has occasionally been reported as flaky. Since a failure here silently stores NULL and defers to runtime auto-detection, it's not a correctness bug — but using a consistent, slightly more tolerant timeout avoids an unnecessary miss that permanently leavesdefault_branchNULL for this codebase.♻️ Optional adjustment
- ['-C', targetPath, 'rev-parse', '--abbrev-ref', 'HEAD'], - { timeout: 5000 } + ['-C', targetPath, 'rev-parse', '--abbrev-ref', 'HEAD'], + { timeout: 10000 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/handlers/clone.ts` around lines 288 - 293, The timeout for the local git metadata exec in the clone handler is currently 5000ms and should be aligned with getDefaultBranch's 10000ms to avoid flaky failures; update the execFileAsync call that runs 'git -C <targetPath> rev-parse --abbrev-ref HEAD' (the invocation in the clone handler where stdout is read) to use a 10000ms timeout instead of 5000ms so the behavior matches getDefaultBranch's allowance for cold I/O.packages/core/src/handlers/clone.test.ts (1)
306-368: Good coverage of the branch-detection behavior.All four cases the reviewer flagged are now exercised (detected branch forwarded, detached
HEAD→ undefined, rev-parse rejection → non-throwing + undefined, empty stdout → undefined), and assertions check the actualcreateCodebaseargument rather than only the public return shape.One small robustness note: the
args[2] === 'rev-parse'matcher relies on the exact positional layout (-C,<path>,rev-parse, ...). If someone later refactors to['rev-parse', '--abbrev-ref', 'HEAD']without-C, these tests would silently fall through to the default mock anddefault_branchwould beundefined— i.e. the "stores master" test would start failing with a confusing message rather than catching the regression at the matcher. Optionally useargs.includes('rev-parse')(consistent with the other mocks in this file, e.g. Line 641, 687, 863).♻️ Optional — more tolerant matcher
- if (args[0] === '-C' && args[2] === 'rev-parse') { + if (args.includes('rev-parse') && args.includes('--abbrev-ref')) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/handlers/clone.test.ts` around lines 306 - 368, The tests mock spyExecFileAsync to detect git rev-parse by checking args[2] === 'rev-parse', which is brittle; update each mockImplementation in the branch-detection tests (the ones that call spyExecFileAsync used by cloneRepository and assert mockCreateCodebase calls) to use a tolerant matcher like args.includes('rev-parse') so the mock triggers regardless of positional changes and the assertions (createArg.default_branch) remain meaningful.
🤖 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/handlers/clone.test.ts`:
- Around line 306-368: The tests mock spyExecFileAsync to detect git rev-parse
by checking args[2] === 'rev-parse', which is brittle; update each
mockImplementation in the branch-detection tests (the ones that call
spyExecFileAsync used by cloneRepository and assert mockCreateCodebase calls) to
use a tolerant matcher like args.includes('rev-parse') so the mock triggers
regardless of positional changes and the assertions (createArg.default_branch)
remain meaningful.
In `@packages/core/src/handlers/clone.ts`:
- Around line 288-293: The timeout for the local git metadata exec in the clone
handler is currently 5000ms and should be aligned with getDefaultBranch's
10000ms to avoid flaky failures; update the execFileAsync call that runs 'git -C
<targetPath> rev-parse --abbrev-ref HEAD' (the invocation in the clone handler
where stdout is read) to use a 10000ms timeout instead of 5000ms so the behavior
matches getDefaultBranch's allowance for cold I/O.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3444641d-8ef1-4d88-a407-96688a3eb492
📒 Files selected for processing (12)
migrations/000_combined.sqlmigrations/022_add_default_branch_to_codebases.sqlpackages/core/src/db/codebases.test.tspackages/core/src/db/codebases.tspackages/core/src/handlers/clone.test.tspackages/core/src/handlers/clone.tspackages/core/src/orchestrator/orchestrator-agent.test.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/orchestrator/orchestrator-isolation.test.tspackages/core/src/orchestrator/orchestrator.test.tspackages/core/src/types/index.tspackages/server/src/routes/schemas/codebase.schemas.ts
✅ Files skipped from review due to trivial changes (6)
- migrations/000_combined.sql
- packages/core/src/orchestrator/orchestrator-agent.ts
- packages/core/src/orchestrator/orchestrator-agent.test.ts
- packages/core/src/orchestrator/orchestrator.test.ts
- packages/core/src/orchestrator/orchestrator-isolation.test.ts
- packages/core/src/db/codebases.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- migrations/022_add_default_branch_to_codebases.sql
- packages/core/src/types/index.ts
- packages/core/src/db/codebases.ts
🔍 Comprehensive PR ReviewPR: #1274 — fix(core): pass configured default_branch to syncWorkspace to prevent hard-reset to wrong branch SummaryThe core design is sound: non-fatal branch detection after clone, safe
🟠 HIGH — Must Fix Before MergeH-1: Missing
|
| # | Issue | Location | Suggestion |
|---|---|---|---|
| L-1 | toBranchName missing from @archon/git mock |
orchestrator-agent.test.ts:189-192 |
Add toBranchName: mock((b: string) => b) — traps future test authors adding non-null branch tests |
| L-2 | Typo: codbaseWithBranch (missing 'e') |
orchestrator.test.ts:672 |
Rename to codebaseWithBranch |
| L-3 | Branch detect failure logged at debug instead of warn |
clone.ts catch block |
Upgrade to warn — aids debugging "why did sync reset to wrong branch" without --verbose |
| L-4 | Schema allows whitespace-only default_branch |
codebase.schemas.ts |
z.string().trim().min(1).nullable() — whitespace-only passes toBranchName and causes recurring sync warnings |
| L-5 | Catch comment says "store NULL" but TS variable is undefined |
clone.ts catch block |
Minor precision nit — recommend leaving as-is, intent is clear |
| L-6 | Migration list in database.md missing entries 021 and 022 |
packages/docs-web/.../database.md |
Pre-existing gap; back-fill both entries (two one-line additions) |
✅ What's Good
- Non-fatal branch detection is correctly designed —
try/catcharoundrev-parsestoresundefined,syncWorkspaceauto-detects at runtime. Comment above the block explains both paths clearly. - 4 edge-case tests in
clone.test.tscover all meaningful states: normal branch, detached HEAD (HEADoutput), rev-parse failure, empty stdout. - Migration 022 deliberately omits DEFAULT — existing rows stay NULL rather than being silently set to
'main'. The migration comment explains the reasoning; this is exactly the right call and prevents future maintainers from "fixing" the absent DEFAULT. HEADdetached state guard correctly filters out the literal string'HEAD', storingundefinedinstead of a nonsensical branch name.orchestrator.test.tsmocking is model-quality:mockToBranchNameis defined, added to the module mock, cleared inclearAllMocks(), and exercised in both branch-forwarding tests.- Truthy guard before
toBranchNamehandles bothnulland"", avoiding the knownBranchName cannot be emptythrow. execFileAsyncused forrev-parse(notexec), consistent with project conventions.
📋 Suggested Follow-up Issues
| Title | Priority |
|---|---|
Add non-null default_branch forwarding test to orchestrator-agent.test.ts |
P3 |
Back-fill migration entries 021 + 022 in database.md |
P3 |
Support updating default_branch via /update-project and Web UI |
P2 |
registerRepository: detect and persist default_branch like cloneRepository |
P2 |
Reviewed by Archon comprehensive-pr-review workflow · 5 agents · Artifacts: /.archon/workspaces/coleam00/Archon/artifacts/runs/b8f0f7fd-9582-4226-929e-e15ebe2359b7/review/
… WorkspaceSyncResult mock shape Fixes two code review issues from PR #1274: - H-1: Add ALTER TABLE migration for default_branch column in remote_agent_codebases so existing SQLite users get the column without data loss (NULL means syncWorkspace auto-detects the branch) - M-1: Fix WorkspaceSyncResult mock in orchestrator.test.ts to return the correct shape matching the interface (branch/synced/previousHead/newHead/updated) and import BranchName for proper type casting Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code review fixes applied (commit b1f93a6)H-1 — SQLite
|
1 participant
Summary
Fixes #1273 —
syncWorkspacewas always called withbaseBranch=undefined, causing it to auto-detectorigin/HEAD(typicallymain) and rungit reset --hard origin/mainon every message for managed clones. This silently destroyed any feature-branch work in the source clone with no warning.default_branch: string | nullto theCodebaseTypeScript interfacecreateCodebaseinpackages/core/src/db/codebases.tsto accept and persistdefault_branchcloneRepository(usinggit rev-parse --abbrev-ref HEAD) and pass it throughregisterRepoAtPathto the DBsyncWorkspacecall inorchestrator-agent.tsto passtoBranchName(codebase.default_branch)instead ofundefineddefault_branchto the codebase API schema (codebase.schemas.ts) and update the OpenAPI baseline022_add_default_branch_to_codebases.sqland update000_combined.sqlbaselineTest plan
bun run type-checkpasses across all packagesbun run lintpasses with zero warningsbun test packages/core/src/db/codebases.test.ts— all 27 tests pass (updated assertions for new INSERT query)bun test packages/core/src/orchestrator/orchestrator.test.ts— all 68 tests pass (updatedmockCodebaseto includedefault_branch)maindefault branch →default_branchin DB matches actual clone HEAD🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Database
Tests