Conversation
Preview unrelated checkpoint reachability before destructive cleanup. Expose pending attribution through status and actionable hook failures.
Summary by CodeRabbit
WalkthroughThe pull request adds a ChangesRecovery workflow
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Merge Risk: 🟡 Moderate · up to Recovery may become excessively slow with many missing bases, while a concurrent branch update could cause checkpoint evidence to be deleted. These issues should be resolved before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 10 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🟠 Major · Make stranded-checkpoint cleanup safe against concurrent ref creation.
internal/provenance/service.go:653-664
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake stranded-checkpoint cleanup safe against concurrent ref creation.
cachedReachablecallsrepo.AnyBranchContains, which checksrefs/headsandrefs/remoteswithgit for-each-ref.dropStrandedCheckpointsthen deletes records withDropCheckpointRecordswhile holding onlybyline/operation.lock. A concurrent local branch creation or remote-tracking ref update can make the base reachable after the check and before deletion.Use synchronization shared by supported Git ref updates. If external Git operations cannot participate, quarantine the records and make final deletion recoverable after a final reachability check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 56578af6-f932-437f-8f69-1be442d9ddb8
📒 Files selected for processing (12)
README.mddocs/ARCHITECTURE.mdinternal/app/app.gointernal/app/product.gointernal/app/recover.gointernal/app/recover_test.gointernal/gitcmd/gitcmd.gointernal/gitcmd/gitcmd_test.gointernal/provenance/recovery.gointernal/provenance/recovery_test.gointernal/provenance/service.gointernal/provenance/service_test.go
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| if branches, exists, err := repo.BranchesContaining(first); err != nil || !exists || | ||
| strings.Join(branches, ",") != "refs/heads/main" { | ||
| t.Fatalf("BranchesContaining(main commit) = %v, %v, %v", branches, exists, err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use table-driven Git boundary cases.
The new cases extend one imperative test sequence. Move the branch-containment scenarios into named table cases with isolated setup and assertions.
As per coding guidelines, “Use table-driven tests for engine, model, store, and Git boundaries.”
Source: Coding guidelines
| "--no-dangling", | ||
| ); fsckErr != nil { | ||
| return false, fmt.Errorf("verify object database: %w", fsckErr) | ||
| if err := repo.verifyObjectDatabase(); err != nil { |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Bound object-database verification per recovery scan.
Line 617 runs a full git fsck for every distinct missing checkpoint base. previewRecovery calls BranchesContaining once per distinct base, and Status also invokes that preview. Rewritten history with many missing bases can therefore run many complete object scans and make recover and status take up to the cumulative command timeouts.
Verify the object database once per recovery/status snapshot, while preserving a verification failure as a fail-closed error. Add a regression test with multiple distinct missing bases.
As per path instructions, internal/**/*.go must have “bounded resource use.”
Source: Path instructions
| | --- | --- | | ||
| | `checkpoint <preset>` | Record a human or AI edit snapshot from hook input | | ||
| | `annotate [--drop-stranded]` | Replay pending snapshots and annotate `HEAD`; explicitly discard unreachable unrelated checkpoints when recovering | | ||
| | `annotate [--drop-stranded]` | Replay pending snapshots and annotate `HEAD`; compatibility recovery flag remains available | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the destructive behavior of annotate --drop-stranded.
This compatibility path discards stranded checkpoint records before retrying annotation. State that it can remove unreachable unrelated checkpoints and direct users to preview with recover first.
- README.md#L465-L465: Describe the records discarded by
annotate --drop-stranded. - docs/ARCHITECTURE.md#L89-L90: Describe the same destructive behavior in the compatibility workflow.
📍 Affects 2 files
README.md#L465-L465(this comment)docs/ARCHITECTURE.md#L89-L90
| remote-tracking branch that reaches that base. Use `recover --drop` only after | ||
| the preview marks records as stranded. The command rechecks reachability, | ||
| drops only unreachable records, and retries annotation. Hook-driven annotation |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the blocked-checkpoint precondition for recover --drop.
The command refuses to drop records when any blocked checkpoint remains. State that all unrelated checkpoints must be stranded before the command can drop records and retry annotation.
- README.md#L488-L490: State the all-stranded requirement or direct users to resolve blocking branches first.
- docs/ARCHITECTURE.md#L86-L88: Document that blocked checkpoints prevent dropping and retrying annotation.
📍 Affects 2 files
README.md#L488-L490(this comment)docs/ARCHITECTURE.md#L86-L88
1 participant
Summary
annotatestill fails closed when checkpoint belongs to another base, but failure now explains that attribution is pending and points togit-byline recover.New
recovercommand is read-only by default. It shows each unrelated checkpoint, object availability, blocking local or remote-tracking branches, and next action.recover --droprefuses reachable bases, rechecks reachability, drops only stranded records, then retries annotation.--jsongives same preview and result for tooling.statusnow shows pending annotation, unrelated, stranded and blocked counts, plus recommended action. Branch inspection also catches successful Git commands that reported broken refs on stderr, so cleanup stays fail closed.Scope
recover [--drop] [--json]command, better annotate hook failure, richer text and JSON status.annotate --drop-strandedremains available. No persisted format change.--drop. No automatic evidence deletion, network, telemetry, or new stored content. Git stderr and returned refs are validated before reachability can allow cleanup.Related issue
Follow-up to #28. #30 tracks branch-scoped checkpoint lanes.
Validation
Checklist