Skip to content

fix: ignore relative snapshot base overrides#2976

Merged
seratch merged 4 commits intoopenai:mainfrom
matthewflint:fix/sandbox-snapshot-base-dir-env-paths
Apr 21, 2026
Merged

fix: ignore relative snapshot base overrides#2976
seratch merged 4 commits intoopenai:mainfrom
matthewflint:fix/sandbox-snapshot-base-dir-env-paths

Conversation

@matthewflint
Copy link
Copy Markdown
Contributor

@matthewflint matthewflint commented Apr 20, 2026

Summary

  • ignore relative XDG_STATE_HOME, LOCALAPPDATA, and APPDATA overrides
  • keep the default local snapshot base dir independent from the current working directory
  • add regression coverage for the relative-path cases

Testing

  • uv run pytest -s tests/sandbox/test_snapshot_defaults.py
  • uv run ruff check src/agents/sandbox/snapshot_defaults.py tests/sandbox/test_snapshot_defaults.py

Signed-off-by: matthewflint 277024436+matthewflint@users.noreply.github.com

Only accept absolute environment overrides for the default local snapshot base directory so snapshot storage does not become cwd-dependent. Add regression coverage for relative XDG and Windows env paths.
@seratch seratch changed the title Ignore relative snapshot base overrides fix: ignore relative snapshot base overrides Apr 20, 2026
@github-actions github-actions Bot added the bug Something isn't working label Apr 20, 2026
Copy link
Copy Markdown
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small concern before merging: the direction looks right, but the Windows-specific coverage is still a bit indirect.

The new tests exercise the os_name="nt" branch, but they use POSIX-style absolute paths from tmp_path. That means we are not actually proving that a Windows-style env value such as C:\Users\me\AppData\Local is treated as absolute when platform/os_name are injected for a simulated Windows case.

If platform / os_name injection is intended to support cross-platform unit tests, we may want either:

  • a test that covers Windows-style absolute env paths explicitly, or
  • branch-specific path handling using PureWindowsPath for the Windows env vars.

Not blocking from my side, but I think this would make the regression coverage more precise.

@seratch seratch added this to the 0.14.x milestone Apr 20, 2026
@matthewflint
Copy link
Copy Markdown
Contributor Author

Yes, that makes sense. I updated the os_name == "nt" branch to use PureWindowsPath(...).is_absolute() for LOCALAPPDATA / APPDATA, and switched the tests to explicit Windows-style values like C:\Users\me\AppData\Local.

I also added coverage for APPDATA fallback, the all-relative fallback, and a guard that /tmp/... is ignored under simulated Windows.

@seratch seratch merged commit 106ef05 into openai:main Apr 21, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working feature:sandboxes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants