Build the gate beneath the agent's tools - #163
Open
ahammadnafiz wants to merge 13 commits into
Open
Conversation
The tools themselves were well built — a nine-strategy replacer that refuses ambiguous and oversized matches, corrective errors showing real file content, BOM and CRLF preservation, process-group kill on cancel, file-backed output capture that cannot deadlock. What was missing was the layer underneath them: durability, containment, and honesty. One gate, applied once. `ToolPolicy` owns the workspace root, canonicalisation, the read registry, schema-driven argument coercion, command classification, the approval cache, and sandbox selection. A `Guarded` decorator wraps every tool the registry emits — Atlas's own, the SDK's, and MCP-discovered ones alike — so a tool added later inherits all of it, and installing an MCP server cannot create an unguarded path. Not losing work: - Refuse an edit to a file changed since it was read, before touching it. The precondition used to run after the write landed, so the model was told the edit was rejected while the file on disk said otherwise. - Atomic writes via temp file plus rename, preserving the executable bit. - Per-file locks keyed on the canonical path. One file reached three ways took three different mutexes, so the serialisation guarantee did not hold. Containment: - Absolutise, collapse `..`, resolve symlinks, reject anything outside the workspace root. Absolute paths used to pass through untouched, and a test asserted that as correct. - Every path field is rewritten to its canonical form before the tool runs, which retires the `CwdTool` decorator that covered three tools by hand. Approvals: - "Allow for this session" now stores the answer instead of re-prompting on the very next call. - Commands are tokenised and parsed, never substring-matched. Read-only commands skip the prompt; anything unparseable fails closed; destructive commands prompt every time and are never cached. No verdict can block — the SDK's classifier is not adopted, and its `Critical` tier would have made `gh repo fork` impossible to run while missing `rm --recursive --force /`. The sandbox is the boundary: - A runtime ladder selects the strongest enforcement the host provides: macOS Seatbelt where available, workspace containment where not. The tier in force is rendered in the composer, because silent degradation is the failure it exists to prevent. - Seatbelt policy data is vendored from Codex (Apache-2.0); the generator is ours, and passes paths as parameters so a workspace name cannot rewrite the profile. Verified against the real kernel, not asserted on profile text. Honest output: - Truncation keeps head and tail with the true omitted count. Head-only capping discarded the end of a failing build, which is where the error is. - Shell output streams into a bounded ring; a command emitting gigabytes no longer buffers whole before being thrown away. - Reads stream and decode strictly. Reading ten lines of a large file no longer allocates the file twice, and invalid UTF-8 is reported rather than turned into replacement characters the model might write back. - Three silent failures became real errors: a failed output read, a panicked directory walk reporting "no files found", and a lossy decode. Two missing tools: a persistent PTY session that survives the call, for a dev server, a REPL, an interactive installer, or a build too slow for Bash's timeout; and an image viewer, absent from the registry when the model cannot accept images rather than failing mid-turn. Structured diffs reach the UI. `ToolEnd` now carries `ToolResult::metadata` (vendored patch `tool-result-metadata-v1`, guarded at compile time), the session layer stops flattening a diff block to its path, and file-change counts come from what the tool did rather than from re-parsing its arguments. Also: a tenth replacer strategy folding typographic punctuation, and a fix for a disproportionate candidate aborting the whole ladder before a later strategy could match.
Two parallel reviews — one against the repo's standards, one against the spec — found seven things worth fixing and two the docs claimed but the code did not do. The one that mattered most: **tier 0 denied all outbound network.** The base Seatbelt policy opens with `(deny default)` and the vendored network policy grants only the supporting rights — DNS configuration, the security server, loopback sockets — because Codex injects the outbound rule from a proxy layer this deliberately did not vendor. Tier 0 is the default on macOS, so `npm install`, `cargo fetch`, `pip install` and `git push` would have failed for every user. Verified under the real kernel, before and after, and pinned by a test. **The persistent terminal was outside the gate entirely.** The shell-tool name list said "Terminal" while the registry emits "TerminalStart", so no terminal command was ever classified — and with no command and no paths, every start collapsed to one cache key. Approving `npm run dev` once auto-allowed `rm -rf ~` for the rest of the session. It also never reached the sandbox: `spawn` built `sh -c` directly, so at tier 0 a terminal session reached paths the identical Bash call was denied. Both fixed, both now tested. `TerminalWrite`'s input is classified too, because text typed into a live shell is command execution. **`List` silently listed the wrong directory.** Folding its private alias table into the shared one dropped `file_path → path`, so a model writing `file_path` got `None` and the tool walked the project root and reported that as the answer, with no error anywhere. Both alias directions now exist; the schema guard is what keeps them from cancelling each other out. **The Apache-2.0 attribution for the vendored Seatbelt policy was not committed** — `.gitignore` swallows `*.md`, so the licensed data shipped and its attribution did not. Force-added, along with the ADRs and specs the code comments reference. Also from the reviews: - Atlas's own `byok-keys.json` — the user's provider keys in plaintext — was readable at tier 0. Added to the sandbox deny list. - The image cap counted raw bytes against a limit the provider applies to the base64 payload, so a 4.5 MB PNG passed the guard and was rejected mid-turn. - Escalation now exists rather than being described: a sandbox denial asks whether to re-run unconfined, applies to that one call, and is never cached. The prompt says the command restarts from the beginning, because it does. - A leaked `busy` flag could make a terminal session permanently un-evictable, and therefore a process nothing reaps. Bounded by staleness. - Spill directories are per session. They were per workspace, so one session's teardown deleted another's retained output. - `vendor/UPSTREAM.md` records the pinned revision and all five patches. Two had no guard constant; a patch without one is the patch a re-vendor drops silently. - CI gained a macOS job. The tier 0 suite is `cfg(target_os = "macos")` and the crates matrix is ubuntu-only, so the only thing establishing that the sandbox confines anything never ran. - `CodeSearch` and `ExaSearch` came back. The spec defers them to a searchable catalogue that does not exist yet; dropping them removed a capability instead. Two doc claims corrected rather than left standing: the guard's header listed a sandbox-denial step it cannot perform (it does not spawn the process), and the coercion module claimed to run once when Atlas-owned tools also call it. Both now say what is true. The ladder's tier 2 is documented as unreachable, because the setting it describes does not exist. Dead code removed: the string-in/string-out capper had no production caller — capping has to happen while output arrives — and two private alias tables and a registry helper went with it. Three remaining gaps are recorded in the spec rather than quietly closed: atomic writes cover Edit and not the four SDK-owned writers, the deferred tool tier does not exist, and tier-per-model waits on the evaluation matrix.
A one-line edit to one file cost ~71k tokens of context and sixteen tool calls. Two of the causes are in the layer beneath the tools. **A repeat `Read` returned the whole file again.** The gate already recorded what every file looked like when it was read, and said nothing: six reads of a 2000-line file put six copies in the conversation at roughly 24k tokens each. An identical call — same tool, same canonical path, same range — of a file that still matches the snapshot taken when it was answered now returns a stub naming the ways forward: `offset`/`limit` for a different part, `Grep` to search, `Edit` to change it. A model told "no" without being told "instead" reaches for `cat`, which costs the same and skips the registry. The answered-reads record carries its **own** snapshot rather than consulting the read registry, because a write *refreshes* that registry — a read taken after an edit would have looked unchanged and hidden the model's own change from it. Only `Read` is short-circuited; `Grep` and `List` return a fraction of a file each, so suppressing them would trade few tokens for a model confused about why its search returned nothing new. **`ExaSearch` was registered for everyone and runs for almost nobody.** It reads its key from `EXA_API_KEY` and errors at call time without one. Its schema is the largest in the registry, and the whole tool list is re-sent on every request of every turn, so an unusable tool cost ~6k tokens across a sixteen-call turn. Registered only when its key is set: the structured tier goes from ~3,050 to ~2,650 tokens per request for everyone who has not set one. `CodeSearch` stays unconditional — it is BM25 over the working tree with no index, key or network, and its ranked snippets with line numbers often answer what would otherwise cost a whole-file read. **Transcript rows kept the last two path segments**, so `crates/atlas-cersei/src/lib.rs` and `crates/atlas-agents/src/lib.rs` both rendered as `src/lib.rs`. The segments nearest a file — `src`, `lib`, `components` — are the ones least likely to tell two files apart, and a turn full of `Read src/lib.rs` said nothing about which file was read. Keep the leading directories and the filename, elide the middle. Each new test fails without its fix, including the edit-visibility one.
`Edit`, `MultiEdit`, `ApplyPatch` and `Write` were four overlapping ways to do the same thing — 715 tokens of schema in every request of every turn, and four chances for a weak model to pick the wrong one. Codex's entire file-mutation surface is a single `apply_patch` at ~171 tokens. **`MultiEdit` folds into `Edit`.** Its schema was `Edit`'s with an array around it, for an identical operation, so it is now an optional `edits` field: applied in order, each against the result of the last, and written only if every one succeeds. That is not only subtraction — batched edits now inherit the ten-strategy replacer, the atomic write and the structured diff the SDK's version had none of, and a failure names its position and says nothing landed. **`ApplyPatch` is registered only in the shell-first tier**, where it is the one way to change a file without hand-composing shell redirection — the arrangement Codex ships. In the structured tier `Edit` and `Write` already cover its ground with better errors. **That closed a containment hole.** The guard extracted patch paths using the *Codex* dialect (`*** Add File:`) while the registered tool parses **unified diff**, so it found no paths at all — which reads to the rest of the gate as "this call touches nothing": no containment, no freshness check, and one shared approval key for every patch. A unified diff could write anywhere on disk. `patch_paths` now understands both dialects, which is what protects any future or MCP-provided patch tool rather than just the one that shipped. **Descriptions carry steering, not prose.** `Bash`, `Read`, `TerminalStart` and `TerminalWrite` kept every line that addresses a real failure mode — the `N: ` prefix, `cd` not persisting, Grep-instead-of-reading-whole — and lost the rest. One line was removed as false and one kept after checking: cersei dispatches tool calls through `join_all`, so "ask for several files in one message" is real advice and the cheapest token win available. Together: 17 tools / 12,213 B → 14 tools / 8,593 B per request, ~2,150 tokens. `the_tool_list_stays_within_its_context_budget` fails when the list grows again, and names the largest tools when it does, so the cost of a new tool is argued for in review instead of appearing silently in every user's context window.
Two parallel reviews — repo standards, and against the spec — found three correctness bugs, one of them mine from an hour earlier, plus a test that had been asserting nothing for as long as it existed. **The shell-first tier lost its only editor, and I broke it.** Containing patch paths made the read-before-edit precondition *reachable* for the first time in a tier that has no `Read` tool — so every patch that updated a file was refused with "call Read first", and nothing in that tier could. The root cause was older: D3 has always claimed "a shell read that names a path registers it", and that half was never built. The test covering it called `record_read()` directly and asserted on the result, so it proved the registry works and nothing about the mechanism. Shell reads now register through `classify::read_paths`, which yields a command's file-like arguments **only** when the whole command tokenises and classifies read-only. A write can never vouch for its own freshness, and an unparseable command registers nothing: an unrecorded read costs one extra call, where a wrongly recorded one costs the user's work. The test now runs `cat` and asserts the precondition is unsatisfied before it and satisfied after. **`Edit` silently dropped input.** With both `edits` and `old_string`/ `new_string` present it discarded the flat pair, and a top-level `replace_all` alongside `edits` was ignored outright — no error, contradicting the batch's own all-or-nothing promise. Both are refused now rather than resolved: the model would otherwise learn a replacement vanished by reading the file back, if it read it back. An empty `old_string` inside a multi-edit batch means "create this file", which cannot combine with edits that must match its contents; it said "File not found" and now explains itself. **The budget test was environment-dependent** — red for anyone with an `EXA_API_KEY` — and scored a serialisation failure as zero bytes. It now excludes the env-gated tool on both sides of the comparison and panics rather than passing silently, and the recorded numbers no longer conflate two commits: 10,626 B/16 tools → 8,593/14 for this change, 12,213/17 → 8,593/14 for the branch. **Patch containment now matches the applier exactly** — the `+++` side, `b/` stripped, nothing else — instead of also collecting `---`, which could only produce false denials on a diff taken against a file outside the workspace. Two things about the SDK's patch tool are recorded in the spec while they are true: it never compares the context lines it is given, splicing by line number instead, so a stale patch corrupts silently rather than failing — which is why read-before-edit is load-bearing for it. And its description tells the model it supports deleting files when it contains no delete path at all, so the structured tier gave up no deletion capability, because there was none. Also: stale doc comments in `tiers.rs` and `policy.rs` that still named `MultiEdit`; the spec's claim to protect "any future or MCP-provided patch tool" narrowed to what the code does; `in_batch` renamed to `batch_failure` since it formats rather than tests; the single-edit path no longer clones the whole file; and `ATTRIBUTION.md` names the two error strings that are Atlas's own. Declined: `Input::ops` returning a `String` error was flagged as primitive obsession, but `ToolResult::error` takes a `String` and every other message in `errors.rs` is one — a bespoke type here would be the odd one out.
A turn reads as a sequence: "I'll check the config" → reads it → "that
confirmed the bug" → edits → "fixed, running tests". The store already splits
a turn into one message per block so it can be replayed in that order —
`ChatMessage.mode` exists for exactly that.
The projection threw the order away. `projectRows` accumulated **every** tool
call in the turn into one array and spliced the whole block at the position of
the first one, so a reader saw every action followed by every paragraph
explaining them:
"First I'll check the config." "First I'll check the config."
<Read> <Read>
<Edited> ← hoisted "That confirmed the bug."
<Ran> ← hoisted "Fixed. Running the tests."
"That confirmed the bug." <Edited>
"Fixed. Running the tests." <Ran>
"All green." "All green."
Tool calls now group by **run** — a maximal stretch with no prose or thinking
between them — and each run is emitted where it happened. Density was the
reason for folding and folding still buys it; hoisting never did.
Each run gets its own block, so they open independently instead of one
all-or-nothing "Show tool calls" per turn, and while a turn is live the
trailing run stays open as its progress report while finished runs fold.
The block also says what it did rather than how many times it did something:
"Ran 2 shell commands, read 1 file" instead of "Tool calls · 3 calls". A count
alone tells the reader nothing about whether the block is worth opening, which
is the whole point of folding it. Three kinds are named before the rest
collapse to "+N more" — hiding a kind behind a number is worse than the line
being slightly longer.
Measured while looking for the cause: `projectRows` is not a bottleneck.
0.021 ms/frame on a short thread, 0.277 ms on a 760-message session and
0.546 ms at 1520 — against a 16.6 ms frame budget, with structural sharing
already keeping row identities stable.
Asked "how to run the dev", the agent read `package.json`, answered correctly
and completely — `npm run dev`, localhost:3000 — and then read ten more files
(layout, page, footer, header, hero, button, utils, next.config, tsconfig) and
wrote an architecture summary nobody asked for. The session went from 28.2K to
41.6K tokens after the answer was already on screen.
Nothing in the harness forced the second round; the guidance did. It opened
with "your tool calls are local and near-instant, **so reach for them freely**",
led with a section headed "Read the codebase first", said "Parallel calls are
cheap here — use them", and closed with "carry the task to a finished, verified
state". Every one of those pushes toward more calls, and not one line anywhere
said to match the effort to the question or to stop once it was answered. A
strong model resists that; the weak ones this harness exists to support do not.
A `# Proportion` section now leads, before exploration, because it has to bind
hardest:
- Read what the question needs, then stop.
- Answer the question that was asked, and only that. Never volunteer an
architecture summary or a tour of files you happened to open.
- The moment you can answer, answer and stop.
- Scale everything to the task.
And the lines pointing the other way are corrected rather than deleted: "before
changing code, read it" instead of "read the codebase first"; parallel calls are
still encouraged, but as *batching the calls you need into one step, not a
reason to make more of them*; "carry it to a finished state" now applies to a
request to change something, while a question is answered and that is the whole
task.
`the_guidance_tells_the_agent_when_to_stop` pins the stopping condition and
fails if either of the two phrasings that produced the over-reading returns.
This is prose, so a test on it is coarse — but it is behaviour-critical prose,
and the repo already guards its vendored patches by constant for the same
reason.
Streaming landed in lumps a few times a second rather than every frame, with
nobody touching the trackpad.
`useTranscriptScroll`'s `onScroll` called `markScrollHot()` unconditionally. A
`scroll` event is a *consequence* and cannot say what caused it — and the
transcript scrolls itself to the live edge on every streaming chunk, because
the follow effect is keyed on the trailing row's text length. So each chunk
marked the reader "mid-gesture", and the agent-delta flush in App.tsx dutifully
held the batch for someone who was sitting perfectly still:
t=0 flush → auto-scroll → scroll event → hot until t=160
t=0 next chunks buffer
t=16 RAF flush: hot → held, retry in 100ms
t=116 still hot → held, retry in 100ms
t=216 cold → flush → auto-scroll → hot until t=376
One update every ~200 ms instead of every frame, in a loop the stream drove
itself. The hold was written to keep a streaming re-render out of a
momentum-scroll frame, which is a real WKWebView tile-blanking problem — it was
just firing for the wrong reason essentially all of the time.
Hot is now marked from `wheel` and `touchmove` on the scroll container, which
fire only for a real reader; macOS keeps sending `wheel` through trackpad
momentum, so a fling stays marked for its whole deceleration and the hold still
does the job it was written for. A scrollbar drag or a keyboard page is not
covered and does not need to be: neither produces the sustained repaint storm
the hold exists to keep clear of.
`bindUserScrollGestures` is the seam, so the distinction can be tested against
a real element: the transcript's own `scrollTop` assignment must not mark hot,
a wheel must, and the mark must decay.
A session reached 172.8K tokens because `npm install` drew a spinner. A PTY carries the *instructions* a terminal follows, not the text it shows. A spinner frame is `ESC[1G` (cursor to column 1), `ESC[0K` (erase to end of line), one character — thousands of times. `TerminalWrite` handed that stream to the model verbatim, so polling a running dev server six times spent the context window on cursor movements describing a spinner that, rendered, is one line. It was also *destroying* output. The buffer is capped, so a chatty spinner pushed whatever came before it off the front — an `npm install` that prints a progress animation and then an error would deliver the animation and drop the error. `screen.rs` renders on the way in: the cursor moves within the current line, a newline commits it, and CR / CHA / EL / backspace are honoured so an overwrite overwrites. Colour, window titles and everything unrecognised are dropped. 5,000 spinner frames (40 KB) render to "added 312 packages". It is deliberately **not** a terminal emulator. One line at a time, no row addressing, no scroll regions, no alternate screen — and anything unrecognised is dropped rather than guessed at, because dropping a colour code loses nothing a model needed where guessing at cursor geometry would corrupt the transcript silently. `ESC[2J` clears only the current line for the same reason: losing a build error to a `clear` is worse than showing stale text. Verified end to end through a real PTY, not just against the parser: a session that emits ten spinner frames and a result line delivers the result line, with no escape byte and no surviving frame. Also: the tool-call summary lower-cased every clause after the first, which turned a tool's own name into "terminalStart 1×". Only mapped verbs are lower-cased now; an unmapped one is a tool name and keeps its casing.
Two follow-ons from the spinner that cost a session 172.8K tokens. **`Bash` shares the renderer.** It captures to a file with no TTY, so most tools turn their spinner off — but `--color=always`, `--progress` and anything drawing with `\r` do not ask whether a terminal is attached, and the cost of raw cursor movements is the same wherever they come from. The two buffers could not simply be stacked: `HeadTail` keeps the head *and* the tail, while `Screen` drops the oldest, so chaining them would have let the renderer silently eat the head the ring was trying to preserve. Instead `Bash` drains only *committed* lines, and does so on every read rather than at EOF — `pump` reads until the pipe is empty, so buffering until then would hold a whole gigabyte in the renderer and undo the flat memory the capture file buys. Draining committed lines only is what keeps a progress line rewritten across several reads collapsing to one. `Screen` gains a line cap for the same reason: a stream carrying no newline at all — `cat` of a binary, a minified bundle — would otherwise grow the line in progress without bound. Cursor columns are clamped to it, so `ESC[999999999G` cannot allocate a line nobody asked for. **A quiet session is an answer.** A live session with no new output said "(no new output)" and invited another read, so a model asked to start a dev server polled it six times: `npm run dev` never exits, so "is it done yet" has no answer it can reach by asking again. It now says that reading again will not change anything — and offers both readings, because the harness genuinely cannot tell them apart. A server that has finished starting and a process blocked on stdin are the same silence from out here; the first draft claimed the silence meant success, and the test for `read line` caught it saying so about a process waiting for input. What the harness *can* state for certain is that another identical read changes nothing, and that is the part that stops the loop.
The harness felt unpredictable because its system prompt described a different
product and then argued with itself.
Atlas appended its guidance to `build_system_prompt`'s base sections, which
advertised an **LSP tool three times** that Atlas does not register, a Bash
"background mode" that does not exist (that is TerminalStart), skills loaded
from `.claude/commands/*.md` when Atlas reads `.atlas/agent-skills`, and memory
"injected into your context automatically" when Atlas exposes it as a tool. A
model told to reach for a tool that is not there flails, and it was told three
times.
Worse, those sections contradicted Atlas head-on and came first:
base: "Never stop at surface-level answers when deeper investigation would
give better results" / "be thorough and structured, use tables, lists"
Atlas: "The moment you can answer, answer and stop" / "a one-line question
gets a one-line answer"
That is not a subtle mismatch — it is the exact behaviour reported: a question
answered from one file, followed by ten more reads and a structured
architecture essay nobody asked for. The essay was the base prompt being
obeyed. Same for todos ("ALWAYS use TodoWrite" against "skip it for simple
tasks") and for the shell ("prefer using Bash (with grep, find)" against "use
the dedicated file tools"), where the base also disagreed with itself two
paragraphs apart. A weak model handed contradictory instructions does not pick
the better one; it oscillates.
`build_system_prompt` also emitted `__SYSTEM_PROMPT_DYNAMIC_BOUNDARY__` as a
literal line. It is an internal cache marker and nothing in the SDK strips or
splits on it, so every request handed the model a nonsense token in the middle
of its instructions.
So Atlas assembles its own prompt, as Claude Code and Codex do. Replace mode
was not usable directly — it returns the custom prompt plus that marker and
discards every dynamic section — so `context::dynamic_sections` renders cwd,
git snapshot, project docs and MCP notes in the same tags the SDK used. Same
framing, different owner of the content.
**The rule that keeps it from drifting again: the prompt states policy, not an
inventory.** Tool schemas already travel with every request, so it never claims
a specific tool exists — it says the tool list is the authority on what does.
That is what makes an added or removed tool a non-event for the prompt, and it
is pinned by a test.
11,465 B (~2,866 tok) to 6,450 B (~1,612 tok), charged on every request of
every turn, with the contradictions gone rather than merely outweighed.
…guidance The prompt is now sectioned with XML tags, one kind of instruction per section, so "how to format a reply" cannot be read as "when to ask before acting". Rules carry their motivation wherever the reason is not self-evident — a model that knows why generalises the rule to cases the text did not cover. Framing is positive throughout, except the safety prohibitions where the prohibition is the point. **It is prose, not bullets, deliberately.** Prompt style carries into output style, and the complaint that began this work was a one-line question answered with a bulleted architecture essay. The prompt now asks for flowing prose and is itself flowing prose, and a test fails if a bullet reappears. New sections earn the added length: `<default_to_action>` (make the change rather than proposing it, and answer a question as the whole task), over-engineering guidance in `<changing_code>` (no abstraction before a second caller, no error handling for what cannot happen, no solution shaped to pass a specific check), worked examples in `<acting_with_care>` of what warrants asking, a prompt-injection rule in `<security>` treating tool output as data rather than instruction, and a `<response_style>` section written to stop the essays. `<use_parallel_tool_calls>` is adapted rather than copied. The published sample says "maximize use of parallel tool calls"; taken literally that fights `<proportion>`, which is exactly the class of contradiction the previous commit removed. It asks instead for the calls you already need to be batched, and says plainly that this is not a reason to make more. **Tool descriptions document what they return** — Bash's interleaved output, exit status, and truncation-with-spill; Read's numbered lines and its unchanged-file note; Edit's diff. And `Edit` shows both accepted argument shapes as worked examples. A JSON Schema states what is structurally valid, never which combination is meant, and `Edit` taking either a flat replacement or an `edits` array is precisely that ambiguity — one the model gets wrong often enough that over-specified input needed an explicit refusal. The examples are in the description text rather than a schema `examples` keyword on purpose. Atlas is BYOK: a restricted function-calling schema, such as Gemini's, may reject an unknown keyword and break tool calls outright, where a description is always just a string. Both budgets were raised once, deliberately, with the reason recorded at the constant: the prompt 7,000 to 9,200 B, the structured tool list 8,800 to 9,300. Costs ~240 tok per request for the tool additions and ~620 for the prompt; the prompt is still ~640 below the 11,465 it replaced. Not applied: `defer_loading` (Tool Search) and `allowed_callers` (Programmatic Tool Calling) are Claude API beta fields with no equivalent in the SDK's tool trait. And the advice to dial back assertive language assumes a model that over-triggers, which is the opposite of the weak BYOK models this harness exists to carry.
The SDK tool had one output shape — every match as `file:line:content`, capped at 250, with nothing saying the cap had been hit. - `output_mode` defaults to `files`: matching paths with a match count each, densest first. On this repo `policy` costs 500 tokens that way against 8,279 as match lines. - `content` mode carries `context` lines (default 2) around each match, grouped by file, with overlapping windows merged so a shared line is never paid for twice. Locating an edit site in guard.rs cost 8,329 tokens as grep-then-whole-file-Read; it now costs 435. - Caps report the true total. The scan runs to 20,000 matches, far above any cap displayed, so the walk completes, the count is exact and the same query twice gives the same answer. The primitive's own cap quits the parallel walk on a raced counter and sorts afterwards, so a partial result arrived looking complete. Grep deliberately does not call `record_read`: context lines are a window, not the file, and letting them satisfy read-before-edit would reopen the staleness hole the shell-read path already had to close. Tool-list budget 9,300 -> 9,900 B; the new description costs 503 B (~126 tokens per request) over the SDK tool it replaces, which is what those numbers buy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Builds the layer beneath the native agent's tools: one gate every tool call passes through, an OS sandbox on macOS, atomic writes, streaming output, and the two tools that were missing (a persistent terminal, an image viewer).
Implements
plans/atlas-tool-layer-spec.mdin full and the tool-resident half ofplans/atlas-agent-harness-spec.md. Both specs are in this PR, with a Known gaps section stating what did not land.Why?
Users report that the tools don't work. Reading the sources end to end, the reports are real but the cause isn't what it looks like: the tools themselves are well built. The replacer runs nine matching strategies, refuses ambiguous matches and refuses one whose span is wildly larger than asked for. The errors show real numbered file lines. Edit preserves BOM and CRLF. Bash runs its child in its own process group so cancelling kills grandchildren, and treats a non-zero exit with output as a success because that's normal for grep and test runners. Several of these beat the equivalents in the harness we considered porting.
What was missing was underneath them. Four defects would each reach a user as "the tools don't work" while being something else:
And the agent ran model-authored shell commands at the desktop app's full privilege, with file tools that accepted absolute paths and unnormalised
..— with a test asserting that pass-through as correct.How?
One gate, applied once.
ToolPolicyowns the workspace root, canonicalisation, the read registry, schema-driven argument coercion, command classification, the approval cache, and sandbox selection. AGuardeddecorator wraps every tool the registry emits — Atlas's own, the SDK's, and MCP-discovered ones alike — so a tool added later inherits all of it, and installing an MCP server cannot create an unguarded path.tests/tool_gate.rsasserts that over the whole registry rather than over a list someone has to remember to update.Not losing work. An edit to a file changed since it was read is refused before anything is touched. Writes go through a temp file and a rename, preserving the executable bit. Per-file locks are keyed on the canonical path — one file reached three ways used to take three different mutexes, so the serialisation guarantee didn't hold.
Containment. Absolutise, collapse
.., resolve symlinks, reject anything outside the root. Every path argument is rewritten to its canonical form before the tool runs, which retires theCwdTooldecorator that covered three tools by hand.Approvals proportional to risk. Commands are tokenised and parsed, never substring-matched. Read-only commands skip the prompt; anything unparseable fails closed; destructive commands prompt every time and are never cached. No verdict can block — the SDK's classifier is not adopted, and its
Criticaltier would have madegh repo forkimpossible to run while missingrm --recursive --force /entirely.The sandbox is the boundary. A runtime ladder picks the strongest enforcement the host provides — macOS Seatbelt where available, workspace containment where not — and the tier in force is rendered in the composer, because silent degradation is the failure it exists to prevent. Policy data is vendored from Codex (Apache-2.0); the generator is ours and passes paths as parameters so a workspace name can't rewrite the profile.
Honest output. Head and tail with the true omitted count. Shell output streams into a bounded ring, so gigabytes never buffer. Reads stream and decode strictly — reading ten lines of a large file no longer allocates it twice, and invalid UTF-8 is reported rather than turned into replacement characters the model might write back. Three silent failures became real errors.
Two new tools. A persistent PTY session that survives the call, for a dev server, a REPL, an interactive installer, or a build too slow for Bash's timeout. And an image viewer, absent from the registry when the model can't accept images rather than failing mid-turn.
Structured diffs reach the UI.
ToolEndnow carriesToolResult::metadata(vendored patch, guarded at compile time), the session layer stops flattening a diff block to its path, and file-change counts come from what the tool did rather than from re-parsing its arguments.Context cost (third commit)
Running this against a real model surfaced a problem the gate did not address and, in one
dimension, made worse: a one-line edit to one file cost ~71k tokens of context across sixteen
tool calls. Three fixes, measured:
Readreturned the whole file againExaSearchregistered without a key it needslib.rsrendered as the same rowsrc/lib.rscrates/atlas-cersei/…/lib.rsThe read registry already knew a file had been read and had not changed, and said nothing — so six
reads of one file put six copies in the conversation. An identical call (same tool, same canonical
path, same range) of a file that still matches the snapshot taken when it was answered now returns a
stub naming the ways forward instead. The answered-reads record carries its own snapshot rather
than consulting the read registry, because a write refreshes that registry: keyed on
reads, aread taken after an edit would look unchanged and hide the model's own change from it. Only
Readis short-circuited —
GrepandListreturn a fraction of a file each.ExaSearcherrors at call time withoutEXA_API_KEY, and the tool list is re-sent on every requestof every turn, so an unusable tool was ~6k tokens across that turn.
CodeSearchdeliberately staysunconditional: it is BM25 over the working tree with no index, key or network, and its ranked
snippets with line numbers often answer what would otherwise cost a whole-file read.
Not fixed here, and worth naming:
Read's 2000-line default and theGrep→Readhandoff bothaffect volume, but changing either could produce more calls rather than fewer. Neither should move
without measurement against real transcripts.
One way to change a file (fourth commit)
Edit,MultiEdit,ApplyPatchandWritewere four overlapping ways to do the same thing: 715tokens of schema in every request of every turn, and four chances for a weak model to pick the
wrong one. Codex's entire file-mutation surface is a single
apply_patchat ~171 tokens.MultiEditfolded intoEdit. Its schema wasEdit's with an array around it, for anidentical operation, so it is now an optional
editsfield — applied in order, each against theresult of the last, written only if every one succeeds. Not only subtraction: batched edits now
inherit the ten-strategy replacer, the atomic write and the structured diff the SDK's version had
none of, and a failure names its position and says nothing landed.
ApplyPatchis registered only in the shell-first tier, where it is the one way to change afile without hand-composing shell redirection — the arrangement Codex ships. In the structured
tier
EditandWritecover its ground with better errors.Bash,Read,TerminalStartandTerminalWritekept every line that addresses a real failure mode (the
N:prefix,cdnot persisting,Grep-instead-of-reading-whole) and lost the rest. One line was cut as false; one was kept after
checking that cersei really does dispatch tool calls through
join_all, which makes "ask forseveral files in one message" the cheapest token win available.
the_tool_list_stays_within_its_context_budgetfails when the list grows again and names thelargest tools when it does, so the cost of a new tool is argued for in review instead of appearing
silently in every user's context window.
A containment hole, found by doing the above
The guard extracted patch paths using the Codex dialect (
*** Add File:) while the registeredSDK tool parses unified diff (
+++ b/path). It therefore found no paths — which reads to therest of the gate as "this call touches nothing": no containment, no freshness check, and one shared
approval cache key for every patch.
working_dir.join(target)with no normalisation meant a patchcould write anywhere on disk.
The existing
no_registered_tool_can_reach_outside_the_workspacemissed it because it onlyexercises tools declaring
file_path, andApplyPatchdeclares onlypatch. The existing unittest passed because it used the dialect the registered tool does not accept.
patch_pathsnow understands both dialects — which is what protects a future or MCP-provided patchtool, rather than just the one that happened to ship.
a_patch_cannot_write_outside_the_workspaceis the regression test; it fails against the previous commit, verified.
Review, and what it found (fifth commit)
Two parallel reviews — repo standards, and against the spec — found three correctness bugs plus a
test that had been asserting nothing. All fixed in
4ed8329.The worst was mine, from an hour earlier. Containing patch paths made the read-before-edit
precondition reachable for the first time in the shell-first tier — which has no
Readtool. Everypatch that updated a file was refused with "call Read first", and nothing in that tier could satisfy
it. Root cause was older: D3 has always claimed "a shell read that names a path registers it", and
that half was never built. The test covering it called
record_read()directly, so it proved theregistry works and nothing about the mechanism.
Shell reads now register through
classify::read_paths, which yields a command's file-like argumentsonly when the whole command tokenises and classifies read-only — so a write can never vouch for
its own freshness, and an unparseable command registers nothing. The failure mode is an unrecorded
read costing one extra call, never a write wrongly believed fresh. The test now runs
catandasserts the precondition is unsatisfied before it and satisfied after.
Editsilently dropped input when both shapes were given, and ignored a top-levelreplace_allalongsideedits. Both refused now rather than resolved — the model would otherwiselearn a replacement vanished by reading the file back, if it read it back.
EXA_API_KEY) and scored aserialisation failure as zero bytes. Both fixed, and the recorded numbers no longer conflate two
commits.
+++side,b/stripped — instead ofalso collecting
---, which could only produce false denials.Two things about the SDK's patch tool are now recorded in the spec while they are true: it never
compares the context lines it is given, splicing by line number, so a stale patch corrupts silently
rather than failing — which is why read-before-edit is load-bearing for it. And its description tells
the model it supports deleting files when it contains no delete path at all, so the structured
tier gave up no deletion capability, because there was none.
One finding was declined with a reason:
Input::opsreturning aStringerror was flagged asprimitive obsession, but
ToolResult::errortakes aStringand every message inerrors.rsisone — a bespoke type would be the odd one out.
Design decisions worth arguing with
cargo,npmandgo, and a sandbox users turn off protects nobody. The deny list covers SSH keys, cloud credentials, keychains, browser profiles, and Atlas's own stored provider keys.Forbiddenoutcome in the classifier and no code path that produces one.Tests and checks run
cargo test—atlas-cerseicargo test—atlas-agents,atlas-memory,atlas-codeindex,atlas-review,atlas-terminalcargo test—src-tauribun run lint/format:check/typecheck/test/buildcargo clippy—atlas-cerseicargo test—atlas-agentsafter the context-cost commitEach test added for the context-cost commit was verified to fail without its fix — including
the edit-visibility one, by temporarily keying the answered-reads set on the read registry.
New suites:
tests/tool_gate.rs(the first test of the real permission path — every previous tool test used a permit-everything policy),tests/sandbox_tier0.rs(behaviour verified against the real kernel, not asserted on profile text),tests/vendor_patch_guard.rs.CI gains a macOS job:
sandbox_tier0.rsiscfg(target_os = "macos")and the crates matrix is ubuntu-only, so the only thing establishing that the sandbox confines anything never ran.Review
Ran a two-axis review (repo standards, and against the spec) before this was ready. It found seven real problems, all fixed in the second commit — the worst being that tier 0 denied all outbound network, which would have broken
npm installandcargo fetchfor every macOS user, and that the persistent terminal was outside the gate entirely: unclassified, unsandboxed, and sharing one approval cache key, so approvingnpm run devonce auto-allowed anything for the rest of the session. It also caught that the Apache-2.0 attribution for the vendored Seatbelt policy wasn't committed —.gitignoreswallows*.md, so the licensed data shipped and its attribution didn't.Each fix has a regression test.
Not verified
Not run in a window. This needs a BYOK key and a live model, so the end-to-end agent flow is untested by me — please exercise it before merging. The frontend can't be checked in a browser either: outside Tauri,
window.__TAURI_INTERNALS__is undefined, everyinvokethrows, and the app renders its error boundary. I confirmed that rather than assuming it.The composer's enforcement note in particular is the piece most worth a look: it renders inside the permission-mode panel and I've only type-checked it.
Known gaps, stated rather than quietly closed
Editonly.Write,MultiEdit,ApplyPatchandNotebookEditare SDK-owned and write in place. Fixing them means vendoringcersei-tools— a dependency commitment of the kind ADR-0001 weighs explicitly, not something to fold into this change.Out of scope and untouched, per the harness spec's own ordering: the wire adapter, the BYOK evaluation matrix, the frontend engine-identity collapse, and the session-actor half of turn integrity.
Checklist