fix(sandbox): harden default snapshot resolution

[GautamSharma99]

Summary

This pull request fixes two default local snapshot path-resolution bugs and removes an unsupported stale-deletion path.

Deterministic environment injection

default_local_snapshot_base_dir() previously selected env or os.environ. An explicitly supplied empty mapping therefore fell back to ambient process state, which made embedded callers and tests depend on host variables such as XDG_STATE_HOME.

The resolver now uses os.environ only when env is None, so env={} is honored as an isolated environment.

Absolute XDG state paths

The POSIX branch previously accepted any non-empty XDG_STATE_HOME, including relative paths. That could place SDK-managed archives below the process working directory. The resolver now accepts the override only when Path(value).is_absolute() and otherwise uses <home>/.local/state, matching the XDG Base Directory requirement and the existing Windows override hardening.

Snapshot retention ownership

The module contained a 30-day stale-archive helper and a resolver now parameter, but production never invoked the helper. Current behavior and regression coverage intentionally retain existing archives during default resolution because an old archive may still back paused or serialized resume state.

This PR takes the non-destructive lifecycle option described in #4059: it removes the unused TTL, deletion helper, time dependency, and dead now argument instead of making an unrelated path-resolution call delete durable resume artifacts. Automatic deletion should only be introduced later through an explicit artifact-cleanup API that can prove ownership and account for backend-specific state.

Explicit caller-provided snapshot specs, archive formats, directory permissions, macOS behavior, and Windows path resolution are unchanged.

Test plan

• Added a regression proving env={} ignores an ambient host XDG_STATE_HOME.
• Added a regression proving relative XDG_STATE_HOME falls back to <home>/.local/state.
• Retained coverage that an absolute XDG path is accepted.
• Retained coverage that resolving the managed directory does not delete an existing stale archive.
• uv run pytest -q tests/sandbox/test_snapshot_defaults.py tests/sandbox/test_session_manager.py (18 passed)
• bash .agents/skills/code-change-verification/scripts/run.sh
  • make format
  • make lint
  • make typecheck
  • make tests

Issue number

Closes #4059

Checks

• I've added new tests, if relevant
• I've run .agents/skills/code-change-verification/scripts/run.sh
• I've confirmed all verification steps pass
• If using Codex, I've run /review before submitting this PR

[seratch]

Please preserve the released snapshot-defaults API from v0.19.1: restore cleanup_stale_default_local_snapshots(), its TTL and test coverage, and the now keyword accepted by resolve_default_local_snapshot_spec().

agents.sandbox.snapshot_defaults is included in the published API reference, so removing these names would break existing direct imports and keyword calls even though the runtime does not invoke cleanup automatically. The existing non-destructive behavior should remain: default snapshot resolution must not automatically delete archives.

Please narrow this PR to the explicit-empty-environment and relative-XDG fixes while preserving the released API. Once that is restored and CI is green, this should be ready for another review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ca8f749000

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Restore released snapshot-default API

For users upgrading from released v0.19.1, code that passes now= to resolve_default_local_snapshot_spec or imports the released cleanup_stale_default_local_snapshots helper from agents.sandbox.snapshot_defaults now fails at call/import time, even though the non-destructive retention change can be implemented while preserving those public entry points by accepting and ignoring now and keeping or deprecating the helper.

Useful? React with 👍 / 👎.

[ErenAta16]

Verified all three points from #4059 against main, then re-ran the same probes on this branch. Every one changes, and the removal is safe to make.

Before, on main:

env={}    -> /host/leaked/openai-agents-python/sandbox/snapshots
env=None  -> /host/leaked/openai-agents-python/sandbox/snapshots      # identical
XDG_STATE_HOME="relative/state" -> relative/state/openai-agents-python/...   # not absolute

An explicit empty mapping and "no mapping given" were indistinguishable, so a caller isolating the environment silently got the host's XDG_STATE_HOME. os.environ if env is None else env separates them, which is the right distinction — {} is a statement about the environment, not the absence of one.

After, on this branch:

env={}                            -> /tmp/h/.local/state/...    # falls back to home, host ignored
env=None                          -> /host/leaked/...           # unchanged, as intended
XDG_STATE_HOME="relative/state"   -> /tmp/h/.local/state/...     # relative value ignored
XDG_STATE_HOME="/abs/state"       -> /abs/state/...              # absolute still honoured

