fix(acp): use tempfile.gettempdir() for workspace auto-approve (#28262)#28675
Merged
Conversation
#28063 fixed the macOS `/tmp`→`/private/tmp` symlink issue by checking the RAW path (pre-resolve) against startswith('/tmp/'). That works on Linux + macOS but not on Windows — Path('/tmp/foo').resolve() returns C:\\tmp\\foo and isn't the real Windows temp anyway. Replace the hardcoded '/tmp/' prefix with Path(tempfile.gettempdir()). resolve() + Path.relative_to() — same idiom as the cwd branch just below. Works correctly on Linux (/tmp), macOS (/private/var/folders/...), and Windows (%LOCALAPPDATA%\\Temp). Test rewritten to use tempfile.gettempdir() so the assertion exercises the same code path on every platform. Conflict against the just-merged #28063 (raw_path approach) resolved by replacing the whole raw_path block — tempfile.gettempdir() is strictly better than that intermediate fix. Salvage of #28262 by @Zyrixtrex.
5 tasks
Contributor
🔎 Lint report:
|
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.
Salvage of #28262 by @Zyrixtrex. Cross-platform improvement on top of just-merged #28063 (@EloquentBrush0x).
What: #28063 fixed the macOS
/tmp→/private/tmpsymlink resolution issue by checking the RAW path (pre-resolve) againststartswith("/tmp/"). That works on Linux + macOS, but doesn't work on Windows —Path("/tmp/foo").resolve()on Windows returnsC:\\tmp\\foo(and the raw form has the wrong separator anyway), so the branch is unreachable there.How: Replace the hardcoded
"/tmp/"prefix check withPath(tempfile.gettempdir()).resolve()+Path.relative_to()— the same idiom already used three lines below for thecwdbranch. Works correctly on:/tmp/private/var/folders/...(the real per-user temp dir)%LOCALAPPDATA%\\TempConflict against #28063 resolved by replacing the whole raw_path workaround —
tempfile.gettempdir()is strictly better than that intermediate fix and renders the raw_path variable unnecessary. Test rewritten to usetempfile.gettempdir()so the assertion exercises the same code path on every platform.Targeted test
test_workspace_auto_approval_allows_workspace_and_tmp_but_not_sensitivepasses locally.Original PR: #28262