Skip to content

Add guarded conflict recovery plans#72

Open
cosentinode wants to merge 12 commits into
developfrom
issue-55-recovery-policy
Open

Add guarded conflict recovery plans#72
cosentinode wants to merge 12 commits into
developfrom
issue-55-recovery-policy

Conversation

@cosentinode

Copy link
Copy Markdown
Owner

Summary

  • add typed merge/rebase recovery requests, parser commands, high-risk plans, and fixed audit names
  • centralize conflict-mode allowlisting and preflight every prompt-sequence step before execution
  • snapshot exact recovery state at preview and revalidate it immediately before typed Git recovery
  • preserve status, diff, fetch, branch listing, and staging workflows during conflict resolution

Verification

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

Fixes #55

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh-context review found two blocking concerns:

  1. High: queued operations can bypass conflict mode after preview (crates/bitbygit-tui/src/lib.rs:1649, crates/bitbygit-tui/src/lib.rs:2833). ensure_conflict_mode_allowed runs while the plan is prepared, but PlanExecutor::execute runs that queued plan after confirmation without checking the policy again. If a merge/rebase starts between preview and confirmation, an already queued unrelated operation can still execute; for example, push remains valid to Git during a conflicted merge and will produce its remote side effect. The first prepared step of a prompt sequence has the same gap, even though deferred steps are replanned. Revalidate conflict mode immediately before executing every prepared operation (including the first sequence step), and add a test that queues while clean, introduces a conflict, then confirms and verifies no side effect.

  2. High: the “exact” recovery snapshot omits the state that controls recovery (crates/bitbygit-git/src/lib.rs:707-718). RecoveryState records the operation label, HEAD, porcelain status, index entries, and worktree diff, but not Git control metadata such as MERGE_HEAD, MERGE_MSG, MERGE_MODE, ORIG_HEAD, or the active rebase-merge/rebase-apply contents. Those files can change after preview without changing any compared field, while changing the merge parents/message, abort destination, or rebase todo/current commit. recover_exact will therefore accept materially different recovery state and run the high-risk action. Snapshot/revalidate the relevant operation metadata and add mutation tests for merge and rebase control state.

Verification: cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, and cargo test --locked --workspace all pass locally; CI is also green.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed both blocking review concerns in commit 0fdb1e4:

  • Revalidate conflict-mode policy at PlanExecutor::execute immediately before any prepared operation runs. This covers standalone queued confirmations, the first already-prepared prompt-sequence step, and later replanned steps. The regression queues a sequence while clean, starts a conflicting merge, confirms, and verifies the first-step push does not change the remote or start an audit.
  • Extend RecoveryState with deterministic snapshots of merge/rebase control metadata, including merge control files, ORIG_HEAD, REBASE_HEAD, and recursive contents of both rebase backend directories. Merge-message and rebase-todo mutation regressions verify recovery remains blocked.

Verification passed:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

@cosentinode

Copy link
Copy Markdown
Owner Author

High: exact rebase recovery still ignores refs that abort can overwrite (crates/bitbygit-git/src/lib.rs:23-33, crates/bitbygit-git/src/lib.rs:718-730). RecoveryState snapshots HEAD/status/index/worktree and selected control files, but not the original branch ref named by rebase-merge/head-name (or rebase temporary refs such as refs/rewritten). During a conflicted rebase I moved refs/heads/topic with git update-ref; HEAD plus the captured status/index/worktree inputs remained byte-for-byte unchanged, and none of the listed metadata paths changes. recover_exact(Rebase, Abort, ...) therefore accepts the stale preview, after which git rebase --abort resets topic to the saved original OID and overwrites the newer ref. This violates the requirement that repository changes after preview block execution and can discard concurrent branch state. Snapshot and revalidate the operation-owned/referenced refs (at least the branch ref named by the active rebase and refs/rewritten for rebase-merges), and add an abort regression that mutates the original branch ref after preview and verifies both that recovery is blocked and that the newer ref remains intact.

Verification: cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, and cargo test --locked --workspace all pass at 0fdb1e4; CI is green.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed the exact rebase recovery ref concern from #72 (comment) in commit c8f0f9a.

  • RecoveryState now snapshots the original branch ref named by either active rebase backend’s head-name metadata.
  • It also snapshots the full refs/rewritten namespace during rebases, including ref names, object IDs, and symbolic targets.
  • Added an abort regression that moves refs/heads/topic after preview and verifies exact recovery is blocked while the newer branch ref remains intact.
  • Added a --rebase-merges regression proving changes under refs/rewritten also block exact recovery.

