fix(cli): harden calculate_dir_size against silent errors and symlink cycles by Ayush7614 · Pull Request #1862 · nolabs-ai/nono

Ayush7614 · 2026-09-11T09:53:59Z

Linked Issue

Summary

Both crates/nono-cli/src/audit_session.rs:273 and crates/nono-cli/src/rollback_session.rs:195 sized session directories via:

WalkDir::new(dir).into_iter().filter_map(|e| e.ok()).filter_map(|e| e.metadata().ok()).filter(|m| m.is_file()).map(|m| m.len()).sum()

filter_map(ok) silently dropped permission-denied / I/O errors → size undercount, so audit cleanup --max-total-size could keep more than the operator's budget.
No follow_links(false) or max_open cap — a symlink loop (potentially planted via sandbox) could traverse until OS limits before being discarded, consuming CPU on every discover_sessions.

Fix hardens both helpers: WalkDir::new(dir).follow_links(false).max_open(128), logs skips via tracing::warn! instead of dropping, and uses saturating_add for total. Symlinked dirs are not descended; broken symlinks and loops are logged and skipped without double-counting.

Files:

crates/nono-cli/src/audit_session.rs:273-304 — hardened + 5 new tests
crates/nono-cli/src/rollback_session.rs:195-226 — hardened + 3 new tests

Agent Disclosure

