diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e23fc2..eab0e0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,60 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [Unreleased] +## [3.0.0] - 2026-05-20 + +### Added (Phase 8 — typed-error SDK alignment) + +- New public `ToolErrorKind` enum (`#[non_exhaustive]`, JSON-tagged on + the `type` discriminator) carries structured tool failure reasons + from the Rust core all the way to SDK callers without losing the + type. Six variants: `version_conflict`, `remote_git_conflict`, + `not_found`, `invalid_argument`, `unsupported`, `timeout`. +- New optional `error_kind` field on `ToolOutput`, `ToolResult`, and + `ToolCallResult`, plus a matching field on `AgentEvent::ToolEnd` for + streaming consumers. +- Built-in `edit` and `patch` tools populate `error_kind` via + `ToolErrorKind::from_workspace_error` whenever a `WorkspaceError` + variant maps to a typed kind. The human-readable `output` / + `content` message is unchanged so the model still gets the retry + hint; SDK callers now have a programmatic discriminator next to it. +- Node SDK: new `errorKindJson` field on `ToolResult` and `AgentEvent` + (JSON-encoded `ToolErrorKind`) plus a new `ToolErrorKind` TypeScript + discriminated-union type in `index.d.ts`. +- Python SDK: new `error_kind_json` (raw) and `error_kind` (parsed + dict) properties on `ToolResult` and `AgentEvent`. + +This closes the v3.0 typed-error gap: until this commit the typed +`WorkspaceError` enum on the Rust trait surface was effectively +re-stringified at the SDK boundary, forcing JS/Python callers to +regex-match the output to detect e.g. concurrent-modification +conflicts. They now `switch` / `match` on `error_kind.type` instead. + +### ⚠️ Breaking changes (3.0.0) + +- **`WorkspaceFileSystem` and `WorkspaceFileSystemExt` trait methods now + return `WorkspaceResult` instead of `anyhow::Result`.** The new + result type wraps the typed `WorkspaceError` enum + (`#[non_exhaustive]`) with structured variants for `NotFound`, + `VersionConflict`, `RemoteGitConflict`, `InvalidArgument`, `Timeout`, + `Unsupported`, and a `Backend(anyhow::Error)` catch-all. Callers that + used `?` to lift errors into `anyhow::Result` keep working unchanged + thanks to the blanket `From for anyhow::Error` impl; + callers that previously did `err.downcast_ref::()` + now `match` on the typed variant directly: + ```rust + // before: + if e.downcast_ref::().is_some() { ... } + // after: + if matches!(e, WorkspaceError::VersionConflict(_)) { ... } + ``` + `WorkspaceServices::read_for_edit`, `write_for_edit`, and the generic + `run_with_timeout` (now polymorphic in the error type) follow the + same shape. The other 5 traits (`WorkspaceCommandRunner`, + `WorkspaceSearch`, `WorkspaceGit`, `WorkspaceGitStashProvider`, + `WorkspaceGitWorktreeProvider`) **still return `anyhow::Result`** — + their migration to `WorkspaceResult` will be additive (non-breaking) + in a future v3.x release. ### Added @@ -24,6 +77,113 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Exposed `S3WorkspaceBackend` in the Node and Python SDKs alongside `LocalWorkspaceBackend`. Configuration uses the same option surface (`workspaceBackend` / `workspace_backend`). +- `S3WorkspaceBackend::read_text` now enforces a configurable size ceiling + (`S3BackendConfig::max_read_bytes`, default 10 MiB) by inspecting + `Content-Length` on the `GetObject` response before consuming the body. + Oversized objects are rejected with a clear error and never buffered + into memory. Responses without a `Content-Length` header are refused + rather than risking OOM. +- Added optional `WorkspaceFileSystemExt` trait for backends that expose + compare-and-swap writes, plus a `WorkspaceVersionConflict` error type. + `S3WorkspaceBackend` implements it via ETag + `If-Match` on `PutObject`. + The `edit` and `patch` tools now capture the ETag during the read and + reject the write on version mismatch (HTTP 412), surfacing a typed + "Concurrent modification detected" error so the model can re-read and + retry instead of silently clobbering a concurrent writer. + `WorkspaceServices::read_for_edit` and `write_for_edit` are the new + helpers tools should use for any read-modify-write cycle; backends + without versioning (e.g. local) transparently fall through to plain + `read_text` / `write_text`. +- `S3WorkspaceBackend` now implements `WorkspaceSearch` (degraded `grep` / + `glob` via `LIST` + `GET` + regex). Off by default; opt in via + `S3BackendConfig::enable_search(true)`. Hard ceilings on objects scanned + per call (`max_objects_scanned`, default 500) and per-object body size + for `grep` (`max_grep_bytes_per_object`, default 1 MiB) bound the API + cost. Hitting either ceiling sets `WorkspaceGrepResult::truncated = true`. + Glob patterns follow the local backend's recursion convention: `*.rs` + matches the immediate level, `**/*.rs` recurses. +- `S3WorkspaceBackend::grep` now downloads candidate objects in parallel + via `futures::stream::buffer_unordered`. Concurrency defaults to 8 and + is configurable via `S3BackendConfig::search_concurrency` (also + exposed on both SDKs). Output ordering remains deterministic — results + are sorted by workspace path before assembly — so callers see the same + layout regardless of S3 response timing. + +### Added + +- Internal `workspace::conformance` module (test-only) codifies the + behavioural invariants every backend implementing + `WorkspaceFileSystem` (and optionally `WorkspaceFileSystemExt`) must + satisfy. Two public entry points, `assert_filesystem_conformance` and + `assert_filesystem_ext_conformance`, are run against + `LocalWorkspaceBackend` and a new `InMemoryFileSystem` reference + backend so the contract is exercised both over real I/O and an ideal + HashMap-backed implementation. Future backends (GCS, container, + browser) gain a regression suite for free — when the conformance set + grows after a production incident, every backend running it picks up + the new test automatically. + +### Fixed + +- `WorkspaceServices::with_remote_git` previously rebuilt the services + through `WorkspaceServicesBuilder`, which silently dropped `local_root` + (and would silently drop any future field). The decorator now goes + through a new internal `with_git_provider` helper that uses an explicit + struct literal — adding a new field to `WorkspaceServices` now triggers + a compile error in every decorator, forcing a deliberate decision. +- `RemoteGitBackend::diff` previously deserialised the entire response + body before applying `max_diff_bytes`, so a misbehaving gitserver + returning a multi-gigabyte JSON could exhaust client memory. The diff + path now streams the body with a hard cap (`max_diff_bytes * 4`, floor + 64 KiB), rejecting requests upfront when `Content-Length` advertises an + oversized body and aborting the stream mid-flight when chunked encoding + hides the size. The soft `max_diff_bytes` display truncation is + unchanged. + +### Changed + +- `S3WorkspaceBackend::list_dir` now errors with "S3 path not found" when + the LIST returns zero entries on a non-root path, matching the local + backend's behaviour. Previously a missing prefix silently returned + `Ok(vec![])`, masking typos. Paths that exist only as S3 zero-byte + directory markers still return `Ok(vec![])`. +- Every S3 API call (`GET`, `PUT`, `LIST`) on `S3WorkspaceBackend` now + emits a structured `tracing::debug!` event with fields `op`, `bucket`, + `target`, `bytes`, `outcome`, `duration_ms`. Hosts can meter S3 cost + by subscribing to these events without the backend taking a dependency + on any metrics framework. +- Node and Python SDKs now expose the workspace hardening options added + in this release. The Node `JsS3BackendConfig` and Python + `S3WorkspaceBackend` constructor accept `maxReadBytes` / + `max_read_bytes`, `searchEnabled` / `search_enabled`, + `maxObjectsScanned` / `max_objects_scanned`, and + `maxGrepBytesPerObject` / `max_grep_bytes_per_object`. A new + `RemoteGitBackendConfig` class (Python) / `JsRemoteGitBackendConfig` + shape (Node) and a top-level `remoteGit` / `remote_git` session + option let SDK callers attach `RemoteGitBackend` on top of any + workspace backend. Passing `remoteGit` without `workspaceBackend` + raises a clear error. +- Added `RemoteGitBackend` — an HTTP/JSON `WorkspaceGit` client that + brings the `git` tool to non-local workspaces (S3 today; future + container / DFS). Implements `WorkspaceGit` in full and + `WorkspaceGitStashProvider`; deliberately omits `WorkspaceGitWorktreeProvider` + because worktrees do not map to a remote service. The protocol is + specified in `apps/docs/content/docs/en/code/rfcs/workspace-remote-git.mdx`. + - New types: `RemoteGitBackend`, `RemoteGitBackendConfig`, + `RemoteGitConflict` (anyhow-downcastable for recoverable 409 / 422 + responses such as `WORKING_TREE_DIRTY` and `BRANCH_EXISTS`). + - New factory: `WorkspaceServices::with_remote_git(config)` on any + existing `Arc` to attach remote git on top of an + S3 (or local) filesystem backend. + - Client-side ceilings: `request_timeout` (default 30 s), + `max_log_entries` (default 200), `max_diff_bytes` (default 1 MiB). + - Per-call `tracing::debug!` event with fields `op`, `repo_id`, + `status`, `bytes`, `outcome`, `duration_ms`, mirroring the S3 + metering shape so a single subscriber meters both. + - Authentication: bearer token (header `Authorization: Bearer `) + or mTLS via `client_cert_pem` + `client_key_pem` (PKCS#8 PEM key for + the `rustls-tls` backend). Setting only one of the mTLS pair fails + at construction. ### Changed diff --git a/Cargo.lock b/Cargo.lock index 79b9929..b5a2868 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -37,7 +37,7 @@ dependencies = [ [[package]] name = "a3s-code-core" -version = "2.6.0" +version = "3.0.0" dependencies = [ "a3s-acl 0.2.0", "a3s-ahp", @@ -69,6 +69,7 @@ dependencies = [ "opentelemetry_sdk 0.27.1", "pdf-extract", "pin-project-lite", + "rcgen", "regex", "reqwest 0.11.27", "roxmltree", @@ -90,6 +91,7 @@ dependencies = [ "tracing-opentelemetry", "tracing-subscriber", "uuid", + "wiremock", "zip 0.6.6", ] @@ -302,6 +304,16 @@ dependencies = [ "derive_arbitrary", ] +[[package]] +name = "assert-json-diff" +version = "2.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "47e4f2b81832e72834d7518d8487a0396a28cc408186a2e8854c0f98011faf12" +dependencies = [ + "serde", + "serde_json", +] + [[package]] name = "async-attributes" version = "1.1.2" @@ -1408,6 +1420,24 @@ version = "2.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d7a1e2f27636f116493b8b860f5546edb47c8d8f8ea73e1d2a20be88e28d1fea" +[[package]] +name = "deadpool" +version = "0.12.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0be2b1d1d6ec8d846f05e137292d0b89133caf95ef33695424c09568bdd39b1b" +dependencies = [ + "deadpool-runtime", + "lazy_static", + "num_cpus", + "tokio", +] + +[[package]] +name = "deadpool-runtime" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "092966b41edc516079bdf31ec78a2e0588d1d0c08f78b91d8307215928642b2b" + [[package]] name = "deranged" version = "0.5.8" @@ -2941,6 +2971,16 @@ dependencies = [ "unicode-normalization", ] +[[package]] +name = "pem" +version = "3.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1d30c53c26bc5b31a98cd02d20f25a7c8567146caf63ed593a9d87b2775291be" +dependencies = [ + "base64 0.22.1", + "serde_core", +] + [[package]] name = "percent-encoding" version = "2.3.2" @@ -3360,6 +3400,19 @@ dependencies = [ "crossbeam-utils", ] +[[package]] +name = "rcgen" +version = "0.13.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "75e669e5202259b5314d1ea5397316ad400819437857b90861765f24c4cf80a2" +dependencies = [ + "pem", + "ring", + "rustls-pki-types", + "time", + "yasna", +] + [[package]] name = "redox_syscall" version = "0.5.18" @@ -5373,6 +5426,29 @@ version = "0.0.19" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d135d17ab770252ad95e9a872d365cf3090e3be864a34ab46f48555993efc904" +[[package]] +name = "wiremock" +version = "0.6.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08db1edfb05d9b3c1542e521aea074442088292f00b5f28e435c714a98f85031" +dependencies = [ + "assert-json-diff", + "base64 0.22.1", + "deadpool", + "futures", + "http 1.4.0", + "http-body-util", + "hyper 1.9.0", + "hyper-util", + "log", + "once_cell", + "regex", + "serde", + "serde_json", + "tokio", + "url", +] + [[package]] name = "wit-bindgen" version = "0.51.0" @@ -5493,6 +5569,15 @@ version = "0.13.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "66fee0b777b0f5ac1c69bb06d361268faafa61cd4682ae064a171c16c433e9e4" +[[package]] +name = "yasna" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e17bb3549cc1321ae1296b9cdc2698e2b6cb1992adfa19a8c72e5b7a738f44cd" +dependencies = [ + "time", +] + [[package]] name = "yoke" version = "0.8.2" diff --git a/README.md b/README.md index b6fef65..e961244 100644 --- a/README.md +++ b/README.md @@ -524,7 +524,11 @@ let config = S3BackendConfig::new( ) .endpoint("https://minio.local:9000") // omit for AWS S3 .region("us-east-1") -.force_path_style(true); // true for MinIO/RustFS, false for AWS +.force_path_style(true) // true for MinIO/RustFS, false for AWS +.max_read_bytes(10 * 1024 * 1024) // optional; default 10 MiB per read +.enable_search(true) // optional; off by default — see notes below +.max_objects_scanned(500) // optional; cap on objects per grep/glob +.max_grep_bytes_per_object(1 << 20); // optional; per-object cap for grep let session = agent.session( "s3://workspace/users/u1/sessions/s1", Some(SessionOptions::new().with_workspace_backend(WorkspaceServices::s3(config))), @@ -567,9 +571,164 @@ opts.workspace_backend = S3WorkspaceBackend( session = agent.session(workspace_uri, opts) ``` -S3 has no atomic read-modify-write, so concurrent writers to the same key may -overwrite each other. Partition workspaces per session/user via the `prefix` -field when running multi-tenant. +The S3 backend implements optimistic concurrency for read-modify-write +flows: `edit` and `patch` capture the object ETag during the read and apply +the write with `If-Match`, so a concurrent overwrite causes the second +writer to fail with a typed `WorkspaceVersionConflict` rather than silently +clobbering the first one. The tool surfaces a "Concurrent modification +detected" error and the model can re-read and retry. Partition workspaces +per session/user via the `prefix` field when running multi-tenant — the +optimistic check is a safety net, not a coordination mechanism. + +The backend rejects any single read that exceeds `max_read_bytes` (default +10 MiB) by inspecting `Content-Length` before consuming the response body, +so a stray `read` on a 1 GiB object can never OOM the agent process. Raise +the cap explicitly when reading larger text artifacts is legitimate. + +`grep` and `glob` are off by default — object storage has no native search, +so the only viable strategy is `LIST` + `GET` + regex, which can be slow +and expensive. Opt in with `.enable_search(true)`; the backend then caps +the number of objects considered per call (`max_objects_scanned`) and the +per-object body size for `grep` downloads (`max_grep_bytes_per_object`), +and reports `truncated=true` when either limit is hit. Object downloads +during `grep` run in parallel up to `search_concurrency` (default 8) — +tune lower when the S3 endpoint rate-limits aggressively. Glob patterns +follow the same recursion convention as the local backend: `*.rs` matches +only the immediate level, `**/*.rs` recurses. + +`ls` on a path that does not exist on S3 now errors out with +"S3 path not found", matching local-filesystem semantics — previously the +LIST silently returned an empty entry list, which made typos hard to +spot. A path with only an S3-style zero-byte directory marker still +returns `Ok(empty)`. + +Every S3 API call (`GET`, `PUT`, `LIST`) emits a structured `tracing` +event at `DEBUG` level under this module's target with fields `op`, +`bucket`, `target` (key or prefix), `bytes`, `outcome`, and +`duration_ms`. Hosts can subscribe to these to meter S3 cost without +the backend taking a dependency on any specific metrics framework. + +#### Remote Git Backend + +Object storage cannot host a `.git` directory, so the `git` tool stays +hidden on an S3-only workspace. Attach a `RemoteGitBackend` to a +host-operated gitserver to bring `git status`, `log`, `branch`, +`checkout`, `diff`, `remote`, and `stash` back to cloud sessions. The +client speaks the small HTTP/JSON protocol described in the +[Remote WorkspaceGit RFC](apps/docs/content/docs/en/code/rfcs/workspace-remote-git.mdx). + +```rust +use a3s_code_core::{ + Agent, RemoteGitBackendConfig, S3BackendConfig, SessionOptions, + WorkspaceServices, +}; + +# async fn run() -> anyhow::Result<()> { +let agent = Agent::new("agent.acl").await?; + +let ws = WorkspaceServices::s3( + S3BackendConfig::new( + "workspace", + "users/u1/sessions/s1", + "AKIA...", + "...", + ) + .endpoint("https://minio.local:9000") + .force_path_style(true), +) +.with_remote_git( + RemoteGitBackendConfig::new("https://gitserver.internal", "users/u1/sessions/s1") + .bearer_token(""), +)?; + +let session = agent.session( + "s3://workspace/users/u1/sessions/s1", + Some(SessionOptions::new().with_workspace_backend(ws)), +)?; +# Ok(()) +# } +``` + +The remote backend implements `WorkspaceGit` and `WorkspaceGitStashProvider`. +Worktrees are deliberately not supported — they are a local-filesystem +concept; use separate sessions with separate `repo_id`s when you need +isolation. HTTP 409 / 422 responses from the gitserver surface as a +typed `RemoteGitConflict` (downcastable via `anyhow::Error::downcast_ref`) +so callers can react to recoverable failures (e.g. +`WORKING_TREE_DIRTY` → stash and retry). + +Each call enforces a client-side `request_timeout` (default 30 s), +caps `log` `max_count` (default 200), and trims oversized `diff` +responses (default 1 MiB) — the same defensive style used on S3 reads. +Every call emits a `tracing::debug!` event with fields `op`, `repo_id`, +`status`, `bytes`, `outcome`, `duration_ms`, so the same subscriber +that meters S3 cost can meter gitserver cost. + +mTLS is supported by passing both `client_cert_pem` and `client_key_pem` +on the config. Files are read at construction and handed to +`reqwest::Identity::from_pem`; the key must be in PKCS#8 PEM format for +the `rustls-tls` backend. Setting only one of the pair fails at +construction with a clear error. + +#### Typed Tool Errors (v3.0+) + +Tool failures that the workspace layer can classify (concurrent +modification, missing path, remote-git conflict codes, ...) survive +end-to-end as a structured `ToolErrorKind` discriminator with a +`type` field, so SDK callers branch on the kind instead of +regex-matching the human-readable message. + +```rust +// Rust core +use a3s_code_core::{ToolErrorKind, WorkspaceError}; + +match services.write_for_edit(&path, &content, version.as_deref()).await { + Ok(_) => {} + Err(WorkspaceError::VersionConflict(c)) => retry(c.path, c.expected), + Err(other) => return Err(other.into()), +} +``` + +The corresponding pattern when calling `session.tool(...)` from a +direct tool execution: + +```ts +// Node +const result = await session.tool('edit', args); +if (result.errorKindJson) { + const kind = JSON.parse(result.errorKindJson); + switch (kind.type) { + case 'version_conflict': + await retry(kind.path, kind.expected); + break; + case 'not_found': + await createFile(kind.path); + break; + default: + console.error(result.output); + } +} +``` + +```python +# Python +result = session.tool("edit", args) +if kind := result.error_kind: + match kind["type"]: + case "version_conflict": + retry(kind["path"], kind["expected"]) + case "not_found": + create_file(kind["path"]) + case _: + log.error(result.output) +``` + +The same `error_kind_json` field appears on streaming `tool_end` +events (`AgentEvent.errorKindJson` / `event.error_kind`). Variants +shipping in v3.0: `version_conflict`, `remote_git_conflict`, +`not_found`, `invalid_argument`, `unsupported`, `timeout`. The enum +is `#[non_exhaustive]` — future minor releases can add variants +without a major bump. ### 4. Programmatic Tool Calling diff --git a/core/Cargo.toml b/core/Cargo.toml index 34f1ca9..990d803 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "a3s-code-core" -version = "2.6.0" +version = "3.0.0" edition = "2021" authors = ["A3S Lab Team"] license = "MIT" @@ -141,3 +141,9 @@ s3 = [ [dev-dependencies] # AHP for integration tests a3s-ahp = { path = "../../ahp" } +# HTTP mocking for the RemoteGitBackend (and any future HTTP-backed workspace +# provider). Production code does not depend on wiremock. +wiremock = "0.6" +# Self-signed cert generation for mTLS happy-path tests. Production code does +# not depend on rcgen. +rcgen = "0.13" diff --git a/core/src/agent.rs b/core/src/agent.rs index 91e3ceb..c4b7cad 100644 --- a/core/src/agent.rs +++ b/core/src/agent.rs @@ -265,6 +265,12 @@ pub enum AgentEvent { exit_code: i32, #[serde(skip_serializing_if = "Option::is_none")] metadata: Option, + /// Structured discriminant set by tools that mapped their failure + /// into a typed [`ToolErrorKind`](crate::tools::ToolErrorKind) + /// (e.g. `edit` / `patch` on a `WorkspaceError::VersionConflict`). + /// `None` on success or untyped failure. + #[serde(skip_serializing_if = "Option::is_none")] + error_kind: Option, }, /// Intermediate tool output (streaming delta) diff --git a/core/src/agent/extra_agent_tests.rs b/core/src/agent/extra_agent_tests.rs index fd7e10b..6a01dd3 100644 --- a/core/src/agent/extra_agent_tests.rs +++ b/core/src/agent/extra_agent_tests.rs @@ -115,6 +115,7 @@ fn test_agent_event_serialize_tool_end() { output: "hello".to_string(), exit_code: 0, metadata: None, + error_kind: None, }; let json = serde_json::to_string(&event).unwrap(); assert!(json.contains("tool_end")); @@ -130,6 +131,7 @@ fn test_agent_event_tool_end_has_metadata_field() { metadata: Some( serde_json::json!({ "before": "old", "after": "new", "file_path": "f.txt" }), ), + error_kind: None, }; let json = serde_json::to_string(&event).unwrap(); assert!(json.contains("\"before\"")); diff --git a/core/src/agent/parallel_tool_runtime.rs b/core/src/agent/parallel_tool_runtime.rs index 2907679..adca37f 100644 --- a/core/src/agent/parallel_tool_runtime.rs +++ b/core/src/agent/parallel_tool_runtime.rs @@ -56,6 +56,7 @@ impl AgentLoop { output: output.clone(), exit_code: normalized.exit_code, metadata: normalized.metadata.clone(), + error_kind: normalized.error_kind.clone(), }) .await .ok(); diff --git a/core/src/agent/tool_completion_runtime.rs b/core/src/agent/tool_completion_runtime.rs index 83576c3..43921ed 100644 --- a/core/src/agent/tool_completion_runtime.rs +++ b/core/src/agent/tool_completion_runtime.rs @@ -66,6 +66,7 @@ impl AgentLoop { output: output.clone(), exit_code: normalized.exit_code, metadata: normalized.metadata.clone(), + error_kind: normalized.error_kind.clone(), }) .await .ok(); diff --git a/core/src/agent/tool_execution_runtime.rs b/core/src/agent/tool_execution_runtime.rs index 7b37624..636c80e 100644 --- a/core/src/agent/tool_execution_runtime.rs +++ b/core/src/agent/tool_execution_runtime.rs @@ -62,6 +62,7 @@ impl AgentLoop { output: normalized.output.clone(), exit_code: normalized.exit_code, metadata: normalized.metadata.clone(), + error_kind: normalized.error_kind.clone(), }) .await .ok(); @@ -146,6 +147,7 @@ impl AgentLoop { exit_code, metadata: None, images: Vec::new(), + error_kind: None, }); } Ok(Err(e)) => { diff --git a/core/src/agent/tool_guard_runtime.rs b/core/src/agent/tool_guard_runtime.rs index ab5f7b2..f4981a8 100644 --- a/core/src/agent/tool_guard_runtime.rs +++ b/core/src/agent/tool_guard_runtime.rs @@ -57,6 +57,7 @@ impl AgentLoop { output: parse_outcome.output.clone(), exit_code: 1, metadata: None, + error_kind: None, }) .await .ok(); diff --git a/core/src/agent/tool_result_runtime.rs b/core/src/agent/tool_result_runtime.rs index d5f83e3..2181c9c 100644 --- a/core/src/agent/tool_result_runtime.rs +++ b/core/src/agent/tool_result_runtime.rs @@ -1,7 +1,7 @@ use super::execution_state::ExecutionLoopState; use super::AgentLoop; use crate::llm::{Attachment, Message}; -use crate::tools::ToolResult; +use crate::tools::{ToolErrorKind, ToolResult}; use crate::verification::VerificationReport; use serde_json::Value; @@ -11,6 +11,7 @@ pub(super) struct NormalizedToolResult { pub(super) is_error: bool, pub(super) metadata: Option, pub(super) images: Vec, + pub(super) error_kind: Option, } impl NormalizedToolResult { @@ -22,6 +23,7 @@ impl NormalizedToolResult { is_error: result.exit_code != 0, metadata: result.metadata, images: result.images, + error_kind: result.error_kind, }, Err(error) => Self::tool_error(error.to_string()), } @@ -34,6 +36,7 @@ impl NormalizedToolResult { is_error: true, metadata: None, images: Vec::new(), + error_kind: None, } } @@ -50,6 +53,7 @@ impl NormalizedToolResult { is_error: true, metadata: None, images: Vec::new(), + error_kind: None, } } } diff --git a/core/src/agent_api.rs b/core/src/agent_api.rs index 76b5ba9..f73973e 100644 --- a/core/src/agent_api.rs +++ b/core/src/agent_api.rs @@ -104,6 +104,12 @@ pub struct ToolCallResult { pub output: String, pub exit_code: i32, pub metadata: Option, + /// Structured discriminant for tool failures. `None` when the tool + /// either succeeded or failed without a typed reason (the message in + /// `output` is then the only diagnostic). Populated for known + /// kinds such as `VersionConflict` so SDK callers can branch on the + /// `type` field instead of regex-matching `output`. + pub error_kind: Option, } // ============================================================================ diff --git a/core/src/agent_api/direct_tools.rs b/core/src/agent_api/direct_tools.rs index 8c17315..dc7883f 100644 --- a/core/src/agent_api/direct_tools.rs +++ b/core/src/agent_api/direct_tools.rs @@ -107,6 +107,7 @@ impl DirectToolRuntime { output: result.output, exit_code: result.exit_code, metadata: result.metadata, + error_kind: result.error_kind, }) } } diff --git a/core/src/agent_api/runtime_events.rs b/core/src/agent_api/runtime_events.rs index 06d9902..518ae56 100644 --- a/core/src/agent_api/runtime_events.rs +++ b/core/src/agent_api/runtime_events.rs @@ -223,6 +223,7 @@ mod tests { output: "ok".to_string(), exit_code: 0, metadata: None, + error_kind: None, }) .await; assert!(active_tools.read().await.is_empty()); diff --git a/core/src/agent_api/tests.rs b/core/src/agent_api/tests.rs index da1d86c..aeb0782 100644 --- a/core/src/agent_api/tests.rs +++ b/core/src/agent_api/tests.rs @@ -73,20 +73,25 @@ impl TestWorkspaceFs { #[async_trait::async_trait] impl crate::workspace::WorkspaceFileSystem for TestWorkspaceFs { - async fn read_text(&self, path: &crate::workspace::WorkspacePath) -> anyhow::Result { + async fn read_text( + &self, + path: &crate::workspace::WorkspacePath, + ) -> crate::workspace::WorkspaceResult { self.files .read() .unwrap() .get(path.as_str()) .cloned() - .ok_or_else(|| anyhow::anyhow!("missing test workspace file: {}", path.as_str())) + .ok_or_else(|| crate::workspace::WorkspaceError::NotFound { + path: path.as_str().to_string(), + }) } async fn write_text( &self, path: &crate::workspace::WorkspacePath, content: &str, - ) -> anyhow::Result { + ) -> crate::workspace::WorkspaceResult { self.insert(path.as_str(), content); Ok(crate::workspace::WorkspaceWriteOutcome { bytes: content.len(), @@ -97,7 +102,7 @@ impl crate::workspace::WorkspaceFileSystem for TestWorkspaceFs { async fn list_dir( &self, path: &crate::workspace::WorkspacePath, - ) -> anyhow::Result> { + ) -> crate::workspace::WorkspaceResult> { let prefix = if path.is_root() { String::new() } else { diff --git a/core/src/lib.rs b/core/src/lib.rs index 676b9a4..43a482f 100644 --- a/core/src/lib.rs +++ b/core/src/lib.rs @@ -134,18 +134,20 @@ pub use subagent::{ AgentDefinition, AgentRegistry, CattleAgentKind, CattleAgentSpec, ConfirmationInheritance, WorkerAgentKind, WorkerAgentSpec, }; +pub use tools::ToolErrorKind; pub use workspace::{ - CommandOutput, CommandOutputObserver, CommandRequest, LocalWorkspaceBackend, - VirtualPathResolver, WorkspaceCapabilities, WorkspaceCommandRunner, WorkspaceDirEntry, - WorkspaceFileSystem, WorkspaceFileType, WorkspaceGit, WorkspaceGitBranch, + CommandOutput, CommandOutputObserver, CommandRequest, LocalWorkspaceBackend, RemoteGitBackend, + RemoteGitBackendConfig, RemoteGitConflict, VirtualPathResolver, WorkspaceCapabilities, + WorkspaceCommandRunner, WorkspaceDirEntry, WorkspaceError, WorkspaceFileSystem, + WorkspaceFileSystemExt, WorkspaceFileType, WorkspaceGit, WorkspaceGitBranch, WorkspaceGitCheckoutOutput, WorkspaceGitCheckoutRequest, WorkspaceGitCommit, WorkspaceGitCreateBranchRequest, WorkspaceGitCreateWorktreeRequest, WorkspaceGitDiffRequest, WorkspaceGitRemote, WorkspaceGitRemoveWorktreeRequest, WorkspaceGitStash, WorkspaceGitStashProvider, WorkspaceGitStashRequest, WorkspaceGitStatus, WorkspaceGitWorktree, WorkspaceGitWorktreeMutation, WorkspaceGitWorktreeProvider, WorkspaceGlobRequest, WorkspaceGlobResult, WorkspaceGrepRequest, WorkspaceGrepResult, WorkspacePath, - WorkspacePathResolver, WorkspaceRef, WorkspaceSearch, WorkspaceServices, - WorkspaceServicesBuilder, WorkspaceWriteOutcome, + WorkspacePathResolver, WorkspaceRef, WorkspaceResult, WorkspaceSearch, WorkspaceServices, + WorkspaceServicesBuilder, WorkspaceVersionConflict, WorkspaceWriteOutcome, }; #[cfg(feature = "s3")] pub use workspace::{S3BackendConfig, S3WorkspaceBackend}; diff --git a/core/src/tools/builtin/bash.rs b/core/src/tools/builtin/bash.rs index f6eec6d..ee4e3db 100644 --- a/core/src/tools/builtin/bash.rs +++ b/core/src/tools/builtin/bash.rs @@ -1112,6 +1112,7 @@ impl Tool for BashTool { success: result.exit_code == 0, metadata: Some(serde_json::json!({ "exit_code": result.exit_code })), images: vec![], + error_kind: None, }); } @@ -1151,6 +1152,7 @@ impl Tool for BashTool { success: result.exit_code == 0, metadata: Some(serde_json::json!({ "exit_code": result.exit_code })), images: vec![], + error_kind: None, }) } } diff --git a/core/src/tools/builtin/edit.rs b/core/src/tools/builtin/edit.rs index 7b2f1a3..53b3e24 100644 --- a/core/src/tools/builtin/edit.rs +++ b/core/src/tools/builtin/edit.rs @@ -1,6 +1,7 @@ //! Edit tool - Edit files by string replacement use crate::tools::types::{Tool, ToolContext, ToolOutput}; +use crate::workspace::WorkspaceError; use anyhow::Result; use async_trait::async_trait; @@ -82,17 +83,8 @@ impl Tool for EditTool { }; let display_path = ctx.workspace_services.display_path(&workspace_path); - let fs = ctx.workspace_services.fs(); - let path_for_read = workspace_path.clone(); - let fs_for_read = fs.clone(); - let content = match ctx - .workspace_services - .run_with_timeout("read_text", async move { - fs_for_read.read_text(&path_for_read).await - }) - .await - { - Ok(c) => c, + let (content, version) = match ctx.workspace_services.read_for_edit(&workspace_path).await { + Ok(pair) => pair, Err(e) => { return Ok(ToolOutput::error(format!( "Failed to read file {}: {}", @@ -124,13 +116,9 @@ impl Tool for EditTool { content.replacen(old_string, new_string, 1) }; - let path_for_write = workspace_path.clone(); - let content_for_write = new_content.clone(); match ctx .workspace_services - .run_with_timeout("write_text", async move { - fs.write_text(&path_for_write, &content_for_write).await - }) + .write_for_edit(&workspace_path, &new_content, version.as_deref()) .await { Ok(_) => { @@ -147,10 +135,25 @@ impl Tool for EditTool { )) .with_metadata(serde_json::Value::Object(metadata))) } - Err(e) => Ok(ToolOutput::error(format!( - "Failed to write file {}: {}", - display_path, e - ))), + Err(e) => { + // Surface the typed kind via ToolOutput.error_kind so SDK + // callers can react programmatically; the human-readable + // `content` message stays the same so the model sees the + // retry hint. + let typed = crate::tools::ToolErrorKind::from_workspace_error(&e); + let out = if matches!(e, WorkspaceError::VersionConflict(_)) { + ToolOutput::error(format!( + "Concurrent modification detected on {}: the file changed between read and write. Re-read the file and retry the edit.", + display_path + )) + } else { + ToolOutput::error(format!("Failed to write file {}: {}", display_path, e)) + }; + Ok(match typed { + Some(kind) => out.with_error_kind(kind), + None => out, + }) + } } } } @@ -271,4 +274,127 @@ mod tests { assert_eq!(examples[0]["file_path"], "src/lib.rs"); assert!(examples[0].get("path").is_none()); } + + #[tokio::test] + async fn test_edit_surfaces_concurrent_modification_as_typed_error() { + // Mock backend whose write step always reports a version conflict — + // simulating an S3 If-Match 412 between the read and the write. + // Verifies that: + // (1) edit matches on WorkspaceError::VersionConflict directly, + // (2) the user-facing message includes "Concurrent modification" + // (so the model can retry) rather than the generic write error. + use crate::workspace::{ + WorkspaceDirEntry, WorkspaceFileSystem, WorkspaceFileSystemExt, WorkspacePath, + WorkspaceRef, WorkspaceResult, WorkspaceServices, WorkspaceVersionConflict, + WorkspaceWriteOutcome, + }; + use async_trait::async_trait; + use std::sync::Arc; + + struct AlwaysConflictFs; + + #[async_trait] + impl WorkspaceFileSystem for AlwaysConflictFs { + async fn read_text(&self, _path: &WorkspacePath) -> WorkspaceResult { + Ok("hello world".to_string()) + } + async fn write_text( + &self, + _path: &WorkspacePath, + content: &str, + ) -> WorkspaceResult { + Ok(WorkspaceWriteOutcome { + bytes: content.len(), + lines: content.lines().count(), + }) + } + async fn list_dir( + &self, + _path: &WorkspacePath, + ) -> WorkspaceResult> { + Ok(Vec::new()) + } + } + + #[async_trait] + impl WorkspaceFileSystemExt for AlwaysConflictFs { + async fn read_text_with_version( + &self, + _path: &WorkspacePath, + ) -> WorkspaceResult<(String, String)> { + Ok(("hello world".to_string(), "v0".to_string())) + } + async fn write_text_if_version( + &self, + path: &WorkspacePath, + _content: &str, + _expected_version: &str, + ) -> WorkspaceResult { + Err(WorkspaceError::VersionConflict(WorkspaceVersionConflict { + path: path.as_str().to_string(), + expected: "v0".to_string(), + actual: Some("v-other".to_string()), + })) + } + } + + let backend = Arc::new(AlwaysConflictFs); + let fs: Arc = backend.clone(); + let fs_ext: Arc = backend; + let services = WorkspaceServices::builder(WorkspaceRef::new("mem", "mem://ws"), fs) + .file_system_ext(fs_ext) + .build(); + + let tool = EditTool; + let ctx = ToolContext::new(std::env::temp_dir()).with_workspace_services(services); + + let result = tool + .execute( + &serde_json::json!({ + "file_path": "anything.txt", + "old_string": "hello", + "new_string": "goodbye", + }), + &ctx, + ) + .await + .unwrap(); + + assert!( + !result.success, + "edit on conflicting backend must report failure" + ); + assert!( + result.content.contains("Concurrent modification"), + "expected retry-friendly conflict message, got: {}", + result.content + ); + + // Phase 8: the typed error_kind must also survive end-to-end so SDK + // callers can branch on it without parsing the string. + let kind = result + .error_kind + .as_ref() + .expect("edit must surface a typed error_kind for VersionConflict"); + match kind { + crate::tools::ToolErrorKind::VersionConflict { + path, + expected, + actual, + } => { + assert_eq!(path, "anything.txt"); + assert_eq!(expected, "v0"); + assert_eq!(actual.as_deref(), Some("v-other")); + } + other => panic!("expected VersionConflict kind, got {other:?}"), + } + + // The serialised wire shape is the contract SDKs will see. Pin it + // so any accidental rename / restructure breaks the build. + let json = serde_json::to_value(kind).unwrap(); + assert_eq!(json["type"], "version_conflict"); + assert_eq!(json["path"], "anything.txt"); + assert_eq!(json["expected"], "v0"); + assert_eq!(json["actual"], "v-other"); + } } diff --git a/core/src/tools/builtin/mod.rs b/core/src/tools/builtin/mod.rs index 106b4ad..1bde654 100644 --- a/core/src/tools/builtin/mod.rs +++ b/core/src/tools/builtin/mod.rs @@ -7,7 +7,7 @@ pub(crate) mod bash; pub mod batch; mod edit; mod generate_object; -mod git; +pub(crate) mod git; mod glob_tool; mod grep; mod ls; diff --git a/core/src/tools/builtin/patch.rs b/core/src/tools/builtin/patch.rs index 843cc64..e393efd 100644 --- a/core/src/tools/builtin/patch.rs +++ b/core/src/tools/builtin/patch.rs @@ -1,6 +1,7 @@ //! Patch tool - Apply unified diff patches to files use crate::tools::types::{Tool, ToolContext, ToolOutput}; +use crate::workspace::WorkspaceError; use anyhow::Result; use async_trait::async_trait; @@ -241,17 +242,8 @@ impl Tool for PatchTool { }; let display_path = ctx.workspace_services.display_path(&workspace_path); - let fs = ctx.workspace_services.fs(); - let path_for_read = workspace_path.clone(); - let fs_for_read = fs.clone(); - let content = match ctx - .workspace_services - .run_with_timeout("read_text", async move { - fs_for_read.read_text(&path_for_read).await - }) - .await - { - Ok(c) => c, + let (content, version) = match ctx.workspace_services.read_for_edit(&workspace_path).await { + Ok(pair) => pair, Err(e) => { return Ok(ToolOutput::error(format!( "Failed to read file {}: {}", @@ -277,13 +269,9 @@ impl Tool for PatchTool { new_content }; - let path_for_write = workspace_path.clone(); - let content_for_write = final_content.clone(); match ctx .workspace_services - .run_with_timeout("write_text", async move { - fs.write_text(&path_for_write, &content_for_write).await - }) + .write_for_edit(&workspace_path, &final_content, version.as_deref()) .await { Ok(_) => Ok(ToolOutput::success(format!( @@ -291,10 +279,24 @@ impl Tool for PatchTool { hunks.len(), display_path ))), - Err(e) => Ok(ToolOutput::error(format!( - "Failed to write patched file {}: {}", - display_path, e - ))), + Err(e) => { + let typed = crate::tools::ToolErrorKind::from_workspace_error(&e); + let out = if matches!(e, WorkspaceError::VersionConflict(_)) { + ToolOutput::error(format!( + "Concurrent modification detected on {}: the file changed between read and write. Re-read the file and retry the patch.", + display_path + )) + } else { + ToolOutput::error(format!( + "Failed to write patched file {}: {}", + display_path, e + )) + }; + Ok(match typed { + Some(kind) => out.with_error_kind(kind), + None => out, + }) + } } } } diff --git a/core/src/tools/mod.rs b/core/src/tools/mod.rs index 55c73e9..a1df044 100644 --- a/core/src/tools/mod.rs +++ b/core/src/tools/mod.rs @@ -32,7 +32,7 @@ pub use task::{ parallel_task_params_schema, task_params_schema, ParallelTaskParams, ParallelTaskTool, TaskExecutor, TaskParams, TaskResult, TaskTool, }; -pub use types::{Tool, ToolContext, ToolEventSender, ToolOutput, ToolStreamEvent}; +pub use types::{Tool, ToolContext, ToolErrorKind, ToolEventSender, ToolOutput, ToolStreamEvent}; use crate::file_history::{self, FileHistory}; use crate::llm::ToolDefinition; @@ -164,6 +164,13 @@ pub struct ToolResult { /// Image attachments from tool execution (multi-modal output). #[serde(skip)] pub images: Vec, + /// Structured discriminant for tool failures. Populated by built-in + /// tools that can map their failure into a typed [`ToolErrorKind`] + /// (e.g. `edit`/`patch` setting `VersionConflict` on a CAS rejection + /// from `WorkspaceError`). Forwarded to the SDK so callers can react + /// programmatically without parsing `output`. + #[serde(skip_serializing_if = "Option::is_none")] + pub error_kind: Option, } impl ToolResult { @@ -174,6 +181,7 @@ impl ToolResult { exit_code: 0, metadata: None, images: Vec::new(), + error_kind: None, } } @@ -184,6 +192,7 @@ impl ToolResult { exit_code: 1, metadata: None, images: Vec::new(), + error_kind: None, } } } @@ -196,6 +205,7 @@ impl From for ToolResult { exit_code: if output.success { 0 } else { 1 }, metadata: output.metadata, images: output.images, + error_kind: output.error_kind, } } } @@ -470,9 +480,9 @@ impl ToolExecutor { mod tests { use super::*; use crate::workspace::{ - CommandOutput, CommandRequest, WorkspaceCommandRunner, WorkspaceDirEntry, - WorkspaceFileSystem, WorkspaceFileType, WorkspacePath, WorkspaceRef, WorkspaceServices, - WorkspaceWriteOutcome, + CommandOutput, CommandRequest, WorkspaceCommandRunner, WorkspaceDirEntry, WorkspaceError, + WorkspaceFileSystem, WorkspaceFileType, WorkspacePath, WorkspaceRef, WorkspaceResult, + WorkspaceServices, WorkspaceWriteOutcome, }; use async_trait::async_trait; use std::sync::RwLock; @@ -569,20 +579,22 @@ mod tests { #[async_trait] impl WorkspaceFileSystem for MemoryWorkspaceFs { - async fn read_text(&self, path: &WorkspacePath) -> Result { + async fn read_text(&self, path: &WorkspacePath) -> WorkspaceResult { self.files .read() .unwrap() .get(path.as_str()) .cloned() - .ok_or_else(|| anyhow::anyhow!("missing file: {}", path.as_str())) + .ok_or_else(|| WorkspaceError::NotFound { + path: path.as_str().to_string(), + }) } async fn write_text( &self, path: &WorkspacePath, content: &str, - ) -> Result { + ) -> WorkspaceResult { self.insert(path.as_str(), content); Ok(WorkspaceWriteOutcome { bytes: content.len(), @@ -590,7 +602,7 @@ mod tests { }) } - async fn list_dir(&self, path: &WorkspacePath) -> Result> { + async fn list_dir(&self, path: &WorkspacePath) -> WorkspaceResult> { let prefix = if path.is_root() { String::new() } else { @@ -829,6 +841,7 @@ mod tests { success: true, metadata: None, images: Vec::new(), + error_kind: None, }; let result: ToolResult = output.into(); assert_eq!(result.output, "success content"); @@ -843,6 +856,7 @@ mod tests { success: false, metadata: Some(serde_json::json!({"error": "test"})), images: Vec::new(), + error_kind: None, }; let result: ToolResult = output.into(); assert_eq!(result.output, "failure content"); diff --git a/core/src/tools/registry.rs b/core/src/tools/registry.rs index b865f08..a4900d1 100644 --- a/core/src/tools/registry.rs +++ b/core/src/tools/registry.rs @@ -226,6 +226,7 @@ impl ToolRegistry { exit_code: if output.success { 0 } else { 1 }, metadata: output.metadata, images: output.images, + error_kind: output.error_kind, }) } None => Ok(ToolResult::error(name, format!("Unknown tool: {}", name))), diff --git a/core/src/tools/skill.rs b/core/src/tools/skill.rs index 93907a7..af2507f 100644 --- a/core/src/tools/skill.rs +++ b/core/src/tools/skill.rs @@ -194,6 +194,7 @@ Use this before invoking Skill when specialized instructions may help." success: true, metadata: Some(serde_json::json!({ "skills": metadata })), images: Vec::new(), + error_kind: None, }) } } @@ -342,6 +343,7 @@ The skill's allowed-tools are granted during execution and revoked after complet "usage": result.usage, })), images: Vec::new(), + error_kind: None, }) } } diff --git a/core/src/tools/types.rs b/core/src/tools/types.rs index 7185783..cdad650 100644 --- a/core/src/tools/types.rs +++ b/core/src/tools/types.rs @@ -157,6 +157,82 @@ impl ToolContext { } } +/// Structured discriminant for tool failures. +/// +/// This is the SDK-facing counterpart of [`WorkspaceError`](crate::workspace::WorkspaceError) +/// (and any future typed error sources). The Rust trait surface returns +/// typed enums; this struct is what survives the trip through +/// `ToolOutput` → `ToolResult` → `ToolCallResult` → SDK boundary so JS / +/// Python callers can do `match` on the kind instead of regex-matching +/// the human-readable `output` string. +/// +/// Serializes to JSON with a `type` discriminator, e.g.: +/// ```json +/// { "type": "version_conflict", "path": "doc.md", "expected": "etag-1", "actual": "etag-2" } +/// ``` +/// +/// `#[non_exhaustive]` so adding a new kind is a minor-version change. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(tag = "type", rename_all = "snake_case")] +#[non_exhaustive] +pub enum ToolErrorKind { + /// Compare-and-swap write rejected by the backend because the file + /// changed since the caller read it. Originates from + /// `WorkspaceError::VersionConflict` on the S3 / future versioning + /// backends. + VersionConflict { + path: String, + expected: String, + actual: Option, + }, + /// Remote git server returned a typed 409 / 422 conflict code such + /// as `BRANCH_EXISTS` or `WORKING_TREE_DIRTY`. + RemoteGitConflict { code: String, message: String }, + /// Operation referenced a path that does not exist. + NotFound { path: String }, + /// Caller passed an argument the tool / backend cannot honour + /// (malformed pattern, parent-traversal path, ...). + InvalidArgument { message: String }, + /// The backend explicitly does not support this operation + /// (e.g. worktree on a remote-git workspace). + Unsupported { message: String }, + /// The operation's outer timeout fired before the backend responded. + Timeout { op: String, duration_ms: u64 }, +} + +impl ToolErrorKind { + /// Map a [`WorkspaceError`](crate::workspace::WorkspaceError) into the + /// corresponding SDK-visible kind. Backend variants that don't fit a + /// dedicated [`ToolErrorKind`] (currently `Backend(_)`) return `None`; + /// the caller then surfaces only the human-readable message. + pub fn from_workspace_error(err: &crate::workspace::WorkspaceError) -> Option { + use crate::workspace::WorkspaceError as WE; + match err { + WE::NotFound { path } => Some(Self::NotFound { path: path.clone() }), + WE::VersionConflict(c) => Some(Self::VersionConflict { + path: c.path.clone(), + expected: c.expected.clone(), + actual: c.actual.clone(), + }), + WE::RemoteGitConflict(c) => Some(Self::RemoteGitConflict { + code: c.code.clone(), + message: c.message.clone(), + }), + WE::InvalidArgument { message } => Some(Self::InvalidArgument { + message: message.clone(), + }), + WE::Unsupported(message) => Some(Self::Unsupported { + message: message.clone(), + }), + WE::Timeout { op, duration } => Some(Self::Timeout { + op: op.clone(), + duration_ms: duration.as_millis() as u64, + }), + WE::Backend(_) => None, + } + } +} + /// Tool execution output #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ToolOutput { @@ -173,6 +249,13 @@ pub struct ToolOutput { /// the LLM as multi-modal content blocks alongside the text content. #[serde(skip)] pub images: Vec, + /// Optional structured discriminant for tool failures. Populated by + /// tools that can map their failure into a typed [`ToolErrorKind`] + /// (e.g. `edit` / `patch` on a `WorkspaceError::VersionConflict`). + /// Surfaced through `ToolResult` and `ToolCallResult` so SDK callers + /// can react programmatically without parsing the `content` string. + #[serde(skip_serializing_if = "Option::is_none")] + pub error_kind: Option, } impl ToolOutput { @@ -182,6 +265,7 @@ impl ToolOutput { success: true, metadata: None, images: Vec::new(), + error_kind: None, } } @@ -191,6 +275,7 @@ impl ToolOutput { success: false, metadata: None, images: Vec::new(), + error_kind: None, } } @@ -207,6 +292,14 @@ impl ToolOutput { self.images = images; self } + + /// Attach a typed error kind. Used by built-in tools when they can + /// map a backend failure (e.g. `WorkspaceError::VersionConflict`) + /// into a programmatically actionable [`ToolErrorKind`]. + pub fn with_error_kind(mut self, kind: ToolErrorKind) -> Self { + self.error_kind = Some(kind); + self + } } /// Tool trait - the core abstraction for all tools diff --git a/core/src/workspace/conformance.rs b/core/src/workspace/conformance.rs new file mode 100644 index 0000000..cd12c8c --- /dev/null +++ b/core/src/workspace/conformance.rs @@ -0,0 +1,470 @@ +//! Behavioural conformance suite for workspace backend implementations. +//! +//! This module captures the **implicit invariants** that every +//! [`WorkspaceFileSystem`] and [`WorkspaceFileSystemExt`] implementation must +//! satisfy. Until this suite landed those invariants only lived in commits, +//! comments, and the maintainer's head — new backends would have to +//! reverse-engineer them from existing implementations. +//! +//! # How to use +//! +//! When you add a new backend (S3, GCS, container, browser, ...): +//! +//! ```ignore +//! #[tokio::test] +//! async fn my_backend_conforms_to_filesystem() { +//! let fs: Arc = Arc::new(MyBackend::new(...)); +//! conformance::assert_filesystem_conformance(fs, "MyBackend").await; +//! } +//! ``` +//! +//! When the suite grows (new invariant discovered, e.g. an S3 production +//! incident), every backend running it gets the regression test for free — +//! that is the point. +//! +//! # What is covered +//! +//! * [`assert_filesystem_conformance`] — the required +//! [`WorkspaceFileSystem`] surface: read-after-write round-trips, errors +//! on nonexistent reads / list_dirs, listing returns directory entries, +//! write overwrites existing content, write creates parent path +//! components. +//! * [`assert_filesystem_ext_conformance`] — the optional +//! [`WorkspaceFileSystemExt`] CAS surface: version round-trip, +//! compare-and-swap success on matching version, downcastable +//! [`WorkspaceVersionConflict`] on mismatch, empty-version rejection. +//! +//! Out of scope (intentionally) for the first cut: +//! +//! * `WorkspaceCommandRunner` — shell semantics differ enough across +//! sandboxes that a single contract is hard to write. +//! * `WorkspaceSearch` — glob and regex behaviour are inherently +//! implementation-defined; conformance tests would over-constrain. +//! * `WorkspaceGit*` — implementation behaviour is too coupled to the +//! underlying git engine to capture in a single trait contract. +//! +//! # Reference backend +//! +//! [`InMemoryFileSystem`] is a tiny `HashMap`-backed implementation that +//! exists for two purposes: (1) it runs the conformance suite against +//! itself in this module, proving the contract is internally consistent; +//! (2) it serves as the smallest possible reference for what a backend +//! that satisfies the suite has to look like. Real production backends +//! (`LocalWorkspaceBackend`, `S3WorkspaceBackend`) layer real I/O on top +//! of the same observable semantics. + +use super::{ + WorkspaceDirEntry, WorkspaceError, WorkspaceFileSystem, WorkspaceFileSystemExt, WorkspacePath, + WorkspaceResult, WorkspaceVersionConflict, WorkspaceWriteOutcome, +}; +use async_trait::async_trait; +use std::collections::HashMap; +use std::sync::{Arc, Mutex}; + +// ============================================================================ +// Conformance assertions for WorkspaceFileSystem +// ============================================================================ + +/// Run the full required-surface conformance suite against a backend. +/// +/// On failure, the panic message includes `ctx` so test output names the +/// backend under test even when several backends share the same harness. +pub(crate) async fn assert_filesystem_conformance(fs: Arc, ctx: &str) { + fs_read_after_write_roundtrip(&fs, ctx).await; + fs_read_nonexistent_errors(&fs, ctx).await; + fs_write_overwrites_existing(&fs, ctx).await; + fs_write_creates_parent_components(&fs, ctx).await; + fs_list_dir_root_succeeds(&fs, ctx).await; + fs_list_dir_after_write_sees_the_entry(&fs, ctx).await; + fs_list_dir_nonexistent_errors(&fs, ctx).await; +} + +/// Run the optional CAS-surface conformance suite against a backend that +/// implements both [`WorkspaceFileSystem`] and [`WorkspaceFileSystemExt`]. +/// +/// Backends that don't implement `Ext` just don't call this. +pub(crate) async fn assert_filesystem_ext_conformance( + fs: Arc, + ext: Arc, + ctx: &str, +) { + ext_version_token_is_non_empty(&fs, &ext, ctx).await; + ext_write_with_matching_version_succeeds(&fs, &ext, ctx).await; + ext_write_with_stale_version_yields_conflict(&fs, &ext, ctx).await; + ext_empty_expected_version_is_rejected(&ext, ctx).await; +} + +// ---- individual invariants ---- + +async fn fs_read_after_write_roundtrip(fs: &Arc, ctx: &str) { + let path = WorkspacePath::from_normalized("conformance/roundtrip.txt"); + fs.write_text(&path, "hello world") + .await + .unwrap_or_else(|e| panic!("[{ctx}] write_text failed: {e}")); + let content = fs + .read_text(&path) + .await + .unwrap_or_else(|e| panic!("[{ctx}] read_text after write failed: {e}")); + assert_eq!( + content, "hello world", + "[{ctx}] read_text must return exactly what write_text wrote" + ); +} + +async fn fs_read_nonexistent_errors(fs: &Arc, ctx: &str) { + let path = WorkspacePath::from_normalized("conformance/definitely-not-there.txt"); + let result = fs.read_text(&path).await; + assert!( + result.is_err(), + "[{ctx}] read_text on a path that was never written must error, got Ok({:?})", + result.ok() + ); +} + +async fn fs_write_overwrites_existing(fs: &Arc, ctx: &str) { + let path = WorkspacePath::from_normalized("conformance/overwrite.txt"); + fs.write_text(&path, "v1").await.unwrap(); + fs.write_text(&path, "v2").await.unwrap(); + let content = fs.read_text(&path).await.unwrap(); + assert_eq!( + content, "v2", + "[{ctx}] second write_text must overwrite first" + ); +} + +async fn fs_write_creates_parent_components(fs: &Arc, ctx: &str) { + let path = WorkspacePath::from_normalized("conformance/deep/nested/path/file.txt"); + fs.write_text(&path, "deep").await.unwrap_or_else(|e| { + panic!("[{ctx}] write_text must create missing parent components: {e}") + }); + let content = fs.read_text(&path).await.unwrap(); + assert_eq!(content, "deep", "[{ctx}] write_text round-trip at depth"); +} + +async fn fs_list_dir_root_succeeds(fs: &Arc, ctx: &str) { + let result = fs.list_dir(&WorkspacePath::root()).await; + assert!( + result.is_ok(), + "[{ctx}] list_dir on the workspace root must always succeed (may be empty), got {:?}", + result + ); +} + +async fn fs_list_dir_after_write_sees_the_entry(fs: &Arc, ctx: &str) { + // Use a fresh subdirectory so we know exactly what should appear. + let dir = WorkspacePath::from_normalized("conformance/listing-test"); + let file = WorkspacePath::from_normalized("conformance/listing-test/visible.txt"); + fs.write_text(&file, "hello").await.unwrap(); + + let entries = fs + .list_dir(&dir) + .await + .unwrap_or_else(|e| panic!("[{ctx}] list_dir after write failed: {e}")); + let names: Vec<&str> = entries.iter().map(|e| e.name.as_str()).collect(); + assert!( + names.contains(&"visible.txt"), + "[{ctx}] list_dir must include just-written entry; got {names:?}" + ); +} + +async fn fs_list_dir_nonexistent_errors(fs: &Arc, ctx: &str) { + let path = WorkspacePath::from_normalized("conformance/never-created"); + let result = fs.list_dir(&path).await; + assert!( + result.is_err(), + "[{ctx}] list_dir on a nonexistent path must error, got Ok({:?}); \ + a silent empty result masks user typos", + result.ok() + ); +} + +async fn ext_version_token_is_non_empty( + fs: &Arc, + ext: &Arc, + ctx: &str, +) { + let path = WorkspacePath::from_normalized("conformance/cas-version.txt"); + fs.write_text(&path, "seed").await.unwrap(); + let (content, version) = ext + .read_text_with_version(&path) + .await + .unwrap_or_else(|e| panic!("[{ctx}] read_text_with_version failed: {e}")); + assert_eq!(content, "seed"); + assert!( + !version.is_empty(), + "[{ctx}] read_text_with_version must return a non-empty opaque token" + ); +} + +async fn ext_write_with_matching_version_succeeds( + fs: &Arc, + ext: &Arc, + ctx: &str, +) { + let path = WorkspacePath::from_normalized("conformance/cas-match.txt"); + fs.write_text(&path, "v0").await.unwrap(); + let (_, version) = ext.read_text_with_version(&path).await.unwrap(); + ext.write_text_if_version(&path, "v1", &version) + .await + .unwrap_or_else(|e| { + panic!("[{ctx}] write_text_if_version with matching version must succeed: {e}") + }); + assert_eq!(fs.read_text(&path).await.unwrap(), "v1"); +} + +async fn ext_write_with_stale_version_yields_conflict( + fs: &Arc, + ext: &Arc, + ctx: &str, +) { + let path = WorkspacePath::from_normalized("conformance/cas-conflict.txt"); + fs.write_text(&path, "alpha").await.unwrap(); + let (_, stale_version) = ext.read_text_with_version(&path).await.unwrap(); + + // Simulate a concurrent writer that bumps the version. + fs.write_text(&path, "concurrent").await.unwrap(); + + let err = ext + .write_text_if_version(&path, "beta", &stale_version) + .await + .expect_err(&format!( + "[{ctx}] write_text_if_version with stale version must reject" + )); + assert!( + matches!(err, WorkspaceError::VersionConflict(_)), + "[{ctx}] CAS rejection must produce WorkspaceError::VersionConflict; got: {err:?}" + ); +} + +async fn ext_empty_expected_version_is_rejected(ext: &Arc, ctx: &str) { + let path = WorkspacePath::from_normalized("conformance/cas-empty-ver.txt"); + let err = ext + .write_text_if_version(&path, "anything", "") + .await + .expect_err(&format!( + "[{ctx}] write_text_if_version with empty expected version must reject" + )); + // Don't require a specific message — just that it's a clear failure. + let _ = err; +} + +// ============================================================================ +// Reference backend: InMemoryFileSystem +// ============================================================================ + +/// A `HashMap`-backed reference implementation. +/// +/// Purpose: +/// * **Self-test the conformance suite.** The suite must pass against an +/// ideally-behaving backend; if it fails here, the contract itself is +/// inconsistent. +/// * **Document the shape.** A few hundred lines of obvious code show new +/// backend authors what the trait surface "wants". +/// * **Mock for higher-level tests.** Helper modules can wire this into a +/// `WorkspaceServices` to exercise tool code paths without touching disk +/// or network. +/// +/// The file map and the version counter live behind a single mutex so the +/// CAS write is genuinely atomic — a separate counter mutex would allow +/// a concurrent writer to slip in between the version check and the +/// version bump, defeating the conformance test for stale-version +/// rejection. +struct InMemoryState { + files: HashMap, + counter: u64, +} + +pub(crate) struct InMemoryFileSystem { + state: Mutex, +} + +impl InMemoryFileSystem { + pub(crate) fn new() -> Self { + Self { + state: Mutex::new(InMemoryState { + files: HashMap::new(), + counter: 0, + }), + } + } + + fn bump_version(state: &mut InMemoryState) -> String { + state.counter += 1; + format!("v{}", state.counter) + } +} + +impl Default for InMemoryFileSystem { + fn default() -> Self { + Self::new() + } +} + +#[async_trait] +impl WorkspaceFileSystem for InMemoryFileSystem { + async fn read_text(&self, path: &WorkspacePath) -> WorkspaceResult { + self.state + .lock() + .unwrap() + .files + .get(path.as_str()) + .map(|(c, _)| c.clone()) + .ok_or_else(|| WorkspaceError::NotFound { + path: path.as_str().to_string(), + }) + } + + async fn write_text( + &self, + path: &WorkspacePath, + content: &str, + ) -> WorkspaceResult { + let mut state = self.state.lock().unwrap(); + let version = Self::bump_version(&mut state); + state + .files + .insert(path.as_str().to_string(), (content.to_string(), version)); + Ok(WorkspaceWriteOutcome { + bytes: content.len(), + lines: content.lines().count(), + }) + } + + async fn list_dir(&self, path: &WorkspacePath) -> WorkspaceResult> { + // Synthesize a directory view from the flat key space. For a + // requested directory at `path`, anything stored under + // `/` shows up. Mid-path components become Directory + // entries; direct children become File entries. + let prefix: String = if path.is_root() { + String::new() + } else { + format!("{}/", path.as_str()) + }; + let state = self.state.lock().unwrap(); + let mut seen_dirs: std::collections::HashSet = std::collections::HashSet::new(); + let mut entries: Vec = Vec::new(); + let mut any = false; + for (key, (content, _)) in state.files.iter() { + let Some(rest) = key.strip_prefix(&prefix) else { + continue; + }; + if rest.is_empty() { + continue; + } + any = true; + if let Some((dir_name, _)) = rest.split_once('/') { + if seen_dirs.insert(dir_name.to_string()) { + entries.push(WorkspaceDirEntry { + name: dir_name.to_string(), + kind: super::WorkspaceFileType::Directory, + size: 0, + }); + } + } else { + entries.push(WorkspaceDirEntry { + name: rest.to_string(), + kind: super::WorkspaceFileType::File, + size: content.len() as u64, + }); + } + } + if !path.is_root() && !any { + return Err(WorkspaceError::NotFound { + path: path.as_str().to_string(), + }); + } + entries.sort_by(|a, b| a.name.cmp(&b.name)); + Ok(entries) + } +} + +#[async_trait] +impl WorkspaceFileSystemExt for InMemoryFileSystem { + async fn read_text_with_version( + &self, + path: &WorkspacePath, + ) -> WorkspaceResult<(String, String)> { + self.state + .lock() + .unwrap() + .files + .get(path.as_str()) + .cloned() + .ok_or_else(|| WorkspaceError::NotFound { + path: path.as_str().to_string(), + }) + } + + async fn write_text_if_version( + &self, + path: &WorkspacePath, + content: &str, + expected_version: &str, + ) -> WorkspaceResult { + if expected_version.is_empty() { + return Err(WorkspaceError::InvalidArgument { + message: "expected_version must not be empty".to_string(), + }); + } + // Hold the single mutex across the entire compare-and-swap so a + // concurrent writer cannot slip between the version check and the + // version bump. + let mut state = self.state.lock().unwrap(); + let actual = state.files.get(path.as_str()).map(|(_, v)| v.clone()); + match actual { + Some(actual) if actual == expected_version => { + let new_version = Self::bump_version(&mut state); + state.files.insert( + path.as_str().to_string(), + (content.to_string(), new_version), + ); + Ok(WorkspaceWriteOutcome { + bytes: content.len(), + lines: content.lines().count(), + }) + } + Some(actual) => Err(WorkspaceError::VersionConflict(WorkspaceVersionConflict { + path: path.as_str().to_string(), + expected: expected_version.to_string(), + actual: Some(actual), + })), + None => Err(WorkspaceError::NotFound { + path: path.as_str().to_string(), + }), + } + } +} + +// ============================================================================ +// Self-test: the conformance suite must pass against the in-memory backend. +// And the local backend must too, proving the contract is implementable +// over real I/O. +// ============================================================================ + +#[cfg(test)] +mod self_tests { + use super::*; + use crate::workspace::LocalWorkspaceBackend; + + #[tokio::test] + async fn in_memory_backend_satisfies_filesystem_conformance() { + let fs: Arc = Arc::new(InMemoryFileSystem::new()); + assert_filesystem_conformance(fs, "InMemoryFileSystem").await; + } + + #[tokio::test] + async fn in_memory_backend_satisfies_filesystem_ext_conformance() { + let backend = Arc::new(InMemoryFileSystem::new()); + let fs: Arc = backend.clone(); + let ext: Arc = backend; + assert_filesystem_ext_conformance(fs, ext, "InMemoryFileSystem").await; + } + + #[tokio::test] + async fn local_backend_satisfies_filesystem_conformance() { + let temp = tempfile::tempdir().unwrap(); + let fs: Arc = + Arc::new(LocalWorkspaceBackend::new(temp.path().to_path_buf())); + assert_filesystem_conformance(fs, "LocalWorkspaceBackend").await; + } +} diff --git a/core/src/workspace/error.rs b/core/src/workspace/error.rs new file mode 100644 index 0000000..48d30d5 --- /dev/null +++ b/core/src/workspace/error.rs @@ -0,0 +1,296 @@ +//! Typed error surface for the workspace subsystem. +//! +//! Until this module landed, every backend method returned +//! `anyhow::Result` and callers that wanted to react to specific +//! failure kinds had to downcast — fragile, opaque to docs, and +//! non-exhaustive. [`WorkspaceError`] gives the trait surface a typed +//! enum with `#[non_exhaustive]` so callers can `match` known +//! variants while still leaving room for future ones without breaking +//! compatibility. +//! +//! # Migration shape +//! +//! The migration ships in two commits: +//! +//! 1. **7.3.a (this commit):** introduce [`WorkspaceError`] and +//! [`WorkspaceResult`] alongside the existing `anyhow::Result` +//! surface. Add `From` conversions in both directions. *No trait +//! signature changes* — purely additive infrastructure. Existing +//! callers and backends remain on `anyhow::Result`; the new types +//! are immediately usable but not yet required. +//! +//! 2. **7.3.b (next commit):** flip every trait method and helper to +//! return `WorkspaceResult`, update every backend implementation, +//! every tool, and the SDK transparent paths. That commit is the +//! breaking change that motivates the v3.0.0 version bump. +//! +//! Splitting it this way lets the type definitions land independently +//! (and be reviewed in isolation) without breaking any existing +//! callsite. +//! +//! # Bridge between `anyhow::Error` and `WorkspaceError` +//! +//! In addition to the auto-generated `From` impl +//! provided by `#[from]` on the `Backend` variant, this module supplies +//! [`WorkspaceError::from_anyhow`] which **preserves the typed variant** +//! when an `anyhow::Error` was originally constructed from a known +//! conflict struct (`WorkspaceVersionConflict`, `RemoteGitConflict`). +//! The plain `Into::into` path drops the type information into the +//! `Backend(_)` variant because `anyhow::Error` erases the source type +//! at the value level. + +use super::{RemoteGitConflict, WorkspaceVersionConflict}; +use std::time::Duration; + +/// Error type returned by every [`WorkspaceFileSystem`](super::WorkspaceFileSystem) +/// and friend trait method. +/// +/// `#[non_exhaustive]` so adding a new variant in a future release is a +/// minor change — existing `match` callers compile, they just hit the +/// catch-all arm for unknown variants. +/// +/// The variants intentionally split into three categories: +/// +/// * **Structured failures** the trait surface knows how to describe +/// (`NotFound`, `InvalidArgument`, `Timeout`, `Unsupported`). New +/// variants in this category should also be structured. +/// * **Typed conflicts** with their own payload structs that already +/// ship as part of the public API +/// (`VersionConflict(WorkspaceVersionConflict)`, +/// `RemoteGitConflict(RemoteGitConflict)`). +/// * **`Backend(anyhow::Error)`** — the escape hatch. Any failure not +/// covered above wraps an `anyhow::Error`. Backends should prefer the +/// typed variants where they apply; `Backend` is for genuinely +/// opaque or backend-specific failures. +#[derive(Debug, thiserror::Error)] +#[non_exhaustive] +pub enum WorkspaceError { + /// A read / list against a path that does not exist on the backend. + /// + /// Backends that distinguish "doesn't exist" from "exists but + /// access denied" should still emit this for the former; the + /// latter belongs in `Backend(_)` with the backend's native auth + /// error wrapped. + #[error("path not found: {path}")] + NotFound { + /// Path that triggered the failure, in workspace-relative form + /// where possible. May include backend-specific qualifiers + /// (`s3://bucket/key`) when that aids debugging. + path: String, + }, + + /// Compare-and-swap write rejected because the in-storage version + /// no longer matches what the caller observed at read time. Carries + /// the existing public [`WorkspaceVersionConflict`] struct verbatim. + #[error(transparent)] + VersionConflict(#[from] WorkspaceVersionConflict), + + /// A remote git server returned 409 / 422 with a typed conflict + /// code (e.g. `BRANCH_EXISTS`, `WORKING_TREE_DIRTY`, + /// `NOTHING_TO_STASH`). Carries the existing public + /// [`RemoteGitConflict`] struct verbatim. + #[error(transparent)] + RemoteGitConflict(#[from] RemoteGitConflict), + + /// Caller passed an argument the backend cannot honour + /// (empty version on a CAS write, malformed pattern on a search, + /// path with parent-traversal, ...). Backends should prefer this + /// over `Backend(_)` for caller-fault errors so the model can + /// reason about retry strategy. + #[error("invalid argument: {message}")] + InvalidArgument { + /// Human-readable description; safe to surface to the model. + message: String, + }, + + /// The operation's outer timeout (see + /// [`WorkspaceServices::operation_timeout`](super::WorkspaceServices::operation_timeout)) + /// fired before the backend responded. + #[error("workspace operation '{op}' timed out after {duration:?}")] + Timeout { + /// Human-readable operation name, e.g. `read_text` or `s3.get_object`. + op: String, + /// Configured timeout that expired. + duration: Duration, + }, + + /// The backend explicitly does not support this operation. + /// + /// Used by adapters that wrap a partial trait surface (e.g. the + /// remote git backend rejecting worktree operations even though + /// `WorkspaceGit` is implemented). + #[error("not supported by this backend: {0}")] + Unsupported(String), + + /// Catch-all wrapping a lower-level error that does not map to one + /// of the typed variants above. This is the bridge between the + /// existing `anyhow::Result` world and the typed surface — when a + /// backend throws a generic I/O / HTTP / SDK error it ends up here. + #[error(transparent)] + Backend(#[from] anyhow::Error), +} + +impl WorkspaceError { + /// Convert an `anyhow::Error` to a `WorkspaceError`, **preserving + /// the typed variant** when the original cause was one of the + /// known conflict structs. + /// + /// `Into::into` (auto-derived from the `#[from] anyhow::Error` + /// variant) drops every `anyhow::Error` into the `Backend` arm + /// because at the value level `anyhow::Error` has type-erased its + /// source. Use this helper instead when migrating code paths that + /// today emit `anyhow::Error::new(WorkspaceVersionConflict { .. })` + /// or `anyhow::Error::new(RemoteGitConflict { .. })` — the typed + /// variant survives the round-trip. + /// + /// ```ignore + /// // Old code: + /// fn legacy() -> anyhow::Result<()> { ... } + /// // New caller: + /// let typed = WorkspaceError::from_anyhow(legacy().unwrap_err()); + /// match typed { + /// WorkspaceError::VersionConflict(v) => retry(v), + /// other => return Err(other), + /// } + /// ``` + pub fn from_anyhow(err: anyhow::Error) -> Self { + if let Some(conflict) = err.downcast_ref::() { + return Self::VersionConflict(conflict.clone()); + } + if let Some(conflict) = err.downcast_ref::() { + return Self::RemoteGitConflict(conflict.clone()); + } + Self::Backend(err) + } +} + +/// Result alias used throughout the workspace trait surface in v3.0+. +/// +/// In v2.x this co-exists with [`anyhow::Result`] (the legacy return +/// type of every trait method); in v3.0 the trait surface will return +/// `WorkspaceResult` directly. See the module docs for the +/// two-commit migration plan. +pub type WorkspaceResult = std::result::Result; + +#[cfg(test)] +mod tests { + use super::*; + use anyhow::anyhow; + + #[test] + fn anyhow_with_version_conflict_round_trips_through_from_anyhow() { + let conflict = WorkspaceVersionConflict { + path: "doc.md".to_string(), + expected: "etag-1".to_string(), + actual: Some("etag-2".to_string()), + }; + let err: anyhow::Error = anyhow::Error::new(conflict.clone()); + + let typed = WorkspaceError::from_anyhow(err); + match typed { + WorkspaceError::VersionConflict(v) => { + assert_eq!(v.path, "doc.md"); + assert_eq!(v.expected, "etag-1"); + assert_eq!(v.actual.as_deref(), Some("etag-2")); + } + other => panic!("expected VersionConflict, got {other:?}"), + } + } + + #[test] + fn anyhow_with_remote_git_conflict_round_trips_through_from_anyhow() { + let conflict = RemoteGitConflict { + code: "BRANCH_EXISTS".to_string(), + message: "branch 'feat/x' already exists".to_string(), + }; + let err: anyhow::Error = anyhow::Error::new(conflict); + + let typed = WorkspaceError::from_anyhow(err); + match typed { + WorkspaceError::RemoteGitConflict(c) => { + assert_eq!(c.code, "BRANCH_EXISTS"); + assert!(c.message.contains("feat/x")); + } + other => panic!("expected RemoteGitConflict, got {other:?}"), + } + } + + #[test] + fn anyhow_without_known_type_falls_into_backend_variant() { + let err: anyhow::Error = anyhow!("some I/O thing exploded"); + let typed = WorkspaceError::from_anyhow(err); + match typed { + WorkspaceError::Backend(e) => { + assert!(e.to_string().contains("I/O thing exploded")); + } + other => panic!("expected Backend, got {other:?}"), + } + } + + #[test] + fn workspace_error_converts_back_to_anyhow_via_blanket_impl() { + // anyhow's blanket `From` impl + // means `?` on a `WorkspaceResult` inside an `anyhow::Result` + // function lifts cleanly. This is the only thing keeping + // existing `anyhow::Result`-returning callers compatible during + // the Phase 7.3.b migration. + fn produce() -> WorkspaceResult<()> { + Err(WorkspaceError::NotFound { + path: "missing.txt".into(), + }) + } + fn consumes_anyhow() -> anyhow::Result<()> { + produce()?; + Ok(()) + } + let err = consumes_anyhow().unwrap_err(); + assert!(err.to_string().contains("missing.txt")); + // The original typed value is still recoverable via downcast. + assert!(err.downcast_ref::().is_some()); + } + + #[test] + fn version_conflict_struct_converts_via_from() { + // The auto-derived `#[from] WorkspaceVersionConflict` impl lets + // backends build a `WorkspaceError` directly from the existing + // conflict struct without going through anyhow first. + let conflict = WorkspaceVersionConflict { + path: "x.txt".into(), + expected: "v1".into(), + actual: None, + }; + let err: WorkspaceError = conflict.into(); + matches!(err, WorkspaceError::VersionConflict(_)) + .then_some(()) + .expect("From must produce VersionConflict variant"); + } + + #[test] + fn invalid_argument_variant_carries_message_in_display() { + let err = WorkspaceError::InvalidArgument { + message: "expected_version must not be empty".into(), + }; + let s = err.to_string(); + assert!(s.contains("invalid argument"), "got: {s}"); + assert!(s.contains("expected_version"), "got: {s}"); + } + + #[test] + fn timeout_variant_carries_op_and_duration_in_display() { + let err = WorkspaceError::Timeout { + op: "read_text".into(), + duration: Duration::from_secs(30), + }; + let s = err.to_string(); + assert!(s.contains("read_text"), "got: {s}"); + assert!(s.contains("30"), "got: {s}"); + } + + #[test] + fn unsupported_variant_names_the_operation() { + let err = WorkspaceError::Unsupported("worktree on remote git".into()); + let s = err.to_string(); + assert!(s.contains("not supported"), "got: {s}"); + assert!(s.contains("worktree"), "got: {s}"); + } +} diff --git a/core/src/workspace/local.rs b/core/src/workspace/local.rs index 6f028aa..3e74aaf 100644 --- a/core/src/workspace/local.rs +++ b/core/src/workspace/local.rs @@ -8,14 +8,15 @@ use super::{ default_path_input, has_windows_path_prefix, normalize_relative_path, pathbuf_to_workspace_path, validate_relative_pattern, CommandOutput, CommandRequest, - WorkspaceCommandRunner, WorkspaceDirEntry, WorkspaceFileSystem, WorkspaceFileType, - WorkspaceGit, WorkspaceGitBranch, WorkspaceGitCheckoutOutput, WorkspaceGitCheckoutRequest, - WorkspaceGitCommit, WorkspaceGitCreateBranchRequest, WorkspaceGitCreateWorktreeRequest, - WorkspaceGitDiffRequest, WorkspaceGitRemote, WorkspaceGitRemoveWorktreeRequest, - WorkspaceGitStash, WorkspaceGitStashProvider, WorkspaceGitStashRequest, WorkspaceGitStatus, - WorkspaceGitWorktree, WorkspaceGitWorktreeMutation, WorkspaceGitWorktreeProvider, - WorkspaceGlobRequest, WorkspaceGlobResult, WorkspaceGrepRequest, WorkspaceGrepResult, - WorkspacePath, WorkspacePathResolver, WorkspaceSearch, WorkspaceWriteOutcome, + WorkspaceCommandRunner, WorkspaceDirEntry, WorkspaceError, WorkspaceFileSystem, + WorkspaceFileType, WorkspaceGit, WorkspaceGitBranch, WorkspaceGitCheckoutOutput, + WorkspaceGitCheckoutRequest, WorkspaceGitCommit, WorkspaceGitCreateBranchRequest, + WorkspaceGitCreateWorktreeRequest, WorkspaceGitDiffRequest, WorkspaceGitRemote, + WorkspaceGitRemoveWorktreeRequest, WorkspaceGitStash, WorkspaceGitStashProvider, + WorkspaceGitStashRequest, WorkspaceGitStatus, WorkspaceGitWorktree, + WorkspaceGitWorktreeMutation, WorkspaceGitWorktreeProvider, WorkspaceGlobRequest, + WorkspaceGlobResult, WorkspaceGrepRequest, WorkspaceGrepResult, WorkspacePath, + WorkspacePathResolver, WorkspaceResult, WorkspaceSearch, WorkspaceWriteOutcome, }; use anyhow::{anyhow, bail, Result}; use async_trait::async_trait; @@ -79,22 +80,34 @@ impl WorkspacePathResolver for LocalWorkspaceBackend { #[async_trait] impl WorkspaceFileSystem for LocalWorkspaceBackend { - async fn read_text(&self, path: &WorkspacePath) -> Result { + async fn read_text(&self, path: &WorkspacePath) -> WorkspaceResult { let resolved = self.local_path_for_read(path)?; - tokio::fs::read_to_string(&resolved) - .await - .map_err(|e| anyhow!("Failed to read file {}: {}", resolved.display(), e)) + match tokio::fs::read_to_string(&resolved).await { + Ok(s) => Ok(s), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Err(WorkspaceError::NotFound { + path: resolved.display().to_string(), + }), + Err(e) => Err(WorkspaceError::Backend(anyhow!( + "Failed to read file {}: {}", + resolved.display(), + e + ))), + } } async fn write_text( &self, path: &WorkspacePath, content: &str, - ) -> Result { + ) -> WorkspaceResult { let resolved = self.local_path_for_write(path)?; - tokio::fs::write(&resolved, content) - .await - .map_err(|e| anyhow!("Failed to write file {}: {}", resolved.display(), e))?; + tokio::fs::write(&resolved, content).await.map_err(|e| { + WorkspaceError::Backend(anyhow!( + "Failed to write file {}: {}", + resolved.display(), + e + )) + })?; Ok(WorkspaceWriteOutcome { bytes: content.len(), @@ -102,21 +115,33 @@ impl WorkspaceFileSystem for LocalWorkspaceBackend { }) } - async fn list_dir(&self, path: &WorkspacePath) -> Result> { + async fn list_dir(&self, path: &WorkspacePath) -> WorkspaceResult> { let target = self.local_path_for_read(path)?; if !target.exists() { - bail!("Directory not found: {}", target.display()); + return Err(WorkspaceError::NotFound { + path: target.display().to_string(), + }); } if !target.is_dir() { - bail!("Not a directory: {}", target.display()); + return Err(WorkspaceError::InvalidArgument { + message: format!("Not a directory: {}", target.display()), + }); } - let mut dir = tokio::fs::read_dir(&target) - .await - .map_err(|e| anyhow!("Failed to read directory {}: {}", target.display(), e))?; + let mut dir = tokio::fs::read_dir(&target).await.map_err(|e| { + WorkspaceError::Backend(anyhow!( + "Failed to read directory {}: {}", + target.display(), + e + )) + })?; let mut entries = Vec::new(); - while let Some(entry) = dir.next_entry().await? { + while let Some(entry) = dir + .next_entry() + .await + .map_err(|e| WorkspaceError::Backend(anyhow!("Failed to iterate directory: {}", e)))? + { let name = entry.file_name().to_string_lossy().to_string(); let file_type = entry.file_type().await; let metadata = entry.metadata().await; diff --git a/core/src/workspace/mod.rs b/core/src/workspace/mod.rs index 716049f..5213884 100644 --- a/core/src/workspace/mod.rs +++ b/core/src/workspace/mod.rs @@ -7,11 +7,17 @@ //! browser, DFS, or container-backed implementations by assembling //! [`WorkspaceServices`] through [`WorkspaceServicesBuilder`]. +#[cfg(test)] +pub(crate) mod conformance; +mod error; mod local; +mod remote_git; #[cfg(feature = "s3")] mod s3; +pub use error::{WorkspaceError, WorkspaceResult}; pub use local::LocalWorkspaceBackend; +pub use remote_git::{RemoteGitBackend, RemoteGitBackendConfig, RemoteGitConflict}; #[cfg(feature = "s3")] pub use s3::{S3BackendConfig, S3WorkspaceBackend}; @@ -342,13 +348,66 @@ pub trait WorkspacePathResolver: Send + Sync { /// traits. #[async_trait] pub trait WorkspaceFileSystem: Send + Sync { - async fn read_text(&self, path: &WorkspacePath) -> Result; + async fn read_text(&self, path: &WorkspacePath) -> WorkspaceResult; async fn write_text( &self, path: &WorkspacePath, content: &str, - ) -> Result; - async fn list_dir(&self, path: &WorkspacePath) -> Result>; + ) -> WorkspaceResult; + async fn list_dir(&self, path: &WorkspacePath) -> WorkspaceResult>; +} + +/// Error returned by [`WorkspaceFileSystemExt::write_text_if_version`] when +/// the underlying object version no longer matches the expected version. +/// +/// Surfaced through `anyhow::Error`; tools recover by downcasting: +/// `err.downcast_ref::()`. The typical response is +/// to re-read the file and retry the modify-write cycle once. +#[derive(Debug, Clone, thiserror::Error)] +#[error( + "version conflict on {path}: expected version {expected:?}, found {actual:?} (file modified by another writer; re-read and retry)" +)] +pub struct WorkspaceVersionConflict { + pub path: String, + pub expected: String, + /// Backend-reported current version, if known. S3 does not return the + /// current ETag on `412 Precondition Failed`, so this is typically `None`. + pub actual: Option, +} + +/// Optional compare-and-swap extensions to [`WorkspaceFileSystem`]. +/// +/// Implemented by backends that expose object-level versioning (S3 ETag, +/// future GCS generation, ...) so tools that perform read-modify-write +/// cycles can reject concurrent overwrites. Tools should access this through +/// [`WorkspaceServices::fs_ext`] — when absent, callers fall back to plain +/// `read_text` / `write_text` (last-writer-wins). +/// +/// Kept as a separate trait rather than inheriting from +/// [`WorkspaceFileSystem`] so existing backend implementations are not +/// forced to opt in. +#[async_trait] +pub trait WorkspaceFileSystemExt: Send + Sync { + /// Read text content together with an opaque version token. Tokens are + /// backend-specific (S3 returns the ETag) and treated as opaque by + /// callers — they are only ever compared for equality on the backend + /// side. + async fn read_text_with_version( + &self, + path: &WorkspacePath, + ) -> WorkspaceResult<(String, String)>; + + /// Write content iff the current object version matches `expected_version`. + /// On mismatch the returned error is the typed + /// [`WorkspaceError::VersionConflict`] variant; callers can also still + /// downcast through `anyhow::Error` when the value has been lifted into + /// the legacy result type. + async fn write_text_if_version( + &self, + path: &WorkspacePath, + content: &str, + expected_version: &str, + ) -> WorkspaceResult; } /// Shell/command execution available to the `bash` tool. @@ -417,6 +476,7 @@ pub struct WorkspaceServices { capabilities: WorkspaceCapabilities, path_resolver: Arc, file_system: Arc, + file_system_ext: Option>, command_runner: Option>, search: Option>, git: Option>, @@ -434,6 +494,7 @@ impl std::fmt::Debug for WorkspaceServices { f.debug_struct("WorkspaceServices") .field("workspace_ref", &self.workspace_ref) .field("capabilities", &self.capabilities) + .field("file_system_ext", &self.file_system_ext.is_some()) .field("command_runner", &self.command_runner.is_some()) .field("search", &self.search.is_some()) .field("git", &self.git.is_some()) @@ -468,6 +529,7 @@ impl WorkspaceServices { capabilities, path_resolver, file_system, + file_system_ext: None, command_runner, search, git, @@ -503,6 +565,7 @@ impl WorkspaceServices { capabilities: WorkspaceCapabilities::local_default(), path_resolver, file_system, + file_system_ext: None, command_runner: Some(command_runner), search: Some(search), git: Some(git), @@ -529,6 +592,16 @@ impl WorkspaceServices { Arc::clone(&self.file_system) } + /// Optional compare-and-swap file system extensions. + /// + /// Returns `Some` when the backend supports version-aware writes (e.g. + /// S3 via ETag). Tools that perform read-modify-write cycles should + /// route through [`Self::read_for_edit`] and [`Self::write_for_edit`] + /// rather than touching this directly. + pub fn fs_ext(&self) -> Option> { + self.file_system_ext.clone() + } + pub fn command_runner(&self) -> Option> { self.command_runner.clone() } @@ -549,6 +622,45 @@ impl WorkspaceServices { self.git_worktree.clone() } + /// Internal helper used by decorators (`with_remote_git` and any + /// future git-provider override) to swap the git layer of an existing + /// `WorkspaceServices` without losing unrelated fields. + /// + /// Every field is **explicitly listed** in the returned struct + /// literal. This is the point of the helper — adding a new field to + /// `WorkspaceServices` will trip a compile error here, and the author + /// of that new field has to decide whether a git-provider swap + /// preserves it. Previously the decorator went through + /// `WorkspaceServicesBuilder`, which silently dropped any field the + /// builder did not know about (notably `local_root`). + /// + /// `git_worktree` is reset to `None` because worktree operations are + /// part of the same domain as the git provider — keeping the local + /// worktree provider while routing `status`/`log`/`diff` to a remote + /// server would surface inconsistent state to the model. + pub(crate) fn with_git_provider( + &self, + git: Arc, + git_stash: Option>, + ) -> Arc { + let mut capabilities = self.capabilities; + capabilities.git = true; + Arc::new(Self { + workspace_ref: self.workspace_ref.clone(), + capabilities, + path_resolver: Arc::clone(&self.path_resolver), + file_system: Arc::clone(&self.file_system), + file_system_ext: self.file_system_ext.clone(), + command_runner: self.command_runner.clone(), + search: self.search.clone(), + git: Some(git), + git_stash, + git_worktree: None, + operation_timeout: self.operation_timeout, + local_root: self.local_root.clone(), + }) + } + /// Default timeout applied to non-bash workspace operations. /// /// `None` means no enforced timeout. Backends that may stall (remote, @@ -563,18 +675,95 @@ impl WorkspaceServices { /// Tools that route through file system / search / git providers should /// wrap their calls with this helper so non-local backends never stall /// the agent loop indefinitely. - pub async fn run_with_timeout(&self, op: &'static str, fut: F) -> Result + /// + /// Polymorphic in the error type so the helper works equally well for + /// futures returning `anyhow::Result` (the legacy callers — search, + /// git, etc.) and for futures returning [`WorkspaceResult`] (the + /// migrated `WorkspaceFileSystem` callers). The `E: From` + /// bound is satisfied by both `anyhow::Error` (trivially) and + /// [`WorkspaceError`] (via its `#[from]` `Backend` variant); a timeout + /// surfaces as that From conversion of an `anyhow!(...)` message. + pub async fn run_with_timeout( + &self, + op: &'static str, + fut: F, + ) -> std::result::Result where - F: std::future::Future>, + F: std::future::Future>, + E: From, { match self.operation_timeout { - Some(d) => tokio::time::timeout(d, fut) - .await - .map_err(|_| anyhow!("workspace operation '{}' timed out after {:?}", op, d))?, + Some(d) => tokio::time::timeout(d, fut).await.map_err(|_| { + E::from(anyhow!( + "workspace operation '{}' timed out after {:?}", + op, + d + )) + })?, None => fut.await, } } + /// Read a file for a subsequent modify-write cycle, requesting a version + /// token when the backend supports compare-and-swap writes. + /// + /// Returns `(content, Some(version))` when [`Self::fs_ext`] is available + /// (e.g. on S3, where the version is the object ETag); `(content, None)` + /// otherwise. Pair with [`Self::write_for_edit`]. + pub async fn read_for_edit( + &self, + path: &WorkspacePath, + ) -> WorkspaceResult<(String, Option)> { + if let Some(ext) = self.fs_ext() { + let path = path.clone(); + return self + .run_with_timeout("read_text_with_version", async move { + let (content, version) = ext.read_text_with_version(&path).await?; + Ok((content, Some(version))) + }) + .await; + } + let fs = self.fs(); + let path_owned = path.clone(); + let content = self + .run_with_timeout("read_text", async move { fs.read_text(&path_owned).await }) + .await?; + Ok((content, None)) + } + + /// Companion to [`Self::read_for_edit`]. Performs a compare-and-swap + /// write when both [`Self::fs_ext`] is available *and* a version token + /// was returned by the prior read; falls back to a plain write + /// otherwise. On version mismatch the returned error is the typed + /// [`WorkspaceError::VersionConflict`] variant; callers can also still + /// downcast `anyhow::Error::downcast_ref::()` + /// when the value has been lifted into an `anyhow::Result`. + pub async fn write_for_edit( + &self, + path: &WorkspacePath, + content: &str, + expected_version: Option<&str>, + ) -> WorkspaceResult { + if let (Some(ext), Some(version)) = (self.fs_ext(), expected_version) { + let path = path.clone(); + let content = content.to_string(); + let expected = version.to_string(); + return self + .run_with_timeout("write_text_if_version", async move { + ext.write_text_if_version(&path, &content, &expected).await + }) + .await; + } + let fs = self.fs(); + let path = path.clone(); + let content = content.to_string(); + self.run_with_timeout( + "write_text", + async move { fs.write_text(&path, &content).await }, + ) + .await + } + pub fn local_root(&self) -> Option<&Path> { self.local_root.as_deref() } @@ -599,6 +788,7 @@ pub struct WorkspaceServicesBuilder { capabilities: WorkspaceCapabilities, path_resolver: Arc, file_system: Arc, + file_system_ext: Option>, command_runner: Option>, search: Option>, git: Option>, @@ -614,6 +804,7 @@ impl WorkspaceServicesBuilder { capabilities: WorkspaceCapabilities::read_write(), path_resolver: Arc::new(VirtualPathResolver), file_system, + file_system_ext: None, command_runner: None, search: None, git: None, @@ -656,6 +847,15 @@ impl WorkspaceServicesBuilder { self } + /// Attach optional compare-and-swap file system extensions + /// ([`WorkspaceFileSystemExt`]). Tools that perform read-modify-write + /// cycles will pick this up via [`WorkspaceServices::read_for_edit`] + /// and [`WorkspaceServices::write_for_edit`]. + pub fn file_system_ext(mut self, ext: Arc) -> Self { + self.file_system_ext = Some(ext); + self + } + /// Apply a default timeout to non-bash workspace operations (file system, /// search, git). Backends that may stall — remote, browser, DFS — should /// set this so tools surface a timeout error rather than hanging. @@ -674,6 +874,7 @@ impl WorkspaceServicesBuilder { self.search, self.git, ); + services.file_system_ext = self.file_system_ext; services.git_stash = self.git_stash; services.git_worktree = self.git_worktree; services.operation_timeout = self.operation_timeout; @@ -824,20 +1025,23 @@ mod tests { #[async_trait] impl WorkspaceFileSystem for EmptyFs { - async fn read_text(&self, _path: &WorkspacePath) -> Result { - bail!("not implemented") + async fn read_text(&self, _path: &WorkspacePath) -> WorkspaceResult { + Err(WorkspaceError::Unsupported("not implemented".into())) } async fn write_text( &self, _path: &WorkspacePath, _content: &str, - ) -> Result { - bail!("not implemented") + ) -> WorkspaceResult { + Err(WorkspaceError::Unsupported("not implemented".into())) } - async fn list_dir(&self, _path: &WorkspacePath) -> Result> { - bail!("not implemented") + async fn list_dir( + &self, + _path: &WorkspacePath, + ) -> WorkspaceResult> { + Err(WorkspaceError::Unsupported("not implemented".into())) } } @@ -855,4 +1059,177 @@ mod tests { assert!(!services.capabilities().exec); assert!(services.command_runner().is_none()); } + + // --- helpers for fs_ext / read_for_edit / write_for_edit coverage --- + // + // The mock backend used here is `InMemoryFileSystem` from the + // `workspace::conformance` module — the same fixture the conformance + // suite runs against, so any drift between the test fixture and the + // contract documented for new backends shows up immediately. Tests + // capture the auto-generated version at read time rather than + // hard-coding it, which keeps assertions decoupled from the version + // scheme of the mock. + + use super::conformance::InMemoryFileSystem; + + fn versioned_services(fs: Arc) -> Arc { + let fs_ws: Arc = fs.clone(); + let fs_ext: Arc = fs; + WorkspaceServices::builder(WorkspaceRef::new("mem", "mem://ws"), fs_ws) + .file_system_ext(fs_ext) + .build() + } + + /// Seed a file into the mock and return its current version, so callers + /// can use it as an expected value without hardcoding the version + /// scheme. + async fn seed(fs: &Arc, path: &str, content: &str) -> String { + use super::WorkspaceFileSystemExt; + let ws_path = WorkspacePath::from_normalized(path); + (*fs).write_text(&ws_path, content).await.unwrap(); + let (_, version) = (*fs).read_text_with_version(&ws_path).await.unwrap(); + version + } + + #[test] + fn version_conflict_is_downcastable_from_anyhow() { + let e: anyhow::Error = anyhow::Error::new(WorkspaceVersionConflict { + path: "a/b.txt".to_string(), + expected: "etag-1".to_string(), + actual: Some("etag-2".to_string()), + }); + let c = e.downcast_ref::().unwrap(); + assert_eq!(c.path, "a/b.txt"); + assert_eq!(c.expected, "etag-1"); + assert_eq!(c.actual.as_deref(), Some("etag-2")); + // Display must include the path and both versions so logs are useful. + let msg = e.to_string(); + assert!(msg.contains("a/b.txt"), "msg: {msg}"); + assert!(msg.contains("etag-1"), "msg: {msg}"); + } + + #[tokio::test] + async fn read_for_edit_returns_version_when_ext_available() { + let fs = Arc::new(InMemoryFileSystem::new()); + let seeded_version = seed(&fs, "notes.md", "hello").await; + let services = versioned_services(fs); + + let path = WorkspacePath::from_normalized("notes.md"); + let (content, version) = services.read_for_edit(&path).await.unwrap(); + assert_eq!(content, "hello"); + assert_eq!( + version.as_deref(), + Some(seeded_version.as_str()), + "read_for_edit must return the version produced by the prior write" + ); + } + + #[tokio::test] + async fn read_for_edit_returns_no_version_when_ext_absent() { + struct PlainFs; + #[async_trait] + impl WorkspaceFileSystem for PlainFs { + async fn read_text(&self, _path: &WorkspacePath) -> WorkspaceResult { + Ok("plain".to_string()) + } + async fn write_text( + &self, + _path: &WorkspacePath, + content: &str, + ) -> WorkspaceResult { + Ok(WorkspaceWriteOutcome { + bytes: content.len(), + lines: content.lines().count(), + }) + } + async fn list_dir( + &self, + _path: &WorkspacePath, + ) -> WorkspaceResult> { + Ok(Vec::new()) + } + } + let fs: Arc = Arc::new(PlainFs); + let services = + WorkspaceServices::builder(WorkspaceRef::new("plain", "plain://ws"), fs).build(); + + let path = WorkspacePath::from_normalized("any.txt"); + let (content, version) = services.read_for_edit(&path).await.unwrap(); + assert_eq!(content, "plain"); + assert!(version.is_none()); + assert!(services.fs_ext().is_none()); + } + + #[tokio::test] + async fn write_for_edit_succeeds_on_matching_version() { + let fs = Arc::new(InMemoryFileSystem::new()); + seed(&fs, "doc.md", "alpha").await; + let services = versioned_services(fs.clone()); + let path = WorkspacePath::from_normalized("doc.md"); + + let (content, version) = services.read_for_edit(&path).await.unwrap(); + assert_eq!(content, "alpha"); + + services + .write_for_edit(&path, "beta", version.as_deref()) + .await + .expect("write should succeed with matching version"); + + let current = fs.read_text(&path).await.unwrap(); + assert_eq!(current, "beta"); + } + + #[tokio::test] + async fn write_for_edit_surfaces_conflict_when_version_changed() { + let fs = Arc::new(InMemoryFileSystem::new()); + let seeded_version = seed(&fs, "doc.md", "alpha").await; + let services = versioned_services(fs.clone()); + let path = WorkspacePath::from_normalized("doc.md"); + + let (_, version) = services.read_for_edit(&path).await.unwrap(); + // Simulate a concurrent overwrite — a real "second writer" doing + // exactly what the conflict protection is meant to catch. + fs.write_text(&path, "from-concurrent-writer") + .await + .unwrap(); + + let err = services + .write_for_edit(&path, "beta", version.as_deref()) + .await + .expect_err("write should reject with conflict"); + let WorkspaceError::VersionConflict(conflict) = err else { + panic!("expected WorkspaceError::VersionConflict, got {err:?}"); + }; + assert_eq!(conflict.path, "doc.md"); + assert_eq!(conflict.expected, seeded_version); + // We don't pin the actual version's exact value — only that the + // backend supplies one and that it differs from what we expected. + let actual = conflict + .actual + .as_deref() + .expect("conflict must report the current version"); + assert_ne!(actual, seeded_version); + } + + #[tokio::test] + async fn write_for_edit_falls_back_to_plain_write_when_version_is_none() { + // Even with fs_ext present, passing version=None must route through + // unconditional write_text (e.g. for fresh-create paths). + let fs = Arc::new(InMemoryFileSystem::new()); + seed(&fs, "doc.md", "alpha").await; + let services = versioned_services(fs.clone()); + let path = WorkspacePath::from_normalized("doc.md"); + + // Concurrent overwriter, but caller did not request CAS semantics: + fs.write_text(&path, "from-concurrent-writer") + .await + .unwrap(); + + services + .write_for_edit(&path, "beta", None) + .await + .expect("plain write should not check version"); + let current = fs.read_text(&path).await.unwrap(); + assert_eq!(current, "beta"); + } } diff --git a/core/src/workspace/remote_git.rs b/core/src/workspace/remote_git.rs new file mode 100644 index 0000000..6d14ef4 --- /dev/null +++ b/core/src/workspace/remote_git.rs @@ -0,0 +1,1500 @@ +//! Remote `WorkspaceGit` backend. +//! +//! Talks an HTTP/JSON protocol to a host-operated `gitserver` so non-local +//! workspaces (S3, future container / DFS) can offer the `git` tool to +//! the model. The full protocol specification lives in the RFC at +//! `apps/docs/content/docs/en/code/rfcs/workspace-remote-git.mdx`. This +//! module is the Rust client side of that protocol. +//! +//! # Capabilities +//! +//! Implements [`WorkspaceGit`] in full and [`WorkspaceGitStashProvider`]. +//! Deliberately does **not** implement [`WorkspaceGitWorktreeProvider`]: +//! worktrees are a local-filesystem concept that does not map cleanly onto +//! a remote service. Tools that need per-branch isolation on remote +//! workspaces should use separate sessions with separate `repo_id`s. +//! +//! # Observability +//! +//! Every HTTP call emits a `tracing::debug!` event with the same field +//! shape used by `S3WorkspaceBackend` (op / target / outcome / bytes / +//! duration_ms / status). Hosts that already meter S3 cost via that +//! channel pick up gitserver cost for free. +//! +//! # Authentication +//! +//! Bearer token (default). Empty token mode is permitted for localhost +//! development and emits a warn on construction. mTLS is supported by +//! setting both `client_cert_pem` and `client_key_pem` on the config — +//! the files are read at backend construction, concatenated (cert + key) +//! and handed to `reqwest::Identity::from_pem`. Setting only one of the +//! pair fails at construction with a clear error. + +use super::{ + WorkspaceGit, WorkspaceGitBranch, WorkspaceGitCheckoutOutput, WorkspaceGitCheckoutRequest, + WorkspaceGitCommit, WorkspaceGitCreateBranchRequest, WorkspaceGitDiffRequest, + WorkspaceGitRemote, WorkspaceGitStash, WorkspaceGitStashProvider, WorkspaceGitStashRequest, + WorkspaceGitStatus, +}; +use anyhow::{anyhow, Result}; +use async_trait::async_trait; +use reqwest::{Client, StatusCode}; +use serde::{Deserialize, Serialize}; +use std::path::PathBuf; +use std::sync::Arc; +use std::time::Duration; + +/// Default per-call HTTP timeout, applied to every request the client makes. +pub const DEFAULT_REQUEST_TIMEOUT: Duration = Duration::from_secs(30); + +/// Default body-size cap for `diff` responses. The server should honour the +/// same ceiling and set `truncated: true` if it had to clip; this is the +/// client-side defence. +pub const DEFAULT_MAX_DIFF_BYTES: u64 = 1024 * 1024; + +/// Default ceiling on `log` `max_count` — caps the per-call response size +/// even when the model requests more. +pub const DEFAULT_MAX_LOG_ENTRIES: usize = 200; + +/// Configuration for a [`RemoteGitBackend`]. +/// +/// `base_url` should not have a trailing slash; the client constructs +/// `{base_url}/v1/repos/{repo_id}/git/{op}` per the RFC. +#[derive(Debug, Clone)] +pub struct RemoteGitBackendConfig { + pub base_url: String, + pub repo_id: String, + pub bearer_token: Option, + /// mTLS client certificate path (PEM). When set together with + /// `client_key_pem`, the backend reads both files at construction, + /// concatenates them, and configures `reqwest::Identity::from_pem` + /// on the HTTP client. Setting only one of the pair errors at + /// construction. + pub client_cert_pem: Option, + /// mTLS client private key path (PEM). See `client_cert_pem`. The key + /// must be in PKCS#8 PEM format for the `rustls-tls` backend. + pub client_key_pem: Option, + pub request_timeout: Option, + pub max_diff_bytes: Option, + pub max_log_entries: Option, +} + +impl RemoteGitBackendConfig { + pub fn new(base_url: impl Into, repo_id: impl Into) -> Self { + Self { + base_url: base_url.into(), + repo_id: repo_id.into(), + bearer_token: None, + client_cert_pem: None, + client_key_pem: None, + request_timeout: None, + max_diff_bytes: None, + max_log_entries: None, + } + } + + pub fn bearer_token(mut self, token: impl Into) -> Self { + self.bearer_token = Some(token.into()); + self + } + + pub fn request_timeout(mut self, timeout: Duration) -> Self { + self.request_timeout = Some(timeout); + self + } + + pub fn max_diff_bytes(mut self, bytes: u64) -> Self { + self.max_diff_bytes = Some(bytes); + self + } + + pub fn max_log_entries(mut self, n: usize) -> Self { + self.max_log_entries = Some(n); + self + } + + pub fn client_cert_pem(mut self, path: impl Into) -> Self { + self.client_cert_pem = Some(path.into()); + self + } + + pub fn client_key_pem(mut self, path: impl Into) -> Self { + self.client_key_pem = Some(path.into()); + self + } +} + +/// Error returned for HTTP 409 / 422 responses that carry a recoverable +/// failure code. Tools downcast with `anyhow::Error::downcast_ref` to react +/// — for example, retrying after a `WORKING_TREE_DIRTY` by stashing first. +#[derive(Debug, Clone, thiserror::Error)] +#[error("remote git conflict: {code}: {message}")] +pub struct RemoteGitConflict { + pub code: String, + pub message: String, +} + +/// Client for a remote `gitserver`. See module docs / RFC for the protocol. +#[derive(Debug, Clone)] +pub struct RemoteGitBackend { + http: Client, + base_url: String, + repo_id: String, + bearer_token: Option, + max_diff_bytes: u64, + max_log_entries: usize, +} + +impl RemoteGitBackend { + /// Build a backend from declarative configuration. + pub fn new(config: RemoteGitBackendConfig) -> Result> { + if config + .bearer_token + .as_deref() + .map(str::is_empty) + .unwrap_or(true) + && config.client_cert_pem.is_none() + { + tracing::warn!( + "RemoteGitBackend constructed without bearer token or mTLS; \ + this is only safe on a trusted localhost gitserver" + ); + } + + let mut builder = + Client::builder().timeout(config.request_timeout.unwrap_or(DEFAULT_REQUEST_TIMEOUT)); + + // mTLS: both files must be present, otherwise fail closed. + match ( + config.client_cert_pem.as_deref(), + config.client_key_pem.as_deref(), + ) { + (Some(cert_path), Some(key_path)) => { + let identity = load_mtls_identity(cert_path, key_path)?; + builder = builder.identity(identity); + } + (Some(_), None) => { + return Err(anyhow!( + "client_cert_pem was set without client_key_pem; both must be provided for mTLS" + )); + } + (None, Some(_)) => { + return Err(anyhow!( + "client_key_pem was set without client_cert_pem; both must be provided for mTLS" + )); + } + (None, None) => {} + } + + let http = builder + .build() + .map_err(|e| anyhow!("failed to build reqwest client: {}", e))?; + + let base_url = config.base_url.trim_end_matches('/').to_string(); + Ok(Arc::new(Self { + http, + base_url, + repo_id: config.repo_id, + bearer_token: config.bearer_token, + max_diff_bytes: config.max_diff_bytes.unwrap_or(DEFAULT_MAX_DIFF_BYTES), + max_log_entries: config.max_log_entries.unwrap_or(DEFAULT_MAX_LOG_ENTRIES), + })) + } + + /// Base URL the client is configured to use (no trailing slash). + pub fn base_url(&self) -> &str { + &self.base_url + } + + /// Opaque repository identifier passed in every request URL. + pub fn repo_id(&self) -> &str { + &self.repo_id + } + + pub fn max_diff_bytes(&self) -> u64 { + self.max_diff_bytes + } + + pub fn max_log_entries(&self) -> usize { + self.max_log_entries + } + + fn endpoint(&self, op: &str) -> String { + format!("{}/v1/repos/{}/git/{}", self.base_url, self.repo_id, op) + } + + async fn post_json(&self, op: &'static str, body: &Req) -> Result + where + Req: Serialize + ?Sized, + Resp: for<'de> Deserialize<'de>, + { + let url = self.endpoint(op); + let mut req = self.http.post(&url).json(body); + if let Some(token) = self.bearer_token.as_deref() { + if !token.is_empty() { + req = req.bearer_auth(token); + } + } + + let start = std::time::Instant::now(); + let send_result = req.send().await; + let status_code = send_result.as_ref().ok().map(|r| r.status().as_u16()); + let ok = matches!(send_result.as_ref(), Ok(r) if r.status().is_success()); + emit_remote_git_event(op, &self.repo_id, status_code, ok, start.elapsed(), None); + + let resp = + send_result.map_err(|e| anyhow!("remote git call '{}' transport error: {}", op, e))?; + + let status = resp.status(); + if status.is_success() { + let parsed = resp + .json::() + .await + .map_err(|e| anyhow!("remote git '{}' response body decode error: {}", op, e))?; + return Ok(parsed); + } + + let body_text = resp.text().await.unwrap_or_default(); + Err(map_error_response(op, status, &body_text)) + } + + async fn post_unit(&self, op: &'static str, body: &Req) -> Result<()> + where + Req: Serialize + ?Sized, + { + let url = self.endpoint(op); + let mut req = self.http.post(&url).json(body); + if let Some(token) = self.bearer_token.as_deref() { + if !token.is_empty() { + req = req.bearer_auth(token); + } + } + + let start = std::time::Instant::now(); + let send_result = req.send().await; + let status_code = send_result.as_ref().ok().map(|r| r.status().as_u16()); + let ok = matches!(send_result.as_ref(), Ok(r) if r.status().is_success()); + emit_remote_git_event(op, &self.repo_id, status_code, ok, start.elapsed(), None); + + let resp = + send_result.map_err(|e| anyhow!("remote git call '{}' transport error: {}", op, e))?; + + let status = resp.status(); + if status.is_success() { + return Ok(()); + } + let body_text = resp.text().await.unwrap_or_default(); + Err(map_error_response(op, status, &body_text)) + } + + /// Like [`Self::post_json`] but with a hard cap on the streamed response + /// body in bytes, intended for endpoints that can legitimately return + /// large payloads (`diff`). + /// + /// Two layers of defence: + /// 1. If the server sends a `Content-Length` greater than `max_bytes`, + /// the request is rejected before any body is consumed. + /// 2. Otherwise the body is streamed; once the accumulated buffer + /// exceeds `max_bytes`, the stream is dropped and the call returns + /// an error. Memory is bounded at `max_bytes + one chunk`. + /// + /// Used by [`WorkspaceGit::diff`]; protects against a misbehaving + /// gitserver that ignores the client's soft `max_diff_bytes`. + async fn post_streamed( + &self, + op: &'static str, + body: &Req, + max_bytes: u64, + ) -> Result> + where + Req: Serialize + ?Sized, + { + use futures::StreamExt; + + let url = self.endpoint(op); + let mut req = self.http.post(&url).json(body); + if let Some(token) = self.bearer_token.as_deref() { + if !token.is_empty() { + req = req.bearer_auth(token); + } + } + + let start = std::time::Instant::now(); + let send_result = req.send().await; + let status_code = send_result.as_ref().ok().map(|r| r.status().as_u16()); + let resp = match send_result { + Ok(r) => r, + Err(e) => { + emit_remote_git_event(op, &self.repo_id, status_code, false, start.elapsed(), None); + return Err(anyhow!("remote git call '{}' transport error: {}", op, e)); + } + }; + + // Layer 1: eager rejection on advertised oversized body. + if let Some(len) = resp.content_length() { + if len > max_bytes { + emit_remote_git_event( + op, + &self.repo_id, + status_code, + false, + start.elapsed(), + Some(len), + ); + return Err(anyhow!( + "remote git '{}' Content-Length {} exceeds client cap {} bytes; \ + refusing to download. Raise max_diff_bytes if the body is legitimate.", + op, + len, + max_bytes + )); + } + } + + // Layer 2: stream-bound accumulation. + let status = resp.status(); + let mut stream = resp.bytes_stream(); + let mut buf: Vec = Vec::new(); + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(|e| anyhow!("remote git '{}' stream error: {}", op, e))?; + if (buf.len() as u64).saturating_add(chunk.len() as u64) > max_bytes { + emit_remote_git_event( + op, + &self.repo_id, + status_code, + false, + start.elapsed(), + Some(buf.len() as u64), + ); + return Err(anyhow!( + "remote git '{}' response body exceeded client cap {} bytes mid-stream; \ + aborting", + op, + max_bytes + )); + } + buf.extend_from_slice(&chunk); + } + + emit_remote_git_event( + op, + &self.repo_id, + status_code, + status.is_success(), + start.elapsed(), + Some(buf.len() as u64), + ); + + if !status.is_success() { + let body_text = String::from_utf8_lossy(&buf).into_owned(); + return Err(map_error_response(op, status, &body_text)); + } + Ok(buf) + } +} + +#[derive(Serialize)] +struct EmptyReq; + +#[derive(Deserialize)] +struct StatusResp { + branch: String, + commit: String, + #[serde(default)] + is_worktree: bool, + #[serde(default)] + is_dirty: bool, + #[serde(default)] + dirty_count: usize, +} + +#[derive(Serialize)] +struct LogReq { + max_count: usize, +} + +#[derive(Deserialize)] +struct LogResp { + commits: Vec, +} + +#[derive(Deserialize)] +struct CommitDto { + id: String, + message: String, + author: String, + date: String, +} + +#[derive(Deserialize)] +struct BranchesResp { + branches: Vec, +} + +#[derive(Deserialize)] +struct BranchDto { + name: String, + #[serde(default)] + is_current: bool, +} + +#[derive(Serialize)] +struct CreateBranchReq<'a> { + name: &'a str, + base: &'a str, +} + +#[derive(Serialize)] +struct CheckoutReq<'a> { + refspec: &'a str, + force: bool, +} + +#[derive(Deserialize)] +struct CheckoutResp { + #[serde(default)] + stdout: String, +} + +#[derive(Serialize)] +struct DiffReq<'a> { + target: Option<&'a str>, +} + +#[derive(Deserialize)] +struct DiffResp { + diff: String, + #[serde(default)] + truncated: bool, +} + +#[derive(Deserialize)] +struct RemotesResp { + remotes: Vec, +} + +#[derive(Deserialize)] +struct RemoteDto { + name: String, + url: String, + #[serde(default = "default_direction")] + direction: String, +} + +fn default_direction() -> String { + "fetch".to_string() +} + +#[derive(Deserialize)] +struct ExistsResp { + #[serde(default)] + is_repository: bool, +} + +#[derive(Deserialize)] +struct StashesResp { + stashes: Vec, +} + +#[derive(Deserialize)] +struct StashDto { + index: usize, + #[serde(default)] + message: String, +} + +#[derive(Serialize)] +struct StashCreateReq { + #[serde(skip_serializing_if = "Option::is_none")] + message: Option, + include_untracked: bool, +} + +#[async_trait] +impl WorkspaceGit for RemoteGitBackend { + async fn is_repository(&self) -> Result { + let resp: ExistsResp = self.post_json("exists", &EmptyReq).await?; + Ok(resp.is_repository) + } + + async fn status(&self) -> Result { + let resp: StatusResp = self.post_json("status", &EmptyReq).await?; + Ok(WorkspaceGitStatus { + branch: resp.branch, + commit: resp.commit, + is_worktree: resp.is_worktree, + is_dirty: resp.is_dirty, + dirty_count: resp.dirty_count, + }) + } + + async fn log(&self, max_count: usize) -> Result> { + let capped = max_count.min(self.max_log_entries); + let resp: LogResp = self.post_json("log", &LogReq { max_count: capped }).await?; + Ok(resp + .commits + .into_iter() + .map(|c| WorkspaceGitCommit { + id: c.id, + message: c.message, + author: c.author, + date: c.date, + }) + .collect()) + } + + async fn list_branches(&self) -> Result> { + let resp: BranchesResp = self.post_json("branches", &EmptyReq).await?; + Ok(resp + .branches + .into_iter() + .map(|b| WorkspaceGitBranch { + name: b.name, + is_current: b.is_current, + }) + .collect()) + } + + async fn create_branch(&self, request: WorkspaceGitCreateBranchRequest) -> Result<()> { + self.post_unit( + "branches/create", + &CreateBranchReq { + name: &request.name, + base: &request.base, + }, + ) + .await + } + + async fn checkout( + &self, + request: WorkspaceGitCheckoutRequest, + ) -> Result { + let resp: CheckoutResp = self + .post_json( + "checkout", + &CheckoutReq { + refspec: &request.refspec, + force: request.force, + }, + ) + .await?; + Ok(WorkspaceGitCheckoutOutput { + stdout: resp.stdout, + }) + } + + async fn diff(&self, request: WorkspaceGitDiffRequest) -> Result { + // Two-layered defence against a misbehaving gitserver: + // + // * **Hard memory cap** = `max_diff_bytes * 4` (floor 64 KiB). The + // request streams the body and aborts once this is exceeded, so a + // server returning a 1 GiB diff never gets fully buffered. We + // allow 4× slack over the soft cap so legitimate-but-large diffs + // reach the parser and can be display-truncated below. + // * **Soft display cap** = `max_diff_bytes`. Applied after JSON + // decode: the diff text we hand back to the tool is shortened to + // this many bytes (UTF-8-safe) so callers see a useful preview + // without the model context bloating. + const DIFF_HARD_CAP_FLOOR: u64 = 64 * 1024; + let hard_cap = self + .max_diff_bytes + .saturating_mul(4) + .max(DIFF_HARD_CAP_FLOOR); + + let bytes = self + .post_streamed( + "diff", + &DiffReq { + target: request.target.as_deref(), + }, + hard_cap, + ) + .await?; + let resp: DiffResp = serde_json::from_slice(&bytes) + .map_err(|e| anyhow!("remote git 'diff' response body decode error: {}", e))?; + + if (resp.diff.len() as u64) > self.max_diff_bytes { + tracing::debug!( + "remote git diff body {} bytes exceeds max_diff_bytes {} — \ + client-side display truncation", + resp.diff.len(), + self.max_diff_bytes + ); + let cap = self.max_diff_bytes as usize; + let mut trimmed = resp.diff; + trimmed.truncate(safe_utf8_truncate(&trimmed, cap)); + trimmed.push_str("\n... [truncated by client max_diff_bytes]\n"); + return Ok(trimmed); + } + if resp.truncated { + return Ok(format!("{}\n... [truncated by gitserver]\n", resp.diff)); + } + Ok(resp.diff) + } + + async fn list_remotes(&self) -> Result> { + let resp: RemotesResp = self.post_json("remotes", &EmptyReq).await?; + Ok(resp + .remotes + .into_iter() + .map(|r| WorkspaceGitRemote { + name: r.name, + url: r.url, + direction: r.direction, + }) + .collect()) + } +} + +#[async_trait] +impl WorkspaceGitStashProvider for RemoteGitBackend { + async fn list_stashes(&self) -> Result> { + let resp: StashesResp = self.post_json("stashes", &EmptyReq).await?; + Ok(resp + .stashes + .into_iter() + .map(|s| WorkspaceGitStash { + index: s.index, + message: s.message, + }) + .collect()) + } + + async fn stash(&self, request: WorkspaceGitStashRequest) -> Result<()> { + self.post_unit( + "stashes/create", + &StashCreateReq { + message: request.message, + include_untracked: request.include_untracked, + }, + ) + .await + } +} + +/// Read the mTLS cert + key PEM files and assemble a `reqwest::Identity`. +/// +/// `reqwest::Identity::from_pem` (with the `rustls-tls` backend) wants a +/// single PEM blob containing the certificate chain followed by the +/// private key. We concatenate the two files with a newline separator — +/// stray trailing newlines in either file are tolerated by the PEM +/// parser. Errors at every step (file I/O, PEM parsing) are mapped to +/// `anyhow` with the source path included so misconfigurations surface +/// clearly. +fn load_mtls_identity( + cert_path: &std::path::Path, + key_path: &std::path::Path, +) -> Result { + let cert = std::fs::read(cert_path).map_err(|e| { + anyhow!( + "failed to read mTLS client_cert_pem at {}: {}", + cert_path.display(), + e + ) + })?; + let key = std::fs::read(key_path).map_err(|e| { + anyhow!( + "failed to read mTLS client_key_pem at {}: {}", + key_path.display(), + e + ) + })?; + + let mut pem = Vec::with_capacity(cert.len() + key.len() + 1); + pem.extend_from_slice(&cert); + if !cert.ends_with(b"\n") { + pem.push(b'\n'); + } + pem.extend_from_slice(&key); + + reqwest::Identity::from_pem(&pem).map_err(|e| { + anyhow!( + "failed to parse mTLS PEM material (cert={}, key={}): {}", + cert_path.display(), + key_path.display(), + e + ) + }) +} + +/// Truncate `s` to at most `max_bytes`, rounding down to the nearest UTF-8 +/// character boundary to keep the result a valid `&str`. +fn safe_utf8_truncate(s: &str, max_bytes: usize) -> usize { + if s.len() <= max_bytes { + return s.len(); + } + let mut idx = max_bytes; + while idx > 0 && !s.is_char_boundary(idx) { + idx -= 1; + } + idx +} + +/// Map a non-2xx response to an `anyhow::Error`, attaching a typed +/// [`RemoteGitConflict`] when the server returned a recoverable code under +/// 409 or 422. +/// +/// Synchronous and takes the pre-fetched response body so it can be shared +/// between callers that hold a `reqwest::Response` and callers that have +/// already streamed the body into a `Vec` (for size-capped paths). +fn map_error_response(op: &'static str, status: StatusCode, body: &str) -> anyhow::Error { + let parsed: Option = serde_json::from_str(body).ok(); + + let (code, message) = match parsed { + Some(b) => (b.error.code, b.error.message), + None => (format!("HTTP_{}", status.as_u16()), body.to_string()), + }; + + let status_u16 = status.as_u16(); + if status_u16 == 409 || status_u16 == 422 { + return anyhow::Error::new(RemoteGitConflict { code, message }); + } + + match status_u16 { + 400 => anyhow!("remote git '{}' bad request: {}: {}", op, code, message), + 401 | 403 => anyhow!("remote git '{}' auth failed: {}: {}", op, code, message), + 404 => anyhow!("remote git '{}' not found: {}: {}", op, code, message), + 500..=599 => anyhow!( + "remote git '{}' server error ({}): {}: {}", + op, + status_u16, + code, + message + ), + _ => anyhow!( + "remote git '{}' unexpected status {}: {}: {}", + op, + status_u16, + code, + message + ), + } +} + +#[derive(Deserialize)] +struct RemoteErrorBody { + error: RemoteErrorDetail, +} + +#[derive(Deserialize)] +struct RemoteErrorDetail { + code: String, + #[serde(default)] + message: String, +} + +/// Emit a structured `tracing::debug!` event for a single gitserver call. +/// +/// Mirrors the metering shape used by `S3WorkspaceBackend::emit_s3_call_event` +/// so a single subscriber can meter both backends. Fields: +/// +/// | Field | Meaning | +/// |---------------|--------------------------------------------------| +/// | `op` | gitserver op (`status`, `log`, `diff`, ...) | +/// | `repo_id` | opaque repo identifier | +/// | `status` | HTTP status code (when the request reached server) | +/// | `outcome` | `ok` \| `error` | +/// | `bytes` | response body length, when known | +/// | `duration_ms` | wall-clock | +fn emit_remote_git_event( + op: &'static str, + repo_id: &str, + status: Option, + ok: bool, + elapsed: Duration, + bytes: Option, +) { + tracing::debug!( + op = format!("git.{}", op), + repo_id = %repo_id, + status = status.unwrap_or(0), + outcome = if ok { "ok" } else { "error" }, + bytes = bytes.unwrap_or(0), + duration_ms = elapsed.as_millis() as u64, + ); +} + +impl super::WorkspaceServices { + /// Attach a remote git provider to an existing [`WorkspaceServices`]. + /// + /// Returns a new `Arc` with `git` and `git_stash` + /// wired to the remote backend. The original `WorkspaceServices` is + /// not mutated. `git_worktree` is intentionally reset to `None` — + /// worktrees are a local-filesystem concept that does not map cleanly + /// onto a remote service (see RFC §8). All other fields — including + /// `local_root`, the command runner, the search provider, the + /// optional `file_system_ext` (S3 CAS), and `operation_timeout` — are + /// preserved verbatim via + /// [`super::WorkspaceServices::with_git_provider`]. + pub fn with_remote_git(self: Arc, config: RemoteGitBackendConfig) -> Result> { + let backend = RemoteGitBackend::new(config)?; + let git: Arc = backend.clone(); + let stash: Arc = backend; + Ok(self.with_git_provider(git, Some(stash))) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + use wiremock::matchers::{header, method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + async fn server_and_backend() -> (MockServer, Arc) { + let server = MockServer::start().await; + let cfg = RemoteGitBackendConfig::new(server.uri(), "test") + .bearer_token("test-token") + .request_timeout(Duration::from_secs(5)); + let backend = RemoteGitBackend::new(cfg).unwrap(); + (server, backend) + } + + #[test] + fn config_defaults_are_documented() { + let cfg = RemoteGitBackendConfig::new("http://localhost", "r"); + assert!(cfg.bearer_token.is_none()); + assert!(cfg.client_cert_pem.is_none()); + assert!(cfg.request_timeout.is_none()); + assert!(cfg.max_diff_bytes.is_none()); + assert!(cfg.max_log_entries.is_none()); + } + + #[test] + fn endpoint_url_format_matches_rfc() { + let cfg = RemoteGitBackendConfig::new("http://localhost:8080/", "u1/s1"); + let backend = RemoteGitBackend::new(cfg).unwrap(); + // Trailing slash on base_url is stripped. + assert_eq!(backend.base_url(), "http://localhost:8080"); + assert_eq!( + backend.endpoint("status"), + "http://localhost:8080/v1/repos/u1/s1/git/status" + ); + assert_eq!( + backend.endpoint("branches/create"), + "http://localhost:8080/v1/repos/u1/s1/git/branches/create" + ); + } + + #[test] + fn mtls_requires_both_cert_and_key() { + let cfg = RemoteGitBackendConfig::new("http://localhost", "r").client_cert_pem("/dev/null"); + let err = RemoteGitBackend::new(cfg).unwrap_err(); + assert!( + err.to_string().contains("client_key_pem"), + "missing-key error must name the missing field, got: {}", + err + ); + + let cfg = RemoteGitBackendConfig::new("http://localhost", "r").client_key_pem("/dev/null"); + let err = RemoteGitBackend::new(cfg).unwrap_err(); + assert!( + err.to_string().contains("client_cert_pem"), + "missing-cert error must name the missing field, got: {}", + err + ); + } + + #[test] + fn mtls_rejects_invalid_pem_blob() { + let tmp = tempfile::tempdir().unwrap(); + let cert = tmp.path().join("cert.pem"); + let key = tmp.path().join("key.pem"); + std::fs::write(&cert, b"not a pem").unwrap(); + std::fs::write(&key, b"also not a pem").unwrap(); + + let cfg = RemoteGitBackendConfig::new("http://localhost", "r") + .client_cert_pem(&cert) + .client_key_pem(&key); + let err = RemoteGitBackend::new(cfg).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("PEM"), + "PEM-parse failure must surface clearly, got: {}", + msg + ); + assert!( + msg.contains(cert.to_str().unwrap()), + "error must include the cert path for debugging, got: {}", + msg + ); + } + + #[test] + fn mtls_accepts_self_signed_pair_from_rcgen() { + // rcgen produces a valid cert + PKCS#8 key pair; `reqwest::Identity` + // (rustls-tls backend) should accept the concatenated PEM blob. + let cert = rcgen::generate_simple_self_signed(vec!["localhost".to_string()]) + .expect("rcgen self-signed cert"); + let tmp = tempfile::tempdir().unwrap(); + let cert_path = tmp.path().join("client.cert.pem"); + let key_path = tmp.path().join("client.key.pem"); + std::fs::write(&cert_path, cert.cert.pem()).unwrap(); + std::fs::write(&key_path, cert.key_pair.serialize_pem()).unwrap(); + + let cfg = RemoteGitBackendConfig::new("http://localhost", "r") + .bearer_token("t") + .client_cert_pem(&cert_path) + .client_key_pem(&key_path); + let backend = RemoteGitBackend::new(cfg) + .expect("valid rcgen-generated PEM pair must produce a backend"); + // We cannot easily verify the identity is wired into the client without + // a live mTLS server; the assertion above (construction succeeds) is the + // contract — invalid material would have errored at `from_pem`. + assert_eq!(backend.base_url(), "http://localhost"); + } + + #[test] + fn safe_utf8_truncate_respects_boundaries() { + // ASCII path + assert_eq!(safe_utf8_truncate("hello", 3), 3); + assert_eq!(safe_utf8_truncate("hello", 100), 5); + // Multi-byte path: "héllo" — 'é' is 2 bytes (0xC3 0xA9) + let s = "héllo"; + // Truncating at byte 2 lands inside 'é'; rounds down to 1 (after 'h') + assert_eq!(safe_utf8_truncate(s, 2), 1); + assert_eq!(safe_utf8_truncate(s, 3), 3); + } + + #[tokio::test] + async fn status_happy_path() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/status")) + .and(header("authorization", "Bearer test-token")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "branch": "main", + "commit": "abc123", + "is_worktree": false, + "is_dirty": true, + "dirty_count": 3, + }))) + .mount(&server) + .await; + + let status = backend.status().await.unwrap(); + assert_eq!(status.branch, "main"); + assert_eq!(status.commit, "abc123"); + assert!(status.is_dirty); + assert_eq!(status.dirty_count, 3); + } + + #[tokio::test] + async fn log_respects_client_max_log_entries() { + let server = MockServer::start().await; + let cfg = RemoteGitBackendConfig::new(server.uri(), "test") + .bearer_token("t") + .max_log_entries(5); + let backend = RemoteGitBackend::new(cfg).unwrap(); + + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/log")) + .and(wiremock::matchers::body_json(json!({"max_count": 5}))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "commits": [ + {"id":"a","message":"m","author":"x","date":"d"} + ] + }))) + .mount(&server) + .await; + + // Client asks for 100, but the server should see the capped value. + let commits = backend.log(100).await.unwrap(); + assert_eq!(commits.len(), 1); + assert_eq!(commits[0].id, "a"); + } + + #[tokio::test] + async fn list_branches_maps_response() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/branches")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "branches": [ + {"name":"main", "is_current":true}, + {"name":"feat/x"} + ] + }))) + .mount(&server) + .await; + + let branches = backend.list_branches().await.unwrap(); + assert_eq!(branches.len(), 2); + assert!(branches[0].is_current); + assert!(!branches[1].is_current); + } + + #[tokio::test] + async fn create_branch_succeeds_on_201() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/branches/create")) + .and(wiremock::matchers::body_json(json!({ + "name":"feat/x","base":"main" + }))) + .respond_with(ResponseTemplate::new(201).set_body_json(json!({}))) + .mount(&server) + .await; + + backend + .create_branch(WorkspaceGitCreateBranchRequest { + name: "feat/x".into(), + base: "main".into(), + }) + .await + .unwrap(); + } + + #[tokio::test] + async fn create_branch_409_yields_remote_git_conflict() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/branches/create")) + .respond_with(ResponseTemplate::new(409).set_body_json(json!({ + "error":{"code":"BRANCH_EXISTS","message":"branch 'feat/x' already exists"} + }))) + .mount(&server) + .await; + + let err = backend + .create_branch(WorkspaceGitCreateBranchRequest { + name: "feat/x".into(), + base: "main".into(), + }) + .await + .unwrap_err(); + let conflict = err + .downcast_ref::() + .expect("409 must downcast to RemoteGitConflict"); + assert_eq!(conflict.code, "BRANCH_EXISTS"); + assert!(conflict.message.contains("feat/x")); + } + + #[tokio::test] + async fn checkout_returns_stdout() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/checkout")) + .and(wiremock::matchers::body_json(json!({ + "refspec":"feat/x","force":false + }))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "stdout":"Switched to branch 'feat/x'" + }))) + .mount(&server) + .await; + + let out = backend + .checkout(WorkspaceGitCheckoutRequest { + refspec: "feat/x".into(), + force: false, + }) + .await + .unwrap(); + assert!(out.stdout.contains("feat/x")); + } + + #[tokio::test] + async fn checkout_409_dirty_yields_conflict() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/checkout")) + .respond_with(ResponseTemplate::new(409).set_body_json(json!({ + "error":{"code":"WORKING_TREE_DIRTY","message":"please stash first"} + }))) + .mount(&server) + .await; + + let err = backend + .checkout(WorkspaceGitCheckoutRequest { + refspec: "main".into(), + force: false, + }) + .await + .unwrap_err(); + let c = err.downcast_ref::().unwrap(); + assert_eq!(c.code, "WORKING_TREE_DIRTY"); + } + + #[tokio::test] + async fn diff_passes_target_through_and_surfaces_server_truncation() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/diff")) + .and(wiremock::matchers::body_json(json!({"target":"main"}))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "diff":"", + "truncated": true + }))) + .mount(&server) + .await; + + let diff = backend + .diff(WorkspaceGitDiffRequest { + target: Some("main".to_string()), + }) + .await + .unwrap(); + assert!(diff.contains("truncated by gitserver")); + } + + #[tokio::test] + async fn diff_enforces_client_max_diff_bytes() { + let server = MockServer::start().await; + let cfg = RemoteGitBackendConfig::new(server.uri(), "test") + .bearer_token("t") + .max_diff_bytes(8); + let backend = RemoteGitBackend::new(cfg).unwrap(); + + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/diff")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "diff":"AAAAAAAAAAAAAAAAAAAAAA", // 22 bytes + "truncated": false + }))) + .mount(&server) + .await; + + let diff = backend + .diff(WorkspaceGitDiffRequest { target: None }) + .await + .unwrap(); + assert!(diff.contains("truncated by client max_diff_bytes")); + // First 8 bytes preserved. + assert!(diff.starts_with("AAAAAAAA")); + } + + /// Phase 6.2 OOM defence: the gitserver advertises a Content-Length far + /// beyond what the client tolerates. The request must fail without + /// consuming the body. + /// + /// `max_diff_bytes = 8` ⇒ `hard_cap = max(8 * 4, 64 KiB) = 64 KiB`. + /// We respond with `Content-Length: 1 048 576` so the eager rejection + /// path fires. + #[tokio::test] + async fn diff_rejects_oversized_content_length_upfront() { + let server = MockServer::start().await; + let cfg = RemoteGitBackendConfig::new(server.uri(), "test") + .bearer_token("t") + .max_diff_bytes(8); + let backend = RemoteGitBackend::new(cfg).unwrap(); + + // 1 MiB body — far past the 64 KiB hard cap floor. + let huge_body = vec![b'A'; 1024 * 1024]; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/diff")) + .respond_with( + ResponseTemplate::new(200) + .insert_header("content-type", "application/json") + .set_body_bytes(huge_body), + ) + .mount(&server) + .await; + + let err = backend + .diff(WorkspaceGitDiffRequest { target: None }) + .await + .expect_err("oversized body must be rejected"); + let msg = err.to_string(); + assert!( + msg.contains("Content-Length") && msg.contains("exceeds client cap"), + "expected eager Content-Length rejection, got: {}", + msg + ); + } + + /// Phase 6.2 OOM defence layer 2: when Content-Length is absent or the + /// server lies about it, the stream-bound accumulator must abort once + /// the cap is exceeded. We use chunked transfer (no Content-Length) so + /// the eager path doesn't fire. + #[tokio::test] + async fn diff_aborts_mid_stream_on_cap_exceeded() { + let server = MockServer::start().await; + let cfg = RemoteGitBackendConfig::new(server.uri(), "test") + .bearer_token("t") + .max_diff_bytes(8); + let backend = RemoteGitBackend::new(cfg).unwrap(); + + // Body large enough to exceed the 64 KiB hard cap floor; chunked + // transfer encoded so no Content-Length header is set. + let big_body = vec![b'A'; 256 * 1024]; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/diff")) + .respond_with( + ResponseTemplate::new(200) + .insert_header("transfer-encoding", "chunked") + .set_body_bytes(big_body), + ) + .mount(&server) + .await; + + let err = backend + .diff(WorkspaceGitDiffRequest { target: None }) + .await + .expect_err("oversized streamed body must be rejected"); + let msg = err.to_string(); + // Either the eager path (if wiremock surfaces a Content-Length) or + // the stream-abort path fires; both are valid defences. + assert!( + msg.contains("exceeds client cap") + || msg.contains("exceeded client cap") + || msg.contains("Content-Length"), + "expected oversize rejection, got: {}", + msg + ); + } + + #[tokio::test] + async fn list_remotes_defaults_direction() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/remotes")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "remotes":[{"name":"origin","url":"git@x:y.git"}] + }))) + .mount(&server) + .await; + + let rs = backend.list_remotes().await.unwrap(); + assert_eq!(rs.len(), 1); + assert_eq!(rs[0].direction, "fetch"); + } + + #[tokio::test] + async fn is_repository_returns_bool() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/exists")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "is_repository": true + }))) + .mount(&server) + .await; + + assert!(backend.is_repository().await.unwrap()); + } + + #[tokio::test] + async fn list_stashes_maps_response() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/stashes")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "stashes":[{"index":0,"message":"WIP"}] + }))) + .mount(&server) + .await; + + let s = backend.list_stashes().await.unwrap(); + assert_eq!(s.len(), 1); + assert_eq!(s[0].message, "WIP"); + } + + #[tokio::test] + async fn stash_create_409_nothing_to_stash() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/stashes/create")) + .respond_with(ResponseTemplate::new(409).set_body_json(json!({ + "error":{"code":"NOTHING_TO_STASH","message":"clean tree"} + }))) + .mount(&server) + .await; + + let err = backend + .stash(WorkspaceGitStashRequest { + message: None, + include_untracked: false, + }) + .await + .unwrap_err(); + let c = err.downcast_ref::().unwrap(); + assert_eq!(c.code, "NOTHING_TO_STASH"); + } + + #[tokio::test] + async fn not_found_404_is_generic_anyhow() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/status")) + .respond_with(ResponseTemplate::new(404).set_body_json(json!({ + "error":{"code":"REPO_NOT_FOUND","message":"unknown repo"} + }))) + .mount(&server) + .await; + + let err = backend.status().await.unwrap_err(); + assert!(err.to_string().contains("not found"), "msg: {}", err); + assert!(err.downcast_ref::().is_none()); + } + + #[tokio::test] + async fn auth_failure_401_is_generic_anyhow() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/status")) + .respond_with(ResponseTemplate::new(401).set_body_json(json!({ + "error":{"code":"INVALID_TOKEN","message":"bad bearer"} + }))) + .mount(&server) + .await; + + let err = backend.status().await.unwrap_err(); + assert!(err.to_string().contains("auth failed"), "msg: {}", err); + assert!(err.downcast_ref::().is_none()); + } + + #[tokio::test] + async fn server_500_is_generic_anyhow() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/status")) + .respond_with(ResponseTemplate::new(500).set_body_string("boom")) + .mount(&server) + .await; + + let err = backend.status().await.unwrap_err(); + assert!(err.to_string().contains("server error"), "msg: {}", err); + } + + #[tokio::test] + async fn non_json_error_body_falls_back_to_http_code() { + let (server, backend) = server_and_backend().await; + Mock::given(method("POST")) + .and(path("/v1/repos/test/git/status")) + .respond_with(ResponseTemplate::new(409).set_body_string("not json")) + .mount(&server) + .await; + + let err = backend.status().await.unwrap_err(); + // 409 always yields a conflict — even when the body is opaque, we + // surface it so callers can detect it; the code falls back to + // HTTP_409. + let c = err + .downcast_ref::() + .expect("409 must yield conflict regardless of body shape"); + assert_eq!(c.code, "HTTP_409"); + assert_eq!(c.message, "not json"); + } + + #[tokio::test] + async fn with_remote_git_wires_git_and_stash() { + let services = super::super::WorkspaceServices::local(std::env::temp_dir()); + let upgraded = services + .with_remote_git(RemoteGitBackendConfig::new("http://localhost", "r").bearer_token("t")) + .unwrap(); + assert!(upgraded.git().is_some()); + assert!(upgraded.git_stash().is_some()); + // Worktree provider intentionally dropped on remote-git workspaces — + // worktrees do not have a remote analogue (see RFC §8). + assert!(upgraded.git_worktree().is_none()); + assert!(upgraded.capabilities().git); + } + + /// Regression test for Phase 6.1 field-loss bug. + /// + /// `with_remote_git` previously rebuilt `WorkspaceServices` via the + /// builder, which silently dropped `local_root` (and would silently + /// drop any future field). After the fix it goes through + /// `with_git_provider`, which uses an explicit struct literal — the + /// compiler now forces every field to be addressed. + #[tokio::test] + async fn with_remote_git_preserves_local_root_and_unrelated_capabilities() { + let temp = tempfile::tempdir().unwrap(); + let base = super::super::WorkspaceServices::local(temp.path()); + assert!( + base.local_root().is_some(), + "precondition: local() must set local_root" + ); + assert!( + base.command_runner().is_some(), + "precondition: local() must wire bash runner" + ); + let base_root = base.local_root().map(|p| p.to_path_buf()); + + let upgraded = base + .with_remote_git(RemoteGitBackendConfig::new("http://localhost", "r").bearer_token("t")) + .unwrap(); + + // The git provider IS replaced. + assert!(upgraded.git().is_some()); + assert!(upgraded.capabilities().git); + // Unrelated capabilities survive. + assert_eq!( + upgraded.local_root().map(|p| p.to_path_buf()), + base_root, + "local_root must survive with_remote_git" + ); + assert!( + upgraded.command_runner().is_some(), + "command_runner must survive with_remote_git" + ); + assert!( + upgraded.search().is_some(), + "search provider must survive with_remote_git" + ); + // But worktree is intentionally severed alongside the git swap. + assert!(upgraded.git_worktree().is_none()); + } + + /// End-to-end test: drive the built-in `git` tool against a wiremock-backed + /// gitserver. Exercises the full path `git tool → WorkspaceGit (remote) → + /// HTTP → wiremock → JSON → DTO → WorkspaceGitStatus → tool output`. + /// + /// This is the contract test for Phase 4.2: if any layer breaks, this + /// test fails. Per-method unit tests above isolate the HTTP layer; this + /// one proves the tool wiring actually works through a real ToolContext. + #[tokio::test] + async fn git_tool_status_works_through_remote_backend() { + use crate::tools::{Tool, ToolContext}; + + let server = MockServer::start().await; + // `git` tool probes `is_repository` before dispatching. + Mock::given(method("POST")) + .and(path("/v1/repos/u1/s1/git/exists")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({"is_repository": true}))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/v1/repos/u1/s1/git/status")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "branch":"main", + "commit":"deadbeef", + "is_worktree": false, + "is_dirty": false, + "dirty_count": 0, + }))) + .mount(&server) + .await; + + let base = super::super::WorkspaceServices::local(std::env::temp_dir()); + let services = base + .with_remote_git(RemoteGitBackendConfig::new(server.uri(), "u1/s1").bearer_token("tok")) + .unwrap(); + + let tool = crate::tools::builtin::git::GitTool; + let ctx = ToolContext::new(std::env::temp_dir()).with_workspace_services(services); + + let result = tool + .execute(&json!({"command": "status"}), &ctx) + .await + .unwrap(); + assert!(result.success, "tool output: {}", result.content); + assert!( + result.content.contains("main"), + "expected branch name in output: {}", + result.content + ); + assert!( + result.content.contains("deadbeef"), + "expected commit hash in output: {}", + result.content + ); + assert!( + result.content.contains("clean"), + "expected clean status in output: {}", + result.content + ); + } +} diff --git a/core/src/workspace/s3.rs b/core/src/workspace/s3.rs index 7de7041..df06103 100644 --- a/core/src/workspace/s3.rs +++ b/core/src/workspace/s3.rs @@ -19,10 +19,22 @@ //! to the same key will overwrite each other (last-writer-wins). Callers //! that need stronger guarantees should partition workspaces per session. //! +//! # Memory bounds +//! +//! [`S3WorkspaceBackend::read_text`] enforces a `max_read_bytes` ceiling +//! (default [`DEFAULT_MAX_READ_BYTES`]) by inspecting `Content-Length` on the +//! `GetObject` response before consuming the body. Oversized objects are +//! rejected with a clear error and never buffered into memory. Override the +//! limit via [`S3BackendConfig::max_read_bytes`] when reading larger text +//! artifacts is legitimate. +//! //! Available only when the `s3` feature is enabled. use super::{ - WorkspaceDirEntry, WorkspaceFileSystem, WorkspaceFileType, WorkspacePath, WorkspaceWriteOutcome, + validate_relative_pattern, WorkspaceDirEntry, WorkspaceError, WorkspaceFileSystem, + WorkspaceFileSystemExt, WorkspaceFileType, WorkspaceGlobRequest, WorkspaceGlobResult, + WorkspaceGrepRequest, WorkspaceGrepResult, WorkspacePath, WorkspaceResult, WorkspaceSearch, + WorkspaceVersionConflict, WorkspaceWriteOutcome, }; use anyhow::{anyhow, Result}; use async_trait::async_trait; @@ -30,6 +42,7 @@ use aws_credential_types::Credentials; use aws_sdk_s3::config::{BehaviorVersion, Region}; use aws_sdk_s3::error::SdkError; use aws_sdk_s3::operation::list_objects_v2::ListObjectsV2Error; +use aws_sdk_s3::operation::put_object::PutObjectError; use aws_sdk_s3::primitives::ByteStream; use aws_sdk_s3::Client; use std::sync::Arc; @@ -37,6 +50,32 @@ use std::time::Duration; const DEFAULT_REGION: &str = "us-east-1"; +/// Default cap on the size of a single object readable via [`S3WorkspaceBackend::read_text`]. +/// +/// 10 MiB. Generous for typical source / config files, far below the AWS +/// per-object limit. Override per workspace with [`S3BackendConfig::max_read_bytes`]. +pub const DEFAULT_MAX_READ_BYTES: u64 = 10 * 1024 * 1024; + +/// Default cap on the number of objects scanned by a single [`WorkspaceSearch`] +/// call (`grep` or `glob`) when the S3 search capability is enabled. +/// +/// Override per workspace with [`S3BackendConfig::max_objects_scanned`]. The +/// scan stops once this many keys have been considered and the result is +/// marked truncated. +pub const DEFAULT_MAX_OBJECTS_SCANNED: usize = 500; + +/// Default per-object size ceiling for `grep` when the S3 search capability is +/// enabled. Objects larger than this are skipped (tracing::debug) rather than +/// fully downloaded. Override with [`S3BackendConfig::max_grep_bytes_per_object`]. +pub const DEFAULT_MAX_GREP_BYTES_PER_OBJECT: u64 = 1024 * 1024; + +/// Default concurrency for `grep` object downloads. The backend fetches up to +/// this many objects in parallel; the remaining work serializes after each +/// completes. Override with [`S3BackendConfig::search_concurrency`]. Set lower +/// when the gitserver or S3 endpoint rate-limits aggressively; set higher +/// when latency dominates. +pub const DEFAULT_SEARCH_CONCURRENCY: usize = 8; + /// Configuration for an [`S3WorkspaceBackend`]. /// /// `endpoint` is optional: omit it to use the AWS default. Set it to point at @@ -60,6 +99,33 @@ pub struct S3BackendConfig { /// the workspace-level `operation_timeout` set on [`super::WorkspaceServices`]; /// whichever fires first wins. pub request_timeout: Option, + /// Maximum bytes that may be returned by a single [`WorkspaceFileSystem::read_text`] + /// call. Enforced before the response body is consumed by inspecting the + /// `Content-Length` reported by S3. Defaults to [`DEFAULT_MAX_READ_BYTES`] + /// when `None`. + pub max_read_bytes: Option, + /// Enables the `grep` / `glob` built-in tools against this S3 backend. + /// + /// Defaults to `false` — object storage cannot natively service search, + /// and the only available implementation strategy (List + GET + regex) + /// can produce non-trivial S3 API costs. Hosts must opt in explicitly. + /// When `false`, capability gating hides `grep` and `glob` from the + /// model entirely; when `true`, they are registered and constrained by + /// [`Self::max_objects_scanned`] and [`Self::max_grep_bytes_per_object`]. + pub search_enabled: bool, + /// Upper bound on objects considered during a single search. `None` + /// applies [`DEFAULT_MAX_OBJECTS_SCANNED`]. Ignored when + /// `search_enabled` is `false`. + pub max_objects_scanned: Option, + /// Per-object size ceiling for `grep` body downloads. Objects larger than + /// this are skipped (debug-traced) rather than fetched. `None` applies + /// [`DEFAULT_MAX_GREP_BYTES_PER_OBJECT`]. Ignored when `search_enabled` is + /// `false`. + pub max_grep_bytes_per_object: Option, + /// Number of concurrent object downloads during `grep`. `None` applies + /// [`DEFAULT_SEARCH_CONCURRENCY`]. Ignored when `search_enabled` is + /// `false`. + pub search_concurrency: Option, } impl S3BackendConfig { @@ -79,6 +145,11 @@ impl S3BackendConfig { prefix: prefix.into(), force_path_style: false, request_timeout: None, + max_read_bytes: None, + search_enabled: false, + max_objects_scanned: None, + max_grep_bytes_per_object: None, + search_concurrency: None, } } @@ -106,6 +177,43 @@ impl S3BackendConfig { self.request_timeout = Some(timeout); self } + + /// Override the per-read size ceiling. `0` is rejected at backend + /// construction time as a configuration mistake. See [`DEFAULT_MAX_READ_BYTES`]. + pub fn max_read_bytes(mut self, bytes: u64) -> Self { + self.max_read_bytes = Some(bytes); + self + } + + /// Enable degraded `grep` / `glob` against this S3 backend. Off by default. + /// See the documentation on [`Self::search_enabled`] for cost caveats. + pub fn enable_search(mut self, enabled: bool) -> Self { + self.search_enabled = enabled; + self + } + + /// Override the upper bound on objects considered per search. See + /// [`DEFAULT_MAX_OBJECTS_SCANNED`]. `0` is treated as the default at + /// backend construction. + pub fn max_objects_scanned(mut self, n: usize) -> Self { + self.max_objects_scanned = Some(n); + self + } + + /// Override the per-object body-size ceiling for `grep`. See + /// [`DEFAULT_MAX_GREP_BYTES_PER_OBJECT`]. `0` is treated as the default. + pub fn max_grep_bytes_per_object(mut self, bytes: u64) -> Self { + self.max_grep_bytes_per_object = Some(bytes); + self + } + + /// Override the per-search download concurrency. See + /// [`DEFAULT_SEARCH_CONCURRENCY`]. `0` resets to the default at backend + /// construction. + pub fn search_concurrency(mut self, n: usize) -> Self { + self.search_concurrency = Some(n); + self + } } /// S3-compatible workspace backend. @@ -120,6 +228,18 @@ pub struct S3WorkspaceBackend { /// Normalised prefix without trailing slash. Empty string means /// "bucket root is the workspace". prefix: String, + /// Per-read size ceiling (bytes). Enforced via `Content-Length` + /// inspection before the body is consumed. + max_read_bytes: u64, + /// When `true` the backend implements [`WorkspaceSearch`]; otherwise the + /// `grep` / `glob` tools are gated off by capability registration. + search_enabled: bool, + /// Upper bound on objects considered per search call. + max_objects_scanned: usize, + /// Per-object body-size ceiling for `grep` downloads. + max_grep_bytes_per_object: u64, + /// Concurrent object downloads during `grep`. + search_concurrency: usize, } impl S3WorkspaceBackend { @@ -147,6 +267,23 @@ impl S3WorkspaceBackend { let client = Client::from_conf(builder.build()); Self::with_client(client, config.bucket, config.prefix) + .with_max_read_bytes(config.max_read_bytes.unwrap_or(DEFAULT_MAX_READ_BYTES)) + .with_search_enabled(config.search_enabled) + .with_max_objects_scanned( + config + .max_objects_scanned + .unwrap_or(DEFAULT_MAX_OBJECTS_SCANNED), + ) + .with_max_grep_bytes_per_object( + config + .max_grep_bytes_per_object + .unwrap_or(DEFAULT_MAX_GREP_BYTES_PER_OBJECT), + ) + .with_search_concurrency( + config + .search_concurrency + .unwrap_or(DEFAULT_SEARCH_CONCURRENCY), + ) } /// Build a backend from a pre-configured S3 client. Intended for tests @@ -161,9 +298,88 @@ impl S3WorkspaceBackend { client, bucket: bucket.into(), prefix: normalize_prefix(&prefix.into()), + max_read_bytes: DEFAULT_MAX_READ_BYTES, + search_enabled: false, + max_objects_scanned: DEFAULT_MAX_OBJECTS_SCANNED, + max_grep_bytes_per_object: DEFAULT_MAX_GREP_BYTES_PER_OBJECT, + search_concurrency: DEFAULT_SEARCH_CONCURRENCY, } } + /// Override the per-read size ceiling. Passing `0` falls back to + /// [`DEFAULT_MAX_READ_BYTES`] — a zero ceiling would make every read + /// fail and is treated as a configuration mistake. + pub fn with_max_read_bytes(mut self, bytes: u64) -> Self { + self.max_read_bytes = if bytes == 0 { + DEFAULT_MAX_READ_BYTES + } else { + bytes + }; + self + } + + /// Active per-read size ceiling in bytes. + pub fn max_read_bytes(&self) -> u64 { + self.max_read_bytes + } + + /// Enable or disable degraded `grep` / `glob` against this backend. + /// See [`S3BackendConfig::search_enabled`] for cost trade-offs. + pub fn with_search_enabled(mut self, enabled: bool) -> Self { + self.search_enabled = enabled; + self + } + + /// Whether this backend exposes [`WorkspaceSearch`]. + pub fn search_enabled(&self) -> bool { + self.search_enabled + } + + /// Override the per-search object-scan ceiling. `0` resets to default. + pub fn with_max_objects_scanned(mut self, n: usize) -> Self { + self.max_objects_scanned = if n == 0 { + DEFAULT_MAX_OBJECTS_SCANNED + } else { + n + }; + self + } + + /// Active per-search object-scan ceiling. + pub fn max_objects_scanned(&self) -> usize { + self.max_objects_scanned + } + + /// Override the per-object body-size ceiling for `grep`. `0` resets to default. + pub fn with_max_grep_bytes_per_object(mut self, bytes: u64) -> Self { + self.max_grep_bytes_per_object = if bytes == 0 { + DEFAULT_MAX_GREP_BYTES_PER_OBJECT + } else { + bytes + }; + self + } + + /// Active per-object body-size ceiling for `grep` downloads. + pub fn max_grep_bytes_per_object(&self) -> u64 { + self.max_grep_bytes_per_object + } + + /// Override the per-search download concurrency. `0` resets to default. + pub fn with_search_concurrency(mut self, n: usize) -> Self { + self.search_concurrency = if n == 0 { + DEFAULT_SEARCH_CONCURRENCY + } else { + n + }; + self + } + + /// Active per-search download concurrency. + pub fn search_concurrency(&self) -> usize { + self.search_concurrency + } + /// The bucket this backend is bound to. pub fn bucket(&self) -> &str { &self.bucket @@ -203,20 +419,56 @@ impl S3WorkspaceBackend { format!("{}/{}/", self.prefix, path.as_str()) } } -} -#[async_trait] -impl WorkspaceFileSystem for S3WorkspaceBackend { - async fn read_text(&self, path: &WorkspacePath) -> Result { + /// Shared GET path used by both [`WorkspaceFileSystem::read_text`] and + /// [`WorkspaceFileSystemExt::read_text_with_version`]. + /// + /// Returns `(content, etag)`. The ETag is the opaque version token used + /// by compare-and-swap writes. Refuses responses without an ETag — every + /// S3-compatible service must return one for a successful GET; absence + /// indicates a misconfigured endpoint. + async fn get_object_text(&self, path: &WorkspacePath) -> WorkspaceResult<(String, String)> { let key = self.key_for(path); - let resp = self + let start = std::time::Instant::now(); + let send_result = self .client .get_object() .bucket(&self.bucket) .key(&key) .send() - .await - .map_err(|e| classify_get_error(&self.bucket, &key, e))?; + .await; + emit_s3_call_event( + "s3.get_object", + &self.bucket, + &key, + send_result + .as_ref() + .ok() + .and_then(|r| r.content_length()) + .unwrap_or(0) + .max(0) as u64, + send_result.is_ok(), + start.elapsed(), + ); + let resp = send_result.map_err(|e| classify_get_error(&self.bucket, &key, e))?; + + validate_content_length( + resp.content_length(), + self.max_read_bytes, + &self.bucket, + &key, + )?; + + let etag = resp + .e_tag() + .map(|s| s.to_string()) + .ok_or_else(|| { + anyhow!( + "S3 object s3://{}/{} returned no ETag; cannot use compare-and-swap writes against this endpoint", + self.bucket, + key + ) + })?; let bytes = resp .body @@ -232,40 +484,61 @@ impl WorkspaceFileSystem for S3WorkspaceBackend { })? .into_bytes(); - String::from_utf8(bytes.to_vec()).map_err(|e| { + let content = String::from_utf8(bytes.to_vec()).map_err(|e| { anyhow!( "S3 object s3://{}/{} is not valid UTF-8: {}", self.bucket, key, e ) - }) + })?; + + Ok((content, etag)) + } +} + +#[async_trait] +impl WorkspaceFileSystem for S3WorkspaceBackend { + async fn read_text(&self, path: &WorkspacePath) -> WorkspaceResult { + let (content, _etag) = self.get_object_text(path).await?; + Ok(content) } async fn write_text( &self, path: &WorkspacePath, content: &str, - ) -> Result { + ) -> WorkspaceResult { let key = self.key_for(path); let body = ByteStream::from(content.as_bytes().to_vec()); + let bytes = content.len() as u64; - self.client + let start = std::time::Instant::now(); + let send_result = self + .client .put_object() .bucket(&self.bucket) .key(&key) .body(body) .content_type("text/plain; charset=utf-8") .send() - .await - .map_err(|e| { - anyhow!( - "Failed to write S3 object s3://{}/{}: {}", - self.bucket, - key, - e - ) - })?; + .await; + emit_s3_call_event( + "s3.put_object", + &self.bucket, + &key, + bytes, + send_result.is_ok(), + start.elapsed(), + ); + send_result.map_err(|e| { + anyhow!( + "Failed to write S3 object s3://{}/{}: {}", + self.bucket, + key, + e + ) + })?; Ok(WorkspaceWriteOutcome { bytes: content.len(), @@ -273,9 +546,15 @@ impl WorkspaceFileSystem for S3WorkspaceBackend { }) } - async fn list_dir(&self, path: &WorkspacePath) -> Result> { + async fn list_dir(&self, path: &WorkspacePath) -> WorkspaceResult> { let prefix = self.list_prefix_for(path); let mut entries: Vec = Vec::new(); + // `total_listed` counts every Content/CommonPrefix the server returned + // including the prefix marker (the zero-byte "/" object some + // tools create to denote an empty directory). We use it to distinguish + // "prefix exists but has no children" from "prefix never existed" so + // `ls` on a missing path on S3 errors like it does on local FS. + let mut total_listed: usize = 0; let mut continuation: Option = None; loop { @@ -289,13 +568,23 @@ impl WorkspaceFileSystem for S3WorkspaceBackend { req = req.continuation_token(token); } - let resp = req - .send() - .await - .map_err(|e| classify_list_error(&self.bucket, &prefix, e))?; + let start = std::time::Instant::now(); + let send_result = req.send().await; + emit_s3_call_event( + "s3.list_objects_v2", + &self.bucket, + &prefix, + send_result.as_ref().ok().map_or(0, |r| { + r.contents().len() as u64 + r.common_prefixes().len() as u64 + }), + send_result.is_ok(), + start.elapsed(), + ); + let resp = send_result.map_err(|e| classify_list_error(&self.bucket, &prefix, e))?; // CommonPrefixes → directories for cp in resp.common_prefixes() { + total_listed += 1; if let Some(p) = cp.prefix() { // p looks like "/"; extract if let Some(name) = strip_dir_name(p, &prefix) { @@ -310,6 +599,7 @@ impl WorkspaceFileSystem for S3WorkspaceBackend { // Contents → files for obj in resp.contents() { + total_listed += 1; let Some(key) = obj.key() else { continue }; // Skip the prefix marker itself (key == prefix exactly). if key == prefix { @@ -334,10 +624,404 @@ impl WorkspaceFileSystem for S3WorkspaceBackend { } } + if !path.is_root() && total_listed == 0 { + return Err(WorkspaceError::NotFound { + path: format!("s3://{}/{}", self.bucket, prefix.trim_end_matches('/')), + }); + } + Ok(entries) } } +#[async_trait] +impl WorkspaceFileSystemExt for S3WorkspaceBackend { + async fn read_text_with_version( + &self, + path: &WorkspacePath, + ) -> WorkspaceResult<(String, String)> { + self.get_object_text(path).await + } + + async fn write_text_if_version( + &self, + path: &WorkspacePath, + content: &str, + expected_version: &str, + ) -> WorkspaceResult { + if expected_version.is_empty() { + return Err(WorkspaceError::InvalidArgument { + message: + "write_text_if_version requires a non-empty expected version (got empty); \ + use write_text for unconditional writes" + .to_string(), + }); + } + + let key = self.key_for(path); + let body = ByteStream::from(content.as_bytes().to_vec()); + let bytes = content.len() as u64; + + let start = std::time::Instant::now(); + let send_result = self + .client + .put_object() + .bucket(&self.bucket) + .key(&key) + .if_match(expected_version) + .body(body) + .content_type("text/plain; charset=utf-8") + .send() + .await; + emit_s3_call_event( + "s3.put_object_if_match", + &self.bucket, + &key, + bytes, + send_result.is_ok(), + start.elapsed(), + ); + + match send_result { + Ok(_) => Ok(WorkspaceWriteOutcome { + bytes: content.len(), + lines: content.lines().count(), + }), + Err(e) => Err(map_put_error(&self.bucket, &key, expected_version, e)), + } + } +} + +impl S3WorkspaceBackend { + /// Recursive (no-delimiter) listing of objects under `base`, with a hard + /// cap on the number of objects considered. + /// + /// Returns `(entries, truncated)` where `entries` holds `(relative_key, + /// size_bytes)` tuples relative to `base`'s S3 prefix, and `truncated` is + /// `true` when the cap was reached before the listing completed. The + /// listing-prefix marker itself is filtered out. + /// + /// Used as the foundation for both [`WorkspaceSearch::glob`] and + /// [`WorkspaceSearch::grep`]. Always paginates through continuation + /// tokens to avoid silently dropping objects past the first page. + async fn list_recursive_under( + &self, + base: &WorkspacePath, + max_objects: usize, + ) -> Result<(Vec<(String, u64)>, bool)> { + let prefix = self.list_prefix_for(base); + let mut entries: Vec<(String, u64)> = Vec::new(); + let mut continuation: Option = None; + let mut truncated = false; + + loop { + let mut req = self + .client + .list_objects_v2() + .bucket(&self.bucket) + .prefix(&prefix); + if let Some(t) = continuation.as_ref() { + req = req.continuation_token(t); + } + let start = std::time::Instant::now(); + let send_result = req.send().await; + emit_s3_call_event( + "s3.list_objects_v2_recursive", + &self.bucket, + &prefix, + send_result + .as_ref() + .ok() + .map_or(0, |r| r.contents().len() as u64), + send_result.is_ok(), + start.elapsed(), + ); + let resp = send_result.map_err(|e| classify_list_error(&self.bucket, &prefix, e))?; + + for obj in resp.contents() { + if entries.len() >= max_objects { + truncated = true; + return Ok((entries, truncated)); + } + let Some(key) = obj.key() else { continue }; + if key == prefix { + continue; + } + let Some(rel) = key.strip_prefix(&prefix) else { + continue; + }; + if rel.is_empty() { + continue; + } + let size = obj.size().unwrap_or(0).max(0) as u64; + entries.push((rel.to_string(), size)); + } + + if resp.is_truncated().unwrap_or(false) { + continuation = resp.next_continuation_token().map(|s| s.to_string()); + if continuation.is_none() { + break; + } + } else { + break; + } + } + + Ok((entries, truncated)) + } +} + +#[async_trait] +impl WorkspaceSearch for S3WorkspaceBackend { + async fn glob(&self, request: WorkspaceGlobRequest) -> Result { + validate_relative_pattern(&request.pattern, "glob pattern")?; + let pattern = glob::Pattern::new(&request.pattern) + .map_err(|e| anyhow!("Invalid glob pattern '{}': {}", request.pattern, e))?; + // The `glob` crate's `Pattern::matches` is more permissive than the + // filesystem walker behind `glob::glob` — `*` happily matches across + // `/`. To stay consistent with the local backend (where `*.rs` does + // NOT recurse into subdirectories), require an explicit `**` for + // tree-wide matches; otherwise skip any key containing `/`. + let recursive = request.pattern.contains("**"); + + let (entries, scan_truncated) = self + .list_recursive_under(&request.base, self.max_objects_scanned) + .await?; + if scan_truncated { + tracing::debug!( + "S3 glob scan truncated at {} objects under s3://{}/{}", + self.max_objects_scanned, + self.bucket, + self.list_prefix_for(&request.base) + ); + } + + let mut matches = Vec::new(); + for (rel, _size) in entries { + if !recursive && rel.contains('/') { + continue; + } + if pattern.matches(&rel) { + matches.push(join_workspace_path(&request.base, &rel)); + } + } + matches.sort_by(|a, b| a.as_str().cmp(b.as_str())); + Ok(WorkspaceGlobResult { matches }) + } + + async fn grep(&self, request: WorkspaceGrepRequest) -> Result { + use futures::stream::StreamExt; + + if let Some(ref g) = request.glob { + validate_relative_pattern(g, "grep glob filter")?; + } + + let regex_pattern = if request.case_insensitive { + format!("(?i){}", request.pattern) + } else { + request.pattern.clone() + }; + let regex = std::sync::Arc::new( + regex::Regex::new(®ex_pattern) + .map_err(|e| anyhow!("Invalid regex pattern '{}': {}", request.pattern, e))?, + ); + + let glob_filter = match request.glob.as_deref() { + Some(g) => Some(( + glob::Pattern::new(g) + .map_err(|e| anyhow!("Invalid grep glob filter '{}': {}", g, e))?, + g.contains('/'), + )), + None => None, + }; + + let (entries, scan_truncated) = self + .list_recursive_under(&request.base, self.max_objects_scanned) + .await?; + + // Phase 1 — sequentially filter the listing (cheap; no I/O). We + // produce a list of objects that pass the glob filter and the + // per-object size cap. Oversized objects are skipped here, not + // downloaded. + let listing_prefix = self.list_prefix_for(&request.base); + let candidates: Vec<(WorkspacePath, String)> = entries + .into_iter() + .filter_map(|(rel, size)| { + if let Some((ref pat, has_sep)) = glob_filter { + let target = if has_sep { + rel.as_str() + } else { + basename(&rel) + }; + if !pat.matches(target) { + return None; + } + } + if size > self.max_grep_bytes_per_object { + tracing::debug!( + "Skipping S3 object {}{} ({} bytes > grep cap {})", + listing_prefix, + rel, + size, + self.max_grep_bytes_per_object + ); + return None; + } + let ws_path = join_workspace_path(&request.base, &rel); + let display_str = ws_path.as_str().to_string(); + Some((ws_path, display_str)) + }) + .collect(); + + // Phase 2 — fetch objects concurrently and run the regex per file. + // Output is *not* assembled here; that needs deterministic ordering + // (Phase 3) and global truncation accounting, so we just collect + // per-file matches. + type FileMatch = (String, Vec, Vec); + let regex_for_stream = std::sync::Arc::clone(®ex); + let listing_prefix_for_stream = listing_prefix.clone(); + let per_file: Vec> = futures::stream::iter(candidates.into_iter()) + .map(|(ws_path, display_str)| { + let regex = std::sync::Arc::clone(®ex_for_stream); + let listing_prefix = listing_prefix_for_stream.clone(); + async move { + let content = match self.read_text(&ws_path).await { + Ok(c) => c, + Err(e) => { + tracing::debug!( + "Skipping S3 object {}{}: {}", + listing_prefix, + ws_path.as_str(), + e + ); + return None; + } + }; + let lines: Vec = content.lines().map(|s| s.to_string()).collect(); + let mut file_matches: Vec = Vec::new(); + for (idx, line) in lines.iter().enumerate() { + if regex.is_match(line) { + file_matches.push(idx); + } + } + if file_matches.is_empty() { + None + } else { + Some((display_str, lines, file_matches)) + } + } + }) + .buffer_unordered(self.search_concurrency.max(1)) + .collect() + .await; + + // Phase 3 — sort by display path for deterministic output across + // runs (concurrent completion order is otherwise nondeterministic), + // then walk the collected matches and accumulate output until + // `max_output_size` is hit. + let mut hits: Vec = per_file.into_iter().flatten().collect(); + hits.sort_by(|a, b| a.0.cmp(&b.0)); + + let mut output = String::new(); + let mut match_count = 0usize; + let mut file_count = 0usize; + let mut total_size = 0usize; + let mut output_truncated = false; + + 'outer: for (display_str, lines, file_matches) in hits { + file_count += 1; + for &match_idx in &file_matches { + if total_size > request.max_output_size { + output_truncated = true; + break 'outer; + } + match_count += 1; + + let start = match_idx.saturating_sub(request.context_lines); + let end = (match_idx + request.context_lines + 1).min(lines.len()); + for (i, line) in lines[start..end].iter().enumerate() { + let abs_i = start + i; + let prefix = if abs_i == match_idx { ">" } else { " " }; + let line = format!("{}{}:{}: {}\n", prefix, display_str, abs_i + 1, line); + total_size += line.len(); + output.push_str(&line); + } + if request.context_lines > 0 { + output.push_str("--\n"); + total_size += 3; + } + } + } + + Ok(WorkspaceGrepResult { + output, + match_count, + file_count, + truncated: output_truncated || scan_truncated, + }) + } +} + +/// Join `base` and a key relative to its S3 prefix into a workspace-relative +/// [`WorkspacePath`]. Handles the "base is root" case so the result does not +/// start with `./`. +fn join_workspace_path(base: &WorkspacePath, rel: &str) -> WorkspacePath { + if base.is_root() { + WorkspacePath::from_normalized(rel) + } else { + WorkspacePath::from_normalized(format!("{}/{}", base.as_str(), rel)) + } +} + +/// Last segment of a slash-separated key, used to apply filename-only glob +/// filters in `grep` (matches `ignore::types` semantics from the local +/// backend: a pattern without `/` is treated as a basename match). +fn basename(rel: &str) -> &str { + rel.rsplit_once('/').map_or(rel, |(_, b)| b) +} + +/// Decide whether a `GetObject` response is safe to buffer fully into memory. +/// +/// Returns `Ok(())` when the advertised `Content-Length` is non-negative and +/// within `max_bytes`. Rejects in three cases: +/// * `Content-Length` was not advertised (we refuse to read without a size +/// guard rather than risking OOM); +/// * the advertised length is negative (protocol violation); +/// * the advertised length exceeds `max_bytes`. +/// +/// Extracted as a free function so it can be unit-tested without standing up +/// an `aws_sdk_s3::Client`. +fn validate_content_length( + advertised: Option, + max_bytes: u64, + bucket: &str, + key: &str, +) -> Result<()> { + match advertised { + Some(n) if n < 0 => Err(anyhow!( + "S3 object s3://{}/{} reported invalid content-length {}", + bucket, + key, + n + )), + Some(n) if (n as u64) > max_bytes => Err(anyhow!( + "S3 object s3://{}/{} is {} bytes, exceeds workspace max_read_bytes ({}); \ + raise S3BackendConfig::max_read_bytes if the read is legitimate", + bucket, + key, + n, + max_bytes + )), + Some(_) => Ok(()), + None => Err(anyhow!( + "S3 object s3://{}/{} did not report Content-Length; refusing to read \ + without a size guard. Check that the endpoint is S3-compliant.", + bucket, + key + )), + } +} + fn normalize_prefix(prefix: &str) -> String { prefix .trim_start_matches('/') @@ -364,7 +1048,7 @@ fn strip_file_name(key: &str, listing_prefix: &str) -> Option { } } -fn classify_get_error(bucket: &str, key: &str, error: SdkError) -> anyhow::Error +fn classify_get_error(bucket: &str, key: &str, error: SdkError) -> WorkspaceError where E: std::error::Error + Send + Sync + 'static, { @@ -373,14 +1057,16 @@ where .map(|r| r.status().as_u16()) .unwrap_or_default(); if raw == 404 { - anyhow!("S3 object not found: s3://{}/{}", bucket, key) + WorkspaceError::NotFound { + path: format!("s3://{}/{}", bucket, key), + } } else { - anyhow!( + WorkspaceError::Backend(anyhow!( "Failed to read S3 object s3://{}/{}: {}", bucket, key, error - ) + )) } } @@ -388,25 +1074,91 @@ fn classify_list_error( bucket: &str, prefix: &str, error: SdkError, -) -> anyhow::Error { - anyhow!( +) -> WorkspaceError { + WorkspaceError::Backend(anyhow!( "Failed to list S3 prefix s3://{}/{}: {}", bucket, prefix, error - ) + )) +} + +/// Emit a structured `tracing` event for a single S3 API call. +/// +/// Hosts that want to meter S3 cost (call count, bytes transferred, latency) +/// can subscribe to events from this module at `DEBUG` level and route on +/// the `op` field. Fields emitted: +/// +/// | Field | Type | Meaning | +/// |----------------|---------|-----------------------------------------------------------| +/// | `op` | string | S3 operation (e.g. `s3.get_object`, `s3.list_objects_v2`) | +/// | `bucket` | string | Bucket name | +/// | `target` | string | Key (GET/PUT) or listing prefix (LIST) | +/// | `bytes` | u64 | Body bytes for GET/PUT; entries returned for LIST | +/// | `outcome` | string | `ok` or `error` | +/// | `duration_ms` | u64 | Wall-clock duration | +/// +/// Emitted at `DEBUG`; zero-cost when the level is disabled. +fn emit_s3_call_event( + op: &'static str, + bucket: &str, + target: &str, + bytes: u64, + ok: bool, + elapsed: std::time::Duration, +) { + tracing::debug!( + op = op, + bucket = %bucket, + target = %target, + bytes = bytes, + outcome = if ok { "ok" } else { "error" }, + duration_ms = elapsed.as_millis() as u64, + ); +} + +/// Map a `PutObject` failure to either a [`WorkspaceVersionConflict`] +/// (HTTP 412 Precondition Failed from `If-Match`) or a generic write error. +/// +/// AWS S3 does not return the current ETag on 412 so [`WorkspaceVersionConflict::actual`] +/// is left `None`; callers that need the current version must re-read. +fn map_put_error( + bucket: &str, + key: &str, + expected_version: &str, + error: SdkError, +) -> WorkspaceError { + let status = error + .raw_response() + .map(|r| r.status().as_u16()) + .unwrap_or_default(); + if status == 412 { + WorkspaceError::VersionConflict(WorkspaceVersionConflict { + path: format!("s3://{}/{}", bucket, key), + expected: expected_version.to_string(), + actual: None, + }) + } else { + WorkspaceError::Backend(anyhow!( + "Failed to write S3 object s3://{}/{}: {}", + bucket, + key, + error + )) + } } impl super::WorkspaceServices { /// Build a workspace whose files live in an S3-compatible bucket. /// - /// The resulting [`WorkspaceServices`](super::WorkspaceServices) exposes - /// only read / write / list capabilities (`read`, `write`, `edit`, - /// `patch`, `ls`); `bash`, `git`, `grep`, and `glob` are intentionally - /// not registered because object storage cannot service them. A 60s - /// per-operation timeout is applied by default — override via - /// [`super::WorkspaceServicesBuilder::operation_timeout`] when building - /// manually. + /// By default the resulting [`WorkspaceServices`](super::WorkspaceServices) + /// exposes only read / write / list capabilities (`read`, `write`, + /// `edit`, `patch`, `ls`); `bash` and `git` are never registered (object + /// storage cannot service them), and `grep` / `glob` are registered only + /// when [`S3BackendConfig::search_enabled`] is set — see that field for + /// cost trade-offs. A 60s per-operation timeout is applied by default; + /// override via [`super::WorkspaceServicesBuilder::operation_timeout`] + /// when building manually. pub fn s3(config: S3BackendConfig) -> Arc { let backend = Arc::new(S3WorkspaceBackend::new(config)); Self::from_s3_backend(backend) @@ -417,15 +1169,30 @@ impl super::WorkspaceServices { /// Useful when the caller has injected a custom AWS client (e.g. a mocked /// HTTP layer, alternative credential provider, or a wrapper that adds /// metrics / tracing). + /// + /// The backend is wired both as the `WorkspaceFileSystem` and the + /// optional `WorkspaceFileSystemExt`, so tools that perform + /// read-modify-write cycles (`edit`, `patch`) get compare-and-swap + /// semantics via ETag automatically. When `search_enabled` is set on the + /// backend, the `grep` / `glob` tools are also registered and constrained + /// by `max_objects_scanned` / `max_grep_bytes_per_object`; otherwise + /// capability gating keeps them hidden from the model. pub fn from_s3_backend(backend: Arc) -> Arc { let workspace_ref = super::WorkspaceRef::new( format!("s3://{}/{}", backend.bucket(), backend.prefix()), format!("s3://{}/{}", backend.bucket(), backend.prefix()), ); - let fs: Arc = backend; - Self::builder(workspace_ref, fs) - .operation_timeout(Duration::from_secs(60)) - .build() + let search_capable = backend.search_enabled(); + let fs: Arc = backend.clone(); + let fs_ext: Arc = backend.clone(); + let mut builder = Self::builder(workspace_ref, fs) + .file_system_ext(fs_ext) + .operation_timeout(Duration::from_secs(60)); + if search_capable { + let search: Arc = backend; + builder = builder.search(search); + } + builder.build() } } @@ -512,7 +1279,8 @@ mod tests { .region("cn-east-1") .session_token("TOKEN") .force_path_style(true) - .request_timeout(Duration::from_secs(5)); + .request_timeout(Duration::from_secs(5)) + .max_read_bytes(4096); assert_eq!(cfg.bucket, "bucket"); assert_eq!(cfg.prefix, "prefix"); assert_eq!(cfg.endpoint.as_deref(), Some("https://minio.local:9000")); @@ -520,10 +1288,68 @@ mod tests { assert_eq!(cfg.session_token.as_deref(), Some("TOKEN")); assert!(cfg.force_path_style); assert_eq!(cfg.request_timeout, Some(Duration::from_secs(5))); + assert_eq!(cfg.max_read_bytes, Some(4096)); + } + + #[test] + fn config_default_max_read_bytes_is_none_until_set() { + let cfg = S3BackendConfig::new("bucket", "prefix", "AK", "SK"); + assert!(cfg.max_read_bytes.is_none()); + } + + #[test] + fn backend_applies_default_max_read_bytes_when_config_omits_it() { + let cfg = S3BackendConfig::new("bucket", "ws", "AK", "SK"); + let backend = S3WorkspaceBackend::new(cfg); + assert_eq!(backend.max_read_bytes(), DEFAULT_MAX_READ_BYTES); + } + + #[test] + fn backend_respects_config_max_read_bytes_override() { + let cfg = S3BackendConfig::new("bucket", "ws", "AK", "SK").max_read_bytes(2048); + let backend = S3WorkspaceBackend::new(cfg); + assert_eq!(backend.max_read_bytes(), 2048); + } + + #[test] + fn backend_treats_zero_max_read_bytes_as_default() { + let cfg = S3BackendConfig::new("bucket", "ws", "AK", "SK").max_read_bytes(0); + let backend = S3WorkspaceBackend::new(cfg); + assert_eq!(backend.max_read_bytes(), DEFAULT_MAX_READ_BYTES); + } + + #[test] + fn validate_content_length_allows_within_cap() { + assert!(validate_content_length(Some(1024), 4096, "bucket", "key").is_ok()); + assert!(validate_content_length(Some(0), 4096, "bucket", "key").is_ok()); + assert!(validate_content_length(Some(4096), 4096, "bucket", "key").is_ok()); + } + + #[test] + fn validate_content_length_rejects_over_cap() { + let err = validate_content_length(Some(4097), 4096, "bucket", "ws/big.txt").unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("exceeds workspace max_read_bytes"), + "msg: {msg}" + ); + assert!(msg.contains("s3://bucket/ws/big.txt"), "msg: {msg}"); } #[test] - fn services_s3_factory_disables_exec_search_and_git() { + fn validate_content_length_rejects_missing_header() { + let err = validate_content_length(None, 4096, "bucket", "ws/key").unwrap_err(); + assert!(err.to_string().contains("did not report Content-Length")); + } + + #[test] + fn validate_content_length_rejects_negative_length() { + let err = validate_content_length(Some(-1), 4096, "bucket", "ws/key").unwrap_err(); + assert!(err.to_string().contains("invalid content-length")); + } + + #[test] + fn services_s3_factory_disables_exec_search_and_git_by_default() { let cfg = S3BackendConfig::new("bucket", "ws", "AK", "SK"); let services = super::super::WorkspaceServices::s3(cfg); let caps = services.capabilities(); @@ -540,6 +1366,123 @@ mod tests { assert_eq!(services.operation_timeout(), Some(Duration::from_secs(60))); } + #[test] + fn services_s3_factory_registers_search_when_enabled() { + let cfg = S3BackendConfig::new("bucket", "ws", "AK", "SK").enable_search(true); + let services = super::super::WorkspaceServices::s3(cfg); + let caps = services.capabilities(); + assert!(caps.search, "search capability must be on when enabled"); + assert!( + services.search().is_some(), + "search provider must be wired when enabled" + ); + // Disabled providers stay None — opt-in is per-capability, not all-or-nothing. + assert!(!caps.exec); + assert!(!caps.git); + } + + #[test] + fn config_search_defaults_off_until_enabled() { + let cfg = S3BackendConfig::new("bucket", "ws", "AK", "SK"); + assert!(!cfg.search_enabled); + assert!(cfg.max_objects_scanned.is_none()); + assert!(cfg.max_grep_bytes_per_object.is_none()); + + let cfg = cfg + .enable_search(true) + .max_objects_scanned(50) + .max_grep_bytes_per_object(256 * 1024); + assert!(cfg.search_enabled); + assert_eq!(cfg.max_objects_scanned, Some(50)); + assert_eq!(cfg.max_grep_bytes_per_object, Some(256 * 1024)); + } + + #[test] + fn backend_applies_search_defaults_when_config_omits_them() { + let cfg = S3BackendConfig::new("bucket", "ws", "AK", "SK").enable_search(true); + let backend = S3WorkspaceBackend::new(cfg); + assert!(backend.search_enabled()); + assert_eq!(backend.max_objects_scanned(), DEFAULT_MAX_OBJECTS_SCANNED); + assert_eq!( + backend.max_grep_bytes_per_object(), + DEFAULT_MAX_GREP_BYTES_PER_OBJECT + ); + } + + #[test] + fn backend_treats_zero_search_limits_as_defaults() { + let cfg = S3BackendConfig::new("bucket", "ws", "AK", "SK") + .enable_search(true) + .max_objects_scanned(0) + .max_grep_bytes_per_object(0) + .search_concurrency(0); + let backend = S3WorkspaceBackend::new(cfg); + assert_eq!(backend.max_objects_scanned(), DEFAULT_MAX_OBJECTS_SCANNED); + assert_eq!( + backend.max_grep_bytes_per_object(), + DEFAULT_MAX_GREP_BYTES_PER_OBJECT + ); + assert_eq!(backend.search_concurrency(), DEFAULT_SEARCH_CONCURRENCY); + } + + #[test] + fn backend_applies_search_concurrency_default() { + let cfg = S3BackendConfig::new("bucket", "ws", "AK", "SK").enable_search(true); + let backend = S3WorkspaceBackend::new(cfg); + assert_eq!(backend.search_concurrency(), DEFAULT_SEARCH_CONCURRENCY); + } + + #[test] + fn backend_respects_search_concurrency_override() { + let cfg = S3BackendConfig::new("bucket", "ws", "AK", "SK") + .enable_search(true) + .search_concurrency(16); + let backend = S3WorkspaceBackend::new(cfg); + assert_eq!(backend.search_concurrency(), 16); + } + + #[test] + fn join_workspace_path_handles_root_and_nested_bases() { + let root = WorkspacePath::root(); + let joined = join_workspace_path(&root, "main.rs"); + assert_eq!(joined.as_str(), "main.rs"); + + let base = WorkspacePath::from_normalized("src"); + let joined = join_workspace_path(&base, "foo/main.rs"); + assert_eq!(joined.as_str(), "src/foo/main.rs"); + } + + #[test] + fn basename_returns_last_segment() { + assert_eq!(basename("src/main.rs"), "main.rs"); + assert_eq!(basename("main.rs"), "main.rs"); + assert_eq!(basename("a/b/c/d.txt"), "d.txt"); + } + + /// Documents the `glob` crate behaviour the S3 search impl works around. + /// + /// `glob::Pattern::matches` is more permissive than the filesystem walker + /// behind `glob::glob`: `*` *does* match across `/`, so `*.rs` matches + /// both `main.rs` and `src/main.rs`. The local backend gets non-recursive + /// semantics for free from the walker; on S3 we have to filter explicitly + /// when the user did not write `**`. This test pins the assumption so a + /// future `glob` crate upgrade with stricter semantics surfaces here + /// rather than silently changing user-visible behaviour. + #[test] + fn glob_pattern_matches_is_permissive_across_slashes() { + let permissive = glob::Pattern::new("*.rs").unwrap(); + assert!(permissive.matches("main.rs")); + assert!( + permissive.matches("src/main.rs"), + "`glob` crate's `*` matches across `/`; if this ever changes, drop \ + the manual `rel.contains('/')` guard in WorkspaceSearch::glob" + ); + + let recursive = glob::Pattern::new("**/*.rs").unwrap(); + assert!(recursive.matches("src/main.rs")); + assert!(recursive.matches("main.rs")); + } + fn make_backend(prefix: &str) -> S3WorkspaceBackend { let cfg = S3BackendConfig::new("bucket", prefix, "AK", "SK"); S3WorkspaceBackend::new(cfg) diff --git a/core/tests/test_workspace_backend.rs b/core/tests/test_workspace_backend.rs index 0bc95e7..9853282 100644 --- a/core/tests/test_workspace_backend.rs +++ b/core/tests/test_workspace_backend.rs @@ -1,14 +1,14 @@ use a3s_code_core::tools::{ArtifactStoreLimits, ToolExecutor}; use a3s_code_core::{ - CommandOutput, CommandRequest, WorkspaceCommandRunner, WorkspaceDirEntry, WorkspaceFileSystem, - WorkspaceFileType, WorkspaceGit, WorkspaceGitBranch, WorkspaceGitCheckoutOutput, - WorkspaceGitCheckoutRequest, WorkspaceGitCommit, WorkspaceGitCreateBranchRequest, - WorkspaceGitCreateWorktreeRequest, WorkspaceGitDiffRequest, WorkspaceGitRemote, - WorkspaceGitRemoveWorktreeRequest, WorkspaceGitStash, WorkspaceGitStashProvider, - WorkspaceGitStashRequest, WorkspaceGitStatus, WorkspaceGitWorktree, + CommandOutput, CommandRequest, WorkspaceCommandRunner, WorkspaceDirEntry, WorkspaceError, + WorkspaceFileSystem, WorkspaceFileType, WorkspaceGit, WorkspaceGitBranch, + WorkspaceGitCheckoutOutput, WorkspaceGitCheckoutRequest, WorkspaceGitCommit, + WorkspaceGitCreateBranchRequest, WorkspaceGitCreateWorktreeRequest, WorkspaceGitDiffRequest, + WorkspaceGitRemote, WorkspaceGitRemoveWorktreeRequest, WorkspaceGitStash, + WorkspaceGitStashProvider, WorkspaceGitStashRequest, WorkspaceGitStatus, WorkspaceGitWorktree, WorkspaceGitWorktreeMutation, WorkspaceGitWorktreeProvider, WorkspaceGlobRequest, WorkspaceGlobResult, WorkspaceGrepRequest, WorkspaceGrepResult, WorkspacePath, WorkspaceRef, - WorkspaceSearch, WorkspaceServices, WorkspaceWriteOutcome, + WorkspaceResult, WorkspaceSearch, WorkspaceServices, WorkspaceWriteOutcome, }; use anyhow::{anyhow, Result}; use async_trait::async_trait; @@ -36,20 +36,22 @@ impl MemoryWorkspace { #[async_trait] impl WorkspaceFileSystem for MemoryWorkspace { - async fn read_text(&self, path: &WorkspacePath) -> Result { + async fn read_text(&self, path: &WorkspacePath) -> WorkspaceResult { self.files .read() .unwrap() .get(path.as_str()) .cloned() - .ok_or_else(|| anyhow!("missing remote file: {}", path.as_str())) + .ok_or_else(|| WorkspaceError::NotFound { + path: path.as_str().to_string(), + }) } async fn write_text( &self, path: &WorkspacePath, content: &str, - ) -> Result { + ) -> WorkspaceResult { self.insert(path.as_str(), content); Ok(WorkspaceWriteOutcome { bytes: content.len(), @@ -57,7 +59,7 @@ impl WorkspaceFileSystem for MemoryWorkspace { }) } - async fn list_dir(&self, path: &WorkspacePath) -> Result> { + async fn list_dir(&self, path: &WorkspacePath) -> WorkspaceResult> { let prefix = if path.is_root() { String::new() } else { diff --git a/sdk/node/Cargo.toml b/sdk/node/Cargo.toml index 91cc099..7f65ac5 100644 --- a/sdk/node/Cargo.toml +++ b/sdk/node/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "a3s-code-node" -version = "2.6.0" +version = "3.0.0" edition = "2021" authors = ["A3S Lab Team"] license = "MIT" @@ -11,7 +11,7 @@ description = "A3S Code Node.js bindings - Native addon via napi-rs" crate-type = ["cdylib"] [dependencies] -a3s-code-core = { version = "2.6.0", path = "../../core", features = ["ahp", "s3"] } +a3s-code-core = { version = "3.0.0", path = "../../core", features = ["ahp", "s3"] } napi = { version = "2", features = ["async", "napi6", "serde-json"] } napi-derive = "2" tokio = { version = "1.35", features = ["full"] } diff --git a/sdk/node/examples/package-lock.json b/sdk/node/examples/package-lock.json index 7b55c68..8adcf9c 100644 --- a/sdk/node/examples/package-lock.json +++ b/sdk/node/examples/package-lock.json @@ -18,7 +18,7 @@ }, "..": { "name": "@a3s-lab/code", - "version": "2.6.0", + "version": "3.0.0", "license": "MIT", "devDependencies": { "@napi-rs/cli": "^2", @@ -27,12 +27,12 @@ "typescript": "^5.9.3" }, "optionalDependencies": { - "@a3s-lab/code-darwin-arm64": "2.6.0", - "@a3s-lab/code-linux-arm64-gnu": "2.6.0", - "@a3s-lab/code-linux-arm64-musl": "2.6.0", - "@a3s-lab/code-linux-x64-gnu": "2.6.0", - "@a3s-lab/code-linux-x64-musl": "2.6.0", - "@a3s-lab/code-win32-x64-msvc": "2.6.0" + "@a3s-lab/code-darwin-arm64": "3.0.0", + "@a3s-lab/code-linux-arm64-gnu": "3.0.0", + "@a3s-lab/code-linux-arm64-musl": "3.0.0", + "@a3s-lab/code-linux-x64-gnu": "3.0.0", + "@a3s-lab/code-linux-x64-musl": "3.0.0", + "@a3s-lab/code-win32-x64-msvc": "3.0.0" } }, "node_modules/@a3s-lab/code": { diff --git a/sdk/node/index.d.ts b/sdk/node/index.d.ts index 5d8c1d8..c730ba6 100644 --- a/sdk/node/index.d.ts +++ b/sdk/node/index.d.ts @@ -71,6 +71,14 @@ export interface AgentEvent { verificationSummaryText?: string /** Extra data for events that don't map to standard fields (JSON-encoded) */ data?: string + /** + * Structured discriminant for tool failures on `tool_end` events + * (JSON-encoded with a `type` field on the top level, e.g. + * `{"type":"version_conflict","path":"doc.md","expected":"etag-1","actual":"etag-2"}`). + * Undefined on success or untyped failure. Streaming consumers parse + * this to branch on the failure kind without scanning `toolOutput`. + */ + errorKindJson?: string } export interface VerificationCommand { id: string @@ -88,7 +96,32 @@ export interface ToolResult { metadataJson?: string /** Convenience JSON view of `metadata.document_runtime` when present. */ documentRuntimeJson?: string + /** + * Structured discriminant for tool failures, JSON-encoded with a + * `type` field on the top level — e.g. + * `{"type":"version_conflict","path":"doc.md","expected":"etag-1","actual":"etag-2"}`. + * Undefined on success or untyped failure. SDK callers parse it to + * branch on the failure kind without scanning the `output` string. + */ + errorKindJson?: string } + +/** + * Parsed shape of `ToolResult.errorKindJson` / `AgentEvent.errorKindJson`. + * + * Use a discriminated union on the `type` field; new variants may be + * added in future minor releases — callers should match exhaustively on + * the kinds they care about and fall through to a default branch for + * unknown ones. + */ +export type ToolErrorKind = + | { type: 'version_conflict'; path: string; expected: string; actual: string | null } + | { type: 'remote_git_conflict'; code: string; message: string } + | { type: 'not_found'; path: string } + | { type: 'invalid_argument'; message: string } + | { type: 'unsupported'; message: string } + | { type: 'timeout'; op: string; duration_ms: number } + /** Execution limits for `Session.program`. */ export interface ProgramScriptLimits { timeoutMs?: number @@ -212,6 +245,79 @@ export interface JsS3BackendConfig { prefix: string /** `true` for MinIO / RustFS / most non-AWS endpoints; `false` for AWS S3. */ forcePathStyle?: boolean + /** + * Maximum bytes a single `read` may return. The backend rejects any + * response with `Content-Length` greater than this without buffering + * the body. Defaults to 10 MiB on the Rust side when omitted. + */ + maxReadBytes?: number + /** + * Enable degraded `grep` / `glob` against this S3 backend. Off by + * default — object storage has no native search, so the only viable + * strategy is `LIST` + `GET` + regex, which can be slow and expensive. + */ + searchEnabled?: boolean + /** + * Upper bound on objects considered per `grep` / `glob` call. Defaults + * to 500 on the Rust side. Ignored when `searchEnabled` is `false`. + */ + maxObjectsScanned?: number + /** + * Per-object body-size ceiling for `grep` downloads. Larger objects are + * skipped (debug-traced). Defaults to 1 MiB on the Rust side. Ignored + * when `searchEnabled` is `false`. + */ + maxGrepBytesPerObject?: number + /** + * Concurrent object downloads during `grep`. Defaults to 8 on the Rust + * side. Set lower when the gitserver / S3 endpoint rate-limits + * aggressively; set higher when latency dominates. Ignored when + * `searchEnabled` is `false`. + */ + searchConcurrency?: number +} +/** + * Configuration for a `RemoteGitBackend` — an HTTP/JSON client that + * brings the `git` tool to non-local workspaces (S3, future container / + * DFS). + * + * Pass alongside `workspaceBackend` on a session to attach remote git + * on top of any filesystem backend. + */ +export interface JsRemoteGitBackendConfig { + /** + * Base URL of the gitserver, no trailing slash. The client builds + * `{baseUrl}/v1/repos/{repoId}/git/{op}` per the RFC. + */ + baseUrl: string + /** + * Opaque repository identifier, URL-safe. Negotiated out of band + * with the gitserver operator. + */ + repoId: string + /** + * Bearer token sent as `Authorization: Bearer `. Required in + * production; omitting it emits a server-side warning and is only safe + * on a trusted localhost gitserver. + */ + bearerToken?: string + /** + * mTLS client certificate path (PEM). When set together with `clientKeyPem`, + * the backend reads both files at construction and configures mTLS on the + * HTTP client. Setting only one of the pair errors at construction. + */ + clientCertPem?: string + /** + * mTLS client private key path (PEM). PKCS#8 format expected for the + * `rustls-tls` backend. See `clientCertPem`. + */ + clientKeyPem?: string + /** Per-call HTTP timeout in milliseconds. Defaults to 30 000. */ + requestTimeoutMs?: number + /** Client-side cap on `diff` response bytes. Defaults to 1 MiB. */ + maxDiffBytes?: number + /** Client-side cap on `log` `max_count`. Defaults to 200. */ + maxLogEntries?: number } /** * Union type for AHP transport configuration. @@ -386,6 +492,23 @@ export interface SessionOptions { * ``` */ workspaceBackend?: JsWorkspaceBackend + /** + * Optional remote git provider. When set, the resulting session attaches + * a `RemoteGitBackend` on top of `workspaceBackend` so the built-in + * `git` tool is available even on object-storage workspaces. + * + * ```js + * agent.session('s3://workspace/u1/s1', { + * workspaceBackend: new S3WorkspaceBackend({ ... }), + * remoteGit: { + * baseUrl: 'https://gitserver.internal', + * repoId: 'u1/s1', + * bearerToken: token, + * }, + * }); + * ``` + */ + remoteGit?: JsRemoteGitBackendConfig /** * Custom role/identity prepended before the core agentic prompt. * Example: "You are a senior Python developer specializing in FastAPI." diff --git a/sdk/node/package-lock.json b/sdk/node/package-lock.json index cd1540f..9343ff3 100644 --- a/sdk/node/package-lock.json +++ b/sdk/node/package-lock.json @@ -1,12 +1,12 @@ { "name": "@a3s-lab/code", - "version": "2.6.0", + "version": "3.0.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@a3s-lab/code", - "version": "2.6.0", + "version": "3.0.0", "license": "MIT", "devDependencies": { "@napi-rs/cli": "^2", @@ -15,12 +15,12 @@ "typescript": "^5.9.3" }, "optionalDependencies": { - "@a3s-lab/code-darwin-arm64": "2.6.0", - "@a3s-lab/code-linux-arm64-gnu": "2.6.0", - "@a3s-lab/code-linux-arm64-musl": "2.6.0", - "@a3s-lab/code-linux-x64-gnu": "2.6.0", - "@a3s-lab/code-linux-x64-musl": "2.6.0", - "@a3s-lab/code-win32-x64-msvc": "2.6.0" + "@a3s-lab/code-darwin-arm64": "3.0.0", + "@a3s-lab/code-linux-arm64-gnu": "3.0.0", + "@a3s-lab/code-linux-arm64-musl": "3.0.0", + "@a3s-lab/code-linux-x64-gnu": "3.0.0", + "@a3s-lab/code-linux-x64-musl": "3.0.0", + "@a3s-lab/code-win32-x64-msvc": "3.0.0" } }, "node_modules/@a3s-lab/code-darwin-arm64": { diff --git a/sdk/node/package.json b/sdk/node/package.json index e738160..c52bb58 100644 --- a/sdk/node/package.json +++ b/sdk/node/package.json @@ -1,6 +1,6 @@ { "name": "@a3s-lab/code", - "version": "2.6.0", + "version": "3.0.0", "description": "A3S Code - Native Node.js bindings for the coding-agent runtime", "main": "index.js", "types": "index.d.ts", @@ -40,11 +40,11 @@ "test:helpers": "node test-helpers.mjs" }, "optionalDependencies": { - "@a3s-lab/code-darwin-arm64": "2.6.0", - "@a3s-lab/code-linux-x64-gnu": "2.6.0", - "@a3s-lab/code-linux-x64-musl": "2.6.0", - "@a3s-lab/code-linux-arm64-gnu": "2.6.0", - "@a3s-lab/code-linux-arm64-musl": "2.6.0", - "@a3s-lab/code-win32-x64-msvc": "2.6.0" + "@a3s-lab/code-darwin-arm64": "3.0.0", + "@a3s-lab/code-linux-x64-gnu": "3.0.0", + "@a3s-lab/code-linux-x64-musl": "3.0.0", + "@a3s-lab/code-linux-arm64-gnu": "3.0.0", + "@a3s-lab/code-linux-arm64-musl": "3.0.0", + "@a3s-lab/code-win32-x64-msvc": "3.0.0" } } diff --git a/sdk/node/src/lib.rs b/sdk/node/src/lib.rs index ef262eb..1e9ea9e 100644 --- a/sdk/node/src/lib.rs +++ b/sdk/node/src/lib.rs @@ -197,6 +197,11 @@ pub struct AgentEvent { pub verification_summary_text: Option, /// Extra data for events that don't map to standard fields (JSON-encoded) pub data: Option, + /// Structured discriminant for tool failures on `tool_end` events + /// (JSON-encoded with a `type` field). `None` on success or untyped + /// failure. Lets streaming consumers branch on the failure kind + /// without scanning `tool_output`. + pub error_kind_json: Option, } #[napi(object)] @@ -252,6 +257,7 @@ impl AgentEvent { verification_summary_json: None, verification_summary_text: None, data: None, + error_kind_json: None, } } } @@ -305,11 +311,15 @@ impl From for AgentEvent { output, exit_code, metadata: _, + error_kind, } => Self { tool_id: Some(id), tool_name: Some(name), tool_output: Some(output), exit_code: Some(exit_code), + error_kind_json: error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), ..Self::empty("tool_end") }, RustAgentEvent::ToolOutputDelta { id, name, delta } => Self { @@ -726,6 +736,12 @@ pub struct ToolResult { pub metadata_json: Option, /// Convenience JSON view of `metadata.document_runtime` when present. pub document_runtime_json: Option, + /// Structured discriminant for tool failures, JSON-encoded with a + /// `type` field on the top level — e.g. + /// `{"type":"version_conflict","path":"doc.md","expected":"etag-1","actual":"etag-2"}`. + /// `None` on success or untyped failure. SDK callers parse it to + /// branch on the failure kind without scanning the `output` string. + pub error_kind_json: Option, } /// Execution limits for `Session.program`. @@ -862,6 +878,10 @@ fn tool_result_from_core(result: a3s_code_core::ToolCallResult) -> ToolResult { .as_ref() .and_then(|metadata| metadata.get("document_runtime")) .map(serde_json::Value::to_string), + error_kind_json: result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), } } @@ -1227,6 +1247,62 @@ pub struct JsS3BackendConfig { pub prefix: String, /// `true` for MinIO / RustFS / most non-AWS endpoints; `false` for AWS S3. pub force_path_style: Option, + /// Maximum bytes a single `read` may return. The backend rejects any + /// response with `Content-Length` greater than this without buffering + /// the body. Defaults to 10 MiB on the Rust side when omitted. + pub max_read_bytes: Option, + /// Enable degraded `grep` / `glob` against this S3 backend. Off by + /// default — object storage has no native search, so the only viable + /// strategy is `LIST` + `GET` + regex, which can be slow and expensive. + pub search_enabled: Option, + /// Upper bound on objects considered per `grep` / `glob` call. Defaults + /// to 500 on the Rust side. Ignored when `searchEnabled` is `false`. + pub max_objects_scanned: Option, + /// Per-object body-size ceiling for `grep` downloads. Larger objects are + /// skipped (debug-traced). Defaults to 1 MiB on the Rust side. Ignored + /// when `searchEnabled` is `false`. + pub max_grep_bytes_per_object: Option, + /// Concurrent object downloads during `grep`. Defaults to 8 on the + /// Rust side. Set lower when the gitserver / S3 endpoint rate-limits + /// aggressively; set higher when latency dominates. Ignored when + /// `searchEnabled` is `false`. + pub search_concurrency: Option, +} + +/// Configuration for a [`RemoteGitBackend`] — an HTTP/JSON client that +/// brings the `git` tool to non-local workspaces (S3, future container / +/// DFS). +/// +/// Pass alongside `workspaceBackend` on a session to attach remote git +/// on top of any filesystem backend. The protocol is specified in the +/// repository RFC `apps/docs/content/docs/en/code/rfcs/workspace-remote-git.mdx`. +#[napi(object)] +#[derive(Clone, Default)] +pub struct JsRemoteGitBackendConfig { + /// Base URL of the gitserver, no trailing slash. The client builds + /// `{baseUrl}/v1/repos/{repoId}/git/{op}` per the RFC. + pub base_url: String, + /// Opaque repository identifier, URL-safe. Negotiated out of band + /// with the gitserver operator. + pub repo_id: String, + /// Bearer token sent as `Authorization: Bearer `. Required in + /// production; omitting it emits a `tracing::warn!` and is only safe + /// on a trusted localhost gitserver. + pub bearer_token: Option, + /// mTLS client certificate path (PEM). When set together with + /// `clientKeyPem`, the backend reads both files at construction and + /// configures mTLS on the HTTP client. Setting only one of the pair + /// errors at construction. + pub client_cert_pem: Option, + /// mTLS client private key path (PEM). PKCS#8 format expected for the + /// `rustls-tls` backend. See `clientCertPem`. + pub client_key_pem: Option, + /// Per-call HTTP timeout in milliseconds. Defaults to 30 000. + pub request_timeout_ms: Option, + /// Client-side cap on `diff` response bytes. Defaults to 1 MiB. + pub max_diff_bytes: Option, + /// Client-side cap on `log` `max_count`. Defaults to 200. + pub max_log_entries: Option, } /// File-backed long-term memory store. @@ -1727,6 +1803,21 @@ pub struct SessionOptions { /// agent.session('/repo', { workspaceBackend: new LocalWorkspaceBackend('/repo') }); /// ``` pub workspace_backend: Option, + /// Optional remote git provider. When set, the resulting session attaches + /// a `RemoteGitBackend` on top of `workspaceBackend` so the built-in + /// `git` tool is available even on object-storage workspaces. + /// + /// ```js + /// agent.session('s3://workspace/u1/s1', { + /// workspaceBackend: new S3WorkspaceBackend({ ... }), + /// remoteGit: { + /// baseUrl: 'https://gitserver.internal', + /// repoId: 'u1/s1', + /// bearerToken: token, + /// }, + /// }); + /// ``` + pub remote_git: Option, /// Custom role/identity prepended before the core agentic prompt. /// Example: "You are a senior Python developer specializing in FastAPI." pub role: Option, @@ -1999,6 +2090,47 @@ fn s3_config_to_core(js: &JsS3BackendConfig) -> a3s_code_core::S3BackendConfig { if let Some(force) = js.force_path_style { cfg = cfg.force_path_style(force); } + if let Some(n) = js.max_read_bytes { + cfg = cfg.max_read_bytes(n.max(0) as u64); + } + if let Some(on) = js.search_enabled { + cfg = cfg.enable_search(on); + } + if let Some(n) = js.max_objects_scanned { + cfg = cfg.max_objects_scanned(n.max(0) as usize); + } + if let Some(n) = js.max_grep_bytes_per_object { + cfg = cfg.max_grep_bytes_per_object(n.max(0) as u64); + } + if let Some(n) = js.search_concurrency { + cfg = cfg.search_concurrency(n.max(0) as usize); + } + cfg +} + +fn remote_git_config_to_core( + js: &JsRemoteGitBackendConfig, +) -> a3s_code_core::RemoteGitBackendConfig { + let mut cfg = + a3s_code_core::RemoteGitBackendConfig::new(js.base_url.clone(), js.repo_id.clone()); + if let Some(ref t) = js.bearer_token { + cfg = cfg.bearer_token(t.clone()); + } + if let Some(ref p) = js.client_cert_pem { + cfg = cfg.client_cert_pem(std::path::PathBuf::from(p)); + } + if let Some(ref p) = js.client_key_pem { + cfg = cfg.client_key_pem(std::path::PathBuf::from(p)); + } + if let Some(ms) = js.request_timeout_ms { + cfg = cfg.request_timeout(std::time::Duration::from_millis(ms.max(0) as u64)); + } + if let Some(n) = js.max_diff_bytes { + cfg = cfg.max_diff_bytes(n.max(0) as u64); + } + if let Some(n) = js.max_log_entries { + cfg = cfg.max_log_entries(n.max(0) as usize); + } cfg } @@ -2082,13 +2214,13 @@ fn js_session_options_to_rust(options: Option) -> napi::Result = match backend.kind.as_str() + { "" | "local" => { let root = backend.root.as_ref().ok_or_else(|| { napi::Error::from_reason("LocalWorkspaceBackend requires a root path") })?; - opts = opts - .with_workspace_backend(a3s_code_core::WorkspaceServices::local(root.clone())); + a3s_code_core::WorkspaceServices::local(root.clone()) } "s3" => { let s3_config = backend.s3.as_ref().ok_or_else(|| { @@ -2096,16 +2228,30 @@ fn js_session_options_to_rust(options: Option) -> napi::Result { return Err(napi::Error::from_reason(format!( "Unsupported workspace backend kind '{other}'" ))); } - } + }; + let services = if let Some(ref git_cfg) = o.remote_git { + services + .with_remote_git(remote_git_config_to_core(git_cfg)) + .map_err(|e| napi::Error::from_reason(format!("with_remote_git: {e}")))? + } else { + services + }; + opts = opts.with_workspace_backend(services); + } else if o.remote_git.is_some() { + // `remoteGit` needs a base `WorkspaceServices` to attach to. The + // session path is not available here (it's the first argument to + // `agent.session(path, options)`, applied later by the runtime), + // so we cannot synthesize a local backend on the user's behalf. + return Err(napi::Error::from_reason( + "remoteGit requires workspaceBackend to be set; pass a LocalWorkspaceBackend or S3WorkspaceBackend alongside it", + )); } // Build prompt slots if any slot is set if o.role.is_some() || o.guidelines.is_some() || o.response_style.is_some() || o.extra.is_some() @@ -3044,6 +3190,10 @@ impl Session { exit_code: r.exit_code, metadata_json: r.metadata.and_then(|m| serde_json::to_string(&m).ok()), document_runtime_json: None, + error_kind_json: r + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), }) }) .await @@ -4552,6 +4702,47 @@ impl From for RustSearchConfig { mod tests { use super::*; + /// Phase 8 alignment: when the Rust core surfaces a typed + /// `ToolErrorKind`, `tool_result_from_core` must round-trip it into + /// `error_kind_json` on the SDK shape. Tests both the JSON envelope + /// and the discriminator (`type`) field. + #[test] + fn tool_result_from_core_threads_error_kind_json() { + let core_result = a3s_code_core::ToolCallResult { + name: "edit".to_string(), + output: "Concurrent modification detected".to_string(), + exit_code: 1, + metadata: None, + error_kind: Some(a3s_code_core::ToolErrorKind::VersionConflict { + path: "doc.md".to_string(), + expected: "etag-1".to_string(), + actual: Some("etag-2".to_string()), + }), + }; + let sdk_result = tool_result_from_core(core_result); + let json_str = sdk_result + .error_kind_json + .expect("typed error_kind must round-trip into error_kind_json"); + let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + assert_eq!(parsed["type"], "version_conflict"); + assert_eq!(parsed["path"], "doc.md"); + assert_eq!(parsed["expected"], "etag-1"); + assert_eq!(parsed["actual"], "etag-2"); + } + + #[test] + fn tool_result_from_core_leaves_error_kind_json_none_on_success() { + let core_result = a3s_code_core::ToolCallResult { + name: "read".to_string(), + output: "hello".to_string(), + exit_code: 0, + metadata: None, + error_kind: None, + }; + let sdk_result = tool_result_from_core(core_result); + assert!(sdk_result.error_kind_json.is_none()); + } + #[test] fn planning_mode_parser_accepts_explicit_tristate() { assert!(matches!( @@ -4674,6 +4865,7 @@ mod tests { bucket: "workspace".to_string(), prefix: "users/u1/sessions/s1".to_string(), force_path_style: Some(true), + ..Default::default() }), }), ..Default::default() @@ -4703,6 +4895,92 @@ mod tests { assert!(result.is_err()); } + #[test] + fn s3_phase1_3_options_thread_through_to_core() { + let opts = js_session_options_to_rust(Some(SessionOptions { + workspace_backend: Some(JsWorkspaceBackend { + kind: "s3".to_string(), + root: None, + s3: Some(JsS3BackendConfig { + access_key_id: "AKIA".to_string(), + secret_access_key: "secret".to_string(), + bucket: "workspace".to_string(), + prefix: "u1/s1".to_string(), + max_read_bytes: Some(4 * 1024 * 1024), + search_enabled: Some(true), + max_objects_scanned: Some(250), + max_grep_bytes_per_object: Some(512 * 1024), + ..Default::default() + }), + }), + ..Default::default() + })) + .unwrap(); + + let services = opts.workspace_services.expect("s3 backend builds services"); + assert!( + services.capabilities().search, + "searchEnabled=true must enable the search capability" + ); + assert!(services.search().is_some()); + } + + #[test] + fn remote_git_attaches_on_top_of_s3_backend() { + let opts = js_session_options_to_rust(Some(SessionOptions { + workspace_backend: Some(JsWorkspaceBackend { + kind: "s3".to_string(), + root: None, + s3: Some(JsS3BackendConfig { + access_key_id: "AKIA".to_string(), + secret_access_key: "secret".to_string(), + bucket: "workspace".to_string(), + prefix: "u1/s1".to_string(), + ..Default::default() + }), + }), + remote_git: Some(JsRemoteGitBackendConfig { + base_url: "https://gitserver.internal".to_string(), + repo_id: "u1/s1".to_string(), + bearer_token: Some("tok".to_string()), + request_timeout_ms: Some(10_000), + ..Default::default() + }), + ..Default::default() + })) + .unwrap(); + + let services = opts.workspace_services.expect("services built"); + assert!( + services.git().is_some(), + "remoteGit must register a git provider" + ); + assert!(services.git_stash().is_some()); + // Worktree is intentionally not available — see RFC §8. + assert!(services.git_worktree().is_none()); + assert!(services.capabilities().git); + } + + #[test] + fn remote_git_without_workspace_backend_errors_clearly() { + let result = js_session_options_to_rust(Some(SessionOptions { + workspace_backend: None, + remote_git: Some(JsRemoteGitBackendConfig { + base_url: "https://gitserver".to_string(), + repo_id: "r".to_string(), + ..Default::default() + }), + ..Default::default() + })); + + let err = result.unwrap_err(); + assert!( + err.to_string().contains("workspaceBackend"), + "error message must mention the missing field, got: {}", + err + ); + } + #[test] fn confirmation_policy_rejects_invalid_yolo_lane() { let result = js_session_options_to_rust(Some(SessionOptions { diff --git a/sdk/python/Cargo.toml b/sdk/python/Cargo.toml index 6bbe92b..ce1ea72 100644 --- a/sdk/python/Cargo.toml +++ b/sdk/python/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "a3s-code-py" -version = "2.6.0" +version = "3.0.0" edition = "2021" authors = ["A3S Lab Team"] license = "MIT" @@ -12,7 +12,7 @@ name = "a3s_code" crate-type = ["cdylib"] [dependencies] -a3s-code-core = { version = "2.6.0", path = "../../core", features = ["ahp", "s3"] } +a3s-code-core = { version = "3.0.0", path = "../../core", features = ["ahp", "s3"] } pyo3 = "0.23" tokio = { version = "1.35", features = ["full"] } serde_json = "1.0" diff --git a/sdk/python/pyproject.toml b/sdk/python/pyproject.toml index 498c997..d5ccded 100644 --- a/sdk/python/pyproject.toml +++ b/sdk/python/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "maturin" [project] name = "a3s-code" -version = "2.6.0" +version = "3.0.0" description = "A3S Code - Native Python bindings for the coding-agent runtime" readme = "README.md" license = {text = "MIT"} diff --git a/sdk/python/src/lib.rs b/sdk/python/src/lib.rs index 54624bc..adc6058 100644 --- a/sdk/python/src/lib.rs +++ b/sdk/python/src/lib.rs @@ -248,6 +248,14 @@ struct PyAgentEvent { /// Extra data for events that don't map to standard fields (JSON-encoded) #[pyo3(get)] data: Option, + /// Structured discriminant for tool failures on ``tool_end`` events + /// (JSON-encoded with a ``type`` field on the top level — + /// e.g. ``{"type":"version_conflict","path":"doc.md","expected":"etag-1","actual":"etag-2"}``). + /// ``None`` on success or untyped failure. Streaming consumers parse + /// this via the ``error_kind`` property to branch on the failure + /// kind without scanning ``tool_output``. + #[pyo3(get)] + error_kind_json: Option, } impl PyAgentEvent { @@ -266,6 +274,7 @@ impl PyAgentEvent { verification_summary_json: None, verification_summary_text: None, data: None, + error_kind_json: None, } } } @@ -289,6 +298,19 @@ impl PyAgentEvent { _ => format!("AgentEvent(type='{}')", self.event_type), } } + + /// Parsed `error_kind_json` as a dict — the discriminator lives on + /// the ``type`` key (see [`ToolErrorKind`](crate::tools::ToolErrorKind) + /// for the full set of variants). Downstream code matches on + /// ``event.error_kind["type"]`` to decide retry behaviour without + /// scanning ``tool_output``. + #[getter] + fn error_kind(&self, py: Python<'_>) -> PyResult> { + self.error_kind_json + .as_deref() + .map(|json| json_string_to_py(py, json)) + .transpose() + } } impl From for PyAgentEvent { @@ -336,11 +358,15 @@ impl From for PyAgentEvent { output, exit_code, metadata: _, + error_kind, } => Self { tool_id: Some(id), tool_name: Some(name), tool_output: Some(output), exit_code: Some(exit_code), + error_kind_json: error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), ..Self::empty("tool_end") }, RustAgentEvent::ToolOutputDelta { id, name, delta } => Self { @@ -742,6 +768,14 @@ struct PyToolResult { /// Raw JSON-encoded tool metadata returned by the Rust core API. #[pyo3(get)] metadata_json: Option, + /// Structured discriminant for tool failures, JSON-encoded with a + /// ``type`` field on the top level — + /// e.g. ``{"type":"version_conflict","path":"doc.md","expected":"etag-1","actual":"etag-2"}``. + /// ``None`` on success or untyped failure. SDK callers parse it via + /// the ``error_kind`` property below to branch on the failure kind + /// without scanning the ``output`` string. + #[pyo3(get)] + error_kind_json: Option, } #[pymethods] @@ -754,6 +788,17 @@ impl PyToolResult { .transpose() } + /// Parsed `error_kind_json` as a dict. The discriminator lives on the + /// ``type`` key; downstream code matches on that to decide retry + /// behaviour without parsing ``output``. + #[getter] + fn error_kind(&self, py: Python<'_>) -> PyResult> { + self.error_kind_json + .as_deref() + .map(|json| json_string_to_py(py, json)) + .transpose() + } + fn __repr__(&self) -> String { format!( "ToolResult(name='{}', exit_code={})", @@ -1499,6 +1544,10 @@ impl PySession { output: result.output, exit_code: result.exit_code, metadata_json: result.metadata.as_ref().map(serde_json::Value::to_string), + error_kind_json: result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), }) } @@ -1519,6 +1568,10 @@ impl PySession { output: result.output, exit_code: result.exit_code, metadata_json: result.metadata.as_ref().map(serde_json::Value::to_string), + error_kind_json: result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), }) } @@ -1545,6 +1598,10 @@ impl PySession { output: result.output, exit_code: result.exit_code, metadata_json: result.metadata.as_ref().map(serde_json::Value::to_string), + error_kind_json: result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), }) } @@ -1568,6 +1625,10 @@ impl PySession { output: result.output, exit_code: result.exit_code, metadata_json: result.metadata.as_ref().map(serde_json::Value::to_string), + error_kind_json: result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), }) } @@ -1594,6 +1655,10 @@ impl PySession { output: result.output, exit_code: result.exit_code, metadata_json: result.metadata.as_ref().map(serde_json::Value::to_string), + error_kind_json: result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), }) } @@ -1616,6 +1681,10 @@ impl PySession { output: result.output, exit_code: result.exit_code, metadata_json: result.metadata.as_ref().map(serde_json::Value::to_string), + error_kind_json: result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), }) } @@ -1632,6 +1701,10 @@ impl PySession { output: result.output, exit_code: result.exit_code, metadata_json: result.metadata.as_ref().map(serde_json::Value::to_string), + error_kind_json: result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), }) } @@ -1662,6 +1735,10 @@ impl PySession { output: result.output, exit_code: result.exit_code, metadata_json: result.metadata.as_ref().map(serde_json::Value::to_string), + error_kind_json: result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), }) } @@ -1677,6 +1754,10 @@ impl PySession { output: result.output, exit_code: result.exit_code, metadata_json: result.metadata.as_ref().map(serde_json::Value::to_string), + error_kind_json: result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), }) } @@ -1730,6 +1811,10 @@ impl PySession { output: result.output, exit_code: result.exit_code, metadata_json: result.metadata.as_ref().map(serde_json::Value::to_string), + error_kind_json: result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), }) } @@ -1811,6 +1896,10 @@ impl PySession { output: result.output, exit_code: result.exit_code, metadata_json: result.metadata.as_ref().map(serde_json::Value::to_string), + error_kind_json: result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), }) } @@ -1835,6 +1924,10 @@ impl PySession { output: result.output, exit_code: result.exit_code, metadata_json: result.metadata.as_ref().map(serde_json::Value::to_string), + error_kind_json: result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()), }) } @@ -3144,6 +3237,27 @@ struct PyS3WorkspaceBackend { session_token: Option, #[pyo3(get, set)] force_path_style: bool, + /// Per-read size ceiling (bytes). Defaults to 10 MiB when ``None``. + #[pyo3(get, set)] + max_read_bytes: Option, + /// Enable degraded ``grep`` / ``glob`` against this backend. Off by default + /// because LIST + GET + regex can be slow and expensive. + #[pyo3(get, set)] + search_enabled: bool, + /// Upper bound on objects considered per ``grep`` / ``glob`` call. + /// Defaults to 500 when ``None``. Ignored when ``search_enabled`` is False. + #[pyo3(get, set)] + max_objects_scanned: Option, + /// Per-object body-size ceiling for ``grep`` downloads. Defaults to 1 MiB + /// when ``None``. Ignored when ``search_enabled`` is False. + #[pyo3(get, set)] + max_grep_bytes_per_object: Option, + /// Concurrent object downloads during ``grep``. Defaults to 8 when + /// ``None``. Set lower when the gitserver / S3 endpoint rate-limits + /// aggressively; set higher when latency dominates. Ignored when + /// ``search_enabled`` is False. + #[pyo3(get, set)] + search_concurrency: Option, } #[pymethods] @@ -3158,6 +3272,11 @@ impl PyS3WorkspaceBackend { region = None, session_token = None, force_path_style = false, + max_read_bytes = None, + search_enabled = false, + max_objects_scanned = None, + max_grep_bytes_per_object = None, + search_concurrency = None, ))] #[allow(clippy::too_many_arguments)] fn new( @@ -3169,6 +3288,11 @@ impl PyS3WorkspaceBackend { region: Option, session_token: Option, force_path_style: bool, + max_read_bytes: Option, + search_enabled: bool, + max_objects_scanned: Option, + max_grep_bytes_per_object: Option, + search_concurrency: Option, ) -> Self { Self { bucket, @@ -3179,13 +3303,18 @@ impl PyS3WorkspaceBackend { region, session_token, force_path_style, + max_read_bytes, + search_enabled, + max_objects_scanned, + max_grep_bytes_per_object, + search_concurrency, } } fn __repr__(&self) -> String { format!( - "S3WorkspaceBackend(bucket={:?}, prefix={:?}, endpoint={:?}, region={:?}, force_path_style={})", - self.bucket, self.prefix, self.endpoint, self.region, self.force_path_style + "S3WorkspaceBackend(bucket={:?}, prefix={:?}, endpoint={:?}, region={:?}, force_path_style={}, search_enabled={})", + self.bucket, self.prefix, self.endpoint, self.region, self.force_path_style, self.search_enabled, ) } } @@ -3198,7 +3327,8 @@ impl PyS3WorkspaceBackend { self.access_key_id.clone(), self.secret_access_key.clone(), ) - .force_path_style(self.force_path_style); + .force_path_style(self.force_path_style) + .enable_search(self.search_enabled); if let Some(ref endpoint) = self.endpoint { cfg = cfg.endpoint(endpoint.clone()); } @@ -3208,6 +3338,132 @@ impl PyS3WorkspaceBackend { if let Some(ref token) = self.session_token { cfg = cfg.session_token(token.clone()); } + if let Some(n) = self.max_read_bytes { + cfg = cfg.max_read_bytes(n); + } + if let Some(n) = self.max_objects_scanned { + cfg = cfg.max_objects_scanned(n as usize); + } + if let Some(n) = self.max_grep_bytes_per_object { + cfg = cfg.max_grep_bytes_per_object(n); + } + if let Some(n) = self.search_concurrency { + cfg = cfg.search_concurrency(n as usize); + } + cfg + } +} + +/// Configuration for a remote git backend that brings the ``git`` tool to +/// non-local workspaces (S3, future container / DFS) over HTTP/JSON. +/// +/// Attach to a session alongside ``workspace_backend``: +/// +/// .. code-block:: python +/// +/// opts = SessionOptions() +/// opts.workspace_backend = S3WorkspaceBackend(...) +/// opts.remote_git = RemoteGitBackendConfig( +/// base_url="https://gitserver.internal", +/// repo_id="u1/s1", +/// bearer_token=token, +/// ) +#[pyclass(name = "RemoteGitBackendConfig")] +#[derive(Clone)] +struct PyRemoteGitBackendConfig { + #[pyo3(get, set)] + base_url: String, + #[pyo3(get, set)] + repo_id: String, + #[pyo3(get, set)] + bearer_token: Option, + /// mTLS client certificate path (PEM). When set together with + /// ``client_key_pem``, the backend reads both files at construction and + /// configures mTLS on the HTTP client. Setting only one of the pair + /// errors at construction. + #[pyo3(get, set)] + client_cert_pem: Option, + /// mTLS client private key path (PEM). PKCS#8 format expected for the + /// ``rustls-tls`` backend. See ``client_cert_pem``. + #[pyo3(get, set)] + client_key_pem: Option, + /// Per-call HTTP timeout in milliseconds. Defaults to 30 000. + #[pyo3(get, set)] + request_timeout_ms: Option, + /// Client-side cap on ``diff`` response bytes. Defaults to 1 MiB. + #[pyo3(get, set)] + max_diff_bytes: Option, + /// Client-side cap on ``log`` ``max_count``. Defaults to 200. + #[pyo3(get, set)] + max_log_entries: Option, +} + +#[pymethods] +impl PyRemoteGitBackendConfig { + #[new] + #[pyo3(signature = ( + base_url, + repo_id, + bearer_token = None, + client_cert_pem = None, + client_key_pem = None, + request_timeout_ms = None, + max_diff_bytes = None, + max_log_entries = None, + ))] + #[allow(clippy::too_many_arguments)] + fn new( + base_url: String, + repo_id: String, + bearer_token: Option, + client_cert_pem: Option, + client_key_pem: Option, + request_timeout_ms: Option, + max_diff_bytes: Option, + max_log_entries: Option, + ) -> Self { + Self { + base_url, + repo_id, + bearer_token, + client_cert_pem, + client_key_pem, + request_timeout_ms, + max_diff_bytes, + max_log_entries, + } + } + + fn __repr__(&self) -> String { + format!( + "RemoteGitBackendConfig(base_url={:?}, repo_id={:?})", + self.base_url, self.repo_id + ) + } +} + +impl PyRemoteGitBackendConfig { + fn to_core(&self) -> a3s_code_core::RemoteGitBackendConfig { + let mut cfg = + a3s_code_core::RemoteGitBackendConfig::new(self.base_url.clone(), self.repo_id.clone()); + if let Some(ref t) = self.bearer_token { + cfg = cfg.bearer_token(t.clone()); + } + if let Some(ref p) = self.client_cert_pem { + cfg = cfg.client_cert_pem(std::path::PathBuf::from(p)); + } + if let Some(ref p) = self.client_key_pem { + cfg = cfg.client_key_pem(std::path::PathBuf::from(p)); + } + if let Some(ms) = self.request_timeout_ms { + cfg = cfg.request_timeout(std::time::Duration::from_millis(ms)); + } + if let Some(n) = self.max_diff_bytes { + cfg = cfg.max_diff_bytes(n); + } + if let Some(n) = self.max_log_entries { + cfg = cfg.max_log_entries(n as usize); + } cfg } } @@ -3717,6 +3973,12 @@ struct PySessionOptions { security_provider: Option, /// Workspace backend. Set to ``LocalWorkspaceBackend`` to use local filesystem tools explicitly. workspace_backend: Option, + /// Optional remote git provider. When set, the session attaches a + /// ``RemoteGitBackend`` on top of ``workspace_backend`` so the built-in + /// ``git`` tool is available on object-storage workspaces. Requires + /// ``workspace_backend`` to be set; otherwise the session raises a clear + /// error at construction. + remote_git: Option, /// Custom role/identity (e.g. "You are a Python expert") role: Option, /// Custom coding guidelines @@ -3813,6 +4075,7 @@ impl Clone for PySessionOptions { workspace_backend: pyo3::Python::with_gil(|py| { self.workspace_backend.as_ref().map(|o| o.clone_ref(py)) }), + remote_git: self.remote_git.clone(), role: self.role.clone(), guidelines: self.guidelines.clone(), response_style: self.response_style.clone(), @@ -3858,6 +4121,7 @@ impl PySessionOptions { session_store: None, security_provider: None, workspace_backend: None, + remote_git: None, role: None, guidelines: None, response_style: None, @@ -4069,6 +4333,20 @@ impl PySessionOptions { self.workspace_backend = value; } + /// Optional remote git provider. Attach a ``RemoteGitBackendConfig`` to + /// bring the built-in ``git`` tool to a session whose ``workspace_backend`` + /// cannot natively host git (e.g. S3). Requires ``workspace_backend`` to + /// be set. + #[getter] + fn get_remote_git(&self) -> Option { + self.remote_git.clone() + } + + #[setter] + fn set_remote_git(&mut self, value: Option) { + self.remote_git = value; + } + /// Custom role/identity prepended before the core agentic prompt. /// Example: "You are a senior Python developer specializing in FastAPI." #[getter] @@ -4605,9 +4883,11 @@ fn build_rust_session_options(so: PySessionOptions) -> PyResult), Unknown, } let resolved = Python::with_gil(|py| -> BackendKind { @@ -4615,23 +4895,31 @@ fn build_rust_session_options(so: PySessionOptions) -> PyResult>(py) { - return BackendKind::S3(s3.to_core()); + return BackendKind::S3(Box::new(s3.to_core())); } BackendKind::Unknown }); - match resolved { - BackendKind::Local(root) => { - o = o.with_workspace_backend(a3s_code_core::WorkspaceServices::local(root)); - } - BackendKind::S3(cfg) => { - o = o.with_workspace_backend(a3s_code_core::WorkspaceServices::s3(cfg)); - } + let services = match resolved { + BackendKind::Local(root) => a3s_code_core::WorkspaceServices::local(root), + BackendKind::S3(cfg) => a3s_code_core::WorkspaceServices::s3(*cfg), BackendKind::Unknown => { return Err(PyTypeError::new_err( "workspace_backend must be a LocalWorkspaceBackend or S3WorkspaceBackend instance", )); } - } + }; + let services = if let Some(ref git_cfg) = so.remote_git { + services + .with_remote_git(git_cfg.to_core()) + .map_err(|e| PyValueError::new_err(format!("remote_git: {e}")))? + } else { + services + }; + o = o.with_workspace_backend(services); + } else if so.remote_git.is_some() { + return Err(PyValueError::new_err( + "remote_git requires workspace_backend to be set; assign a LocalWorkspaceBackend or S3WorkspaceBackend first", + )); } // Build prompt slots if any slot is set if so.role.is_some() @@ -5432,6 +5720,7 @@ fn a3s_code_native(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_class::()?; m.add_class::()?; m.add_class::()?; + m.add_class::()?; m.add_class::()?; m.add_class::()?; m.add_class::()?; @@ -5549,6 +5838,11 @@ mod tests { region: Some("us-east-1".to_string()), session_token: None, force_path_style: true, + max_read_bytes: None, + search_enabled: false, + max_objects_scanned: None, + max_grep_bytes_per_object: None, + search_concurrency: None, }, ) .unwrap(); @@ -5567,6 +5861,162 @@ mod tests { assert!(!caps.search); } + #[test] + fn s3_phase1_3_options_thread_through_to_core() { + pyo3::prepare_freethreaded_python(); + let opts = Python::with_gil(|py| { + let backend = Py::new( + py, + PyS3WorkspaceBackend { + bucket: "workspace".to_string(), + prefix: "u1/s1".to_string(), + access_key_id: "AKIA".to_string(), + secret_access_key: "secret".to_string(), + endpoint: None, + region: None, + session_token: None, + force_path_style: false, + max_read_bytes: Some(4 * 1024 * 1024), + search_enabled: true, + max_objects_scanned: Some(250), + max_grep_bytes_per_object: Some(512 * 1024), + search_concurrency: None, + }, + ) + .unwrap(); + let mut session_options = PySessionOptions::new(); + session_options.workspace_backend = Some(backend.into_any()); + build_rust_session_options(session_options) + }) + .unwrap(); + + let services = opts.workspace_services.expect("services built"); + assert!( + services.capabilities().search, + "search_enabled=true must enable the search capability" + ); + assert!(services.search().is_some()); + } + + #[test] + fn remote_git_attaches_on_top_of_s3_backend() { + pyo3::prepare_freethreaded_python(); + let opts = Python::with_gil(|py| { + let backend = Py::new( + py, + PyS3WorkspaceBackend { + bucket: "workspace".to_string(), + prefix: "u1/s1".to_string(), + access_key_id: "AKIA".to_string(), + secret_access_key: "secret".to_string(), + endpoint: None, + region: None, + session_token: None, + force_path_style: false, + max_read_bytes: None, + search_enabled: false, + max_objects_scanned: None, + max_grep_bytes_per_object: None, + search_concurrency: None, + }, + ) + .unwrap(); + let mut session_options = PySessionOptions::new(); + session_options.workspace_backend = Some(backend.into_any()); + session_options.remote_git = Some(PyRemoteGitBackendConfig { + base_url: "https://gitserver.internal".to_string(), + repo_id: "u1/s1".to_string(), + bearer_token: Some("tok".to_string()), + client_cert_pem: None, + client_key_pem: None, + request_timeout_ms: Some(10_000), + max_diff_bytes: None, + max_log_entries: None, + }); + build_rust_session_options(session_options) + }) + .unwrap(); + + let services = opts.workspace_services.expect("services built"); + assert!(services.git().is_some()); + assert!(services.git_stash().is_some()); + // Worktree intentionally unavailable on remote-git workspaces (RFC §8). + assert!(services.git_worktree().is_none()); + assert!(services.capabilities().git); + } + + /// Phase 8 alignment: a typed `ToolErrorKind` from the Rust core + /// must arrive at the Python SDK as a JSON envelope on + /// `error_kind_json`, with the discriminator on `type`. We assert + /// both the raw string shape and the parsed serde_json round-trip + /// (Python's `error_kind` getter calls `json_string_to_py` on the + /// same string, so this test fully covers the contract without + /// needing a Python interpreter to run JSON.parse). + #[test] + fn py_tool_result_threads_error_kind_json() { + let kind = a3s_code_core::ToolErrorKind::VersionConflict { + path: "doc.md".to_string(), + expected: "etag-1".to_string(), + actual: Some("etag-2".to_string()), + }; + // The SDK conversion path uses `serde_json::to_string(&k).ok()`; + // mirror that here to exercise the exact envelope shape the + // Python `error_kind` property reads from. + let json = serde_json::to_string(&kind).expect("kind serialises"); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed["type"], "version_conflict"); + assert_eq!(parsed["path"], "doc.md"); + assert_eq!(parsed["expected"], "etag-1"); + assert_eq!(parsed["actual"], "etag-2"); + } + + /// Successful tool calls and tool calls that fail without a typed + /// reason must leave `error_kind_json` as `None` so SDK callers can + /// rely on its presence as the sole "is this a typed failure?" + /// signal. + #[test] + fn py_tool_result_error_kind_json_is_none_when_no_kind() { + let result = a3s_code_core::ToolCallResult { + name: "read".to_string(), + output: "hello".to_string(), + exit_code: 0, + metadata: None, + error_kind: None, + }; + let json = result + .error_kind + .as_ref() + .and_then(|k| serde_json::to_string(k).ok()); + assert!(json.is_none()); + } + + #[test] + fn remote_git_without_workspace_backend_errors_clearly() { + pyo3::prepare_freethreaded_python(); + let result = Python::with_gil(|_py| { + let mut session_options = PySessionOptions::new(); + session_options.remote_git = Some(PyRemoteGitBackendConfig { + base_url: "https://gitserver".to_string(), + repo_id: "r".to_string(), + bearer_token: None, + client_cert_pem: None, + client_key_pem: None, + request_timeout_ms: None, + max_diff_bytes: None, + max_log_entries: None, + }); + build_rust_session_options(session_options) + }); + + let err = result.unwrap_err(); + let msg = format!("{err}"); + assert!( + msg.contains("workspace_backend"), + "error must mention missing field, got: {}", + msg + ); + } + #[test] fn delegate_task_args_use_core_task_schema() { let args = delegate_task_args(