fix(acp): let a generic-ACP agent declare the env vars it authenticates with

[dhruv0811]

Related issue

Closes #4390

(Root cause behind #4281, whose visible symptom — the blank error — is
addressed separately by #4362. Also unblocks #3460, where the ACP model list
never arrives because the turn dies before session/new returns.)

Summary

A generic-ACP agent configured the documented way — an acp.agents: row, or
omnigent setup → Custom ACP agent — was spawned with no provider
credentials and no way to be given any, so it started unauthenticated, stalled
during the handshake, and every turn failed.

The spawn env is deny-by-default with an empty prefix family, and that part is
correct: the executor drives an arbitrary agent, so it cannot know which
vendor family the agent authenticates with, and guessing would re-widen the leak
that #3479 closed. The gap was the escape hatch. env_passthrough only existed
on a full agent spec's os_env.sandbox, which a user configuring an agent
through acp.agents: never authors — so in practice there was no hatch at all.
Measured against a realistic environment, only HOME / PATH / TERM
survived; XAI_API_KEY, OPENAI_API_KEY, ANTHROPIC_API_KEY, GOOSE_* were
all stripped.

Keep deny-by-default; make the hatch reachable per agent:

acp:
  agents:
    - name: Grok Build
      command: grok agent stdio
      env_passthrough: [XAI_API_KEY]

• Names only, never values. The variable is read from the host environment
  at spawn, so no secret lands in config.yaml. A NAME=value entry is
  rejected rather than accepted-and-ignored — that mistake would write a
  plaintext credential and silently not reach the agent, so it has to be loud.
• Threaded through the existing plumbing: AcpAgentEntry →
  HARNESS_ACP_ENV_PASSTHROUGH → AcpAgentConfig → _build_spawn_env's
  extra_allowed, unioned with any spec-declared
  os_env.sandbox.env_passthrough (neither shadows the other). A spec-embedded
  one-shot acp_agent honors it too.
• Undeclared variables are still withheld, so fix(inner): stop agent-CLI subprocesses inheriting unrelated host secrets #3479's guarantee is unchanged.

Second, no more blank turn errors from a stalled handshake.
asyncio.TimeoutError carries no message, so a caller reporting it via
str(exc) produced inner executor error: with nothing to act on. _rpc now
raises a TimeoutError naming the agent, the stalled method and the deadline —
fixed once at the single point every handshake RPC routes through, rather than at
each caller:

before:  inner executor error:
after:   inner executor error: ACP agent 'Grok Build' did not answer
         session/new within 30s (command: 'grok agent stdio')

ELI5 + flow

Omnigent deliberately hands an agent a near-empty environment so one agent can't
read another provider's API key. For built-in agents it then adds back that
agent's own family (GOOSE_* for goose, QWEN_* for qwen). For a custom ACP
agent it can't know what to add back — and there was no way for you to say. So
the agent got no key, hung waiting on its provider, and the turn died without a
message. Now you name the variable you use, and only that one is passed through.

config.yaml (acp.agents: env_passthrough: [XAI_API_KEY])
   │  names only
   ▼
_build_acp_spawn_env  ──HARNESS_ACP_ENV_PASSTHROUGH=XAI_API_KEY──▶  acp_harness
                                                                        │
                                                        AcpAgentConfig.env_passthrough
                                                                        │
                                          clean_agent_env(extra_allowed=…) ──▶ agent
                                          (base + declared names only; value read
                                           from the host env, never from config)

Test Plan

New tests (all three layers of the path, so a future break in any link fails):

• tests/onboarding/test_acp_auth.py — parse → persist round-trip, bare-string
  form, de-dup/trim, and the NAME=value + non-name rejections.
• tests/runtime/test_acp_spawn_env.py — names forwarded to
  HARNESS_ACP_ENV_PASSTHROUGH; var absent when undeclared (deny-by-default
  preserved); embedded-agent path.
• tests/inner/test_acp_executor.py — the credential lands in the real spawn
  env from the agent config, from the spec, and from both unioned; an undeclared
  secret still does not; the wrap decodes the forwarded names; and a stalled
  handshake yields a non-blank error naming session/new.
• tests/test_agent_spawn_env_canary.py — extends the Host-secret env leak fixed in pi/codex executors is still present in qwen_executor and acp_executor #3445 canary: a declared
  name is an allowlist, not a bypass (the named variable arrives; every planted
  canary secret still stays out).

pytest tests/test_agent_spawn_env_canary.py tests/inner/test_acp_executor.py \
       tests/onboarding/test_acp_auth.py tests/runtime/test_acp_spawn_env.py   # 111 passed

The handshake test fails on main (ExecutorError(message='')) and passes here.

Regression sweep over the executors/config this touches — 1082 passed:

pytest tests/inner/test_goose_executor.py tests/inner/test_qwen_agent_integration.py \
       tests/test_acp_cli_harnesses.py tests/runtime/test_spawn_env_cwd.py tests/onboarding/

13 failures in tests/onboarding/sandboxes/test_base.py (host-config rendering)
and a json5 import error in test_openclaw_config.py reproduce identically
with these changes stashed — pre-existing, unrelated to this PR.

ruff check + ruff format --check clean; pyrefly reports 0 errors on all
four changed modules.