The relative case now matches the XDG spec's "if not absolute, ignore it", and it lines up with what the Windows branch already did — _first_absolute_windows_env_path was checking is_absolute() all along, so this removes an inconsistency between the two platform paths rather than adding a new rule.

On deleting the cleanup helper. I checked whether anything outside its own test used it before agreeing this is the right call: grep across src/, tests/, examples/ and docs/ for cleanup_stale_default_local_snapshots and _DEFAULT_LOCAL_SNAPSHOT_TTL_SECONDS returns nothing on this branch, and sandbox/__init__.py's __all__ never re-exported it. The only production entry point into this module is resolve_default_local_snapshot_spec, called once from runtime_session_manager.py:506 with no arguments. So dropping it isn't removing a public API, it's removing a function that existed only to be tested — and shipping a 30-day TTL that never runs is worse than not having one, because it reads like a guarantee.

tests/sandbox/test_snapshot_defaults.py is at 9 passed on the branch.

One thing worth deciding explicitly rather than by omission: with the helper gone, nothing ever prunes that directory, so ~/.local/state/openai-agents-python/sandbox/snapshots grows without bound across sessions. That's the honest state either way — it was already true, since the TTL never ran — but the issue framed it as a lifecycle gap, and this PR closes the "dead code" half without the "who cleans up" half. Might be worth a follow-up issue so it isn't lost with the code.

[ErenAta16]

Both behavioural fixes hold up. I ran default_local_snapshot_base_dir on main and on ca8f749, forcing the POSIX branch through the injected platform/os_name so XDG_STATE_HOME is actually consulted.

Ambient leakage, with XDG_STATE_HOME=/ambient/from/process set in the real process environment:

env={}   main:      /ambient/from/process/openai-agents-python/sandbox/snapshots
         this PR:   /tmp/home/.local/state/openai-agents-python/sandbox/snapshots

So an explicitly empty mapping stops reading host state, which is the point.

Relative values:

                     main                    this PR
'state'              used as base            ignored, falls back to $HOME/.local/state
'./state'            used as base            ignored
'../state'           used as base            ignored

Worth citing the spec here, because it makes this a conformance fix rather than a preference. The XDG Base Directory specification says:

All paths set in these environment variables must be absolute. If an implementation encounters a relative path in any of these variables it should consider the path invalid and ignore it.

and gives $HOME/.local/state as the documented default for XDG_STATE_HOME, which is exactly the fallback taken. Both halves match.

On removing cleanup_stale_default_local_snapshots: I checked before assuming it was safe. It has no callers anywhere under src/, and it is not re-exported from any __init__.py, so the only reference was the test that goes with it. Removal does not change the importable surface for anyone going through the package.

One thing I would change. The new absoluteness check uses a bare Path, whose flavour follows the host rather than the injected platform:

xdg_base = Path(xdg_state_home) if xdg_state_home else None
base = xdg_base if xdg_base is not None and xdg_base.is_absolute() else ...

platform and os_name exist as parameters specifically so this function can be exercised for one OS while running on another, and this check quietly opts out of that. On a Windows host:

Path('/abs/state').is_absolute()           False
PurePosixPath('/abs/state').is_absolute()  True

so a POSIX-absolute value injected alongside platform="linux" is treated as relative and discarded. This is not a production bug, on a real Linux box Path is PosixPath and the answer is right. It is a correctness gap in the thing the parameters were added for, and tests.yml does have a tests-windows job, so the suite genuinely runs on both.

test_default_local_snapshot_base_dir_uses_xdg_state_home currently passes on Windows only because it feeds str(tmp_path / "state"), which carries a drive letter and is therefore Windows-absolute by accident. Swap that for a literal "/abs/state" and it fails on the Windows job while passing on Linux.

PurePosixPath(xdg_state_home).is_absolute() fixes it and matches what the module already does one function up, where _first_absolute_windows_env_path deliberately pins PureWindowsPath(value).is_absolute() instead of trusting the host. Same reasoning, same shape.

Smaller, take it or leave it. The PR fixes env or os.environ, but the two lines beside it keep the pattern:

resolved_platform = platform or sys.platform
resolved_os_name  = os_name or os.name

An empty string falls back to ambient state the same way env={} used to. Nobody realistically passes platform="", so this is consistency rather than a defect, but leaving two of the three as they were makes the fixed one look arbitrary. X if X is not None else ambient across all three would read as one rule.

[seratch]

Thanks again for sharing this patch. This one is not yet ready and we wanted to narrow the scope to fix, so #4141 w/ your contribution credit is going to resolve the issue.