Skip to content

Add named and scoped checkpoint restore - #300

Merged
Surendra Goutham (is-goutham) merged 7 commits into
mainfrom
users/gouthams/pr290-01-checkpoint
Sep 24, 2026
Merged

Surendra Goutham (is-goutham) merged 7 commits into
mainfrom
users/gouthams/pr290-01-checkpoint

Conversation

@is-goutham

Copy link
Copy Markdown
Contributor

Summary

Adds precise checkpoint restoration needed by resumable integration setup:

  • restores the newest checkpoint matching an exact reason
  • optionally restores only a selected path or glob
  • rejects path traversal and unmatched restore patterns
  • preserves the current workspace in an automatic checkpoint before restoration

This is replacement PR 1 of the PR #290 split.

Validation

  • python -m pytest tests/scripts/test_checkpoint.py -q — 5 passed
  • git diff --check

@nkemms

nkemms commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Verdict: request changes

F-1 [MAJOR] — solutions/ess-maker-skills/scripts/checkpoint.py:222-231, 307-327 — --only failures leave a stray checkpoint and crash the CLI
The named-restore flow creates the auto-save checkpoint before validating only, and main() does not catch the ValueError from restore_matching(). A bad or non-matching glob therefore leaves behind an unexpected checkpoint and exits with a traceback instead of the script's normal ERROR:/sys.exit(1) path.

Fix: validate --only before creating the auto-save checkpoint, and surface restore errors through the existing CLI error handling.

@is-goutham

Copy link
Copy Markdown
Contributor Author

Fixed in 6972470. Scoped restore patterns are now fully validated before the safety checkpoint is created, so traversal and no-match failures leave no stray checkpoint. The CLI also catches these validation errors and reports the normal ERROR message with exit code 1 instead of a traceback. Added regressions for both no-side-effect failures and the CLI error path.

@nkemms

nkemms commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Updated review findings — merge blockers remain

  • Critical — scoped restore can delete the agent root. --only "." and root-normalizing inputs such as topics/.. resolve to the agent directory itself, delete the full tree (including recovery state), and report success. Reject any scope that resolves to the restore root.
  • Critical — filesystem links can escape the workspace. Textual abspath/commonpath checks do not prevent a directory junction beneath the agent tree from targeting files outside it. Resolve the real target through existing ancestors and require real-path containment before deleting or writing.
  • Major — recovery state is selectable as payload. Explicit scopes such as .checkpoints and .baseline can delete protected recovery data. Exclude these paths and their descendants from scoped restore targets.
  • Major — valid non-object metadata crashes lookup. JSON such as null reaches object-field access and raises AttributeError. Validate that decoded metadata is an object and return the normal invalid-checkpoint error otherwise.

@nkemms

nkemms commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Re-reviewed the current head (6972470) against main. The earlier containment fixes are incomplete, so this is still not ready to merge.

  • Scoped restore can delete the agent root and recovery state — solutions/ess-maker-skills/scripts/checkpoint.py:108-153: patterns such as ** can include the root (.), and .checkpoints/.baseline are not excluded from scoped deletion. This can remove the live agent directory and the safety checkpoint while still reporting success. Reject root matches and all protected recovery paths before deletion/copying, with regressions for ., **, and protected-directory patterns.
  • Filesystem containment is lexical rather than physical — checkpoint.py:59-70,114-153: checkpoint creation dereferences reparse points, and scoped restore can follow a junction parent when writing a selected file. Reject/preserve reparse points and verify resolved source and target parents remain beneath their intended roots.

The new traversal validation and malformed-metadata handling do address those portions of the earlier feedback.

@is-goutham

Copy link
Copy Markdown
Contributor Author

Addressed the latest blockers in fe46a88: scoped restore now rejects root/recovery paths and validates physical containment against symlinks/junctions before mutation. Regression suite: 13 checkpoint tests passed.

@nkemms

nkemms commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Re-reviewed the latest head (fe46a88). The previously reported destructive restore issues are fixed: root matches and protected recovery paths are rejected, and scoped operations enforce physical containment around descendant reparse points.

I found no remaining critical blocker. Good to merge.

@nkemms

nkemms commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Follow-up PR: Consider rejecting a reparse point at the configured checkpoint root itself and bringing scoped restore's Windows locked-file handling to parity with the full restore path. These are additional hardening improvements, not blockers for this PR.

@is-goutham
Surendra Goutham (is-goutham) merged commit fd5f679 into main Sep 24, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants