diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 918aee5..ea23f24 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,11 +17,13 @@ env: jobs: check: name: fmt, clippy, test - # One GitHub-hosted runner for the complete decision. Audit used to be a - # second VM that repeated checkout, toolchain setup, and dependency - # resolution. Pin the image so an upstream `latest` change cannot move the - # toolchain underneath an otherwise unchanged pull request. - runs-on: ubuntu-24.04 + # One runner for the complete decision. Audit used to be a second VM that + # repeated checkout, toolchain setup, and dependency resolution. + # `-2` is deliberate: this job finishes in well under a minute, and + # Ubicloud bills by vCPU-minute, so a larger runner costs more for no + # wall-clock gain. The repository variable keeps a pool switch operational + # rather than requiring another workflow change. + runs-on: ${{ vars.UBICLOUD_RUNNER || 'ubicloud-standard-2-arm' }} steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index f0b71e4..2e29ae1 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -28,7 +28,7 @@ env: jobs: publish: name: publish to crates.io - runs-on: ubuntu-24.04 + runs-on: ${{ vars.UBICLOUD_RUNNER || 'ubicloud-standard-2-arm' }} permissions: # For pushing the version tag. contents: write diff --git a/AGENTS.md b/AGENTS.md index b6d3dac..5f6a29b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -52,11 +52,15 @@ Copilot CLIs headlessly behind one API. Consumed as a direct dependency by the ## CI runners -Use the explicitly pinned GitHub-hosted `ubuntu-24.04` image. Keep the complete CI -decision in one job so checkout, toolchain setup, dependency resolution, and runner -startup happen once per pull request. Do not add an external runner dependency without -an operational fallback: unavailable third-party capacity must not leave releases queued -indefinitely. +Use the configurable Ubicloud runner in both CI and publishing. The default is +`ubicloud-standard-2-arm`: this pure Rust crate finishes in well under a minute, and a +larger runner adds billed vCPU-minutes without shortening the decision. Keep the complete +CI decision in one job so checkout, toolchain setup, dependency resolution, and runner +startup happen once per pull request. + +Ubicloud must be enabled for the repository before its runners can pick up jobs. Change +the `UBICLOUD_RUNNER` repository variable when capacity moves rather than editing workflow +files or silently falling back to GitHub-hosted minutes. ## Build & run diff --git a/CHANGELOG.md b/CHANGELOG.md index e438ca4..0d75958 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,38 @@ appear in a patch rather than inflating the version toward 1.0 on a crate still shape. **Where that happens the entry says so at the top**, because a version number that under-signals is only acceptable if the changelog over-signals to compensate. +## 0.4.15 + +### Fixed + +- **Codex terminal usage now includes every model call in a tool-heavy turn.** + Interactive token updates report disjoint calls. Their additive fields are + accumulated while context-shaped fields remain latest-wins, so the terminal + outcome no longer collapses to only the final call. +- The verified Codex boundary is now CLI `0.146.0`. Visible models marked + `supported_in_api: false`, including inline-only + `gpt-5.3-codex-spark`, are excluded from the runnable catalogue. +- Copilot's flag mapping is verified against CLI `1.0.78`. + +## 0.4.14 + +### Added + +- **Named sessions now have cross-process concurrency control.** A run holds a + non-blocking OS lease across the full read-run-commit cycle. A second run on + the same project and session returns `Error::SessionBusy`, which is transient, + instead of forking the provider conversation and silently losing one binding. + The lease is released by the kernel if its holder is killed. + +## 0.4.13 + +### Fixed + +- **Writable Codex runs retain the git repository safety check.** + `--skip-git-repo-check` is now limited to ReadOnly and Plan, whose sandbox + cannot edit files. Edit, Auto, and Bypass no longer waive Codex's guard + against changes without a version-control recovery path. + ## 0.4.12 ### Fixed @@ -15,8 +47,7 @@ under-signals is only acceptable if the changelog over-signals to compensate. - **Codex Auto runs can use GitHub without an approval round trip.** The Codex workspace sandbox now enables network access for Auto while retaining its configured writable roots. Edit remains offline and Ask remains interactive. -- Release and pull request checks now use a pinned GitHub-hosted runner so an - unavailable external runner pool cannot leave a release queued indefinitely. +- Release and pull request checks use one configurable Ubicloud runner. ## 0.4.10 diff --git a/Cargo.toml b/Cargo.toml index 5877d74..f4303e8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "agent-abstraction" -version = "0.4.12" +version = "0.4.15" edition = "2024" # The floor edition 2024 requires, and where the strictest dependencies (uuid, # getrandom) sit. Derived from the dependency graph rather than compile-tested. @@ -39,6 +39,7 @@ tokio = { version = "1", features = ["process", "io-util", "sync", "time", "rt", serde = { version = "1", features = ["derive"] } serde_json = "1" thiserror = "2" +fs2 = "0.4.3" # Session ids are caller-minted UUIDv4 (`claude --session-id`, and Copilot's # handle, which the CLI never prints). v4 only, since these are opaque handles and not # sort keys, so the v7 timestamp would leak wall-clock into a stable id. diff --git a/README.md b/README.md index 496e269..36e8cd6 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ println!("{:?}", outcome.usage.cost_usd); ## What each agent can actually do -Verified live, against `claude 2.1.205`, `codex-cli 0.145.0` and `GitHub Copilot CLI 1.0.75` +Verified live, against `claude 2.1.205`, `codex-cli 0.146.0` and `GitHub Copilot CLI 1.0.78` and not inferred from documentation. | | session id | fork | events | system prompt | resume flag | @@ -129,6 +129,13 @@ in two checkouts never collides, and written through a temp file and a rename so concurrent reader never sees a half-written record. A corrupt record reads as absent: the next turn opens a fresh conversation rather than failing over a cache nobody asked about. +One named session can have only one run at a time, across processes. `stream()` takes a +non-blocking OS lease for the full read-run-commit cycle and returns `Error::SessionBusy` +when another holder is active. Rebuild or retry the request after that run settles; the +binding is re-read under the lease, so a request prepared while the prior run was active +cannot resume a stale token. The kernel releases the lease if its holder is killed, so a +crashed host cannot deadlock the session. + Names are percent-encoded into a single path segment, which is **injective**: two different names can never land on the same file. That matters more than it sounds, because the failure mode of a lossy scheme is silent, not loud. Folding unsafe characters to `-` would map @@ -240,7 +247,7 @@ assert_eq!(outcome.structured.unwrap()["name"], "Alice"); The delivery differs and is hidden: Claude takes the schema inline and reports the value in its own field, Codex reads it from a file this crate writes and removes, and returns the -value as its answer text. **Copilot 1.0.75 has no schema support at all**, so asking is an +value as its answer text. **Copilot 1.0.78 has no schema support at all**, so asking is an `Error::Unsupported` rather than prose dressed up as data. **Write schemas strictly.** Codex sends yours to OpenAI's structured-output API, which @@ -534,10 +541,10 @@ own variables do not reach the child. ## Gotchas worth knowing -- **`codex exec` refuses to run outside a git repository.** This crate always passes - `--skip-git-repo-check`, so it runs anywhere. That check exists to stop an agent editing - files with no way to undo them; the sandbox is the real containment here, and it defaults - to `read-only`. +- **`codex exec` refuses writable runs outside a git repository.** This crate passes + `--skip-git-repo-check` only for `ReadOnly` and `Plan`, where the sandbox prevents edits. + `Edit`, `Auto`, and `Bypass` retain Codex's guard because a change outside version control + may have no recovery path. - **`codex exec resume` does not accept `--sandbox`.** It is a different option set from `codex exec` and rejects the flag outright, so the permission posture is applied as `-c sandbox_mode=...` on the resume path. Only a multi-turn run reveals this: every @@ -766,7 +773,7 @@ A Rust port of [nickderobertis/oneharness](https://github.com/nickderobertis/one Some findings did not survive re-verification against the current CLIs. oneharness models Copilot as having no headless session id and no event stream (`session_formats: &[]`, -`events_format: None`); Copilot 1.0.75 has both. Where this crate and oneharness disagree, +`events_format: None`); Copilot 1.0.78 has both. Where this crate and oneharness disagree, this crate matches what the CLI does today. ## License diff --git a/src/agent.rs b/src/agent.rs index 142c3a5..f109c29 100644 --- a/src/agent.rs +++ b/src/agent.rs @@ -388,10 +388,10 @@ impl Agent { let (major, minor, patch) = match self { // `claude --version` -> "2.1.212 (Claude Code)" Agent::Claude => (2, 1, 212), - // `codex --version` -> "codex-cli 0.145.0" - Agent::Codex => (0, 145, 0), - // `copilot --version` -> "GitHub Copilot CLI 1.0.75." - Agent::Copilot => (1, 0, 75), + // `codex --version` -> "codex-cli 0.146.0" + Agent::Codex => (0, 146, 0), + // `copilot --version` -> "GitHub Copilot CLI 1.0.78." + Agent::Copilot => (1, 0, 78), }; crate::Version { major, @@ -536,7 +536,7 @@ impl Agent { live_follow_up: true, approvals: true, }, - // Verified against Copilot CLI 1.0.75: `--session-id ` both + // Verified against Copilot CLI 1.0.78: `--session-id ` both // mints a new session and resumes an existing one (one flag, both // directions), and `--output-format json` is a JSONL event stream. // There is no headless fork. @@ -545,7 +545,7 @@ impl Agent { fork: false, events: true, native_system: false, - // Copilot 1.0.75 exposes no schema flag at all. + // Copilot 1.0.78 exposes no schema flag at all. schema: SchemaSupport::None, commands: false, live_follow_up: false, @@ -643,7 +643,7 @@ impl Agent { /// # Errors /// [`Error::Unsupported`] if the plan needs a capability this agent lacks. pub(crate) fn typed_argv(self, plan: &Plan) -> Result> { - // Codex app-server supplies an approval callback. Copilot CLI 1.0.75 + // Codex app-server supplies an approval callback. Copilot CLI 1.0.78 // needs `--allow-all-tools` to run headlessly at all and gates only // through `--deny-tool`. A run that quietly never asked would be the // worst outcome here, since a caller would read silence as "nothing @@ -655,7 +655,7 @@ impl Agent { what: "routing tool approvals to the caller", }); } - // Codex app-server accepts `turn/steer`. Copilot CLI 1.0.75 has no + // Codex app-server accepts `turn/steer`. Copilot CLI 1.0.78 has no // structured input stream and cannot take a second message mid-turn. if plan.duplex && !caps.live_follow_up { return Err(Error::Unsupported { @@ -931,17 +931,18 @@ fn argv_codex(plan: &Plan) -> Vec { .arg_sensitive(id.clone(), Sensitivity::SessionId); } - // `codex exec` aborts outside a git repository unless told not to. 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, and a hard abort there is - // useless to them. The real containment is the sandbox below, which is - // `read-only` by default, so nothing is unrecoverable regardless. - a.bare("--skip-git-repo-check"); + // `codex exec` aborts outside a git repository unless told not to. Waiving + // that check is safe only while the sandbox cannot write: scratch + // directories and review exports remain readable, while Edit, Auto, and + // Bypass retain Codex's guard against changes with no version-control + // recovery path. + if matches!(plan.permission, Permission::ReadOnly | Permission::Plan) { + a.bare("--skip-git-repo-check"); + } // `codex exec` takes `--sandbox`, but `codex exec resume` does **not**: it // rejects the flag outright and takes the same setting as a `-c` config - // override instead. Verified against codex-cli 0.145.0, where passing + // override instead. Verified against codex-cli 0.146.0, where passing // `--sandbox` to a resume fails with "unexpected argument '--sandbox'". // Dropping the sandbox on resume would silently run a continued turn under a // different posture than the caller asked for. @@ -960,13 +961,13 @@ fn argv_codex(plan: &Plan) -> Vec { }; a.opt("--model", plan.model.as_ref()); - // Verified against codex-cli 0.145.0: `codex exec` has no effort flag, it + // Verified against codex-cli 0.146.0: `codex exec` has no effort flag, it // is a config override, and `--strict-config` accepts this key. A bad value // is refused by the provider with its own enum rather than by the CLI. if let Some(effort) = plan.effort.as_ref() { a.pair("-c", format!("model_reasoning_effort={effort}")); } - // Verified against codex-cli 0.145.0. Options remain options after the + // Verified against codex-cli 0.146.0. Options remain options after the // positional prompt, but keeping roots before it makes the command's // security posture readable and matches the CLI's help shape. for dir in &plan.extra_dirs { @@ -1000,7 +1001,7 @@ fn argv_codex(plan: &Plan) -> Vec { /// `copilot -p --allow-all-tools [...] [--session-id ]` /// -/// Flags verified against Copilot CLI 1.0.75. Two of its conventions matter: +/// Flags verified against Copilot CLI 1.0.78. Two of its conventions matter: /// `--allow-all-tools` is *required* for non-interactive mode, and the /// repeatable tool filters are declared `--allow-tool[=tools...]`, an optional /// value, which only binds with `=`, never across a space. @@ -1031,7 +1032,7 @@ fn argv_copilot(plan: &Plan) -> Vec { }; a.opt("--model", plan.model.as_ref()); - // Verified against Copilot CLI 1.0.75: `--effort` is the documented spelling + // Verified against Copilot CLI 1.0.78: `--effort` is the documented spelling // and `--reasoning-effort` its alias (none, minimal, low, medium, high, // xhigh, max). A wider set than Claude's, which is why the level is passed // through rather than mapped to a shared enum. @@ -1458,18 +1459,30 @@ mod tests { } } - /// `codex exec` aborts outside a git repository. A host embedding this - /// crate runs against scratch dirs and review checkouts, so the check is - /// waived on every invocation; the sandbox is what actually contains a run. + /// Read-only runs can inspect scratch dirs and review exports safely. A + /// writable posture keeps Codex's repository guard, since there may be no + /// way to undo a change outside version control. #[test] - fn codex_always_waives_the_git_repo_check() { + fn codex_waives_the_git_repo_check_only_without_writes() { for cont in [Continue::New, Continue::Resume("t-1".into())] { - let mut p = plan("codex"); - p.cont = cont.clone(); - assert!( - argv(Agent::Codex, &p).contains(&"--skip-git-repo-check".to_string()), - "{cont:?} must still run outside a repo" - ); + for permission in [Permission::ReadOnly, Permission::Plan] { + let mut p = plan("codex"); + p.cont = cont.clone(); + p.permission = permission; + assert!( + argv(Agent::Codex, &p).contains(&"--skip-git-repo-check".to_string()), + "{cont:?} {permission:?} must still read outside a repo" + ); + } + for permission in [Permission::Edit, Permission::Auto, Permission::Bypass] { + let mut p = plan("codex"); + p.cont = cont.clone(); + p.permission = permission; + assert!( + !argv(Agent::Codex, &p).contains(&"--skip-git-repo-check".to_string()), + "{cont:?} {permission:?} must keep Codex's repository guard" + ); + } } } @@ -1518,7 +1531,7 @@ mod tests { assert!(!a.iter().any(|arg| arg.contains("\"type\""))); } - /// Copilot 1.0.75 has no schema flag, and a prose answer presented as data + /// Copilot 1.0.78 has no schema flag, and a prose answer presented as data /// is exactly the silent downgrade this crate refuses elsewhere. #[test] fn copilot_refuses_a_schema_rather_than_answering_in_prose() { diff --git a/src/codex_app_server.rs b/src/codex_app_server.rs index c8af4f5..c21ea6a 100644 --- a/src/codex_app_server.rs +++ b/src/codex_app_server.rs @@ -288,7 +288,14 @@ impl Protocol { } "thread/tokenUsage/updated" => { if let Some(usage) = usage(¶ms) { - self.terminal.usage = usage; + // `last` is one model call, not the whole interactive + // turn. Tool-heavy turns receive one update after every + // call; replacing here made the live stream correctly add + // 1.7M processed tokens while the terminal outcome fell + // back to only its final 226k call. The snapshots are + // disjoint billing traffic, so accumulate their additive + // fields while keeping context-shaped fields latest. + self.terminal.usage.accumulate(&usage); step.events.push(Event::Usage(usage)); } } @@ -651,6 +658,28 @@ mod tests { assert_eq!(usage.context_window, Some(258_400)); } + #[test] + fn codex_terminal_usage_accumulates_every_model_call_in_the_turn() { + let mut protocol = Protocol::new(request()); + for (input, cached, output) in [(206_011, 188_160, 321), (206_692, 204_544, 285)] { + protocol.push(&json!({ + "method": "thread/tokenUsage/updated", + "params": {"tokenUsage": {"last": { + "inputTokens": input, + "cachedInputTokens": cached, + "outputTokens": output, + "reasoningOutputTokens": 0 + }, "modelContextWindow": 997_500}}, + })); + } + + assert_eq!(protocol.terminal.usage.input_tokens, Some(19_999)); + assert_eq!(protocol.terminal.usage.cache_read_tokens, Some(392_704)); + assert_eq!(protocol.terminal.usage.output_tokens, Some(606)); + assert_eq!(protocol.terminal.usage.context_tokens, Some(206_692)); + assert_eq!(protocol.terminal.usage.context_window, Some(997_500)); + } + #[test] fn completed_agent_messages_preserve_their_boundary() { let mut protocol = Protocol::new(request()); diff --git a/src/error.rs b/src/error.rs index c9ccaf4..56f8e05 100644 --- a/src/error.rs +++ b/src/error.rs @@ -117,6 +117,19 @@ pub enum Error { requested: Agent, }, + /// Another run currently owns the same named session. + /// + /// The lease is cross-process and ends when that run settles or its process + /// dies. Retrying later is safe; running both would fork the provider's + /// conversation and leave the store pointing at whichever finished last. + #[error("session `{name}` for project `{project}` already has a run in progress")] + SessionBusy { + /// The caller-owned session name. + name: String, + /// The project namespace containing it. + project: String, + }, + /// The session store could not be read or written. #[error("session store I/O failed at {path}: {source}")] Store { @@ -256,15 +269,19 @@ pub enum Error { impl Error { /// Whether retrying this exact request later could plausibly succeed. /// - /// True for quota and timeout failures; false for a missing binary, an - /// unsupported capability, or a session conflict, which need the caller to - /// change something first. This classifies; it does not retry. A caller - /// must stop the old run before retrying [`Error::ControlTimeout`]. + /// True for quota and timeout failures, and for a named session whose prior + /// run has not settled yet. False for a missing binary, an unsupported + /// capability, or an agent mismatch, which need the caller to change + /// something first. This classifies; it does not retry. A caller must stop + /// the old run before retrying [`Error::ControlTimeout`]. #[must_use] pub fn is_transient(&self) -> bool { matches!( self, - Error::RateLimited { .. } | Error::Timeout { .. } | Error::ControlTimeout { .. } + Error::RateLimited { .. } + | Error::Timeout { .. } + | Error::ControlTimeout { .. } + | Error::SessionBusy { .. } ) } diff --git a/src/model.rs b/src/model.rs index 36bf63b..89b78d4 100644 --- a/src/model.rs +++ b/src/model.rs @@ -166,8 +166,8 @@ impl Agent { }, Agent::Codex => Verified { source: Source::Cli, - checked: "2026-07-29", - against: "codex-cli 0.145.0", + checked: "2026-08-07", + against: "codex-cli 0.146.0", }, // Read from the `/model` picker. Copilot has no headless list; see // `discover_models`. @@ -382,8 +382,8 @@ fn claude_pinned() -> Vec { /// Codex, in the priority order the CLI itself reports. /// -/// Verified by running `codex debug models` against codex-cli 0.145.0 on -/// 2026-07-29. `codex-auto-review` is reported with `visibility: "hide"` and is +/// Verified by running `codex debug models` against codex-cli 0.146.0 on +/// 2026-08-07. `codex-auto-review` is reported with `visibility: "hide"` and is /// left out for that reason; [`discover_codex`] applies the same filter. fn codex_models() -> Vec { const FULL: &[&str] = &["low", "medium", "high", "xhigh", "max", "ultra"]; @@ -557,10 +557,12 @@ fn parse_codex_models(stdout: &str) -> Result> { // so it is read rather than assumed. let mut ranked: Vec<(u64, Model)> = listed .iter() - // `visibility` is how Codex marks its internal models, and - // `codex-auto-review` is one. Offering it in a picker hands a user a - // model the vendor deliberately withheld. + // `visibility` marks internal models such as `codex-auto-review`. + // Separately, Codex can list a visible model for inline coding while + // saying `supported_in_api: false`; it is not runnable through this + // crate's app-server/exec paths and must not reach their picker. .filter(|m| m.get("visibility").and_then(Value::as_str) != Some("hide")) + .filter(|m| m.get("supported_in_api").and_then(Value::as_bool) != Some(false)) .filter_map(|m| { let id = m.get("slug").and_then(Value::as_str)?; let model = Model { @@ -628,6 +630,9 @@ mod tests { "supported_reasoning_levels":[{"effort":"low"},{"effort":"medium"},{"effort":"high"}]}, {"slug":"codex-auto-review","display_name":"Codex Auto Review","description":"Internal.", "visibility":"hide","priority":43,"supported_reasoning_levels":[{"effort":"low"}]}, + {"slug":"gpt-5.3-codex-spark","display_name":"GPT-5.3-Codex-Spark","description":"Inline only.", + "visibility":"list","supported_in_api":false,"priority":26, + "supported_reasoning_levels":[{"effort":"high"}]}, {"slug":"gpt-5.6-sol","display_name":"GPT-5.6-Sol","description":"Latest frontier model.", "default_reasoning_level":"low","visibility":"list","priority":1, "supported_reasoning_levels":[{"effort":"low"},{"effort":"ultra"}]} @@ -667,6 +672,15 @@ mod tests { ); } + #[test] + fn codex_discovery_drops_visible_models_that_are_not_api_supported() { + let models = parse_codex_models(CODEX_OUTPUT).expect("should parse"); + assert!( + !models.iter().any(|m| m.id == "gpt-5.3-codex-spark"), + "inline-only models cannot run through the crate's API paths" + ); + } + #[test] fn unparseable_output_is_an_error_not_an_empty_list() { assert!(matches!( diff --git a/src/request.rs b/src/request.rs index 1e6cc61..e8bd5c4 100644 --- a/src/request.rs +++ b/src/request.rs @@ -62,6 +62,7 @@ pub(crate) struct Binding { pub(crate) project: PathBuf, pub(crate) name: String, pub(crate) phase: Phase, + pub(crate) fork: bool, } impl Request { @@ -335,7 +336,7 @@ impl Request { /// /// The two CLIs that support this take it differently, and the difference /// is hidden: Claude accepts the schema inline, Codex reads it from a file - /// this crate writes for the run and removes afterwards. **Copilot 1.0.75 + /// this crate writes for the run and removes afterwards. **Copilot 1.0.78 /// has no schema support**, so asking is [`crate::Error::Unsupported`] /// rather than a prose answer presented as data. /// @@ -425,6 +426,7 @@ impl Request { project, name, phase, + fork, }); // A named session needs an id back. The default format carries one, so // this only has to refuse a format the caller pinned that cannot: diff --git a/src/run.rs b/src/run.rs index 13acace..331b5d3 100644 --- a/src/run.rs +++ b/src/run.rs @@ -484,6 +484,26 @@ pub fn stream(request: &Request) -> Result { // hide that, so the context is checked and reported as an ordinary error. let runtime = tokio::runtime::Handle::try_current().map_err(|_| Error::NoRuntime)?; + let mut request = request.clone(); + let session_lease = if let Some(binding) = &request.binding { + let lease = binding.store.lease(&binding.project, &binding.name)?; + // `Request::session` may have been built while a prior run still held + // the lease. Re-plan now, under the lease, so retrying that same request + // resumes the binding the prior run committed rather than starting from + // the stale token it observed earlier. + let (phase, cont) = + binding + .store + .plan(request.agent, &binding.project, &binding.name, binding.fork)?; + request.cont = cont; + if let Some(binding) = &mut request.binding { + binding.phase = phase; + } + Some(lease) + } else { + None + }; + let initial_plan = request.plan(); let codex_app_server = request.agent == crate::Agent::Codex && (initial_plan.duplex || initial_plan.approvals); @@ -500,7 +520,6 @@ pub fn stream(request: &Request) -> Result { } _ => None, }; - let mut request = request.clone(); if let Some(file) = &schema_file { request.schema_file = Some(file.0.display().to_string()); } @@ -605,6 +624,10 @@ pub fn stream(request: &Request) -> Result { let reaped_for_task = std::sync::Arc::clone(&reaped); let request = request.clone(); let task = runtime.spawn(async move { + // Held across the whole read-run-commit cycle. It deliberately lives in + // the driver task rather than `Run`, so `detach` keeps the session + // reserved until the background agent actually exits. + let _session_lease = session_lease; // Moved in so the file outlives the run and is removed with it. let _schema_file = schema_file; if codex_app_server { diff --git a/src/session.rs b/src/session.rs index 8e50f73..18a4dbe 100644 --- a/src/session.rs +++ b/src/session.rs @@ -63,6 +63,22 @@ pub struct SessionStore { dir: PathBuf, } +/// An exclusive lease on one named session. +/// +/// The file remains on disk after release, but the OS lock does not: closing +/// the handle, including when a process is killed, releases it. Keeping the +/// inert file avoids an unlink race where a new opener could lock a different +/// inode while the prior holder still owns the old one. +pub(crate) struct SessionLease { + file: fs::File, +} + +impl Drop for SessionLease { + fn drop(&mut self) { + let _ = fs2::FileExt::unlock(&self.file); + } +} + impl SessionStore { /// A store rooted at `dir`. The directory is created lazily on first write. pub fn open(dir: impl Into) -> Self { @@ -99,6 +115,41 @@ impl SessionStore { .join(format!("{}.json", encode_segment(name))) } + /// Claim one named session until the returned guard is dropped. + /// + /// Advisory file locks are cross-process and are released by the OS when a + /// holder dies, so a crashed agent host cannot strand a stale lease. The + /// lock is non-blocking: a queue or server can report contention and decide + /// its own retry policy instead of tying up a worker indefinitely. + pub(crate) fn lease(&self, project: &Path, name: &str) -> Result { + let path = self.path_of(project, name).with_extension("lock"); + let store_err = |source| Error::Store { + path: path.display().to_string(), + source, + }; + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).map_err(store_err)?; + restrict_to_owner(parent).map_err(store_err)?; + } + let file = fs::OpenOptions::new() + .create(true) + .truncate(false) + .read(true) + .write(true) + .open(&path) + .map_err(store_err)?; + match fs2::FileExt::try_lock_exclusive(&file) { + Ok(()) => Ok(SessionLease { file }), + Err(error) if error.kind() == std::io::ErrorKind::WouldBlock => { + Err(Error::SessionBusy { + name: name.to_string(), + project: project.display().to_string(), + }) + } + Err(error) => Err(store_err(error)), + } + } + /// The stored record, or `None` when there is none. /// /// Only a genuinely absent file is `Ok(None)`. A permission error, an I/O @@ -152,8 +203,10 @@ impl SessionStore { let mut out = Vec::new(); for entry in entries { let path = entry.map_err(|e| store_err(&dir, e))?.path(); - // Skip the temp files a concurrent write may have in flight. - if path.extension().is_some_and(|ext| ext == "tmp") { + // Only records belong in the result. Temp files can be present + // during an atomic write and `.lock` files persist so every process + // coordinates on the same inode. + if path.extension().is_none_or(|ext| ext != "json") { continue; } let text = fs::read_to_string(&path).map_err(|e| store_err(&path, e))?; diff --git a/tests/live.rs b/tests/live.rs index 0ccf369..3052aa7 100644 --- a/tests/live.rs +++ b/tests/live.rs @@ -151,6 +151,10 @@ async fn the_codex_catalogue_still_matches_what_codex_reports() { !discovered.iter().any(|m| m.id == "codex-auto-review"), "a model codex marks hidden must not reach a picker" ); + assert!( + !discovered.iter().any(|m| m.id == "gpt-5.3-codex-spark"), + "a visible model codex marks unsupported in the API must not reach a picker" + ); } /// Both of these have an interactive picker and no headless listing, so asking @@ -916,10 +920,9 @@ async fn copilot_answers_and_reports_a_session() { ); } -/// `codex exec` aborts outside a git repository unless the check is waived. -/// This crate waives it on every invocation, so a run from a scratch directory -/// must still work. Running the rest of the suite from the repo would never -/// catch a regression here, because the repo *is* a git checkout. +/// A read-only `codex exec` can safely inspect a non-repository scratch +/// directory. Running the rest of the suite from the repo would never catch a +/// regression here, because the repo already satisfies Codex's guard. #[tokio::test] #[ignore = "spawns a real agent and consumes quota"] async fn codex_runs_outside_a_git_repository() { @@ -933,9 +936,11 @@ async fn codex_runs_outside_a_git_repository() { "the point of this test is that it is not a repo" ); - let outcome = run(&ping(Agent::Codex).cwd(&scratch)) - .await - .expect("codex refused to run outside a git repo"); + let outcome = run(&ping(Agent::Codex) + .cwd(&scratch) + .permission(Permission::ReadOnly)) + .await + .expect("codex refused to run outside a git repo"); assert!(outcome.is_ok(), "unexpected stop: {outcome:?}"); assert!( diff --git a/tests/process.rs b/tests/process.rs index 9f478e9..adb1278 100644 --- a/tests/process.rs +++ b/tests/process.rs @@ -13,7 +13,7 @@ use std::path::{Path, PathBuf}; use std::time::Duration; -use agent_abstraction::{Agent, EnvPolicy, Request, stream}; +use agent_abstraction::{Agent, EnvPolicy, Error, Request, SessionStore, stream}; /// A scratch directory unique to one test. fn scratch(tag: &str) -> PathBuf { @@ -50,6 +50,20 @@ fn fake_agent(dir: &Path) -> PathBuf { script } +/// Write a quiet long-running process for session lease tests. +fn sleeping_agent(dir: &Path) -> PathBuf { + use std::os::unix::fs::PermissionsExt as _; + + let script = dir.join("sleeping-agent.sh"); + std::fs::write( + &script, + "#!/bin/sh\necho '{\"type\":\"system\",\"session_id\":\"session-from-agent\"}'\nsleep 120\n", + ) + .unwrap(); + std::fs::set_permissions(&script, std::fs::Permissions::from_mode(0o700)).unwrap(); + script +} + /// The process state as `ps` reports it, or `None` if the pid is gone. fn process_state(pid: i32) -> Option { let out = std::process::Command::new("ps") @@ -110,6 +124,46 @@ async fn wait_until_dead(pid: i32, limit: Duration) -> bool { /// still fails the test rather than hanging it. const TEARDOWN_GRACE: Duration = Duration::from_secs(10); +/// The session lease covers the whole run, not just the atomic record write. +/// A second host must fail before spawning and leave the sole binding intact; +/// once the holder settles, the same name is immediately usable again. +#[tokio::test] +async fn one_named_session_admits_exactly_one_run() { + let dir = scratch("session-lease"); + let script = sleeping_agent(&dir); + let store = SessionStore::open(dir.join("sessions")); + let project = dir.join("project"); + let request = || { + Request::new(Agent::Claude, "hi") + .bin(script.to_str().unwrap()) + .session(&store, &project, "thread-42", false) + .unwrap() + }; + + let first = stream(&request()).expect("first run should claim the session"); + let binding = store.get(&project, "thread-42").unwrap().unwrap(); + + let conflict = stream(&request()).expect_err("second run must not share the session"); + assert!( + matches!(conflict, Error::SessionBusy { ref name, .. } if name == "thread-42"), + "got {conflict:?}" + ); + assert!(conflict.is_transient(), "the holder can settle and free it"); + assert_eq!( + store.get(&project, "thread-42").unwrap().unwrap(), + binding, + "a rejected concurrent run must not rewrite the binding" + ); + + let cancelled = first.cancel().await.unwrap_err(); + assert!(cancelled.is_cancelled()); + + let next = stream(&request()).expect("the settled holder must release the session"); + let cancelled = next.cancel().await.unwrap_err(); + assert!(cancelled.is_cancelled()); + std::fs::remove_dir_all(&dir).ok(); +} + /// The default that matters for a GUI: closing a window must stop the agent, /// not leave it running invisibly and spending quota. #[tokio::test]