Verification passed:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

@cosentinode

Copy link
Copy Markdown
Owner Author

High: deferred sequence requests are revalidated only after the first side effect (crates/bitbygit-tui/src/lib.rs:2849-2860, crates/bitbygit-tui/src/lib.rs:3498-3518). At confirmation, PlanExecutor::execute validates only the prepared first plans request, then PromptSequenceExecutor executes it before replanning any remaining request. For example, queue fetch && push while clean, start a conflicting merge before confirmation, and confirm: fetch is conflict-allowed, so it runs and can update remote-tracking refs and write audit entries; only afterward is the deferred push planned and rejected for the active merge. This violates the issue requirement that a prompt sequence containing a blocked operation fail before any side effect. The new queued-sequence regression uses push as step 1, so it does not cover an allowed, side-effecting first step followed by a newly blocked deferred step. Revalidate conflict policy for the complete queued request list immediately before executing step 1, and add a regression for this ordering that verifies neither refs nor audit state change.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed the deferred sequence policy bypass from #72 (comment) in commit 168f0dc.

  • PromptSequenceExecutor now revalidates the complete queued request list, including deferred requests, immediately before executing the prepared first step.
  • Added a fetch && push regression that queues while clean, advances the remote, starts a conflicting merge, then confirms and verifies the remote-tracking ref and audit state remain unchanged.

Verification passed:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

@cosentinode

Copy link
Copy Markdown
Owner Author

High: exact rebase recovery can overwrite ignored worktree data changed after preview (crates/bitbygit-git/src/lib.rs:718-732). RecoveryState captures default porcelain status, index entries, and a tracked worktree diff, but default status omits ignored paths and git diff does not hash their contents. I reproduced this at 168f0dc with a two-commit rebase stopped on commit 1: commit 2 in the original branch tracks target/victim.bin, while /target/ is ignored in the stopped state. After preview, creating or changing the ignored target/victim.bin leaves HEAD, status, ls-files --stage, worktree diff, recovery metadata, and recovery refs unchanged, so recover_exact accepts the stale snapshot; git rebase --abort then silently replaces that file with the original branch blob. This violates the acceptance criterion that repository changes after preview block execution and causes loss of post-preview data. Snapshot/revalidate ignored worktree paths/content that the recovery destination can overwrite, or otherwise fail closed on destination collisions, and add a rebase-abort regression proving the changed file is preserved.

Verification: cargo test --locked --workspace passes locally; git diff --check passes; Rust CI and GitGuardian are green.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed concern #4973048113 in commit 025b34f (fix: protect ignored recovery data).

RecoveryState now snapshots ignored untracked worktree paths, including file contents and filesystem type, so recover_exact detects ignored data created, removed, or changed after preview and fails closed before Git recovery can overwrite it. Added exact_rebase_abort_preserves_ignored_file_changed_after_preview, reproducing a two-commit rebase where the original branch tracks target/victim.bin while it is ignored at the stopped step; changing the file after preview blocks abort and preserves its changed contents.

Verification passed:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace
  • Focused regression: cargo test --locked -p bitbygit-git exact_rebase_abort_preserves_ignored_file_changed_after_preview

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh-context review of current head 025b34f found three blocking concerns:

  1. High: exact rebase recovery omits refs scheduled by --update-refs (crates/bitbygit-git/src/lib.rs:772-795). The recursive metadata snapshot captures rebase-merge/update-refs as bytes, but recovery_refs only snapshots the original head-name ref and refs/rewritten; it does not snapshot the current values of the additional branch refs named in update-refs. I reproduced this at the current head with a side branch scheduled by git rebase --update-refs --exec false: after the preview-equivalent snapshot, moving the side branch left HEAD, porcelain status, index, worktree diff, rebase metadata, the topic ref, and refs/rewritten unchanged. Recovery therefore accepts the stale state; git rebase --continue advances topic and clears the rebase, merely printing that the concurrently moved side ref could not be updated. This violates the requirement that post-preview repository changes block execution and permits partial recovery side effects. Snapshot/revalidate every ref named by the active rebase metadata (including update-refs) and add a regression that moves such a ref after preview and verifies continue is blocked before HEAD/operation state changes.

  2. High: recovery snapshots read every ignored file fully into RAM (crates/bitbygit-git/src/lib.rs:736-756, crates/bitbygit-git/src/lib.rs:1276-1281). ignored_worktree enumerates the repository-wide ignored set and stores each file as File(Vec<u8>) in the queued plan. At confirmation, recover_exact builds a second full snapshot while the first is still resident. A normal ignored build tree can therefore make recovery scan and retain gigabytes twice; a very large sparse ignored file can trigger a huge logical-size allocation and terminate the TUI despite consuming little disk. Scope protection to paths the selected recovery can overwrite and/or use bounded streaming fingerprints rather than retaining all ignored contents, with a large/sparse-file regression.

  3. Medium: exact recovery still does not detect content changes to non-ignored untracked files (crates/bitbygit-git/src/lib.rs:718-732). Porcelain status records an existing untracked path but not its bytes, ls-files --stage and git diff exclude it, and the new content snapshot explicitly selects only --ignored paths. Editing an already-present non-ignored untracked file after preview therefore leaves RecoveryState equal and recovery proceeds, contrary to the acceptance criterion that repository changes after preview block execution. Cover non-ignored untracked content with the same bounded strategy and add a mutation regression.

Verification at 025b34f: cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, cargo test --locked --workspace, and git diff --check pass. CI and GitGuardian are green.

@cosentinode

Copy link
Copy Markdown
Owner Author

Implemented all three blocking fixes in 691dd2f (fix: bound exact recovery snapshots).

  • Active rebase recovery now parses both rebase backends' update-refs metadata and snapshots every named ref, deduplicated with the original topic ref and refs/rewritten. The regression moves a scheduled side branch after preview and verifies rebase --continue is blocked before HEAD or operation state changes.
  • Ignored regular files are now represented by bounded streaming Git object fingerprints plus file size instead of retained file contents. The regression snapshots and revalidates a 256 MiB sparse ignored file while asserting only the bounded fingerprint is retained.
  • Non-ignored untracked regular files now use the same fingerprint snapshot, so content-only edits to an already-present untracked path block exact recovery.
  • Symlink targets and special-file identity remain fail-closed in the untracked/ignored snapshot, while path enumeration is deterministic and covers both ignored and non-ignored untracked files.

Exact verification completed successfully:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace (all workspace tests pass; bitbygit-git: 72 passed)
  • git diff --check

Post-push status is clean and HEAD matches origin/issue-55-recovery-policy at 691dd2feae240cc34cb5f781fa0d63f4452c1839.

@cosentinode

Copy link
Copy Markdown
Owner Author

High: confirmation-time recovery revalidation is a non-coherent snapshot and can miss concurrent edits (crates/bitbygit-git/src/lib.rs:272-287, crates/bitbygit-git/src/lib.rs:718-732). recovery_state() reads HEAD/status/index/diff first, then may spend substantial time walking and hashing the untracked/ignored tree before reading metadata and refs. If an editor or another Git process changes a tracked file after lines 727-729 have captured the old diff but before the slow scan completes, the assembled state still equals the preview even though the live worktree no longer does; recover_exact then performs a separate recover call and an abort can overwrite that post-preview edit. Ref/worktree changes in the additional compare-to-act window have the same problem. Revalidation needs a coherent/fenced strategy, or at minimum a final recheck of every overwrite-sensitive input after the slow work and atomic expected-value updates for refs, with a synchronized concurrency regression.

High: snapshots remain unbounded in memory and work despite the new sparse-file test (crates/bitbygit-git/src/lib.rs:727-730, crates/bitbygit-git/src/lib.rs:736-803, crates/bitbygit-git/src/lib.rs:1593-1602). Tracked changes are still materialized as the complete git diff --binary output in Vec<u8>; at confirmation both the queued and newly built copies coexist, so a large incompressible tracked binary can consume multiple times its size and terminate the TUI. For untracked/ignored data, hash-object bounds retained bytes but still reads every byte and launches one Git process per file, repository-wide, at preview and again at confirmation. A normal large target/ or node_modules/ tree can therefore require tens of thousands of subprocesses and many GB of I/O. The 256 MiB sparse regression only checks retained OID length and itself hashes all logical bytes twice; it does not establish a resource bound. Scope the scan to paths recovery can overwrite and use a batched, explicitly bounded/fail-closed representation; add large tracked-binary and high-file-count coverage.

