fix(sdk): make ACP auth failures self-diagnosing - #4404
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
👋 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 |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
|
🔍 Review in progress… We are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here. |
|
🤖 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 is a focused, observability-only fix that addresses a real diagnostic gap: when the Codex ACP server offers chat-gpt and/or api-key auth methods but the SDK cannot supply matching credentials, the prior single-line warning gave operators no way to tell whether the codex auth.json was missing, malformed, or the wrong type — and stderr from the ACP subprocess was never drained at all, so hangs surfaced as silent -32603 Internal errors. The change reports each failure mode distinctly via _auth_selection_failure_reason, drains stderr into a redaction+truncation pipeline (_log_acp_subprocess_stderr), and promotes the existing non-JSON stdout path to INFO with the same redaction+truncation. Subprocess stream teardown is also covered: the new assert process.stderr is not None makes the post-Popen invariant explicit.
Two minor suggestions below — neither blocks merging.
Risk Assessment: 🟢 LOW — logging only, no control-flow or wire-protocol changes; secret redaction and per-line truncation are in place; new tests cover the warning content and stream logging.
✅ Worth merging.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
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.
Summary
This is a focused, logging-only improvement that makes Codex ACP auth-selection failures self-diagnosing and adds subprocess stderr draining. The changes are correct, well-scoped, and safe. No material issues found.
Risk: Low — No protocol, control-flow, or persistence changes. All new code paths are diagnostic logging with secret redaction and line truncation.
Findings
1. _auth_selection_failure_reason only diagnoses Codex methods (minor)
_select_auth_method handles five method types (chat-gpt, vertex-ai, oauth-personal, api-key, gemini-api-key), but _auth_selection_failure_reason only produces specific reasons for chat-gpt and api-key. If a server offers only gemini-api-key (with GEMINI_API_KEY unset) or vertex-ai (with GOOGLE_APPLICATION_CREDENTIALS missing), the warning falls through to the generic "no supported credential source is available" — which is exactly the kind of un-actionable message this PR set out to fix.
This is acceptable given the PR is explicitly scoped to the Codex/enterprise#120 investigation, but consider extending the reason builder to cover the other methods (or at least gemini-api-key) in a follow-up so the function name doesn't overpromise.
2. Stream-logging tests don't exercise redaction or truncation (minor test gap)
test_logs_non_jsonrpc_stdout and test_logs_subprocess_stderr verify the message content at INFO level, but don't assert that (a) a line exceeding 4,000 chars is truncated, or (b) a line containing a secret pattern (e.g. sk-...) is redacted. Both maybe_truncate and redact_text_secrets are independently tested utilities, so this is a coverage gap rather than a defect, but a single assertion per path would guard against wiring regressions (e.g., calling them in the wrong order, or forgetting to pass truncate_after).
3. Fire-and-forget stderr task (informational)
The stderr-drain task created at line 2728 is not stored — matching the existing pattern for the stdout filter task at line 2725. In CPython asyncio, create_task does not hold a strong reference; the caller must. In practice the task runs to completion because it only awaits readline() until EOF, but the modern guidance is to save the task reference (e.g. on self._stderr_task) so it can also be awaited/cancelled during _shutdown_runtime. This is a pre-existing pattern, not a regression introduced here.
4. Env-var-style secrets in subprocess output may not be fully redacted (minor security note)
redact_text_secrets targets dict/quote patterns (api_key='...', 'KEY': '...'), URL query params, and bare API-key literals from known providers (sk-..., xoxb-..., Bearer ...). If codex-acp or another ACP server logs an env-var-style secret like CODEX_API_KEY=<opaque-token> where the value doesn't match a known provider prefix, it would not be redacted. This is a residual best-effort limitation of the existing utility rather than a new issue, but worth being aware of given the PR explicitly moves stderr from debug to INFO level.
Verdict
The PR is safe to merge. The changes are correct, the redaction/truncation approach is reasonable, and the test coverage is adequate for the scope. The observations above are minor/informational and suitable for follow-up rather than blocking.
Review feedback on #4404 (line 929): the stderr-drain task added by this PR wasn't stored anywhere, so nothing held a strong reference to it or cancelled it on shutdown — the same pre-existing gap as the stdout filter task. Store both on self and cancel/await them in _shutdown_runtime, mirroring how _process/_conn are already torn down.
2 participants
HUMAN:
Improve error logs for codex acp
AGENT:
Why
Codex ACP auth-selection failures currently collapse missing auth material, invalid auth JSON, and missing API keys into the same warning. The subprocess stderr is also never consumed, while non-protocol stdout is debug-only, leaving hangs and crashes without actionable diagnostics. This blocked the investigation documented in OpenHands/enterprise#120.
Summary
Issue Number
https://github.com/OpenHands/enterprise/issues/120
How to Test
Run
uv run pytest tests/sdk/agent/test_acp_agent.py. The full ACP agent module passes:436 passed in 10.86s. The stream tests feed fake stdout/stderr readers through the same async logging functions used by spawned ACP processes and assert the resulting info-level records.Run
uv run pre-commit run --files openhands-sdk/openhands/sdk/agent/acp_agent.py tests/sdk/agent/test_acp_agent.py. Ruff formatting/lint, pycodestyle, Pyright, import rules, and tool registration all pass.A live codex-acp binary is not required because this PR changes SDK-side stream draining and logging only; protocol JSON remains connected to the ACP client unchanged.
Video/Screenshots
Not applicable; this is backend observability with no visual change.
Type
Notes
This is logging-only. Subprocess diagnostics are redacted before logging and individual lines are truncated to bound Datadog event size.
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:a106bc1-pythonRun
All tags pushed for this build
About Multi-Architecture Support
a106bc1-python) is a multi-arch manifest supporting both amd64 and arm64a106bc1-python-amd64) are also available if needed