Skip to content

fix(cli): support safe unrecord by hash or prefix - #196

Merged
graywolf336 merged 1 commit into
devfrom
fix/unrecord-by-hash
Sep 21, 2026
Merged

graywolf336 merged 1 commit into
devfrom
fix/unrecord-by-hash

Conversation

@vinceblock99

Copy link
Copy Markdown
Contributor

atomic unrecord <hash> is advertised in CLI help but currently always returns “not yet supported.” This implements full Base32 hashes and case-insensitive unique prefixes, including --dry-run. Omitting the argument still selects the last change.

Before removal, the repository checks view membership and dependency safety in the same write transaction. It rejects inherited changes and changes required by the view's remaining dependency closure, using stored change metadata when a legacy repository lacks the dependency index. Preview and execution use the same guard. Independent middle changes can be removed without modifying working files or other views; their stored objects remain available for reinsertion. This does not add cascading removal.

Validation:

  • cargo test -p atomic-repository -p atomic-cli --lib --bins --tests: 2,929 passed, 0 failed, 2 ignored, including 13 new regression tests.
  • Tests exercise hash/prefix selection, invalid and ambiguous inputs, absent/nonmember changes, dependency and inheritance rejection, legacy metadata, dry-run behavior, preserved file content in another view, and reinsertion.
  • cargo fmt --all -- --check passed.
  • cargo clippy -p atomic-cli -p atomic-repository --lib --bin atomic --test unrecord_integration_test --test unrecord_safety_test -- -D warnings passed.
  • All-target Clippy is blocked by four pre-existing lints in the unchanged atomic-repository/src/repository/tests/merge_property_tests.rs (lines 11, 29, 159, and 164).

Authored in Atomic under intent ATOM::vince::1, then exported as four source/test files against dev (d34974a).

@graywolf336

Copy link
Copy Markdown
Contributor

Critical review — git-ism audit (verdict: clear) + one consolidation finding

Reviewed this against atomic's model (graph/patch-based; views are filters; the graph is the source of truth), specifically hunting for git semantics smuggled in. No git-isms found. In fact this PR is unusually model-faithful:

What it gets right (the anti-git list):

  • Removal is from the view's change log (the filter), never history rewriting. check_unrecord_safety requires get_change_seq(view, change_id) membership and then removes the entry — the patch itself is untouched.
  • "Last change" = the view's log order, via get_last_change(&txn, &view) — view-scoped, not a global tip/HEAD notion.
  • Dependency safety is a graph walk, not a linear rule. It rejects a change required by the transitive dependency closure of the view's remaining changes (own log + ancestor chains), and fails closed on legacy repos: "Never mistake an unindexed change for one with no dependencies" (unrecord_safety_test.rs proves unindexed/transitive/inherited dependents all block). A git-shaped implementation would have checked "is it the tip".
  • Inherited changes are rejected with the right remedy ("unrecord it there instead"), walking the draft overlay chain — the view model, not "published history".
  • Nothing is garbage-collected. Objects stay in the store; the test removes a middle change and then proves load_change(&hashes[1]).is_ok() and insert_change(&hashes[1]) re-materializes it — "the retained change is usable, not just a leftover object on disk". Other views' logs are asserted unchanged.
  • The working copy is never clobbered — the exact opposite of git reset --hard: FILE_INDEX entries for affected paths are invalidated instead of re-materializing, "that would overwrite the user's disk changes", so the next status recomputes against the graph.
  • No cascading removal, preview and execution share the same guard inside one write transaction (no TOCTOU), and open_or_create_view → get_view fixes a latent bug where unrecording on a missing view would create it.

One finding (consistency, not a git-ism): a third prefix resolver.

Repository::find_change_by_prefix already exists (changes.rs:1020) — documented case-insensitive, at least 2 characters, returning AmbiguousHash { prefix, matches } with the match list — and is what atomic insert uses. atomic change has its own (change/command.rs::resolve_hash_prefix) that is case-sensitive. This PR adds a third (unrecord.rs::resolve_change: case-insensitive, ≥1 char, no match list).

The new resolver's stated goal ("resolve against the whole change store so a non-member gets the repository's membership error") is already satisfied by find_change_by_prefix — it scans the same iter_changes(). Suggest calling the canonical helper and mapping Ok(None) → ChangeNotFound, Err(AmbiguousHash) → CliError::AmbiguousHash, optionally moving this PR's nice base32/≤52 validation into the shared helper so every hash-taking command validates alike. Otherwise hash addressing drifts by command (≥2 vs ≥1, case-insensitive vs case-sensitive).

Verification performed: merged onto current dev (fe51d4b) — clean; unrecord_integration_test 8/8, unrecord_safety_test 5/5; cargo clippy -p atomic-cli -p atomic-repository -- -D warnings clean.

@graywolf336

Copy link
Copy Markdown
Contributor

Resolved the review finding: resolve_change now delegates to the shared Repository::find_change_by_prefix (the same case-insensitive resolver atomic insert uses), with the base32/≤52 input validation kept locally for precise malformed-input errors and ambiguity errors surfaced with the match list. The JS story: one resolver, one rule set — no per-command drift. Also rebased the branch onto current dev (fe51d4b) as part of the update, since the previous base predated the recent merges. Verified: unrecord_integration_test 8/8, unrecord_safety_test 5/5, clippy -D warnings clean.

@graywolf336
graywolf336 merged commit 0692978 into dev Sep 21, 2026
15 of 16 checks passed
@graywolf336
graywolf336 deleted the fix/unrecord-by-hash branch September 21, 2026 23:38
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.

2 participants