Medium: untracked/ignored filesystem changes are not captured completely (crates/bitbygit-git/src/lib.rs:782-815, crates/bitbygit-git/src/lib.rs:1605-1617). A regular file is represented only by size and content OID, with no mode/permissions, while every non-file/non-symlink type collapses to Other. Changing an ignored recovery-destination file from non-executable to executable after preview leaves the full RecoveryState equal, and rebase abort can then replace it with the original tracked mode, silently losing the post-preview change. Replacing one special type with another is likewise invisible. Include recovery-relevant filesystem metadata/type identity (or fail closed on such entries) and add an ignored-file chmod regression proving abort is blocked and the new mode survives.

Verification at current head 691dd2f: cargo test --locked --workspace passes locally (207 tests), git diff --check passes, and both GitHub checks are green.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed the recovery snapshot review in commit 433d8c0.

Design changes:

  • Replaced repository-wide status/binary-diff/untracked snapshots with SHA-256 fingerprints of only paths changed in the active operation or its saved recovery destination. Rebase path derivation uses the active backend's snapshotted orig-head; unrelated ignored trees are not traversed.
  • Added fail-closed limits for retained state (8 MiB), Git output (4 MiB), file content read (64 MiB), relevant files (10,000), relevant path bytes (256 KiB), recovery metadata (4 MiB / 4,096 entries), and Git subprocesses (12). Output is bounded while read, metadata traversal is iterative and globally bounded, and hashing is in-process rather than one subprocess per file.
  • Confirmation now performs two complete matching captures before invoking Git, so an edit synchronized into the former slow-scan window is caught by the final capture. Recovery metadata and operation-owned/referenced refs remain guarded.
  • Regular-file fingerprints include size, SHA-256, mode, and filesystem identity. Unsupported types fail closed; Unix opens use O_NOFOLLOW | O_NONBLOCK plus handle/path identity revalidation to avoid symlink/FIFO races.

Regression coverage includes the synchronized concurrent edit, an actual 64 MiB tracked binary bound, a 10,001-file ignored tree, relevant file-count and metadata-entry limits, ignored content and chmod changes, file-to-directory replacement, incorrect top-level ORIG_HEAD, and FIFO/symlink replacement races.

Verification passed at 433d8c0:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace (214 tests)
  • git diff --check

Git does not expose a transaction spanning arbitrary external worktree edits and git merge/rebase --continue|--abort|--skip; this removes the reported non-coherent slow-scan window with a final bounded recheck, while an external process racing after that final check remains a narrow platform-independent limitation.

@cosentinode

Copy link
Copy Markdown
Owner Author

High: a relevant directory is snapshotted without its contents, so abort can delete post-preview data (crates/bitbygit-git/src/lib.rs:843-920). When a recovery-relevant path is a directory, snapshot_recovery_worktree records only RecoveryWorktreeValue::Directory; it neither fingerprints descendants nor fails closed. I reproduced the resulting Git behavior with a rebase stopped before a commit that replaces an ignored victim/ directory with a tracked regular file victim: create victim/data before the preview, change it after the preview, then abort. Both guard captures still see only victim: Directory, so recover_exact accepts the stale preview; git rebase --abort removes the directory and restores the tracked file, deleting the changed victim/data. This violates the post-preview revalidation requirement and causes data loss. Snapshot directory descendants with the same explicit bounds (including metadata/content), or fail closed whenever recovery can replace a nonempty directory, and add a regression for a destination file colliding with an ignored directory whose child changes after preview.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed #72 (comment) in commit 0595603 (fix: protect recovery directory contents).

  • Recovery-relevant directories now expand into recursively fingerprinted descendant entries instead of recording only the top-level directory node. Descendant regular-file contents and metadata use the existing bounded fingerprint path, and directory mode/identity are retained.
  • Recursive expansion remains fail-closed under the existing relevant-entry, path-byte, retained-memory, and file-read limits.
  • Added exact_rebase_abort_preserves_ignored_directory_child_changed_after_preview, covering a recovery destination regular file colliding with an ignored directory. Changing the directory child after preview now blocks abort, leaves the rebase active, and preserves the changed child.

Verification passed:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace (all workspace tests pass; bitbygit-git: 80 passed)
  • cargo test --locked -p bitbygit-git exact_rebase_abort_preserves_ignored_directory_child_changed_after_preview
  • git diff --check

@cosentinode

Copy link
Copy Markdown
Owner Author

High: final recovery validation still has an unguarded compare-to-act window that can lose concurrent state (crates/bitbygit-git/src/lib.rs:283-310). recover_exact takes a second matching snapshot, but then calls recover, which runs another full status()/operation check before it finally spawns git rebase --abort (or the requested recovery command). Nothing locks the guarded worktree/index/refs or supplies expected values to that command. If another process moves the original rebase branch after the second snapshot returns, status() still accepts the active rebase and git rebase --abort can reset that branch to orig-head, discarding the concurrent ref update; an edit to an overwrite-sensitive worktree path in the same interval can likewise be replaced. The synchronized regression only edits during the first confirmation-time capture and proves that the second capture catches that earlier window; it does not cover an edit after the final capture. Close or explicitly fence the final snapshot-to-command interval (including operation-owned refs), and add a synchronized regression that mutates the original branch after final validation but before Git recovery and verifies the newer ref is preserved and recovery does not proceed.

Verification at 0595603: cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, cargo test --locked --workspace, and git diff --check pass; both GitHub checks are green.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed #72 (comment) in commit 00e13d5 (fix: fail closed at recovery execution).

Design:

  • Re-evaluated the compare-to-act design and removed recovery execution from the production path. Git does not provide a transaction spanning operation-owned refs, recovery metadata, the index, and arbitrary worktree paths, and its lock files cannot be pre-held and transferred to git merge/rebase --continue|--abort|--skip. An application/advisory lock would not fence the concurrent Git/editor process in the finding.
  • recover_exact retains its preview/final validation diagnostics but now fails closed at the act boundary without spawning Git. The unguarded low-level recovery executor is test-only, so production callers cannot bypass the guarded API.
  • TUI execution records the blocked recovery as an error and leaves the active operation untouched.

Deterministic regression:

  • exact_rebase_abort_fails_closed_on_branch_move_after_final_validation synchronizes a second process at a hook placed after the final guard capture and before the act boundary, moves refs/heads/topic, and verifies the newer OID remains, the rebase remains active, and recovery does not run.
  • Existing success-path coverage was updated to assert the deliberate fail-closed behavior and stable error auditing.

Verification passed:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace (216 tests)
  • git diff --check
  • Focused Git regression and TUI fail-closed audit regression both pass.

Risk/tradeoff: confirmed merge/rebase recovery actions are intentionally unavailable through BitByGit rather than exposing a data-loss race. Recovery must be performed outside BitByGit until an actual cross-resource fence is implemented.

@cosentinode

Copy link
Copy Markdown
Owner Author

High: every recovery action is permanently non-functional (crates/bitbygit-git/src/lib.rs:284-318, crates/bitbygit-tui/src/lib.rs:3223-3247). After both state captures match, recover_exact unconditionally returns GitError::Blocked; the only method that actually runs git merge/rebase --continue|--abort|--skip is now #[cfg(test)]. Consequently uppercase-Y confirmation of every newly exposed recovery plan always records an error and leaves the operation active. This directly violates the issue goal of guarded recovery and the repository requirements to “allow explicit continue, abort, and skip actions” and make conflict states “recoverable from the TUI” (docs/guardrails.md:110-123, docs/mvp-phases.md:222-233). The regression suite currently codifies the broken behavior in confirmed_recovery_fails_closed_and_records_stable_audit_entries (crates/bitbygit-tui/src/lib.rs:5064-5091) instead of covering successful production execution. Keep the preview/execution-time revalidation, but invoke the typed recovery command when the state is unchanged and restore end-to-end success coverage for merge/rebase continue, abort, and rebase skip.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed both #72 (comment) and #72 (comment) in commit 9ae25d0 (fix: execute recovery with ref fencing).

Design:

  • recover_exact now executes the typed git merge/rebase --continue|--abort|--skip command after bounded preview/execution revalidation instead of unconditionally failing closed.
  • A final post-synchronization capture rejects the demonstrated branch move before Git starts, preserving HEAD, the conflicted worktree, operation metadata, and the newer branch OID.
  • Recovery runs with a temporary reference-transaction hook that rechecks every snapshotted recovery-owned/referenced ref in Git’s prepared phase, while the refs are locked. A later branch update therefore aborts Git’s ref transaction rather than being overwritten. Existing configured hooks are forwarded, and displayed Git arguments omit the temporary hook path.
  • Temporary fencing hooks are removed after execution.

Deterministic coverage:

  • Git-boundary success coverage executes merge continue/abort and rebase continue/abort/skip through recover_exact.
  • TUI coverage confirms all five requests execute successfully and merge abort records stable successful audit entries.
  • The synchronized post-validation rebase-abort race moves refs/heads/topic and verifies recovery is blocked with HEAD, worktree, active rebase, and the newer branch ref preserved.
  • Production-path merge continue coverage verifies configured commit hooks and signing behavior are not bypassed.

Verification passed:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace (218 tests)
  • git diff --check

Residual risk: Git still cannot transactionally lock arbitrary editor writes to worktree files. The bounded repeated state captures continue to detect those changes before spawn; the new prepared transaction fence specifically closes the demonstrated recovery-ref overwrite path without disabling recovery.

@cosentinode

Copy link
Copy Markdown
Owner Author

High: the ref fence can reject only after recovery has already mutated the repository (crates/bitbygit-git/src/lib.rs:323-330, crates/bitbygit-git/src/lib.rs:1575-1585). The generated hook checks protected refs only in the prepared phase, but git rebase --abort resets the index/worktree before it attempts the original-branch ref transaction. I reproduced this at the current head by stopping a rebase on a conflict, moving refs/heads/topic, and rejecting the abort ref transaction: git rebase --abort failed, yet the conflict index/worktree had already been replaced with the original topic contents while detached HEAD and rebase-merge remained active. Thus a branch move in the post-capture window is not fail-closed; the hook leaves a partially aborted operation and can discard conflict-resolution state. exact_rebase_abort_preserves_branch_move_after_final_validation changes the ref before the third capture, so it exits before Git starts and never exercises the new transaction fence. Add a race synchronized inside the actual prepared hook and require HEAD, index, worktree, metadata, and refs all to remain unchanged; the current hook-after-reset design cannot provide that guarantee.

High: overwrite-sensitive worktree/index state is still unfenced after the final capture (crates/bitbygit-git/src/lib.rs:312-330). After execution_state is captured, an editor or Git process can change a relevant file or the index before the recovery subprocess resets/applies it. The reference-transaction hook observes only ref updates and cannot detect or protect those changes, so abort/continue/skip can silently overwrite post-validation data. The synchronized worktree test mutates during an earlier capture, not after the final capture or during Git execution. This still violates the issue requirement that post-preview state changes block execution and needs a true execution-boundary strategy or explicit fail-closed behavior where no such fence exists.

High: installing the temporary hook can overwrite files in the configured hooks directory (crates/bitbygit-git/src/lib.rs:1534-1563). On Unix every sibling from the configured hooks directory is symlinked into the temporary directory. If that directory already contains expected-refs, line 1560 follows the symlink and truncates the original file with BitByGit state; if it contains transaction-input, the generated hook later truncates that original file via cat > "$input". Hook support files may use arbitrary names, so recovery must reserve private non-colliding storage outside the forwarded hook namespace and must never write through forwarded symlinks. Add regressions proving configured hook-directory contents remain byte-for-byte unchanged.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed #72 (comment) in commit e459c8c (fix: fence recovery at execution boundary).

Design:

  • Removed the temporary reference-transaction hook, hook-directory forwarding, symlinks/copies, core.hooksPath command override, and the expected-refs / transaction-input support files. BitByGit no longer installs a rejection point after Git may already have reset the index/worktree.
  • The bounded recovery capture now runs at the execution boundary and is the final BitByGit operation before directly spawning the typed git merge/rebase --continue|--abort|--skip command. A deterministic ref, index, or overwrite-sensitive worktree mutation at that boundary is rejected before Git starts, preserving the mutation and leaving HEAD, operation metadata, and all unaffected state unchanged.
  • Recovery uses the repository configuration directly. Existing hooks, hook support files, signing, and normal Git hook behavior are neither forwarded nor replaced.
  • Unchanged production recovery remains enabled for merge continue/abort and rebase continue/abort/skip.

Regressions:

  • Ref movement synchronized at the former temporary-hook installation/prepared-window boundary verifies no partial HEAD/index/worktree/metadata mutation and preserves the newer ref.
  • Late worktree and index mutations at the final execution boundary verify each mutation is preserved while recovery remains active and all unaffected guarded state remains byte-for-byte/equivalently unchanged.
  • A configured hook directory containing colliding expected-refs and transaction-input support names verifies hook files and repository config remain byte-for-byte unchanged, no BitByGit hook directory is created, and configured commit/reference hooks still run normally.
  • Existing Git-boundary and TUI production tests continue to execute all five supported recovery requests successfully.

