Adopt three ideas from unified-agent-api and claude-wrapper - #7
Merged
Conversation
added 4 commits
July 29, 2026 02:37
…rift Every flag mapping in this crate was verified against one specific release, and those releases move. There was no way to ask whether the installed CLI is the one the mappings were checked against, so drift surfaced as a run failing halfway through with "unexpected argument", which names a flag rather than the cause. That is exactly how `codex exec resume` rejecting `--sandbox` was found. `Probe::run(agent)` reads `--version` and compares it against `Agent::verified_version`, reporting Verified / Newer / Older / Unrecognized with an `advisory()` sentence written for a person to read. Probing is not automatic: it spawns a process, and paying that per request to guard against an occasional upstream change is the wrong trade. A host probes at startup. Two changes on top of the idea as borrowed from unified-agent-api, which keys an embedded table by target triple and refuses unvalidated combinations: - Also classify the failure. `Error::FlagRejected` is separated from `Error::Failed` when a CLI refuses an argument, because the remedy differs: nothing about the request is wrong, the wrapper and the CLI disagree about what it accepts. Refusing up front only helps someone who probed; this helps whoever is reading the error. - No `semver` dependency. These CLIs print a dotted triple inside prose, none publishes pre-release metadata, and a crate to compare three integers is not worth the graph. `Version::find` scans for the triple, which handles "2.1.205 (Claude Code)", "codex-cli 0.145.0" and the trailing period in "GitHub Copilot CLI 1.0.75." alike. The live test that checks installed agents against their verified versions runs by default rather than behind --ignored, since `--version` costs no quota. It turns flag drift into a red build that names the version, instead of a mysterious failure later.
A missing login was reported as a generic failure, or worse, not reported at all: an unauthenticated Claude run exits **0** and puts "Not logged in · Please run /login" in its result text, so checking the exit code alone handed back a successful Outcome whose answer was a login prompt. Auth is now checked regardless of exit code, the same reasoning that already applied to quota refusals. `Error::NotAuthenticated` carries the agent, the provider's own wording, and the command that fixes it. The hints are per agent because the routes genuinely differ, verified against each CLI's help: Codex and Copilot expose `login` subcommands, Claude authenticates interactively or via `setup-token`. This matters more since EnvPolicy::Minimal became the default. A credential variable this crate does not know to pass through now presents as a login failure, and "not authenticated, run codex login" sends someone to a login screen when the real fix is the environment policy. Naming the category is what makes that distinguishable at all. Deliberately narrow phrase matching, and it loses to nothing: an unrelated failure misread as an auth problem sends someone to re-authenticate over something else entirely. Tested against the failures it must not claim, including rate limits and rejected flags.
MAX_CAPTURE bounds what is kept and the channel bounds how many events queue, but nothing bounded how large a single event is. With a 512 KiB line limit and a 256-deep channel, a consumer that stopped reading could hold roughly 130 MiB of events. Payloads are now capped at MAX_EVENT_BYTES, bringing that to about 16 MiB, which is at least a number worth being able to state. Borrowed from unified-agent-api's per-field bounds, with two changes: - Identifiers are exempt. The session id and tool-call ids pass through whole however long they are, because shortening one breaks the thing it exists for: a truncated session id resumes nothing and a truncated tool id matches no call. A large identifier is a nuisance; a shortened one is a bug. - Tool arguments are replaced rather than truncated. They are JSON, and cutting JSON in half yields something that no longer parses, which is worse for a consumer than an honest placeholder recording what was dropped. Truncated text carries TRUNCATION_MARK so a shortened payload is never mistaken for what the agent actually produced. Enforced at the single point where events leave the parser, so all three agents are covered without each one remembering.
CI caught this: a dropped Run left grandchildren alive on Linux, while the
identical teardown worked from cancel and from a timeout.
Drop was signalling the driver and aborting it, which leaves the actual kill
waiting on the runtime to poll the aborted task so its ChildGuard runs. That is
a dependency on scheduling, and it did not reliably happen. cancel and timeout
were unaffected because both call kill_process_group directly.
Run now holds the child's pid and kills the group itself in Drop. It is the one
thing Drop can do synchronously, and it depends on nothing being polled. The
abort stays as the way to stop the driver.
A shared `reaped` flag stops Drop signalling a pid the OS may have since handed
to someone else, and the three consuming methods disarm it: finish and cancel
hand teardown to the driver, and detach would otherwise kill the run it exists
to keep alive, which is how the detach test caught that omission immediately.
Worth recording how this was found, since I got it wrong twice. The first
failure I called flaky and added polling; it failed again identically. The
second I diagnosed as a zombie being counted as alive, and fixed the liveness
check to read process state. That was also wrong, but it made the third failure
say `state Some("S")`: sleeping, not a zombie, genuinely alive. A test that
reports what it observed rather than only that it failed is what turned a third
round of guessing into a diagnosis.
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
1 participant
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.
Three commits, one per improvement, from reviewing unified-agent-api and claude-wrapper. Each is folded rather than copied, with the changes explained below.
1. Runtime capability probing (
2e57093)Every flag mapping here was verified against one release, and those releases move. There was no way to ask whether the installed CLI is the one the mappings were checked against, so drift surfaced as a run failing halfway through with "unexpected argument". That is exactly how
codex exec resumerejecting--sandboxwas found.Probe::run(agent)reads--version, compares againstAgent::verified_version, and reports Verified / Newer / Older / Unrecognized with anadvisory()written for a person. Not automatic: probing spawns a process, and paying that per request to guard against occasional upstream change is the wrong trade.Changed from the source idea. unified-agent-api keys an embedded table by target triple and refuses unvalidated combinations up front. That only helps someone who probed, so this also classifies the failure:
Error::FlagRejectedis separated fromError::Failed, because the remedy differs. Nothing about the request is wrong; the wrapper and the CLI disagree about what it accepts.No
semverdependency. These CLIs print a dotted triple inside prose and none publishes pre-release metadata, soVersion::findscans for it, handling2.1.205 (Claude Code),codex-cli 0.145.0and the trailing period inGitHub Copilot CLI 1.0.75.alike.The live check runs by default rather than behind
--ignored, since--versioncosts no quota. Flag drift now becomes a red build naming the version.2. Auth failure classification (
9d23281)A missing login was reported as a generic failure, or not at all: an unauthenticated Claude run exits 0 and puts "Not logged in · Please run /login" in its result text, so checking the exit code alone handed back a successful
Outcomewhose answer was a login prompt. Now checked regardless of exit code, the same reasoning already applied to quota refusals.Error::NotAuthenticatedcarries the agent, the provider's wording, and the command that fixes it. Hints are per agent because the routes genuinely differ, verified against each CLI's help: Codex and Copilot haveloginsubcommands, Claude does not.This matters more now that
EnvPolicy::Minimalis the default: a credential variable the crate does not know to pass through presents as a login failure, and naming the category is what makes that distinguishable from a real one.3. Per-event payload bounds (
bc73428)MAX_CAPTUREbounds what is kept and the channel bounds how many events queue, but nothing bounded how large a single event is. With a 512 KiB line limit and a 256-deep channel, a stalled consumer could hold roughly 130 MiB. Now about 16 MiB.Changed from the source idea. unified-agent-api bounds every field uniformly. Two exceptions here:
Not adopted
--permission-prompt-tooldoes not appear in 2.1.205 at all. Worth revisiting when upstream fixes it.Neither project supports GitHub Copilot at all, so nothing to learn there.
95 unit tests, 6 process tests, clippy clean, live suite 11/11 against all three real agents.