OpenHands / OpenHands/enterprise
Codex credential preflight is bypassed by every integration-triggered conversation start
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 4
- Forks
- 2
- Avg merge
- 1d 22h
- Merged PRs (30d)
- 101
Description
Actual Behavior
_validate_codex_credentials runs only in the two HTTP conversation-start endpoints. It is reachable exclusively through _validate_acp_start:
openhands/app_server/app_conversation/app_conversation_router.py:230 await _validate_codex_credentials(...)
openhands/app_server/app_conversation/app_conversation_router.py:534 await _validate_acp_start(start_request, ...) # POST /api/v1/app-conversations
openhands/app_server/app_conversation/app_conversation_router.py:1199 await _validate_acp_start(request, ...) # POST /stream-start
LiveStatusAppConversationService has no equivalent check — grep -rn 'is_valid_codex_auth\|CODEX_AUTH_JSON' over the service returns nothing. But eight integrations call app_conversation_service.start_app_conversation directly, never touching the router:
integrations/{github,gitlab,bitbucket,bitbucket_data_center,jira,jira_dc,slack,azure_devops}/*_view.py
So a user whose saved agent settings select acp_server='codex' without a usable CODEX_AUTH_JSON / OPENAI_API_KEY / CODEX_API_KEY gets:
- from the UI → a clean
400: "Connect your Codex account or set an API key before starting a Codex conversation." - from a GitHub/Jira/Slack/... trigger → a provisioned sandbox, then an opaque failure inside the ACP handshake
The runtime symptom is the ACP server rejecting the session rather than anything naming credentials. Reproduced against the real codex-acp binary with a deliberately invalid auth blob:
RequestError: Authentication required
The sandbox is already running by then, so the cost is paid and the error surfaces far from its cause.
This is the same defect class as the ACP provider allowlist fixed in #336 — a guard placed only in the endpoints is not a gate, because the integrations are a second entry point into the same start path. #336 moved the provider check into the service; this one was left in place deliberately to keep that PR scoped.
Reproduction
# 1. Save Codex agent settings with no Codex credential
curl -sX POST localhost:PORT/api/v1/settings -H 'Content-Type: application/json' \
-d '{"agent_settings_diff":{"agent_kind":"acp","acp_server":"codex","acp_command":[],"acp_args":[]}}'
# 2. Start via HTTP -> 400, no sandbox
curl -sX POST localhost:PORT/api/v1/app-conversations -d '{}'
# 3. Start via any integration view (which calls the service directly)
# -> sandbox provisioned, then an opaque ACP handshake failure
uv run pytest tests/unit/app_server/test_live_status_app_conversation_service.py -k codex
Desired Behavior
The Codex credential preflight runs wherever a conversation starts, not only on the HTTP endpoints — so an integration-triggered start fails fast with the same actionable message and never provisions a sandbox.
The natural home is next to the provider allowlist check added in #336: LiveStatusAppConversationService._start_app_conversation, after _apply_suggested_task and before _wait_for_sandbox_start. It needs a SecretsStore, which the service does not currently hold — that is the design question worth settling here rather than in a rushed patch.
Raising a subclass of OpenHandsError (itself an HTTPException) keeps the endpoints returning the identical 400 while giving integrations a real exception, which is the pattern ACPProviderNotAvailableError already uses.
Acceptance Criteria
- A Codex conversation started directly through
AppConversationService.start_app_conversationwith no usable credential is rejected before any sandbox is provisioned - The rejection carries the same actionable message as the HTTP path
- The existing HTTP-endpoint behaviour is unchanged (still a 400 with that message)
- Regression test drives the service directly with
_wait_for_sandbox_startstubbed to raise, asserting it is never called — mirroringTestACPProviderAllowlistAtServiceStart - Whatever mechanism supplies the
SecretsStoreto the service is covered, so the check cannot silently no-op when the store is absent
Related: #336 · OpenHands/software-agent-sdk#4841
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with LiveStatusAppConversationService._start_app_conversation and the provider allowlist test TestACPProviderAllowlistAtServiceStart. Compare the router checks in app_conversation_router.py with direct calls from the eight integration *_view.py files, then run the Codex-focused service tests. Done means direct starts fail before sandbox provisioning with the existing actionable message, while HTTP behavior remains unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100