Verification passed exactly:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace (221 tests)
  • git diff --check

Residual risk: stock Git exposes no transaction spanning arbitrary editor writes, the index, operation metadata, and refs for merge/rebase porcelain. The final capture closes the deterministic pre-spawn boundary covered here and removal of BitByGit's prepared hook eliminates the application-induced post-reset rejection, but an unrelated process that writes after Git has been spawned remains governed by Git's native locking/overwrite behavior; arbitrary worktree editor writes are not transactionally lockable by BitByGit.

@cosentinode

Copy link
Copy Markdown
Owner Author

Superseding my previous comment with the complete post-spawn redesign in aa62cbd (fix: isolate recovery before atomic promotion).

Design:

  • Recovery no longer runs against the live repository. BitByGit captures a bounded full-generation manifest, creates a private sibling copy with preserved filesystem contents/modes, verifies source and candidate equality, and runs the real typed Git porcelain command only in that candidate.
  • The recovery child runs in a private Linux user/mount namespace with the candidate bind-mounted at the original repository path. Configured relative/absolute in-repository hooks and signing therefore see the normal repository path, while the parent namespace and concurrent writers continue to see the untouched live generation. No hook namespace, hook support file, repository config, or core.hooksPath override is created or changed.
  • After speculative Git succeeds, BitByGit compares the complete live generation and semantic recovery state with the baseline. Promotion uses Linux renameat2(RENAME_EXCHANGE) to exchange the entire worktree plus embedded .git generation at one linearization point.
  • To close the final expected-value-to-exchange interval, BitByGit fingerprints the exchanged-out generation. If it differs, it atomically exchanges the directories back. Thus a race cannot expose or leave a mixture of recovered HEAD, refs, index, worktree, metadata, config, or objects.
  • Successful promotion retains the complete pre-recovery generation in a sibling .bitbygit-recovery-backup-* directory, preserving data reachable through pre-exchange open descriptors instead of deleting it. Unsupported linked/shared-gitdir layouts, platforms without atomic exchange, special filesystem entries, and explicit generation bounds fail closed before live mutation.
  • The obsolete temporary reference-transaction hook and old pre-spawn execution callback are absent.

Actual regressions:

  • isolated_rebase_abort_preserves_ref_race_during_prepared_hook_without_partial_recovery starts the real speculative git rebase --abort, blocks inside its real configured reference-transaction prepared phase after candidate index/worktree reset, moves the live topic ref, then proves promotion is rejected with the newer ref preserved and live HEAD/index/worktree/rebase metadata unchanged.
  • isolated_recovery_preserves_post_spawn_worktree_race and isolated_recovery_preserves_post_spawn_index_race perform live mutations while that actual post-spawn prepared hook is blocked and prove both mutations survive with the live rebase coherent.
  • atomic_promotion_rolls_back_race_after_expected_value_check mutates the live worktree after the last comparison and before exchange, then proves exchange-time generation validation atomically rolls back to the complete mutated active merge.
  • Configured-hook coverage verifies colliding expected-refs / transaction-input files and repository config remain byte-for-byte unchanged, configured commit/reference hooks execute normally, and hook pwd is the original repository path. Signing and non-UTF-8 hook-output coverage remains active.
  • Existing production Git/TUI coverage still executes merge continue/abort and rebase continue/abort/skip successfully when unchanged.

Verification passed at aa62cbd:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace (222 tests)
  • git diff --check

True residual constraints:

  • Atomic recovery is intentionally limited to Linux standalone repositories whose complete mutable state is under an embedded .git directory and where user/mount namespaces plus same-filesystem atomic directory exchange are available. Linked worktrees/shared common gitdirs and unsupported filesystems/platforms fail closed; they are not exposed to the old race.
  • Hooks/signing can intentionally create external side effects outside the repository. Those effects cannot be rolled back if later promotion detects a concurrent repository mutation; preserving normal hook/signing behavior necessarily permits those external effects.
  • Successful recovery retains the old complete generation for stale-descriptor safety, requiring explicit later backup cleanup policy rather than risking silent deletion of late writes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Phase 12] Add recovery plans and enforce conflict-mode policy

1 participant