Add guarded conflict recovery plans#72
Conversation
|
Fresh-context review found two blocking concerns:
Verification: |
|
Addressed both blocking review concerns in commit
Verification passed:
|
|
High: exact rebase recovery still ignores refs that abort can overwrite ( Verification: |
|
Addressed the exact rebase recovery ref concern from #72 (comment) in commit
Verification passed:
|
|
High: deferred sequence requests are revalidated only after the first side effect ( |
|
Addressed the deferred sequence policy bypass from #72 (comment) in commit
Verification passed:
|
|
High: exact rebase recovery can overwrite ignored worktree data changed after preview ( Verification: |
|
Addressed concern #4973048113 in commit
Verification passed:
|
|
Fresh-context review of current head
Verification at |
|
Implemented all three blocking fixes in
Exact verification completed successfully:
Post-push status is clean and |
|
High: confirmation-time recovery revalidation is a non-coherent snapshot and can miss concurrent edits ( High: snapshots remain unbounded in memory and work despite the new sparse-file test ( Medium: untracked/ignored filesystem changes are not captured completely ( Verification at current head |
|
Addressed the recovery snapshot review in commit Design changes:
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 Verification passed at
Git does not expose a transaction spanning arbitrary external worktree edits and |
|
High: a relevant directory is snapshotted without its contents, so abort can delete post-preview data ( |
|
Addressed #72 (comment) in commit
Verification passed:
|
|
High: final recovery validation still has an unguarded compare-to-act window that can lose concurrent state ( Verification at |
|
Addressed #72 (comment) in commit Design:
Deterministic regression:
Verification passed:
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. |
|
High: every recovery action is permanently non-functional ( |
|
Addressed both #72 (comment) and #72 (comment) in commit Design:
Deterministic coverage:
Verification passed:
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. |
|
High: the ref fence can reject only after recovery has already mutated the repository ( High: overwrite-sensitive worktree/index state is still unfenced after the final capture ( High: installing the temporary hook can overwrite files in the configured hooks directory ( |
|
Addressed every finding in commit
Verification completed:
macOS and Windows recovery-platform jobs are left to PR CI. No unresolved local risks found. |
|
Follow-up: Post-merge verification:
The transient pre-commit workspace test failure caused by Git detecting the in-progress merge was rerun after the merge commit and passed. No merge was performed on PR #72. |
|
CI follow-up commit
Reverification: format, clippy with warnings denied, all 44 recovery tests, and all 299 workspace tests pass locally. Awaiting the rerun macOS/Windows matrix. |
|
Final verification update: CI run https://github.com/cosentinode/bitbygit/actions/runs/29615520789 is fully green, including macOS and Windows recovery safety and all Rust 1.85 target builds. PR #72 is mergeable with |
|
Fresh independent review of current head
Verification at |
|
Addressed the ignored-file overwrite finding in commit The recovery fingerprint now hashes ignored worktree file contents with the existing race/identity fence, does not follow ignored symlinks, and uses a separate aggregate 64 MiB budget. If ignored data exceeds that bound, recovery planning fails closed instead of allowing an unverified abort. The rebase-abort regression again mutates the same ignored collision path after preview ( Local verification:
Hosted macOS and Windows recovery-safety checks are running for |
|
Final hosted verification for |
cosentinode
left a comment
cosentinode
left a comment
There was a problem hiding this comment.
Fresh independent review of current head 5891bf991bf27de86e5f1a3e0fd167c932d9ac36 found two blocking concerns:
-
High: the process-safety check still validates a different filesystem state from the fingerprint and can be raced to launch an executable hook.
recovery_state_until_withhashes and retains the hooks input, butRecoveryInputFence::validateruns the independent pathname-based process-safety scan first and only afterwards validates the recorded probes/handles. A writer can keep a non-executable hooks directory at the path for the initial scan, swap in an executable-hook directory while the snapshot records it, swap the safe directory back only forensure_recovery_process_configuration_safe_until, and restore the same unsafe directory before input identity validation. The fingerprint and retained handle then match the unsafe directory, while the safety scan passed against the safe one. Repeating that sequence during preview and execution letsrecover_exactspawn Git with the executable hook after uppercase-Y confirmation. The new test only enables a hook and leaves it enabled, so the safety scan detects it; it does not exercise alternating safe/unsafe path identities. Evaluate process safety from the exact retained observations used to construct/validate the fingerprint (or otherwise couple safety and identity atomically), and add a synchronized directory-swap regression proving no hook starts. -
High: fingerprinting every ignored artifact makes recovery unavailable after ordinary builds, including in this repository. The latest change enumerates every ignored path, reads every ignored regular file, and imposes both the generic retained-output/100,000-entry limits and a 64 MiB aggregate content limit (ignored loop, path-list rejection). After running this PR's required
cargo test --locked --workspacein a clean worktree, ignoredtarget/is 641,842,755 bytes andgit ls-files --others --ignored --exclude-standard --full-name -z --emits 209,653 bytes. Recovery therefore fails before producing a plan, first on the 64 KiB retained path-list limit and, absent that, on the 64 MiB content limit. Commontarget,node_modules, build caches, and generated trees make the feature unusable in normal repositories. Guard only ignored paths that can actually be overwritten by the selected recovery, or use a scalable policy that preserves same-path collision protection without whole-repository caps; add a production recovery test with a realistic ignored build tree.
Verification at 5891bf9: cargo fmt --all -- --check, cargo clippy --locked --workspace --all-targets -- -D warnings, cargo test --locked --workspace (299 passed), and git diff --check pass. All hosted checks are green and the PR is mergeable; neither case above is covered.
|
Addressed both concerns from review #72 (review) in
Verification passed:
|
|
Hosted verification for |
cosentinode
left a comment
cosentinode
left a comment
There was a problem hiding this comment.
Fresh independent review of the complete develop...dab7f21 diff found three blocking concerns:
-
High: the ignored-collision candidate set misses intermediate rebase writes and recovery can delete ignored data.
ensure_no_ignored_recovery_collisions_untilchecks only paths in the netgit diff HEAD <orig-head>, and every rebase action uses the final original head as that target (lines 1807-1830). Remaining commits can touch a path and later revert it, so that path is absent from the net diff even thoughrebase --continuewrites it. I reproduced this with a paused rebase whose remaining commits add then delete ignoredvictim.txt: the candidate diff listed only an unrelated upstream path, the literal ignored query reportedvictim.txt, planning would not query it, andgit rebase --continuecompleted with the local ignored file deleted. Derive collision paths from every tree/patch that continue/skip can install, or conservatively block relevant ignored paths, and add an intermediate-commit data-loss regression. -
High: the final input fence still permits a post-preview index/control mutation before spawn.
RecoveryInputFence::validaterevalidates retained inputs once in sequence. After an early input such as the index is checked, validation continues through later control trees, hooks, attributes, and untracked paths; there is no recheck of that index before the callback returns andrun_bounded_commandspawns Git. An externalgit addsynchronized after the index's turn therefore leaves the computed fingerprint equal to the preview and is committed by merge/rebase continue. The current synchronized test mutates after the initial index read, beforevalidatechecks retained identities, so it does not exercise this window. Recheck/fence consumed inputs at the actual spawn boundary and add synchronization after the index has passed its final-loop check. -
High: non-hook process-safety checks still inspect a different state from the retained fingerprint.
RecoveryInputFence::validateruns the independent pathname/config safety scan first, then validates config probes and retained control-file observations. The latest fix evaluates executable-hook metadata from the hashed observation, but there is no equivalent semantic validation forcore.fsmonitor, external drivers, signing config, persisted strategy/signing files, or rebase todoexecentries (safety scan, hook-only observation check). A racing writer can present safe config/control paths during each safety scan, restore the same unsafe state captured by the preview before probe/identity validation, and leave it active for spawn; the expected fingerprint then explicitly matches the unsafe state and Git can launch an uncontained helper. Evaluate all process-safety policy from the exact retained observations used by the fingerprint, with synchronized safe/unsafe config and todo/strategy swaps.
Verification at dab7f21: cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, cargo test --locked --workspace (299 passed), git diff --check, and the ignored-path reproduction above. Hosted Linux, macOS, Windows, Rust 1.85 source-build, installation-doc, and GitGuardian checks are green; these cases are not covered.
|
Addressed every concern from review #72 (review) in Fixes
Verification
Hosted macOS and Windows recovery/build checks are left to the PR CI matrix. No merge performed. |
|
Hosted verification for |
cosentinode
left a comment
cosentinode
left a comment
There was a problem hiding this comment.
Fresh independent review of the complete current develop...cd9f6a0 diff found four blocking concerns:
-
High: abort recovery can overwrite ignored local data on paths omitted from the endpoint diff.
ensure_no_ignored_recovery_collisions_untilseeds collision candidates fromgit diff --name-only HEAD <abort-target>, while merge abort selectsORIG_HEAD(lines 1925-1934). In a normal conflicted mergeHEAD == ORIG_HEAD, so that candidate set is empty. I reproduced this at the publicprepare_recovery/recover_exactpath: start a conflict with trackedstable.txtunchanged on both branches, rungit rm --cached stable.txt, ignorestable.txt, and replace its worktree contents with local data. Planning succeeds because ordinary untracked enumeration excludes ignored paths and the endpoint diff does not namestable.txt; confirmed merge abort then restores the tracked blob over the local data. The same endpoint-net-diff gap applies to unchanged target paths during rebase abort. Collision planning must cover every target path the abort checkout can overwrite, with a regression proving ignored contents remain intact. -
High: a standard global Git LFS installation disables every recovery, while the test configuration hides this production behavior. Recovery rejects every configured
filter.*.(clean|smudge|process)key regardless of whether any repository/target path selects that filter (lines 1595-1625). A normalgit lfs installcreates globalfilter.lfs.clean,filter.lfs.smudge, andfilter.lfs.process, so even a non-LFS repository fails planning with “configured external Git drivers...” and none of issue #55's actions are usable. I reproduced that through productionGit::new; the suite misses it because constructors setisolated_test_config: cfg!(test)(lines 134-168) and replace global config for all tests. Check effective attributes/actual activation rather than mere global key presence, or explicitly narrow and test/document this substantial compatibility limit with a non-isolated production-path test. -
Medium: the configured 60-second execution deadline does not apply after the mutating Git process starts.
recover/recover_exactcalculaterecovery_execution_timeout(lines 381-484), butrun_bounded_commandexplicitly skips its deadline whenever the policy isRecoveryExecution(lines 2805-2835); tests deliberately assert that behavior (lines 5784-5806). A stalled Git/filesystem operation or nonterminating output therefore blocks the synchronous TUI and holds the application recovery lock indefinitely. Either enforce a safe execution bound/termination strategy or remove the false timeout contract and make execution cancellable without leaving the UI permanently wedged. -
Blocking integration: the PR is currently unmergeable with
develop. GitHub reportsmergeable: CONFLICTING;git merge-tree --write-tree origin/develop cd9f6a0confirms a content conflict incrates/bitbygit-tui/src/lib.rsafter develop advanced to62ae924. The green checks ran against the prior base32c0cb8, so they do not verify the required conflict resolution or combined recovery/default-PR-base behavior. Update from currentdevelop, resolve the overlap, and rerun the full matrix.
Verification at cd9f6a0: cargo test --locked --workspace passes (303 tests), git diff --check passes, current hosted checks are green, the ignored-data overwrite was reproduced through recover_exact, the global Git LFS rejection was reproduced through production Git::new, and the current merge conflict was reproduced with git merge-tree.
…policy # Conflicts: # crates/bitbygit-tui/src/lib.rs
|
Addressed all findings from review 4726642751 in Changes:
Verification (all passed locally on Linux):
Hosted macOS/Windows recovery and supported-target source-build checks are pending on the pushed commits. The PR remains unmerged. |
|
Hosted verification for |
cosentinode
left a comment
cosentinode
left a comment
There was a problem hiding this comment.
Fresh independent review findings at f4ffe98
-
High: the fixed execution deadline can SIGKILL Git after destructive recovery has begun, leaving a partially rewritten repository.
recover_exact_withstarts one 60-second deadline before all execution-time checks, andrun_bounded_commandkills the process group whenever that deadline expires after spawn. The final fingerprint can consume most of the budget, so a valid recovery may start with almost no execution time left. Merge/rebase continue, skip, and abort update the worktree/index incrementally; SIGKILL is not transactional. I reproduced the underlying Git behavior with a 20,000-file repository: killinggit reset --hardafter 20 ms left the old HEAD, 881 dirty paths containing a partial rewrite, and.git/index.lock. The added timeout test only verifies that a synthetic child returnsTimedOut; it never checks repository consistency. Do not force-kill Git after a mutating recovery starts (or provide a recovery-specific cancellation design that can prove consistency), and add an interrupted real-recovery regression that verifies no partial state or stale lock remains. -
Medium: release installation requirements still claim any Git version works, while every recovery action rejects Git older than 2.42. The shipped installation guide lists only “
git... available onPATH” as a runtime prerequisite (docs/installation.md:8-13), but recovery unconditionally rejects older Git (crates/bitbygit-git/src/lib.rs:3206-3223). The README change is under development prerequisites, so archive users on stock Ubuntu 22.04 (Git 2.34) or Debian 12 (2.39) are still told they satisfy runtime requirements and only discover that issue #55 is unavailable after attempting recovery. Document the feature-specific minimum in the runtime installation instructions and validate that statement in the installation-doc tests. -
Medium: the macOS/Windows recovery job does not execute a real recovery action. The new matrix runs only
cargo test --locked -p bitbygit-git recovery_platform_(.github/workflows/ci.yml:86-107). That filter selects capability/lock primitives (four tests on Linux), notmerge_abort_clears_operation_and_restores_head, rebase continue/abort/skip, or the TUI five-action integration test. All actual recovery success coverage therefore runs only in the Linux workspace job despite platform-specific process groups, Windows Job Objects, file sharing, and identity handling. Run at least one real merge and rebase recovery through the public exact-recovery path on each shipped platform.
Verification: git diff --check, cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, and cargo test --locked --workspace all pass locally. All current hosted checks are green; the timeout consistency case and cross-platform end-to-end actions are not covered.
Review fixes pushedCommit: Addresses the full review at #pullrequestreview-4726769095:
Local verification passed:
Hosted macOS/Windows execution and the Windows PowerShell docs validator are running in CI. A broader cross-target workspace |
|
Windows CI follow-up: commit Reverified locally:
A new hosted run is in progress. |
|
Final hosted verification is green at head Passed checks include:
PR #72 is open and mergeable. No merge was performed. |
cosentinode
left a comment
cosentinode
left a comment
There was a problem hiding this comment.
Fresh independent review of the complete current develop...f47a1bc diff found two blocking concerns:
-
High:
rebase --skipcan overwrite ignored local data on paths unchanged betweenHEADand the original branch. For every non-abort rebase action, collision candidates start withgit diff --name-only HEAD <orig-head>(lines 1919-1944), whilerecovery_overwrite_target_untiltreats skip exactly like continue (lines 2106-2141). But skip first resets the conflicted index/worktree to the currentHEAD; any path tracked identically byHEADand the original branch is absent from that diff even though the reset writes it. I reproduced this through publicprepare_recovery/recover_exact: during a one-commit conflict, remove an otherwise unchanged trackedstable.txtfrom the index, ignore it, and replace it with local data. Planning succeeds and confirmed skip replaces the local data with the tracked blob. Include every path the skip reset can overwrite (as well as future commits), and add a regression asserting ignored contents survive. -
High: apply-backend rebases omit all remaining patches from collision and external-process checks.
remaining_rebase_commit_oids_untilonly readsrebase-{merge,apply}/git-rebase-todoand returns an empty set when those files are absent (lines 2087-2104); a realgit rebase --applystores numbered patch files plusnext/last, not a todo file. Both future collision discovery (lines 1984-2025) and future filter activation checks (lines 1733-1749) therefore see no remaining work. I reproduced a three-commit apply rebase where an intermediate commit added an ignoredvictim.txtand.gitattributes, and the next removed both so the endpoint diff was empty. Public exact continue accepted the preview, deleted the local ignored file, and launched the configured smudge filter selected only by that intermediate attributes patch. Either model every remaining apply patch safely for both paths and attributes, or fail closed on apply-backend continue/skip; cover both data preservation and process non-execution.
Verification at f47a1bc: cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, cargo test --locked --workspace (321 passed), bash scripts/test-ci-workflow.sh, and git diff --check pass. All current hosted Linux, macOS, Windows, Rust 1.85 source-build, installation-doc, and GitGuardian checks are green; the two public-API repros above fail and are not covered.
1 participant
Summary
Verification
cargo fmt --all --checkcargo clippy --locked --workspace --all-targets -- -D warningscargo test --locked --workspaceFixes #55