Capture and restore nu_state/paths.json in snapshots - #84
Conversation
PreMutation snapshots now always record the Nu path cache (or Absent), and rollback restores it even with an empty lockfile. init --refresh snapshots before rewriting paths so refresh-then-rollback is consistent. Co-authored-by: Anthony Thompson <github@trackdub.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (9)**/*📄 CodeRabbit inference engine (CLAUDE.md)
Files:
!**/.env,!**/credentials.json,!**/*.pem📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{rs,md}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,md,toml}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.md📄 CodeRabbit inference engine (REVIEW.md)
Files:
**/*.{js,jsx,ts,tsx,py,java,go,rs,rb,php,c,cpp,h,hpp,cs,swift,kt,kts}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.rs📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,toml}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,nu}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
🧠 Learnings (1)📓 Common learnings🔍 Remote MCP DeepWiki, GitHub CopilotRelevant review context
🔇 Additional comments (4)
📝 WalkthroughWalkthroughSnapshots now capture and verify the Nu path cache, recording present or absent state. Rollback restores, deletes, or preserves the cache based on snapshot metadata through a new lifecycle stage. Inspection output, lifecycle journaling, documentation, refresh comments, and related tests reflect this change. The architecture rule excludes ChangesNu paths snapshot and rollback
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. Analyzed 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tonythethompson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
PR Summary by QodoCapture and restore
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
There was a problem hiding this comment.
Pull request overview
This PR extends Numan’s snapshot/rollback system to also capture and restore the Nu path cache (nu_state/paths.json), closing the gap where numan init --refresh could rewrite the cache and rollback would not restore it (notably when the lockfile is empty / no active packages).
Changes:
- Add a
paths.jsonsnapshot sidecar (SnapshotPaths::{Absent, Present(NuPaths)}) with a digest recorded in the snapshot manifest, while keeping legacy snapshots loadable. - Restore/clear
nu_state/paths.jsonduring rollback as a journaled stage (PathsCommitted) after imports. - Update
numan snapshot inspectoutput and documentation to reflect paths capture/restore behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/state/snapshot.rs | Adds SnapshotPaths sidecar capture + digesting + legacy-aware load behavior. |
| src/state/rollback.rs | Restores/clears nu_state/paths.json as an additional journaled rollback commit step. |
| src/state/lifecycle_journal.rs | Adds LifecycleStage::PathsCommitted for rollback journaling. |
| src/cmd/snapshot.rs | Prints paths sidecar digest and a summary of captured Nu paths during inspect. |
| src/cmd/init.rs | Adds snapshotting around init --refresh (but currently duplicates the snapshot). |
| docs/snapshots-and-rollback.md | Documents paths sidecar scope, legacy behavior, and restore order. |
| AGENTS.md | Updates architecture notes to include paths capture/restore semantics. |
Suppressed comments (1)
src/state/snapshot.rs:628
capture_paths_sidecarcurrently maps allNuPaths::loadfailures toSnapshotPaths::Absent. This incorrectly treats parse/IO errors as an "uninitialized root" and can lead to snapshots/rollbacks that delete a real-but-corrupt path cache. Only returnAbsentwhennu_state/paths.jsonis missing; otherwise return an error so the user can repair/re-init.
fn capture_paths_sidecar(root: &Path) -> SnapshotPaths {
match NuPaths::load(root) {
Ok(paths) => SnapshotPaths::Present(paths),
Err(_) => SnapshotPaths::Absent,
}
Greptile SummaryThis PR closes the
Confidence Score: 5/5Safe to merge; the paths sidecar is additive and all three rollback cases are idempotent and crash-recoverable. The core changes are well-bounded: an optional sidecar written atomically at snapshot time, a single new journaled commit step in rollback, and a pre-existing pattern for digest verification. The Files Needing Attention: No files require special attention; the test duplication in
|
| Filename | Overview |
|---|---|
| src/state/snapshot.rs | Adds SnapshotPaths enum (Absent/Present), paths_sha256 to SidecarDigests, and paths field to Snapshot; capture_paths_sidecar correctly errors on a corrupt-but-present paths.json (not silent Absent) and all load/verify logic is consistent with the existing digest pattern. |
| src/state/rollback.rs | Adds journaled Commit 5 for SnapshotPaths; all three cases (None/legacy, Absent, Present) are handled correctly; crash-resume idempotency is preserved; nu_state directory is created before paths.save in the Present branch. |
| src/cmd/init.rs | Single create_snapshot call correctly placed before any mutation in execute_refresh; test refresh_updates_plugin_activation_identity creates and writes the payload directory twice due to new lines shadowing the pre-existing setup. |
| src/state/lifecycle_journal.rs | Adds PathsCommitted variant between ImportsCommitted and Completed; serializes cleanly with rename_all = "snake_case"; no other state machines consume this enum so no match exhaustiveness regressions. |
| src/cmd/snapshot.rs | Inspect command correctly displays all three path states (Present, Absent, legacy None); digest line added alongside existing autoload/imports lines. |
Reviews (6): Last reviewed commit: "Merge branch 'master' into cursor/snapsh..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
- Differentiate missing and invalid paths sidecars - Ignore unverified legacy paths sidecars
Qodo Fixer✅ Committed (3) · ☑ Fixed (3) Commits pushed directly to this PR — no separate fix PR opened. Process — 3 fixed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 522cc06f57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Remove `let mut lockfile = lockfile;` in execute_refresh so the existing mutable binding is used through activation refresh. Clears unused-mut and redundant_locals under `cargo clippy -- -D warnings`. Co-authored-by: Anthony Thompson <github@trackdub.com>
4a1724b
capture_paths_sidecar already returns Absent only when paths.json is missing and propagates other load errors. Add regression coverage and refuse load when a paths digest is present but the sidecar file is gone. Co-authored-by: Anthony Thompson <github@trackdub.com>
25d47ca
|
Re: Codex P2 on Already addressed on this branch:
Current shape: fn capture_paths_sidecar(root: &Path) -> Result<SnapshotPaths> {
let paths_path = root.join("nu_state/paths.json");
if !paths_path.exists() {
return Ok(SnapshotPaths::Absent);
}
let paths = NuPaths::load(root).with_context(|| { /* ... */ })?;
Ok(SnapshotPaths::Present(paths))
}No further code change needed for this finding. |
Summary
numan init --refreshrewritesnu_state/paths.json, but snapshots previously only stored lockfile / autoload / imports plus optionalnu_identity(hash + version). Rollback restored the activation graph, not the path cache, so refresh-then-rollback with no active packages could leave paths on the post-refresh Nu.This PR adds an additive paths sidecar to every new snapshot and restores it during rollback, including the empty-lockfile case.
Changes
src/state/snapshot.rs): optionalSidecarDigests.paths_sha256+SnapshotPaths::{Absent, Present(NuPaths)}sidecar written on everycreate_snapshot. Legacy snapshots without the file still load (paths: None).src/state/rollback.rs): journaledPathsCommittedstep after imports. Present →NuPaths::save; Absent → deletepaths.json; legacyNone→ leave live cache alone. Keeps the existing Nu-identity precondition for Present managed autoload (checked against the live cache before restore).src/cmd/init.rs):init --refreshtakes aPreMutation/Initsnapshot before rewriting lockfile identity orpaths.json.docs/snapshots-and-rollback.md+AGENTS.md(capture set, restore order, no-active-packages behavior,nu_identityvs paths sidecar).Non-goals
PATH.Test plan
Absentpaths.jsonAbsentdeletes pathscargo fmt --checkcargo clippy -- -D warningscargo test(full suite green; one pre-existing flakynu::pathsPATH fall-through test observed under parallel load, passes on retry / isolation)