fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

CB-587: the CB-578 stage C snapshot loses --skip-worktree flags, so it commits never-commit files and misreports what the worker changed #68

New Issue
Closed
opened 2026-08-15 15:21:57 +02:00 by ltms · 1 comment
ltms commented 2026-08-15 15:21:57 +02:00
Owner
Copy Link
Copy Source

Found by dogfooding stage C on the live daemon right after it shipped (merge a3842c8). Not caught by its tests.

What happens

GitWorktrees.snapshot stages into a fresh temporary index:

GIT_INDEX_FILE=<temp> git -C worktree add -A

--skip-worktree is an index flag. A brand-new temp index starts empty, so it carries none of those flags from the worker's real index. git add -A therefore stages every skip-worktree file using its local on-disk content, while git status --porcelain — which reads the real index — correctly hides them.

The two views disagree, and the snapshot follows the wrong one.

Measured

A probe worktree where the only changes I made were one modified file and one new file:

$ git -C <worktree> status --porcelain
 M README.md
?? STAGEC-PROBE.txt

The snapshot the daemon then wrote contains 15 changed files, including:

.mcp.json        | 11 +---
opencode.json    | 35 +----------------------------------
wiki             |  1 -

.mcp.json is the file CLAUDE.md names as never-commit. wiki is a submodule with its own remote.

Two separate harms

1. The snapshot misreports the work — this is the one that bites first. The whole point of stage C is that a lead can recover a dead worker's changes. A recovered snapshot now carries spurious edits to .mcp.json, opencode.json and the wiki submodule pointer that the worker never made. Anyone diffing refs/wip/<branch> against the base to see "what did this worker do?" gets a wrong answer, and applying it would revert three files.

2. It commits whatever those local files hold. In the case measured the leak was benign — the worktree's .mcp.json is a 23-byte stub against the 231-byte tracked version, so nothing sensitive reached the object. That is luck, not design. .mcp.json is marked never-commit precisely because it is local config, and the snapshot path ignores that mark.

Note this is a different category from the gitignore protection, which does work: add -A respects .gitignore, and I confirmed a gitignored file stays out of the snapshot. --skip-worktree is a tracked file the index is told to ignore, and nothing in the current code honours it.

Why the tests missed it

GitWorktreesTest builds a clean throwaway repo and never sets a --skip-worktree bit on anything, so the temp index and the real index agree there. The defect only appears in a repo that uses the flag — which this one does, deliberately, for exactly the files that must not be committed.

Scope

Make the snapshot honour the real index's flags. The straightforward fix is to seed the temporary index from the worktree's real one rather than starting empty:

cp <worktree>/.git/index <temp>     # or: git read-tree into the temp index
GIT_INDEX_FILE=<temp> git add -A

Copying preserves every --skip-worktree and --assume-unchanged bit, so add -A then skips exactly the files git status skips, and the snapshot matches what the worker actually changed. It also keeps the existing guarantee that the worker's real index is never written, since all staging still goes to the copy.

Resolve the worktree's index path with git rev-parse --git-path index rather than assuming .git/index — in a linked worktree .git is a file, not a directory, and the real index lives under the main repo's worktrees/<name>/ directory.

Whatever approach lands, a snapshot must never contain a file that git status --porcelain in that same worktree does not report.

Acceptance criteria

  1. A file marked --skip-worktree in the worktree's index is absent from the snapshot's diff against its parent, even when its on-disk content differs.
  2. A snapshot's diff against its parent lists exactly the paths git status --porcelain reports for that worktree — no more.
  3. The worker's real index, working tree and HEAD are still never modified (existing criterion, must not regress).
  4. Gitignored files are still excluded (existing criterion, must not regress).
  5. A test sets a real --skip-worktree bit and asserts 1 and 2. The current tests pass without one, which is why this shipped.
  6. A worktree whose index cannot be read fails the snapshot the way any other failure does — WARN, worktree still preserved, pane still stopped, nothing propagates out of release().

Cleanup

One existing ref carries the bad shape: refs/wip/worker/cb554-b585b5-3, written by hand with the same recipe. Its work is superseded by the merged CB-554 (c29c3f0) and its .mcp.json blob is the harmless stub, so it can be dropped once this is fixed. No other snapshot refs exist.

Found by dogfooding stage C on the live daemon right after it shipped (merge `a3842c8`). Not caught by its tests. ## What happens `GitWorktrees.snapshot` stages into a **fresh temporary index**: ``` GIT_INDEX_FILE=<temp> git -C worktree add -A ``` `--skip-worktree` is an **index flag**. A brand-new temp index starts empty, so it carries none of those flags from the worker's real index. `git add -A` therefore stages every skip-worktree file using its local on-disk content, while `git status --porcelain` — which reads the real index — correctly hides them. The two views disagree, and the snapshot follows the wrong one. ## Measured A probe worktree where the only changes I made were one modified file and one new file: ``` $ git -C <worktree> status --porcelain M README.md ?? STAGEC-PROBE.txt ``` The snapshot the daemon then wrote contains **15 changed files**, including: ``` .mcp.json | 11 +--- opencode.json | 35 +---------------------------------- wiki | 1 - ``` `.mcp.json` is the file `CLAUDE.md` names as never-commit. `wiki` is a submodule with its own remote. ## Two separate harms **1. The snapshot misreports the work — this is the one that bites first.** The whole point of stage C is that a lead can recover a dead worker's changes. A recovered snapshot now carries spurious edits to `.mcp.json`, `opencode.json` and the `wiki` submodule pointer that the worker never made. Anyone diffing `refs/wip/<branch>` against the base to see "what did this worker do?" gets a wrong answer, and applying it would revert three files. **2. It commits whatever those local files hold.** In the case measured the leak was benign — the worktree's `.mcp.json` is a 23-byte stub against the 231-byte tracked version, so nothing sensitive reached the object. That is luck, not design. `.mcp.json` is marked never-commit precisely because it is local config, and the snapshot path ignores that mark. Note this is a **different category** from the gitignore protection, which does work: `add -A` respects `.gitignore`, and I confirmed a gitignored file stays out of the snapshot. `--skip-worktree` is a tracked file the index is told to ignore, and nothing in the current code honours it. ## Why the tests missed it `GitWorktreesTest` builds a clean throwaway repo and never sets a `--skip-worktree` bit on anything, so the temp index and the real index agree there. The defect only appears in a repo that uses the flag — which this one does, deliberately, for exactly the files that must not be committed. ## Scope Make the snapshot honour the real index's flags. The straightforward fix is to **seed the temporary index from the worktree's real one** rather than starting empty: ``` cp <worktree>/.git/index <temp> # or: git read-tree into the temp index GIT_INDEX_FILE=<temp> git add -A ``` Copying preserves every `--skip-worktree` and `--assume-unchanged` bit, so `add -A` then skips exactly the files `git status` skips, and the snapshot matches what the worker actually changed. It also keeps the existing guarantee that the worker's real index is never written, since all staging still goes to the copy. Resolve the worktree's index path with `git rev-parse --git-path index` rather than assuming `.git/index` — in a linked worktree `.git` is a file, not a directory, and the real index lives under the main repo's `worktrees/<name>/` directory. Whatever approach lands, a snapshot must never contain a file that `git status --porcelain` in that same worktree does not report. ## Acceptance criteria 1. A file marked `--skip-worktree` in the worktree's index is **absent** from the snapshot's diff against its parent, even when its on-disk content differs. 2. A snapshot's diff against its parent lists exactly the paths `git status --porcelain` reports for that worktree — no more. 3. The worker's real index, working tree and HEAD are still never modified (existing criterion, must not regress). 4. Gitignored files are still excluded (existing criterion, must not regress). 5. A test sets a real `--skip-worktree` bit and asserts 1 and 2. The current tests pass without one, which is why this shipped. 6. A worktree whose index cannot be read fails the snapshot the way any other failure does — WARN, worktree still preserved, pane still stopped, nothing propagates out of `release()`. ## Cleanup One existing ref carries the bad shape: `refs/wip/worker/cb554-b585b5-3`, written by hand with the same recipe. Its work is superseded by the merged CB-554 (`c29c3f0`) and its `.mcp.json` blob is the harmless stub, so it can be dropped once this is fixed. No other snapshot refs exist.
ltms added the ready-to-delegate label 2026-08-15 15:22:03 +02:00
ltms commented 2026-08-15 16:01:27 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main at ef186a1. My own build on the merged tree: 778 tests, 0 failures, BUILD SUCCESS, exit 0 (unpiped). CI green on the PR head (run 1194).

The fix resolves the real index with git -C <worktree> rev-parse --git-path index and copies it into the temp index before add -A, so the skip-worktree and assume-unchanged bits travel with it. That was the part most likely to be got wrong — a linked worktree's .git is a file, not a directory, and its real index lives under the main repo's worktrees/<name>/.

Both new tests are the right shape: one sets a real --skip-worktree bit via git update-index and asserts the snapshot's diff matches git status --porcelain exactly; the other breaks the worktree's .git pointer and asserts snapshot() throws rather than silently falling back to an empty index. The absence of the first was precisely why the defect shipped.

On the flagged open question — whether to also cover "rev-parse succeeds but the resolved file does not exist" — I agree with the judgment. That is a corrupted-repo case, it fails safe through the same WorktreeException path, and the existing test covers criterion 6's intent. Not worth a third test.

The stale refs/wip/worker/cb554-b585b5-3 ref is correctly left alone; it is repo state, not code. I will drop it separately now this is fixed.

Merged to `main` at `ef186a1`. My own build on the merged tree: **778 tests, 0 failures, BUILD SUCCESS, exit 0** (unpiped). CI green on the PR head (run 1194). The fix resolves the real index with `git -C <worktree> rev-parse --git-path index` and copies it into the temp index before `add -A`, so the skip-worktree and assume-unchanged bits travel with it. That was the part most likely to be got wrong — a linked worktree's `.git` is a file, not a directory, and its real index lives under the main repo's `worktrees/<name>/`. Both new tests are the right shape: one sets a **real** `--skip-worktree` bit via `git update-index` and asserts the snapshot's diff matches `git status --porcelain` exactly; the other breaks the worktree's `.git` pointer and asserts `snapshot()` throws rather than silently falling back to an empty index. The absence of the first was precisely why the defect shipped. On the flagged open question — whether to also cover "`rev-parse` succeeds but the resolved file does not exist" — I agree with the judgment. That is a corrupted-repo case, it fails safe through the same `WorktreeException` path, and the existing test covers criterion 6's intent. Not worth a third test. The stale `refs/wip/worker/cb554-b585b5-3` ref is correctly left alone; it is repo state, not code. I will drop it separately now this is fixed.
ltms closed this issue 2026-08-15 16:01:50 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label ready-to-delegate
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#68
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?