Skip to content

Add --restore flag to apply command for clean package restoration#9

Merged
nomyfan merged 12 commits into
mainfrom
claude/eager-gates-QTmY1
May 30, 2026
Merged

Add --restore flag to apply command for clean package restoration#9
nomyfan merged 12 commits into
mainfrom
claude/eager-gates-QTmY1

Conversation

@nomyfan

@nomyfan nomyfan commented May 26, 2026

Copy link
Copy Markdown
Owner

Summary

This PR adds a --restore flag to the apply command that allows users to restore a package to its clean state before applying patches. This is useful when the environment may already contain a previous version of the patch or manual edits.

Key Changes

  • New --restore parameter in apply_patch() function: Added optional env_path and restore parameters to the apply_patch() function. When restore=True, the function calls restore_clean_package() before attempting to apply the patch.

  • CLI flag support: Added --restore argument to the apply subcommand that passes the flag through to apply_patch().

  • Validation: Added validation to ensure env_path is provided when restore=True, raising a ValueError if missing.

  • Improved error handling: Modified error logging to distinguish between:

    • "already applied" warnings when restore=False (existing behavior)
    • "Failed to apply patch" errors when restore=True and the patch fails after restoration (new behavior)
  • Documentation updates: Updated SKILL.md to document the new --restore flag and clarified terminology around "restore" vs "reinstall" throughout the documentation.

Implementation Details

  • The restore operation happens before the dry-run patch check, ensuring the package is in a clean state before validation
  • The dry-run and actual patch application still occur after restoration (3 subprocess calls total when restore is enabled)
  • Error messages are context-aware: when a patch fails after restoration, it's logged as an error rather than assuming it was already applied
  • The --restore flag is optional and defaults to False to maintain backward compatibility

https://claude.ai/code/session_01UUAdSdSQWt6eAP11HMQU3J

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fe813505a0

ℹ️ 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".

Comment thread patch_package_py/core.py

Copilot AI 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.

Pull request overview

This PR introduces a --restore option for the p12y apply workflow to restore (force-reinstall) a package to a clean state before applying patch files, which helps recover from previously-applied patches or manual edits in the environment.

Changes:

  • Added --restore flag to the apply subcommand and plumbed it through to core.apply_patch().
  • Extended apply_patch() with restore/env_path parameters and a call to restore_clean_package() when enabled.
  • Updated documentation and added unit tests covering the new restore behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
patch_package_py/core.py Adds restore-aware behavior to apply_patch() (including new parameters and error logging).
patch_package_py/cli.py Adds --restore CLI flag and passes it into apply_patch().
skills/patch-package-py/SKILL.md Documents --restore and updates “reinstall” → “restore” terminology.
tests/test_core.py Adds tests validating restore invocation and related error handling.
tests/test_e2e.py Formatting-only changes (line wrapping).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread patch_package_py/core.py
Comment thread patch_package_py/core.py Outdated
…store mode

- Move restore_clean_package call before resolve/version checks so --restore
  can recover a missing or version-drifted package
- Raise RuntimeError (non-zero exit) when dry-run fails after restore instead
  of silently returning

https://claude.ai/code/session_01UUAdSdSQWt6eAP11HMQU3J

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Comment thread patch_package_py/core.py Outdated
Comment thread patch_package_py/core.py
Comment on lines 374 to 376
logger.warning(
f"Patch `{patch_name}` appears to be already applied, skipping...",
)
Comment thread tests/test_core.py Outdated
nomyfan and others added 2 commits May 26, 2026 23:55
When restore=True and the real patch application fails (after a
successful dry-run), raise RuntimeError instead of leaking a raw
CalledProcessError. Adds test coverage for this scenario.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The restore happy path test now uses a real venv and real patch command
instead of mocking subprocess. Error branch tests (missing env_path,
dry-run failure, real apply failure) stay in unit tests since they need
to simulate specific failure scenarios.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

patch_package_py/core.py:396

  • Same as above: from None drops the original CalledProcessError context on real apply failures when restore=True, making it harder to debug why patch application failed.
    except subprocess.CalledProcessError:
        if restore:
            raise RuntimeError(
                f"Failed to apply patch `{patch_name}` after restoring clean package."
            ) from None

Comment thread patch_package_py/core.py Outdated
Comment thread patch_package_py/core.py Outdated
nomyfan and others added 3 commits May 27, 2026 00:10
…n change

Previously --restore ran before version validation, which meant it would
force-reinstall the version from the patch filename regardless of what
the project actually had installed. Now version mismatch is caught first,
and restore only reinstalls when the versions already match.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove `from None` so the underlying CalledProcessError is chained,
making failures easier to diagnose.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
apply_patch now derives site_packages_dir internally via find_site_packages,
eliminating the redundant parameter that callers always computed from env_path.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Comment thread patch_package_py/cli.py
Comment on lines +106 to +110
apply_patch(
patch_file,
env_path,
restore=args.restore,
)
Comment thread tests/test_e2e.py Outdated
Comment thread tests/test_e2e.py
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@nomyfan nomyfan merged commit e510a43 into main May 30, 2026
18 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.

3 participants