Skip to content

fix(undo): skip symlinks when walking snapshots - #1493

Merged
SequeI merged 5 commits into
mainfrom
kurtisc/symlink-rep
Jul 27, 2026
Merged

fix(undo): skip symlinks when walking snapshots#1493
SequeI merged 5 commits into
mainfrom
kurtisc/symlink-rep

Conversation

@kurtisc

@kurtisc kurtisc commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Linked Issue

Closes #1480

Summary

Skips symlinks when walking snapshots to avoid including the size erroneously in budgets.

Test Plan

Steps to reproduce

Red/green test added

Checklist

  • An issue exists and is linked above
  • All commits are signed-off, using DCO
  • All new code follows the project's coding standards (CLAUDE.md) and is covered by tests
  • Public-facing changes are paired with documentation updates
  • Release note has been added to CHANGELOG.md if needed

@github-actions github-actions Bot added bug Something isn't working nono nono-cli size/medium labels Jul 23, 2026
@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

PR Review Summary

Size

Metric Value
Lines added +374
Lines removed -30
Total changed 404
Classification Large (> 300 lines)

Affected crates

  • crates/nono (core library) — careful review required. This is the security-critical sandbox primitive. A bug here bypasses OS-level isolation for every downstream user.
  • crates/nono-cli — CLI changes. Verify argument parsing, flag documentation, and UX behaviour across supported platforms.

Blast radius — Contained

This PR touches: source code


Updated automatically on each push to this PR.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@nogent-nolabs-ai nogent-nolabs-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nogent code review

No critical vulnerabilities found; 2 consistency bugs identified in symlink preflight checks.

Findings (not tied to a changed line):

  • 🔒 [HIGH · security] crates/nono/src/undo/snapshot.rs:256 — By completely skipping symlinks in walk_current, any new symlink created by the untrusted sandboxed agent during its execution will be omitted from current_files. Consequently, the cleanup loop in restore_to (which deletes files present in current_files but absent from the manifest) will never see or delete these symlinks, allowing untrusted symlinks to persist across session rollbacks. To fix this, walk_current should yield symlink paths with a sentinel size/hash (or a dedicated symlink variant/collection) so that restore_to can safely detect and clean them up.
  • 🐛 [MEDIUM · bug] crates/nono-cli/src/rollback_preflight.rs:142 — The preflight helper detect_heavy_dirs does not check if the tracked root itself is a symlink. If a tracked root is a symlink targeting a directory, detect_heavy_dirs will traverse it using std::fs::read_dir, whereas SnapshotManager::walk_and_store and walk_current will explicitly skip it. Check the root's fs::symlink_metadata and skip it if it is a symlink, to ensure consistency between preflight detection and the actual snapshot walk.
  • 🐛 [LOW · bug] crates/nono-cli/src/rollback_preflight.rs:227 — In exceeds_file_threshold, entry.path().is_file() is used to count files, which follows symlinks to their targets. This causes symlinked files inside a directory to be counted toward the preflight budget limits, despite being ignored during snapshot walking. Update the check to ensure symlinks are skipped (e.g. by checking !entry.file_type().is_symlink() && entry.path().is_file()) to match the snapshot walk logic.

Automated code + security review. CI already covers clippy, rustfmt, tests, cargo-audit and commit-lint.

@lukehinds
lukehinds self-requested a review July 27, 2026 08:29
@lukehinds lukehinds added this to the 0.70 milestone Jul 27, 2026
@SequeI

SequeI commented Jul 27, 2026

Copy link
Copy Markdown
Member

Once the nogent security issues are addressed, we can review and likely merge!

kurtisc added 5 commits July 27, 2026 15:00
Asserts create_baseline keeps a symlink to a large file within the rollback
budget and omits the link from the tracked file set. Fails until the
follow-symlink fix lands.

Signed-off-by: Kurtis Charnock <kurtis@nolabs.ai>
Excludes symlink entries from walk_and_store and walk_current via the
file type WalkDir already lstat'd, and guards symlinked tracked roots.
Keeps both walks in agreement so a symlink never surfaces as a spurious
Created/Deleted change. Captures a symlink target inside a write grant
via the real parent during the crawl.

Signed-off-by: Kurtis Charnock <kurtis@nolabs.ai>
Covers symlinks created after the baseline: a link to a file outside the
tracked roots, and a dangling link. Both survive restore_to, since
walk_current skips symlinks entirely and the cleanup loop only iterates
what that walk returned. The dry-run preview misses them for the same
reason.

Signed-off-by: Kurtis Charnock <kurtis@nolabs.ai>
Walks return files and symlinks separately, and SnapshotManifest records
the link paths present at snapshot time. Restore unlinks any link absent
from the manifest and leaves the rest alone; a path the manifest holds as
a file is left to the restore loop, which replaces it via atomic rename.
Link targets are not stored, so a deleted link is not recreated.

Cleanup validates the restore target first, so a parent component swapped
for a symlink cannot redirect the unlink outside the tracked root. The
manifest field defaults on load, so sessions written before this change
still open.

Signed-off-by: Kurtis Charnock <kurtis@nolabs.ai>
exists, is_dir, and is_file all follow links, so preflight traversed
symlinked tracked roots, reported symlinked children as heavy
directories, and counted links toward the size threshold. The last of
those is the reproducer from the linked issue: a directory of redundant
links no longer aborts the walk, but still gets offered for exclusion.
An is_symlink helper guards each check, matching the snapshot walk.

exceeds_file_threshold takes the limit as an argument so the counting
logic can be covered without creating 10,001 files.

Signed-off-by: Kurtis Charnock <kurtis@nolabs.ai>
@SequeI

SequeI commented Jul 27, 2026

Copy link
Copy Markdown
Member

/nogent review

@SequeI SequeI left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thank you!

@SequeI
SequeI merged commit d322771 into main Jul 27, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rollback feature's size counter follows symlinks (and doesn't deduplicate inodes or resolved symlink paths)

4 participants