Repository navigation
fix(acp): materialise only the running provider's file secrets - #4927
Conversation
Scope acp_file_secrets to the resolved provider, so registering a harness upstream cannot change how another provider's conversation delivers a secret that carries the new reserved name. A caller-supplied list and an unrecognised server both keep today's behaviour. Closes #4923 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
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.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
🔴 Needs improvement
The provider scoping itself is the right direction, but inferring whether acp_file_secrets was caller-supplied by comparing it with the current registry default is not stable across serialization or SDK upgrades. See the inline finding.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This changes credential routing and disk materialization. The new provider-specific path is well tested for fresh/current configurations, but older persisted defaults can still route unrelated credential blobs as file secrets after upgrade, and an explicit current-union override is silently ignored. A human maintainer should validate the persistence policy before merge; this runtime behavior change should remain a COMMENT review pending that decision.
VERDICT:
❌ Needs rework: The configuration needs durable provenance/policy rather than value-based default detection.
KEY INSIGHT:
A mutable registry-derived value cannot reliably encode whether a persisted list was an implicit default or an explicit override.
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 is merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
Filter by spec ownership instead of testing whether acp_file_secrets is the built-in default. A persisted conversation carries the union as it was when written, so comparing against today's default read an older list as a caller override and stopped scoping after an upgrade. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
🟢 Good taste
No material findings. The ownership-based filter fixes the previous persistence/provenance issue without adding serialized policy state, and the focused provider-scoping tests pass locally (16 passed).
The PR description statements that caller-supplied acp_file_secrets are returned verbatim and that defaults are recognized by value are now stale after cf1283ca; please update the AGENT section so it describes the ownership-based policy. Do not modify the human-only HUMAN: field on behalf of its author.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This changes credential routing, environment injection, and on-disk materialization for ACP subprocesses. The implementation and regression coverage are sound, but it can affect agent execution behavior, so per repository policy this remains a COMMENT review for a human maintainer to decide after lightweight eval validation.
VERDICT:
✅ Worth merging: Core logic is sound; human maintainer confirmation is appropriate because this changes ACP runtime behavior.
KEY INSIGHT:
Filtering by registry ownership is stable across persisted historical defaults and avoids unreliable provenance inference.
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. 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 is merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
|
This comment was posted by an AI agent (OpenHands). |
…ands#4927) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: allhands-bot <allhands-bot@users.noreply.github.com>
3 participants
HUMAN:
materialise only the running provider's file secrets- #4927
AGENT:
Closes #4923
Why
ACPAgent.acp_file_secretsdefaults todefault_acp_file_secrets()— the union across every registered provider — and_materialise_file_secretsiterated all of it regardless of which harness the conversation actually runs. So registering a harness with afile_secretsspec changed behaviour for conversations running a completely different one. 1.45.0 grew that union by two (KIMI_CODE_CONFIG_TOML,PI_AUTH_JSON).I filed this from reading the code. Running it against the real CLIs turned out worse than the issue described.
Live, on
claude-agent-acp(real subprocess, real ACP handshake), with all four reserved blobs in the secret registry:mainCODEX_HOME,GOOGLE_APPLICATION_CREDENTIALS,KIMI_CODE_HOME,PI_CODING_AGENT_DIRcodex/auth.json,gemini-cli/gcloud-credentials.json,kimi-code/config.toml,pi/auth.jsonSo today a Claude Code conversation is handed Pi's and Kimi's data-dir variables and has their credential files seeded next to it — and the secrets the user actually set stop arriving as env vars, because
_present_file_secret_namesexcludes every configured name from env injection and from the<CUSTOM_SECRETS>advertisement.Summary
_active_file_secrets()filters by ownership: drop the specs that are another registered provider's reserved credential, keep everything else.Scope: this changes credential-file handling, not credential access. A secret is never withheld from the subprocess — it stops being written to disk as a credential file (with that provider's data-dir env var pointed at it) and instead travels the ordinary env-var channel, which is where it already went before its owning provider was registered. A Claude conversation stops creating Codex/Pi credential files; a supplied
CODEX_AUTH_JSONstill reaches its subprocess as$CODEX_AUTH_JSONand is advertised in<CUSTOM_SECRETS>._resolved_provider()—acp_serverfirst, thendetect_acp_provider_by_commandfor a custom or aliased command.acp_file_secretsfor a custom ACP server are unaffected.owned_elsewheresubtracts the running provider's own names first._strip_conflicting_envalready documents._materialise_file_secrets,_present_file_secret_names,restart_for_updated_credentials, and the_preconfigured_credentialsauth-fallback log.Deliberately no provenance test. An earlier revision decided whether the list was "the built-in default" by comparing it against
default_acp_file_secrets(). That is not stable: a persisted conversation carries the union as it was when written, so after an upgrade an older list no longer equals today's default, is read as a caller override, and silently stops scoping — the same bug, reintroduced. Filtering by ownership needs no such distinction and no migration.How to Test
Real e2e against the installed CLIs. Spawns the actual
codex-acp/claude-agent-acpbinaries through the real_start_acp_serverpath (real handshake — Claude reachesset_session_mode, Codex reaches its auth gate), spying onasyncio.create_subprocess_execto read back the env the subprocess was launched with, plus what landed under<persistence_dir>/acp/:Codex still materialises its own
auth.jsonand getsCODEX_HOME; the other three providers' blobs stay ordinary env vars. Onmainthe same probe produces the four-file / four-dir-var row in the table above for both providers.Unit.
TestACPFileSecretProviderScoping— 16 tests: per-provider scoping, an unrecognised command keeping every spec, command-derived resolution, out-of-registry specs always applying, a shared name being kept,[]staying empty, round-trip survival, the cross-version upgrade case, and two end-to-end cases through_start_acp_server(another provider's blob stays a plain env var; the running provider's still materialises with its dir var). The disjointness test is parametrized offACP_PROVIDERS, so a provider added upstream is covered without editing it.test_a_list_persisted_before_a_provider_was_added_still_scopespins the regression the review caught — it fails against the provenance-comparison revision:Suites.
tests/sdk/agent/+tests/sdk/settings/+tests/agent_server/test_credential_binding.py→ 1009 passed.ruff check,ruff format --checkandpyrightclean on both changed files.Notes
Context: #4841 (registering a harness upstream must not change a consuming deployment). This is the one instance of that coupling that lives in the SDK's own default rather than in a client's provider list, so neither OpenHands/enterprise#336 nor OpenHands/OpenHands#17228 covers it.
🤖 Generated with Claude Code
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimnikolaik/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:56930cd-pythonRun
All tags pushed for this build
About Multi-Architecture Support
56930cd-python) is a multi-arch manifest supporting both amd64 and arm6456930cd-python-amd64) are also available if needed