[Fix] Harden git-backed task sync#5
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR replaces sequential/child task ID allocation with a random canonical ChangesRandom canonical task IDs, sync hardening, and merge driver
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant SyncWorktree
participant Git
participant Origin
CLI->>SyncWorktree: sync_worktree(repo_root, push)
SyncWorktree->>SyncWorktree: acquire write lock
SyncWorktree->>Git: finalize in-progress merge
SyncWorktree->>Git: commit local changes
alt origin not configured
SyncWorktree-->>CLI: local-only result
else remote branch missing
SyncWorktree->>Origin: push_branch_with_status (publish)
else
loop up to 3 attempts
SyncWorktree->>Origin: fetch_branch
SyncWorktree->>Git: merge_tracking_branch
alt conflict detected
SyncWorktree-->>CLI: SYNC_MERGE_CONFLICT
end
SyncWorktree->>Origin: push_branch_with_status
alt push rejected
SyncWorktree->>SyncWorktree: retry
else push succeeds
SyncWorktree-->>CLI: pushed result
end
end
SyncWorktree-->>CLI: SYNC_PUSH_REJECTED (attempts exhausted)
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/store/merge_driver.rs (1)
205-240: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider atomic write for merged output.
write_events_to_pathtruncatespathviaFile::createand writes line-by-line beforesync_all(). A crash or kill mid-write leaves the merge-driver target file partially written/corrupted rather than either the original or the fully-merged content. Writing to a sibling.tmpfile and renaming into place (as done elsewhere in this codebase for durability) would make this crash-safe.🛡️ Proposed atomic-write fix
fn write_events_to_path(path: &Path, events: &[EventRecord]) -> Result<(), TsqError> { - let mut file = fs::File::create(path).map_err(|e| { + let tmp_path = path.with_extension("tmp"); + let mut file = fs::File::create(&tmp_path).map_err(|e| { TsqError::new( "MERGE_WRITE_FAILED", format!("Failed writing merged events to {}: {}", path.display(), e), 2, ) })?; for record in events { let line = serde_json::to_string(record).map_err(|e| { TsqError::new( "MERGE_SERIALIZE_FAILED", format!("Failed serializing merged event: {}", e), 2, ) })?; writeln!(file, "{}", line).map_err(|e| { TsqError::new( "MERGE_WRITE_FAILED", format!("Failed writing merged event line: {}", e), 2, ) })?; } file.sync_all().map_err(|e| { TsqError::new( "MERGE_WRITE_FAILED", format!("Failed syncing merged events file: {}", e), 2, ) })?; + fs::rename(&tmp_path, path).map_err(|e| { + TsqError::new( + "MERGE_WRITE_FAILED", + format!("Failed renaming merged events into {}: {}", path.display(), e), + 2, + ) + })?; + Ok(()) }🤖 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 `@src/store/merge_driver.rs` around lines 205 - 240, `write_events_to_path` currently truncates the target file with `fs::File::create` and writes directly, which can leave partially written merged output if the process crashes mid-write. Update this function to write the serialized `EventRecord`s to a sibling temporary file first, flush/sync it, and then atomically rename it into place so `merge_driver` only ever exposes the old file or the fully merged file. Keep the existing `TsqError` handling and make sure the temp-file creation, write loop, and final rename all surface errors through `write_events_to_path`.src/store/events.rs (1)
700-733: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSplit
src/store/events.rshelpersConflict-marker detection and CRLF handling are fine, but
src/store/events.rsis already 768 LOC, over the 500-line guideline forsrc/**/*.rs. Move event parsing/validation helpers into a submodule.🤖 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 `@src/store/events.rs` around lines 700 - 733, Split the event parsing/validation helpers out of the large events module: keep the conflict-marker detection and CRLF normalization behavior intact, but move the parsing logic from the current `src/store/events.rs` flow into a dedicated submodule so the file stays under the size guideline. Refactor the relevant helper functions used by the event-reading path (the code around the `lines` iteration and `TsqError::new` conflict-marker check) into a separate module, then import and call them from `src/store/events.rs` without changing behavior.Source: Coding guidelines
src/store/git.rs (1)
160-646: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftFile exceeds the 500 LOC guideline.
This file is now roughly ~990 lines (test module ends around line 992), and this PR adds another ~150 lines of git subprocess helpers (push/merge/fetch/branch-status). As per path instructions,
src/**/*.rs: "Keep individual files under 500 LOC." Consider splitting sync-related git helpers (push/fetch/merge/status:push_branch_with_status,MergeStatus,merge_in_progress,unmerged_paths,remote_has_branch,fetch_branch,merge_tracking_branch,finalize_merge) into a dedicated module (e.g.src/store/git_sync.rs).🤖 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 `@src/store/git.rs` around lines 160 - 646, The file has grown well beyond the 500 LOC guideline due to the new git subprocess helpers. Move the sync-related functions and types from `src/store/git.rs` into a dedicated module such as `git_sync.rs`, including `push_current_set_upstream`, `PushOutcome`, `push_branch_with_status`, `MergeStatus`, `merge_in_progress`, `unmerged_paths`, `remote_has_branch`, `fetch_branch`, `merge_tracking_branch`, and `finalize_merge`. Keep `git.rs` focused on shared utilities and update any call sites/imports to reference the new module so the file stays under the size limit.Source: Path instructions
🤖 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.
Inline comments:
In `@docs/sync.md`:
- Around line 40-41: The sync docs currently imply that a missing remote branch
causes push to be skipped, but `sync_worktree_locked` still publishes the sync
branch with `push -u` when `origin` exists and only skips pushing when there is
no `origin` remote at all. Update the sentence in the `tsq sync` documentation
to split these cases clearly, using `sync_worktree_locked` and `push -u` as the
reference points so the wording matches the actual behavior.
In `@src/app/sync.rs`:
- Around line 247-351: The sync logic in sync_worktree_locked has pushed the
file over the 500 LOC guideline, so split the new publish/retry orchestration
out into a separate module or helper functions. Keep sync_worktree_locked
focused on high-level flow and move the retry loop, push/merge handling, and
merge_conflict_error-related logic into a dedicated helper with a clear name so
the main file stays under the size limit.
- Line 244: The sync path in sync_worktree_locked currently holds the write lock
while running fetch_branch, merge_tracking_branch, and push_branch_with_status,
and those git calls use Command::output() without any deadline. Add a bounded
timeout or transport-level timeout around those subprocesses so a stalled
network operation cannot keep with_write_lock(repo_root, ...) held indefinitely;
locate the fix in sync_worktree_locked and the helper methods it calls, and
ensure hung git children are killed or fail fast before releasing the lock.
- Around line 294-345: The SYNC_PUSH_REJECTED details currently undercount total
push attempts when the initial publish push in sync.rs is rejected and the later
fetch/merge retry loop also exhausts. Update the attempt tracking in the sync
flow around the remote-branch bootstrap and the retry loop so the final
TsqError::new(...).with_details(...) reports the total number of push attempts
across both phases, not just SYNC_PUSH_MAX_ATTEMPTS from the loop. Use the
existing sync-run branch/push logic and the git::push_branch_with_status outcome
handling to carry a cumulative attempt count into the rejected stderr error
details.
In `@src/domain/ids.rs`:
- Around line 65-75: The legacy ID validator in is_legacy_random_root_id is
duplicating the Crockford alphabet inline, which can drift from the shared
source. Update the validation logic to derive its allowed characters from
CROCKFORD_ALPHABET instead of hardcoded ranges, keeping the check in sync with
any future alphabet changes. Use the existing symbols is_legacy_random_root_id
and CROCKFORD_ALPHABET in src/domain/ids.rs to centralize the character set.
In `@src/store/merge_driver.rs`:
- Around line 39-56: `apply_events(&create_empty_state(), &merged)` in
`merge_driver` replays the entire merged history on every merge, which will
scale poorly as the event log grows. Update the validation path around
`merge_events`/`apply_events` to avoid full from-scratch replay when possible,
such as by validating only the newly merged delta or reusing an already-built
state snapshot instead of always starting from `create_empty_state()`.
---
Outside diff comments:
In `@src/store/events.rs`:
- Around line 700-733: Split the event parsing/validation helpers out of the
large events module: keep the conflict-marker detection and CRLF normalization
behavior intact, but move the parsing logic from the current
`src/store/events.rs` flow into a dedicated submodule so the file stays under
the size guideline. Refactor the relevant helper functions used by the
event-reading path (the code around the `lines` iteration and `TsqError::new`
conflict-marker check) into a separate module, then import and call them from
`src/store/events.rs` without changing behavior.
In `@src/store/git.rs`:
- Around line 160-646: The file has grown well beyond the 500 LOC guideline due
to the new git subprocess helpers. Move the sync-related functions and types
from `src/store/git.rs` into a dedicated module such as `git_sync.rs`, including
`push_current_set_upstream`, `PushOutcome`, `push_branch_with_status`,
`MergeStatus`, `merge_in_progress`, `unmerged_paths`, `remote_has_branch`,
`fetch_branch`, `merge_tracking_branch`, and `finalize_merge`. Keep `git.rs`
focused on shared utilities and update any call sites/imports to reference the
new module so the file stays under the size limit.
In `@src/store/merge_driver.rs`:
- Around line 205-240: `write_events_to_path` currently truncates the target
file with `fs::File::create` and writes directly, which can leave partially
written merged output if the process crashes mid-write. Update this function to
write the serialized `EventRecord`s to a sibling temporary file first,
flush/sync it, and then atomically rename it into place so `merge_driver` only
ever exposes the old file or the fully merged file. Keep the existing `TsqError`
handling and make sure the temp-file creation, write loop, and final rename all
surface errors through `write_events_to_path`.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4fd02c8d-1893-48f1-ae78-7f76ba690052
📒 Files selected for processing (22)
AGENTS-reference.mdREADME.mddocs/sync.mdsrc/app/service_create_update.rssrc/app/service_utils.rssrc/app/sync.rssrc/domain/ids.rssrc/domain/projector_helpers.rssrc/domain/projector_tasks.rssrc/domain/resolve.rssrc/domain/state.rssrc/store/events.rssrc/store/git.rssrc/store/merge_driver.rssrc/types.rstests/common/mod.rstests/create_children_batch.rstests/event_read_validation.rstests/merge_driver.rstests/readable_identity.rstests/sync_branch.rstests/verb_first_task_commands.rs
💤 Files with no reviewable changes (4)
- src/domain/state.rs
- src/domain/projector_helpers.rs
- src/types.rs
- src/app/service_utils.rs
| When no remote/remote branch exists, `tsq sync` commits locally and reports that | ||
| push was skipped. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc inaccuracy: "no remote branch" does not result in a skipped push.
Per src/app/sync.rs::sync_worktree_locked, when origin exists but doesn't yet have the sync branch, tsq sync actively publishes it via push -u (and only falls back to the fetch/merge/retry loop if that publish push is rejected) — it does not skip the push. Only the "no origin remote at all" case skips push. Suggest splitting this sentence to avoid implying the sync branch is left unpublished when a remote exists.
📝 Proposed doc fix
-When no remote/remote branch exists, `tsq sync` commits locally and reports that
-push was skipped.
+When no `origin` remote exists, `tsq sync` commits locally and reports that push
+was skipped. When `origin` exists but has no sync branch yet, `tsq sync` publishes
+it and sets upstream automatically.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| When no remote/remote branch exists, `tsq sync` commits locally and reports that | |
| push was skipped. | |
| When no `origin` remote exists, `tsq sync` commits locally and reports that push | |
| was skipped. When `origin` exists but has no sync branch yet, `tsq sync` publishes | |
| it and sets upstream automatically. |
🤖 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 `@docs/sync.md` around lines 40 - 41, The sync docs currently imply that a
missing remote branch causes push to be skipped, but `sync_worktree_locked`
still publishes the sync branch with `push -u` when `origin` exists and only
skips pushing when there is no `origin` remote at all. Update the sentence in
the `tsq sync` documentation to split these cases clearly, using
`sync_worktree_locked` and `push -u` as the reference points so the wording
matches the actual behavior.
|
|
||
| // Hold the Tasque write lock around the whole commit/fetch/merge/push | ||
| // sequence so concurrent writers can't append events mid-sync. | ||
| with_write_lock(repo_root, || sync_worktree_locked(path, &branch, push)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Sync holds the write lock across unbounded network calls (fetch/merge/push) with no timeout.
sync_worktree now wraps the whole commit/fetch/merge/push retry sequence in with_write_lock (line 244), and the git subprocesses invoked by fetch_branch/merge_tracking_branch/push_branch_with_status run via Command::output() with no timeout. If a network call hangs (unreachable remote, slow transport, auth prompt), the write lock is held indefinitely, and all other concurrent writers (create/edit/etc.) will fail with lock/concurrency errors for as long as the hang lasts, since they only get a short lock-acquire retry.
Consider adding a bounded timeout around the git subprocess calls used during sync (e.g. http.lowSpeedLimit/http.lowSpeedTime for HTTP transports, GIT_SSH_COMMAND with ConnectTimeout for SSH, or a wrapper that kills the child process after a deadline) so a stuck sync can't indefinitely block the rest of the tool.
Also applies to: 312-348
🤖 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 `@src/app/sync.rs` at line 244, The sync path in sync_worktree_locked currently
holds the write lock while running fetch_branch, merge_tracking_branch, and
push_branch_with_status, and those git calls use Command::output() without any
deadline. Add a bounded timeout or transport-level timeout around those
subprocesses so a stalled network operation cannot keep
with_write_lock(repo_root, ...) held indefinitely; locate the fix in
sync_worktree_locked and the helper methods it calls, and ensure hung git
children are killed or fail fast before releasing the lock.
| fn sync_worktree_locked(path: &Path, branch: &str, push: bool) -> Result<SyncRunResult, TsqError> { | ||
| let worktree_path = path.to_string_lossy().to_string(); | ||
|
|
||
| // Finalize any merge left in progress by a prior conflicted sync. If | ||
| // conflicts remain unresolved, re-emit the structured conflict error before | ||
| // any local commit path can stage conflict markers. | ||
| let mut committed = false; | ||
| if git::merge_in_progress(path)? { | ||
| let unmerged = git::unmerged_paths(path)?; | ||
| if !unmerged.is_empty() { | ||
| return Err(merge_conflict_error(branch, &worktree_path, unmerged)); | ||
| } | ||
| git::finalize_merge(path)?; | ||
| committed = true; | ||
| } | ||
|
|
||
| // `--no-push`: purely local commit, no network. | ||
| if !push { | ||
| if git::commit_worktree(path, SYNC_COMMIT_MESSAGE)? { | ||
| committed = true; | ||
| } | ||
| return Ok(SyncRunResult { | ||
| branch: branch.to_string(), | ||
| worktree_path, | ||
| committed, | ||
| pushed: false, | ||
| has_upstream: git::has_upstream(path)?, | ||
| }); | ||
| } | ||
|
|
||
| // Commit local changes first. | ||
| if git::commit_worktree(path, SYNC_COMMIT_MESSAGE)? { | ||
| committed = true; | ||
| } | ||
|
|
||
| // Origin-only sync: no remote configured means local commit only. | ||
| if !git::has_remote(path, "origin")? { | ||
| return Ok(SyncRunResult { | ||
| branch: branch.to_string(), | ||
| worktree_path, | ||
| committed, | ||
| pushed: false, | ||
| has_upstream: false, | ||
| }); | ||
| } | ||
| let remote = "origin"; | ||
|
|
||
| // No remote branch yet: publish and set upstream. If another clone creates | ||
| // it after our check but before our push, fall through to the same | ||
| // fetch/merge/retry loop used for established branches. | ||
| if !git::remote_has_branch(path, remote, branch)? { | ||
| match git::push_branch_with_status(path, remote, branch)? { | ||
| git::PushOutcome::Ok => { | ||
| return Ok(SyncRunResult { | ||
| branch: branch.to_string(), | ||
| worktree_path, | ||
| committed, | ||
| pushed: true, | ||
| has_upstream: true, | ||
| }); | ||
| } | ||
| git::PushOutcome::Rejected(_) => {} | ||
| } | ||
| } | ||
|
|
||
| // Remote branch exists: fetch + merge before pushing, with bounded retry on | ||
| // non-fast-forward rejection (remote advanced during our merge/push). | ||
| for attempt in 0..SYNC_PUSH_MAX_ATTEMPTS { | ||
| git::fetch_branch(path, remote, branch)?; | ||
| match git::merge_tracking_branch(path, remote, branch)? { | ||
| git::MergeStatus::Clean => {} | ||
| git::MergeStatus::Conflict(paths) => { | ||
| return Err(merge_conflict_error(branch, &worktree_path, paths)); | ||
| } | ||
| } | ||
| match git::push_branch_with_status(path, remote, branch)? { | ||
| git::PushOutcome::Ok => { | ||
| return Ok(SyncRunResult { | ||
| branch: branch.to_string(), | ||
| worktree_path, | ||
| committed, | ||
| pushed: true, | ||
| has_upstream: true, | ||
| }); | ||
| } | ||
| git::PushOutcome::Rejected(stderr) => { | ||
| if attempt + 1 >= SYNC_PUSH_MAX_ATTEMPTS { | ||
| return Err(TsqError::new( | ||
| "SYNC_PUSH_REJECTED", | ||
| "remote rejected push after retrying fetch/merge", | ||
| 2, | ||
| ) | ||
| .with_details(serde_json::json!({ | ||
| "branch": branch, | ||
| "remote": remote, | ||
| "attempts": SYNC_PUSH_MAX_ATTEMPTS, | ||
| "stderr": crate::cli::render::sanitize_inline(&stderr), | ||
| }))); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| unreachable!("push retry loop returns on success, conflict, or exhausted attempts") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
File likely exceeds the 500 LOC guideline.
Shown content already spans to line ~594, and this PR adds ~130 more lines (sync_worktree_locked, retry loop, merge_conflict_error). As per path instructions, src/**/*.rs: "Keep individual files under 500 LOC." Consider splitting the retry/publish orchestration in sync_worktree_locked into its own module.
🤖 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 `@src/app/sync.rs` around lines 247 - 351, The sync logic in
sync_worktree_locked has pushed the file over the 500 LOC guideline, so split
the new publish/retry orchestration out into a separate module or helper
functions. Keep sync_worktree_locked focused on high-level flow and move the
retry loop, push/merge handling, and merge_conflict_error-related logic into a
dedicated helper with a clear name so the main file stays under the size limit.
Source: Path instructions
| // No remote branch yet: publish and set upstream. If another clone creates | ||
| // it after our check but before our push, fall through to the same | ||
| // fetch/merge/retry loop used for established branches. | ||
| if !git::remote_has_branch(path, remote, branch)? { | ||
| match git::push_branch_with_status(path, remote, branch)? { | ||
| git::PushOutcome::Ok => { | ||
| return Ok(SyncRunResult { | ||
| branch: branch.to_string(), | ||
| worktree_path, | ||
| committed, | ||
| pushed: true, | ||
| has_upstream: true, | ||
| }); | ||
| } | ||
| git::PushOutcome::Rejected(_) => {} | ||
| } | ||
| } | ||
|
|
||
| // Remote branch exists: fetch + merge before pushing, with bounded retry on | ||
| // non-fast-forward rejection (remote advanced during our merge/push). | ||
| for attempt in 0..SYNC_PUSH_MAX_ATTEMPTS { | ||
| git::fetch_branch(path, remote, branch)?; | ||
| match git::merge_tracking_branch(path, remote, branch)? { | ||
| git::MergeStatus::Clean => {} | ||
| git::MergeStatus::Conflict(paths) => { | ||
| return Err(merge_conflict_error(branch, &worktree_path, paths)); | ||
| } | ||
| } | ||
| match git::push_branch_with_status(path, remote, branch)? { | ||
| git::PushOutcome::Ok => { | ||
| return Ok(SyncRunResult { | ||
| branch: branch.to_string(), | ||
| worktree_path, | ||
| committed, | ||
| pushed: true, | ||
| has_upstream: true, | ||
| }); | ||
| } | ||
| git::PushOutcome::Rejected(stderr) => { | ||
| if attempt + 1 >= SYNC_PUSH_MAX_ATTEMPTS { | ||
| return Err(TsqError::new( | ||
| "SYNC_PUSH_REJECTED", | ||
| "remote rejected push after retrying fetch/merge", | ||
| 2, | ||
| ) | ||
| .with_details(serde_json::json!({ | ||
| "branch": branch, | ||
| "remote": remote, | ||
| "attempts": SYNC_PUSH_MAX_ATTEMPTS, | ||
| "stderr": crate::cli::render::sanitize_inline(&stderr), | ||
| }))); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
SYNC_PUSH_REJECTED.details.attempts undercounts actual push attempts.
When the "no remote branch yet" publish push (lines 297-310) is itself rejected (branch appeared concurrently) and the subsequent retry loop is then exhausted, the reported attempts field is SYNC_PUSH_MAX_ATTEMPTS (3), omitting the earlier publish attempt. The error message reads "remote rejected push after retrying fetch/merge," which is accurate for the loop but the attempts count under-reports the real number of push attempts made, which could mislead debugging.
♻️ Proposed fix: track total attempts across publish + retry loop
- if !git::remote_has_branch(path, remote, branch)? {
- match git::push_branch_with_status(path, remote, branch)? {
+ let mut total_attempts = 0usize;
+ if !git::remote_has_branch(path, remote, branch)? {
+ total_attempts += 1;
+ match git::push_branch_with_status(path, remote, branch)? {
git::PushOutcome::Ok => {
return Ok(SyncRunResult {
branch: branch.to_string(),
worktree_path,
committed,
pushed: true,
has_upstream: true,
});
}
git::PushOutcome::Rejected(_) => {}
}
}
for attempt in 0..SYNC_PUSH_MAX_ATTEMPTS {
+ total_attempts += 1;
git::fetch_branch(path, remote, branch)?;
...
git::PushOutcome::Rejected(stderr) => {
if attempt + 1 >= SYNC_PUSH_MAX_ATTEMPTS {
return Err(TsqError::new(...)
.with_details(serde_json::json!({
"branch": branch,
"remote": remote,
- "attempts": SYNC_PUSH_MAX_ATTEMPTS,
+ "attempts": total_attempts,
"stderr": ...,
})));
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // No remote branch yet: publish and set upstream. If another clone creates | |
| // it after our check but before our push, fall through to the same | |
| // fetch/merge/retry loop used for established branches. | |
| if !git::remote_has_branch(path, remote, branch)? { | |
| match git::push_branch_with_status(path, remote, branch)? { | |
| git::PushOutcome::Ok => { | |
| return Ok(SyncRunResult { | |
| branch: branch.to_string(), | |
| worktree_path, | |
| committed, | |
| pushed: true, | |
| has_upstream: true, | |
| }); | |
| } | |
| git::PushOutcome::Rejected(_) => {} | |
| } | |
| } | |
| // Remote branch exists: fetch + merge before pushing, with bounded retry on | |
| // non-fast-forward rejection (remote advanced during our merge/push). | |
| for attempt in 0..SYNC_PUSH_MAX_ATTEMPTS { | |
| git::fetch_branch(path, remote, branch)?; | |
| match git::merge_tracking_branch(path, remote, branch)? { | |
| git::MergeStatus::Clean => {} | |
| git::MergeStatus::Conflict(paths) => { | |
| return Err(merge_conflict_error(branch, &worktree_path, paths)); | |
| } | |
| } | |
| match git::push_branch_with_status(path, remote, branch)? { | |
| git::PushOutcome::Ok => { | |
| return Ok(SyncRunResult { | |
| branch: branch.to_string(), | |
| worktree_path, | |
| committed, | |
| pushed: true, | |
| has_upstream: true, | |
| }); | |
| } | |
| git::PushOutcome::Rejected(stderr) => { | |
| if attempt + 1 >= SYNC_PUSH_MAX_ATTEMPTS { | |
| return Err(TsqError::new( | |
| "SYNC_PUSH_REJECTED", | |
| "remote rejected push after retrying fetch/merge", | |
| 2, | |
| ) | |
| .with_details(serde_json::json!({ | |
| "branch": branch, | |
| "remote": remote, | |
| "attempts": SYNC_PUSH_MAX_ATTEMPTS, | |
| "stderr": crate::cli::render::sanitize_inline(&stderr), | |
| }))); | |
| } | |
| // No remote branch yet: publish and set upstream. If another clone creates | |
| // it after our check but before our push, fall through to the same | |
| // fetch/merge/retry loop used for established branches. | |
| let mut total_attempts = 0usize; | |
| if !git::remote_has_branch(path, remote, branch)? { | |
| total_attempts += 1; | |
| match git::push_branch_with_status(path, remote, branch)? { | |
| git::PushOutcome::Ok => { | |
| return Ok(SyncRunResult { | |
| branch: branch.to_string(), | |
| worktree_path, | |
| committed, | |
| pushed: true, | |
| has_upstream: true, | |
| }); | |
| } | |
| git::PushOutcome::Rejected(_) => {} | |
| } | |
| } | |
| // Remote branch exists: fetch + merge before pushing, with bounded retry on | |
| // non-fast-forward rejection (remote advanced during our merge/push). | |
| for attempt in 0..SYNC_PUSH_MAX_ATTEMPTS { | |
| total_attempts += 1; | |
| git::fetch_branch(path, remote, branch)?; | |
| match git::merge_tracking_branch(path, remote, branch)? { | |
| git::MergeStatus::Clean => {} | |
| git::MergeStatus::Conflict(paths) => { | |
| return Err(merge_conflict_error(branch, &worktree_path, paths)); | |
| } | |
| } | |
| match git::push_branch_with_status(path, remote, branch)? { | |
| git::PushOutcome::Ok => { | |
| return Ok(SyncRunResult { | |
| branch: branch.to_string(), | |
| worktree_path, | |
| committed, | |
| pushed: true, | |
| has_upstream: true, | |
| }); | |
| } | |
| git::PushOutcome::Rejected(stderr) => { | |
| if attempt + 1 >= SYNC_PUSH_MAX_ATTEMPTS { | |
| return Err(TsqError::new( | |
| "SYNC_PUSH_REJECTED", | |
| "remote rejected push after retrying fetch/merge", | |
| 2, | |
| ) | |
| .with_details(serde_json::json!({ | |
| "branch": branch, | |
| "remote": remote, | |
| "attempts": total_attempts, | |
| "stderr": crate::cli::render::sanitize_inline(&stderr), | |
| }))); | |
| } |
🤖 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 `@src/app/sync.rs` around lines 294 - 345, The SYNC_PUSH_REJECTED details
currently undercount total push attempts when the initial publish push in
sync.rs is rejected and the later fetch/merge retry loop also exhausts. Update
the attempt tracking in the sync flow around the remote-branch bootstrap and the
retry loop so the final TsqError::new(...).with_details(...) reports the total
number of push attempts across both phases, not just SYNC_PUSH_MAX_ATTEMPTS from
the loop. Use the existing sync-run branch/push logic and the
git::push_branch_with_status outcome handling to carry a cumulative attempt
count into the rejected stderr error details.
| fn is_legacy_random_root_id(raw: &str) -> bool { | ||
| let Some(rest) = raw.strip_prefix("tsq-") else { | ||
| return false; | ||
| }; | ||
| rest.len() == 8 | ||
| && rest | ||
| .chars() | ||
| .all(|ch| matches!(ch, '0'..='9' | 'a'..='h' | 'j'..='k' | 'm'..='n' | 'p'..='t' | 'v'..='z')) | ||
| } | ||
|
|
||
| pub fn next_child_id(state: &State, parent_id: &str) -> String { | ||
| let max_child = state.child_counters.get(parent_id).copied().unwrap_or(0); | ||
| format!("{}.{}", parent_id, max_child + 1) | ||
| } | ||
|
|
||
| fn sequential_number(raw: &str) -> Option<u64> { | ||
| if !is_sequential_root_id(raw) { | ||
| return None; | ||
| } | ||
| let suffix = raw.strip_prefix("tsq-")?; | ||
| suffix.parse::<u64>().ok() | ||
| && rest.chars().all(|ch| { | ||
| matches!( | ||
| ch, | ||
| '0'..='9' | 'a'..='h' | 'j'..='k' | 'm'..='n' | 'p'..='t' | 'v'..='z' | ||
| ) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Derive the legacy-random char set from CROCKFORD_ALPHABET to avoid drift.
is_legacy_random_root_id hardcodes the Crockford set as inline char ranges ('0'..='9' | 'a'..='h' | 'j'..='k' | 'm'..='n' | 'p'..='t' | 'v'..='z'). It currently matches CROCKFORD_ALPHABET exactly, but minting and validation now maintain two independent copies of the same alphabet; a future edit to one can silently diverge and accept/reject IDs inconsistently. Consider validating against the shared constant.
♻️ Reuse the shared alphabet
fn is_legacy_random_root_id(raw: &str) -> bool {
let Some(rest) = raw.strip_prefix("tsq-") else {
return false;
};
- rest.len() == 8
- && rest.chars().all(|ch| {
- matches!(
- ch,
- '0'..='9' | 'a'..='h' | 'j'..='k' | 'm'..='n' | 'p'..='t' | 'v'..='z'
- )
- })
+ rest.len() == RANDOM_ID_LEN
+ && rest
+ .bytes()
+ .all(|b| CROCKFORD_ALPHABET.contains(&b))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn is_legacy_random_root_id(raw: &str) -> bool { | |
| let Some(rest) = raw.strip_prefix("tsq-") else { | |
| return false; | |
| }; | |
| rest.len() == 8 | |
| && rest | |
| .chars() | |
| .all(|ch| matches!(ch, '0'..='9' | 'a'..='h' | 'j'..='k' | 'm'..='n' | 'p'..='t' | 'v'..='z')) | |
| } | |
| pub fn next_child_id(state: &State, parent_id: &str) -> String { | |
| let max_child = state.child_counters.get(parent_id).copied().unwrap_or(0); | |
| format!("{}.{}", parent_id, max_child + 1) | |
| } | |
| fn sequential_number(raw: &str) -> Option<u64> { | |
| if !is_sequential_root_id(raw) { | |
| return None; | |
| } | |
| let suffix = raw.strip_prefix("tsq-")?; | |
| suffix.parse::<u64>().ok() | |
| && rest.chars().all(|ch| { | |
| matches!( | |
| ch, | |
| '0'..='9' | 'a'..='h' | 'j'..='k' | 'm'..='n' | 'p'..='t' | 'v'..='z' | |
| ) | |
| }) | |
| fn is_legacy_random_root_id(raw: &str) -> bool { | |
| let Some(rest) = raw.strip_prefix("tsq-") else { | |
| return false; | |
| }; | |
| rest.len() == RANDOM_ID_LEN | |
| && rest | |
| .bytes() | |
| .all(|b| CROCKFORD_ALPHABET.contains(&b)) |
🤖 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 `@src/domain/ids.rs` around lines 65 - 75, The legacy ID validator in
is_legacy_random_root_id is duplicating the Crockford alphabet inline, which can
drift from the shared source. Update the validation logic to derive its allowed
characters from CROCKFORD_ALPHABET instead of hardcoded ranges, keeping the
check in sync with any future alphabet changes. Use the existing symbols
is_legacy_random_root_id and CROCKFORD_ALPHABET in src/domain/ids.rs to
centralize the character set.
| fn order_key(id: &str, records: &HashMap<String, EventRecord>) -> Reverse<(String, String)> { | ||
| let record = records.get(id).expect("id present in map"); | ||
| Reverse((record.ts.clone(), id.to_string())) | ||
| } | ||
|
|
||
| /// Merge three versions of an events.jsonl file (ancestor, ours, theirs). | ||
| /// | ||
| /// Algorithm: | ||
| /// 1. Read all three files | ||
| /// 2. Build a stable union keyed by event ID | ||
| /// 2. Build a union keyed by event ID (prefers `id`, falls back to `event_id`) | ||
| /// 3. Detect conflicts: same ID but different payload across files | ||
| /// 4. Deduplicate identical events | ||
| /// 5. Preserve source order: ancestor, then ours-only, then theirs-only | ||
| /// 5. Produce a deterministic order via a topological sort that: | ||
| /// - preserves each source's internal causal order (event[i] before event[i+1]) | ||
| /// - breaks ties by smallest event ID (min-heap) | ||
| /// This makes the merged output byte-identical for `A<-B` and `B<-A` when | ||
| /// both sides add independent events, while still keeping causal chains | ||
| /// (e.g. create-before-update) intact even when IDs sort backwards. |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
LGTM! The causal-edge + Kahn topological sort with (ts, id) tie-breaking, cycle fallback, and pre-write replay validation are correctly implemented and match the new test expectations (causal order, cross-timezone tie-breaks, commutativity, cycle fallback, replay-failure rollback).
One scalability note: apply_events(&create_empty_state(), &merged) fully replays the entire merged event stream from scratch on every merge invocation. For large event logs this could become a noticeable cost on every git merge/sync cycle; worth keeping an eye on as event history grows.
Also applies to: 68-193
🤖 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 `@src/store/merge_driver.rs` around lines 39 - 56,
`apply_events(&create_empty_state(), &merged)` in `merge_driver` replays the
entire merged history on every merge, which will scale poorly as the event log
grows. Update the validation path around `merge_events`/`apply_events` to avoid
full from-scratch replay when possible, such as by validating only the newly
merged delta or reusing an already-built state snapshot instead of always
starting from `create_empty_state()`.
1 participant
Summary
Changes
origin/tsq-syncappears betweenls-remoteand first publish push.tsq sync --no-pushfrom committing unresolved conflict markers.(ts, event id).Testing
cargo test --test sync_branch sync_retries_when_remote_branch_appears_during_publish -- --nocapturecargo test --test sync_branch sync_conflict_returns_structured_error_and_resumes -- --nocapturecargo test --test merge_driver test_merge_driver_concurrent_updates_use_timestamp_order -- --nocapturecargo test --test event_read_validation read_events_rejects_non_rfc3339_timestamp_as_corrupt -- --nocapturecargo fmt --checkcargo clippy --all-targets --all-features -- -D warningscargo test --quietcargo build --release~/.local/bin/tsq --version→tsq 0.9.0Review Notes
EVENTS_CORRUPT; native writers already emit canonical RFC3339.--allow-unrelated-historiesis intentional for independently-created sync branches; merged event streams are still replay-validated.