feat(sandbox): resolve manifest environment values from the process environment - #4399
feat(sandbox): resolve manifest environment values from the process environment#4399xumaple wants to merge 1 commit into
Conversation
…he process environment Environment can only hold literals today: StrEnvValue is the only concrete EnvValue, so anything a sandbox needs in its environment has to be written into the manifest itself. That is awkward wherever the manifest is persisted or handed across a boundary, because the value travels with it. OsEnvValue names a variable instead. The backend clients pick it up where they already call manifest.environment.resolve(), at create, exec and resume, so the value is read on the machine actually building the sandbox. An unset variable resolves to the empty string, matching shell expansion rather than introducing a new failure mode into sandbox creation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e8ee09632
ℹ️ 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".
| name: str | ||
|
|
||
| async def resolve(self) -> str: | ||
| return os.environ.get(self.name, "") |
There was a problem hiding this comment.
When a manifest or serialized session-state payload is not fully trusted, this lets that payload choose any variable name and have the SDK process read it during manifest.environment.resolve() before create/exec/resume, so a manifest containing {"type":"os_env","name":"OPENAI_API_KEY"} would copy the host API key into the sandbox environment. That makes serialized manifest data itself authorize host secret access; keep OS environment references as trusted application-side configuration or require an explicit allowlist/rebind before resolving them.
AGENTS.md reference: AGENTS.md:L125-L125
Useful? React with 👍 / 👎.
|
Thanks for putting this together. The underlying need is real: This approach can make sense when another layer already owns and enforces the trust boundary. For the SDK’s built-in generic mechanism, though, the trust boundary needs to live in trusted sandbox runtime configuration rather than in serialized manifest data. We are pursuing that direction in #4408, where the manifest contains only references and the sandbox client owns the allowed destination/source bindings. Closing this PR in favor of #4408 for the core SDK implementation. Thanks again for the proposal. |
2 participants
Summary
Environmentcan only hold literal values today.StrEnvValueis the only concreteEnvValue, so anything a sandbox needs in its environment has to be written into the manifest itself, and it travels with the manifest wherever that gets persisted or handed across a boundary.This adds
OsEnvValue, which names a variable instead. The backend clients pick it up where they already callmanifest.environment.resolve(), at create, exec and resume, so the value is read on the machine actually building the sandbox. It also seemed like the obvious second implementation of an interface whoseresolve()isasync, which only earns the keyword if resolution can do more than return a field.Three choices I would rather hear your view on than assume:
""rather than raising, matching shell expansion instead of adding a new failure mode to sandbox creation"os_env"for the discriminator, which departs fromStrEnvValue's class-name-minus-suffix rule but reads better in a serialized manifest."os"is just as easy if consistency matters more, and it is a wire string, so cheaper to settle now than laternamefor the field, since no existing model carries a single variable name andnameis the identifier field used elsewhere in the packageNote: this touches
test_core_discriminator_type_strings_are_stable, because that table maps every core discriminated class and leavingOsEnvValueout would have made it the only one missing. I left the members list indocs/ref/sandbox/manifest.mdalone under the Documentation Release Timing rule inAGENTS.md; let me know if you would like that as a separate PR when it is due.Test plan
Five tests in
tests/sandbox/test_manifest.py: resolving a variable that is set, unset and empty both resolving to"", round-tripping throughManifestserialization with the discriminator intact and coming back asOsEnvValue, and resolving throughEnvironment.resolve()alongside a bare literal and anEnvEntrywrapper..agents/skills/code-change-verification/scripts/run.shpasses: format, lint, typecheck and tests all green.Two notes about my environment rather than the change, in case they show up elsewhere: building
cffineededlibffi-devinstalled, and my container setsno_proxyto a valuehttpx2cannot parse (Invalid port: ':1]'), which takes out around 138 unrelated tests intest_config.py,tracing/,mcp/andvoice/until those variables are cleared.Issue number
None.
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR