feat(acp): materialise reserved file-content secrets onto disk#3269
feat(acp): materialise reserved file-content secrets onto disk#3269simonrosenberg wants to merge 5 commits into
Conversation
Some ACP servers authenticate via a JSON credential file rather than an env var (Codex's ChatGPT subscription via ``$CODEX_HOME/auth.json``, Gemini's Vertex AI service-account / ADC via ``GOOGLE_APPLICATION_CREDENTIALS``). Previously the only path for these deployments was to pass the credentials in the host filesystem out of band, which doesn't work when the SDK runs in a container without access to the user's home directory. This commit adds a small registry-driven mechanism in ``ACPAgent`` for reserved secret names that should be **materialised as files** before the subprocess spawns, rather than exported as env vars: - ``CODEX_AUTH_JSON`` → written to ``<tempdir>/codex_auth_json/auth.json``; ``CODEX_HOME`` is set to that directory. Codex writes back to the file on token refresh, so a real writable file (not an env var) is required. - ``GOOGLE_APPLICATION_CREDENTIALS_JSON`` → written to ``<tempdir>/google_application_credentials_json/gcloud-credentials.json``; ``GOOGLE_APPLICATION_CREDENTIALS`` is set to that file path. This covers Vertex AI service-account keys and ADC. Personal-account "Sign in with Google" subscriptions are intentionally not covered: gemini-cli encrypts those credentials with a key derived from ``hostname + username``, making the cached file machine-bound. Mechanism: 1. ``_FILE_SECRETS`` is a module-level registry mapping a reserved secret name to a ``_FileSecretSpec(filename, env_var, env_points_to)``. ``env_points_to`` is ``"dir"`` when the binary expects an env var naming a directory (Codex) and ``"file"`` when it expects an absolute file path (Google). 2. ``_materialise_file_secrets`` walks ``agent_context.secrets``, writes each matching payload into a per-agent ``TemporaryDirectory`` with 0o600 perms, sets the controlling env var, and returns the subset of secrets that still belong on the env-var injection path. 3. The existing env-var loop in ``_start_acp_server`` now iterates the filtered remainder, so a file-secret's (potentially multi-KB) JSON payload is **not** also leaked into ``os.environ``. 4. ``_cleanup`` removes the per-agent tempdir after the subprocess and executor are torn down, so we don't yank credential files out from under a still-running child. Adding a new file-based auth mechanism for a future ACP server is one ``_FILE_SECRETS`` entry. Consumers (e.g. OpenHands) get subscription auth by writing a plain secret with the reserved name — no provider- specific backend code on the host side. Tested with seven new cases in ``TestACPFileSecretMaterialisation`` covering both reserved names, file permissions, coexistence of file- and plain-secrets, empty-payload skip, shared-tempdir layout, and cleanup on ``close()``. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED Behavioral default changes detectedThese public
|
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable - Solid implementation solving real problems, but one test needs fixing.
Overview: This PR elegantly solves the file-based authentication problem for Codex and Gemini with a simple registry pattern. The data structure is clean, the implementation is straightforward, and the security considerations are mostly sound. One test has an environment collision issue that needs fixing.
Risk Assessment[RISK ASSESSMENT]
This PR adds optional file-based secret materialization for specific ACP providers. The risk is low because:
[IMPROVEMENT OPPORTUNITIES]
VERDICT: KEY INSIGHT:
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
File-secret materialization works correctly for both Codex and Gemini CLI authentication, with proper security isolation and cleanup.
Does this PR achieve its stated goal?
Yes. This PR set out to add a mechanism for ACP servers that authenticate via JSON credential files on disk (specifically Codex ChatGPT subscription and Gemini Vertex AI service accounts). I verified that:
- CODEX_AUTH_JSON is correctly materialized to
auth.jsonwithCODEX_HOMEpointing to the containing directory - GOOGLE_APPLICATION_CREDENTIALS_JSON is correctly materialized to
gcloud-credentials.jsonwithGOOGLE_APPLICATION_CREDENTIALSpointing to the file path - Files are written with secure
0o600permissions (owner read/write only) - File-secrets are excluded from env-var injection (preventing multi-KB JSON payloads in the environment)
- Plain secrets continue to work normally alongside file-secrets
- Cleanup correctly removes the tempdir after agent close
The implementation delivers exactly what the PR description promises: a registry-driven mechanism that lets consumers (OpenHands or SDK embedders) provide file-based credentials without writing provider-specific file-injection code.
| Phase | Result |
|---|---|
| Environment Setup | ✅ Dependencies installed, project built successfully |
| CI Status | ✅ All critical checks passing (sdk-tests, tools-tests, pre-commit, API checks) |
| Functional Verification | ✅ All functionality verified end-to-end |
Functional Verification
Test 1: CODEX_AUTH_JSON Materialization
Setup: Created ACPAgent with CODEX_AUTH_JSON secret containing test JSON payload.
Execution:
from pydantic import SecretStr
from openhands.sdk.secret import StaticSecret
CODEX_PAYLOAD = '{"auth_mode":"chatgpt","tokens":{"id_token":"test_token_123"}}'
agent = ACPAgent(
llm=LLM(model="openai/gpt-4o", api_key="dummy"),
acp_command=["echo", "test"],
agent_context=AgentContext(
secrets={"CODEX_AUTH_JSON": StaticSecret(value=SecretStr(CODEX_PAYLOAD))}
),
)
env = {}
remaining = agent._materialise_file_secrets(env)Result:
- ✅
CODEX_HOMEenv var set:/tmp/acp-file-secrets-_1wzte74/codex_auth_json - ✅
auth.jsonfile created at$CODEX_HOME/auth.json - ✅ File content matches payload exactly
- ✅ File permissions:
0o600(verified withstat.S_IMODE()) - ✅
CODEX_AUTH_JSONremoved from regular secrets dict - ✅ Tempdir cleaned up after
agent.close()
Test 2: GOOGLE_APPLICATION_CREDENTIALS_JSON Materialization
Setup: Created ACPAgent with GOOGLE_APPLICATION_CREDENTIALS_JSON secret.
Result:
- ✅
GOOGLE_APPLICATION_CREDENTIALSenv var set to file path (not directory) - ✅
gcloud-credentials.jsoncreated at the specified path - ✅ File content matches payload
- ✅ File permissions:
0o600 - ✅ Secret removed from regular env-var injection
Test 3: Integration with Subprocess Environment
Setup: Created a shell script that prints environment variables and reads the materialized files. Simulated the full _start_acp_server() flow.
Result:
CODEX_HOME=/tmp/acp-file-secrets-ai1he84j/codex_auth_json
GOOGLE_APPLICATION_CREDENTIALS=/tmp/.../gcloud-credentials.json
CODEX_AUTH_FILE_EXISTS=yes
CODEX_AUTH_CONTENT={"test":"codex_auth"}
GAC_FILE_EXISTS=yes
GAC_CONTENT={"test":"gac_auth"}- ✅ Subprocess received correct environment variables
- ✅ Subprocess could read materialized files
- ✅ File-secret payloads NOT present as raw env vars
- ✅ Plain secrets (
GITHUB_TOKEN) still injected normally
Test 4: Security Isolation
Setup: Created agent with both file-secrets to verify isolation.
Result:
- ✅ Each file-secret in separate subdirectory:
codex_auth_json/auth.jsongoogle_application_credentials_json/gcloud-credentials.json
- ✅
CODEX_HOMEdirectory contains ONLYauth.json(no GAC file) - ✅ GAC directory contains ONLY
gcloud-credentials.json(no Codex file) - ✅ File permissions:
0o600(owner read/write only) - ✅ Directory permissions:
0o700(owner access only)
Test 5: Edge Cases
Empty secret value:
- ✅ Warning logged:
Reserved file-secret 'CODEX_AUTH_JSON' has an empty value; skipping materialisation - ✅ No tempdir created
- ✅ No
CODEX_HOMEenv var set
Mixed file-secrets and plain secrets:
- ✅ Both file-secrets materialized correctly
- ✅ Plain secrets (
GITHUB_TOKEN) retained in secrets dict - ✅ All secrets accessible to subprocess as expected
Issues Found
None.
Replaces the host-side codex auth.json materialisation introduced in d019357 (and the Claude-OAuth credential textarea introduced in e74389a) with the SDK's new reserved-secret materialisation in ``ACPAgent`` (OpenHands/software-agent-sdk#3269, commit 6a379c03f). Authentication for ACP subscriptions now lives entirely in the standard Settings → Secrets surface, with provider-specific behaviour pushed down to the SDK: - Claude Code · Max subscription → secret named ``CLAUDE_CODE_OAUTH_TOKEN``, value = the OAuth access token. SDK exports it as an env var on the spawned subprocess (claude reads it directly). - Codex · ChatGPT subscription → secret named ``CODEX_AUTH_JSON``, value = the contents of ``~/.codex/auth.json``. SDK writes it to a per-agent temp file and sets ``CODEX_HOME``. Codex's refresh-token state is preserved across turns because the file is real and writable. API-key paths (``ANTHROPIC_API_KEY``, ``OPENAI_API_KEY``) continue to work via the existing ``_acp_provider_env`` translation of the LLM profile's API key into the provider-native env var. What this PR drops from OpenHands: - ``_inject_codex_auth``, ``_cleanup_acp_temp_dir``, ``_acp_file_secrets_base_dir``, the ``CODEX_AUTH_JSON_SECRET_NAME`` constant, and their call sites in ``live_status_app_conversation_service.py``. - ``OH_ACP_FILE_SECRETS_DIR`` env var and the auto bind-mount in ``DockerSandboxService`` config — the SDK now writes the file next to the subprocess inside the agent-server, so the backend doesn't need filesystem reach into the agent's container. - ``/{secret_id:path}`` → ``/{secret_id}`` reverted in ``secrets_router`` (the slashed ``FILE:~/.codex/auth.json`` name is gone). - ``encodeURIComponent`` on PUT/DELETE in ``secrets-service.ts`` reverted for the same reason. - ``SecretsService.upsertSecret`` and the ``useUpsertSecret`` hook — no longer needed; users create secrets through the regular Secrets UI. - The two credential textareas under the Claude Code / Codex presets in ``agent-settings.tsx``, the ``extractClaudeOauthToken`` and ``isLikelyCodexAuthJson`` helpers, and the save-credentials flow. What replaces it: - A small ``SubscriptionAuthInfo`` panel per preset (Claude Code, Codex) telling the user which magic secret name to create, surfacing a "Secret found" / "Secret not set" indicator (via the existing ``useSearchSecrets`` hook), and showing copy-pasteable extraction commands for macOS / Linux. - Five new i18n keys × 15 languages (``SETTINGS$AGENT_AUTH_*``, ``SETTINGS$AGENT_CLAUDE_AUTH_INSTRUCTIONS``, ``SETTINGS$AGENT_CODEX_AUTH_INSTRUCTIONS``). Old credential- textarea keys are still in ``translation.json`` for now; they're no longer referenced from code and can be swept in a follow-up. Net diff: −596 / +300 lines on this PR vs the previous tip. The SDK pin is temporarily a git source pointing at the SDK PR's commit (6a379c03f). It must be replaced with a regular pinned release version (>= whichever SDK release first includes that commit) before this PR can land on main. Frontend tests updated: ``TestACPSecretsEnvInjection`` and codex credential textarea tests deleted; five new cases under "subscription auth info panels" covering visibility per preset, secret-name surfaced in the panel text, and detected / not-detected indicators. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Solid implementation solving real problems, but critical issues need fixing.
Assessment
This PR elegantly solves file-based authentication for Codex and Gemini using a clean registry pattern. The data structure (_FileSecretSpec) is well-designed, the implementation is straightforward, and the test coverage is comprehensive. However, two critical issues must be addressed:
- Security race condition in file permission setting (minor risk, easy fix)
- Failing tests due to incorrect secret precedence logic (critical, pre-existing bug exposed by new tests)
[CRITICAL ISSUES]
Secret Precedence Bug (Line 1131)
Problem: Agent secrets are silently ignored when the same key exists in os.environ. Both test_static_secret_injected_into_subprocess_env (existing) and test_plain_secrets_still_exported_as_env_vars (new) fail.
Root cause: The condition if name not in env checks the merged environment dict that already includes os.environ, giving inherited environment variables precedence over user-provided agent secrets.
Expected precedence:
acp_env(explicit agent configuration)agent_context.secrets(user-provided runtime secrets)os.environ(inherited environment)
Fix: Change line 1131 from if name not in env to if name not in self.acp_env
Note: This is a pre-existing bug from PR #2984, but must be fixed here since the new tests expose it. Without this fix, users cannot override environment variables with agent secrets, which breaks the expected behavior.
File Permission Race Condition (Lines 1095-1096)
Problem: There's a brief window between write_text() and chmod(0o600) where files have default permissions.
Current mitigation: Parent directory is 0o700, preventing access from other users.
Better solution: Use atomic permission setting:
target_path.write_text(
value,
opener=lambda path, flags: os.open(path, flags, 0o600)
)This eliminates the race window entirely and doesn't rely on parent directory protection.
[TESTING GAPS]
The PR includes comprehensive tests (7 test cases), but two tests are currently failing:
test_static_secret_injected_into_subprocess_env(pre-existing)test_plain_secrets_still_exported_as_env_vars(new)
Both fail with:
AssertionError: assert '<secret-hidden>' == 'ghp_xyz'
This PR should not be merged with failing tests. The fix is straightforward (see line 1131 comment above).
[RISK ASSESSMENT]
Factors:
- Touches security-sensitive credential handling (file-based secrets)
- Has failing tests that must pass before merge
- Pre-existing bug affects all agent secrets, not just file-secrets
- Changes are additive (only affects new reserved secret names)
- Well-isolated with clear separation of concerns
Recommendation: Fix the two critical issues above before merging. The core file-secret materialization mechanism is sound and well-tested. Once the secret precedence bug is fixed and tests pass, this is ready to merge.
VERDICT
❌ Needs fixes: Core logic is excellent, but failing tests and the secret precedence bug must be addressed. Both fixes are straightforward.
KEY INSIGHT
The file-secret registry pattern is elegant and extensible — adding support for future ACP providers with file-based auth is a one-line registry entry. However, the pre-existing secret precedence bug undermines the entire secret injection mechanism by preventing users from overriding environment variables with agent secrets.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
File-secret materialization works as described; verified CODEX_AUTH_JSON and GOOGLE_APPLICATION_CREDENTIALS_JSON materialize to temp files with correct permissions, env vars, and cleanup.
Does this PR achieve its stated goal?
Yes. The PR successfully enables ACP agents to authenticate via file-based credentials without requiring consumers to implement provider-specific file injection. Verified end-to-end:
- Reserved secrets (
CODEX_AUTH_JSON,GOOGLE_APPLICATION_CREDENTIALS_JSON) are written to temp files with 0o600 permissions - Appropriate env vars (
CODEX_HOME,GOOGLE_APPLICATION_CREDENTIALS) are set automatically - File secrets are filtered from regular env var injection (not leaked)
- Plain secrets continue to work normally alongside file secrets
- Temp files are cleaned up on
agent.close() - Consumer usage pattern works transparently as documented
| Phase | Result |
|---|---|
| Environment Setup | ✅ Dependencies synced, environment ready |
| CI Status | ✅ All checks passing (SUCCESS) |
| Functional Verification | ✅ 12 tests passed across unit + integration suites |
Functional Verification
Test Suite 1: Unit Tests
Created /tmp/test_file_secrets.py to verify the materialization mechanism:
Test 1 — CODEX_AUTH_JSON Materialization:
agent = ACPAgent(
llm=LLM(model="anthropic/claude-3-5-sonnet-20241022", api_key="fake"),
acp_command=["echo", "fake"],
agent_context=AgentContext(
secrets={"CODEX_AUTH_JSON": StaticSecret(value=SecretStr(codex_payload))}
)
)
test_env = {}
remaining = agent._materialise_file_secrets(test_env)Result:
✓ Materialization method returned remaining secrets: []
✓ CODEX_HOME set: /tmp/acp-file-secrets-djx050v9/codex_auth_json
✓ File exists: /tmp/acp-file-secrets-djx050v9/codex_auth_json/auth.json
✓ File content matches payload
✓ File permissions: 0o600
✓ CODEX_AUTH_JSON not in remaining env vars
✓ Agent closed successfully
✓ Temp directory cleaned up
This confirms CODEX_AUTH_JSON is written to auth.json with CODEX_HOME pointing to the containing directory, exactly as required by the Codex binary.
Test 2 — GOOGLE_APPLICATION_CREDENTIALS_JSON Materialization:
agent_context=AgentContext(
secrets={"GOOGLE_APPLICATION_CREDENTIALS_JSON": StaticSecret(value=SecretStr(gac_payload))}
)Result:
✓ GOOGLE_APPLICATION_CREDENTIALS set: /tmp/acp-file-secrets-mdh7zvaa/google_application_credentials_json/gcloud-credentials.json
✓ File exists at path
✓ File content matches payload
✓ File permissions: 0o600
✓ GOOGLE_APPLICATION_CREDENTIALS_JSON not in remaining env vars
✓ Cleanup verified
This confirms the Gemini Vertex AI credential is written with GOOGLE_APPLICATION_CREDENTIALS pointing to the file path (not dir), as required by Google's SDK.
Test 3 — Plain Secrets Still Work:
secrets={
"CODEX_AUTH_JSON": StaticSecret(value=SecretStr("{}")),
"GITHUB_TOKEN": StaticSecret(value=SecretStr("ghp_fake_token")),
"CUSTOM_SECRET": StaticSecret(value=SecretStr("custom_value")),
}Result:
✓ Remaining secrets: ['GITHUB_TOKEN', 'CUSTOM_SECRET']
✓ CODEX_AUTH_JSON filtered out (file secret)
✓ GITHUB_TOKEN and CUSTOM_SECRET in remaining (plain secrets)
This confirms the change doesn't break existing secret injection — file secrets are filtered, plain secrets pass through.
Tests 4-5: Both file secrets coexist with shared base tempdir but isolated subdirs ✅, empty payloads skipped gracefully ✅
Test Suite 2: Integration Tests
Created /tmp/test_integration.py to verify consumer usage pattern:
Test: Consumer Usage Pattern (as documented in PR description):
codex_auth_content = json.dumps({
"auth_mode": "chatgpt",
"tokens": {"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9..."},
})
agent = ACPAgent(
llm=LLM(model="anthropic/claude-3-5-sonnet-20241022", api_key="test"),
acp_command=["codex", "serve"],
agent_context=AgentContext(
secrets={"CODEX_AUTH_JSON": StaticSecret(value=SecretStr(codex_auth_content))}
)
)Result:
✓ Agent created successfully
✓ CODEX_HOME set: /tmp/acp-file-secrets-zooasij9/codex_auth_json
✓ auth.json materialized automatically at /tmp/acp-file-secrets-zooasij9/codex_auth_json/auth.json
✓ Secret not leaked as env var
✓ Agent closed and cleaned up
This confirms the exact consumer pattern described in the PR works: create agent with reserved secret name, materialization happens transparently, no provider-specific code needed.
All 12 tests passed across both suites.
Issues Found
None.
github-actions
Bot
added
the
release-note-required
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
The PR achieves its goal: ACP file-content secrets are now materialized as private files and passed to spawned ACP subprocesses via provider-specific env vars instead of leaking the JSON payload as env vars.
Does this PR achieve its stated goal?
Yes. I exercised the SDK through the public ACPAgentSettings + Conversation path with real subprocess launches that captured their received environment and argv. On main, Codex/Gemini JSON secrets were exported directly as env vars with no files; on this PR, Codex received CODEX_HOME pointing to an auth.json file, Gemini received GOOGLE_APPLICATION_CREDENTIALS pointing to a JSON file, both files contained the secret payload with 0o600 permissions, and the original JSON secret env vars were absent. I also verified custom file-secret specs append args/env correctly, Codex auth selection now honors the materialized CODEX_HOME/auth.json, and ACPAgent.close() removes the tempdir.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully. |
| CI Status | 🟡 Core checks observed green; several Docker/image or binary jobs were still in progress at refresh time. |
| Functional Verification | ✅ Verified before/after Codex + Gemini subprocess behavior, custom specs, auth selection, exports, and cleanup. |
Functional Verification
Test 1: Codex CODEX_AUTH_JSON materializes to CODEX_HOME/auth.json
Step 1 — Baseline on origin/main:
Ran OPENHANDS_SUPPRESS_BANNER=1 timeout 8s uv run python /tmp/qa_launch_public_acp.py codex /tmp/qa_codex_base.json using ACPAgentSettings(acp_server="codex") and a dummy ACP subprocess that records env/argv.
{
"env": {
"CODEX_AUTH_JSON": "{"auth":"codex"}",
"PLAIN_SECRET": "plain-value"
},
"files": {}
}This confirms the old behavior: the JSON payload was exported as CODEX_AUTH_JSON, and no credential file/path was provided to the subprocess.
Step 2 — PR branch (2dd60666a3d78a4042bbd7a812e2f604f5aa7970):
Ran the same command after checking out feat-acp-file-secrets.
{
"env": {
"CODEX_HOME": "/tmp/acp-file-secrets-q1hhivmz/00-CODEX_AUTH_JSON",
"PLAIN_SECRET": "plain-value"
},
"files": {
"CODEX_HOME": {
"path": "/tmp/acp-file-secrets-q1hhivmz/00-CODEX_AUTH_JSON/auth.json",
"exists": true,
"content": "{"auth":"codex"}",
"mode": "0o600"
}
}
}This confirms the fix: the subprocess receives CODEX_HOME, auth.json exists with the expected contents and owner-only permissions, CODEX_AUTH_JSON is no longer in the subprocess env, and unrelated plain secrets still flow through.
Test 2: Gemini GOOGLE_APPLICATION_CREDENTIALS_JSON materializes to a credentials path
Step 1 — Baseline on origin/main:
Ran OPENHANDS_SUPPRESS_BANNER=1 timeout 8s uv run python /tmp/qa_launch_public_acp.py gemini /tmp/qa_gemini_base.json.
{
"env": {
"GOOGLE_APPLICATION_CREDENTIALS_JSON": "{"type":"service_account","project_id":"qa"}"
},
"files": {}
}This confirms the old behavior: Gemini credential JSON was only exported as an env var payload, not as a file path.
Step 2 — PR branch:
Ran the same command on the PR branch.
{
"env": {
"GOOGLE_APPLICATION_CREDENTIALS": "/tmp/acp-file-secrets-akzb43_9/00-GOOGLE_APPLICATION_CREDENTIALS_JSON/gcloud-credentials.json"
},
"files": {
"GOOGLE_APPLICATION_CREDENTIALS": {
"exists": true,
"content": "{"type":"service_account","project_id":"qa"}",
"mode": "0o600"
}
}
}This confirms the SDK now gives Gemini the contractual file path in GOOGLE_APPLICATION_CREDENTIALS while omitting the raw GOOGLE_APPLICATION_CREDENTIALS_JSON env var.
Test 3: Codex auth selection recognizes materialized CODEX_HOME/auth.json
Ran a short SDK script calling the Codex auth selector with an existing CODEX_HOME/auth.json:
base _select_auth_method with CODEX_HOME/auth.json => None
pr _select_auth_method with CODEX_HOME/auth.json => chatgpt
This confirms the materialized Codex auth file is not only written, but also participates in the SDK's ChatGPT auth-mode selection path.
Test 4: Custom ACP file-secret specs set env and append args
Ran OPENHANDS_SUPPRESS_BANNER=1 timeout 8s uv run python /tmp/qa_launch_public_acp.py custom /tmp/qa_custom_pr.json with a custom ACPFileSecretSpec.
{
"argv": [
"/tmp/qa_capture_acp.py",
"--existing",
"--config",
"/tmp/acp-file-secrets-wtp5et73/00-CUSTOM_AUTH_JSON/nested/config.json"
],
"env": {
"CUSTOM_AUTH_FILE": "/tmp/acp-file-secrets-wtp5et73/00-CUSTOM_AUTH_JSON/nested/config.json",
"CUSTOM_AUTH_DIR": "/tmp/acp-file-secrets-wtp5et73/00-CUSTOM_AUTH_JSON",
"CUSTOM_AUTH_ROOT": "/tmp/acp-file-secrets-wtp5et73"
},
"files": {
"CUSTOM_AUTH_FILE": {
"exists": true,
"content": "{"token":"custom"}",
"mode": "0o600"
}
}
}This confirms consumer-defined mappings work through the public settings path and can expose {file}, {dir}, {root}, plus appended command args.
Test 5: Public exports and cleanup
Ran uv run python /tmp/qa_exports.py:
codex_secret= CODEX_AUTH_JSON
codex_env= {'CODEX_HOME': '{dir}'}
gemini_secret= GOOGLE_APPLICATION_CREDENTIALS_JSON
gemini_env= {'GOOGLE_APPLICATION_CREDENTIALS': '{file}'}
custom_spec= {'secret_name': 'X', 'relative_path': 'x.json', 'env': {'X_FILE': '{file}'}, 'args': [], 'overwrite_env': True}
Ran uv run python /tmp/qa_cleanup_file_secrets.py:
before_close_exists= True
before_close_mode= 0o600
regular_secret_keys= []
extra_args= []
base_dir_exists_before= True
base_dir_exists_after= False
auth_path_exists_after= False
This confirms the new public metadata/spec types are importable and ACPAgent.close() cleans up the materialized tempdir.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable — the registry shape is solid, but a few precedence/persistence edges need tightening before merge. Existing unresolved env-precedence/test-failure threads still apply; I did not duplicate them inline.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM — touches ACP launch/auth behavior and persisted settings, with credential-routing implications.
VERDICT:
❌ Needs follow-up on the inline issues before this is merge-ready.
This review was created by an AI agent (OpenHands) on behalf of the user.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Verified the ACPAgent subprocess path: reserved Codex/Gemini secrets are materialized as 0600 files, passed via the expected controlling env vars, omitted from raw env/prompt exposure, and cleaned up on close.
Does this PR achieve its stated goal?
Yes. On main, the same SDK scenario exported CODEX_AUTH_JSON / GOOGLE_APPLICATION_CREDENTIALS_JSON directly into the ACP subprocess environment and advertised them in prompt context, with no credential file present. On this PR, a real Conversation using ACPAgentSettings.create_agent() launched a protocol-speaking ACP subprocess that observed CODEX_HOME=/tmp/.../auth.json for Codex and GOOGLE_APPLICATION_CREDENTIALS=/tmp/.../gcloud-credentials.json for Gemini, while the raw JSON secret env vars disappeared and plain secrets still flowed normally.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully with uv sync --dev; no tests/lints were run locally. |
| CI Status | 🟡 Observed 21 successful, 2 skipped, 7 pending, 0 failing checks. |
| Functional Verification | ✅ Started real SDK conversations and real ACP subprocesses for Codex, Gemini, and a custom file-secret mapping. |
Functional Verification
Test 1: Codex reserved secret becomes CODEX_HOME/auth.json
Step 1 — Reproduce / establish baseline without the fix:
Checked out main (38b27ab) and ran:
OPENHANDS_SUPPRESS_BANNER=1 uv run python -u /tmp/qa_acp_provider_scenario.py codex /tmp/qa_main_codex.jsonObserved result excerpt:
{
"calls": ["startup", "initialize", "new_session", "prompt"],
"env": {
"CODEX_AUTH_JSON": "{...fake qa payload...}",
"PLAIN_SECRET": "plain-value"
},
"files": {}
}This confirms the old behavior: the raw JSON was exported as CODEX_AUTH_JSON, no CODEX_HOME was set, no auth.json file existed, and the ACP agent warned that chatgpt auth had no matching on-disk auth.
Step 2 — Apply the PR's changes:
Checked out feat-acp-file-secrets at 29c3dc42c365b8220b76462c4c79c6e326b313ce.
Step 3 — Re-run with the fix in place:
Ran the same command:
OPENHANDS_SUPPRESS_BANNER=1 uv run python -u /tmp/qa_acp_provider_scenario.py codex /tmp/qa_pr_codex.jsonObserved result excerpt:
{
"calls": ["startup", "initialize", "authenticate", "new_session", "prompt"],
"authenticated_method": "chatgpt",
"env": {
"CODEX_HOME": "/tmp/acp-file-secrets-msz_gm3w/00-CODEX_AUTH_JSON",
"PLAIN_SECRET": "plain-value"
},
"files": {
"codex_auth": {
"exists": true,
"mode": "0o600",
"path": "/tmp/acp-file-secrets-msz_gm3w/00-CODEX_AUTH_JSON/auth.json"
}
},
"temp_paths_exist_after_close": {"CODEX_HOME": false}
}This confirms the PR behavior: the child process sees CODEX_HOME, auth.json exists with owner-only permissions, raw CODEX_AUTH_JSON is not exported, the fake ACP server can authenticate with chatgpt, and ACPAgent.close() removes the temp directory. The rendered dynamic context also only advertised $PLAIN_SECRET on the PR branch, whereas main advertised $CODEX_AUTH_JSON.
Test 2: Gemini reserved secret becomes GOOGLE_APPLICATION_CREDENTIALS=<file>
Step 1 — Reproduce / establish baseline without the fix:
On main, ran:
OPENHANDS_SUPPRESS_BANNER=1 uv run python -u /tmp/qa_acp_provider_scenario.py gemini-cli /tmp/qa_main_gemini.jsonObserved result excerpt:
{
"env": {
"GOOGLE_APPLICATION_CREDENTIALS_JSON": "{...fake service account payload...}",
"PLAIN_SECRET": "plain-value"
},
"files": {}
}This confirms the old behavior: Gemini credential JSON was only exported as raw env content, not as a path to a file.
Step 2 — Apply the PR's changes:
Checked out the PR branch at 29c3dc42c365b8220b76462c4c79c6e326b313ce.
Step 3 — Re-run with the fix in place:
Ran:
OPENHANDS_SUPPRESS_BANNER=1 uv run python -u /tmp/qa_acp_provider_scenario.py gemini-cli /tmp/qa_pr_gemini.jsonObserved result excerpt:
{
"env": {
"GOOGLE_APPLICATION_CREDENTIALS": "/tmp/acp-file-secrets-soi7z3db/00-GOOGLE_APPLICATION_CREDENTIALS_JSON/gcloud-credentials.json",
"PLAIN_SECRET": "plain-value"
},
"files": {
"google_credentials": {
"exists": true,
"mode": "0o600",
"path": "/tmp/acp-file-secrets-soi7z3db/00-GOOGLE_APPLICATION_CREDENTIALS_JSON/gcloud-credentials.json"
}
},
"temp_paths_exist_after_close": {"GOOGLE_APPLICATION_CREDENTIALS": false}
}This confirms Gemini gets the file-path contract it needs; the raw GOOGLE_APPLICATION_CREDENTIALS_JSON payload is not exported, the credential file is 0600, and cleanup happens after close.
Test 3: Custom file-secret mapping sets env vars and command args
On the PR branch, ran:
OPENHANDS_SUPPRESS_BANNER=1 uv run python -u /tmp/qa_acp_custom_scenario.pyObserved result excerpt:
{
"argv": [
"/tmp/qa-acp-custom-nrv9de26/fake_acp_server.py",
"--existing",
"--config",
"/tmp/acp-file-secrets-ke2to8_q/00-CUSTOM_AUTH_JSON/nested/config.json"
],
"env": {
"CUSTOM_AUTH_FILE": "/tmp/acp-file-secrets-ke2to8_q/00-CUSTOM_AUTH_JSON/nested/config.json",
"CUSTOM_AUTH_DIR": "/tmp/acp-file-secrets-ke2to8_q/00-CUSTOM_AUTH_JSON",
"CUSTOM_AUTH_ROOT": "/tmp/acp-file-secrets-ke2to8_q",
"PLAIN_SECRET": "plain-value"
},
"files": {"custom_auth": {"exists": true, "mode": "0o600"}},
"temp_paths_exist_after_close": {
"CUSTOM_AUTH_FILE": false,
"CUSTOM_AUTH_DIR": false,
"CUSTOM_AUTH_ROOT": false
}
}This confirms custom ACPFileSecretSpec values are honored for templated env vars and extra launch args, while the file-secret payload itself is not exported as CUSTOM_AUTH_JSON.
Unable to Verify
I did not authenticate against the real Codex or Gemini CLI services because this QA environment does not have ChatGPT subscription OAuth state or Google service-account/ADC credentials. Instead, I verified the SDK contract at the affected boundary by launching a real protocol-speaking ACP subprocess and observing its argv, env, credential files, prompt flow, and cleanup behavior.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
|
[Automatic Post]: It has been a while since there was any activity on this PR. @simonrosenberg, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. This comment was created by an AI agent (OpenHands) on behalf of the repository maintainers. |
2 participants
Why
Two ACP servers we already support authenticate via a JSON file on disk rather than an env var:
$CODEX_HOME/auth.jsonand writes back to that file on refresh. There is no env-var alternative — codex's auth state is inherently file-backed.GOOGLE_APPLICATION_CREDENTIALS=/abs/path/to/key.jsonpointing at a JSON file.Today the SDK has no way to surface these to the spawned subprocess. Consumers (e.g. OpenHands) end up reinventing per-conversation file injection, bind-mounts, cleanup, and path-routing — all of which is provider-specific knowledge that more properly belongs in the SDK alongside the rest of the per-provider config (
acp_command,acp_model,_acp_provider_env, etc.).This PR adds a small registry-driven mechanism: a reserved secret name → write its value to a per-agent tempdir → set the controlling env var on the subprocess → strip the secret from the regular env-var injection path so the (potentially multi-KB) JSON payload isn't also exported.
What changes
openhands-sdk/openhands/sdk/agent/acp_agent.py:ACPAgent._materialise_file_secrets(env)walksagent_context.secrets, materialises matches intoself._file_secrets_tempdir(atempfile.TemporaryDirectorycreated lazily), sets the controlling env vars, and returns the remaining (plain) secrets that should flow through the existing env-var injection loop in_start_acp_server. Files are written with0o600perms; each file-secret lives in its own subdirectory soenv_points_to="dir"handlers don't expose siblings. The per-agent tempdir is cleaned up in_cleanupafter the subprocess and executor are torn down.Adding a new file-based auth mechanism for a future ACP server is one
_FILE_SECRETSentry.Why these two reserved names
auth.jsonon token refresh. An env var would lose refresh state between turns.GOOGLE_APPLICATION_CREDENTIALSis contractually a path to a JSON file. There is no env-var-with-content form of this credential.Intentionally not covered: Gemini personal-account "Sign in with Google" subscription.
gemini-clicaches those credentials in~/.gemini/gemini-credentials.jsonencrypted with a key derived fromos.hostname() + os.userInfo().username(see@google/gemini-cli-core/dist/src/services/fileKeychain.js,deriveEncryptionKey()salted with${hostname}-${username}-gemini-clithen scrypt'd). That file is machine-bound by design and cannot be transported to a cloud host. Google's own headless docs confirm: subscription users running headless must fall back to API key or Vertex AI.Why not just inject as env vars
The SDK already exports
AgentContext.secretsas env vars on the spawned subprocess. We could put the JSON payload in an env var named (say)CODEX_AUTH_JSONand tell the binary to read it from there. But:GOOGLE_APPLICATION_CREDENTIALSis contractually a file path, not file content. There's no way to make Google's SDKs read a JSON blob out of an env var.os.environis visible to anything that can read/proc/<pid>/environ. Materialising the file with0o600and dropping the secret from the env-var path is the safer default.How consumers use this
A consumer (OpenHands or anyone embedding the SDK) doesn't need provider-specific code. To authenticate Codex via ChatGPT subscription, they write a secret named
CODEX_AUTH_JSONwith the contents of the user's~/.codex/auth.json:The same flow with
GOOGLE_APPLICATION_CREDENTIALS_JSONactivates Gemini Vertex AI auth.Tests
Seven new cases in
TestACPFileSecretMaterialisation:auth.json+CODEX_HOMEpoints at containing dirGOOGLE_APPLICATION_CREDENTIALSpoints at the file path0o600ACPAgent.close()removes the materialised tempdirAll 180 tests in
test_acp_agent.pypass.Related
This unblocks an architectural simplification on the OpenHands side: OpenHands/OpenHands#14425 currently carries ~500 lines of provider-specific file-injection machinery (
_inject_codex_auth,_cleanup_acp_temp_dir,OH_ACP_FILE_SECRETS_DIR, Docker bind-mount auto-config,secrets_router:pathhack, special credential textareas per preset). Once this PR is released and pinned, that all collapses to a small informational panel telling users which secret name to create — provider-specific knowledge stays here in the SDK where it already lives next to_acp_provider_env.Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:29c3dc4-pythonRun
All tags pushed for this build
About Multi-Architecture Support
29c3dc4-python) is a multi-arch manifest supporting both amd64 and arm6429c3dc4-python-amd64) are also available if needed