From 63850ade13739ffbfe41280a124b3ef09149aa08 Mon Sep 17 00:00:00 2001 From: Aditya Sharma Date: Wed, 8 Jul 2026 06:34:05 -0700 Subject: [PATCH 1/4] fix(sync): repair git-backed task sync --- AGENTS-reference.md | 8 +- README.md | 17 +- docs/sync.md | 101 ++++++++++ src/app/sync.rs | 151 ++++++++++++-- src/domain/ids.rs | 76 ++++--- src/domain/resolve.rs | 23 ++- src/store/events.rs | 23 ++- src/store/git.rs | 149 +++++++++++++- src/store/merge_driver.rs | 133 ++++++++++--- tests/common/mod.rs | 12 ++ tests/create_children_batch.rs | 30 ++- tests/event_read_validation.rs | 28 +++ tests/merge_driver.rs | 150 ++++++++++++++ tests/readable_identity.rs | 117 +++++++++-- tests/sync_branch.rs | 320 +++++++++++++++++++++++++++++- tests/verb_first_task_commands.rs | 29 ++- 16 files changed, 1225 insertions(+), 142 deletions(-) create mode 100644 docs/sync.md diff --git a/AGENTS-reference.md b/AGENTS-reference.md index 1871b00..de1ace3 100644 --- a/AGENTS-reference.md +++ b/AGENTS-reference.md @@ -65,7 +65,7 @@ Write path: Task fields: -- `id` (`tsq-` root, `.` child); legacy `tsq-<8 crockford base32 chars>` IDs remain valid +- `id` (new tasks mint flat random canonical `tsq-<8 lowercase crockford chars>` to avoid sync collisions; sequential `tsq-` root and `.` child ids remain valid/readable; legacy `tsq-<8 crockford base32 chars>` also valid; `--id` accepts any of these) - `alias` (kebab-case slug generated from the creation title; stable across title edits) - `kind` (`task|feature|epic`) - `title` @@ -160,7 +160,7 @@ Notes: - `tsq labels` - `tsq history [--limit ] [--type ] [--actor ] [--since ]` - `tsq root` -- `tsq sync [--no-push]` +- `tsq sync [--no-push]` — two-way sync: commit local changes, fetch remote (upstream then `origin`), merge, push; sets upstream on first push; `--no-push` commits locally only. See [docs/sync.md](./docs/sync.md) for conflict resolution. - `tsq hooks install [--force]` - `tsq hooks uninstall` - `tsq migrate [--sync-branch|--worktree-name ]` @@ -237,7 +237,9 @@ Error: ## Repo Conventions -- commit `.tasque/events.jsonl` and `.tasque/config.json` +- commit `.gitattributes` (registers `.tasque/events.jsonl merge=tasque-events`) +- in the main code worktree, commit `.tasque/config.json` (the sync-branch pointer) +- task data (`events.jsonl`, `specs/`, `config.json`) is committed by `tsq sync` inside the sync worktree; do not stage it manually - do not commit `.tasque/state.json` - do not create or edit `.tasque/tasks.jsonl` - snapshots optional to commit (default local-only) diff --git a/README.md b/README.md index ba7fbe2..d2941e3 100644 --- a/README.md +++ b/README.md @@ -160,12 +160,18 @@ Git repos default to a dedicated sync worktree: - `tsq init` configures `tsq-sync` by default and redirects data operations there. - Fresh clones fetch the configured sync branch and create the worktree on first use. -- `tsq sync` pushes the sync branch to `origin` and sets upstream automatically when needed. +- `tsq sync` runs local-first two-way sync: commit local changes, fetch the remote + branch (upstream then `origin`), merge, then push — setting upstream on first push. +- `tsq sync --no-push` commits locally without touching the network. - Existing git repos with main-tree `.tasque` data migrate automatically when `tsq` next resolves the project root. - The main worktree keeps `.tasque/config.json` so `tsq` can find the sync branch. - The sync worktree owns the canonical `.tasque/events.jsonl`, specs, snapshots, and cache. +See [`docs/sync.md`](./docs/sync.md) for the full sync workflow, conflict resolution, +and task id model. New tasks mint random canonical ids (`tsq-<8 crockford chars>`); +existing sequential (`tsq-42`) and child (`tsq-42.3`) ids stay valid. + Non-git directories use repo-local `.tasque/`: - `events.jsonl`: canonical append-only event log @@ -177,8 +183,13 @@ Non-git directories use repo-local `.tasque/`: - `.gitignore`: local-only artifacts (`state.json`, `.lock`, `snapshots/`, temp files) - `tasks.jsonl`: legacy state-cache name; read-only fallback when `state.json` is absent, removal target -Recommended commit policy: +Recommended commit policy — main code worktree: -- Commit `.tasque/events.jsonl` and `.tasque/config.json` +- Commit `.tasque/config.json` (pointer to the sync branch) and `.gitattributes` +- `.gitattributes` registers `.tasque/events.jsonl merge=tasque-events`; keep it committed so event merges stay correct - Do not commit `.tasque/state.json` - Do not create or edit `.tasque/tasks.jsonl` + +Task data (`events.jsonl`, `specs/`, `config.json`) lives in the sync worktree and is +committed by `tsq sync`; do not stage it manually. `state.json`, `.lock`, and +`snapshots/` are gitignored there. diff --git a/docs/sync.md b/docs/sync.md new file mode 100644 index 0000000..67dba81 --- /dev/null +++ b/docs/sync.md @@ -0,0 +1,101 @@ +# Task sync across machines + +Tasque is local-first. `tsq sync` is the only path that touches the network, +and only when a remote is configured. It turns the local `.tasque/` data into a +git branch you can share between machines and collaborators. + +## Where task data lives + +In a git repo, `tsq init` moves task data off your code branch into a dedicated +**sync worktree** on a separate branch (default `tsq-sync`, configurable via +`--sync-branch` / `--worktree-name`). Your code branch keeps only +`.tasque/config.json` so Tasque can find the sync branch. + +- `.tasque/events.jsonl` — append-only event log (canonical source of truth). +- `.tasque/specs//spec.md` — task specs. +- `.tasque/config.json` — project settings (in the main worktree). +- `.tasque/state.json`, `.tasque/.lock`, `.tasque/snapshots/` — local-only, + gitignored, always rebuildable. + +## What `tsq sync` does + +`tsq sync` runs a local-first two-way sync in this order: + +1. **Commit** local changes to the sync branch (events, specs, config). +2. **Fetch** the remote branch (upstream first, then `origin`), if it exists. +3. **Merge** fetched changes. `events.jsonl` is merged by the + `tasque-events` driver (see below); other files use git's default merge. +4. **Push** the result back to the remote, setting upstream on first push. + +`tsq sync --no-push` stops after step 1: local commit only, no network. The +bundled pre-push hook runs exactly this so a manual `git push` from the sync +worktree still commits pending task updates first. + +When no remote/remote branch exists, `tsq sync` commits locally and reports that +push was skipped. + +## How conflicts resolve + +Different files merge differently by design: + +- `events.jsonl`: the `tasque-events` driver unions events by event id, + deduplicates identical events, and replay-validates the result. Independent + edits auto-merge. Same event id with divergent payloads, or an invalid merged + history, surfaces a structured `MERGE_*` error. +- `specs//spec.md`: git default 3-way text merge. Conflicting spec edits + leave conflict markers and require manual resolution. +- `config.json`: git default 3-way text merge. If both sides edit config, + resolve it manually. + +The event merge is commutative for independent task creates: pulling A then B +yields the same `events.jsonl` as B then A, because events are deduplicated by +id (ULID `id`, legacy `event_id` accepted on read). + +### Conflict UX + +If a git merge conflict cannot be auto-resolved, `tsq sync` leaves the merge in +progress and returns a structured error with the conflicted paths and the sync +worktree path (`SYNC_MERGE_CONFLICT`). Resolve the conflicts in that worktree, +then re-run `tsq sync`: with no unmerged paths remaining it completes the merge +and pushes; if unmerged paths still exist it re-emits the conflict. + +Spec conflicts in particular must be resolved by a human: open the marked +`spec.md`, pick the intended content, remove the `<<<<<<<` / `=======` / `>>>>>>>` +markers, save, then re-run `tsq sync`. + +## Task IDs + +New tasks get a flat random canonical id of the form `tsq-<8 lowercase crockford +chars>` (for example `tsq-7k3q9x2a`). Random ids avoid collisions when two +machines create tasks independently and later sync. + +Existing ids keep working unchanged: + +- Sequential root ids `tsq-` (for example `tsq-42`) — still valid. +- Child ids `.` (for example `tsq-42.3`) — still valid. +- Legacy 8-char ids `tsq-<8 crockford>` — still valid. + +`--id ` on `tsq create` accepts any of these shapes. Commands that take a +task id accept the id, exact alias, or a unique alias prefix. An exact alias that +matches more than one task returns `TASK_ID_AMBIGUOUS` rather than guessing. + +## Commit policy + +For the **sync worktree** (task data), commit: + +- `.tasque/events.jsonl` +- `.tasque/specs/` (specs are meant to sync) +- `.tasque/config.json` + +Do not commit `.tasque/state.json`, `.tasque/.lock`, or `.tasque/snapshots/` — +they are gitignored and rebuildable. `tsq sync` handles this set automatically; +you should not stage task data manually. + +For the **main code worktree**, commit: + +- `.tasque/config.json` (the pointer to the sync branch) +- `.gitattributes` (it registers `.tasque/events.jsonl merge=tasque-events`) + +`.gitattributes` matters: without the `tasque-events` driver entry, event merges +fall back to git's text merge and can corrupt the log. `tsq init` / +`tsq migrate` ensure the entry exists; keep it committed. diff --git a/src/app/sync.rs b/src/app/sync.rs index aefd461..88e8549 100644 --- a/src/app/sync.rs +++ b/src/app/sync.rs @@ -2,6 +2,7 @@ use crate::errors::TsqError; use crate::store::config::{read_config, write_config}; use crate::store::events::{append_events, read_events}; use crate::store::git; +use crate::store::lock::with_write_lock; use crate::store::paths::get_paths; use crate::types::{ HookInstallResult, HookUninstallResult, MigrateResult, SyncRunResult, SyncSetupResult, @@ -110,7 +111,10 @@ fn setup_sync_branch_locked(repo_root: &str, branch: &str) -> Result Result Result { let path = Path::new(repo_root); if !git::is_git_repo(path) { @@ -232,27 +242,126 @@ pub fn sync_worktree(repo_root: &str, push: bool) -> Result Result { + let worktree_path = path.to_string_lossy().to_string(); + + // `--no-push`: purely local commit, no network. + if !push { + let committed = git::commit_worktree(path, SYNC_COMMIT_MESSAGE)?; + return Ok(SyncRunResult { + branch: branch.to_string(), + worktree_path, + committed, + pushed: false, + has_upstream: git::has_upstream(path)?, + }); + } + + // Finalize any merge left in progress by a prior conflicted sync. If + // conflicts remain unresolved, re-emit the structured conflict error. + 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; + } + + // Commit local changes first. + if git::commit_worktree(path, SYNC_COMMIT_MESSAGE)? { + committed = true; + } + + // Prefer the configured upstream remote, fall back to `origin`. + let Some(remote) = git::current_upstream_remote(path)? else { + return Ok(SyncRunResult { + branch: branch.to_string(), + worktree_path, + committed, + pushed: false, + has_upstream: false, + }); }; - Ok(SyncRunResult { - branch, - worktree_path: path.to_string_lossy().to_string(), - committed, - pushed, - has_upstream, - }) + + // No remote branch yet: publish and set upstream. + if !git::remote_has_branch(path, &remote, branch)? { + git::push_current_set_upstream(path, &remote, branch)?; + return Ok(SyncRunResult { + branch: branch.to_string(), + worktree_path, + committed, + pushed: true, + has_upstream: true, + }); + } + + // 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") +} + +/// Build the structured merge-conflict error returned when `tsq sync` leaves a +/// merge in progress. Callers can inspect `details.conflicted_paths` and +/// `details.worktree_path` to guide resolution, then rerun `tsq sync`. +fn merge_conflict_error( + branch: &str, + worktree_path: &str, + conflicted_paths: Vec, +) -> TsqError { + TsqError::new( + "SYNC_MERGE_CONFLICT", + "sync merge left conflicts; resolve them in the worktree and rerun 'tsq sync'", + 1, + ) + .with_details(serde_json::json!({ + "branch": branch, + "worktree_path": worktree_path, + "conflicted_paths": conflicted_paths, + })) } pub fn auto_commit_if_sync_worktree(repo_root: impl AsRef) -> Result<(), TsqError> { diff --git a/src/domain/ids.rs b/src/domain/ids.rs index 1b094cf..48f29de 100644 --- a/src/domain/ids.rs +++ b/src/domain/ids.rs @@ -1,64 +1,75 @@ use crate::errors::TsqError; use crate::types::State; use once_cell::sync::Lazy; +use rand::RngExt; use regex::Regex; use std::collections::HashSet; static SEQUENTIAL_ROOT_ID: Lazy = Lazy::new(|| Regex::new(r"^tsq-[1-9][0-9]*$").expect("sequential root id regex")); +/// Crockford base32 alphabet (lowercase, excluding i, l, o, u). +/// Matches the accepted legacy random root id shape `tsq-<8 crockford chars>`. +const CROCKFORD_ALPHABET: &[u8] = b"0123456789abcdefghjkmnpqrstvwxyz"; +const RANDOM_ID_LEN: usize = 8; + +fn mint_random_canonical_id() -> String { + let mut rng = rand::rng(); + let mut buf = [0u8; RANDOM_ID_LEN]; + for slot in buf.iter_mut() { + let idx = rng.random_range(0..CROCKFORD_ALPHABET.len()); + *slot = CROCKFORD_ALPHABET[idx]; + } + format!("tsq-{}", std::str::from_utf8(&buf).expect("ascii")) +} + +/// Mint a flat random canonical root id (`tsq-<8 crockford chars>`) that does +/// not collide with any existing task id. Old sequential `tsq-` ids +/// remain valid/readable; new tasks no longer use sequential allocation. pub fn make_root_id(state: &State) -> Result { - let mut next = next_root_number(state)?; loop { - let candidate = format!("tsq-{}", next); + let candidate = mint_random_canonical_id(); if !state.tasks.contains_key(&candidate) { return Ok(candidate); } - next = next.checked_add(1).ok_or_else(id_overflow_error)?; } } +/// Mint a flat random canonical id for a child task. Children share the same +/// canonical id shape as roots (`tsq-<8 crockford chars>`); the parent link is +/// carried by `parent_id`, not encoded in the id. Old `parent.N` ids remain +/// valid/readable; new children no longer use the counter suffix shape. +pub fn next_child_id(state: &State, _parent_id: &str) -> String { + loop { + let candidate = mint_random_canonical_id(); + if !state.tasks.contains_key(&candidate) { + return candidate; + } + } +} + +/// Batch-friendly flat random id allocator. Reserves generated ids against +/// existing state and ids minted earlier in the same batch so a single +/// write lock can produce a consistent set of new tasks. pub struct RootIdAllocator { - next: u64, reserved_ids: HashSet, } impl RootIdAllocator { pub fn new(state: &State) -> Result { Ok(Self { - next: next_root_number(state)?, reserved_ids: state.tasks.keys().cloned().collect(), }) } pub fn next_id(&mut self) -> Result { loop { - let candidate = format!("tsq-{}", self.next); + let candidate = mint_random_canonical_id(); if self.reserved_ids.insert(candidate.clone()) { - if let Some(next) = self.next.checked_add(1) { - self.next = next; - } return Ok(candidate); } - self.next = self.next.checked_add(1).ok_or_else(id_overflow_error)?; - } - } -} - -fn next_root_number(state: &State) -> Result { - let mut max_seen = 0u64; - for task in state.tasks.values() { - if task.parent_id.is_none() { - if let Some(number) = sequential_number(&task.id) { - max_seen = max_seen.max(number); - } } } - max_seen.checked_add(1).ok_or_else(id_overflow_error) -} - -fn id_overflow_error() -> TsqError { - TsqError::new("ID_OVERFLOW", "unable to allocate sequential task id", 2) } pub fn is_valid_root_id(raw: &str) -> bool { @@ -78,16 +89,3 @@ pub fn is_legacy_random_root_id(raw: &str) -> bool { .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 { - if !is_sequential_root_id(raw) { - return None; - } - let suffix = raw.strip_prefix("tsq-")?; - suffix.parse::().ok() -} diff --git a/src/domain/resolve.rs b/src/domain/resolve.rs index 98fdb77..b5cc62a 100644 --- a/src/domain/resolve.rs +++ b/src/domain/resolve.rs @@ -15,12 +15,27 @@ pub fn resolve_task_id(state: &State, raw: &str, exact_id: bool) -> Result = state .tasks .values() - .find(|task| task.alias.to_lowercase() == raw_alias) - { - return Ok(task.id.clone()); + .filter(|task| task.alias.to_lowercase() == raw_alias) + .map(|task| (task.id.clone(), task.alias.clone())) + .collect(); + exact_alias_matches.sort_by(|a, b| a.0.cmp(&b.0)); + match exact_alias_matches.len() { + 1 => return Ok(exact_alias_matches[0].0.clone()), + n if n > 1 => { + return Err( + TsqError::new("TASK_ID_AMBIGUOUS", "Task ID is ambiguous", 1).with_details(json!({ + "input": raw, + "candidates": exact_alias_matches + .into_iter() + .map(|(id, alias)| json!({ "id": id, "alias": alias })) + .collect::>() + })), + ); + } + _ => {} } let mut id_matches: Vec<(String, String)> = state diff --git a/src/store/events.rs b/src/store/events.rs index 98e36f9..02f8114 100644 --- a/src/store/events.rs +++ b/src/store/events.rs @@ -687,10 +687,31 @@ fn parse_events_raw( for (index, line) in lines.iter().enumerate() { let line = line.trim_end_matches('\r'); - if line.trim().is_empty() { + let trimmed = line.trim(); + if trimmed.is_empty() { continue; } + // Reject unresolved git conflict markers up front. These are never + // valid JSON and would otherwise surface as a generic "malformed + // JSONL" error; call them out explicitly so the operator resolves + // the conflict before retrying. + if trimmed.starts_with("<<<<<<<") + || trimmed.starts_with("|||||||") + || trimmed.starts_with(">>>>>>>") + || trimmed.starts_with("=======") + { + return Err(TsqError::new( + "EVENTS_CORRUPT", + format!( + "Conflict marker detected in {} at line {}; resolve git conflicts before reading events", + path.display(), + line_offset + index + 1 + ), + 2, + )); + } + match serde_json::from_str::(line) { Ok(parsed) => match parse_event_record(&parsed, line_offset + index + 1) { Ok(record) => events.push(record), diff --git a/src/store/git.rs b/src/store/git.rs index 8a20b4b..0ed13bd 100644 --- a/src/store/git.rs +++ b/src/store/git.rs @@ -200,6 +200,127 @@ pub fn push_current_set_upstream( Ok(()) } +/// Outcome of a push attempt that distinguishes recoverable rejections from +/// hard errors. +#[derive(Debug)] +pub enum PushOutcome { + /// Push succeeded. + Ok, + /// Remote rejected the push (non-fast-forward / fetch-first). Recoverable + /// by fetch + merge + retry. Carries the raw stderr for diagnostics. + Rejected(String), +} + +/// Push `branch` to `remote`, setting upstream, and classify the result. +/// +/// A non-fast-forward / fetch-first rejection returns `PushOutcome::Rejected` +/// (recoverable); any other failure is a hard `TsqError`. +pub fn push_branch_with_status( + repo_root: &Path, + remote: &str, + branch: &str, +) -> Result { + validate_branch_name(branch)?; + let output = Command::new("git") + .args(["push", "-u", remote, branch]) + .current_dir(repo_root) + .output() + .map_err(|_| git_not_available())?; + if output.status.success() { + return Ok(PushOutcome::Ok); + } + let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + let lower = stderr.to_lowercase(); + if lower.contains("non-fast-forward") + || lower.contains("fetch first") + || lower.contains("[rejected]") + || lower.contains("[remote rejected]") + { + return Ok(PushOutcome::Rejected(stderr)); + } + Err(git_error("git push failed", stderr)) +} + +/// Result of merging a remote tracking branch into the current worktree branch. +#[derive(Debug)] +pub enum MergeStatus { + /// Merge completed (fast-forward, real merge, or already up to date). + Clean, + /// Merge stopped with unmerged paths; the list of conflicted paths. + Conflict(Vec), +} + +/// Returns true if a merge is in progress (`MERGE_HEAD` exists). +pub fn merge_in_progress(repo_root: &Path) -> Result { + Ok(git_dir(repo_root)?.join("MERGE_HEAD").exists()) +} + +/// Returns the list of unmerged (conflicted) paths in the worktree. +pub fn unmerged_paths(repo_root: &Path) -> Result, TsqError> { + let out = run_git(repo_root, &["diff", "--name-only", "--diff-filter=U"])?; + Ok(out + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + .map(String::from) + .collect()) +} + +/// Returns true if `remote` publishes a branch named `branch`. +pub fn remote_has_branch(repo_root: &Path, remote: &str, branch: &str) -> Result { + validate_branch_name(branch)?; + let refspec = format!("refs/heads/{}", branch); + run_git_status( + repo_root, + &["ls-remote", "--exit-code", "--heads", remote, &refspec], + ) +} + +/// Fetch `branch` from `remote` into its remote-tracking ref. +pub fn fetch_branch(repo_root: &Path, remote: &str, branch: &str) -> Result<(), TsqError> { + validate_branch_name(branch)?; + let remote_ref = format!("+refs/heads/{branch}:refs/remotes/{remote}/{branch}"); + run_git(repo_root, &["fetch", remote, &remote_ref])?; + Ok(()) +} + +/// Merge the `/` tracking ref into the current worktree branch. +/// +/// Uses `--no-edit` so the merge is non-interactive. On conflict, the merge is +/// left in progress (`MERGE_HEAD` set) and the unmerged paths are returned. +pub fn merge_tracking_branch( + repo_root: &Path, + remote: &str, + branch: &str, +) -> Result { + validate_branch_name(branch)?; + let tracking = format!("{remote}/{branch}"); + let output = Command::new("git") + .args(["merge", "--no-edit", &tracking]) + .current_dir(repo_root) + .output() + .map_err(|_| git_not_available())?; + if output.status.success() { + return Ok(MergeStatus::Clean); + } + let unmerged = unmerged_paths(repo_root)?; + if !unmerged.is_empty() { + return Ok(MergeStatus::Conflict(unmerged)); + } + let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + Err(git_error("git merge failed", stderr)) +} + +/// Finalize an in-progress merge whose conflicts have been resolved. +/// +/// Stages any resolved/pending changes and creates the merge commit, preserving +/// the merge message (`--no-edit`). +pub fn finalize_merge(repo_root: &Path) -> Result<(), TsqError> { + run_git(repo_root, &["add", "."])?; + run_git(repo_root, &["commit", "--no-edit"])?; + Ok(()) +} + /// Returns true if the path is inside a git working tree. pub fn is_git_repo(repo_root: &Path) -> bool { run_git_status(repo_root, &["rev-parse", "--is-inside-work-tree"]).unwrap_or(false) @@ -520,17 +641,30 @@ pub fn setup_merge_driver_config(repo_root: &Path) -> Result<(), TsqError> { "Tasque JSONL event merge", ], )?; + let exe = std::env::current_exe().map_err(|error| { + git_error( + "Failed determining current executable for merge driver", + error.to_string(), + ) + })?; + let driver = format!( + "{} merge-driver %O %A %B", + shell_quote(&exe.to_string_lossy()) + ); run_git( repo_root, - &[ - "config", - "merge.tasque-events.driver", - "tsq merge-driver %O %A %B", - ], + &["config", "merge.tasque-events.driver", &driver], )?; Ok(()) } +fn shell_quote(value: &str) -> String { + if value.is_empty() { + return "''".to_string(); + } + format!("'{}'", value.replace('\'', "'\\''")) +} + // --------------------------------------------------------------------------- // Internal helpers // --------------------------------------------------------------------------- @@ -696,7 +830,10 @@ mod tests { let name = run_git(tmp.path(), &["config", "merge.tasque-events.name"]).unwrap(); assert_eq!(name, "Tasque JSONL event merge"); let driver = run_git(tmp.path(), &["config", "merge.tasque-events.driver"]).unwrap(); - assert_eq!(driver, "tsq merge-driver %O %A %B"); + assert!( + driver.ends_with(" merge-driver %O %A %B"), + "unexpected driver command: {driver}" + ); } #[test] diff --git a/src/store/merge_driver.rs b/src/store/merge_driver.rs index 7169395..60c69b1 100644 --- a/src/store/merge_driver.rs +++ b/src/store/merge_driver.rs @@ -3,7 +3,8 @@ use crate::domain::state::create_empty_state; use crate::errors::TsqError; use crate::store::events::read_events_from_path; use crate::types::{EventRecord, MergeDriverOutcome}; -use std::collections::HashMap; +use std::cmp::Reverse; +use std::collections::{BinaryHeap, HashMap, HashSet}; use std::fs; use std::io::Write; use std::path::Path; @@ -30,10 +31,15 @@ fn canonical_json(record: &EventRecord) -> Result { /// /// 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. /// 6. Replay the merged events to validate causal ordering /// 7. Write merged result to `ours` (git merge convention: result goes to %A) pub fn merge_events_files( @@ -45,22 +51,24 @@ pub fn merge_events_files( let ours_result = read_events_from_path(ours)?; let theirs_result = read_events_from_path(theirs)?; - // Map: event_id -> canonical_json - let mut seen: HashMap = HashMap::new(); - let mut merged: Vec<(String, EventRecord)> = Vec::new(); - let mut conflicting_ids: Vec = Vec::new(); + // id -> (record, canonical_json). First occurrence wins for the payload. + let mut id_to_record: HashMap = HashMap::new(); + let mut id_to_json: HashMap = HashMap::new(); + let mut conflicting_ids: HashSet = HashSet::new(); let mut total_input = 0usize; - let all_sources = [ + // Per-source ordered id sequences, used to derive causal edges. + let mut source_orders: Vec> = Vec::new(); + + for events in [ ancestor_result.events, ours_result.events, theirs_result.events, - ]; - - for events in &all_sources { + ] { + let mut order: Vec = Vec::new(); for record in events { total_input += 1; - let id = match event_id(record) { + let id = match event_id(&record) { Some(id) => id.to_string(), None => { return Err(TsqError::new( @@ -70,40 +78,101 @@ pub fn merge_events_files( )); } }; - - let json = canonical_json(record)?; - - match seen.get(&id) { + let json = canonical_json(&record)?; + match id_to_json.get(&id) { Some(existing_json) => { - if *existing_json != json && !conflicting_ids.contains(&id) { - conflicting_ids.push(id.clone()); + if *existing_json != json { + conflicting_ids.insert(id.clone()); } - // Same ID + same payload = duplicate, skip + // Same ID + same payload = duplicate, nothing to store. } None => { - seen.insert(id.clone(), json); - merged.push((id, record.clone())); + id_to_record.insert(id.clone(), record); + id_to_json.insert(id.clone(), json); } } + order.push(id); } + source_orders.push(order); } - conflicting_ids.sort(); + let unique_count = id_to_record.len(); + let duplicates_removed = total_input.saturating_sub(unique_count); if !conflicting_ids.is_empty() { + let mut ids: Vec = conflicting_ids.into_iter().collect(); + ids.sort(); return Ok(MergeDriverOutcome { - total_events: seen.len(), - duplicates_removed: total_input.saturating_sub(seen.len()), + total_events: unique_count, + duplicates_removed, conflict: true, - conflicting_ids, + conflicting_ids: ids, }); } - let duplicates_removed = total_input.saturating_sub(merged.len()); - let total_events = merged.len(); + // Derive causal edges from each source's order. Dedupe edges across sources + // so the incoming-edge count stays accurate. + let mut edges: HashSet<(String, String)> = HashSet::new(); + for order in &source_orders { + for window in order.windows(2) { + if window[0] != window[1] { + edges.insert((window[0].clone(), window[1].clone())); + } + } + } + + let mut incoming: HashMap = HashMap::new(); + let mut outgoing: HashMap> = HashMap::new(); + for id in id_to_record.keys() { + incoming.entry(id.clone()).or_insert(0); + outgoing.entry(id.clone()).or_default(); + } + for (from, to) in &edges { + outgoing.entry(from.clone()).or_default().push(to.clone()); + *incoming.entry(to.clone()).or_default() += 1; + } + + // Kahn's algorithm with a min-heap keyed by event ID. Picking the smallest + // available ID at each step yields a stable, direction-independent order. + let mut heap: BinaryHeap> = incoming + .iter() + .filter(|(_, count)| **count == 0) + .map(|(id, _)| Reverse(id.clone())) + .collect(); + let mut sorted_ids: Vec = Vec::with_capacity(unique_count); + while let Some(Reverse(id)) = heap.pop() { + sorted_ids.push(id.clone()); + if let Some(nexts) = outgoing.get(&id) { + for next in nexts { + let count = incoming.entry(next.clone()).or_insert(0); + *count -= 1; + if *count == 0 { + heap.push(Reverse(next.clone())); + } + } + } + } + + // Cycle fallback: if constraints formed a cycle (same events recorded in + // conflicting orders across sources), append the remaining IDs in sorted + // order to keep the output deterministic rather than failing silently. + if sorted_ids.len() < unique_count { + let emitted: HashSet = sorted_ids.iter().cloned().collect(); + let mut remaining: Vec = id_to_record + .keys() + .filter(|id| !emitted.contains(*id)) + .cloned() + .collect(); + remaining.sort(); + sorted_ids.extend(remaining); + } - let replay_events: Vec = merged.iter().map(|(_, record)| record.clone()).collect(); - apply_events(&create_empty_state(), &replay_events).map_err(|e| { + let merged: Vec = sorted_ids + .iter() + .map(|id| id_to_record.get(id).expect("id present in map").clone()) + .collect(); + + apply_events(&create_empty_state(), &merged).map_err(|e| { TsqError::new( "MERGE_REPLAY_FAILED", format!("Merged event stream failed replay validation: {}", e), @@ -111,7 +180,7 @@ pub fn merge_events_files( ) })?; - // Write merged result to ours path (git expects result at %A) + let total_events = merged.len(); write_events_to_path(ours, &merged)?; Ok(MergeDriverOutcome { @@ -123,7 +192,7 @@ pub fn merge_events_files( } /// Write merged events to a file as JSONL. -fn write_events_to_path(path: &Path, events: &[(String, EventRecord)]) -> Result<(), TsqError> { +fn write_events_to_path(path: &Path, events: &[EventRecord]) -> Result<(), TsqError> { let mut file = fs::File::create(path).map_err(|e| { TsqError::new( "MERGE_WRITE_FAILED", @@ -132,7 +201,7 @@ fn write_events_to_path(path: &Path, events: &[(String, EventRecord)]) -> Result ) })?; - for (_, record) in events { + for record in events { let line = serde_json::to_string(record).map_err(|e| { TsqError::new( "MERGE_SERIALIZE_FAILED", diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 8086062..bc8250d 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1,5 +1,7 @@ #![allow(dead_code)] +use once_cell::sync::Lazy; +use regex::Regex; use serde_json::Value; use std::path::{Path, PathBuf}; use std::process::Command; @@ -7,6 +9,16 @@ use std::sync::OnceLock; use tasque::types::SCHEMA_VERSION; use tempfile::{Builder, TempDir}; +static RANDOM_CANONICAL_ID: Lazy = Lazy::new(|| { + Regex::new(r"^tsq-[0123456789abcdefghjkmnpqrstvwxyz]{8}$").expect("random canonical id regex") +}); + +/// Matches the flat random canonical id shape (`tsq-<8 crockford base32 chars>`) +/// used for new root and child task ids. +pub fn is_random_canonical_id(id: &str) -> bool { + RANDOM_CANONICAL_ID.is_match(id) +} + #[derive(Debug)] pub struct CliOutput { pub code: i32, diff --git a/tests/create_children_batch.rs b/tests/create_children_batch.rs index 3f9c2f9..ec07676 100644 --- a/tests/create_children_batch.rs +++ b/tests/create_children_batch.rs @@ -29,7 +29,16 @@ fn create_supports_multiple_children_for_single_parent() { .filter_map(|task| task.get("id").and_then(Value::as_str)) .map(ToString::to_string) .collect(); - assert_eq!(ids, vec![format!("{}.1", parent), format!("{}.2", parent)]); + assert_eq!(ids.len(), 2, "expected two distinct child ids"); + assert_ne!(ids[0], ids[1], "child ids must be distinct"); + assert!( + ids.iter().all(|id| common::is_random_canonical_id(id)), + "child ids must be flat random canonical: {ids:?}" + ); + assert!( + ids.iter().all(|id| !id.starts_with(&format!("{parent}."))), + "child ids must not use parent.N shape: {ids:?}" + ); let parent_ids: Vec = tasks .iter() @@ -129,11 +138,24 @@ fn create_ensure_is_idempotent_for_parent_children_batch() { let second = run_json(repo.path(), cmd); assert_eq!(second.cli.code, 0); - let expected_ids = vec![format!("{}.1", parent), format!("{}.2", parent)]; let first_ids = task_ids(&first); let second_ids = task_ids(&second); - assert_eq!(first_ids, expected_ids); - assert_eq!(second_ids, expected_ids); + assert_eq!(first_ids.len(), 2); + assert_eq!(first_ids, second_ids, "ensure must reuse same ids"); + assert_ne!(first_ids[0], first_ids[1]); + assert!( + first_ids + .iter() + .all(|id| common::is_random_canonical_id(id)), + "child ids must be flat random canonical: {first_ids:?}" + ); + + // Children must point back at the parent regardless of id shape. + let first_parent_ids: Vec<&str> = data_tasks(&first) + .iter() + .filter_map(|task| task["parent_id"].as_str()) + .collect(); + assert_eq!(first_parent_ids, vec![parent.as_str(), parent.as_str()]); let listed = run_json(repo.path(), ["find", "open"]); assert_eq!(listed.cli.code, 0); diff --git a/tests/event_read_validation.rs b/tests/event_read_validation.rs index ba04148..aa74930 100644 --- a/tests/event_read_validation.rs +++ b/tests/event_read_validation.rs @@ -33,6 +33,34 @@ fn write_event(payload: serde_json::Value) -> (TempDir, std::path::PathBuf) { (dir, path) } +#[test] +fn read_events_rejects_conflict_marker_as_corrupt() { + let dir = TempDir::new().expect("tempdir"); + let path = dir.path().join("events.jsonl"); + let first = task_created_event("tsq-root0001", "first"); + let first_line = serde_json::to_string(&first).expect("serialize first"); + fs::write( + &path, + format!( + "{first_line}\n<<<<<<< HEAD\n{{}}\n||||||| base\n{{}}\n=======\n{{}}\n>>>>>>> branch\n" + ), + ) + .expect("write conflict markers"); + + let err = match read_events_from_path(&path) { + Ok(_) => panic!("conflict markers must not be ingested as valid JSON"), + Err(error) => error, + }; + + assert_eq!(err.code, "EVENTS_CORRUPT"); + assert_eq!(err.exit_code, 2); + assert!( + err.message.contains("Conflict marker"), + "error must name the conflict marker: {}", + err.message + ); +} + #[test] fn read_events_rejects_invalid_status_set_payload_as_corrupt() { let (_dir, path) = write_event(json!({"status": "done"})); diff --git a/tests/merge_driver.rs b/tests/merge_driver.rs index a346351..9c5d729 100644 --- a/tests/merge_driver.rs +++ b/tests/merge_driver.rs @@ -195,6 +195,156 @@ fn test_merge_driver_preserves_causal_source_order_when_ids_sort_backwards() { ); } +#[test] +fn test_merge_driver_commutative_independent_creates() { + // Both sides add independent root task creates. A<-B and B<-A must produce + // byte-identical merged output. + let repo_left = make_repo(); + let repo_right = make_repo(); + + let ours_events = vec![ + make_event("01AAA", "ours-task"), + make_event("01AAB", "ours-task-2"), + ]; + let theirs_events = vec![ + make_event("01BBB", "theirs-task"), + make_event("01BBC", "theirs-task-2"), + ]; + + let run_left = run_merge_driver(repo_left.path(), &[], &ours_events, &theirs_events); + let run_right = run_merge_driver(repo_right.path(), &[], &theirs_events, &ours_events); + + assert_eq!( + run_left.result.code, 0, + "stderr: {}", + run_left.result.stderr + ); + assert_eq!( + run_right.result.code, 0, + "stderr: {}", + run_right.result.stderr + ); + + let left_out = fs::read_to_string(&run_left.ours_path).unwrap(); + let right_out = fs::read_to_string(&run_right.ours_path).unwrap(); + assert_eq!( + left_out, right_out, + "merged output must be byte-identical regardless of merge direction" + ); + assert_eq!( + merged_ids(&run_left.ours_path), + vec!["01AAA", "01AAB", "01BBB", "01BBC"] + ); +} + +#[test] +fn test_merge_driver_event_id_fallback_dedup() { + // Legacy records carrying only `event_id` (no `id`) still dedup correctly + // against a peer carrying `id` with the same value. + let repo = make_repo(); + let mut payload = Map::new(); + payload.insert("title".to_string(), Value::String("shared".to_string())); + let legacy_only_event_id = EventRecord { + id: None, + event_id: Some("01SHARED".to_string()), + ts: "2026-01-01T00:00:00Z".to_string(), + actor: "test".to_string(), + event_type: EventType::TaskCreated, + task_id: "tsq-01SHARED".to_string(), + payload: payload.clone(), + }; + let canonical = EventRecord { + id: Some("01SHARED".to_string()), + event_id: Some("01SHARED".to_string()), + ..legacy_only_event_id.clone() + }; + + let run = run_merge_driver(repo.path(), &[], &[legacy_only_event_id], &[canonical]); + + assert_eq!(run.result.code, 0, "stderr: {}", run.result.stderr); + assert_eq!(read_jsonl(&run.ours_path).len(), 1); + assert!(run.result.stderr.contains("duplicates removed")); +} + +#[test] +fn test_merge_driver_duplicate_task_id_yields_replay_failure() { + // Two sides independently create events with distinct event ids but the + // SAME task_id. The merged stream is causally invalid (TASK_EXISTS), so the + // merge driver must surface MERGE_REPLAY_FAILED instead of writing a bad file. + let repo = make_repo(); + let ours_event = make_event("01OURS", "ours-title"); + let mut theirs_event = make_event("01THRS", "theirs-title"); + // Collision: same task_id as ours, different event id. + theirs_event.task_id = ours_event.task_id.clone(); + + let run = run_merge_driver(repo.path(), &[], &[ours_event], &[theirs_event]); + + assert_eq!(run.result.code, 2, "stderr: {}", run.result.stderr); + assert!( + run.result.stderr.contains("MERGE_REPLAY_FAILED"), + "stderr: {}", + run.result.stderr + ); + // Replay failure must not have overwritten the working file with the bad + // merged stream; it should still hold only the original ours input. + assert_eq!(read_jsonl(&run.ours_path).len(), 1); + assert_eq!(merged_ids(&run.ours_path), vec!["01OURS"]); +} + +#[test] +fn test_merge_driver_cycle_fallback_is_deterministic() { + // If two sources contain the same independent events in opposite order, + // source-order constraints form a cycle. The fallback must stay stable and + // direction-independent instead of producing merge-order drift. + let repo_left = make_repo(); + let repo_right = make_repo(); + let first = make_event("01AAA", "first"); + let second = make_event("01BBB", "second"); + + let ours = vec![second.clone(), first.clone()]; + let theirs = vec![first, second]; + + let run_left = run_merge_driver(repo_left.path(), &[], &ours, &theirs); + let run_right = run_merge_driver(repo_right.path(), &[], &theirs, &ours); + + assert_eq!( + run_left.result.code, 0, + "stderr: {}", + run_left.result.stderr + ); + assert_eq!( + run_right.result.code, 0, + "stderr: {}", + run_right.result.stderr + ); + assert_eq!( + fs::read_to_string(&run_left.ours_path).unwrap(), + fs::read_to_string(&run_right.ours_path).unwrap(), + "cycle fallback must be byte-identical regardless of merge direction" + ); + assert_eq!(merged_ids(&run_left.ours_path), vec!["01AAA", "01BBB"]); +} + +#[test] +fn test_merge_driver_both_sides_add_identical_dedup() { + // Both sides add the SAME new event (same id, same payload). It must appear + // exactly once in the merged output. + let repo = make_repo(); + let base_events = vec![make_event("01BASE", "base")]; + let shared_new = make_event("01NEW", "new-from-both"); + let mut ours_events = base_events.clone(); + ours_events.push(shared_new.clone()); + let mut theirs_events = base_events.clone(); + theirs_events.push(shared_new); + + let run = run_merge_driver(repo.path(), &base_events, &ours_events, &theirs_events); + + assert_eq!(run.result.code, 0, "stderr: {}", run.result.stderr); + assert_eq!(read_jsonl(&run.ours_path).len(), 2); + assert_eq!(merged_ids(&run.ours_path), vec!["01BASE", "01NEW"]); + assert!(run.result.stderr.contains("duplicates removed")); +} + #[test] fn test_merge_driver_mixed_event_types() { let repo = make_repo(); diff --git a/tests/readable_identity.rs b/tests/readable_identity.rs index 1a61ac5..5f801df 100644 --- a/tests/readable_identity.rs +++ b/tests/readable_identity.rs @@ -1,10 +1,20 @@ mod common; use common::{create_task, init_repo, run_cli, run_json}; +use once_cell::sync::Lazy; +use regex::Regex; use serde_json::Value; use std::fs; use tasque::domain::similarity::DEFAULT_SIMILARITY_MIN_SCORE; +static RANDOM_ROOT_ID: Lazy = Lazy::new(|| { + Regex::new(r"^tsq-[0123456789abcdefghjkmnpqrstvwxyz]{8}$").expect("random root id regex") +}); + +fn is_random_canonical(id: &str) -> bool { + RANDOM_ROOT_ID.is_match(id) +} + #[test] fn create_projects_stable_alias_from_title() { let repo = common::make_repo(); @@ -58,19 +68,28 @@ fn title_edit_does_not_recompute_alias() { } #[test] -fn new_root_ids_are_sequential() { +fn new_root_ids_are_random_canonical() { let repo = common::make_repo(); init_repo(repo.path()); let first = create_task(repo.path(), "First sequential task"); let second = create_task(repo.path(), "Second sequential task"); - assert_eq!(first, "tsq-1"); - assert_eq!(second, "tsq-2"); + assert!( + is_random_canonical(&first), + "first id {first} not random canonical" + ); + assert!( + is_random_canonical(&second), + "second id {second} not random canonical" + ); + assert_ne!(first, second, "ids must be distinct"); + assert_ne!(first, "tsq-1"); + assert_ne!(second, "tsq-2"); } #[test] -fn child_ids_keep_parent_suffix_shape() { +fn child_ids_are_flat_random_with_parent_set() { let repo = common::make_repo(); init_repo(repo.path()); @@ -78,9 +97,21 @@ fn child_ids_keep_parent_suffix_shape() { let child = run_json(repo.path(), ["create", "--parent", &parent, "Child task"]); assert_eq!(child.cli.code, 0); + let child_id = child.envelope["data"]["task"]["id"] + .as_str() + .expect("child id"); + assert!( + is_random_canonical(child_id), + "child id {child_id} not random canonical" + ); + assert_ne!(child_id, parent, "child id must not equal parent id"); + assert!( + !child_id.starts_with(&format!("{parent}.")), + "child id must not use parent.N shape" + ); assert_eq!( - child.envelope["data"]["task"]["id"].as_str(), - Some("tsq-1.1") + child.envelope["data"]["task"]["parent_id"].as_str(), + Some(parent.as_str()) ); } @@ -102,14 +133,18 @@ fn explicit_sequential_id_is_allowed() { } #[test] -fn numeric_legacy_root_id_does_not_advance_sequential_allocation() { +fn explicit_legacy_random_id_does_not_affect_allocation() { let repo = common::make_repo(); init_repo(repo.path()); common::create_task_with_args(repo.path(), "Legacy numeric ID", &["--id", "tsq-00000042"]); let next = create_task(repo.path(), "First sequential after legacy"); - assert_eq!(next, "tsq-1"); + assert!( + is_random_canonical(&next), + "next id {next} not random canonical" + ); + assert_ne!(next, "tsq-00000042"); } #[test] @@ -144,6 +179,53 @@ fn commands_accept_alias_case_insensitively() { ); } +#[test] +fn duplicate_exact_alias_is_ambiguous() { + use std::collections::HashMap; + use tasque::domain::resolve::resolve_task_id; + use tasque::types::{State, Task}; + + // Two tasks with colliding exact aliases (case-insensitive). The CLI path + // normally dedupes aliases with a numeric suffix, so construct the state + // directly to exercise resolver behavior on a real collision. + let mk_task = |id: &str, alias: &str| -> Task { + serde_json::from_value(serde_json::json!({ + "id": id, + "alias": alias, + "title": alias, + "kind": "task", + "status": "open", + "priority": 3, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + })) + .expect("task from json") + }; + let mut tasks: HashMap = HashMap::new(); + tasks.insert( + "tsq-aaaaaaaa".to_string(), + mk_task("tsq-aaaaaaaa", "improve-search-warnings"), + ); + tasks.insert( + "tsq-bbbbbbbb".to_string(), + mk_task("tsq-bbbbbbbb", "IMPROVE-SEARCH-WARNINGS"), + ); + let state: State = serde_json::from_value(serde_json::json!({ + "tasks": {}, + "deps": {}, + "links": {}, + "child_counters": {}, + "created_order": [], + "applied_events": 0 + })) + .expect("base state"); + let state = State { tasks, ..state }; + + let result = resolve_task_id(&state, "improve-search-warnings", false); + let err = result.expect_err("expected ambiguous error"); + assert_eq!(err.code, "TASK_ID_AMBIGUOUS"); +} + #[test] fn ambiguous_alias_prefix_returns_candidates() { let repo = common::make_repo(); @@ -194,21 +276,26 @@ fn id_prefix_ambiguity_returns_id_and_alias_candidates() { } #[test] -fn sequential_allocation_after_u64_max_returns_error() { +fn allocation_succeeds_even_with_u64_max_sequential_present() { let repo = common::make_repo(); init_repo(repo.path()); - // Plant a task at u64::MAX so next allocation would overflow. + // Plant a task at u64::MAX. Random allocation must not depend on the + // sequential counter and must not overflow. let max_id = "tsq-18446744073709551615"; let setup = run_json(repo.path(), ["create", "Max ID task", "--id", max_id]); assert_eq!(setup.cli.code, 0); - let result = run_json(repo.path(), ["create", "Should overflow"]); - assert_eq!(result.cli.code, 2); - assert_eq!( - result.envelope["error"]["code"].as_str(), - Some("ID_OVERFLOW") + let result = run_json(repo.path(), ["create", "Random after max"]); + assert_eq!(result.cli.code, 0); + let next = result.envelope["data"]["task"]["id"] + .as_str() + .expect("next id"); + assert!( + is_random_canonical(next), + "next id {next} not random canonical" ); + assert_ne!(next, max_id); } #[test] diff --git a/tests/sync_branch.rs b/tests/sync_branch.rs index 1f28983..d211338 100644 --- a/tests/sync_branch.rs +++ b/tests/sync_branch.rs @@ -3,6 +3,8 @@ mod common; use common::{git, git_output, init_git_repo_with_identity, make_repo, run_cli}; use serde_json::Value; use std::fs; +#[cfg(unix)] +use std::os::unix::fs::PermissionsExt; fn git_out(repo: &std::path::Path, args: &[&str]) -> String { let output = git_output(repo, args); @@ -72,7 +74,7 @@ fn sync_branch_requires_git_repo() { assert_eq!(code, Some("GIT_NOT_AVAILABLE")); } -const EXPECTED_MERGE_DRIVER: &str = "tsq merge-driver %O %A %B"; +const EXPECTED_MERGE_DRIVER_SUFFIX: &str = " merge-driver %O %A %B"; /// Fresh clones only receive the sync branch and its committed /// `.gitattributes` (`merge=tasque-events`) via git; the merge driver @@ -126,9 +128,9 @@ fn clone_materializes_worktree_and_configures_merge_driver() { ); let driver = git_out(&clone, &["config", "--get", "merge.tasque-events.driver"]); - assert_eq!( - driver, EXPECTED_MERGE_DRIVER, - "expected merge driver to be configured after worktree materializes" + assert!( + driver.ends_with(EXPECTED_MERGE_DRIVER_SUFFIX), + "expected merge driver to be configured after worktree materializes, got: {driver}" ); // `tsq sync` heals a clone whose worktree was materialized on a @@ -145,9 +147,9 @@ fn clone_materializes_worktree_and_configures_merge_driver() { assert_eq!(sync_result.code, 0, "stderr: {}", sync_result.stderr); let restored = git_out(&clone, &["config", "--get", "merge.tasque-events.driver"]); - assert_eq!( - restored, EXPECTED_MERGE_DRIVER, - "expected `tsq sync` to re-ensure the merge driver config" + assert!( + restored.ends_with(EXPECTED_MERGE_DRIVER_SUFFIX), + "expected `tsq sync` to re-ensure the merge driver config, got: {restored}" ); } @@ -254,3 +256,307 @@ fn explicit_migrate_fails_on_unreachable_origin_after_local_migration() { "expected events present in sync worktree" ); } + +/// Read the sync-worktree `events.jsonl` for a clone/repo whose sync branch +/// lives at `/.git/tsq-sync`. +fn read_sync_worktree_events(root: &std::path::Path) -> String { + fs::read_to_string( + root.join(".git") + .join("tsq-sync") + .join(".tasque") + .join("events.jsonl"), + ) + .unwrap_or_default() +} + +/// Seed a `source` repo (main + tsq-sync) and push both branches to a fresh +/// bare `origin`. Returns `(source_path, origin_path)`. +fn seed_source_with_origin( + base: &std::path::Path, + title: &str, +) -> (std::path::PathBuf, std::path::PathBuf) { + let source = base.join("source"); + fs::create_dir(&source).expect("source dir"); + init_git_repo_with_identity(&source, Some("main")); + + let init = run_cli(&source, ["init"]); + assert_eq!(init.code, 0, "stderr: {}", init.stderr); + let create = run_cli(&source, ["create", title, "--force"]); + assert_eq!(create.code, 0, "stderr: {}", create.stderr); + + git(&source, &["add", ".tasque/config.json", ".gitattributes"]); + git(&source, &["commit", "-m", "seed main config"]); + + let remote = create_bare_origin(base); + let remote_arg = remote.to_string_lossy().to_string(); + git(&source, &["remote", "add", "origin", remote_arg.as_str()]); + git(&source, &["push", "origin", "HEAD:main"]); + git(&source, &["push", "origin", "tsq-sync"]); + (source, remote) +} + +/// Clone `remote` into `/` and configure a test git identity. +fn clone_from_origin( + base: &std::path::Path, + remote: &std::path::Path, + name: &str, +) -> std::path::PathBuf { + let clone = base.join(name); + let clone_arg = clone.to_string_lossy().to_string(); + let remote_arg = remote.to_string_lossy().to_string(); + git(base, &["clone", remote_arg.as_str(), clone_arg.as_str()]); + git(&clone, &["config", "user.name", "rust-test"]); + git(&clone, &["config", "user.email", "rust-test@example.com"]); + clone +} + +/// Two clones create distinct tasks and sync. The clone that pushes second must +/// fetch + merge the first clone's events before pushing (local-first two-way +/// sync), and a subsequent sync on the first clone must converge to both events. +/// Explicit ids avoid the sequential-id collision that would otherwise force a +/// merge conflict between independently created tasks. +#[test] +fn two_clone_sync_converges_after_non_fast_forward() { + let repo = make_repo(); + let base = repo.path(); + let (_source, remote) = seed_source_with_origin(base, "Seed task"); + + let clone_a = clone_from_origin(base, &remote, "cloneA"); + let clone_b = clone_from_origin(base, &remote, "cloneB"); + + let ca = run_cli( + &clone_a, + ["create", "Task A", "--id", "tsq-aaaa1111", "--force"], + ); + assert_eq!(ca.code, 0, "stderr: {}", ca.stderr); + let sa = run_cli(&clone_a, ["sync"]); + assert_eq!(sa.code, 0, "clone A first sync\nstderr: {}", sa.stderr); + + let cb = run_cli( + &clone_b, + ["create", "Task B", "--id", "tsq-bbbb2222", "--force"], + ); + assert_eq!(cb.code, 0, "stderr: {}", cb.stderr); + // Clone B is behind origin (A pushed first): sync must fetch + merge + push. + let sb = run_cli(&clone_b, ["sync"]); + assert_eq!( + sb.code, 0, + "clone B sync should fetch+merge+push after non-ff\nstdout:\n{}\nstderr:\n{}", + sb.stdout, sb.stderr + ); + + // Clone A pulls in clone B's task on its next sync. + let sa2 = run_cli(&clone_a, ["sync"]); + assert_eq!(sa2.code, 0, "clone A second sync\nstderr: {}", sa2.stderr); + + let events_a = read_sync_worktree_events(&clone_a); + let events_b = read_sync_worktree_events(&clone_b); + for id in ["tsq-aaaa1111", "tsq-bbbb2222"] { + assert!(events_a.contains(id), "clone A missing {id}:\n{events_a}"); + assert!(events_b.contains(id), "clone B missing {id}:\n{events_b}"); + } +} + +/// A third clone can advance the remote between our fetch/merge and push. Sync +/// must classify that non-fast-forward rejection as recoverable, then fetch, +/// merge, and push again. +#[cfg(unix)] +#[test] +fn sync_retries_when_remote_advances_during_push() { + let repo = make_repo(); + let base = repo.path(); + let (_source, remote) = seed_source_with_origin(base, "Seed task"); + + let target = clone_from_origin(base, &remote, "target"); + let advancer = clone_from_origin(base, &remote, "advancer"); + + let target_create = run_cli( + &target, + ["create", "Target task", "--id", "tsq-cccc1111", "--force"], + ); + assert_eq!(target_create.code, 0, "stderr: {}", target_create.stderr); + let advancer_create = run_cli( + &advancer, + ["create", "Advancer task", "--id", "tsq-cccc2222", "--force"], + ); + assert_eq!( + advancer_create.code, 0, + "stderr: {}", + advancer_create.stderr + ); + + let flag = base.join("pre-push-fired"); + let hook = target.join(".git").join("hooks").join("pre-push"); + let script = format!( + "#!/bin/sh\nif [ ! -f '{flag}' ]; then\n touch '{flag}'\n unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE\n git -C '{advancer_wt}' push origin tsq-sync >/dev/null 2>&1 || exit 1\nfi\nexit 0\n", + flag = flag.display(), + advancer_wt = advancer.join(".git").join("tsq-sync").display() + ); + fs::write(&hook, script).expect("write pre-push hook"); + let mut permissions = fs::metadata(&hook).expect("hook metadata").permissions(); + permissions.set_mode(0o755); + fs::set_permissions(&hook, permissions).expect("chmod pre-push hook"); + + let sync = run_cli(&target, ["sync"]); + assert_eq!( + sync.code, 0, + "sync should retry after pre-push remote advance\nstdout:\n{}\nstderr:\n{}", + sync.stdout, sync.stderr + ); + assert!(flag.exists(), "pre-push hook should have advanced remote"); + + let target_events = read_sync_worktree_events(&target); + for id in ["tsq-cccc1111", "tsq-cccc2222"] { + assert!( + target_events.contains(id), + "target should contain retried merge id {id}:\n{target_events}" + ); + } +} + +/// With an `origin` remote that has no sync branch yet, `tsq sync` publishes the +/// local sync branch with `push -u` (no upstream configured beforehand). +#[test] +fn sync_publishes_sync_branch_when_origin_has_no_upstream() { + let repo = make_repo(); + let base = repo.path(); + let source = base.join("repo"); + fs::create_dir(&source).expect("repo dir"); + init_git_repo_with_identity(&source, Some("main")); + + let init = run_cli(&source, ["init"]); + assert_eq!(init.code, 0, "stderr: {}", init.stderr); + let create = run_cli(&source, ["create", "Publish task", "--force"]); + assert_eq!(create.code, 0, "stderr: {}", create.stderr); + git(&source, &["add", ".tasque/config.json", ".gitattributes"]); + git(&source, &["commit", "-m", "seed main config"]); + + let remote = create_bare_origin(base); + let remote_arg = remote.to_string_lossy().to_string(); + git(&source, &["remote", "add", "origin", remote_arg.as_str()]); + // Only main is published; the sync branch does not yet exist on origin. + git(&source, &["push", "origin", "HEAD:main"]); + + let pre = git_output(&source, &["ls-remote", "--heads", "origin", "tsq-sync"]); + assert!( + String::from_utf8_lossy(&pre.stdout).trim().is_empty(), + "expected no remote sync branch before sync" + ); + + let sync = run_cli(&source, ["sync", "--json"]); + assert_eq!(sync.code, 0, "stderr: {}", sync.stderr); + let envelope: Value = serde_json::from_str(sync.stdout.trim()).expect("json envelope"); + let data = envelope.get("data").expect("data"); + assert_eq!(data.get("pushed").and_then(Value::as_bool), Some(true)); + assert_eq!( + data.get("has_upstream").and_then(Value::as_bool), + Some(true) + ); + + let post = git_out(&source, &["ls-remote", "--heads", "origin", "tsq-sync"]); + assert!( + post.contains("refs/heads/tsq-sync"), + "expected sync branch published to origin:\n{post}" + ); +} + +/// Two clones create the SAME task id with divergent payloads. The second sync +/// hits the tasque-events merge driver conflict, leaves the merge in progress, +/// and returns a structured `SYNC_MERGE_CONFLICT` error carrying the conflicted +/// paths and worktree path. Rerunning while conflicts remain re-emits the same +/// error; resolving + rerunning finalizes the merge and pushes. +#[test] +fn sync_conflict_returns_structured_error_and_resumes() { + let repo = make_repo(); + let base = repo.path(); + let (_source, remote) = seed_source_with_origin(base, "Seed task"); + + let clone_a = clone_from_origin(base, &remote, "cloneA"); + let clone_b = clone_from_origin(base, &remote, "cloneB"); + + let ca = run_cli( + &clone_a, + ["create", "Title A", "--id", "tsq-dddd0000", "--force"], + ); + assert_eq!(ca.code, 0, "stderr: {}", ca.stderr); + let sa = run_cli(&clone_a, ["sync"]); + assert_eq!(sa.code, 0, "clone A sync\nstderr: {}", sa.stderr); + + let cb = run_cli( + &clone_b, + ["create", "Title B", "--id", "tsq-dddd0000", "--force"], + ); + assert_eq!(cb.code, 0, "stderr: {}", cb.stderr); + + // Clone B fetches A's divergent event for the same id -> merge conflict. + let sb = run_cli(&clone_b, ["sync", "--json"]); + assert_eq!( + sb.code, 1, + "expected conflict exit code 1\nstdout:\n{}\nstderr:\n{}", + sb.stdout, sb.stderr + ); + let envelope: Value = serde_json::from_str(sb.stdout.trim()).expect("json envelope"); + let error = envelope.get("error").expect("error object"); + assert_eq!( + error.get("code").and_then(Value::as_str), + Some("SYNC_MERGE_CONFLICT") + ); + let details = error.get("details").expect("conflict details"); + let paths = details + .get("conflicted_paths") + .and_then(Value::as_array) + .expect("conflicted_paths array"); + assert!( + paths + .iter() + .filter_map(Value::as_str) + .any(|p| p.contains("events.jsonl")), + "expected events.jsonl among conflicted paths: {paths:?}" + ); + assert!( + details + .get("worktree_path") + .and_then(Value::as_str) + .is_some(), + "expected worktree_path in conflict details" + ); + + let worktree = clone_b.join(".git").join("tsq-sync"); + assert!( + worktree.join(".git").exists() && worktree.join(".tasque").exists(), + "expected merge left in worktree" + ); + + // Rerun while conflicts remain -> same structured conflict error. + let sb_again = run_cli(&clone_b, ["sync", "--json"]); + assert_eq!( + sb_again.code, 1, + "expected repeated conflict\nstderr: {}", + sb_again.stderr + ); + let envelope_again: Value = + serde_json::from_str(sb_again.stdout.trim()).expect("json envelope"); + assert_eq!( + envelope_again + .get("error") + .and_then(|e| e.get("code")) + .and_then(Value::as_str), + Some("SYNC_MERGE_CONFLICT") + ); + + // Resolve by keeping clone B's version, then rerun: merge finalizes + pushes. + git(&worktree, &["checkout", "--ours", ".tasque/events.jsonl"]); + git(&worktree, &["add", ".tasque/events.jsonl"]); + let sb_resolved = run_cli(&clone_b, ["sync"]); + assert_eq!( + sb_resolved.code, 0, + "expected resolved sync to succeed\nstdout:\n{}\nstderr:\n{}", + sb_resolved.stdout, sb_resolved.stderr + ); + // Merge is no longer in progress once finalized. + let merge_head = git_output(&worktree, &["rev-parse", "-q", "--verify", "MERGE_HEAD"]); + assert!( + !merge_head.status.success(), + "expected MERGE_HEAD cleared after resolved sync" + ); +} diff --git a/tests/verb_first_task_commands.rs b/tests/verb_first_task_commands.rs index 9547de1..945679f 100644 --- a/tests/verb_first_task_commands.rs +++ b/tests/verb_first_task_commands.rs @@ -32,7 +32,7 @@ fn create_from_file_accepts_markdown_bullets() { } #[test] -fn create_from_file_allocates_root_ids_sequentially_after_high_existing_id() { +fn create_from_file_allocates_random_ids_unrelated_to_high_existing_id() { let repo = common::make_repo(); init_repo(repo.path()); create_task_with_args(repo.path(), "Existing high root", &["--id", "tsq-42"]); @@ -43,12 +43,23 @@ fn create_from_file_allocates_root_ids_sequentially_after_high_existing_id() { assert_eq!(result.cli.code, 0); let tasks = result.envelope["data"]["tasks"].as_array().expect("tasks"); - assert_eq!(tasks[0]["id"].as_str(), Some("tsq-43")); - assert_eq!(tasks[1]["id"].as_str(), Some("tsq-44")); + let first = tasks[0]["id"].as_str().expect("first id"); + let second = tasks[1]["id"].as_str().expect("second id"); + assert!( + common::is_random_canonical_id(first), + "first id {first} not random canonical" + ); + assert!( + common::is_random_canonical_id(second), + "second id {second} not random canonical" + ); + assert_ne!(first, second); + assert_ne!(first, "tsq-43"); + assert_ne!(second, "tsq-44"); } #[test] -fn create_from_file_can_allocate_last_u64_root_id() { +fn create_from_file_succeeds_alongside_last_u64_root_id() { let repo = common::make_repo(); init_repo(repo.path()); create_task_with_args( @@ -62,10 +73,14 @@ fn create_from_file_can_allocate_last_u64_root_id() { let result = run_json(repo.path(), ["create", "--from-file", "tasks.md"]); assert_eq!(result.cli.code, 0); - assert_eq!( - result.envelope["data"]["task"]["id"].as_str(), - Some("tsq-18446744073709551615") + let id = result.envelope["data"]["task"]["id"] + .as_str() + .expect("task id"); + assert!( + common::is_random_canonical_id(id), + "new id {id} not random canonical" ); + assert_ne!(id, "tsq-18446744073709551615"); } #[test] From 834d04ab77a895b6a1eb73cc8258dedc5108d3f7 Mon Sep 17 00:00:00 2001 From: Aditya Sharma Date: Wed, 8 Jul 2026 08:50:52 -0700 Subject: [PATCH 2/4] Remove sequential child ID counters and simplify task ID generation --- src/app/service_create_update.rs | 26 ++------- src/app/service_utils.rs | 5 -- src/app/sync.rs | 2 +- src/domain/ids.rs | 50 +++++++---------- src/domain/projector_helpers.rs | 19 ------- src/domain/projector_tasks.rs | 10 +--- src/domain/state.rs | 1 - src/store/git.rs | 33 ++++------- src/types.rs | 1 - tests/merge_driver.rs | 7 ++- tests/readable_identity.rs | 21 ++----- tests/sync_branch.rs | 92 +++++++++++++++++++++++++++++-- tests/verb_first_task_commands.rs | 25 --------- 13 files changed, 136 insertions(+), 156 deletions(-) diff --git a/src/app/service_create_update.rs b/src/app/service_create_update.rs index fdd145e..8710408 100644 --- a/src/app/service_create_update.rs +++ b/src/app/service_create_update.rs @@ -1,11 +1,9 @@ use crate::app::service_types::{CreateBatchInput, CreateInput, ServiceContext, UpdateInput}; -use crate::app::service_utils::{ - must_resolve_existing, must_task, normalize_duplicate_title, unique_root_id, -}; +use crate::app::service_utils::{must_resolve_existing, must_task, normalize_duplicate_title}; use crate::app::state::{load_projected_state, persist_projection}; use crate::domain::alias::allocate_alias; use crate::domain::events::make_event; -use crate::domain::ids::{RootIdAllocator, is_valid_root_id, next_child_id}; +use crate::domain::ids::{TaskIdAllocator, is_valid_root_id, make_task_id}; use crate::domain::labels::add_label; use crate::domain::projector::apply_events; use crate::domain::similarity::{ @@ -95,11 +93,7 @@ pub fn create(ctx: &ServiceContext, input: &CreateInput) -> Result Result = Vec::with_capacity(planned.len()); let mut working_state = loaded.state.clone(); let mut parent_stack: Vec = Vec::new(); // depth → created/reused ID - let mut root_id_allocator: Option = None; + let mut id_allocator = TaskIdAllocator::new(&loaded.state); // Track tasks created in this batch by (normalized_title, parent_id) // so ensure can reuse earlier batch-created tasks for exact duplicates. @@ -447,17 +441,7 @@ pub fn create_batch(ctx: &ServiceContext, input: &CreateBatchInput) -> Result Result { - make_root_id(state) -} - pub fn must_task(state: &State, id: &str) -> Result { state .tasks diff --git a/src/app/sync.rs b/src/app/sync.rs index 88e8549..235c098 100644 --- a/src/app/sync.rs +++ b/src/app/sync.rs @@ -113,7 +113,7 @@ fn setup_sync_branch_locked(repo_root: &str, branch: &str) -> Result String { format!("tsq-{}", std::str::from_utf8(&buf).expect("ascii")) } -/// Mint a flat random canonical root id (`tsq-<8 crockford chars>`) that does -/// not collide with any existing task id. Old sequential `tsq-` ids -/// remain valid/readable; new tasks no longer use sequential allocation. -pub fn make_root_id(state: &State) -> Result { - loop { - let candidate = mint_random_canonical_id(); - if !state.tasks.contains_key(&candidate) { - return Ok(candidate); - } - } -} - -/// Mint a flat random canonical id for a child task. Children share the same -/// canonical id shape as roots (`tsq-<8 crockford chars>`); the parent link is -/// carried by `parent_id`, not encoded in the id. Old `parent.N` ids remain -/// valid/readable; new children no longer use the counter suffix shape. -pub fn next_child_id(state: &State, _parent_id: &str) -> String { +/// Mint a flat random canonical task id (`tsq-<8 crockford chars>`) that does +/// not collide with any existing task id. Roots and children share this shape; +/// parentage lives in `parent_id`, not in the id text. Old sequential +/// `tsq-` and `parent.N` ids remain valid/readable. +pub fn make_task_id(state: &State) -> String { loop { let candidate = mint_random_canonical_id(); if !state.tasks.contains_key(&candidate) { @@ -49,24 +36,24 @@ pub fn next_child_id(state: &State, _parent_id: &str) -> String { } /// Batch-friendly flat random id allocator. Reserves generated ids against -/// existing state and ids minted earlier in the same batch so a single -/// write lock can produce a consistent set of new tasks. -pub struct RootIdAllocator { +/// existing state and ids minted earlier in the same batch so a single write +/// lock can produce a consistent set of new tasks. +pub struct TaskIdAllocator { reserved_ids: HashSet, } -impl RootIdAllocator { - pub fn new(state: &State) -> Result { - Ok(Self { +impl TaskIdAllocator { + pub fn new(state: &State) -> Self { + Self { reserved_ids: state.tasks.keys().cloned().collect(), - }) + } } - pub fn next_id(&mut self) -> Result { + pub fn next_id(&mut self) -> String { loop { let candidate = mint_random_canonical_id(); if self.reserved_ids.insert(candidate.clone()) { - return Ok(candidate); + return candidate; } } } @@ -85,7 +72,10 @@ pub fn is_legacy_random_root_id(raw: &str) -> bool { return false; }; rest.len() == 8 - && rest - .chars() - .all(|ch| matches!(ch, '0'..='9' | 'a'..='h' | 'j'..='k' | 'm'..='n' | 'p'..='t' | 'v'..='z')) + && rest.chars().all(|ch| { + matches!( + ch, + '0'..='9' | 'a'..='h' | 'j'..='k' | 'm'..='n' | 'p'..='t' | 'v'..='z' + ) + }) } diff --git a/src/domain/projector_helpers.rs b/src/domain/projector_helpers.rs index 73c7361..cd9ed5c 100644 --- a/src/domain/projector_helpers.rs +++ b/src/domain/projector_helpers.rs @@ -238,30 +238,11 @@ pub(crate) fn clone_state(state: &State) -> State { tasks: state.tasks.clone(), deps, links, - child_counters: state.child_counters.clone(), created_order: state.created_order.clone(), applied_events: state.applied_events, } } -pub(crate) fn set_child_counter(state: &mut State, parent_id: &str, child_id: &str) { - let prefix = format!("{}.", parent_id); - if !child_id.starts_with(&prefix) { - return; - } - let segment = &child_id[prefix.len()..]; - if segment.is_empty() || !segment.chars().all(|c| c.is_ascii_digit()) { - return; - } - let Ok(counter) = segment.parse::() else { - return; - }; - let current = state.child_counters.get(parent_id).copied().unwrap_or(0); - if counter > current { - state.child_counters.insert(parent_id.to_string(), counter); - } -} - pub(crate) fn set_task_closed_state(task: &Task, ts: &str) -> Task { let mut next = task.clone(); next.status = TaskStatus::Closed; diff --git a/src/domain/projector_tasks.rs b/src/domain/projector_tasks.rs index ea543c9..1413882 100644 --- a/src/domain/projector_tasks.rs +++ b/src/domain/projector_tasks.rs @@ -2,7 +2,7 @@ use super::projector_helpers::{ as_bool, as_string, as_task_status, event_id_value, event_identifier, optional_planning_state_field, optional_priority_field, optional_string_array_field, optional_task_kind_field, optional_task_ref_field, optional_task_status_field, require_task, - set_child_counter, set_task_closed_state, task_status_to_string, + set_task_closed_state, task_status_to_string, }; use crate::domain::alias::{allocate_alias, is_alias_or_id_taken, normalize_alias}; use crate::errors::TsqError; @@ -96,7 +96,7 @@ pub(crate) fn apply_task_created( assignee: as_string(payload.get("assignee")), external_ref: as_string(payload.get("external_ref")), discovered_from, - parent_id: parent_id.clone(), + parent_id, superseded_by, duplicate_of, planning_state: Some(planning_state), @@ -113,9 +113,6 @@ pub(crate) fn apply_task_created( state.tasks.insert(event.task_id.clone(), task); state.created_order.push(event.task_id.clone()); - if let Some(parent_id) = parent_id { - set_child_counter(state, &parent_id, &event.task_id); - } Ok(()) } @@ -196,8 +193,7 @@ pub(crate) fn apply_task_updated( optional_task_ref_field(state, payload, "parent_id", event, "task.updated")? { assert_no_parent_cycle(state, &event.task_id, &parent_id, event, "task.updated")?; - next.parent_id = Some(parent_id.clone()); - set_child_counter(state, &parent_id, &event.task_id); + next.parent_id = Some(parent_id); } if let Some(duplicate_of) = diff --git a/src/domain/state.rs b/src/domain/state.rs index 06307d5..180a487 100644 --- a/src/domain/state.rs +++ b/src/domain/state.rs @@ -5,7 +5,6 @@ pub fn create_empty_state() -> State { tasks: std::collections::HashMap::new(), deps: std::collections::HashMap::new(), links: std::collections::HashMap::new(), - child_counters: std::collections::HashMap::new(), created_order: Vec::new(), applied_events: 0, } diff --git a/src/store/git.rs b/src/store/git.rs index 0ed13bd..1cd1041 100644 --- a/src/store/git.rs +++ b/src/store/git.rs @@ -137,15 +137,6 @@ pub fn branch_exists(repo_root: &Path, name: &str) -> Result { run_git_status(repo_root, &["show-ref", "--verify", "--quiet", &refspec]) } -pub fn remote_branch_exists(repo_root: &Path, name: &str) -> Result { - validate_branch_name(name)?; - let refspec = format!("refs/heads/{}", name); - run_git_status( - repo_root, - &["ls-remote", "--exit-code", "--heads", "origin", &refspec], - ) -} - pub fn remote_tracking_branch_exists(repo_root: &Path, name: &str) -> Result { validate_branch_name(name)?; let refspec = format!("refs/remotes/origin/{}", name); @@ -162,9 +153,7 @@ pub fn track_remote_branch(repo_root: &Path, name: &str) -> Result<(), TsqError> } pub fn fetch_remote_branch(repo_root: &Path, name: &str) -> Result<(), TsqError> { - validate_branch_name(name)?; - let remote_ref = format!("+refs/heads/{name}:refs/remotes/origin/{name}"); - run_git(repo_root, &["fetch", "origin", &remote_ref])?; + fetch_branch(repo_root, "origin", name)?; track_remote_branch(repo_root, name)?; Ok(()) } @@ -185,11 +174,6 @@ pub fn has_remote(repo_root: &Path, name: &str) -> Result { run_git_status(repo_root, &["remote", "get-url", name]) } -pub fn push_current(repo_root: &Path) -> Result<(), TsqError> { - run_git(repo_root, &["push"])?; - Ok(()) -} - pub fn push_current_set_upstream( repo_root: &Path, remote: &str, @@ -206,15 +190,15 @@ pub fn push_current_set_upstream( pub enum PushOutcome { /// Push succeeded. Ok, - /// Remote rejected the push (non-fast-forward / fetch-first). Recoverable - /// by fetch + merge + retry. Carries the raw stderr for diagnostics. + /// Remote rejected the push because it advanced during our push attempt. + /// Recoverable by fetch + merge + retry. Carries raw stderr for diagnostics. Rejected(String), } /// Push `branch` to `remote`, setting upstream, and classify the result. /// -/// A non-fast-forward / fetch-first rejection returns `PushOutcome::Rejected` -/// (recoverable); any other failure is a hard `TsqError`. +/// A non-fast-forward / fetch-first / ref-lock race returns +/// `PushOutcome::Rejected` (recoverable); any other failure is a hard `TsqError`. pub fn push_branch_with_status( repo_root: &Path, remote: &str, @@ -224,6 +208,8 @@ pub fn push_branch_with_status( let output = Command::new("git") .args(["push", "-u", remote, branch]) .current_dir(repo_root) + .env("LC_ALL", "C") + .env("LANG", "C") .output() .map_err(|_| git_not_available())?; if output.status.success() { @@ -234,7 +220,8 @@ pub fn push_branch_with_status( if lower.contains("non-fast-forward") || lower.contains("fetch first") || lower.contains("[rejected]") - || lower.contains("[remote rejected]") + || lower.contains("cannot lock ref") + || lower.contains("failed to update ref") { return Ok(PushOutcome::Rejected(stderr)); } @@ -427,7 +414,7 @@ pub fn ensure_worktree(repo_root: &Path, branch: &str) -> Result, pub deps: HashMap>, pub links: HashMap>>, - pub child_counters: HashMap, pub created_order: Vec, pub applied_events: usize, } diff --git a/tests/merge_driver.rs b/tests/merge_driver.rs index 9c5d729..3b358fb 100644 --- a/tests/merge_driver.rs +++ b/tests/merge_driver.rs @@ -146,7 +146,10 @@ fn test_merge_driver_duplicate_events() { assert_eq!(run.result.code, 0, "stderr: {}", run.result.stderr); assert_eq!(read_jsonl(&run.ours_path).len(), 6); - assert!(run.result.stderr.contains("duplicates removed")); + assert_eq!( + merged_ids(&run.ours_path), + vec!["01AAA", "01AAB", "01AAC", "01BBB", "01BBC", "01CCC"] + ); } #[test] @@ -263,7 +266,6 @@ fn test_merge_driver_event_id_fallback_dedup() { assert_eq!(run.result.code, 0, "stderr: {}", run.result.stderr); assert_eq!(read_jsonl(&run.ours_path).len(), 1); - assert!(run.result.stderr.contains("duplicates removed")); } #[test] @@ -342,7 +344,6 @@ fn test_merge_driver_both_sides_add_identical_dedup() { assert_eq!(run.result.code, 0, "stderr: {}", run.result.stderr); assert_eq!(read_jsonl(&run.ours_path).len(), 2); assert_eq!(merged_ids(&run.ours_path), vec!["01BASE", "01NEW"]); - assert!(run.result.stderr.contains("duplicates removed")); } #[test] diff --git a/tests/readable_identity.rs b/tests/readable_identity.rs index 5f801df..29ae0e7 100644 --- a/tests/readable_identity.rs +++ b/tests/readable_identity.rs @@ -1,20 +1,10 @@ mod common; use common::{create_task, init_repo, run_cli, run_json}; -use once_cell::sync::Lazy; -use regex::Regex; use serde_json::Value; use std::fs; use tasque::domain::similarity::DEFAULT_SIMILARITY_MIN_SCORE; -static RANDOM_ROOT_ID: Lazy = Lazy::new(|| { - Regex::new(r"^tsq-[0123456789abcdefghjkmnpqrstvwxyz]{8}$").expect("random root id regex") -}); - -fn is_random_canonical(id: &str) -> bool { - RANDOM_ROOT_ID.is_match(id) -} - #[test] fn create_projects_stable_alias_from_title() { let repo = common::make_repo(); @@ -76,11 +66,11 @@ fn new_root_ids_are_random_canonical() { let second = create_task(repo.path(), "Second sequential task"); assert!( - is_random_canonical(&first), + common::is_random_canonical_id(&first), "first id {first} not random canonical" ); assert!( - is_random_canonical(&second), + common::is_random_canonical_id(&second), "second id {second} not random canonical" ); assert_ne!(first, second, "ids must be distinct"); @@ -101,7 +91,7 @@ fn child_ids_are_flat_random_with_parent_set() { .as_str() .expect("child id"); assert!( - is_random_canonical(child_id), + common::is_random_canonical_id(child_id), "child id {child_id} not random canonical" ); assert_ne!(child_id, parent, "child id must not equal parent id"); @@ -141,7 +131,7 @@ fn explicit_legacy_random_id_does_not_affect_allocation() { let next = create_task(repo.path(), "First sequential after legacy"); assert!( - is_random_canonical(&next), + common::is_random_canonical_id(&next), "next id {next} not random canonical" ); assert_ne!(next, "tsq-00000042"); @@ -214,7 +204,6 @@ fn duplicate_exact_alias_is_ambiguous() { "tasks": {}, "deps": {}, "links": {}, - "child_counters": {}, "created_order": [], "applied_events": 0 })) @@ -292,7 +281,7 @@ fn allocation_succeeds_even_with_u64_max_sequential_present() { .as_str() .expect("next id"); assert!( - is_random_canonical(next), + common::is_random_canonical_id(next), "next id {next} not random canonical" ); assert_ne!(next, max_id); diff --git a/tests/sync_branch.rs b/tests/sync_branch.rs index d211338..3bbdcdd 100644 --- a/tests/sync_branch.rs +++ b/tests/sync_branch.rs @@ -310,6 +310,14 @@ fn clone_from_origin( clone } +#[cfg(unix)] +fn write_executable_hook(path: &std::path::Path, script: String) { + fs::write(path, script).expect("write git hook"); + let mut permissions = fs::metadata(path).expect("hook metadata").permissions(); + permissions.set_mode(0o755); + fs::set_permissions(path, permissions).expect("chmod git hook"); +} + /// Two clones create distinct tasks and sync. The clone that pushes second must /// fetch + merge the first clone's events before pushing (local-first two-way /// sync), and a subsequent sync on the first clone must converge to both events. @@ -392,10 +400,7 @@ fn sync_retries_when_remote_advances_during_push() { flag = flag.display(), advancer_wt = advancer.join(".git").join("tsq-sync").display() ); - fs::write(&hook, script).expect("write pre-push hook"); - let mut permissions = fs::metadata(&hook).expect("hook metadata").permissions(); - permissions.set_mode(0o755); - fs::set_permissions(&hook, permissions).expect("chmod pre-push hook"); + write_executable_hook(&hook, script); let sync = run_cli(&target, ["sync"]); assert_eq!( @@ -414,6 +419,85 @@ fn sync_retries_when_remote_advances_during_push() { } } +/// If the remote advances before every push attempt, sync must stop at the +/// documented retry cap and surface a structured storage error instead of +/// spinning forever or reporting success. +#[cfg(unix)] +#[test] +fn sync_stops_after_repeated_remote_advances() { + let repo = make_repo(); + let base = repo.path(); + let (_source, remote) = seed_source_with_origin(base, "Seed task"); + + let target = clone_from_origin(base, &remote, "target"); + let advancer = clone_from_origin(base, &remote, "advancer"); + + let target_create = run_cli( + &target, + ["create", "Target task", "--id", "tsq-cccc3333", "--force"], + ); + assert_eq!(target_create.code, 0, "stderr: {}", target_create.stderr); + let advancer_create = run_cli( + &advancer, + ["create", "Advancer task", "--id", "tsq-cccc4444", "--force"], + ); + assert_eq!( + advancer_create.code, 0, + "stderr: {}", + advancer_create.stderr + ); + + let count_file = base.join("pre-push-count"); + let hook = target.join(".git").join("hooks").join("pre-push"); + let script = format!( + r#"#!/bin/sh +count=$(cat '{count_file}' 2>/dev/null || echo 0) +count=$((count + 1)) +printf '%s' "$count" > '{count_file}' +unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE + +event_id=$(printf '01HOOK%018d' "$count") +task_id=$(printf 'tsq-eeee%04d' "$count") +cat >> '{advancer_wt}/.tasque/events.jsonl' </dev/null 2>&1 || exit 1 +git -C '{advancer_wt}' push origin tsq-sync >/dev/null 2>&1 || exit 1 +exit 0 +"#, + count_file = count_file.display(), + advancer_wt = advancer.join(".git").join("tsq-sync").display() + ); + write_executable_hook(&hook, script); + + let sync = run_cli(&target, ["sync", "--json"]); + assert_eq!( + sync.code, 2, + "sync should stop after repeated remote advances\nstdout:\n{}\nstderr:\n{}", + sync.stdout, sync.stderr + ); + let envelope: Value = serde_json::from_str(sync.stdout.trim()).expect("json envelope"); + let error = envelope.get("error").expect("error object"); + assert_eq!( + error.get("code").and_then(Value::as_str), + Some("SYNC_PUSH_REJECTED") + ); + assert_eq!( + error + .get("details") + .and_then(|details| details.get("attempts")) + .and_then(Value::as_u64), + Some(3) + ); + assert_eq!( + fs::read_to_string(&count_file).expect("pre-push count"), + "3", + "pre-push hook should have advanced remote once per attempt" + ); +} + /// With an `origin` remote that has no sync branch yet, `tsq sync` publishes the /// local sync branch with `push -u` (no upstream configured beforehand). #[test] diff --git a/tests/verb_first_task_commands.rs b/tests/verb_first_task_commands.rs index 945679f..1040401 100644 --- a/tests/verb_first_task_commands.rs +++ b/tests/verb_first_task_commands.rs @@ -58,31 +58,6 @@ fn create_from_file_allocates_random_ids_unrelated_to_high_existing_id() { assert_ne!(second, "tsq-44"); } -#[test] -fn create_from_file_succeeds_alongside_last_u64_root_id() { - let repo = common::make_repo(); - init_repo(repo.path()); - create_task_with_args( - repo.path(), - "Penultimate root", - &["--id", "tsq-18446744073709551614"], - ); - let file = repo.path().join("tasks.md"); - std::fs::write(&file, "- Last root\n").unwrap(); - - let result = run_json(repo.path(), ["create", "--from-file", "tasks.md"]); - - assert_eq!(result.cli.code, 0); - let id = result.envelope["data"]["task"]["id"] - .as_str() - .expect("task id"); - assert!( - common::is_random_canonical_id(id), - "new id {id} not random canonical" - ); - assert_ne!(id, "tsq-18446744073709551615"); -} - #[test] fn create_from_file_maps_nested_bullets_to_parent_ids() { let repo = common::make_repo(); From 68d6b12d070b64a2a49ba0f8d0f69f4f5ccac2c8 Mon Sep 17 00:00:00 2001 From: Aditya Sharma Date: Wed, 8 Jul 2026 11:35:10 -0700 Subject: [PATCH 3/4] fix(sync): harden sync conflict handling --- AGENTS-reference.md | 4 +- README.md | 4 +- docs/sync.md | 10 ++- src/app/sync.rs | 82 +++++++++++--------- src/domain/ids.rs | 11 +-- src/domain/resolve.rs | 76 +++++++----------- src/store/events.rs | 35 +++++++-- src/store/git.rs | 40 ++++------ src/store/merge_driver.rs | 56 ++++++++------ tests/common/mod.rs | 5 +- tests/event_read_validation.rs | 59 +++++++++----- tests/merge_driver.rs | 101 ++++++++++++++++++------ tests/readable_identity.rs | 36 +++++---- tests/sync_branch.rs | 124 +++++++++++++++++++++++++----- tests/verb_first_task_commands.rs | 2 - 15 files changed, 412 insertions(+), 233 deletions(-) diff --git a/AGENTS-reference.md b/AGENTS-reference.md index de1ace3..61662c3 100644 --- a/AGENTS-reference.md +++ b/AGENTS-reference.md @@ -111,7 +111,7 @@ Relation types: Notes: - For `find ready` and status-based `find` commands, `--full` is only valid with `--tree`. `--tree --full` keeps the full status set instead of applying the default tree status narrowing. `find search --full` remains valid without `--tree`. -- `--id ` accepts `tsq-` or legacy `tsq-<8 crockford base32 chars>`. +- `--id ` accepts random canonical `tsq-<8 lowercase crockford chars>`, sequential root `tsq-`, child `.`, and legacy `tsq-<8 crockford base32 chars>` ids. - Commands that accept a task ID also accept exact aliases and unique alias prefixes unless `--exact-id` is used. - `tsq find similar ""` shows ranked duplicate candidates with scores and reasons. - `tsq create` refuses similar open/in-progress/blocked/deferred tasks unless `--force` is passed. @@ -160,7 +160,7 @@ Notes: - `tsq labels` - `tsq history [--limit ] [--type ] [--actor ] [--since ]` - `tsq root` -- `tsq sync [--no-push]` — two-way sync: commit local changes, fetch remote (upstream then `origin`), merge, push; sets upstream on first push; `--no-push` commits locally only. See [docs/sync.md](./docs/sync.md) for conflict resolution. +- `tsq sync [--no-push]` — two-way sync: commit local changes, fetch remote (`origin`), merge, push; sets upstream on first push; `--no-push` commits locally only. See [docs/sync.md](./docs/sync.md) for conflict resolution. - `tsq hooks install [--force]` - `tsq hooks uninstall` - `tsq migrate [--sync-branch|--worktree-name ]` diff --git a/README.md b/README.md index d2941e3..ed57105 100644 --- a/README.md +++ b/README.md @@ -160,8 +160,8 @@ Git repos default to a dedicated sync worktree: - `tsq init` configures `tsq-sync` by default and redirects data operations there. - Fresh clones fetch the configured sync branch and create the worktree on first use. -- `tsq sync` runs local-first two-way sync: commit local changes, fetch the remote - branch (upstream then `origin`), merge, then push — setting upstream on first push. +- `tsq sync` runs local-first two-way sync: commit local changes, fetch the `origin` + branch, merge, then push — setting upstream on first push. - `tsq sync --no-push` commits locally without touching the network. - Existing git repos with main-tree `.tasque` data migrate automatically when `tsq` next resolves the project root. diff --git a/docs/sync.md b/docs/sync.md index 67dba81..0e2d870 100644 --- a/docs/sync.md +++ b/docs/sync.md @@ -11,9 +11,15 @@ In a git repo, `tsq init` moves task data off your code branch into a dedicated `--sync-branch` / `--worktree-name`). Your code branch keeps only `.tasque/config.json` so Tasque can find the sync branch. +Main worktree: + +- `.tasque/config.json` — sync-branch pointer/locator only. + +Sync worktree: + - `.tasque/events.jsonl` — append-only event log (canonical source of truth). - `.tasque/specs//spec.md` — task specs. -- `.tasque/config.json` — project settings (in the main worktree). +- `.tasque/config.json` — task-data config committed with the sync branch. - `.tasque/state.json`, `.tasque/.lock`, `.tasque/snapshots/` — local-only, gitignored, always rebuildable. @@ -22,7 +28,7 @@ In a git repo, `tsq init` moves task data off your code branch into a dedicated `tsq sync` runs a local-first two-way sync in this order: 1. **Commit** local changes to the sync branch (events, specs, config). -2. **Fetch** the remote branch (upstream first, then `origin`), if it exists. +2. **Fetch** the `origin` branch, if it exists. 3. **Merge** fetched changes. `events.jsonl` is merged by the `tasque-events` driver (see below); other files use git's default merge. 4. **Push** the result back to the remote, setting upstream on first push. diff --git a/src/app/sync.rs b/src/app/sync.rs index 235c098..ba5649a 100644 --- a/src/app/sync.rs +++ b/src/app/sync.rs @@ -141,7 +141,7 @@ fn setup_sync_branch_locked(repo_root: &str, branch: &str) -> Result { - git::push_current_set_upstream(wt_path, &remote, branch)?; + git::push_current_set_upstream(wt_path, "origin", branch)?; } PushMode::BestEffort => { - if let Err(error) = git::push_current_set_upstream(wt_path, &remote, branch) { - // `remote` is git-derived and `error.message` may carry - // untrusted data; sanitize both before writing to stderr to - // avoid emitting raw terminal control sequences. + if let Err(error) = git::push_current_set_upstream(wt_path, "origin", branch) { eprintln!( - "tsq: warning: migrated events to sync branch but push to '{}' failed: {}; run 'tsq sync' to push later", - crate::cli::render::sanitize_inline(&remote), + "tsq: warning: migrated events to sync branch but push to 'origin' failed: {}; run 'tsq sync' to push later", crate::cli::render::sanitize_inline(&error.message) ); } @@ -251,20 +247,9 @@ pub fn sync_worktree(repo_root: &str, push: bool) -> Result Result { let worktree_path = path.to_string_lossy().to_string(); - // `--no-push`: purely local commit, no network. - if !push { - let committed = git::commit_worktree(path, SYNC_COMMIT_MESSAGE)?; - return Ok(SyncRunResult { - branch: branch.to_string(), - worktree_path, - committed, - pushed: false, - has_upstream: git::has_upstream(path)?, - }); - } - // Finalize any merge left in progress by a prior conflicted sync. If - // conflicts remain unresolved, re-emit the structured conflict error. + // 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)?; @@ -275,13 +260,27 @@ fn sync_worktree_locked(path: &Path, branch: &str, push: bool) -> Result Result { + 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::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)? { + match git::push_branch_with_status(path, remote, branch)? { git::PushOutcome::Ok => { return Ok(SyncRunResult { branch: branch.to_string(), diff --git a/src/domain/ids.rs b/src/domain/ids.rs index 8fdb8c4..608acc4 100644 --- a/src/domain/ids.rs +++ b/src/domain/ids.rs @@ -27,12 +27,7 @@ fn mint_random_canonical_id() -> String { /// parentage lives in `parent_id`, not in the id text. Old sequential /// `tsq-` and `parent.N` ids remain valid/readable. pub fn make_task_id(state: &State) -> String { - loop { - let candidate = mint_random_canonical_id(); - if !state.tasks.contains_key(&candidate) { - return candidate; - } - } + TaskIdAllocator::new(state).next_id() } /// Batch-friendly flat random id allocator. Reserves generated ids against @@ -63,11 +58,11 @@ pub fn is_valid_root_id(raw: &str) -> bool { is_sequential_root_id(raw) || is_legacy_random_root_id(raw) } -pub fn is_sequential_root_id(raw: &str) -> bool { +fn is_sequential_root_id(raw: &str) -> bool { SEQUENTIAL_ROOT_ID.is_match(raw) } -pub fn is_legacy_random_root_id(raw: &str) -> bool { +fn is_legacy_random_root_id(raw: &str) -> bool { let Some(rest) = raw.strip_prefix("tsq-") else { return false; }; diff --git a/src/domain/resolve.rs b/src/domain/resolve.rs index b5cc62a..7b697dc 100644 --- a/src/domain/resolve.rs +++ b/src/domain/resolve.rs @@ -15,76 +15,58 @@ pub fn resolve_task_id(state: &State, raw: &str, exact_id: bool) -> Result = state + + let exact_alias_matches = state .tasks .values() .filter(|task| task.alias.to_lowercase() == raw_alias) .map(|task| (task.id.clone(), task.alias.clone())) .collect(); - exact_alias_matches.sort_by(|a, b| a.0.cmp(&b.0)); - match exact_alias_matches.len() { - 1 => return Ok(exact_alias_matches[0].0.clone()), - n if n > 1 => { - return Err( - TsqError::new("TASK_ID_AMBIGUOUS", "Task ID is ambiguous", 1).with_details(json!({ - "input": raw, - "candidates": exact_alias_matches - .into_iter() - .map(|(id, alias)| json!({ "id": id, "alias": alias })) - .collect::>() - })), - ); - } - _ => {} + if let Some(id) = pick_unique_match(exact_alias_matches, raw)? { + return Ok(id); } - let mut id_matches: Vec<(String, String)> = state + let id_matches = state .tasks .values() .filter(|task| task.id.starts_with(raw)) .map(|task| (task.id.clone(), task.alias.clone())) .collect(); - id_matches.sort_by(|a, b| a.0.cmp(&b.0)); - - match id_matches.len() { - 1 => return Ok(id_matches[0].0.clone()), - n if n > 1 => { - return Err( - TsqError::new("TASK_ID_AMBIGUOUS", "Task ID is ambiguous", 1).with_details(json!({ - "input": raw, - "candidates": id_matches - .into_iter() - .map(|(id, alias)| json!({ "id": id, "alias": alias })) - .collect::>() - })), - ); - } - _ => {} + if let Some(id) = pick_unique_match(id_matches, raw)? { + return Ok(id); } - let mut alias_matches: Vec<(String, String)> = state + let alias_matches = state .tasks .values() .filter(|task| task.alias.to_lowercase().starts_with(&raw_alias)) .map(|task| (task.id.clone(), task.alias.clone())) .collect(); - alias_matches.sort_by(|a, b| a.0.cmp(&b.0)); + pick_unique_match(alias_matches, raw)?.ok_or_else(|| not_found(raw)) +} - match alias_matches.len() { - 0 => Err(not_found(raw)), - 1 => Ok(alias_matches[0].0.clone()), - _ => Err( - TsqError::new("TASK_ID_AMBIGUOUS", "Task ID is ambiguous", 1).with_details(json!({ - "input": raw, - "candidates": alias_matches - .into_iter() - .map(|(id, alias)| json!({ "id": id, "alias": alias })) - .collect::>() - })), - ), +fn pick_unique_match( + mut matches: Vec<(String, String)>, + raw: &str, +) -> Result, TsqError> { + matches.sort_by(|a, b| a.0.cmp(&b.0)); + match matches.len() { + 0 => Ok(None), + 1 => Ok(Some(matches.remove(0).0)), + _ => Err(ambiguous(raw, matches)), } } +fn ambiguous(raw: &str, matches: Vec<(String, String)>) -> TsqError { + TsqError::new("TASK_ID_AMBIGUOUS", "Task ID is ambiguous", 1).with_details(json!({ + "input": raw, + "candidates": matches + .into_iter() + .map(|(id, alias)| json!({ "id": id, "alias": alias })) + .collect::>() + })) +} + fn not_found(raw: &str) -> TsqError { TsqError::new("TASK_NOT_FOUND", "Task ID not found", 1).with_details(json!({ "input": raw diff --git a/src/store/events.rs b/src/store/events.rs index 02f8114..4c8d5e3 100644 --- a/src/store/events.rs +++ b/src/store/events.rs @@ -6,6 +6,7 @@ use crate::errors::TsqError; use crate::store::atomic::{any_error_value, io_error_value}; use crate::store::paths::get_paths; use crate::types::{EventLogMetadata, EventRecord, EventType}; +use chrono::{DateTime, SecondsFormat, Utc}; use serde_json::{Map, Value}; use sha2::{Digest, Sha256}; use std::fs::{OpenOptions, create_dir_all, read, read_to_string}; @@ -320,13 +321,31 @@ fn parse_event_record(value: &Value, line: usize) -> Result DateTime::parse_from_rfc3339(value) + .map(|parsed| { + parsed + .with_timezone(&Utc) + .to_rfc3339_opts(SecondsFormat::Millis, true) + }) + .map_err(|_| { + TsqError::new( + "EVENTS_CORRUPT", + format!( + "Invalid event at line {}: ts must be an RFC3339 timestamp", + line + ), + 2, + ) + })?, + None => { + return Err(TsqError::new( + "EVENTS_CORRUPT", + format!("Invalid event at line {}: ts must be a string", line), + 2, + )); + } + }; let actor = obj .get("actor") @@ -404,7 +423,7 @@ fn parse_event_record(value: &Value, line: usize) -> Result Result, TsqError> { } } -pub fn current_upstream_remote(repo_root: &Path) -> Result, TsqError> { - let Some(branch) = current_branch(repo_root)? else { - return Ok(None); - }; - let key = format!("branch.{branch}.remote"); - if run_git_status(repo_root, &["config", "--get", &key])? { - let remote = run_git(repo_root, &["config", "--get", &key])?; - if !remote.is_empty() && remote != "." { - return Ok(Some(remote)); - } - } - if has_remote(repo_root, "origin")? { - return Ok(Some("origin".to_string())); - } - Ok(None) -} - /// Returns true if a local branch with the given name exists. pub fn branch_exists(repo_root: &Path, name: &str) -> Result { validate_branch_name(name)?; @@ -179,9 +162,10 @@ pub fn push_current_set_upstream( remote: &str, branch: &str, ) -> Result<(), TsqError> { - validate_branch_name(branch)?; - run_git(repo_root, &["push", "-u", remote, branch])?; - Ok(()) + match push_branch_with_status(repo_root, remote, branch)? { + PushOutcome::Ok => Ok(()), + PushOutcome::Rejected(stderr) => Err(git_error("git push failed", stderr)), + } } /// Outcome of a push attempt that distinguishes recoverable rejections from @@ -273,8 +257,11 @@ pub fn fetch_branch(repo_root: &Path, remote: &str, branch: &str) -> Result<(), /// Merge the `/` tracking ref into the current worktree branch. /// -/// Uses `--no-edit` so the merge is non-interactive. On conflict, the merge is -/// left in progress (`MERGE_HEAD` set) and the unmerged paths are returned. +/// Uses `--no-edit` so the merge is non-interactive. Allows unrelated histories +/// because two machines can independently create the sync branch before either +/// has pushed it; event replay validation still gates the merged result. +/// On conflict, the merge is left in progress (`MERGE_HEAD` set) and the +/// unmerged paths are returned. pub fn merge_tracking_branch( repo_root: &Path, remote: &str, @@ -283,7 +270,12 @@ pub fn merge_tracking_branch( validate_branch_name(branch)?; let tracking = format!("{remote}/{branch}"); let output = Command::new("git") - .args(["merge", "--no-edit", &tracking]) + .args([ + "merge", + "--no-edit", + "--allow-unrelated-histories", + &tracking, + ]) .current_dir(repo_root) .output() .map_err(|_| git_not_available())?; @@ -303,7 +295,7 @@ pub fn merge_tracking_branch( /// Stages any resolved/pending changes and creates the merge commit, preserving /// the merge message (`--no-edit`). pub fn finalize_merge(repo_root: &Path) -> Result<(), TsqError> { - run_git(repo_root, &["add", "."])?; + run_git(repo_root, &["add", "--all"])?; run_git(repo_root, &["commit", "--no-edit"])?; Ok(()) } diff --git a/src/store/merge_driver.rs b/src/store/merge_driver.rs index 60c69b1..c03b599 100644 --- a/src/store/merge_driver.rs +++ b/src/store/merge_driver.rs @@ -9,10 +9,19 @@ use std::fs; use std::io::Write; use std::path::Path; -/// Extract the canonical event ID from an EventRecord. -/// Prefers `id`, falls back to `event_id`. -fn event_id(record: &EventRecord) -> Option<&str> { - record.id.as_deref().or(record.event_id.as_deref()) +/// Extract the canonical event ID from a reader-normalized EventRecord. +fn event_id(record: &EventRecord) -> Result<&str, TsqError> { + record + .id + .as_deref() + .or(record.event_id.as_deref()) + .ok_or_else(|| { + TsqError::new( + "EVENTS_CORRUPT", + "Event missing id field during merge after read validation", + 2, + ) + }) } /// Serialize an EventRecord to its canonical JSON string for comparison. @@ -27,6 +36,11 @@ fn canonical_json(record: &EventRecord) -> Result { }) } +fn order_key(id: &str, records: &HashMap) -> 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: @@ -68,16 +82,7 @@ pub fn merge_events_files( let mut order: Vec = Vec::new(); for record in events { total_input += 1; - let id = match event_id(&record) { - Some(id) => id.to_string(), - None => { - return Err(TsqError::new( - "MERGE_MISSING_ID", - "Event missing id field during merge", - 2, - )); - } - }; + let id = event_id(&record)?.to_string(); let json = canonical_json(&record)?; match id_to_json.get(&id) { Some(existing_json) => { @@ -132,30 +137,32 @@ pub fn merge_events_files( *incoming.entry(to.clone()).or_default() += 1; } - // Kahn's algorithm with a min-heap keyed by event ID. Picking the smallest - // available ID at each step yields a stable, direction-independent order. - let mut heap: BinaryHeap> = incoming + // Kahn's algorithm with a min-heap keyed by (timestamp, event ID). The + // timestamp primary key makes last-write-wins projection explicit for + // independent updates; the event ID secondary key keeps output stable when + // timestamps collide. + let mut heap: BinaryHeap> = incoming .iter() .filter(|(_, count)| **count == 0) - .map(|(id, _)| Reverse(id.clone())) + .map(|(id, _)| order_key(id, &id_to_record)) .collect(); let mut sorted_ids: Vec = Vec::with_capacity(unique_count); - while let Some(Reverse(id)) = heap.pop() { + while let Some(Reverse((_ts, id))) = heap.pop() { sorted_ids.push(id.clone()); if let Some(nexts) = outgoing.get(&id) { for next in nexts { let count = incoming.entry(next.clone()).or_insert(0); *count -= 1; if *count == 0 { - heap.push(Reverse(next.clone())); + heap.push(order_key(next, &id_to_record)); } } } } // Cycle fallback: if constraints formed a cycle (same events recorded in - // conflicting orders across sources), append the remaining IDs in sorted - // order to keep the output deterministic rather than failing silently. + // conflicting orders across sources), append the remaining IDs in the same + // deterministic (timestamp, event ID) order rather than failing silently. if sorted_ids.len() < unique_count { let emitted: HashSet = sorted_ids.iter().cloned().collect(); let mut remaining: Vec = id_to_record @@ -163,7 +170,10 @@ pub fn merge_events_files( .filter(|id| !emitted.contains(*id)) .cloned() .collect(); - remaining.sort(); + remaining.sort_by_key(|id| { + let record = id_to_record.get(id).expect("id present in map"); + (record.ts.clone(), id.clone()) + }); sorted_ids.extend(remaining); } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index bc8250d..7a8ef3b 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1,15 +1,14 @@ #![allow(dead_code)] -use once_cell::sync::Lazy; use regex::Regex; use serde_json::Value; use std::path::{Path, PathBuf}; use std::process::Command; -use std::sync::OnceLock; +use std::sync::{LazyLock, OnceLock}; use tasque::types::SCHEMA_VERSION; use tempfile::{Builder, TempDir}; -static RANDOM_CANONICAL_ID: Lazy = Lazy::new(|| { +static RANDOM_CANONICAL_ID: LazyLock = LazyLock::new(|| { Regex::new(r"^tsq-[0123456789abcdefghjkmnpqrstvwxyz]{8}$").expect("random canonical id regex") }); diff --git a/tests/event_read_validation.rs b/tests/event_read_validation.rs index aa74930..32a7b88 100644 --- a/tests/event_read_validation.rs +++ b/tests/event_read_validation.rs @@ -34,39 +34,62 @@ fn write_event(payload: serde_json::Value) -> (TempDir, std::path::PathBuf) { } #[test] -fn read_events_rejects_conflict_marker_as_corrupt() { +fn read_events_rejects_conflict_markers_as_corrupt() { let dir = TempDir::new().expect("tempdir"); - let path = dir.path().join("events.jsonl"); let first = task_created_event("tsq-root0001", "first"); let first_line = serde_json::to_string(&first).expect("serialize first"); - fs::write( - &path, - format!( - "{first_line}\n<<<<<<< HEAD\n{{}}\n||||||| base\n{{}}\n=======\n{{}}\n>>>>>>> branch\n" - ), - ) - .expect("write conflict markers"); + + for (index, marker) in ["<<<<<<< HEAD", "||||||| base", "=======", ">>>>>>> branch"] + .into_iter() + .enumerate() + { + let path = dir.path().join(format!("events-{index}.jsonl")); + fs::write(&path, format!("{first_line}\n{marker}\n{{}}\n")).expect("write conflict marker"); + + let err = match read_events_from_path(&path) { + Ok(_) => panic!("conflict marker {marker} must not be ingested as valid JSON"), + Err(error) => error, + }; + + assert_eq!(err.code, "EVENTS_CORRUPT"); + assert_eq!(err.exit_code, 2); + assert!( + err.message.contains("Conflict marker"), + "error must name the conflict marker: {}", + err.message + ); + } +} + +#[test] +fn read_events_rejects_invalid_status_set_payload_as_corrupt() { + let (_dir, path) = write_event(json!({"status": "done"})); let err = match read_events_from_path(&path) { - Ok(_) => panic!("conflict markers must not be ingested as valid JSON"), + Ok(_) => panic!("invalid status should fail at read boundary"), Err(error) => error, }; assert_eq!(err.code, "EVENTS_CORRUPT"); assert_eq!(err.exit_code, 2); - assert!( - err.message.contains("Conflict marker"), - "error must name the conflict marker: {}", - err.message - ); } #[test] -fn read_events_rejects_invalid_status_set_payload_as_corrupt() { - let (_dir, path) = write_event(json!({"status": "done"})); +fn read_events_rejects_non_rfc3339_timestamp_as_corrupt() { + let dir = TempDir::new().expect("tempdir"); + let path = dir.path().join("events.jsonl"); + let event = json!({ + "id": "01HX0000000000000000000002", + "ts": "yesterday", + "actor": "test", + "type": "task.created", + "task_id": "tsq-root0001", + "payload": {"title": "bad timestamp"}, + }); + fs::write(&path, format!("{}\n", event)).expect("write event"); let err = match read_events_from_path(&path) { - Ok(_) => panic!("invalid status should fail at read boundary"), + Ok(_) => panic!("invalid timestamp should fail at read boundary"), Err(error) => error, }; diff --git a/tests/merge_driver.rs b/tests/merge_driver.rs index 3b358fb..bbf25c0 100644 --- a/tests/merge_driver.rs +++ b/tests/merge_driver.rs @@ -5,6 +5,9 @@ use serde_json::{Map, Value}; use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; +use tasque::domain::projector::apply_events; +use tasque::domain::state::create_empty_state; +use tasque::store::events::read_events_from_path; use tasque::types::{EventRecord, EventType}; struct MergeRun { @@ -194,7 +197,55 @@ fn test_merge_driver_preserves_causal_source_order_when_ids_sort_backwards() { assert_eq!(run.result.code, 0, "stderr: {}", run.result.stderr); assert_eq!( merged_ids(&run.ours_path), - vec!["02CREATE", "01UPDATE", "03THEIRS"] + vec!["02CREATE", "03THEIRS", "01UPDATE"] + ); +} + +#[test] +fn test_merge_driver_concurrent_updates_use_timestamp_order() { + let repo_left = make_repo(); + let repo_right = make_repo(); + let create = make_event("02CREATE", "original"); + let mut earlier_update = make_update_event("ZZZEARLY", &create.task_id, "earlier"); + earlier_update.ts = "2026-01-01T11:00:00.000Z".to_string(); + let mut later_update = make_update_event("AAAALATE", &create.task_id, "later"); + later_update.ts = "2026-01-01T08:00:00.000-05:00".to_string(); + + let base_events = vec![create.clone()]; + let ours_events = vec![create.clone(), later_update.clone()]; + let theirs_events = vec![create, earlier_update]; + let run_left = run_merge_driver(repo_left.path(), &base_events, &ours_events, &theirs_events); + let run_right = run_merge_driver( + repo_right.path(), + &base_events, + &theirs_events, + &ours_events, + ); + + assert_eq!( + run_left.result.code, 0, + "stderr: {}", + run_left.result.stderr + ); + assert_eq!( + run_right.result.code, 0, + "stderr: {}", + run_right.result.stderr + ); + assert_eq!( + fs::read_to_string(&run_left.ours_path).unwrap(), + fs::read_to_string(&run_right.ours_path).unwrap(), + "timestamp tie-break must be byte-identical regardless of merge direction" + ); + + let merged_events = read_events_from_path(&run_left.ours_path) + .expect("merged events") + .events; + let state = apply_events(&create_empty_state(), &merged_events).expect("replay merged events"); + assert_eq!(state.tasks["tsq-02CREATE"].title, "later"); + assert_eq!( + merged_ids(&run_left.ours_path), + vec!["02CREATE", "ZZZEARLY", "AAAALATE"] ); } @@ -262,10 +313,35 @@ fn test_merge_driver_event_id_fallback_dedup() { ..legacy_only_event_id.clone() }; - let run = run_merge_driver(repo.path(), &[], &[legacy_only_event_id], &[canonical]); + let run = run_merge_driver( + repo.path(), + &[], + std::slice::from_ref(&legacy_only_event_id), + std::slice::from_ref(&canonical), + ); assert_eq!(run.result.code, 0, "stderr: {}", run.result.stderr); - assert_eq!(read_jsonl(&run.ours_path).len(), 1); + let merged = read_jsonl(&run.ours_path); + assert_eq!(merged.len(), 1); + assert_eq!(merged[0]["id"].as_str(), Some("01SHARED")); + assert_eq!(merged[0]["event_id"].as_str(), Some("01SHARED")); + + let repo_swapped = make_repo(); + let run_swapped = run_merge_driver( + repo_swapped.path(), + &[], + std::slice::from_ref(&canonical), + std::slice::from_ref(&legacy_only_event_id), + ); + assert_eq!( + run_swapped.result.code, 0, + "stderr: {}", + run_swapped.result.stderr + ); + let merged_swapped = read_jsonl(&run_swapped.ours_path); + assert_eq!(merged_swapped.len(), 1); + assert_eq!(merged_swapped[0]["id"].as_str(), Some("01SHARED")); + assert_eq!(merged_swapped[0]["event_id"].as_str(), Some("01SHARED")); } #[test] @@ -327,25 +403,6 @@ fn test_merge_driver_cycle_fallback_is_deterministic() { assert_eq!(merged_ids(&run_left.ours_path), vec!["01AAA", "01BBB"]); } -#[test] -fn test_merge_driver_both_sides_add_identical_dedup() { - // Both sides add the SAME new event (same id, same payload). It must appear - // exactly once in the merged output. - let repo = make_repo(); - let base_events = vec![make_event("01BASE", "base")]; - let shared_new = make_event("01NEW", "new-from-both"); - let mut ours_events = base_events.clone(); - ours_events.push(shared_new.clone()); - let mut theirs_events = base_events.clone(); - theirs_events.push(shared_new); - - let run = run_merge_driver(repo.path(), &base_events, &ours_events, &theirs_events); - - assert_eq!(run.result.code, 0, "stderr: {}", run.result.stderr); - assert_eq!(read_jsonl(&run.ours_path).len(), 2); - assert_eq!(merged_ids(&run.ours_path), vec!["01BASE", "01NEW"]); -} - #[test] fn test_merge_driver_mixed_event_types() { let repo = make_repo(); diff --git a/tests/readable_identity.rs b/tests/readable_identity.rs index 29ae0e7..4fa60d7 100644 --- a/tests/readable_identity.rs +++ b/tests/readable_identity.rs @@ -62,8 +62,8 @@ fn new_root_ids_are_random_canonical() { let repo = common::make_repo(); init_repo(repo.path()); - let first = create_task(repo.path(), "First sequential task"); - let second = create_task(repo.path(), "Second sequential task"); + let first = create_task(repo.path(), "First random task"); + let second = create_task(repo.path(), "Second random task"); assert!( common::is_random_canonical_id(&first), @@ -74,8 +74,6 @@ fn new_root_ids_are_random_canonical() { "second id {second} not random canonical" ); assert_ne!(first, second, "ids must be distinct"); - assert_ne!(first, "tsq-1"); - assert_ne!(second, "tsq-2"); } #[test] @@ -127,8 +125,8 @@ fn explicit_legacy_random_id_does_not_affect_allocation() { let repo = common::make_repo(); init_repo(repo.path()); - common::create_task_with_args(repo.path(), "Legacy numeric ID", &["--id", "tsq-00000042"]); - let next = create_task(repo.path(), "First sequential after legacy"); + common::create_task_with_args(repo.path(), "Legacy random ID", &["--id", "tsq-00000042"]); + let next = create_task(repo.path(), "First random after legacy"); assert!( common::is_random_canonical_id(&next), @@ -173,6 +171,7 @@ fn commands_accept_alias_case_insensitively() { fn duplicate_exact_alias_is_ambiguous() { use std::collections::HashMap; use tasque::domain::resolve::resolve_task_id; + use tasque::domain::state::create_empty_state; use tasque::types::{State, Task}; // Two tasks with colliding exact aliases (case-insensitive). The CLI path @@ -200,19 +199,26 @@ fn duplicate_exact_alias_is_ambiguous() { "tsq-bbbbbbbb".to_string(), mk_task("tsq-bbbbbbbb", "IMPROVE-SEARCH-WARNINGS"), ); - let state: State = serde_json::from_value(serde_json::json!({ - "tasks": {}, - "deps": {}, - "links": {}, - "created_order": [], - "applied_events": 0 - })) - .expect("base state"); - let state = State { tasks, ..state }; + let state = State { + tasks, + ..create_empty_state() + }; let result = resolve_task_id(&state, "improve-search-warnings", false); let err = result.expect_err("expected ambiguous error"); assert_eq!(err.code, "TASK_ID_AMBIGUOUS"); + let details = err.details.expect("ambiguous details"); + let candidates = details["candidates"].as_array().expect("candidates"); + assert_eq!(candidates.len(), 2); + let mut ids: Vec<&str> = candidates + .iter() + .map(|candidate| { + assert!(candidate.get("alias").is_some()); + candidate["id"].as_str().expect("candidate id") + }) + .collect(); + ids.sort_unstable(); + assert_eq!(ids, vec!["tsq-aaaaaaaa", "tsq-bbbbbbbb"]); } #[test] diff --git a/tests/sync_branch.rs b/tests/sync_branch.rs index 3bbdcdd..47aca21 100644 --- a/tests/sync_branch.rs +++ b/tests/sync_branch.rs @@ -269,9 +269,10 @@ fn read_sync_worktree_events(root: &std::path::Path) -> String { .unwrap_or_default() } -/// Seed a `source` repo (main + tsq-sync) and push both branches to a fresh -/// bare `origin`. Returns `(source_path, origin_path)`. -fn seed_source_with_origin( +/// Seed a `source` repo and push only `main` to a fresh bare `origin`. +/// Returns `(source_path, origin_path)`; `tsq-sync` exists locally but not on +/// the remote until a sync publishes it. +fn seed_source_with_origin_main_only( base: &std::path::Path, title: &str, ) -> (std::path::PathBuf, std::path::PathBuf) { @@ -291,6 +292,16 @@ fn seed_source_with_origin( let remote_arg = remote.to_string_lossy().to_string(); git(&source, &["remote", "add", "origin", remote_arg.as_str()]); git(&source, &["push", "origin", "HEAD:main"]); + (source, remote) +} + +/// Seed a `source` repo (main + tsq-sync) and push both branches to a fresh +/// bare `origin`. Returns `(source_path, origin_path)`. +fn seed_source_with_origin( + base: &std::path::Path, + title: &str, +) -> (std::path::PathBuf, std::path::PathBuf) { + let (source, remote) = seed_source_with_origin_main_only(base, title); git(&source, &["push", "origin", "tsq-sync"]); (source, remote) } @@ -419,6 +430,73 @@ fn sync_retries_when_remote_advances_during_push() { } } +/// A second clone can create the remote sync branch after our `ls-remote` +/// check but before the first publish push. That first rejection should reuse +/// the normal fetch/merge/retry flow rather than fail the whole sync. +#[cfg(unix)] +#[test] +fn sync_retries_when_remote_branch_appears_during_publish() { + let repo = make_repo(); + let base = repo.path(); + let (target, remote) = seed_source_with_origin_main_only(base, "Seed task"); + let remote_arg = remote.to_string_lossy().to_string(); + let advancer = clone_from_origin(base, &target, "advancer"); + git( + &advancer, + &["remote", "set-url", "origin", remote_arg.as_str()], + ); + + let target_create = run_cli( + &target, + ["create", "Target task", "--id", "tsq-cccc5555", "--force"], + ); + assert_eq!(target_create.code, 0, "stderr: {}", target_create.stderr); + let advancer_create = run_cli( + &advancer, + ["create", "Advancer task", "--id", "tsq-cccc6666", "--force"], + ); + assert_eq!( + advancer_create.code, 0, + "stderr: {}", + advancer_create.stderr + ); + + let flag = base.join("publish-pre-push-fired"); + let hook = target.join(".git").join("hooks").join("pre-push"); + let script = format!( + "#!/bin/sh\nif [ ! -f '{flag}' ]; then\n touch '{flag}'\n unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE\n git -C '{advancer_wt}' push origin tsq-sync >/dev/null 2>&1 || exit 1\nfi\nexit 0\n", + flag = flag.display(), + advancer_wt = advancer.join(".git").join("tsq-sync").display() + ); + write_executable_hook(&hook, script); + + let sync = run_cli(&target, ["sync", "--json"]); + assert_eq!( + sync.code, 0, + "publish-path rejection should fall through to retry\nstdout:\n{}\nstderr:\n{}", + sync.stdout, sync.stderr + ); + assert!( + flag.exists(), + "pre-push hook should have created remote branch" + ); + let envelope: Value = serde_json::from_str(sync.stdout.trim()).expect("json envelope"); + let data = envelope.get("data").expect("data"); + assert_eq!(data.get("pushed").and_then(Value::as_bool), Some(true)); + assert_eq!( + data.get("has_upstream").and_then(Value::as_bool), + Some(true) + ); + + let target_events = read_sync_worktree_events(&target); + for id in ["tsq-cccc5555", "tsq-cccc6666"] { + assert!( + target_events.contains(id), + "target should contain publish-race merge id {id}:\n{target_events}" + ); + } +} + /// If the remote advances before every push attempt, sync must stop at the /// documented retry cap and surface a structured storage error instead of /// spinning forever or reporting success. @@ -504,22 +582,7 @@ exit 0 fn sync_publishes_sync_branch_when_origin_has_no_upstream() { let repo = make_repo(); let base = repo.path(); - let source = base.join("repo"); - fs::create_dir(&source).expect("repo dir"); - init_git_repo_with_identity(&source, Some("main")); - - let init = run_cli(&source, ["init"]); - assert_eq!(init.code, 0, "stderr: {}", init.stderr); - let create = run_cli(&source, ["create", "Publish task", "--force"]); - assert_eq!(create.code, 0, "stderr: {}", create.stderr); - git(&source, &["add", ".tasque/config.json", ".gitattributes"]); - git(&source, &["commit", "-m", "seed main config"]); - - let remote = create_bare_origin(base); - let remote_arg = remote.to_string_lossy().to_string(); - git(&source, &["remote", "add", "origin", remote_arg.as_str()]); - // Only main is published; the sync branch does not yet exist on origin. - git(&source, &["push", "origin", "HEAD:main"]); + let (source, _remote) = seed_source_with_origin_main_only(base, "Publish task"); let pre = git_output(&source, &["ls-remote", "--heads", "origin", "tsq-sync"]); assert!( @@ -611,6 +674,29 @@ fn sync_conflict_returns_structured_error_and_resumes() { "expected merge left in worktree" ); + // `--no-push` must not stage and commit unresolved conflict markers. + let sb_no_push = run_cli(&clone_b, ["sync", "--no-push", "--json"]); + assert_eq!( + sb_no_push.code, 1, + "expected no-push conflict guard\nstdout:\n{}\nstderr:\n{}", + sb_no_push.stdout, sb_no_push.stderr + ); + let no_push_envelope: Value = + serde_json::from_str(sb_no_push.stdout.trim()).expect("json envelope"); + assert_eq!( + no_push_envelope + .get("error") + .and_then(|e| e.get("code")) + .and_then(Value::as_str), + Some("SYNC_MERGE_CONFLICT") + ); + let merge_head_still_present = + git_output(&worktree, &["rev-parse", "-q", "--verify", "MERGE_HEAD"]); + assert!( + merge_head_still_present.status.success(), + "expected MERGE_HEAD to remain after guarded --no-push" + ); + // Rerun while conflicts remain -> same structured conflict error. let sb_again = run_cli(&clone_b, ["sync", "--json"]); assert_eq!( diff --git a/tests/verb_first_task_commands.rs b/tests/verb_first_task_commands.rs index 1040401..ec402ff 100644 --- a/tests/verb_first_task_commands.rs +++ b/tests/verb_first_task_commands.rs @@ -54,8 +54,6 @@ fn create_from_file_allocates_random_ids_unrelated_to_high_existing_id() { "second id {second} not random canonical" ); assert_ne!(first, second); - assert_ne!(first, "tsq-43"); - assert_ne!(second, "tsq-44"); } #[test] From 2a73bcd108d67e996b5f1e7bb164b2255aef766f Mon Sep 17 00:00:00 2001 From: Aditya Sharma Date: Wed, 8 Jul 2026 11:48:38 -0700 Subject: [PATCH 4/4] test(tui): use generated task ids in contract test --- tui-opentui/tests/contract.test.ts | 32 ++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/tui-opentui/tests/contract.test.ts b/tui-opentui/tests/contract.test.ts index 0000ed1..4e39626 100644 --- a/tui-opentui/tests/contract.test.ts +++ b/tui-opentui/tests/contract.test.ts @@ -18,6 +18,9 @@ const describeContract = bin ? describe : describe.skip; describeContract("tsq CLI contract", () => { let dir = ""; let previousCwd = ""; + let epicId = ""; + let childId = ""; + let blockerId = ""; const run = (args: string[]): string => { const subprocess = Bun.spawnSync([bin as string, ...args], { @@ -37,11 +40,18 @@ describeContract("tsq CLI contract", () => { dir = mkdtempSync(join(tmpdir(), "tsq-contract-")); previousCwd = process.cwd(); run(["init", "--no-wizard"]); - run(["create", "epic demo", "--kind", "epic", "--force"]); // tsq-1 - run(["create", "child", "--parent", "tsq-1", "--force"]); // tsq-1.1 - run(["create", "blocker", "--force"]); // tsq-2 - run(["block", "tsq-1.1", "by", "tsq-2"]); - run(["spec", "tsq-1.1", "--text", "# hello contract"]); + const epic = JSON.parse( + run(["create", "epic demo", "--kind", "epic", "--force", "--json"]), + ); + epicId = epic.data.task.id; + const child = JSON.parse( + run(["create", "child", "--parent", epicId, "--force", "--json"]), + ); + childId = child.data.task.id; + const blocker = JSON.parse(run(["create", "blocker", "--force", "--json"])); + blockerId = blocker.data.task.id; + run(["block", childId, "by", blockerId]); + run(["spec", childId, "--text", "# hello contract"]); // fetchTasks and friends spawn tsq without an explicit cwd, so run the // suite from inside the initialized repo, mirroring the TUI launcher. process.chdir(dir); @@ -66,20 +76,20 @@ describeContract("tsq CLI contract", () => { const snapshot = await fetchTasks(config); expect(snapshot.warning).toBeUndefined(); const ids = snapshot.tasks.map((task) => task.id); - expect(ids).toContain("tsq-1"); - expect(ids).toContain("tsq-1.1"); - expect(ids).toContain("tsq-2"); + expect(ids).toContain(epicId); + expect(ids).toContain(childId); + expect(ids).toContain(blockerId); }); it("fetchDependencyTree returns the root via the deps verb", async () => { - const result = await fetchDependencyTree(bin as string, "tsq-1.1"); + const result = await fetchDependencyTree(bin as string, childId); expect(result.warning).toBeUndefined(); - expect(result.root?.id).toBe("tsq-1.1"); + expect(result.root?.id).toBe(childId); expect(result.root?.children.length).toBeGreaterThan(0); }); it("readSpecLines returns spec content via spec --show", async () => { - const result = await readSpecLines(bin as string, "tsq-1.1"); + const result = await readSpecLines(bin as string, childId); expect(result.warning).toBeUndefined(); expect(result.lines[0]).toBe("# hello contract"); });