fix(runtime): rebase PR 1039 and preserve sub-agent transfer - #1040
Merged
yaozheng-fang merged 8 commits intoSep 2, 2026
Merged
Conversation
`Agent(runtime="codex"|"piagent")` replaces ADK's whole LLM flow, and a number of things that replacement silently dropped turned out to produce wrong answers rather than merely missing features. Codex fidelity - Stop passing `base_instructions` to `thread_start`. It *replaces* Codex's 20.9KB tuned system prompt, and `Agent.name` has a default, so every codex agent was running without Codex's apply_patch/shell/planning guidance. Identity now rides `developer_instructions`, which is additive. Side effect: `personality` was inert whenever `base_instructions` was set and now takes effect. - Replay the shim's own ADK tool calls/results into later requests of the same turn. Codex never sees those items and rebuilds `input` from its own thread, so the model previously saw a conversation in which it had never called the tool and re-issued it, re-running side effects. - Gate tool advertisement and transcript replay on the agent turn. Codex's compaction and review passes reuse the same provider and bearer token, so they were being offered the agent's tools and could execute them. - Drop the dead `disable_response_storage` key; `store:false` is unconditional upstream. Correctness and budgets - Enforce `RunConfig.max_llm_calls`: charged per backend call, surfaced to Codex as a typed 429 rather than a 500 it would retry, and re-raised to the caller on every exit path. - Bound tool iterations per turn rather than per request (was rounds x budget); default raised 8 -> 32 to compensate. - Never let `output_key` + `output_schema` abort an invocation on prose. - piagent: the first assistant message carrying text won the whole invocation, so a preamble became the answer and the answering round was dropped. Observability - Emit the `call_llm` span both runtimes' telemetry chain keys off; without it trace dumps were empty and trace-based evaluation raised. - Populate `usage_metadata` on one merged final event per turn, and sum usage across the shim's internal tool loop (only the last call counted). - Forward `model_extra_config`, restoring Ark prompt caching and billing attribution. - Propagate OTel context into the shim so ADK tool spans keep their parent. Fail-fast instead of silent no-ops - New `veadk/runtime/compat.py`: 8 configurations that produce wrong results now raise at construction and invocation; 11 that degrade gracefully warn once. `_run_live_impl` raises instead of silently running the ADK flow. - `update_model()` now sets `model_name`, which the runtimes actually read. Shim lifecycle - Serialize cold start, bound the bind, reserve across the get_shim -> register_turn window, cap the cache, and map litellm errors correctly (429/401 were surfacing as bare 500s). Packaging - `[codex]` declares fastapi/uvicorn; harness image and template install it. Tests: a differential harness runs one agent config through both `adk` and `codex` against a shared scripted backend and compares an equivalence class of outcomes, with meta-tests that verify the comparator can actually fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-ups from an adversarial review of the previous commit, plus the resolution of a design conflict two parallel changes had left in place. - The `merge_target` fix for tool-only turns parked every partial event behind the held-back durable one, so a turn's final answer and a long command's output were delivered only after the Codex stream had already ended. Partials now go out immediately; overtaking is safe because they are never persisted, so the order among durable events -- the only order session history can observe -- is unchanged. `trailing_events` goes with it, and with it the chance of re-yielding an already-emitted prefix when an exception is thrown into the generator mid-flush. - The agent-turn gate failed *open* in its fallback path: with no turn marker it matched any remembered user text, and a compaction pass re-sends the whole history, so the summarizer was handed the agent's ADK tools and its tool transcript. It now anchors on the last user message only, and deliberately does not mirror the marker path's `tools_advertised` arm, which without a marker would also admit a review pass. Losing ADK tools after a mid-turn compaction is a degradation; running a real tool inside a summarization is a wrong answer with side effects. - `output_schema` under a non-adk runtime: two changes had shipped opposite resolutions -- compat refusing it outright, output_state degrading gracefully -- leaving ~150 lines whose only justification was a reachability that does not exist. Kept the refusal, since the schema reaches neither the backend nor the prompt; deleted the fence-extraction heuristic; kept a minimal guard so a future demotion cannot kill a turn mid-stream, and pinned the invariant with a test that fails on the demoter's own diff. - An id-less Codex thread item made every later id-less tool item match the dedupe key and lose its `function_call`, leaving an orphan `function_response` in session history. Tests: the multi-round replay fixture sent a trailing user message, which is indistinguishable from a compaction pass -- replaced with the shape Codex actually sends. Added the degraded fallback path's first coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All three were found by building the first examples that actually exercise this runtime against a real model, not by tests. - **Every codex turn 400s on Ark under VeADK's defaults.** This branch started forwarding `model_extra_config`, which was the right fix for "the user's config is silently dropped" -- but `DEFAULT_MODEL_EXTRA_CONFIG` carries `caching`, and Ark refuses caching on a request that also has the `instructions` field Codex always sends. So the fix for one silent failure created a loud one, on every turn, including in both pre-existing codex examples. Body keys the Responses transport cannot carry are now filtered; the attribution headers -- the valuable half -- still go through, and a user's own `extra_body` keys are untouched. - **A rejected backend request read as a completed turn.** Codex treats it as the end of its turn and returns what it had, so the caller saw `status=completed`, a half-finished workspace and a plausible-sounding summary, while the 4xx appeared only as a warning that logged no message. Backend errors are now recorded on the turn state -- the runtime already re-raises those after the stream ends -- and the message is logged, still redacted. - **`doubao-seed-1-6` could not complete a turn at all.** After its first tool round Codex replays its own `reasoning` items; Ark refuses them for that model and the turn died mid-investigation. The backend call now retries once without them, but only in response to that specific refusal: stripping them pre-emptively would trade a hard failure on a few models for quieter, worse answers on every model that accepts them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
VeADK had two codex examples and neither demonstrated why anyone would choose the runtime: one was "you are a helpful assistant", the other asked for the weather and answered in one line. Both are tasks where `runtime="codex"` is strictly worse than `runtime="adk"` -- a subprocess spawn per turn, the whole history re-serialized into the prompt, and a matrix of ADK features refused -- for no benefit. No example anywhere set `CodexRuntimeConfig` or touched the sandbox. Two examples that do, both verified end to end against a real Ark model: - `codex_data_analysis` leads on self-iteration. An ADK tool lands a 2,400 row extract in the workspace; Codex writes an analysis script, crashes on a blank numeric cell buried at line 212, probes for bad rows, rewrites twice, and publishes a report whose figures match ground truth to the cent. Turn two reads back its own artifacts and swaps the chart. The data volume is load-bearing: at 40 rows the model just `cat`s the file and writes a correct script first try, proving nothing. - `codex_ops_assistant` leads on the security story. Three tools land a day of logs, a metric series and a deploy log in the workspace; the seeded root cause needs a change point, a pivot, and a join across UTC and UTC+08:00 timestamps, behind three decoys, so a single grep cannot reach it. The sandbox boundary is verified from inside, not assumed. With the network off, the audited ADK tools are the only egress -- the model can chew on production logs but cannot exfiltrate them. Both are built on the technique neither the code nor the docs stated: the workspace is the data plane, tool arguments and results are the control plane. Tool results are `json.dumps`-ed into the model's context and then replayed into every later request of the turn, so a tool hands Codex a path, never a payload. Docs gain a "when to use the codex runtime" section (good fits, bad fits, and the costs stated honestly), the data-plane rule, and the security baseline as one recipe. The two existing examples now say what they are -- wiring and deployment references -- so they stop reading as endorsements. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
**A supported way for an ADK tool to find the workspace.** Tools run in the host process, so a tool that hands Codex a file -- the correct pattern, since tool results go through the model's context -- had no way to locate the session's directory. Both examples had to pin `workspace_root` *and* `reuse_workspace=True`, putting every session in one directory: fine for a demo, wrong for the multi-tenant case they were recommending it for. `veadk.runtime.codex.current_workspace()` now returns it, with nothing pinned. Note the mechanism is *not* a ContextVar set in `run_async`: measured against a real shim, that is not a miss but a cross-tenant leak -- the shim's server task is created inside the first invocation's `get_shim`, so every later handler task inherits that invocation's context forever, and turns B, C and D all read turn A's workspace. The path is captured per executor in a closure instead. The concurrency test drives the shim from a detached task with a decoy workspace bound, because `ASGITransport` would otherwise run the handler inline and let the broken design pass. **piagent's tool-only turns** dropped callback `state_delta` and the turn's `usage_metadata`, the same gap codex had. Porting it surfaced a second, live bug: the translator's text dedup suppressed a final answer whose text matched a preamble the same turn had already parked on a tool-call event, so a model that says "Done." beside its tool call and "Done." again as its answer wrote no `output_key` at all. The dedup was doing two jobs with one rule; they are now separate. **Shim lifecycle.** The reservation protecting the `get_shim` -> `register_turn` window was a 60s deadline that a slow setup (MCP connect, the reaper's rmtree pass) outlives; it is now a lease object tracked weakly, so there is no release call to miss on any exit path and the deadline survives only as a floor. `register_turn` consumed an arbitrary reservation rather than its own. `_SHIMS` was coroutine-atomic but not thread-atomic, so two workers under separate `asyncio.run` loops could orphan a bound socket for the process lifetime. litellm's retries stay uncharged against `max_llm_calls`, matching what ADK counts -- now stated rather than implied. **The workspace root** was created at module import, so importing the runtime made a temp directory that `atexit` could not reclaim after SIGKILL; five orphans were sitting in $TMPDIR. It is lazy now. **Codex is no longer told to use tools it cannot reach.** Its own prompt instructs the model to use `apply_patch`, which the shim never forwards, and `request_user_input`, which nobody can answer mid-run. Both burned a round and both examples counter-instructed by hand; the runtime states it once. **A green baseline.** Five tests failed locally on a missing optional extra, which made every regression hunt start by re-deriving which failures were "ours" -- twice on inherited and wrong claims. They skip cleanly now, still run under CI's `--all-extras`, and gate on the specific missing import so a real breakage cannot hide behind them. That work turned up a user-facing bug unrelated to any of this: `sqlalchemy>=2` leaves greenlet to a platform marker that does not cover macOS arm64, so a bare install on Apple Silicon raised "the greenlet library is required" on the first session write. CI never saw it -- ubuntu-latest is x86_64. Pinning `sqlalchemy[asyncio]` fixes it. **The codex-smoke CI job could go green without running anything** -- a skip exits 0, so a runner that quietly lost the Codex binary told you nothing, which is the one thing that job exists to detect. It now fails unless the test actually ran. `_merge_turn_bookkeeping` was duplicated in both runtimes; it moves to `model_callbacks.py`, which owns both producers of the bookkeeping it folds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
yaozheng-fang
approved these changes
Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests