feat(blueprint): add snapshot prune/delete commands for retention management#5453
Open
vasanth53 wants to merge 6 commits into
Open
feat(blueprint): add snapshot prune/delete commands for retention management#5453vasanth53 wants to merge 6 commits into
vasanth53 wants to merge 6 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughTwo new exported functions, ChangesSnapshot Retention Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nemoclaw/src/blueprint/snapshot.ts`:
- Around line 329-346: The pruneSnapshots function silently excludes snapshot
paths when deleteSnapshot returns false, leaving them out of both the deleted
and kept arrays, which hides failures from users. Add a failed array to track
snapshot paths where deleteSnapshot returns false, and include it in the
returned object alongside deleted and kept. This gives users visibility into
which specific snapshots failed to delete and why the counts might not add up to
the total.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c471987d-87b6-4b91-bb33-a58a59186f72
📒 Files selected for processing (5)
ci/test-file-size-budget.jsonnemoclaw/src/blueprint/runner.test.tsnemoclaw/src/blueprint/runner.tsnemoclaw/src/blueprint/snapshot.test.tsnemoclaw/src/blueprint/snapshot.ts
vasanth53
force-pushed
the
feat/snapshot-retention-management
branch
from
66dc1d2 to
337c93b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nemoclaw/src/blueprint/snapshot.ts (1)
319-327: ⚡ Quick winConsider validating that
snapshotPathis withinSNAPSHOTS_DIR.
rejectSymlinksOnPathguards against symlink attacks under HOME but doesn't restrict deletion to the snapshots directory. A user could inadvertently (or maliciously via a compromised script) runsnapshots delete --path ~/.sshand delete unrelated directories.Adding a prefix check provides defense-in-depth:
🛡️ Suggested validation
export function deleteSnapshot(snapshotPath: string): boolean { try { + const resolved = resolve(snapshotPath); + if (!resolved.startsWith(SNAPSHOTS_DIR + '/') && resolved !== SNAPSHOTS_DIR) { + return false; + } rejectSymlinksOnPath(snapshotPath); rmSync(snapshotPath, { recursive: true, force: true }); return true; } catch { return false; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemoclaw/src/blueprint/snapshot.ts` around lines 319 - 327, The deleteSnapshot function currently only guards against symlinks but doesn't restrict path deletion to the SNAPSHOTS_DIR directory, allowing arbitrary directory deletion. Add a validation check in deleteSnapshot that ensures snapshotPath is within SNAPSHOTS_DIR before proceeding with rejectSymlinksOnPath and rmSync calls. If the path is outside SNAPSHOTS_DIR, return false to prevent the deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nemoclaw/src/blueprint/snapshot.ts`:
- Around line 329-333: The pruneSnapshots function does not validate the keep
parameter for negative values, and when keep is negative, the slice(keep)
operation will behave unexpectedly by returning elements from the end of the
array. Add a defensive guard at the beginning of the pruneSnapshots function to
either throw an error for invalid negative values or clamp the keep value to a
minimum of 0 to ensure predictable behavior.
---
Nitpick comments:
In `@nemoclaw/src/blueprint/snapshot.ts`:
- Around line 319-327: The deleteSnapshot function currently only guards against
symlinks but doesn't restrict path deletion to the SNAPSHOTS_DIR directory,
allowing arbitrary directory deletion. Add a validation check in deleteSnapshot
that ensures snapshotPath is within SNAPSHOTS_DIR before proceeding with
rejectSymlinksOnPath and rmSync calls. If the path is outside SNAPSHOTS_DIR,
return false to prevent the deletion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4406e8b8-b940-40ca-bc6a-54f757bce558
📒 Files selected for processing (5)
ci/test-file-size-budget.jsonnemoclaw/src/blueprint/runner.test.tsnemoclaw/src/blueprint/runner.tsnemoclaw/src/blueprint/snapshot.test.tsnemoclaw/src/blueprint/snapshot.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- ci/test-file-size-budget.json
- nemoclaw/src/blueprint/snapshot.test.ts
- nemoclaw/src/blueprint/runner.test.ts
- nemoclaw/src/blueprint/runner.ts
Comment on lines
+329
to
+333
| export function pruneSnapshots(keep: number): { deleted: string[]; kept: string[]; failed: string[] } { | ||
| const snapshots = listSnapshots(); | ||
| if (snapshots.length <= keep) { | ||
| return { deleted: [], kept: snapshots.map((s) => s.path), failed: [] }; | ||
| } |
Contributor
There was a problem hiding this comment.
Handle negative keep values defensively.
If keep is negative, slice(keep) returns elements from the end of the array, producing unexpected behavior (e.g., pruneSnapshots(-1) would keep all but the oldest snapshot instead of rejecting the input).
Consider clamping or throwing early:
🛡️ Suggested guard
export function pruneSnapshots(keep: number): { deleted: string[]; kept: string[]; failed: string[] } {
+ if (keep < 0) {
+ throw new Error(`Invalid keep count: ${keep}. Must be a non-negative integer.`);
+ }
const snapshots = listSnapshots();
if (snapshots.length <= keep) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nemoclaw/src/blueprint/snapshot.ts` around lines 329 - 333, The
pruneSnapshots function does not validate the keep parameter for negative
values, and when keep is negative, the slice(keep) operation will behave
unexpectedly by returning elements from the end of the array. Add a defensive
guard at the beginning of the pruneSnapshots function to either throw an error
for invalid negative values or clamp the keep value to a minimum of 0 to ensure
predictable behavior.
…agement Add deleteSnapshot and pruneSnapshots to the blueprint snapshot module so users can clean up accumulated migration snapshots under ~/.nemoclaw/snapshots/. - deleteSnapshot(path): remove a single snapshot directory, rejecting symlinks - pruneSnapshots(keep): keep N most recent, delete the rest, report failures - Wire through CLI as 'snapshots list', 'snapshots prune --keep N', and 'snapshots delete --path <path>' subcommands - Test CLI dispatch in snapshot.test.ts to stay under test file size budget - 20 new tests covering snapshot CLI and edge cases Signed-off-by: vasanth53 <vasanth@peak42.in>
vasanth53
force-pushed
the
feat/snapshot-retention-management
branch
from
337c93b to
5e76755
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nemoclaw/src/blueprint/runner.ts`:
- Around line 1092-1104: The delete command accepts a user-provided --path
argument that is passed directly to deleteSnapshot without validation, allowing
deletion of arbitrary directories outside the snapshots directory. After
extracting snapshotPath from argv (around line 1095), add validation to ensure
the path is constrained to the snapshots directory by resolving it to an
absolute path and verifying it starts with the expected snapshots directory path
(typically ~/.nemoclaw/snapshots). Reject the command with an appropriate error
message if the resolved path attempts to escape the snapshots directory. This
validation must occur before the deleteSnapshot function is called to prevent
unauthorized recursive deletion of directories outside the snapshots location.
- Around line 1065-1067: The `Number.parseInt` function at the keep variable
assignment performs lenient parsing and will accept malformed inputs like "3abc"
or "1.5" by silently truncating them to 3 and 1 respectively. To enforce the
"non-negative integer" requirement strictly, validate that the entire input
string represents a pure integer with no trailing characters or decimal points.
You can do this by checking that the parsed keep value, when converted back to a
string, matches the original trimmed input value, or by validating the input
format using a regex pattern that matches only optional whitespace and
non-negative integer digits before attempting to parse.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bd9bfb48-d9d7-47f7-8974-9fc2e082f74f
📒 Files selected for processing (3)
nemoclaw/src/blueprint/runner.tsnemoclaw/src/blueprint/snapshot.test.tsnemoclaw/src/blueprint/snapshot.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- nemoclaw/src/blueprint/snapshot.ts
- nemoclaw/src/blueprint/snapshot.test.ts
Comment on lines
+1065
to
+1067
| keep = Number.parseInt(val, 10); | ||
| if (!Number.isFinite(keep) || keep < 0) { | ||
| throw new Error("--keep must be a non-negative integer"); |
Contributor
There was a problem hiding this comment.
--keep validation accepts malformed values.
Line 1065 uses Number.parseInt, so inputs like --keep 3abc or --keep 1.5 are accepted as 3 / 1, despite the “non-negative integer” requirement.
Suggested fix
- keep = Number.parseInt(val, 10);
- if (!Number.isFinite(keep) || keep < 0) {
+ if (!/^\d+$/.test(val)) {
+ throw new Error("--keep must be a non-negative integer");
+ }
+ keep = Number.parseInt(val, 10);
+ if (!Number.isSafeInteger(keep) || keep < 0) {
throw new Error("--keep must be a non-negative integer");
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nemoclaw/src/blueprint/runner.ts` around lines 1065 - 1067, The
`Number.parseInt` function at the keep variable assignment performs lenient
parsing and will accept malformed inputs like "3abc" or "1.5" by silently
truncating them to 3 and 1 respectively. To enforce the "non-negative integer"
requirement strictly, validate that the entire input string represents a pure
integer with no trailing characters or decimal points. You can do this by
checking that the parsed keep value, when converted back to a string, matches
the original trimmed input value, or by validating the input format using a
regex pattern that matches only optional whitespace and non-negative integer
digits before attempting to parse.
Comment on lines
+1092
to
+1104
| let snapshotPath: string | undefined; | ||
| for (let i = 1; i < argv.length; i++) { | ||
| if (argv[i] === "--path") { | ||
| snapshotPath = argv[++i]; | ||
| } | ||
| } | ||
| if (!snapshotPath) throw new Error("--path is required for delete"); | ||
|
|
||
| if (deleteSnapshot(snapshotPath)) { | ||
| log(`Deleted snapshot: ${snapshotPath}`); | ||
| } else { | ||
| throw new Error(`Failed to delete snapshot: ${snapshotPath}`); | ||
| } |
Contributor
There was a problem hiding this comment.
delete --path allows recursive deletion outside snapshots directory.
Lines 1092-1104 pass user-provided --path straight into recursive deletion. Without constraining it to ~/.nemoclaw/snapshots, this can remove arbitrary directories the user has permissions for.
Suggested fix
-import { join, sep } from "node:path";
+import { join, resolve, sep } from "node:path";
...
case "delete": {
let snapshotPath: string | undefined;
for (let i = 1; i < argv.length; i++) {
if (argv[i] === "--path") {
snapshotPath = argv[++i];
}
}
if (!snapshotPath) throw new Error("--path is required for delete");
+
+ const snapshotsRoot = resolve(join(homedir(), ".nemoclaw", "snapshots"));
+ const targetPath = resolve(snapshotPath);
+ if (!targetPath.startsWith(snapshotsRoot + sep)) {
+ throw new Error(`--path must be under ${snapshotsRoot}`);
+ }
- if (deleteSnapshot(snapshotPath)) {
- log(`Deleted snapshot: ${snapshotPath}`);
+ if (deleteSnapshot(targetPath)) {
+ log(`Deleted snapshot: ${targetPath}`);
} else {
- throw new Error(`Failed to delete snapshot: ${snapshotPath}`);
+ throw new Error(`Failed to delete snapshot: ${targetPath}`);
}
break;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nemoclaw/src/blueprint/runner.ts` around lines 1092 - 1104, The delete
command accepts a user-provided --path argument that is passed directly to
deleteSnapshot without validation, allowing deletion of arbitrary directories
outside the snapshots directory. After extracting snapshotPath from argv (around
line 1095), add validation to ensure the path is constrained to the snapshots
directory by resolving it to an absolute path and verifying it starts with the
expected snapshots directory path (typically ~/.nemoclaw/snapshots). Reject the
command with an appropriate error message if the resolved path attempts to
escape the snapshots directory. This validation must occur before the
deleteSnapshot function is called to prevent unauthorized recursive deletion of
directories outside the snapshots location.
wscurran
added
area: cli
Contributor
|
✨ Thanks for adding the Related open issues: |
1 similar comment
Contributor
|
✨ Thanks for adding the Related open issues: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
snapshots list,snapshots prune --keep <N>, andsnapshots delete --path <path>subcommands to the blueprint runner, letting users manage disk space consumed by accumulated migration snapshots under~/.nemoclaw/snapshots/.Changes
nemoclaw/src/blueprint/snapshot.tsdeleteSnapshot(),pruneSnapshots()with symlink-attack rejectionnemoclaw/src/blueprint/snapshot.test.tsnemoclaw/src/blueprint/runner.tssnapshotsaction withlist,prune,deletesubcommands and usage textnemoclaw/src/blueprint/runner.test.tslstatSync,readlinkSync,rmSyncci/test-file-size-budget.jsonUsage
nemoclaw blueprint snapshots list nemoclaw blueprint snapshots prune --keep 5 nemoclaw blueprint snapshots delete --path ~/.nemoclaw/snapshots/20260101T000000ZTesting
All 503 plugin tests pass. All hooks pass (pre-commit, commitlint). New code adds 20 tests across two test files.
Checklist
feat(blueprint):)mainCloses #5452
Summary by CodeRabbit