fix(approval): recognize the raw temp-dir alias in verification cleanup - #70415
Open
ildunari wants to merge 1 commit into
Open
fix(approval): recognize the raw temp-dir alias in verification cleanup#70415ildunari wants to merge 1 commit into
ildunari wants to merge 1 commit into
Conversation
_is_verification_artifact_cleanup() only matched the realpath()-resolved temp directory, so on systems where tempfile.gettempdir() returns a top-level alias (e.g. macOS's /tmp, which resolves to /private/tmp) a verification-script cleanup command using the literal, unresolved path was never recognized as safe. Accept the raw (non-realpath) operand as an alternative match, but only when the raw temp dir's parent is the filesystem root — the exemption stays narrow and a non-root symlinked temp dir (arbitrary directory symlink tricks) still requires the canonical realpath form. Added a regression test for the top-level-alias case; the existing symlinked_temp_dir_only_exempts_canonical_target test continues to pass unchanged, confirming non-root symlinks are unaffected.
alt-glitch
added
type/bug
Contributor
|
Thanks for the focused regression fix. The premise remains present on current main: Automated hermes-sweeper review. |
teknium1
added
the
sweeper:blast-contained
2 tasks
15 tasks
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
3 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.
Problem:
_is_verification_artifact_cleanup()(which lets the agent self-clean its ownhermes-verify-*/hermes-ad-hoc-*temp scripts without an approval prompt) only matched therealpath()-resolved temp directory. On systems wheretempfile.gettempdir()returns a top-level alias — e.g. macOS's/tmp, which resolves to/private/tmp— a cleanup command using the literal path the shell/OS actually reports (/tmp/hermes-verify-foo.py) was never recognized as safe, since it doesn't match the canonical/private/tmp/...form.Fix: accept the raw (non-realpath) operand as an alternative match, but only when the raw temp dir's own parent is the filesystem root (
os.path.dirname(raw_temp_dir) == os.path.abspath(os.sep)). This keeps the exemption narrow: an arbitrary non-root symlinked temp directory (the existingtest_symlinked_temp_dir_only_exempts_canonical_targetregression) still requires the canonical realpath form and is unaffected.Testing done: full
tests/tools/test_approval.py(313 tests) passes, including the existing symlink-alias regression unchanged, plus one new regression test for the top-level-alias case specifically.