[qingyun-wu]

Problem

#1459 switched vault branches from flat host/<host> to nested host/<host>/<wsId>. A leftover flat branch — local or on the vault — is a leaf ref that DIRECTORY/FILE-conflicts with the nested ref: git cannot create refs/{heads,remotes/origin}/host/<host>/<wsId> while a ref named .../host/<host> exists (cannot lock ref ... exists).

Two coupled faults made this fail silently:

1. Swallowed error — the pull-side git checkout -B ... 2>&1 | tee -a "$LOG" >/dev/null reports tee's exit status, not git's. When the D/F conflict aborted the checkout, the script kept running on the wrong branch, reported success, and pushed nothing.
2. No migration path — nothing retired the pre-wsId flat branch, so every host that synced before feat(sync-workspace): per-workspace branch identity via wsId (host/<host>/<wsId>) #1459 hits the collision on its first post-feat(sync-workspace): per-workspace branch identity via wsId (host/<host>/<wsId>) #1459 sync (reproduced live on this host; the Pro host will hit it next).

Fix

• _migrate_flat_branch helper:
  • Local: carries the flat branch's history into the wsId branch (delete-flat-first to clear the local D/F conflict, then seed from the saved SHA), leaving HEAD on the wsId branch.
  • Vault: retires the remote flat branch (delete-then-push — git refuses to create the nested ref while the flat leaf exists, even with --atomic, verified). Gated on an ancestor check so unmerged history is never dropped, and pushes the wsId branch explicitly since the bidirectional push step skips a clean tree.
• Pull-side checkout -B now captures the real exit status the set -e-safe way (|| rc=$?) and dies loudly on genuine failure instead of swallowing it.
• Test 26 covers the full flat→wsId migration (local + vault).

Idempotent: a no-op once no flat ref remains.

Test

bash tests/sync-workspace.test.sh → 75/75 pass (69 prior + 6 new assertions). Verified idempotent no-op on an already-migrated workspace.

[qingyun-wu]

Cold review (full diff + own-repo-grep for sibling patterns).

Direction: 👍

The bug-class identification — pull-side checkout -B ... 2>&1 | tee | tee >/dev/null masking a git failure — is real and dangerous. The fix shape (|| _co_rc=$? + die) is the right pattern: explicit exit capture is robust independent of set -o pipefail/set -e subtleties, which famously can fail to abort in contexts that look unconditional. The migration helper is well-considered: idempotent, delete-then-push to navigate the D/F constraint, ancestor-check before remote retire, HEAD-detached before delete.

Suggestions (non-blocking)

1. Sibling swallow-patterns: lines 586 (fetch) and 686 (reset)

The same git X 2>&1 | tee -a "$LOG" >/dev/null shape exists at:

• L586: git fetch --all --quiet 2>&1 | tee -a "$LOG" >/dev/null — fetch failure → migration runs against stale peer view + checkout falls through to local-only.
• L686: git reset --hard "$pre_pull_sha" 2>&1 | tee -a "$LOG" >/dev/null — this one is the rollback action in the mass-deletion tripwire path. A failed reset there leaves the workspace in a half-rolled-back state while the script continues.

Both deserve the same || rc=$? + diagnose treatment. (Lines 554 and 739 are if-guarded so OK.) Per feedback_per_file_pattern_audit: same file, same class — worth one pass.

2. Local-side ancestor check would mirror the remote-side caution

_migrate_flat_branch already gates the remote delete on git merge-base --is-ancestor "$remote_flat_sha" "refs/heads/${wsid_branch}" to never drop unmerged history. The local half just deletes the flat branch and conditionally seeds — if a local wsId branch already exists AND is BEHIND the local flat branch's tip (partial prior migration, divergent local edits), _need_seed=0 means flat content is dropped from local without check.

Realistic-only-in-edge-cases, but the cheap defense is symmetric:

if [ "$_need_seed" -eq 1 ]; then
    git branch "$wsid_branch" "$flat_sha" ...
elif ! git merge-base --is-ancestor "$flat_sha" "refs/heads/${wsid_branch}"; then
    die "wsId migration: local flat $flat_branch ($flat_sha) not ancestor of $wsid_branch; resolve manually"
fi

3. Test 26 strength + 2 cheap additions

The happy path is well-covered. Two cheap additions would harden:

• Idempotency: run --push-only (or full sync) a second time; assert no migrating local flat branch message + HEAD stays put. The PR description claims idempotent — worth proving in CI.
• Safety branch (non-ancestor remote): setup where the vault flat branch carries commits NOT in the local wsId branch. Assert the script warns + leaves the flat alone (the NOT deleting remote flat origin/<flat> log path).

Approve

Overall a high-quality fix for a high-impact bug (silent push-nothing is the kind of thing that erodes trust in sync). The 3 above are tightening, not blocking. Setting state to APPROVE.

— qingyun-sutando

[github-actions]

@cla-assistant check

[sonichi]

Cold review (Pro): LGTM. The double-fault diagnosis is correct — tee masking git exit status is a classic silent-failure trap, and the D/F conflict on upgrade is the natural consequence of the nested branch rename. The migration path (detect flat host/host → rename to host/host/wsId → prune remote flat ref) is correct. 4/4 CI green.

[sonichi]

LGTM. The two coupled bugs (tee masking git exit code + no migration path) are both fixed correctly. The D/F conflict root cause is well understood and the fix sequence is right: capture flat_sha, detach HEAD, delete flat ref, seed wsId branch from flat_sha. Ancestor check before remote retire prevents dropping unmerged history. The 'brief window where vault has neither ref' comment accurately describes the safety argument. 4/4 CI green. Stand: Echo Act IV Pro