End-to-end through the real config path (a temp OMNIGENT_CONFIG_HOME with
an acp.agents: row → _build_acp_spawn_env → the harness wrap → a fake ACP
agent that answers initialize offline but contacts its provider on
session/new, so it stalls without a key). One variable changed:

no env_passthrough (old behavior)   FAILED  -> inner executor error: ACP agent 'Grok Build'
                                               did not answer session/new within 3s
env_passthrough: [XAI_API_KEY]      SUCCESS -> turn completed

Demo

N/A — backend/config change, no visual surface. The before/after operator-facing
strings are quoted in the Summary.

Type of change

• Bug fix
• Feature
• UI / frontend change
• Refactor / chore
• Docs
• Test / CI
• Breaking change

Test coverage

• Unit tests added / updated
• Integration tests added / updated
• E2E tests added / updated
• Manual verification completed
• Existing tests cover this change
• Not applicable

Coverage notes

Automated tests cover each link of the chain (config parse → spawn-env
forwarding → wrap decode → the env the subprocess actually receives), plus the
negative case that an undeclared variable is still withheld. Manual verification
was the end-to-end run in the Test Plan: a real acp.agents: config resolved
through _build_acp_spawn_env into the harness wrap against a fake ACP agent
that hangs without a key — reproducing the original blank failure and then
succeeding with the variable declared. Not exercised: a real vendor CLI
(grok agent stdio / goose acp), which needs live credentials.

Changelog

A custom ACP agent can declare the environment variables it authenticates with via env_passthrough, and a stalled ACP handshake now reports which call timed out instead of failing with an empty message.

[omnigent-ci]

Polly AI Review

Review: env_passthrough for generic-ACP agents + named _rpc TimeoutError

1. Blocking issues

None. The core concerns were verified against the diff and source:

• Renamed timeout still caught. On this repo's Python (requires-python >=3.12), asyncio.TimeoutError is TimeoutError, so raising TimeoutError(...) from _rpc is caught identically by the existing broad except Exception in both _ensure_initialized and the run_turn/_ensure_session path. self._pending.pop(req_id, None) still runs before the raise, and raise ... from exc preserves the original via __cause__. Same type, same cleanup, now with a non-blank message flowing into ExecutorError(message=str(exc)). No regression.
• parse_env_passthrough is total over its inputs. None → (); bare str → [raw]; non-list/tuple → ValueError; empty/whitespace/non-str items → ValueError; "=" in a name → ValueError; de-dup preserves first-seen order. The parametrized rejection cases map to the correct branches.

2. Security vulnerabilities

Deny-by-default boundary is preserved — not weakened (#3479 guarantee intact).

• Only variable names are transported and added to clean_agent_env's extra_allowed; values are read from the host process env at spawn, never serialized into config or HARNESS_ACP_ENV_PASSTHROUGH.
• NAME=value is rejected loudly at parse time, so a plaintext secret can't be committed to config.yaml (and can't silently fail to reach the agent).
• The union of agent-config and spec os_env.sandbox.env_passthrough sources is entirely operator-declared and at the same trust level; there is no path for one provider's undeclared secret to reach another agent. test_spawn_env_still_excludes_undeclared_secret confirms an undeclared *_API_KEY never appears.

No injection, no cross-provider leakage, no new secret-exposure surface.

3. Non-blocking notes

• Comma-only serialization. HARNESS_ACP_ENV_PASSTHROUGH joins/splits on ,, so a name containing a comma cannot round-trip. Real env-var names don't contain commas, so this is an edge case, not a defect — but a whitespace/comma-safe encoding (or explicit rejection of commas at parse) would be more robust.
• Whitespace inside names is accepted by config parsing and forwarded literally; that yields a non-matching env name (harmless) rather than a leak.
• Stale operator guidance. _warn_initialize_failed (not touched by this diff) still points operators only at os_env.sandbox.env_passthrough, whereas the newly added per-agent env_passthrough is the knob a user with an acp.agents: row actually has. Worth a follow-up tweak so a stalled-handshake warning points at the reachable hatch.
• Timeout-path coverage. Only the session/new timeout is exercised end-to-end; the initialize timeout shares the same _rpc code, so message-building is covered, but a dedicated case would be tidier. Minor.

4. Summary

A well-scoped, correct fix. It closes a real gap — a documented generic-ACP agent spawned with no credentials and no way to receive any — by making the deny-by-default escape hatch reachable per agent, transporting names only and rejecting NAME=value loudly, so #3479's boundary is fully preserved. The secondary change turns a blank asyncio.TimeoutError into a named, actionable error at the single RPC chokepoint without altering exception handling. Tests genuinely exercise the full parent→child→spawn chain and reproduce the original blank-error failure with a real subprocess. No blocking issues; the notes above are optional follow-ups. Ready for a human to merge.

---

_{Automated review by Polly · workflow run}

[github-actions]

🏷️ Doc impact: no-doc-update

Adds an optional env_passthrough allowlist field to ACP agent config plus a non-blank timeout error message; this refines an existing integration's spawn-env handling rather than adding/removing/reconfiguring a user-facing integration or documented default.

_{Auto-classified on merge. Set the label manually before merging to override. · run}