Fix #1516: non-destructive sync default + post-message reminder#1536
Fix #1516: non-destructive sync default + post-message reminder#1536ztech-gthb wants to merge 13 commits into
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 ChangesDefault Branch Tracking & Sync Mode Refactor
Sequence DiagramsequenceDiagram
participant Client
participant CloneHandler as Clone Handler
participant DB as Database
participant Orchestrator
participant Git as Git Utils
participant SyncManager as Sync Manager
Client->>CloneHandler: request clone
CloneHandler->>Git: git clone <repo>
Git-->>CloneHandler: cloned
CloneHandler->>Git: getCurrentBranch(repoPath)
Git-->>CloneHandler: branch name
CloneHandler->>DB: createCodebase(..., default_branch)
DB-->>CloneHandler: created
Orchestrator->>DB: load codebase
DB-->>Orchestrator: codebase (with default_branch)
Orchestrator->>SyncManager: syncWorkspace(path, default_branch, {mode: 'fast-forward'})
SyncManager->>Git: git fetch origin/<branch>
Git-->>SyncManager: fetch ok
SyncManager->>SyncManager: inspect HEAD vs origin (dirty/ancestor)
alt state == 'behind' and current branch == branchToSync
SyncManager->>Git: git merge --ff-only origin/<branch>
else if mode == 'reset'
SyncManager->>Git: git reset --hard origin/<branch>
else
SyncManager-->>Orchestrator: no-op (preserve local state)
end
SyncManager-->>Orchestrator: WorkspaceSyncResult {state, synced, updated}
Orchestrator->>Git: countCommitsAhead(path, branch)
Git-->>Orchestrator: aheadCount / -1
Orchestrator->>Orchestrator: reportUnpushedWorkInSource if needed
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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/git/src/repo.ts (1)
121-128:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNarrow the “branch not found” detection.
errorMessage.includes('not found')also matches repo-level failures likerepository not found, so callers with a configured base branch can get the wrong remediation path. Match only remote-ref-specific messages here and let the rest fall through to the generic fetch error.Possible fix
if ( baseBranch && - (errorMessage.includes("couldn't find remote ref") || errorMessage.includes('not found')) + ( + errorMessage.includes("couldn't find remote ref") || + /remote ref .* not found/.test(errorMessage) + ) ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/git/src/repo.ts` around lines 121 - 128, The branch-not-found detection is too broad because errorMessage.includes('not found') also matches repo-level errors; update the conditional that checks baseBranch and errorMessage (the block using baseBranch and errorMessage in repo.ts) to only detect remote-ref-specific messages—e.g. keep the existing "couldn't find remote ref" check and replace the generic 'not found' check with a more specific test such as matching "remote ref .*not found" or another remote-ref-specific substring/regex—so repository-level "not found" errors fall through to the generic fetch error handling.packages/core/src/orchestrator/orchestrator-agent.test.ts (1)
1054-1065:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
makeDispatchCodebaseandmakeApprovalCodebaseare missingdefault_branch: null.
makeCodebase(line 217) andmakeCodebaseForSync(line 841) were both updated to includedefault_branch: null, butmakeDispatchCodebase(lines 1054-1065) andmakeApprovalCodebase(lines 1190-1200, same structure) were not. Their return values are missing a field required by theCodebaseinterface. Tests pass only because the mock is typed asPromise<unknown>, so TypeScript doesn't enforce the shape — at runtime,codebase.default_branchresolves toundefined, which coincidentally behaves identically tonullunder?? undefined. If the mock typing is ever tightened toPromise<Codebase>, both helpers will produce type errors.🛠️ Proposed fix
function makeDispatchCodebase() { return { id: 'codebase-1', name: 'test-repo', repository_url: null, default_cwd: '/repos/test-repo', + default_branch: null, ai_assistant_type: 'claude' as const, commands: {}, created_at: new Date(), updated_at: new Date(), }; }Apply the same change to
makeApprovalCodebaseat lines 1190-1200.🤖 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 1054 - 1065, The helpers makeDispatchCodebase and makeApprovalCodebase are missing the required Codebase field default_branch; update both functions to include default_branch: null in their returned object shapes (matching the change already applied to makeCodebase and makeCodebaseForSync) so the mocked objects conform to the Codebase interface and will type-check if the mock promise is tightened to Promise<Codebase>.migrations/000_combined.sql (1)
313-316:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing idempotent
ALTER TABLEfor migration 022 and stale version comment.The upgrade section (lines 274–315) includes an idempotent
ADD COLUMN IF NOT EXISTSfor every prior migration (e.g., migration 021 at lines 313–315), but there is no corresponding entry for migration 022. Existing databases that are brought up to date by replaying000_combined.sql(the intended idempotent upgrade path) will never receivedefault_branchbecause the column only appears in theCREATE TABLE IF NOT EXISTSblock (which is a no-op for pre-existing tables). Additionally, the version comment on line 2 still reads001-020even though migration 021's ALTER is already present here.🛠️ Proposed additions
-- From migration 021: allow_env_keys on codebases ALTER TABLE remote_agent_codebases ADD COLUMN IF NOT EXISTS allow_env_keys BOOLEAN NOT NULL DEFAULT FALSE; + +-- From migration 022: default_branch on codebases +ALTER TABLE remote_agent_codebases + ADD COLUMN IF NOT EXISTS default_branch TEXT;--- Version: Combined (final state after migrations 001-020) +-- Version: Combined (final state after migrations 001-022)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/000_combined.sql` around lines 313 - 316, The upgrade path is missing an idempotent ALTER for migration 022 and the file header version comment is stale; add an idempotent ALTER TABLE for remote_agent_codebases to ADD COLUMN IF NOT EXISTS default_branch (nullable text) so replaying 000_combined.sql will create that column for existing tables, and update the top-of-file version comment (currently "001-020") to include migration 021/022 (e.g. "001-022") so the combined migration header reflects the included changes.
🧹 Nitpick comments (2)
packages/isolation/src/providers/worktree.test.ts (1)
2268-2273: ⚡ Quick winAdd the managed-clone counterpart to this mode assertion.
These cases only exercise the local-repo
fast-forwardpath. Please add a repo-under-getArchonWorkspacesPath()case that expects{ mode: 'reset' }, since that branch is the one preserving the old known-clean-base behavior for managed clones.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/isolation/src/providers/worktree.test.ts` around lines 2268 - 2273, Add a managed-clone test that mirrors the existing assertion but uses a repo path under getArchonWorkspacesPath() and expects mode 'reset'; specifically, invoke whatever triggers syncWorkspaceSpy with a path built from getArchonWorkspacesPath() (e.g., `${getArchonWorkspacesPath()}/owner/repo` or the test helper that produces a managed workspace) and assert syncWorkspaceSpy.toHaveBeenCalledWith('/workspace/owner/repo', undefined, { mode: 'reset' }) so the managed-clone branch is covered alongside the existing fast-forward assertion.packages/core/src/handlers/clone.ts (1)
284-306: ⚡ Quick winConsider delegating to
getCurrentBranchfrom@archon/gitrather than inlining the git command.The block duplicates the logic already in
packages/git/src/branch.ts:getCurrentBranch. Per the coding guidelines,@archon/gitfunctions should be preferred for git operations. The!== 'HEAD'guard on line 294 is necessary becausegit rev-parse --abbrev-ref HEADexits with success in detached HEAD state and outputs the literal stringHEAD, which would corruptdefault_branchif stored as-is. However, that guard can still be applied after callinggetCurrentBranch, keeping the duplication out ofclone.ts:♻️ Proposed refactor
Add
getCurrentBranchto the import from@archon/git:-import { execFileAsync } from '@archon/git'; +import { execFileAsync, getCurrentBranch, toRepoPath as _toRepoPath } from '@archon/git';Replace the inline block (lines 284-299):
- let detectedBranch: string | undefined; - try { - const { stdout } = await execFileAsync( - 'git', - ['-C', targetPath, '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 - } + let detectedBranch: string | undefined; + try { + const branch = await getCurrentBranch(targetPath); + // Discard 'HEAD' (detached HEAD state) — not a valid default branch + if (branch !== 'HEAD') { + detectedBranch = branch; + } + } catch { + // Non-fatal — leave undefined so DB default applies + }Note: if detached-HEAD handling should be a general concern in
@archon/git, consider havinggetCurrentBranchitself returnnullin that case instead.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/core/src/handlers/clone.ts` around lines 284 - 306, Replace the inline git call with the shared helper: import getCurrentBranch from `@archon/git` and use it to determine the branch for targetPath instead of calling execFileAsync directly; assign its result to detectedBranch, then apply the existing guard to ignore the literal 'HEAD' (i.e., if branch && branch !== 'HEAD' set detectedBranch), keep the try/catch semantics so failures remain non-fatal, remove the execFileAsync block and any direct git invocation, and then call registerRepoAtPath(targetPath, `${ownerName}/${repoName}`, workingUrl, detectedBranch) as before.
🤖 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/db/adapters/sqlite.ts`:
- Around line 219-230: The schema mismatch comes from declaring
remote_agent_codebases.default_branch with a DEFAULT 'main' in createSchema()
while the migration adds the column without a default; update createSchema() so
the remote_agent_codebases table defines default_branch as TEXT (nullable) with
no DEFAULT clause, and ensure any ALTER TABLE in this file (the existing
migration block that runs "ALTER TABLE remote_agent_codebases ADD COLUMN
default_branch TEXT") remains adding a plain nullable TEXT column (no DEFAULT);
target symbols: createSchema(), remote_agent_codebases, and the migration ALTER
TABLE/PRAGMA check around default_branch.
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 920-925: The code checks conversation.codebase_id from the stale
in-memory `conversation` object and can miss newly persisted attachments;
refresh the conversation state before deciding to call
reportUnpushedWorkInSource. After dispatchOrchestratorWorkflow() (or wherever
the DB write happens), re-fetch the conversation (or use the updated
conversation returned by that call) and then perform the check: if
updatedConversation.codebase_id find the attached codebase in `codebases` and
call reportUnpushedWorkInSource(platform, conversationId, attachedCodebase).
Ensure you reference the existing symbols `dispatchOrchestratorWorkflow`,
`conversation`/`updatedConversation`, `conversationId`, `codebases`, and
`reportUnpushedWorkInSource` when locating where to add the refresh.
- Around line 436-439: Before calling syncWorkspace, guard so the merge only
runs when HEAD matches the target branch: obtain the current branch via
getCurrentBranch() and compare it to the branch derived for sync
(toBranchName(codebase.default_branch)); if they do not match, no-op and return
a successful/neutral syncResult. Alternatively, implement this same guard inside
the syncWorkspace implementation in `@archon/git` so syncWorkspace itself checks
HEAD==branchToSync before performing git merge --ff-only origin/<branchToSync>.
Ensure references to syncWorkspace, getCurrentBranch, toBranchName, and
toRepoPath are used to locate and implement the check.
In `@packages/core/src/orchestrator/post-message-reminder.ts`:
- Around line 43-48: The call to toRepoPath(codebase.default_cwd) is executed
outside the try/catch so its thrown error can escape and violate the "Non-fatal"
guarantee; move the toRepoPath(...) (and the branchName derivation that depends
on codebase.default_branch) inside the existing try block (or wrap them in a
small try/catch) so any exception from toRepoPath or toBranchName is caught and
logged at debug and swallowed, preserving the non-fatal behavior of the
post-message-reminder logic where platform.sendStructuredEvent is used.
In `@packages/isolation/src/providers/worktree.ts`:
- Around line 783-786: The current isManagedClone detection uses startsWith on a
stringified repoPath which can false-match siblings; update the logic that sets
mode (variable mode of type SyncMode) so it only treats a repo as managed when
repoPath is actually inside getArchonWorkspacesPath(), e.g. normalize both paths
and either (a) compare path.relative(getArchonWorkspacesPath(), repoPath) and
check it does not start with '..' and is not equal to ''/'. or (b) append a path
separator to the normalized workspaces root and use startsWith(rootWithSep) to
guarantee a boundary — change the isManagedClone calculation to use one of these
approaches before selecting 'reset' vs 'fast-forward'.
---
Outside diff comments:
In `@migrations/000_combined.sql`:
- Around line 313-316: The upgrade path is missing an idempotent ALTER for
migration 022 and the file header version comment is stale; add an idempotent
ALTER TABLE for remote_agent_codebases to ADD COLUMN IF NOT EXISTS
default_branch (nullable text) so replaying 000_combined.sql will create that
column for existing tables, and update the top-of-file version comment
(currently "001-020") to include migration 021/022 (e.g. "001-022") so the
combined migration header reflects the included changes.
In `@packages/core/src/orchestrator/orchestrator-agent.test.ts`:
- Around line 1054-1065: The helpers makeDispatchCodebase and
makeApprovalCodebase are missing the required Codebase field default_branch;
update both functions to include default_branch: null in their returned object
shapes (matching the change already applied to makeCodebase and
makeCodebaseForSync) so the mocked objects conform to the Codebase interface and
will type-check if the mock promise is tightened to Promise<Codebase>.
In `@packages/git/src/repo.ts`:
- Around line 121-128: The branch-not-found detection is too broad because
errorMessage.includes('not found') also matches repo-level errors; update the
conditional that checks baseBranch and errorMessage (the block using baseBranch
and errorMessage in repo.ts) to only detect remote-ref-specific messages—e.g.
keep the existing "couldn't find remote ref" check and replace the generic 'not
found' check with a more specific test such as matching "remote ref .*not found"
or another remote-ref-specific substring/regex—so repository-level "not found"
errors fall through to the generic fetch error handling.
---
Nitpick comments:
In `@packages/core/src/handlers/clone.ts`:
- Around line 284-306: Replace the inline git call with the shared helper:
import getCurrentBranch from `@archon/git` and use it to determine the branch for
targetPath instead of calling execFileAsync directly; assign its result to
detectedBranch, then apply the existing guard to ignore the literal 'HEAD'
(i.e., if branch && branch !== 'HEAD' set detectedBranch), keep the try/catch
semantics so failures remain non-fatal, remove the execFileAsync block and any
direct git invocation, and then call registerRepoAtPath(targetPath,
`${ownerName}/${repoName}`, workingUrl, detectedBranch) as before.
In `@packages/isolation/src/providers/worktree.test.ts`:
- Around line 2268-2273: Add a managed-clone test that mirrors the existing
assertion but uses a repo path under getArchonWorkspacesPath() and expects mode
'reset'; specifically, invoke whatever triggers syncWorkspaceSpy with a path
built from getArchonWorkspacesPath() (e.g.,
`${getArchonWorkspacesPath()}/owner/repo` or the test helper that produces a
managed workspace) and assert
syncWorkspaceSpy.toHaveBeenCalledWith('/workspace/owner/repo', undefined, {
mode: 'reset' }) so the managed-clone branch is covered alongside the existing
fast-forward assertion.
🪄 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: 9f699614-0b74-44ff-9812-fd01ec863f3e
📒 Files selected for processing (17)
migrations/000_combined.sqlmigrations/022_add_default_branch_to_codebases.sqlpackages/core/src/db/adapters/sqlite.tspackages/core/src/db/codebases.tspackages/core/src/handlers/clone.tspackages/core/src/orchestrator/orchestrator-agent.test.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/orchestrator/post-message-reminder.tspackages/core/src/types/index.tspackages/git/src/branch.tspackages/git/src/git.test.tspackages/git/src/index.tspackages/git/src/repo.tspackages/git/src/types.tspackages/isolation/src/providers/worktree.test.tspackages/isolation/src/providers/worktree.tspackages/server/src/routes/schemas/codebase.schemas.ts
|
Tests + CodeRabbit review adressiert. Test status
CodeRabbit findings — all five addressed
Plus one self-spotted fix
Branch is at |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-agent.ts`:
- Around line 1393-1403: getCurrentBranch can return the literal "HEAD" in a
detached HEAD state which currently gets stored as default_branch; change the
logic around getCurrentBranch in orchestrator-agent.ts so that after awaiting
getCurrentBranch(toRepoPath(projectPath)) you treat the returned value "HEAD" as
undefined (i.e., if detectedBranch === 'HEAD' set detectedBranch = undefined)
before calling codebaseDb.createCodebase, and update the inline comment to
reflect that detached HEAD yields undefined rather than relying on a thrown
error or a DB default; reference symbols: getCurrentBranch, detectedBranch,
codebaseDb.createCodebase, default_branch.
🪄 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: 8a794698-b9ca-4e36-8b16-ef05bf39ce01
📒 Files selected for processing (7)
migrations/000_combined.sqlpackages/core/src/db/adapters/sqlite.tspackages/core/src/db/codebases.test.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/orchestrator/post-message-reminder.tspackages/git/src/repo.tspackages/isolation/src/providers/worktree.ts
✅ Files skipped from review due to trivial changes (1)
- migrations/000_combined.sql
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/isolation/src/providers/worktree.ts
- packages/core/src/orchestrator/post-message-reminder.ts
- packages/git/src/repo.ts
|
Untracked-files refinement folded into the PR ( Why this is in the PR, not a follow-upIn the real-world verification (see Human Verification section in the PR body), the FF-banner that should have appeared on a The reasoning, made explicitWhy untracked files are safe across both sync modes:
So the only thing the previous "untracked counts as dirty" logic was defending against was an imaginary threat: a sync path that doesn't exist. Meanwhile, every macOS user with a After this commit, Worktree cleanup is unaffectedThe change is local to Testsbun run type-check # clean across all 10 packages
bun --filter @archon/git test # 143 pass / 0 failNo fixture changes needed — existing tests don't exercise the untracked-files boundary explicitly. Branch is now at |
|
CodeRabbit round 2 actionable addressed. What was missed in round 1The
FixMirror the clone.ts guard in Testsbun run type-check # clean across all 10 packages
bun --filter @archon/core test # 815 pass / 0 failNo fixture changes needed — existing |
|
@ztech-gthb related to #1516 — non-destructive sync + default branch fix. |
|
All Round-1 + Round-2 CodeRabbit findings have been addressed across the commit history (drop |
…1273) Backports the not-yet-merged PR coleam00#1273 fix as a prerequisite for coleam00#1516. syncWorkspace was always called with baseBranch=undefined, causing auto-detection to origin/HEAD and git reset --hard on every message for managed clones. - Codebase.default_branch (nullable) + createCodebase persist - cloneRepository detects post-clone branch and stores it - syncWorkspace receives toBranchName(default_branch) with null→undefined fallback - getCurrentBranch() helper in @archon/git - handleRegisterProject detects current branch on register - Codebase OpenAPI schema (nullable) - Migration 022 (no DEFAULT — preserve NULL on upgrade) - SQLite migrateColumns entry - 000_combined.sql baseline updated
…for worktree-creation (coleam00#1516)
Both PostgreSQL baseline and SQLite createSchema declared DEFAULT 'main', diverging from migration 022 which uses NULL. Fresh installs got 'main', migrated installs got NULL — same hard-reset-to-wrong-branch risk this PR is trying to remove.
…view) Non-fatal contract was broken when default_cwd is empty — toRepoPath throws RepoPath cannot be empty before the try block opens.
…k (CodeRabbit review) startsWith(workspacesPath) matched siblings like workspaces-old/. Normalize trailing slashes and require an explicit / separator (or exact match).
…deRabbit review) Without the branch check, a topic branch that happens to be an ancestor of origin/<default_branch> would silently advance to origin's tip, violating the non-default-branches-preserved guarantee. Use getCurrentBranch and noop unless HEAD == branchToSync.
…tached (CodeRabbit review) dispatchOrchestratorWorkflow may have just persisted codebase_id (auto-attach on first turn), so the in-memory conversation can be stale. Re-read by platform id when codebase_id isn't set in our snapshot.
Untracked files (e.g. .DS_Store on macOS, claude/skill caches) are safe across all syncWorkspace paths — neither git merge --ff-only nor git reset --hard origin/<branch> ever touches untracked files. Treating them as 'dirty' was blocking fast-forward sync on every checkout where the user happens to have local-only files, despite there being no destructive risk to defend against. Add --untracked-files=no to the porcelain check; tracked modifications still block ff-merge as before.
…RegisterProject (CodeRabbit round 2) getCurrentBranch returns the literal string 'HEAD' on detached HEAD, not an exception. he catch block never fires for detached HEAD, so the codebase row ended up with default_branch='HEAD' — which later gets passed to syncWorkspace as a branch name and fails the fetch with 'Configured base branch HEAD not found on remote'. Mirror the existing guard in clone.ts:cloneRepository: only assign the branch when it's a real name.
be9dcb7 to
90bcf0d
Compare
Review SummaryVerdict: blocking-issues This PR makes a good structural improvement — replacing the boolean Blocking issues
Documentation required before mergeThree user-visible changes are entirely undocumented — these are also blocking:
Also worth updating: docs that say "server only sees changes after commit + push" — this is no longer true for chat-mode (only worktree-creation resets managed clones). And the 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. |
- Replace two empty catch{} blocks during branch detection with debug
logging that distinguishes detached-HEAD / non-git-dir from actionable
failures (packages/core/src/handlers/clone.ts,
packages/core/src/orchestrator/orchestrator-agent.ts).
- Document the three user-visible changes the PR introduces: fast-forward
chat-tick sync as the new default with the five-state machine
(in_sync/behind/ahead/diverged/dirty), the unpushed-work SSE advisory,
and the distinction between worktree.baseBranch (YAML override) and
the per-codebase default_branch DB column. Updates the previously stale
"server only sees pushed changes" caveat in the overview.
- CHANGELOG entry under [Unreleased].
Note on the third reported blocker (syncWorkspace still using
resetAfterFetch in orchestrator-agent.ts): not reproducible against
PR HEAD 90bcf0d. The orchestrator call site already passes the new
mode-based API; isManagedClone now lives only in worktree.ts where it
controls mode: 'reset' for managed-clone worktree creation by design.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks @Wirasm — addressed in 3e80374: Blocker 2 — empty
Blocker 3 — Docs ✅
Blocker 1 — ```ts No Could you double-check whether the review ran against an older snapshot? If you can point me at the specific line/commit you read I'll happily re-look. Suggested fixes (Phase B, follow-up commit) — will tackle next:
Nits (Phase C) — separate commit:
|
Review SummaryVerdict: minor-fixes-needed Your PR changes workspace sync from destructive ( 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. |
|
Hi @ztech-gthb — your recent commit addressed the review findings, but the PR has since gone DIRTY from dev moving on. Could you rebase onto current |
|
Superseded by #1864 (merged). #1864 lands the same non-destructive-sync direction you proposed here (fast-forward-only default, surface divergence to the user instead of hard-resetting) and closes both #1273 and #1516. Thanks @ztech-gthb — your reporting + the non-destructive framing drove this. Closing as superseded by the merged fix. |
Summary
discoverAllWorkflowsrunsgit fetch + git reset --hard origin/<default_branch>on the canonicalsource/clone. Local commits, uncommitted modifications, and non-default branches are silently destroyed every chat tick. Verified deterministic, 1:1 with each user message.syncWorkspace'soptions.resetAfterFetch: booleanis replaced byoptions.mode: 'fast-forward' | 'reset'(default'fast-forward'). Chat-tick uses the safe default (fetch + ff-only-merge if behind, else noop). Worktree-creation explicitly opts into'reset'. A post-message reminder warns about unpushed work insource/so the user is informed of local-only state that could be lost on the next destructive sync. Includes the unmerged syncWorkspace always resets managed source clone to origin/main, ignoring configured default_branch #1273 fix as a prerequisite (also closes syncWorkspace always resets managed source clone to origin/main, ignoring configured default_branch #1273).validateAndResolveIsolation), forge-adapter sync paths (syncRepositoryis unchanged), CLI behaviour, the destructive-by-designworktree-creationreset (stillmode: 'reset'). Not in scope: the related UI-truncation issue Web UI silently hydrates long conversations to oldest 200 messages — newest messages disappear after refresh or mid-session SSE recovery (DB is OK, UI bug) #1531 (separate PR planned).UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
orchestrator-agent.tssyncWorkspacedefault_branchfrom DB and uses defaultmode: 'fast-forward'worktree.tssyncWorkspacemode: 'reset'for managed clonesorchestrator-agent.tspost-message-reminder.tshandleMessagecalls reminder if codebase attachedpost-message-reminder.tsgit/branch.ts:countCommitsAheadpost-message-reminder.tsgit/branch.ts:hasUncommittedChangesclone.ts:cloneRepositorygetCurrentBranchdefault_branchhandleRegisterProjectgetCurrentBranch/register-projectdb/codebases.ts:createCodebasedefault_branchcolumnLabel Snapshot
risk: medium— touches the canonical clone of every chat session, but the destructive path is preserved for legitimate callers and the new path is strictly less destructive than the old default.size: Mcore, git, isolation, db, servercore:orchestrator,git:repo,isolation:worktree,core:db.codebasesChange Metadata
bugmultiLinked Issue
dev)Validation Evidence (required)
Specific test runs exercised by the touched code:
CodeRabbit review round 1 — all five actionable comments addressed (commits
1db8f711,4c5fcd7a,7c617811,70a39b10,ef76cd67) plus one self-spotted test fixture gap (0031e945).End-to-end manual verification in production-equivalent setup (Docker Compose, real DB, real codebase): see Human Verification section below.
Security Impact (required)
git fetchis the same network operation as before; this PR does fewer destructive ops, not more.70a39b10) explicitly prevents an unintended HEAD movement when the local checkout is on a topic branch — fixes a subtle correctness gap rather than introducing one.Compatibility / Migration
worktree.ts: locally-registered repos under non-managed paths previously gotresetAfterFetch: false(fetch only); now they getmode: 'fast-forward'(fetch + ff-merge if behind). Discussed inline at the call site; happy to revert to fetch-only for that branch if reviewers prefer the strict pre-PR semantics there.default_branchcolumn toremote_agent_codebases:022_add_default_branch_to_codebases.sql(idempotent, no DEFAULT — preserves NULL on upgrade).migrateColumns()adds the column on adapter init (also no DEFAULT).000_combined.sqlupdated.getDefaultBranchauto-detect). No manual data fix required.Human Verification (required)
End-to-end test in production-equivalent setup (macOS Docker Compose, real SQLite DB, real managed clone of FortiGapMinder repo):
Pre-test: HEAD
ca7c0ed(=origin/main), branchfix/marimo-dag-mo-import, working tree clean.Setup:
git pull --ff-onlyto advance HEAD tofb3ec5c, then committed.gitignore+.agents/skills/marimo-notebook/locally on the feature branch (without pushing). HEAD now 2 commits ahead oforigin/fix/marimo-dag-mo-import.Trigger: free-text chat message "auf welchem Branch stehen wir?" (a read-only-from-the-user-side question that forces the agent into
source/).Observed:
/.archon/workspaces/.../FortiGapMinder/source(= the canonical clone, exactly the path that used to be reset every tick).reset: moving to origin/...entry. HEAD still atddfae43(the latest local commit). Both commits intact.ca7c0ed(orfb3ec5c), both local commits orphaned,.gitignoreand the marimo skill files gone from the working tree.Edge cases checked: state='dirty' from untracked files (current
isDirtycounts untracked → blocks ff-merge — defensive but somewhat over-eager; happy to add a follow-up to use--untracked-files=noif reviewers prefer ff-merging through untracked).What was not verified: the explicit
mode: 'reset'path on a worktree-creation has been exercised by automated unit tests and by anarchon-assistworkflow run in the same setup (which logged tworeset: moving to origin/mainevents as expected). Direct manual verification of preservation across a parallel-push race was not run.Side Effects / Blast Radius (required)
syncWorkspaceand is exercised by the same change.state='dirty'is dominant over ancestor checks (untracked files block ff-merge). Not destructive, but defensive — a follow-up to refine with--untracked-files=nois in mind.default_branchcolumn is consumed in two places (orchestrator-agent.ts:434andworktree.ts:syncWorkspaceBeforeCreate). Existing rows with NULL fall back to auto-detect (= pre-PR behaviour for branch resolution).system_statusevent surfacesFast-forwarded ...anddivergedstates to the UI. Existingworkspace.sync_completeddebug log carries the newstatefield for grepability.Rollback Plan (required)
git revertof the merge commit, redeploy. Any DB rows with non-NULLdefault_branchkeep that column populated; no data loss. The migration is forward-only but harmless if rolled back to old code (column simply unused).source/would showreset: moving to origin/mainafter every chat message, agent commits in the canonical clone would silently disappear.70a39b10) were rolled back without rolling back the rest, a topic branch checked out insource/could silently advance toorigin/<default_branch>tip on astate='behind'chat tick.Risks and Mitigations
resetAfterFetch: false.a71c20f6) is logically separable and the maintainer could squash-merge syncWorkspace always resets managed source clone to origin/main, ignoring configured default_branch #1273's content via this PR if they prefer.statefield onWorkspaceSyncResultis required, not optional — external consumers mocking this type would need to add it.packages/. Test fixtures updated in this PR.🤖 Investigation, write-up, and implementation produced with extensive back-and-forth using Claude — final code, naming choices, and architectural decisions reviewed and accepted by ztech-gthb.
Summary by CodeRabbit