Generated by AI assistant (Muse Spark via OpenCode). Audited calculate_dir_size in both modules, compared with collect_symlink_hops MAX_SYMLINKS handling, verified no duplicate open issue (#1819 is the feature, not its budget bug). No NEP required — CLI-only behavior fix.

Test Plan

cargo test -p nono-cli --bin nono -- calculate_dir_size — 9 passed:
- counts_regular_files, empty_dir_is_zero (audit)
- does_not_follow_symlinked_dir (both), handles_broken_symlink (both), handles_symlink_cycle_without_looping (both)
- existing calculate_dir_size_works still passes
cargo test -p nono-cli --bin nono — 2084 passed, 0 failed, 11 ignored
cargo clippy -p nono-cli -- -D warnings -D clippy::unwrap_used — clean
cargo fmt --check — clean

Checklist

An issue exists and is linked above (fix(cli): calculate_dir_size silently ignores I/O errors and symlink loops, mis-budgeting audit cleanup #1858)
All commits are signed-off (DCO)
All new code follows coding standards (CLAUDE.md) and is covered by tests
Public-facing changes paired with docs (N/A)
NEP not required (CLI-only)

… cycles WalkDir sizing previously used filter_map(ok) which silently dropped permission-denied / I/O errors and left symlink-following at the default. This undercounted audit/rollback disk usage, so audit cleanup --max-total-size could keep more than the operator's budget, and a planted symlink loop could cause WalkDir traversal until OS limits before being discarded. Harden both audit_session::calculate_dir_size and rollback_session::calculate_dir_size: disable symlink following, cap open FDs at 128, and log skips via tracing::warn instead of dropping. Use saturating_add for total. Add 8 tests covering regular files, empty dir, broken symlink, symlinked dir not followed, and symlink cycle termination. Closes nolabs-ai#1858 Signed-off-by: Ayush7614 <ayushknj3@gmail.com>

github-actions · 2026-09-11T09:54:16Z

PR Review Summary

Size

Metric	Value
Lines added	+112
Lines removed	-42
Total changed	154
Classification	Medium (50–300 lines)

Affected crates

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.}

nogent-nolabs-ai

nogent code review

2 correctness and security concerns, 1 design concern.

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

nogent-nolabs-ai · 2026-09-11T09:56:06Z

+                continue;
+            }
+        };
+        let metadata = match entry.metadata() {


Warning
🔒 [MEDIUM · security] The helper calculate_dir_size uses entry.metadata() which resolves symbolic links to their targets, even when WalkDir is configured with follow_links(false). If a symlink points to a file, metadata.is_file() returns true and its target length is added to total. This can lead to double-counting of symlinked files or incorrect size attribution for files located outside the session directory. To fix this, skip non-files by checking if !entry.file_type().is_file() { continue; } before retrieving metadata.

nogent-nolabs-ai · 2026-09-11T09:56:06Z

+                continue;
+            }
+        };
+        let metadata = match entry.metadata() {


Warning
🔒 [MEDIUM · security] The helper calculate_dir_size uses entry.metadata() which resolves symbolic links to their targets, even when WalkDir is configured with follow_links(false). If a symlink points to a file, metadata.is_file() returns true and its target length is added to total. This can lead to double-counting of symlinked files or incorrect size attribution for files located outside the session directory. To fix this, skip non-files by checking if !entry.file_type().is_file() { continue; } before retrieving metadata.

nogent-nolabs-ai · 2026-09-11T09:56:06Z

@@ -194,13 +194,36 @@ fn is_process_alive(pid: u32) -> bool {

 /// Calculate the total size of all files in a directory tree.
 fn calculate_dir_size(dir: &Path) -> u64 {


Note
🏗️ [LOW · design] The calculate_dir_size helper logic is duplicated between crates/nono-cli/src/audit_session.rs and crates/nono-cli/src/rollback_session.rs. Consider consolidating this helper into a single shared function under a common utility module such as crates/nono-cli/src/state_paths.rs to avoid duplication.

- Check file_type().is_file() before metadata() so symlink-to-file entries are not counted via metadata() follow (which resolves external targets and double-counts). With follow_links(false), file_type correctly distinguishes symlink vs regular file. - Extract shared helper state_paths::calculate_dir_size and have audit_session and rollback_session delegate to it, removing duplication (LOW design). - Add tests for symlink-to-file outside dir not counted in both modules; now 11 calculate_dir_size tests pass. Fixes nogent MEDIUM security (metadata follows symlink) and LOW design (duplication) on nolabs-ai#1862. Signed-off-by: Ayush7614 <ayushknj3@gmail.com>

Ayush7614 · 2026-09-11T10:04:20Z

Addressed nogent review on b2d60d6:

Added file_type().is_file() pre-check before metadata() so symlink-to-file entries are not counted via metadata() follow (fixes MEDIUM security double-count).
Extracted shared helper state_paths::calculate_dir_size (follow_links(false), max_open(128), warn on errors) and delegated from both audit_session and rollback_session to remove duplication (LOW design).
Added tests ignores_symlink_to_file_outside in both modules (now 11 tests).

New commit 94e250e pushed; clippy clean, 2086 tests pass.

SequeI · 2026-09-14T10:51:51Z

@@ -194,13 +193,7 @@ fn is_process_alive(pid: u32) -> bool {

 /// Calculate the total size of all files in a directory tree.
 fn calculate_dir_size(dir: &Path) -> u64 {


Why do we have calculate_dir_size in rollback_session if you already created a shared helper in state_paths ??

Good catch — that wrapper was leftover from the dedup commit. Removed the thin calculate_dir_size wrappers from both rollback_session.rs and audit_session.rs; call sites now use state_paths::calculate_dir_size directly, and the duplicated tests are consolidated into a single suite in state_paths. Also merged upstream/main so the branch is up to date. All checks pass (2093 tests, clippy, fmt).

Remove thin calculate_dir_size wrappers in audit_session and rollback_session that only delegated to the shared helper. Call sites now use state_paths::calculate_dir_size directly. Consolidate the duplicated size tests into a single suite in state_paths, removing the per-module copies. Addresses SequeI review on nolabs-ai#1862. Signed-off-by: Ayush7614 <ayushknj3@gmail.com>

Signed-off-by: Ayush7614 <ayushknj3@gmail.com>

SequeI

lgtm, thanks

Ayush7614 mentioned this pull request Sep 11, 2026

fix(cli): calculate_dir_size silently ignores I/O errors and symlink loops, mis-budgeting audit cleanup #1858

Closed

2 tasks done

github-actions Bot added bug Something isn't working nono-cli size/medium labels Sep 11, 2026

nogent-nolabs-ai Bot reviewed Sep 11, 2026

View reviewed changes

github-actions Bot added size/medium and removed size/medium labels Sep 11, 2026

SequeI requested changes Sep 14, 2026

View reviewed changes

github-actions Bot added size/medium and removed size/medium labels Sep 14, 2026

Merge upstream main into fix/calculate-dir-size-error-handling-1858

d288de5

Signed-off-by: Ayush7614 <ayushknj3@gmail.com>

github-actions Bot added size/medium and removed size/medium labels Sep 14, 2026

SequeI approved these changes Sep 14, 2026

View reviewed changes

SequeI merged commit 6c1064b into nolabs-ai:main Sep 14, 2026
17 checks passed

BrewTestBot mentioned this pull request Sep 16, 2026

nono 0.78.0 Homebrew/homebrew-core#311540

Merged

scottames-github-bot Bot mentioned this pull request Sep 23, 2026

chore(deps): update ⬆️ mise-packages scottames/dots#1074

Merged

1 task

Navigation Menu

Repository navigation

fix(cli): harden calculate_dir_size against silent errors and symlink cycles - #1862

fix(cli): harden calculate_dir_size against silent errors and symlink cycles#1862
SequeI merged 4 commits into
nolabs-ai:mainfrom
Ayush7614:fix/calculate-dir-size-error-handling-1858

Ayush7614 commented Sep 11, 2026

Uh oh!

github-actions Bot commented Sep 11, 2026 •

edited

Loading

Uh oh!

nogent-nolabs-ai Bot left a comment

Uh oh!

nogent-nolabs-ai Bot Sep 11, 2026

Uh oh!

nogent-nolabs-ai Bot Sep 11, 2026

Uh oh!

nogent-nolabs-ai Bot Sep 11, 2026

Uh oh!

Ayush7614 commented Sep 11, 2026

Uh oh!

SequeI Sep 14, 2026 •

edited

Loading

Uh oh!

Ayush7614 Sep 14, 2026

Uh oh!

SequeI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -194,13 +194,36 @@ fn is_process_alive(pid: u32) -> bool {

		/// Calculate the total size of all files in a directory tree.
		fn calculate_dir_size(dir: &Path) -> u64 {

		@@ -194,13 +193,7 @@ fn is_process_alive(pid: u32) -> bool {

		/// Calculate the total size of all files in a directory tree.
		fn calculate_dir_size(dir: &Path) -> u64 {

Conversation

Ayush7614 commented Sep 11, 2026

Linked Issue

Summary

Agent Disclosure

Test Plan

Checklist

Uh oh!

github-actions Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Summary

Size

Affected crates

Blast radius — Contained

Uh oh!

nogent-nolabs-ai Bot left a comment

Choose a reason for hiding this comment

nogent code review

Uh oh!

nogent-nolabs-ai Bot Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

nogent-nolabs-ai Bot Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

nogent-nolabs-ai Bot Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

Ayush7614 commented Sep 11, 2026

Uh oh!

SequeI Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayush7614 Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

SequeI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Sep 11, 2026 •

edited

Loading

SequeI Sep 14, 2026 •

edited

Loading