fix(sandbox): preserve empty Mountpoint prefixes - #4882
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Using None as the inheritance sentinel is the right distinction here because an empty prefix has real semantics. I also like that the regression exercises Mount.apply and the final argv for both S3 and GCS rather than only checking the dataclass merge.
One consistency question: region/endpoint still use truthiness-based fallback. That is fine if empty strings are invalid for those fields; if an empty value is ever meant to explicitly clear an inherited setting, they would have the same ambiguity this PR fixes for prefix. For prefix itself, the change looks correct.
|
Thanks for raising this. I checked the downstream semantics before keeping the change scoped to |
|
Thanks for investigating this. The empty-string fallback is real, and the patch implements the proposed distinction. However, bucket-root mounts already work by giving the mount a pattern with I would close this PR for now because the report does not establish a concrete workflow where that configuration is insufficient. Changing the fallback also changes released behavior for existing configurations. We can revisit this if there is an application scenario that requires clearing an inherited prefix through the mount entry and cannot reasonably use a separate pattern. |
Summary
This pull request fixes explicit empty prefixes being replaced by a
MountpointMountPatterndefault for S3 and GCS mounts.Both provider adapters used a truthiness fallback, so
prefix=""behaved likeNoneand unexpectedly mounted the configured default subtree instead of the bucket root. The adapters now useNoneas the sole inheritance sentinel, preserving explicit empty prefixes while keeping nonempty overrides and existingNonebehavior unchanged.The regression test exercises the public
Mount.applypath for both providers and verifies the final command omits--prefixfor an explicit empty value while still forwarding the pattern default forNone.Test plan
uv run --locked pytest tests/sandbox/test_mounts.py::test_mountpoint_prefix_uses_none_as_inheritance_sentinel tests/sandbox/test_mounts.py::test_s3_mount_direct_mountpoint_fields_override_pattern_options -q/usr/bin/env -u OPENAI_API_KEY OPENAI_AGENTS_TEST_IN_CODEX_SANDBOX=1 UV_DEFAULT_INDEX=https://pypi.org/simple PYTEST_XDIST_AUTO_NUM_WORKERS=4 bash .agents/skills/code-change-verification/scripts/run.shIssue number
N/A
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR