diff --git a/docs/adr/0012-sandbox-is-the-trust-boundary-network-perimeter-is-the-control.md b/docs/adr/0012-sandbox-is-the-trust-boundary-network-perimeter-is-the-control.md new file mode 100644 index 0000000..4f884e6 --- /dev/null +++ b/docs/adr/0012-sandbox-is-the-trust-boundary-network-perimeter-is-the-control.md @@ -0,0 +1,59 @@ +# The sandbox is the trust boundary; the network perimeter is the only in-app control + +## Status + +accepted + +## Context + +`aikit serve` exposes the in-process `aikit` agent, whose default toolset includes +`run_bash` (an unrestricted `sh -c`), `write_file`, and `git`. A security audit +(`specs/ISSUES-2026-07-07.md`, SEC-2) framed this as unauthenticated prompt-driven RCE: +a caller who reaches the API can steer the agent into arbitrary shell execution on the +serving host. Two fixes were proposed — gate `run_bash`/`yolo` behind a `--allow-shell` +flag, and/or make the per-agent tool policy (which already exists as +`AgentPersona.tools` / `disallowed_tools`, hard-filtered at `loop_runner.rs:355-360`) +default-deny for dangerous tools. + +We rejected both as the *primary* control. An autonomous coding agent whose entire +purpose is to read, write, and run code is not made safe by removing its ability to run +code; it is made safe by running it somewhere disposable. aikit's intended deployment is +inside a sandboxed container (driven by agentrt, the optimization loop, or a chat BFF), +where taking control of the execution environment is expected agent behaviour, not an +exploit. + +## Decision + +**Protecting the host from the agent is the container's responsibility, not aikit's.** +The in-process agent keeps its full default toolset (`run_bash` included); tool +availability is a *capability* concern expressed per-agent via `AgentPersona.tools` / +`disallowed_tools`, not a host-safety control, and is not gated by any serve flag. There +is no `--allow-shell`. + +Because we deliberately do **not** constrain the agent internally, the **network +perimeter is the sole remaining in-application control**, and it must fail closed: +`aikit serve` MUST refuse to start when bound to a non-loopback address without an +`--api-key` (a hard error, not the current warning at `serve/mod.rs:738`). The loopback +default stays open so existing local consumers (agentrt, optimization loop, chat BFF) are +unaffected. + +## Consequences + +- SEC-2's "RCE" reframes from a code defect to a **deployment contract**: aikit must be + run in a sandbox. This is documented in the `serve` help text and `webdocs`, and is a + precondition for any exposure. +- The perimeter carries more weight, not less. The sandbox walls off the *host* but not + the two things the container still holds — **LLM credentials** (an exposed agent can + burn or exfiltrate `ANTHROPIC_API_KEY` / the OpenAI-compat key) and **network egress** + (the agent can reach whatever the container can). Fail-closed auth is the only thing + standing in front of those, so it is mandatory, not optional. +- `yolo` (client-settable permission bypass) and client-supplied `mcp_servers` + (SEC-3) are **not** security bugs under this stance — they are in-sandbox capability + choices. SEC-3's *concurrency cap* (no `max_sessions` on live sessions) remains a real + resource-exhaustion defect and is tracked independently of this decision. +- `AgentPersona.tools` / `disallowed_tools` already exists and is wired; the remaining + work is to **plumb a tool policy through serve** (`SendMessageRequest` cannot currently + carry one) so callers *may* restrict capability when they want to — an opt-in + least-privilege lever, not a default. +- Operators who cannot sandbox must not expose aikit; there is no in-app substitute for + the container boundary, by design. diff --git a/docs/adr/0013-package-install-writes-only-within-the-target-project.md b/docs/adr/0013-package-install-writes-only-within-the-target-project.md new file mode 100644 index 0000000..3240b71 --- /dev/null +++ b/docs/adr/0013-package-install-writes-only-within-the-target-project.md @@ -0,0 +1,54 @@ +# Package install writes only within the target project + +## Status + +accepted (companion to [0012](0012-sandbox-is-the-trust-boundary-network-perimeter-is-the-control.md)) + +## Context + +`aikit install ` fetches a package and copies its artifacts into a project +according to the package's `aikit.toml` `[artifacts]` table. The destination strings come +from an untrusted manifest and were used as `project_root.join(dest_str)` with no +validation (`aikit-sdk/src/install.rs:153`). `Path::join` with an absolute value replaces +the base, and `..` was never rejected, so a package could write outside the project — e.g. +`"payload/**" = "/home/victim/.ssh"` or `"../../../etc/cron.d"` (audit SEC-1). Related +traversal exists via skill/subagent names and `source` paths (SEC-5). + +Unlike `aikit serve` (ADR 0012), `install` does **not** run in a sandbox. It runs on a +developer's own machine with their real `~/.ssh`, cloud credentials, and shell rc files +present. The container trust boundary does not apply here, so the "assume sandboxed" +stance does not transfer: install is a distinct, un-sandboxed boundary. The npm/cargo +"install runs arbitrary code" precedent is a known flaw to avoid, not a licence to copy — +and a *declarative* artifact manifest never legitimately needs to write outside the +project it is scaffolding. + +## Decision + +A package may write **only within the target project root**. Enforcement: + +1. **Parse-time validation.** Every artifact destination (and skill/subagent name and + `source`) is validated when the manifest is deserialized, before any file is written. + A single unsafe mapping **aborts the entire install** with a clear error — no partial + writes, one error site. +2. **Reject lexically:** absolute paths and any `..` component are refused + (`safe_join(base, untrusted)`). +3. **No-follow-symlinks on the write path.** Extraction/copy refuses to follow symlinks, + closing the escape where one artifact writes `link -> /etc` and a later mapping targets + `link/…` (which a lexical-only check would pass). +4. Flat identifiers used to build filenames (package `name`/`version` → cache dir, SEC-4; + client `session_id` → session file, SEC-10) are validated with a strict id charset + (`is_safe_id`), not `safe_join`, since they are not path fragments. + +This logic lives once in `aikit-sdk` and is shared by every install/extract path, +consistent with the ARCH-1 direction of unifying the duplicated install/fetch stacks. + +## Consequences + +- SEC-1 and SEC-5 are closed at the source: a hostile or malformed package cannot escape + the project directory. +- Legitimate templates are unaffected — they only ever write within the project they + scaffold. +- Zip-slip on *extraction* was already mitigated; this closes the separate + artifact-*mapping* layer that sat above it. +- The `safe_join` / `is_safe_id` helpers become the single validation seam reused by + SEC-1/4/5/10, so future install/session code inherits the guarantee by construction. diff --git a/docs/adr/0014-runs-are-cancellable-terminated-via-process-group-signal-escalation.md b/docs/adr/0014-runs-are-cancellable-terminated-via-process-group-signal-escalation.md new file mode 100644 index 0000000..d986398 --- /dev/null +++ b/docs/adr/0014-runs-are-cancellable-terminated-via-process-group-signal-escalation.md @@ -0,0 +1,60 @@ +# Runs are cancellable; termination escalates SIGTERM→SIGKILL over the process group + +## Status + +accepted + +## Context + +aikit had no real notion of *stopping* a run. The subprocess Transport spawned the agent +CLI without process-group isolation, and the only termination path was a watchdog calling +`child.kill()` on the direct child (`aikit-sdk/src/runner/mod.rs:260-266`). Three P1 bugs +share this root (audit `specs/ISSUES-2026-07-07.md`): + +- **BUG-1** — the prompt is written to stdin on the calling thread *before* the reader + threads and watchdog exist, so a large prompt (the documented "use stdin to avoid + ARG_MAX" path) deadlocks against a full stdout pipe with no timeout armed. +- **BUG-2** — on timeout the serve layer emits a `run_timeout` event but does not stop the + run or close the response; the record is set `Idle` while the run keeps executing, so a + new turn passes the busy check and two runs execute on one session, corrupting the + session file. +- **BUG-4** — `child.kill()` targets only the direct child; grandchildren (tools the agent + spawned) are orphaned and keep running, and if any inherited the stdout/stderr write end + the reader threads never see EOF and the run *never returns*. + +## Decision + +**1. stdin is written off the calling thread, after readers exist and the watchdog is +armed.** The subprocess Transport spawns the stdout/stderr reader threads first, then +writes the prompt on a dedicated writer thread (dropping `ChildStdin` to signal EOF), and +the run timeout is armed before the write can block. This removes the BUG-1 deadlock. + +**2. Cancellation is a first-class primitive.** A single cancel token is threaded into a +run and can be triggered by (a) the run timeout, (b) client disconnect, and (c) a future +explicit interrupt — one mechanism, not three ad-hoc kill paths. This token is the seam +that ARCH-3's `ControlHandle` later subsumes; it is built now to fix the P1s and is not +throwaway. + +**3. Termination escalates over the process group.** Agent CLIs are spawned in their own +process group (`setsid` / `process_group(0)`). Cancellation sends `SIGTERM` to the group, +waits a short grace (~3s) so a well-behaved CLI can flush a session/checkpoint, then +`SIGKILL`s the group. `kill_on_drop(true)` backstops dropped handles. Killing the *group* +reaps grandchildren and guarantees the pipes reach EOF, so the run always returns. + +**4. On termination the run reaches a terminal state and the response closes.** The record +becomes `Failed`/`Closed` (never `Idle`), the channel is closed so both sync-drain and SSE +clients unblock promptly, and a terminal error frame (e.g. `run_timeout`) is the last thing +the client sees — no late frames from a zombie run, and the busy check cannot admit a +second concurrent run on the same session. + +## Consequences + +- BUG-1, BUG-2, BUG-4, and the client-disconnect leak in BUG-7 are fixed by one shared + mechanism rather than four point patches. +- The timeout test must exercise a *real* slow run, not the current instant stub, or the + behaviour stays unverified. +- The cancel token is forward-compatible with the `Session`/`ControlHandle` trait from the + ARCH-3 direction; when that lands, cancellation becomes one control op among several + (interrupt, set-permission, disconnect) over the same seam. +- Process-group spawning is Unix-shaped (`setsid`); the Windows path (job objects) is noted + as follow-up but not required for the primary sandboxed-Linux deployment. diff --git a/docs/adr/0015-two-agent-registries-deploy-layout-vs-runnable-backend.md b/docs/adr/0015-two-agent-registries-deploy-layout-vs-runnable-backend.md new file mode 100644 index 0000000..e1839ee --- /dev/null +++ b/docs/adr/0015-two-agent-registries-deploy-layout-vs-runnable-backend.md @@ -0,0 +1,63 @@ +# Two agent registries: deploy-layout vs runnable-backend + +## Status + +accepted (resolves audit ARCH-2; builds on [0008](0008-backend-identity-enum-transport-trait.md)) + +## Context + +Agent identity/config was spread across three tables that had silently diverged +(audit `specs/ISSUES-2026-07-07.md`, ARCH-2): + +- `src/core/agent.rs` `EXTRAS` — {install_url, requires_cli, arg_placeholder, folder}; + keys include `cursor-agent`, `qwen`, `windsurf`, `copilot`. +- `src/models/config.rs` `default_agents` — {name, folder, install_url, requires_cli, + output_format, output_dir, arg_placeholder, extensions}; key `cursor`. +- `aikit-sdk/src/lib.rs` `AgentEntry` — {commands, skills, subagents, instruction_file}; + key `cursor-agent`. + +The divergence was not merely stale values. The tables disagree on the **set of agents** +and their **keys** (`cursor` vs `cursor-agent`), carry **different fields**, and use the +same field name for **two different questions**. `requires_cli` is the clearest example: +`core/agent.rs` says gemini `requires_cli: true` (gemini is an external CLI we spawn to +*run* an agent — correct in the runnable sense), while `models/config.rs` says `false` +(asking a different question — whether *scaffolding* `.gemini/` files needs the CLI +present). Merging into one fatter table (the audit's original "one registry" framing) +would preserve the tangle and force us to adjudicate conflicts that only exist because two +concerns share a name. + +## Decision + +Split by responsibility into **two registries joined by one canonical key**: + +1. **Deploy-layout registry** — for *every* agent, including deploy-only ones (copilot, + windsurf) that are never spawned. Owns where an agent's files live: `folder`, + command/skill/subagent directories, `instruction_file`, `arg_placeholder`, + `output_format`, `install_url`. The SDK `AgentEntry` table is the most complete and + becomes the single source; `core/agent.rs::EXTRAS` and + `models/config.rs::default_agents` are deleted and their unique fields folded in. + +2. **Runnable-backend registry** — the existing closed `Backend` enum (ADR 0008): + claude, codex, gemini, opencode, cursor, aikit. This owns "can we spawn/drive this as + an agent." `requires_cli` **disappears as a stored field** — it is *implied*: every + external Backend requires its CLI, `aikit` does not, and deploy-only agents are not + Backends at all. + +3. **One canonical key per agent** across both registries (resolve `cursor` vs + `cursor-agent`), so a config value, a Backend, and a deploy layout always refer to the + same identifier. + +## Consequences + +- The value conflicts dissolve rather than being adjudicated: gemini is a runnable Backend + (CLI trivially required) *and* has a deploy layout — never the same fact, so they can no + longer disagree. +- The `cursor`/`cursor-agent` key mismatch (a real routing bug where a configured `cursor` + missed the SDK's `cursor-agent` row) is closed by the canonical-key rule. +- Adding or changing an agent touches one deploy-layout row (and, if runnable, the Backend + enum) — not three tables in two crates. +- Deploy-only agents (copilot/windsurf) are representable without pretending they are + Backends; runnable-only concerns stop leaking into the deploy table. +- Lands in Phase 2 of the remediation, alongside the ARCH-1 install/fetch unification; + the stringly per-agent `match` blocks in `aikit-sdk/src/lib.rs:88-114` become fields on + the deploy-layout registry. diff --git a/docs/adr/0016-serve-emits-canonical-events-no-streamframe-no-back-compat.md b/docs/adr/0016-serve-emits-canonical-events-no-streamframe-no-back-compat.md new file mode 100644 index 0000000..d1df802 --- /dev/null +++ b/docs/adr/0016-serve-emits-canonical-events-no-streamframe-no-back-compat.md @@ -0,0 +1,45 @@ +# serve emits canonical agent events; StreamFrame is deleted, no back-compat shim + +## Status + +accepted (realizes [0005](0005-agent-events-are-the-shared-streaming-protocol.md); resolves audit ARCH-4) + +## Context + +`src/cli/serve/mod.rs` defines a second event vocabulary (`StreamFrame`) and lossily +re-maps the canonical `AgentEventPayload` onto it before writing SSE. The remap drops +events consumers want (token usage, reasoning, subagent activity, context compression, +step-finish — per `specs/serve-agent-fidelity-and-ndjson-parity.md`) and overloads +`ToolResult.name` with two meanings (agent-key for CLI backends, call-id for structured +ones). This contradicts ADR 0005, which established the canonical agent events as *the* +shared streaming protocol across serve, agentrt, and the optimization loop. + +serve's consumers (agentrt, the optimization loop, the chat UI) parse the current +`StreamFrame` shape today, so changing the wire is a breaking change for them. The project +stance is to **advance the design rather than preserve backward compatibility** (greenfield, +break for the better shape). A dual-vocabulary/additive option was considered and rejected +as the compatibility option it is. + +## Decision + +serve serializes the canonical `AgentEventPayload` **directly** over SSE. `StreamFrame` and +its translation layer are **deleted** — no dual emission, no deprecation window, no +back-compat shim. Consumers migrate to the canonical shape in one step. + +This lands in **Phase 2** of the remediation, **independent of and ahead of ARCH-3** (the +session-trait seam). ARCH-4 overlaps ARCH-3 in the serve file but does not depend on it: +emitting canonical events is a contained change at the serialization point, so it is not +held back to be bundled with the larger seam rewrite. + +## Consequences + +- ADR 0005 is realized on the serve surface: one event vocabulary end-to-end, no lossy + translation. +- Previously-dropped events (token usage, reasoning, subagent, compression, step-finish) + reach consumers, unblocking cost display and richer chat UI. +- The overloaded `ToolResult.name` disappears with the remap; `call_id` and agent identity + are distinct canonical fields. +- Consumers (agentrt, optimization loop, chat UI) take a one-time breaking migration in + Phase 2. This is accepted as the intended direction of travel, not a regression. +- The duplicated `UsageSource`→string map (`serve/mod.rs:425` vs `run_progress.rs:199`) is + removed in the same change.