Skip to content

safe_rollback discards commits a human made while the run was down #520

Description

@pbean

Summary

_finish_inflight's resume-restart arm rolls an in-place task back with verify.safe_rollback, whose core is git reset --hard <task.baseline_commit>. Any commit a human made while the run was down is after that baseline, so the reset moves the branch back over it. The work is recoverable from the reflog, but nothing warns and nothing preserves it.

policy.toml is already special-cased against exactly this — safe_rollback reads it before the reset and writes it straight back, with the docstring naming "a change committed after baseline" as the case. That rescue is file-specific, so every other tracked file a human touched while the run was down is still exposed.

Evidence

Found while fixing a P1 on #502 (codex). The deferred-work ledger is the case that surfaced it: with a tracked ledger, a gate: entry committed while the run was down is silently reverted by the restart-arm rollback. #502 fixes the gate half by asking the gate before the arm mutates anything (engine.py, _finish_inflight restart arm), so the refusal no longer reads a rewound file — but it deliberately does not touch the rollback, and verify.py is not in that PR's diff.

The general shape is unchanged by #502:

  1. run picks story, captures baseline_commit, dies mid-attempt
  2. human commits something to the repo while the run is down (ledger edit, a fix, a doc change)
  3. bmad-loop resume → restart arm → safe_rollbackgit reset --hard <baseline>
  4. the human's commit is no longer on the branch

keep=(".bmad-loop",) does not help: keep guards untracked deletion, not tracked reverts.

Why this is not obviously wrong

Resetting to baseline is the point of the rollback — it is how a failed attempt's debris is removed, and it cannot distinguish "attempt debris" from "a human's commit" by content alone. _preserve_attempt_commits parks the attempt's commits on a recovery ref, but a post-baseline human commit is not the attempt's.

So this is a design question, not a one-line fix. Options, roughly in increasing cost:

  • Detect and refuse: if HEAD is not baseline_commit and the intervening commits are not this attempt's, pause for manual recovery instead of resetting (the rollback_on_failure = false behavior) rather than silently discarding.
  • Preserve first: extend the attempt-preserve/* ref family to park whatever is above baseline before the reset, so the reset is always recoverable by ref rather than by reflog.
  • Widen the policy.toml rescue into a general "restore these tracked paths across the reset" list — cheapest, but only moves the boundary and needs someone to keep the list right.

My preference is the first: a rollback that would discard commits it cannot attribute to its own attempt should stop, because the operator is the only one who can tell what those commits are.

Not in scope for #502

#502 adds the gate: field and its enforcement. This is verify.safe_rollback / RecoveryFlow behavior that predates it and bites independently of gates.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions