Skip to content

The board carry's ownership proof is a TOCTOU: an edit between the check and git add still enters the commit #686

Description

@pbean

Raised by CodeRabbit on #685 (the #618 carry-ownership work). Deliberately deferred there rather than fixed, on the same call that produced #681.

What happens

Engine._carry_board_advance proves the main checkout's board is this pass's before committing it:

  1. _board_carry_holds_only_this_advance hashes the working tree and the index against sprintstatus.advanced_bytes(HEAD, ...).
  2. verify.commit_paths then runs git add -- <board>, which stages whatever the working tree holds at that moment.

Nothing pins the bytes that passed step 1. A write landing between the two is committed under chore(sprint-status): carry <story> to <target> with the tree left clean behind it — the same silent shape #618 closed for the wide window.

CodeRabbit's words:

commit_paths stages the live working-tree path with git add after the ownership check. An operator edit in that interval can enter the carry commit. Stage the verified blob or use a final staged-content verification before git commit.

It rated this Major / Heavy lift.

Why P3 rather than P2

The interval is a few Python statements plus at most one git ls-files subprocess — single-digit milliseconds — and to hit it an operator has to be hand-editing the board in the main checkout at that instant, on the replay leg where no merge pre-flight ran. #618's window was "the host was down while you edited"; this one is not reachable by ordinary use. It is still a correctness hole rather than robustness, which is why it is filed rather than closed.

Not closed by #618

Verified against pbean/merge-preflight-taxonomy: the pre-advance row check and the post-advance working-tree + index proof both run strictly before commit_paths. Neither narrows the interval — they only decide whether commit_paths is called at all.

Possible directions (needs a maintainer call)

  • Stage the verified blob instead of the live path. git hash-object -w + git update-index --cacheinfo puts the bytes that passed the check into the index, so the commit cannot take anything else. This changes what commit_paths means for its other callers (confirm, decisions, the ledger carries), so it probably wants a carry-specific write path rather than a change to the shared helper.
  • Re-verify the index after git add and before git commit, aborting when it no longer holds the proved content. Cheaper and local, but it leaves the same class of window between that check and the commit — smaller, not closed.
  • Accept it and say so. Document in _board_carry_holds_only_this_advance that the proof is load-bearing only against a writer that stopped before the carry began, and is advisory against one still typing. That is the honest reading of what the probe can promise, and it costs nothing.

Metadata

Metadata

Assignees

No one assigned

    Labels

    P3Robustness, enhancement, tests, or docs worth schedulingarea:engineOrchestrator engine and run lifecyclebugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions