Port oneharness to a pure-Rust embeddable crate - #1
Conversation
Replaces the multi-language oneharness stack with a single Rust library, `agent-abstraction`, covering Claude Code, Codex and GitHub Copilot. The Python SDK, the TypeScript/npm SDK and the JSON-Schema codegen that fed both are dropped rather than ported: they existed only so non-Rust callers could shell out to the `oneharness` binary and re-validate its JSON. For a Rust consumer that layer collapses into the public API. The CLI and the 39 shell scripts go with them, along with five harnesses we do not use. The crate is async over tokio so a Tauri front end can render a run as it happens, and exposes: - one `Request` builder and one `Permission` posture mapped onto each CLI's own vocabulary - one `Event` vocabulary normalized from three different JSON dialects - a session store binding a caller-owned name to each agent's native id, with resume and fork Every flag mapping and output shape is verified against the installed CLIs (claude 2.1.205, codex-cli 0.145.0, Copilot CLI 1.0.75) rather than inherited. That turned up several places where upstream is now stale, notably Copilot, which oneharness models as having no headless session id and no event stream; 1.0.75 has both, and its tool filters only bind with `=`. Capability gaps are loud: asking an agent to fork when it cannot is an `Error::Unsupported`, never a silent linear resume. Quota refusals surface as `Error::RateLimited` and are never retried internally. Covered by 55 unit tests plus a live suite (ignored by default) that drives the real agents end to end: answer, usage, streaming, multi-turn memory and forking.
`codex exec` aborts outside a git repository. That check guards against an agent editing files with no way to undo them, but this crate is embedded in hosts that legitimately run against scratch directories, worktrees and review checkouts, where a hard abort is useless. The real containment is the sandbox, which defaults to read-only, so nothing is unrecoverable either way. Previously callers had to pass --skip-git-repo-check themselves, which meant every consumer rediscovered the failure the hard way. Covered by a live test that runs codex from a non-git scratch directory; the rest of the suite runs from this repo and so could never catch a regression.
|
Addressed the open review note: Rationale recorded next to the mapping: the check guards against an agent editing files with no way to undo them, but this crate is embedded in hosts that legitimately run against scratch directories, worktrees and review checkouts. The sandbox is the real containment, and it defaults to Verified with a new live test that runs Codex from a non-git scratch directory. Worth calling out that the rest of the live suite runs from this repo, which is a git checkout, so it could never have caught a regression here. The other two review notes from the description still stand: |
The previous commit rewrote argv_codex's doc comment header to document --skip-git-repo-check and dropped [--model M] from that line. Only the comment was wrong; the code has always forwarded the model. Restoring it, and adding tests so the two cannot drift again: - every agent forwards a caller-supplied model verbatim - no model means no --model flag, so the agent picks its own default The model is never defaulted, normalized or validated here. A host with a model picker owns that list, and an unknown name should surface as the agent's own error rather than something this crate guessed at.
Priority items from review: - Run::finish no longer forces a JoinError through io::Error and reports it as Error::Spawn. The process started fine, so a spawn failure names the wrong cause. New Error::Interrupted variant distinguishes a panicked driver task from a cancelled one. - Bound every captured buffer at MAX_CAPTURE (1 MiB), keeping the earliest output. Raw stdout, stderr and the parser's own text buffer all grew without limit, so a long-streaming agent ended a run in an OOM rather than an answer. Truncation cuts on a character boundary. stderr keeps draining past the cap, since an undrained pipe blocks the child. - Replace the lossy session-name sanitizer with injective percent-encoding. Folding unsafe characters to `-` mapped `café` and `cafe-` onto one file, so the second session to use that name silently resumed the first one's conversation. Uppercase is encoded too: macOS and Windows are case-insensitive and would otherwise collide `Chat` with `chat`. Over-long names truncate on an encoding boundary and disambiguate with FNV-1a, chosen over DefaultHasher because the latter may change between Rust releases and would repoint every stored session on a toolchain upgrade. Also from review: - SessionRecord keeps the caller's original name rather than the sanitized form, so list() returns the name that was actually passed. - Count unparseable output lines and keep the first as a sample, surfaced as Outcome::looks_like_a_format_change(). A vendor changing its output shape otherwise presents as a successful run that mysteriously returns nothing. - flatten_text keeps non-text content blocks as raw JSON instead of dropping them, and documents the tool-result shapes it assumes. - Extract argv construction into a small builder, keeping every flag literal at its call site so the per-agent flag lists stay greppable against --help. - Document the SessionStore clone in Request::session as a PathBuf, not the sessions themselves. - Assert on Error::NotInstalled's structured hint field rather than the rendered message, which is free to reword. - Add CI: fmt, clippy -D warnings, tests, doctests, doc link check, and an MSRV job. Live tests stay ignored there; they need three authenticated CLIs and spend real quota. CI caught a formatting drift and a clippy error on its first local run.
|
Review feedback folded in ( 1. 2. Bounded capture. Valid, and it was three buffers rather than one: raw stdout, stderr, and the parser's own text buffer for 3. Two follow-ons from that: over-long names truncate on an encoding boundary and disambiguate with FNV-1a (not The rest:
68 unit tests (up from 62), clippy clean under One thing I could not verify locally: the MSRV job. This machine has Homebrew rust with no rustup, so 1.85 is not installable here. CI will be the first real check of that. |
There was no public way to supply a session id: only the store could mint one. A host that already has its own thread identifier had no way to make the agent's session match it. Request::session_id() fills that gap. Verified against the live CLIs rather than inferred from --help, since an id that is accepted but quietly ignored would be worse than one that is refused: - Claude and Copilot both honour an assigned UUID. The id comes back unchanged and resumes the same conversation on a later turn. - Codex has no --session-id on `exec` and cannot be told one. Asking is an Error::Unsupported raised before spawning, not a silently unrelated session. - Codex does reveal its thread_id early: `thread.started` is the first record of the stream and arrives before any answer text, so a binding can be stored when the stream opens rather than when the turn ends. Covered by a test that fails if any text precedes the id. Also drops the MSRV CI job. The claim is derived from the dependency graph (uuid and getrandom both sit at 1.85) and no consumer needs an older toolchain, so defending it in CI was cost without benefit.
Cancellation and process containment: - Dropping a Run now kills the agent. Previously the driver task was detached, so closing a window left an agent running unobserved, spending quota and writing files. Added cancel() for a deterministic stop and detach() for the callers who genuinely want a background run. - Each run gets its own process group on Unix, torn down on drop, cancel and timeout. kill_on_drop only kills the CLI, orphaning the commands it spawned. A ChildGuard owns the teardown so every exit path from the driver is covered rather than each one remembering. - Ordering bug found by the new test: the timeout path reaped the child before signalling the group, and reaping clears the pid the group kill needs, so every grandchild survived. Group first, then reap. - stream() checked Handle::try_current() instead of letting tokio::spawn panic out of a Result-returning function. Permissions, stated honestly: - Copilot's ReadOnly passed --allow-all-paths, which disables path verification. The one posture that exists to narrow filesystem reach was widening it. - Claude's ReadOnly now also denies mcp__*, so an MCP server cannot mutate remote state during a run the caller asked to be read-only. - Documented what these postures cannot do. They constrain each CLI's built-in tools, not MCP servers, plugins or custom tools. Codex has no true plan mode, so Plan is its read-only sandbox: writes blocked, execution still permitted. - args() is now unchecked_args(), documented as voiding the crate's guarantees, since arguments land after the generated ones and can contradict them. Session integrity: - Persist an assigned session id *before* spawning, and a printed one as soon as the stream reveals it. The docs claimed a crashed run stayed resumable while the code only bound on a clean exit, which is exactly backwards. - Store failures are returned rather than discarded with `let _`. - get() returns Result<Option<_>>: only a missing file is None. A permission error or corrupt record silently became "start a new conversation", quietly abandoning a conversation the caller believed they were in. - bind() enforces the agent-conflict invariant too. It is public, so the check could not live only in plan(). - Records are written 0600 into a per-process temp name and fsynced before the rename. A shared <name>.json.tmp let concurrent writers for one session corrupt each other. Also: - A named session with a format that cannot carry an id is refused up front instead of succeeding and silently failing to bind. - The stdin threshold measures the whole command line. A small prompt with a large system prompt could still exceed ARG_MAX, contradicting the comment. - Rate limits are classified regardless of exit code: Claude can report a blocking limit and still exit 0. - Run::redacted_argv(), and argv() documented as sensitive, since prompts and session ids ride the command line. - Dropped the `which` dependency; NotFound off the spawn is one PATH resolution instead of two and has no check-to-use gap. - Narrowed the public API: Plan, Continue, Parser, Terminal and STDIN_THRESHOLD were implementation machinery. New tests/process.rs proves the lifecycle end to end with a fake agent that spawns a grandchild: drop, cancel and timeout kill the tree, detach does not. Full live suite re-run: 9/9 pass. Not done, and why: the six-axis ExecutionPolicy split is a larger redesign than this crate's scope justifies, and Windows Job Objects are unimplemented, so process-tree containment there is documented as absent rather than assumed.
|
Thanks — this was a genuinely useful review. Folded in as First, a scoping note: the review was done against The best catch: #3 / #4Correct, and worse than described. Dropping
Writing the test you asked for found a further bug you did not have visibility into: the timeout path reaped the child before signalling the group, and reaping clears the pid the group kill needs. Every grandchild survived every timeout. Group first, then reap. #1 permissionsThe Copilot finding was a real bug: I did not do the six-axis #7 session persistenceCorrect and embarrassing: the docs promised crash-resumability while the code only bound on a clean exit, which is precisely backwards. Assigned ids persist before spawning; printed ids persist the moment Everything else
Declined, with reasonsThe Windows Job Objects (#4). Unimplemented, and now documented as absent rather than quietly assumed.
Typed wire structs for 71 unit tests, 4 process-lifecycle tests, clippy clean under |
clear_env() was a footgun: it exposed an option known not to work, since an
empty environment breaks every agent until the caller re-adds PATH, HOME and
the CLI's credential variable. Which variables an agent needs is knowledge
about the agent, so the crate should hold it, not each caller.
EnvPolicy::{Inherit, Minimal, Only} replaces it, with Minimal defined by
Agent::essential_env(). The list is derived by experiment rather than assumed:
PATH + HOME alone fails, because Claude's keychain lookup is keyed on USER and
reports "Not logged in" without it. PATH + HOME + USER is the verified floor
for all three on macOS. The Windows names are reasoned, not verified.
Proxy and custom-CA variables are deliberately excluded. They are situational
rather than required, and HTTP_PROXY/HTTPS_PROXY routinely embed credentials,
so forwarding them automatically would leak one through the policy whose whole
purpose is withholding secrets. NETWORK_ENV names them so a host can offer them
as an explicit setting without hardcoding the list.
Also fixes a real bug that only a multi-turn Codex run could reveal:
`codex exec resume` does not accept --sandbox. It is a different option set
from `codex exec` and rejects the flag outright, so every second turn carrying
a permission posture failed with "unexpected argument '--sandbox'". Every
existing test was single-turn, so none caught it. The resume path now applies
the posture as `-c sandbox_mode=...`, verified live to both work and retain
context. Dropping the sandbox on resume instead would have silently continued a
conversation under a different posture than the caller asked for.
Tests: EnvPolicy::Minimal must withhold the host's own variables (using the
CARGO_* set cargo injects, so it needs no secret and no unsafe set_var) while
still passing PATH and HOME; every agent must still authenticate under it, so
an incomplete list fails loudly; no agent may request another's credentials or
any proxy variable. Codex resume is now covered end to end across two
processes, proving continuity comes from the captured thread_id and not from
reading $CODEX_HOME/sessions.
Narrowed the public API for real this time: Plan, Continue, Parser, Terminal
and STDIN_THRESHOLD were still exported because the earlier edit silently
failed to apply.
|
Pushed
|
ALL_PROXY/all_proxy are referenced by Claude Code and Codex but were missing from the list. Reworded the doc comment: it previously said all three CLIs "honour these", which rested on finding the names in the shipped binaries. That shows the names are referenced, not that provider traffic respects them. No vendor documents proxy support and none exposes a proxy flag, so the list is a convenience for hosts that want to forward these names, not a supported-behaviour claim.
It was public while returning `Continue`, which became private when the API was narrowed, so a public method returned a type no caller could name. CI caught it as a broken doc link, but the link was the symptom. Callers reach this through Request::session and can read the decision back via Request::session_phase, so nothing external loses access. Local verification now runs the full CI job (fmt, clippy, test, doctests, doc) rather than clippy and tests alone, which is why the doc step was missed.
Review #3: - Moved process-group teardown into src/proc.rs. The unsafe was already reconciled with the lint (deny rather than forbid, an #[allow] on the block, a SAFETY comment, CI green), but it sat inside the runner, so "where does this crate use unsafe and why" was not answerable from one place. It is now one small module whose docs explain why signalling a group has no safe equivalent in std and why a dependency for one call costs more than it saves. The #[allow] became #[expect], so removing the unsafe later flags the exception as stale instead of leaving it behind. - Recorded the Windows gap in AGENTS.md as an invariant rather than only in the README: cancellation tears down the whole tree on Unix and kills only the direct child on Windows, containing a tree there needs a Job Object, and tests/process.rs stays #![cfg(unix)] instead of passing vacuously elsewhere. - Explained the encoding tradeoff in session.rs. ASCII names stay readable on disk while non-ASCII ones get verbose, because a collision resumes the wrong conversation and unreadable beats wrong.
|
Review #3 addressed in 1. The concern that this "may cause clippy/CI to fail" was not the case: The underlying point still stood, though: the Taking option (b) instead, a safe wrapper, would mean adding 2. Windows no-op — promoted to an invariant. It was in the README's cancellation section but not in I have not filed a tracking issue, since that is @revenge's call on their repo. Say the word and I will. 3. Now states the actual bargain: an ASCII name stays readable on disk while a non-ASCII one gets verbose ( Also in this push: the PR description was stale, still describing Full CI job run locally before pushing this time (fmt, clippy, test, doctests, doc), which is what I should have been doing: the previous red build was the rustdoc step, not clippy, and I had been running only clippy and tests. |
Merge-blocking items, in order.
1. Cancellation is cooperative rather than an abort. Run::cancel now signals the
driver, which kills the process group, reaps the child and joins its stderr
reader before returning Error::Cancelled, so the tree really has exited when
the call returns. The previous version aborted the task and awaited the
JoinError, which killed the group but did not wait for the reap, so the doc
claim of "signalled and reaped" was not quite true. Drop cannot await, so it
signals and then aborts as a backstop. Covered by a test that checks the
grandchild immediately after cancel returns, with no grace period.
2. Output limits are now real. lines() accumulates until a newline, so a stream
that never emits one exhausted memory long before MAX_CAPTURE applied. Both
pipes use a bounded reader that stops accumulating past MAX_LINE and keeps
draining. Tool-call entries are removed when their result arrives and capped
at MAX_PENDING_TOOLS. Covered by a test that floods 64 MiB on a single line.
3. Session naming: "" and "unnamed" both encoded to "unnamed", so an empty name
collided with a caller who literally used that word. Empty now maps to "%",
which no other input can produce because a literal % always escapes. Also
stopped claiming the truncated-hash path preserves uniqueness absolutely; the
docs now state the real guarantee and its bound.
5. Assigned session ids are reserved before spawning rather than at the start of
the driver, closing the window where a half-successful spawn loses a binding
the caller may already be showing.
6. Redaction is derived from per-argument sensitivity recorded where each
argument is built, not reconstructed from the finished line. The heuristic
missed exactly the cases that matter: Codex's bare trailing prompt and
anything from unchecked_args. Both are now covered by tests.
7. COPILOT_ALLOW_ALL is out of the minimal environment. It is Copilot's env
equivalent of --allow-all-tools, so inheriting it let the host's ambient
environment widen a run's permissions behind Permission's back. This one was
a real hole.
8. Structured runs are validated: no recognized records, or no terminal record,
is Error::Parse rather than a silent fallback to raw stdout dressed up as an
answer. Raw fallback remains only for Format::Text, where the stream is the
answer.
9. The whole command line is size-checked, since moving the prompt to stdin does
not move Claude's --append-system-prompt or any unchecked argument. Returns
CommandLineTooLarge naming which input overflowed instead of leaving the OS
to answer E2BIG.
10. list() returns Result; only a missing directory is empty. list_lossy() is
the explicit skip-bad-records variant.
11. The session directory is fsynced after the rename, since syncing the file
persists its contents but not the entry that names it.
14. #[non_exhaustive] on the types expected to grow.
Deliberately not done: per-session cross-process leases (4), removing Codex's
git-check bypass (12, the maintainer's call given a review-only use case), the
full PreparedRun refactor (13, partly addressed by the typed argv), and typed
wire structs for the parsers (15).
Full CI locally and the live suite 11/11 against all three agents.
Full environment inheritance is what a CLI gets from a shell, but this crate runs inside processes that hold unrelated secrets, and inheriting hands every one of them to the agent and to every command the agent runs. The safe posture should be what a caller gets when they configure nothing, so Inherit is now the opt-in. Safe to flip because Minimal is verified rather than assumed: the live suite asserts every agent still authenticates under it, and all 11 live tests now exercise it by default rather than only the one test that asked for it. Deferred items from the review are now tracked instead of only discussed: per-session concurrency control as #2, and Codex's unconditional --skip-git-repo-check as #3.
|
Review #4 addressed across Merge-blocking items1. Cooperative cancellation. Valid, and my docs were overclaiming. 2. Output limits. This was a genuine hole. 3. Session naming. Found a live collision while acting on this: 5. Assigned ids are reserved before the spawn rather than at the top of the driver. 6. Typed redaction. Right that heuristics miss what matters. Sensitivity is now recorded where each argument is built, so Codex's bare trailing prompt and 7. 8. Structured runs that recognized nothing, or never reached a terminal record, now return 9. Whole command line size-checked, returning 10, 11, 14. Also, per @revenge
Deferred, now tracked rather than only discussed
|
1 participant
Replaces the multi-language oneharness stack with a single Rust library,
agent-abstraction, covering Claude Code, Codex and GitHub Copilot behind one API.Library only: no CLI, no binary. The consumer (agencyzero's
apps/gui) links it and spawns the agent directly, so nothing marshals a request through a command line and back out of stdout twice.What was dropped rather than ported
oneharnessbinary and re-validate its JSONoneharnessCLIWhat it does
Requestbuilder and onePermissionposture, mapped onto each CLI's own vocabulary.Eventvocabulary normalized from three different JSON dialects.Runkills the agent and its children;cancel()anddetach()are the explicit alternatives.EnvPolicy::Minimalpasses only what the selected agent needs, so an embedding host's unrelated secrets stay out of the agent and everything it runs.Five dependencies:
tokio,serde,serde_json,thiserror,uuid, pluslibcon Unix.Verified against the real CLIs, not inherited
Every flag mapping and output shape was checked against the installed binaries (
claude 2.1.205,codex-cli 0.145.0,Copilot CLI 1.0.75). That turned up several places where upstream is stale or the docs are silent:claude --session-id <uuid>lets the caller mint the id up front, so a run that dies mid-turn still leaves a resumable session. Copilot does the same through one flag in both directions. Codex cannot be told an id at all.codex exec resumedoes not accept--sandbox. It is a different option set fromcodex exec, so every second turn carrying a permission posture failed until the posture moved to-c sandbox_mode=. Only a multi-turn run reveals this.--deny-tool[=tools...]only binds with=. Across a space the value is read as a positional and the deny is silently lost, so a "read-only" run quietly wasn't.item.startedanditem.completedfor the same tool, which duplicated every tool event.USER.PATH+HOMEalone returns "Not logged in", which is howEnvPolicy::Minimal's variable list was derived rather than guessed.Where this crate and oneharness disagree, this crate matches what the CLI does today.
Capability gaps are loud
Asking an agent for something it cannot do is an
Error::Unsupported, never a quiet downgrade. A caller that asked to fork and silently got a linear resume would corrupt the conversation it meant to branch.thread_id)Known limits, stated rather than implied
Planis its read-only sandbox.tests/process.rsis#![cfg(unix)]rather than passing vacuously.unchecked_argsvoids the crate's guarantees by design, and is named for it.src/proc.rsholds the crate's onlyunsafe, onelibc::killfor group signalling;unsafe_codeisdenyrather thanforbidso that single use can be excepted.Operating limits
Quota refusals surface as
Error::RateLimitedcarrying the provider's own wording, and are never retried internally: a retry loop buried in a dependency is invisible to anyone reviewing the calling code. There is no account-rotation facility. Reasoning indocs/operating-limits.md.Testing
The live suite passes 11/11 against all three agents and covers answer, usage, streaming, multi-turn memory, forking, caller-assigned session ids, Codex resume across processes, and authentication under
EnvPolicy::Minimal.tests/process.rsproves cancellation with a fake agent that spawns a grandchild: drop, cancel and timeout kill the tree, detach does not.Review note
Cargo.lockis gitignored per library convention. If agencyzero pins this by git rather than path, we may want it committed instead.