Repository navigation
[5/5] Add protection to the Goose harness plugin - #839
Conversation
72341b2 to
49d337e
Compare
445e303 to
25e442d
Compare
5d8eafb to
e595973
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5959730b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| protectionContext: async () => { | ||
| if (!conversation) | ||
| throw new Error("The native Goose session is unavailable"); | ||
| const process = await (entry.worker ?? this.goose).get(); | ||
| const { data } = (await process.request( | ||
| "_goose/unstable/session/export", | ||
| { sessionId: conversation, format: "json" }, | ||
| )) as { data: string }; | ||
| return JSON.parse(data); |
There was a problem hiding this comment.
Cancel protected tool calls before acknowledging retirement
When a protected Goose tool call is already waiting in AgentProtection.check, retire() can finish while this detached handler is still running: AcpProcess.close() closes the MCP server without cancelling its JavaScript handler, and this appClient supplies neither a retirement check nor a signal. After the policy save invalidates the handle, protect() can therefore finish against the captured old binding and dispatch the tool; paths such as file attachments call desktop.readFile directly, so handle revocation does not stop them. Tie the client to the entry's retirement lifetime and abort or await pending tool calls before acknowledging retirement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Addressed in fe3277d. The tool client is created with the agent entry's lifetime signal, and retire() aborts that lifetime before shutting the sessions and worker down. protect() passes the signal to the protection check and rechecks it afterwards, so a pending protected tool call is refused, not dispatched against the old binding.
| } finally { | ||
| signal.removeEventListener("abort", cancel); | ||
| await goose.kill(); |
There was a problem hiding this comment.
Wait for the temporary review worker to exit
HostProcess.kill() resolves after requesting termination, while the separate exited promise signals that the process group is actually gone. This finally therefore lets every successful or failed model completion return while its credentialed protected Goose worker may still be alive, so repeated reviews can accumulate overlapping workers and teardown is not complete. Await goose.exited after killing it, as the other protected Goose lifecycle paths do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Addressed in fe3277d. The review's finally now awaits goose.exited after goose.kill().
5c66c96 to
b560bfb
Compare
e595973 to
068f6ee
Compare
b560bfb to
e54b2ed
Compare
5c34d7b to
b461b17
Compare
2113f87 to
a201b00
Compare
b461b17 to
8313861
Compare
|
🤖 Live verification of the current Buzz stack: Test Agent (Claude) and Testing Codex ran in separate conversations. Approving Codex cleared only its card while Claude stayed paused. Codex posted the requested summary and reached native completion without repeating an approved-source warning. The three input decisions covered distinct test messages, including two earlier unapproved messages restored with the chat. Approve Codex while Claude stays paused — 50 seconds 01-two-agent-approval.mp4Codex posts the approved summary — 85 seconds 02-codex-reply.mp4Both clips are continuous trims of the actual Buzz window recording. |
a201b00 to
e11b332
Compare
47b80eb to
c13fedc
Compare
871d01e to
8140250
Compare
c13fedc to
1b4cd99
Compare
loganj
left a comment
There was a problem hiding this comment.
🤖 Round 2 from Larry, Auto and Cerveau: an independent review of 1b4cd9990 (one commit on 8140250ef), with companion 46e5fc5 and the Goose pin unchanged (12283408). We checked each round-1 finding against the code, not against the replies, then reviewed the new code. Evidence is source review plus local probes: the real TS classes on controlled fake native processes, and Buzz's bundled stock goose-acp 1.57.0 in a fake HOME. No packaged sandbox run and no live inference. CI run 41662832192 passed at this head.
Verdict: comment, not approve. Round-1 must-fix 4 is fixed. Must-fix 1, 2 and 3 are partly fixed. There are six must-fix items.
Round-1 must-fix
- Review verdicts through
~/.config/goose: partly fixed. The companion now makesconfig.yaml,secrets.yamlandcustom_providersread-only to contained workers, but only the ones that exist when the plugin starts (engine.py:936-941: "Only files that exist, never created here"), and it records them once. Goose's OAuth token folders stay writable on purpose (engine.py:957). Must-fix A and B go through these gaps. - Retirement does not wait for opening sessions: partly fixed. Pending successful opens are now awaited and closed. Must-fix E is an open that fails after
session/new. - A failed retirement is forgotten: partly fixed. Failed entries are put back, and ordinary session-close failures still block. Must-fix C and D still lose the worker process.
close(strict)acknowledges an in-flight close: fixed.process.ts:326-338shares the in-flight close promise, keeps the failure for strict callers, and lets the close finish after native exit.
Must-fix (6)
A. A worker-created custom_providers replaces a built-in provider and runs a command outside the sandbox (index.ts:42). Reproduced with stock goose-acp 1.57.0.
B. A worker can change the GitHub Copilot review endpoint through the writable token cache. Traced in Goose source only (githubcopilot.rs:178-197,384-399). Inline on docs/agent-protection.md.
C. A failed worker kill is forgotten (sessions.ts:93). Reproduced.
D. A failed initialization drops a worker that is still running (sessions.ts:77). Reproduced.
E. An open that fails after session/new gets past the new retirement barrier (agent-sessions.ts:452). Reproduced.
F. unframed() does not cover plugin notes under Goose. Same cause as #1879 must-fix D. review(text, notes) joins [text, ...notes, turnNote] (goose/runtime.ts:608-611). The code is older, but this PR turns protection on for it. With companion inspectionSources, a forged <buzz-event> in a note on a wake turn becomes a separate current-event source (2 sources; with a real event in the block it stays at 1). Core memory is safe here: Goose puts it in systemPrompt (runtime.ts:892-903), and the companion splits only role: user for buzz.goose. The #1879 fix at the place where the turn text is put together covers this one too.
Details, failure scenarios and fixes are in the inline comments.
Questions and advice
- Round-1 question, not answered: keychain prompts for the locally built engine.
docs/agent-protection.mdand the companion docs do not mention the keychain. A denied prompt now leads to thesecrets.yamlfallback below. - Advice: a worker-made
secrets.yamlis used when the keychain has no entry or its prompt is denied. Goosebase.rs:961-976,1205-1242; keyring 3.6.3 maps a denied prompt toPlatformFailure, which Goose treats as "keyring unavailable". Ifsecrets.yamldid not exist when the plugin started, a worker can create it and swap keys. We found no secret keys that set a host, so the endpoint does not change. The fix for A (create the files before protecting them) also closes this. - Advice: the protected list is recorded once. A Goose config created after the plugin starts stays writable until restart. If
config.yamlis missing, a worker can make it a symlink, and Buzz's uncontained settings save follows one link (base.rs:603-636) and rewrites the target. The worker does not choose the bytes, so this can damage a file but cannot run code. - Advice:
sync()re-add. Lazy re-creation can still lose theprotectedmarker, but we found no current bypass: delivery binds the protected handle before launch, and a reset with no live session does nothing. - Note: the protected agent's own engine also loads
custom_providers, so with A a sibling worker can send that agent's inference to an attacker. Checks still run before each model request, so this exposes data but does not skip a check. The fix for A covers it. - Tests: new tests cover failed retirement (protected and ordinary), strict close refused or failed, shared Goose shutdown after the last retirement, the replacement worker after exit, the protected read, and partial model-list failure. Still not covered from round 1: review cancel or deadline, and review output that does not match the schema. C, D and E each need a held-exit test, and A needs a setup test with a
custom_providersfolder created after start.
Checked fine
- No uncontained preflight runs the pinned engine.
goose-protectedstarts only throughprotection.spawn(sessions.ts:51) and theProtectedSpawnreview path, which both add protection (service.ts:294-330). The capability probe (goose.ts:190-200) runs inside the protected launch and fails closed on anything but version 2. Setup, listing and history use the stock engine (that is where A's command runs). - The protected env scrub (
sessions.ts:57-67).read-protectedruns contained, withprocessProtection. - Goose config layers:
/etc/gooseis owned by root, andGOOSE_ADDITIONAL_CONFIG_FILES/GOOSE_PATH_ROOTcome only from the host env. Hooks are off, extensions are limited to developer plus HTTP MCP, and toolshim and own-loop providers are refused (patch). The companion install refuses symlinks in its private release folder. - The
protectionContextfallback now requires the dedicated worker (runtime.ts:810-812). The export now has a deadline (runtime.ts:813-816); a failure denies the tool and asks for a kill. - Unprotected Goose: the capability probe and naming flag run only when
protected; the abort path kills and waits for exit. Model listing usesallSettled, which fixes the round-1 "one bad provider empties the list" advice. Normal ACP write errors do not remove sessions or mark the process as exited. Idle and reconfigure closes keep strict failure.
| const setup = new GooseSetup(desktop, () => runtime.defaultsChanged()); | ||
| ctx.agentProtection.registerModels({ | ||
| label: "Goose", | ||
| models: async () => { |
There was a problem hiding this comment.
🤖 Must-fix A (error): a worker-created custom_providers replaces a built-in provider in the protected reviewer, and can run a command outside the sandbox.
custom_providers/ usually does not exist when the security plugin starts, and the companion protects only paths that already exist (engine.py:936-941). ~/.config/goose itself is protected only as a literal (protection.rs:52-77), so a contained worker can create the folder. Goose registers custom providers after the built-ins with a HashMap insert keyed by name (declarative_providers.rs:413-429, provider_registry.rs:327, init.rs:226), so a file with "name":"anthropic" replaces the real provider.
- Review redirect. The review worker (
setup.ts:318-364→ patchedreview()→get_from_registry) sends prompts to the attacker'sbase_url, and the attacker picks the verdict. With"api_key_env":"ANTHROPIC_API_KEY", the owner's real key goes there too. - Escape. A provider with
auth.commandandmodels: []shows as configured. When the security settings page loads review models, thisregisterModelscallback callssetup.models→supported-models/listthroughbriefly→host.spawn("goose")(setup.ts:368-395,602). That is the stock engine, uncontained, and Goose runs the worker's command (command_auth.rsfetch_token,acp/server/providers.rs:502-510).
Probe: stockgoose-acp1.57.0, fake HOME,GOOSE_DISABLE_KEYRING=1.providers/listreturnedanthropicas "HIJACKED Anthropic", configured.supported-models/listran/usr/bin/touch <marker>and the marker was created. The pinned source has the same registry insert.
Fix: createcustom_providers(and placeholderconfig.yaml/secrets.yaml) before the companion protects them, or protect all of~/.config/gooseexcept the token folders that must stay writable. Refusing a custom provider that shadows a built-in name closes the redirect but not theauth.commandescape under a new name.
There was a problem hiding this comment.
🤖 Confirmed, and fixed in cef48c6 with the companion in squareup/buzz-plugin-security@5b56d68. Goose registers custom providers after the built-ins, keyed by the name inside the file, so a planted custom_providers/anthropic.json replaces Anthropic and redirects reviews. Its auth.command is planted code that a later uncontained Goose runs. The protection doc puts planted code outside this boundary, but this change closes that route for this folder too.
Buzz's host now accepts a read-only path whose last component is missing, when its parent exists and is not /, and denies creating, renaming, hard-linking or symlinking anything at that name. Hidden paths and controls must still exist. The security plugin lists config.yaml, secrets.yaml, custom_providers and config.tmp whether or not they exist, so contained workers cannot create any of them. Because the host needs an existing parent, the plugin creates the Goose folder (~/.config/goose, or the one GOOSE_PATH_ROOT or XDG_CONFIG_HOME selects) with mode 0700 when it is missing, and creates nothing inside it. While checking this we found that Goose saves config.yaml through a fixed config.tmp beside it, which a worker could plant; listing it closes that too, including the .tmp beside the target when config.yaml is a link.
A new Rust test fails on the previous host rule, and a macOS test shows that a contained process cannot create, rename into, hard-link or symlink at a listed missing path. The companion's tests cover the new list.
The companion change has to ship with or after this host change: on an older host every protected launch fails with "Protection path is unavailable".
🤖 Addressed by Claude Code
| unreviewed native command hooks, recipe shell callbacks and unsupported extension | ||
| launches are refused. Protected agents have dedicated contained workers. Goose | ||
| reviews read the owner's Goose connection settings, and the security plugin makes the | ||
| existing `config.yaml`, `secrets.yaml` and `custom_providers` read-only for contained |
There was a problem hiding this comment.
🤖 Must-fix B (error): the GitHub Copilot token cache holds the review endpoint, and its folder is writable on purpose. Traced in source, not run.
When the review model is a Copilot model, Goose get_api_info reads ~/.config/goose/githubcopilot/info.json before the token and uses the cached endpoints.api, which only has to be https (githubcopilot.rs:178-197,384-399). The companion keeps Goose's OAuth token folders writable (engine.py:957). A worker sets endpoints.api to its own server with a far-future expires_at, and reviews go there. The other token caches we checked (Databricks, ChatGPT/Codex, Gemini, xAI, Kimi, Muse) hold only tokens.
Fix: do not keep githubcopilot/ writable for the reviewer (for example, give reviews their own copy of the token cache), or refuse Copilot as a review provider until the endpoint comes from read-only config.
Also on this paragraph: "the existing config.yaml, secrets.yaml and custom_providers" describes the gap in must-fix A. A file that does not exist yet stays writable and can be created by a worker.
There was a problem hiding this comment.
🤖 Confirmed at the Goose pin, and fixed in cef48c6 with the companion in squareup/buzz-plugin-security@5b56d68. get_api_info and invalidate_token trust the cached endpoints.api in githubcopilot/info.json, and no config or env pins it. Among the providers in the protected build, Copilot is the only one whose token cache carries an endpoint. The others cache tokens only, and Bedrock and SageMaker are not compiled in. While checking the class we also found that the codex and cursor-agent command-line providers are not marked as running their own tool loop, yet they run their own agents, whose settings contained workers may be able to change (inferred from source, not checked live).
Goose reviews now refuse all three. A named one is refused before any worker starts. A review with no provider reads Goose's default provider and model in the review worker itself, refuses when either is missing or refused, and sends them explicitly. The review-model list hides all three. Regression tests cover the explicit, default and list cases.
Companion side: the protected engine's patch refused only providers flagged as running their own tool loop, so codex and cursor-agent got through, for protected Goose agents and Goose-model reviews alike. Both run agents with tools that Goose's dispatcher never sees. The patch now also refuses session-bound providers (which catches codex, which has no setup metadata) and providers set up as agents (cursor-agent), in both places. The patch has not been compiled yet: goose-acp must be rebuilt from it, with its provenance regenerated, before this can be checked live.
On the paragraph you quoted: it no longer says "existing". It now says the security plugin lists those files, plus config.tmp, so they are read-only for contained workers even before they exist (the must-fix A change).
🤖 Addressed by Claude Code
| this.ready = undefined; | ||
| void ready?.then((goose) => goose.kill()).catch(() => undefined); | ||
| const goose = await ready; | ||
| await goose?.kill(); |
There was a problem hiding this comment.
🤖 Must-fix C (error): a failed worker kill is forgotten. shutdown() clears ready before kill() and exited succeed. If the sessions close but the kill rejects, runtime.ts:223-225 puts the entry back with a SharedProcess that no longer knows its process. The next retirement succeeds with no kill and no exit. Cerveau's probe: first retire rejected, second resolved, one kill attempt, native exit still pending. Provider replacement (runtime.ts:854) also swallows the failed shutdown and drops the old worker.
Fix: keep worker ownership until exit is confirmed, including after a failed shutdown or replacement. Clear ready only after exited.
There was a problem hiding this comment.
🤖 Fixed in cef48c6, together with D. SharedProcess.shutdown() no longer drops the process before its exit is confirmed. ready is now cleared only by the existing exit handler, so after a failed kill the next retirement kills again and waits for the exit. With D in place a failed start settles only after its process has exited, so shutdown() treats a rejected start as already stopped. Two tests cover this, and each fails without the fix:
- 'repeats a failed kill on the next retirement': after a first kill that fails, a second shutdown kills again and waits for the exit.
- D's held-exit test: with the old
shutdown(), it rejects with the failed start's error.
I declined the provider-replacement half (keeping the replaced worker in runtime.ts):
- On the desktop host it can't happen: the host's
kill()swallows its own errors (src/features/host/service.ts:353-357), and since D a failed start settles only after exit. Soentry.stoppingalready resolves only once the old worker has exited. - Hardening it for a host whose kill rejects would also be incomplete. The capability-probe cleanup in goose.ts would still reject with the process alive, and D's exit wait would hang rather than retry.
Two related gaps in the replacement path are left as follow-ups, not fixed here:
- Two provider changes before the first replaced worker exits overwrite the single
stoppingslot, which can leave an untracked worker. protectionContextcallsentry.worker.get()directly, so a tool call during the old worker's kill grace can start the new worker early.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
🤖 The earlier reply's citation was wrong: at cef48c6 the host's kill() that swallows its own errors is src/features/host/service.ts:360-364, not :353-357 (those lines are the end of the chunked write).
🤖 Addressed by Claude Code
| await bridge?.close(); | ||
| throw error; | ||
| } | ||
| })(); |
There was a problem hiding this comment.
🤖 Must-fix D (error): a failed initialization drops a worker that is still running. forget (:78-81) drops every rejected start. On an initialize failure, process.ts:178-187 calls acp.stop(STOP_TIMEOUT_MS), which waits for exit only up to the 5 s timeout (process.ts:365-374). So an initialize error plus a slow or failed native shutdown forgets the process before it exits, and a later shutdown() succeeds at once. The later Goose capability-probe cleanup does not cover this earlier failure. Cerveau's probe: native exit held, initialize RPC error, the real 5 s stop deadline, then a successful shutdown.
Fix: keep a failed start's process until its exit is confirmed.
There was a problem hiding this comment.
🤖 Confirmed and fixed in cef48c6. A start that failed initialize waited at most 5 s for its process and was then forgotten. A failed start now kills its process and rejects only after the process has exited (process.ts), so the shared Goose worker stays owned until then and a later shutdown waits for it. Cancelled starts still reject at once, as before.
On the desktop host the old window needed an exit more than 5 s after the kill (past the 3 s SIGTERM grace and the SIGKILL), so it was rare, but it broke the retirement barrier. The trade-off: without the 5 s cap, a host that never reports Exit would now keep a failed start, including the shared Goose's next start, waiting. That matches the uncapped exit waits elsewhere in shutdown. Added a held-exit test that fails before the fix: it checks that a second get() while the process is exiting starts no second goose, and that shutdown waits for the exit.
🤖 Addressed by Claude Code
| async shutdown() { | ||
| this.dispose(); | ||
| await Promise.allSettled(this.opens); | ||
| const closed = await Promise.allSettled( |
There was a problem hiding this comment.
🤖 Must-fix E (error): an open that fails after session/new gets past the new retirement barrier. This waits on opens with allSettled and ignores rejections, then closes only what is in its own closing set. After a successful session/new, a failed Goose system-prompt RPC makes process.ts:305-310 start a non-strict close and throw before AgentSessions gets a Live. That session never enters the retirement set. When another ordinary agent keeps the shared Goose alive, ordinary → protected retirement succeeds while that close is unanswered or failed. Cerveau's probe: runtime.retire() resolved with process.closing.size === 1 and native exit pending.
Fix: move failed-open cleanup into tracked, strict retirement. Waiting on a non-strict close alone still swallows the failure.
There was a problem hiding this comment.
🤖 Confirmed and fixed in cef48c6. A session that goose opened but Buzz failed to set up (system prompt refused, or its tool server stopped) was closed outside the retirement set, so retirement could finish while that close was unanswered or had failed. AcpProcess.open now hands the session to its caller through onSession as soon as it exists. When the open then fails, AgentSessions closes it through the same tracked strict close every other session uses, so retirement waits for it and fails if goose refuses the close.
For scale: that session had no turn and its Buzz tool server was already closed, and protected workers were never affected because they are killed. Added a test with gated system-prompt and close requests that fails before the fix.
🤖 Addressed by Claude Code
- Enforce protection checks in the Claude harness plugin - Use native structured output for Claude security reviews - Use trusted bootstrap tools for protected Claude workers - Contain Claude delegated body and transcript reads - Limit Unix-only Claude hook test code on Windows - fix(claude): keep protected hooks inside the watchdog and cancellable - fix(claude): report a stopped protected turn as stopped - fix(claude): review a transcript whose last line is still being written - fix(protection): review a natively gated Buzz tool call once - test(mcp): let the bundled help fixture read the native tool gate - fix(claude): keep the hook guard off the protected file reader - fix(claude): load the model catalog for review model choices - fix(claude): run protected Claude and its reviews from host-verified artifacts - fix(claude): stop and wait for every process when sessions shut down - fix(claude): keep the protection backstop for entries made before sync - fix(claude): let an agent run again after its retirement failed - fix(host): say why the hook guard stopped a worker - test(ci): check the protected Claude pins against the release - fix(claude): ignore Claude settings files in security reviews - fix(claude): run security reviews with their own Claude folder and sign-in - fix(claude): check the pinned copy by starting it contained - fix(claude): let a waiting review give up on a shared download - fix(claude): keep a failing session subscriber from breaking shutdown - fix(host): ignore output after a guarded worker exits - fix(codex): replace a handed-over sign-in in one rename, never through a link - fix(codex): copy adopted threads without following planted links Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Codex <codex@openai.com> Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
- Connect the Codex harness plugin to protected plugin engines - Preserve failed Codex deliveries and security review deadlines - Contain Codex delegated file reads with its active policy - fix(codex): recover protected workers across provider changes - fix(codex): review a natively gated Buzz tool call once - perf(codex): keep one review worker per protection provider - fix(codex): do not resume saved threads across protection states - fix(codex): run protected Codex in Buzz's Codex home - fix(codex): run Codex reviews in a read-only review home - fix(codex): retire only once a pending start settles and the process exits - fix(agents): escape event openings in wake, attention and Canvas text - fix(codex): retire protected servers on a closed connection - fix(codex): turn off remote control for protected Codex and reviews - fix(agents): escape event openings in plugin notes - fix(codex): pin reviews to the ChatGPT workspace signed in through Buzz - fix(codex): keep tool shells from inheriting the protection bridge Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Codex <codex@openai.com> Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
- Connect the Goose harness plugin and document the protection contract - Fix Goose review serialization and duplicate input approvals - Clarify the declared-file containment boundary - Contain Goose delegated file reads with its active policy - fix(goose): recover protected workers across provider changes - fix(goose): review a natively gated Buzz tool call once - fix(goose): tell Goose security reviewers the reply schema - docs(protection): describe home paths and how protected Claude launches - fix(goose): retire only once opening and closing sessions settle - fix(goose): read protection context from the agent's worker, with a timeout - fix(goose): list models from the providers that answer - fix(protection): accept a read-only path not made yet in an existing folder - fix(goose): refuse reviews on GitHub Copilot, codex and cursor-agent - fix(goose): start a failed process again only once it has exited - fix(agents): close a harness session whose open fails after it was made Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Codex <codex@openai.com> Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
1b4cd99 to
cef48c6
Compare
8140250 to
6a25293
Compare
|
🤖 Replies to the round-2 points in the review summary. Must-fix A to E are answered in their threads.
Fixed in #1879 (6a25293), which this PR is stacked on. Every runtime reads plugin notes only through
Not checked live yet, and the protection doc now says so. Both Goose builds read the same Keychain item (service
Confirmed: Goose reads
Confirmed. With the must-fix A change, the security plugin lists the Goose paths whether or not they exist, and the host protects a missing one by name. Nothing created later stays writable, and a worker cannot put a link in place of a missing
Covered by the must-fix A change: contained workers can no longer create anything under
Agreed, so I've left it as is.
Regression tests that fail before their fix:
Coverage tests for round-1 behaviour, which pass today and lock it in: the review's cancel and deadline paths in Schema validation isn't Buzz's job on this path. Goose's review takes text only, so Buzz states the schema in the trusted instructions and returns the text unchanged. The security plugin's 🤖 Addressed by Claude Code |
|
🤖 Reply to the round-1 advice in the review summary, which we had not answered.
Confirmed. The protected Moving the protected pin is more than a version bump. The companion's This has no owner or schedule yet, so until someone does it, protected Goose agents and Goose-model reviews run an older Goose than unprotected ones. Tracked in #2334 and squareup/buzz-plugin-security#4. 🤖 Addressed by Claude Code |
loganj
left a comment
There was a problem hiding this comment.
🤖 Larry, Auto and Cerveau: round-3 review of cef48c69c61608074b331cabad36797bfb46b691 (one commit on #1879 6a2529319), companion squareup/buzz-plugin-security at fe00293. Scope: the round-2 must-fix items and defects that the fixes added. Other areas were reopened only on new evidence. CI is green at this exact head.
Result: 1 must-fix is open (new, from the replacement fix). All 6 round-2 must-fix items are fixed.
Round-2 items
- A,
custom_providersmade by a worker: fixed. The companion listsconfig.yaml,secrets.yaml,custom_providersandconfig.tmpeven when they do not exist (engine.py:936-951), andmissing_leaf(protection.rs:97-106) protects a missing read-only path by name in its existing parent. Probe on a fresh HOME with the PR'sprofile()and stockgoose-acp1.57.0: a contained worker could not createcustom_providersbymkdir,cp -R,mvorln -s, could not create the other three files (also as links), and could not move or remove~/.config/gooseor~/.config.providers/liststill shows the built-in Anthropic. - B, Copilot review endpoint through the token cache: fixed. Buzz refuses
github_copilot,codexandcursor-agentbefore it starts a worker (setup.ts:343) and again when Goose's default names one (setup.ts:364-365);index.ts:49lists no review models for them. The engine also refuses providers that run their own agent (runs_own_agentin the Goose patch). Copilot'sinfo.jsonis the only Goose sign-in cache that holds an API address. - C, failed worker kill forgotten: fixed for direct retirement (
sessions.ts:86-95): the worker stays recorded and retirement repeats the kill. - D, failed initialization drops a running worker: fixed (
process.ts:183-185): startup stays pending until native exit, and shutdown cannot pass it. - E, an open that fails after
session/new: fixed (agent-sessions.ts:531,554-561,453-455): the session moves into strict-close tracking, and retirement waits forsession/close. - F,
unframed()for Goose plugin notes: fixed at the source by #1879 (runner.ts:733);restoreNotesonly puts back text that was already cleaned. Test:runtime.test.ts:1205-1221. - Advice items (
secrets.yamlfallback when the Keychain prompt is denied, the protected list recorded once, a missingconfig.yamlmade into a link): closed by the companion's missing-path entries. Whenconfig.yamlis already a link, its target and the target's.tmpare read-only. The companion'smkdiradds no uncontained step that follows a worker-made link.
Must-fix
- Two quick provider replacements start a worker while the first is still running (
runtime.ts:863, inline). Reproduced with kills that succeed.
Advice (not must-fix)
secrets.yaml.lockfollows a link a worker planted (companionengine.py:945; Goosebase.rs:984-1004). Before each secret save, Goose opens<config>/secrets.yaml.lockwith read, write and create, outside the sandbox. A contained worker'sln -s /tmp/x …/secrets.yaml.lockwas allowed, and a stockproviders/config/savethen created an empty/tmp/x. Effect: an empty file at any path the owner can write, and a worker that holds the lock can stall settings saves. Existing files do not change. Fix: addsecrets.yaml.locktogoose_connection_config; this PR's host already accepts missing paths.- A dangling Goose
config.yamllink stops every protected agent.process_protection()is one list for every protected launch (engine.py:964-991). Ifconfig.yamlis a link to a missing file,missing_leafrefuses it (correctly,protection.rs:50-53), so Claude, Codex, Pi and Goose all fail to start on macOS with "Protection path is unavailable", which does not name the path. It fails closed; consider naming the path in the error. github_copilotis refused only in Buzz, not in the engine. The engine's review path accepts it, because Copilot is a Model-category provider that is not session-bound. Buzz refuses it before the engine sees it, so this is defense in depth only.- Harmless: when
config.yamlis a link, the profile also has an unnormalized…/goose/../../dotfiles/goose.tmprule next to the canonical one.
Tests, CI and docs
- CI run 41708340227 passed at this head. The macOS job runs
cargo test host_process --lib(ci.yml:89), so the new kernel testkernel_denies_creating_a_missing_read_only_leafruns there.goose.live.test.tsstays out of CI (it needsGOOSE_ACP_BIN), as before. - Local: Goose and agent-harness test folders 338 passed, 8 skipped. Companion Python tests: 64 passed at
fe00293. docs/agent-protection.mdmatches the code for the Goose protected paths, the refused review providers and the missing-path host.
| // Another key means a replaced provider, which is already stopping | ||
| // its worker; the replacement starts once that one has exited. | ||
| if (entry.worker && entry.workerKey !== protection.key) { | ||
| entry.stopping = entry.worker.shutdown().catch(() => undefined); |
There was a problem hiding this comment.
🤖 Must-fix: two quick provider replacements start a worker while the first is still running.
Each replacement overwrites entry.stopping. Failure: worker A is running. Replacement B opens a conversation and waits for A to exit. Before A exits, replacement C opens another conversation. B's worker never started, so B.shutdown() resolves at once. That value replaces A's exit promise in entry.stopping, and C's worker starts while A is still alive. Reproduced through the real AgentSessions.open, runtime and ACP classes, with a held native exit and kills that succeed. Policy-save retirement is not bypassed (opens still tracks B's pending open), but the replacement exit barrier is broken.
Fix: keep every outstanding predecessor shutdown, for example entry.stopping = Promise.all([entry.stopping, entry.worker.shutdown()]).then(() => undefined, () => undefined). Add a test with two replacements while A's exit is held.
The probes with a kill that fails also showed gaps here and at goose.ts:204. The desktop HostService.kill() ignores IPC errors (host/service.ts:360-364), so those are not a current desktop blocker.
loganj
left a comment
There was a problem hiding this comment.
🤖 Round 3 at cef48c69c. Approving: no major blockers.
Logan's direction for this stack (2026-11-04): the reviewer is meant to catch attempts to get around it, so breakout vectors are not blockers, and this prototype needs to land so development can continue. This round is a check for major blockers only. A major blocker is a defect that breaks the reviewer gate itself or breaks agents that are not protected. I read the round-3 change against round 2 and checked CI at this exact head. I did not re-probe each round-2 item.
Round-3 change: the lifecycle items are addressed. SharedProcess.shutdown() keeps the process until it has exited (C and D). AgentSessions closes a session that opened but then failed setup, through the retired close path (E, via onSession). A failed ACP start now waits for the process to exit.
Optional: in the shared ACP harness (acp/process.ts), a failed start now does kill() and then waits for exited with no time limit. Before, it used stop(5 s). This path is also used by agents that are not protected. If a kill ever fails to end the process, that start hangs where it threw before. This is unlikely with SIGKILL, but a deadline here would keep the old bound.
Optional, recorded breakout vectors (out of scope per Logan): round-2 A (a worker-created custom_providers), B (the Copilot token cache) and F (unframed() under Goose). Companion fe00293 now protects missing Goose files too, and needs this PR's missing-path host.
Evidence: CI run 41708340227 is green at this head.
Based on main. The base fix [#2044](#2044) and Pi's adapter [#1877](#1877) have merged. Companion plugin: [squareup/buzz-plugin-security#2](squareup/buzz-plugin-security#2). Protected Claude agents now pause native input and tool calls for the agent's bound policy. Security reviews that use a Claude model run tool-free and return a structured verdict. Both run Claude's pinned release, and the host runs its own verified copy of it. Security reviews also sign in apart from `~/.claude`, with their own Claude folder and token. ### What this changes - **Pinned, host-verified runtime.** The plugin declares Claude 2.1.305 as two macOS artifact processes, `claude-protected-arm64` and `claude-protected-x64`. - Each has the release's SHA-256 and the path `~/Library/Application Support/Buzz/claude-code/2.1.305/claude`. The host expands `~/` under the user's home. - For a contained spawn, the host hashes the bytes from one no-follow descriptor and copies them into its write-denied artifact store, then runs that copy. A changed file in the download folder doesn't start, and swapping it after the check changes nothing. - The plugin picks the process from `uname -sm`. A wrong answer only names a pin whose checksum then fails. - Setup checks the pin by starting the host's copy contained, with the same read-only Claude root as the real launch, and then stopping it. It never runs the download folder's file outside the sandbox. A pruned 2.1.305 folder doesn't trigger a download while the store copy is valid. - If the host refuses its copy, setup installs the pinned release through `claude.sh` and checks again the same way. A failed install step reports "could not be installed"; a copy the host still refuses surfaces the host's own reason. The host, not `claude.sh`, verifies and starts protected Claude. `claude.sh` only installs the pinned release on request, checking its SHA-256, and keeps 2.1.305 when it prunes. - A caller waiting for that download stops waiting when its own signal aborts, and the shared download carries on for later callers. - **Protected sessions.** Hooks pause native input, tool actions and raw output for the policy. The native watchdog (350 s) stops a stalled callback before the CLI's 600 s hook timeout. - Exact tool arguments and unknown evidence are kept, and prompt and history representations map to stable message sources. - Sessions retire before protection changes. - An entry made before `sync`, by `session`, `run` or `runOnce`, is already marked protected, so the "no live policy binding" backstop applies to it. - `AgentSessions` tracks every process it starts, from the request to its exit, including a spare it took. `shutdown()` kills each one and waits for it, even one still starting. A pool that has shut down starts nothing. - A session subscriber that throws is logged and can't stop the others, so it can no longer break a shutdown before every process is killed and waited for. - A failed retirement is dropped rather than kept, so the agent can start again. - When the host's hook guard stops a worker for an invalid or oversized frame, it writes the reason to stderr, so the turn's error names it. - After a guarded worker exits, the host still delivers its last output, such as its final `result` or error. Only the guard's timers stop, and input to an exited worker is still refused. - **Security reviews.** Reviews run the pinned copy, contained and with no tools. Trusted instructions are kept apart from evidence. The output schema goes to native `--json-schema`, and the adapter requires `structured_output`, so formatted prose can't pass as a verdict. - Reviews, like protected agents, pass `--setting-sources ""`, so Claude reads no user, project or local settings file. A contained worker can't redirect reviews through `env` (such as `ANTHROPIC_BASE_URL`) or `apiKeyHelper` in `~/.claude/settings.json`. - **Own folder and sign-in** (Alex's idea of giving reviewers credentials that contained workers can't reach). Reviews run with `CLAUDE_CONFIG_DIR` set to `~/Library/Application Support/Buzz/claude-code-review-home`, and sign in with a `claude setup-token` token in `CLAUDE_CODE_OAUTH_TOKEN`. So a review reads nothing from `~/.claude`: no `.credentials.json` and no cached `remote-settings.json`. The companion creates that folder at 0700 and lists it in `hiddenPaths`, so contained workers can neither read nor write it. - **Setting it up.** The person runs `claude setup-token` in Terminal and pastes the token under **Sign in for reviews** in **Settings → Agent setup → Harness defaults → Claude → Edit configuration**. A new `claude.sh review-token` step reads it from stdin and prints only fixed lines, so the token never appears in argv, step output or the setup log. It refuses a folder that is a link or not a folder, and writes `token` at 0600 through an exclusive temp file and `mv -h`. A `review` step reads the folder and token back for the reviewer, skips a token that is a link, and off macOS fails with "Claude security reviews need macOS". Closing the dialog during a save cancels it, as Cancel does. - **Fail closed.** With no saved token, a review fails before the download or any model starts. Clear auth failures (`authentication_error`, or an expired or revoked OAuth token) map to one message that also says a Claude subscription is needed. - **Env hygiene.** The review env nulls `ANTHROPIC_AUTH_TOKEN`, `ANTHROPIC_BASE_URL`, `CLAUDE_CODE_API_BASE_URL`, `CLAUDE_CODE_SIMPLE`, `ANTHROPIC_UNIX_SOCKET`, `CLAUDE_CODE_REMOTE_SESSION_ID` and the `CLAUDE_CODE_USE_*` routes. - A review's 60 s budget now covers reading its sign-in and waiting for the pinned release's first download. A longer download fails the review closed and carries on for later reviews. - **File reads.** Buzz file tools and the transcript paths a worker chooses use the app-bundled reader under the captured policy. It accepts `~/…`, passes the filename as its own argument, and never falls back to an ordinary reader. - **Codex launcher fixes that are reachable from here.** `codex.sh` `run` and `adopt` already run uncontained at this point in the stack, so two link-following fixes land here rather than in #1879: - The sign-in handover moves Buzz's sign-in file onto the shared `auth.json` in one rename (`mv -f -h` on macOS). It no longer reads or writes through a link, so a worker can't use it to overwrite or copy out a protected file. A user's own link at `~/.codex/auth.json` is now replaced by a regular file, as `codex login` does; the Codex README says so. - `adopt` skips a saved thread that is a link, creates its copy as a new file (`set -C`, keeping the time with `touch -r`), and on macOS publishes it with `ln -P`. A link planted at the temporary name makes adopt stop without touching the target, and the thread starts new. - **CI.** `install-protected-claude-test.mjs` checks that both pins exist in the plugin manifest and match the release manifest before it checks the download. ### Known gaps and tradeoffs - Protected agents and Claude-model reviews need a release build of the macOS desktop app. In dev builds and on other platforms they fail closed. - Claude-model reviews sign in only with their own `claude setup-token` token. That needs a Claude Pro, Max, Team or Enterprise subscription and lasts a year. API-key, gateway, Bedrock and Vertex setups can't run Claude-model reviews; choose another review model. - **Existing users must sign in for reviews once.** Until the review sign-in is saved, and after it expires or is revoked, Claude-model reviews fail closed. - **Release coupling.** Ship the companion change (squareup/buzz-plugin-security#2) in the same Buzz release that carries both this PR and [#839](#839). The companion lists protected paths that may not exist yet, and a host without #839 refuses a protected launch when a listed path is missing. Without the companion change, the review folder is not hidden from contained workers. - **Process environment.** While a review runs, another process of the user can read the review token from that review's process environment. Whether contained agents can read the pinned binary's environment this way (`KERN_PROCARGS2`) has not been checked. Closing this needs a new host channel for the token, so it is documented instead. - **Protected runs still use `~/.claude`.** The review folder applies only to security reviews. Another contained worker can plant a sign-in, or cached managed settings that carry hooks and env, in `~/.claude`, and they reach a protected run's checks. Transcripts in `~/.claude/projects` can also be swapped between a run's checks or between the CLI's `--resume` load and Buzz's read for the review. Moving protected runs to a Buzz-owned folder would also move their sign-in, cached managed settings and session history, so it is a separate change. Both gaps are in `agent-protection.md`. - The review env's null list was read from 2.1.312's strings and is not yet checked against 2.1.305. - Review model choices come from the Claude in use, but reviews run 2.1.305. An alias such as `opus` resolves inside 2.1.305, and a model newer than it may be unknown to it. - A symlinked home directory, or any symlinked component of the artifact path, fails closed. Reinstalling doesn't help, because the install writes to the same path. - A spare still being handed over when a pool shuts down is killed as soon as it arrives, but not waited for. Spares serve only unprotected launches. - The first protected launch copies the release into the artifact store: about 273 MB on Apple silicon, 284 MB on Intel. Each review still hashes the release twice (the contained check, then the launch). - Older Buzz builds prune 2.1.305 from the shared download folder, so switching between builds can download it again. - Pi's protected runtime does not verify its runtime bytes at launch: it runs the shared host file and Node, made read-only for the worker, without hashing or copying them as this PR now does for Claude. Fixing it is a follow-up. - Planting a program on `PATH` stays outside the documented boundary. - A tool result Claude echoes over the 4 MiB frame cap, such as a large PDF or image, stops the turn closed, now with a reason. - Codex `adopt` keeps small residuals that bash can't close: a rollout swapped for a link between the check and the read can still be copied, linked folders along the path can place a new `rollout-*.jsonl` elsewhere, and noclobber still writes through a link to a FIFO or device. The handover guards only the last path part, so a linked `~/.codex` still receives `auth.json` in the folder it points to. - **Size.** The Claude review sign-in is about 195 production lines, against a checkpoint budget of about 55. The author chose to keep it as is. - Follow-ups not in this PR, tracked in [the follow-up issue](#2334) with the protected-run `CLAUDE_CONFIG_DIR` and Pi's runtime bytes: - a per-hook TypeScript deadline below the native 350 s, which would make a stalled review's error clearer; - removing the duplication in `protectedEvents`, and the O(n) source checks per hook; - a wall-clock hook deadline. Tokio's `Instant` stops during macOS sleep. If the CLI's hook timer doesn't stop, a laptop that sleeps through a pending `PostToolUse` review could let the CLI time out first, and that hook fails open; - caching the artifact snapshot's validation in the host, so a review doesn't hash the release twice; - carrying the Buzz destination explicitly in natively gated tool reviews, across Claude, Codex and Goose; - the shared 256 KiB action cap, which a long protected Claude conversation can reach. ### Validation Restacked onto main `066e34f12` with the round-2 fixes: now `4564d2b68` (stack tip `b85084bff`). Main's saved session prompts ([#1855](#1855)) overlapped the Claude runtime's session store: both are kept, the store keyed by protection state and main's `started` prompt save. At the stack tip (`b85084bff`): `pnpm check` (Biome, `tsc`, design checks), the integration suite (248 passed, 1 skipped), the full Vitest run (16,072 passed, 22 skipped), `cargo test -p buzzodz-plugins` (64), `cargo test --lib` in `src-tauri` (841 passed, 2 ignored), Clippy (`-D warnings`) and `cargo fmt --check` pass. One `src-tauri` test was left out: `window_state::tests::onboarding_snapshot_is_kept_until_next_launch_restore` crashes (SIGSEGV) on main as well (seen at `7cb3cb2c1`), and this stack doesn't change `window_state`. The round-2 checks below ran on the round-2 changes over `0caf2fd59`, before the restack. Round 2, on the changes over `0caf2fd59`: - `vitest run src/bundled/claude-code`: 316 passed, 4 skipped. `vitest run src/bundled/codex`: 297 passed, 2 skipped. `tsc --noEmit` and `biome check` on the changed files are clean. `bash -n` and `shellcheck -s bash` are clean for `claude.sh`; `codex.sh` has only the existing SC2174. - `cargo test --lib host_process`: 34 passed. Clippy (`-D warnings`) and `cargo fmt --check` are clean. - **New tests, each seen failing on the previous code:** - the pinned check starts the copy contained, never the download folder's file; - a cancelled wait for the pinned copy ends, during the check or the download, while one download serves the next caller; - a review's 60 s budget fires during a held install; - a failed install step is reported without checking the copy again; - a subscriber that throws during retirement no longer stops the worker being killed; - a guarded worker's last frame reaches the page after it exits, and the guard drains an exited worker's output but refuses its input; - a review without its own sign-in fails closed before any model; a review signs in with its own folder and token, over other sign-ins; only a refused sign-in maps to the sign-in message; the save step gets the token on stdin alone; - the review sign-in field saves through stdin, keeps its result, cancels a save from the footer and when the dialog closes, and keeps the field and Save focusable while a save is pending; - `claude.sh` saves the review sign-in privately, refuses a linked or non-folder home and a non-Claude token without printing it, and reads no sign-in through a link; - `codex.sh` never writes a handed-over sign-in through a link to a file or into a linked folder; adopt never copies a linked thread, never writes through a planted temporary link, and never hard-links what a swapped-in link points to. - **CI gap.** The `claude-sh` and `codex-sh` shell tests that depend on macOS are skipped elsewhere. CI's Vitest job runs on Ubuntu, so CI does not run them; they ran only locally on macOS. - **UI check.** Before the last polish, the real Claude configuration dialog body ran in Chromium through the browser test server: both themes, 390 px and 1280 px, root text at 200%, keyboard and pointer. That check found focus loss while a save was pending, and a dialog close that didn't cancel a save. Both are fixed and covered by the tests above, but the fixed version was not rendered again. - **Agent review:** independent reviews of the round-2 changes scored them 9 or higher with no blockers. Earlier rounds, at `29828e405` (stack tip `6b514fff4`), on main `6dc462a2d`, with later rebases onto main `61a67d28f`: - At this head, `vitest run src/bundled/claude-code`: 281 passed, 4 skipped, and `tsc --noEmit` is clean. At the stack tip, the Claude, Codex, Goose and Pi plugins, agent harness, agent protection, agents, Buzz MCP, Builderlab and scripts: 3,000 passed, 18 skipped. - At the stack tip: `cargo test --lib host_process`: 33 passed. `cargo test -p buzzodz-plugins`: 64 passed. Clippy (`-D warnings`) and `cargo fmt --check` are clean. - `biome check` and `tsc --noEmit` are clean at the stack tip. - After the rebase onto `61a67d28f`, one conflict: main's recent-notes read ([#2040](#2040)) now runs after the protection check at session launch; in protected sessions the notes reach the policy with the rest of the system prompt. The agent, Buzz MCP, relay, Builderlab, Inbox, Channels and scripts suites (6,882 passed, 22 skipped), `tsc --noEmit`, `cargo test --lib host_process` (33), `cargo test -p buzzodz-plugins` (64), Clippy (`-D warnings`) and `cargo fmt --check` passed there. - **New tests, each seen failing on the previous code:** - a review runs the pinned copy, contained; - a review installs the pin first, and fails without starting a worker when that install fails; - shutdown waits for processes still starting and for a spare it took, and starts nothing for a conversation still launching; - the backstop refuses a protected entry made before `sync`; - an agent runs again after its retirement failed. - **Other new tests:** a review passes `--setting-sources ""`; the hook guard's stderr reason and its quiet ordinary exit; `~/` artifact and protection paths; the `uname` mapping for both Mac architectures, refusing Linux; the manifest pins matching `PROTECTED_CLAUDE_ROOT` and `PROTECTED_CLAUDE_VERSION`; `claude.sh` keeping 2.1.305 when it switches past it. - **Agent review:** an independent review of the earlier round's fixes found no blockers. An independent review of the settings change scored it 9/10 with no blockers. ### Not yet done - No packaged-app run of the sandbox and the artifact copy. Dev builds refuse contained spawns. - The CI pin check runs only in hosted CI. I didn't download the release locally. - **Deferred live checks.** No pinned Claude 2.1.305 binary or real Claude account was run for the review sign-in. Not yet checked: - that `CLAUDE_SECURESTORAGE_CONFIG_DIR` and the keychain don't supply another sign-in; - a review with a fresh, empty `CLAUDE_CONFIG_DIR` (trust and onboarding skip, and Claude writing into that folder); - `CLAUDE_CODE_OAUTH_TOKEN` taking precedence over stored sign-ins, and its 401 behavior; - the real `claude setup-token` output, and the real error texts for an expired, revoked or non-subscription token; - whether remote managed settings are fetched into the review folder under an env token. - Not checked live: a Claude-model review under the flag with hostile settings files. - The companion creating and hiding the review folder was not run against this build in the app. - The Codex launcher fixes were checked only in fake homes, not against a real Codex. - The review sign-in field in the native app: both themes, narrow and wide widths, enlarged text, keyboard and pointer. - Human testing. <!-- buzz-security-flow:start --> ### Code flow ![[3/5] Add protection to the Claude harness plugin — illustrative code flow](https://github.com/raw/block/buzz-app/f36b5077959f8c565487457947a479525ff4d05f/docs/assets/agent-protection/claude.png) Claude holds native input, tool actions and raw output. Its 350-second native watchdog stops stalled callbacks before the 600-second CLI timeout. Illustrative flow; checks shown enabled. Source: [claude.ts](https://github.com/block/buzz-app/blob/4564d2b68/src/bundled/claude-code/claude.ts), [claude_hooks.rs](https://github.com/block/buzz-app/blob/4564d2b68/src-tauri/src/host_process/claude_hooks.rs). <!-- buzz-security-flow:end --> Review and merge bottom to top: 1. [[1/5] Keep protected launches on the startup Agents root](#2044) (merged) 2. [[2/5] Pi harness integration](#1877) (merged) 3. [[3/5] Claude harness integration](#1878) (this PR) 4. [[4/5] Codex harness integration](#1879) 5. [[5/5] Goose harness integration](#839) Above these: [#2088](#2088) (exact history records) and [#2105](#2105) (denial reasons for the owner). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Codex <codex@openai.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
A harness whose start failed was killed and then awaited until it exited, with no time limit, so a worker that ignored the kill hung its start forever. The start now waits at most 5 s, the limit the cancel branch's stop already uses, and then fails. The shared goose worker is handed to `SharedProcess` as soon as it has started, so one still running after a failed start stays tracked until it exits: `get()` keeps returning that failed start rather than starting a second worker, and `shutdown()` kills and awaits it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Codex <codex@openai.com> Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
A protected goose start whose worker failed the security-hooks check was killed and then awaited until it exited, with no time limit. It now waits at most the 5 s a failed `AcpProcess.start` waits, and then fails. The shared goose worker still keeps that worker until it exits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Codex <codex@openai.com> Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Co-authored-by: Codex <codex@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Co-authored-by: Codex <codex@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Based on [#1878](#1878). Companion plugin: [squareup/buzz-plugin-security#2](squareup/buzz-plugin-security#2). Protected Codex agents run the Security plugin's verified native engine through the host's authenticated protection bridge. Buzz binds the agent's identity, community and policy independently of model content. It checks prepared model input and exact tool arguments, and retires protected sessions before a policy changes. Security reviews that use a Codex model run on the same pinned engine, with no tools, in a home of their own, pinned to the ChatGPT workspace signed in through Buzz. ### What this changes - **Protected sessions.** Prepared model input and final tool arguments go to the policy, and tool results return through the next input check. Restored history, dynamic instructions, memory, tool results and unknown evidence stay inspectable. - Resuming a saved thread or compacting an inactive one needs the same protection state, in both directions. Protected title generation is off. - Final delivery state belongs to the captured request, so overlapping sends and steering can't erase another request's failure or mark a later success failed. - Buzz skips its own check of a Buzz tool call that the native gate already reviewed with identical arguments. A call with a captured file body is still checked. - Remote control is off (`CODEX_INTERNAL_APP_SERVER_REMOTE_CONTROL_DISABLED=1`) for the protected engine and the review worker. The unprotected spawn is unchanged. - Tool shells get an empty protection bridge address and token (`-c shell_environment_policy.set.BUZZ_SECURITY_URL=""`, and the same for `BUZZ_SECURITY_TOKEN`), so they don't inherit them. `set` merges per key, so the user's own shell policy is kept. - **Buzz's Codex home.** The protected engine runs in Buzz's own Codex home, with `sqlite_home` pinned there, as `codex.sh run` does. Protected conversations stay out of the user's Codex history, and the Codex app's writer lock can't block them. A new `codex.sh home` step links in the settings and sign-in, as `run` does, and prints the home. The sign-in handover never reads or writes through a link ([#1878](#1878)). - **Retirement.** A retirement resolves only once a pending start has settled and the process has exited. - No protected start or preview runs for an agent while it retires, or after a failed retirement until a later one succeeds. - A failed close is retried by the next retirement. - A connection that fails before the process exits (for example, an oversized frame) keeps the server tracked and closes it with the same shutdown. Starts for that agent are refused until the old process exits. - **Security reviews.** There is one review worker per protection provider and workspace, with a short-lived thread for each review, cancelled by interrupt. - The worker runs in its own home, `~/Library/Application Support/Buzz/codex-review-home`, which `codex.sh review` makes. The home holds a link to the shared sign-in, the recorded workspace, and four links to a separate state folder for the names Codex writes at startup. The Security plugin makes the home read-only to contained workers. - `HOME`, `CODEX_HOME` and the cwd are the review home. The thread config turns off MCP servers, web search, plugin features, memories, project docs and skill instructions. - Trusted instructions and evidence go in separate fields, and the output schema goes to `turn/start`. The review has no tool handler, so tool calls and approval requests are refused. - **Review sign-in.** Signing in through Buzz's Codex setup, including the new **Sign in again** in Settings → Agent setup → Harness defaults → Codex, runs a new `codex.sh login` step. It runs `codex login` with `CODEX_HOME` set to the review home, records the sign-in's ChatGPT workspace there, then hands the sign-in to the shared one. The move into Buzz's home uses the no-follow flag on macOS, as the handover does. - Every review re-reads the pin and starts Codex with `-c forced_chatgpt_workspace_id="<id>" -c forced_login_method="chatgpt"`, so Codex refuses an API key or a sign-in recorded for another workspace before it fetches any workspace's cloud config. A changed pin replaces the worker. - The sign-in itself stays linked, because Codex rotates refresh tokens and a copy would stop working. - Before `codex login`, Buzz removes the review home's link, so Codex's sign-out before a sign-in revokes nothing and a cancelled sign-in leaves the shared one working. The replaced sign-in is left to expire. The dialog keeps the sign-in's output after it ends. - Without a matching pin, reviews fail closed before any worker starts, with a message saying where to sign in again. - **File reads.** Protected Buzz file tools use the app-bundled reader under the captured policy. It accepts `~/…`, passes the filename as its own argument, and never falls back to an ordinary reader. - **Text Buzz doesn't own.** Some text Buzz doesn't own goes in a turn in place of its event: plugin-wake and `self_handoff` text, attention prompts and Solo Canvas text. Plugin notes are escaped where they are stored (`runner.note`), for every runtime, and Codex's whole turn context (instructions, team, core memory, repositories) is escaped before it becomes developer text. A line in any of these that starts with `<buzz-event>` or `<new-message-arrived-while-you-were-working>` is escaped, so the text can't pose as the turn's event record. Codex's own user-role text, such as the workspace AGENTS.md, is handled by the companion, which now splits a Codex block only when it is Buzz's own turn input (squareup/buzz-plugin-security@5b56d68). [#2088](#2088) adds the tests for the prompt that Claude, Pi and Goose share (`claude-code/prompt.ts`). ### Known gaps and tradeoffs - Reviews ignore a keyring sign-in, a custom model provider and the user's `config.toml`, `AGENTS.md`, skills and rules. A review that depends on them fails closed. - Existing users must use **Sign in again** once before Codex reviews work. Until then, and while the shared sign-in is an API key or names another workspace, Codex reviews fail closed with a message saying where to sign in. API key and keyring sign-ins get no Codex reviews. - A contained worker can still replace the shared `auth.json`. A forged one that records the pinned workspace with another workspace's tokens is stopped only if OpenAI's servers bind the `account_id` to the token, which is unverified. - A review that starts during a Buzz sign-in waits on the review-home lock until the 15 s step timeout, then fails closed. Reviews work again once the sign-in finishes. - The protected engine still reads the shared `config.toml`, `AGENTS.md`, skills and rules through links, and its rollouts in Buzz's Codex home are worker-writable. With injection inspection on, resumed history is reviewed as context, not trusted (same as #1878). - Tool shells no longer inherit the bridge token, but a same-user command that reads the engine's own environment can still end the bridge session. Later turns then fail closed: an availability gap, not a protection one. - Buzz's stock Codex lists the review models, not the pinned 0.164.0 reviewer. A model only the newer build knows fails at the review turn, closed. - A provider replacement closes the old generation's server, and a failure of that close is swallowed. The provider's abort kills that generation's process, which bounds it. - The companion still declares its `status` and `review` commands and its `checker` process as `env` resolved on `PATH` (#1875 finding A). The `review` command also records approval decisions. The host refuses absolute program paths, and only host processes can declare a hash-checked artifact. So the `checker` process could move to one in the companion, while the `status` and `review` commands need a Buzz host change first. Tracked in squareup/buzz-plugin-security#4. - Protected agents need a release build of the macOS desktop app. In dev builds and on other platforms they fail closed. ### Validation **Restack.** Rebased with the stack onto main `066e34f12`: now `6a2529319` (stack tip `b85084bff`), still one commit on #1878 (`4564d2b68`). No conflicts; main's saved Codex session prompts ([#1855](#1855)) sit beside this commit's changes. At the stack tip (`b85084bff`): `pnpm check` (Biome, `tsc`, design checks), the integration suite (248 passed, 1 skipped), the full Vitest run (16,072 passed, 22 skipped), `cargo test -p buzzodz-plugins` (64), `cargo test --lib` in `src-tauri` (841 passed, 2 ignored), Clippy (`-D warnings`) and `cargo fmt --check` pass. One `src-tauri` test was left out: `window_state::tests::onboarding_snapshot_is_kept_until_next_launch_restore` crashes (SIGSEGV) on main as well (seen at `7cb3cb2c1`), and this stack doesn't change `window_state`. The round-2 checks below ran on top of `8140250ef`, before the restack. **Round 2.** This round fixes loganj's four round-2 must-fix items (A: connection failure, B: review sign-in, C: handover through links, fixed in #1878, D: forged event openings), turns off remote control, and keeps tool shells from inheriting the bridge. - Each fix was checked on top of `8140250ef`, before the restack: the Codex plugin, agent harness and agents suites, `tsc --noEmit` and `biome check` pass, and `bash -n` and `shellcheck` show no new warnings. - **New tests, each seen failing on the previous code:** - a protected server whose connection failed stays tracked until its process exits; a failed close of a failed connection is repeated by the next retirement; - the protected engine sets the remote-control variable; - a plugin note is stored escaped; forged core memory and instructions are escaped in the Codex turn context; - `codex.sh login` records the workspace and hands the sign-in over; it records none for an API key or when Codex saves nothing; `review` prints no pin once the shared sign-in names another workspace; a cancelled sign-in keeps the shared one; a review waits for a sign-in in progress; - the review worker refuses a pin that could escape its config string; - after a ready-state sign-in fails, its output stays visible in the dialog. - **Other new tests:** an idle protected retirement logs no clean-up error through its closing connection; the review worker also sets the remote-control variable; the protected engine's exact arguments, including the bridge env. The macOS-only "never moves a review sign-in into a folder the shared sign-in links to" was added with the restack. - **Agent review:** each fix had an independent review. Blockers fixed: **Sign in again** revoked the shared sign-in before the browser opened, and a ready-state sign-in's failure output disappeared when it ended. **Round 1 and earlier.** Rebased onto main `61a67d28f`: now `8140250ef` (stack tip `80471b41c`). At the new tip, the agent, Buzz MCP, relay, Builderlab, Inbox, Channels and scripts suites (6,882 passed, 22 skipped), `tsc --noEmit`, `cargo test --lib host_process` (33), `cargo test -p buzzodz-plugins` (64), Clippy (`-D warnings`) and `cargo fmt --check` pass. The checks below ran before the rebases. At `f01c81006` (stack tip `6b514fff4`), on main `6dc462a2d`: - At this head, the Codex plugin, agent harness and agent protection: 405 passed, 2 skipped, and `tsc --noEmit` is clean. At the stack tip, the Claude, Codex, Goose and Pi plugins, agent harness, agent protection, agents, Buzz MCP, Builderlab and scripts: 3,000 passed, 18 skipped. - At the stack tip: `cargo test --lib host_process` (33) and `cargo test -p buzzodz-plugins` (64) pass, and Clippy (`-D warnings`), `cargo fmt --check` and `biome check` are clean. - **New tests, each seen failing on the previous code:** - the protected engine starts in Buzz's Codex home; when the home can't be found, the start fails closed and a later one looks again; - `codex.sh home` and `codex.sh review`, including a review home made again; - the review worker's env, cwd and pins; when the review home can't be made, no worker starts and a later review tries again; - a retirement stays pending while its server starts; - a preview is refused while the previous server closes; - no server starts for a standalone turn whose agent was retired while it waited; - after a failed retirement, previews are refused and the next retirement repeats the close until the process exits; - `dispose()` closes an unconfirmed server; - a failed `close()` is tried again. - **Other new tests:** wake, attention and Canvas text can't open an event section. - **Live engine check:** I ran the pinned 0.164.0 engine with the review environment. `config/read` showed no provider, model providers, MCP servers or developer instructions, and no instruction sources. The run also found that skills from a git repository above the review home reached the model; `skills.include_instructions: false` now stops that. - **Agent review:** each fix had an independent review. Two blockers in the review home were fixed: project docs above the home, and `HOME`. ### Not yet done - No packaged-app run. - Not checked live (no real Codex account): the `-c` workspace pin at bootstrap and reload, whether OpenAI's servers bind `account_id` to the token, and `codex login` with `CODEX_HOME` set to the review home. Also not checked live: remote control reporting disabled with the variable set, the bridge env `set` override against the protected binary, Codex token refresh in the review home, and whether 0.164.0 unloads an ephemeral review thread after its last unsubscribe. - **Sign in again** has not been tried in the native app. - CI gap: hosted Vitest runs on Linux, so the macOS-only codex-sh cases (the `mv -h` folder cases for the handover and the login step) run only on a Mac. - Human testing. <!-- buzz-security-flow:start --> ### Code flow ![[4/5] Add protection to the Codex harness plugin — illustrative code flow](https://github.com/raw/block/buzz-app/f36b5077959f8c565487457947a479525ff4d05f/docs/assets/agent-protection/codex.png) Codex gates prepared model input and final tool arguments. Tool results return through the next input gate. The Security plugin supplies its protected engine. Illustrative flow; checks shown enabled. Source: [runtime.ts](https://github.com/block/buzz-app/blob/6a2529319/src/bundled/codex/runtime.ts), [native-bridge.ts](https://github.com/block/buzz-app/blob/6a2529319/src/features/agent-protection/native-bridge.ts). <!-- buzz-security-flow:end --> Review and merge bottom to top: 1. [[1/5] Keep protected launches on the startup Agents root](#2044) (merged) 2. [[2/5] Pi harness integration](#1877) (merged) 3. [[3/5] Claude harness integration](#1878) 4. [[4/5] Codex harness integration](#1879) (this PR) 5. [[5/5] Goose harness integration](#839) Above these: [#2088](#2088) (exact history records) and [#2105](#2105) (denial reasons for the owner). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Codex <codex@openai.com>
* origin/main: Open any Inbox filter view by route (#2394) fix(bestie): deleting an agent saves no Bestie record where there was none (#2420) fix(unread): never treat agents-audience messages as unread (#1552) feat(focus): add an optional Focus plugin that quiets everything except Solo chats (#2277) [5/5] Add protection to the Goose harness plugin (#839) feat(agents): identify plugins in native hook output (#2299) [4/5] Add protection to the Codex harness plugin (#1879) feat(search): show member counts on channel results (#2333) Fix the mute icon flashing when selecting a channel (#2287) feat(agents): open a working agent's transcript from its activity (#2276) Signed-off-by: Michael Neale <michael.neale@gmail.com> # Conflicts: # crates/plugin-manager/tests/management.rs # src/app/pages.integration.test.mjs # src/features/agent-harness/acp/process.ts



Based on #1879. Companion plugin: squareup/buzz-plugin-security#2.
Protected Goose sessions run the Security plugin's verified native engine through the host's authenticated protection bridge. Buzz captures the agent's identity, community and policy independently of model content. Goose inspects native input, restored history and tool results before inference, and checks exact tool arguments, with the preceding conversation, before dispatch.
What this changes
goose-acpworker; ordinary agents keep the shared process.AgentSessions.shutdown()waits for every session still opening and every close not yet acknowledged. It then closes each session strictly, and fails if one could not be closed.AcpProcess.close()share a close in flight, and a failed close stays failed while goose runs.get()meanwhile starts no second goose.config.yaml,secrets.yaml,custom_providersandconfig.tmp(plus the.tmpbeside the target whenconfig.yamlis a link) read-only whether or not they exist, so a worker can't plant a provider or redirect a review. When the Goose folder is missing, it creates the folder (mode 0700) and nothing inside it.github_copilot, whose token cache sets its API endpoint, and thecodexandcursor-agentcommand-line providers. A review with no provider reads Goose's default in the review worker itself and is refused when that is missing or refused. The review-model list hides all three.readOnlyPathsentry may now lack its last component, under an existing parent other than/; the host then denies creating, renaming, hard-linking or symlinking anything at that name. Hidden paths and controls must still exist. This is a small change to the shared protection contract inprotection.rs, documented inplugin-architecture.md.codex,cursor-agent), for agents and reviews alike, not only providers flagged as running their own tool loop.~/…, passes the filename as its own argument, and never falls back to an ordinary reader.Release order
config.tmp), so on an older host every protected launch fails with "Protection path is unavailable".goose-acpmust be rebuilt from the companion's new patch, with its provenance regenerated, before release. The patch change has not been compiled yet.Size
Round 2 adds about 50 net production lines, not counting comments (Goose plugin, agent harness and
protection.rs), against a checkpoint budget of about 35. Tests and docs are extra.Known gaps and tradeoffs
config.yamleither. I haven't yet confirmed live that one never needs to.goose-acpis built separately from the bundled one that stored the keychain keys, so macOS may ask for keychain access on first use and after each rebuild. Denied access leaves Goose withsecrets.yaml, which contained workers can't change. How often a prompt appears, and how an unanswered prompt behaves, are not yet checked live.codexandcursor-agentare refused because, going by their source, contained workers may be able to change the settings their agents run on. This is not checked live.12283408) is behind Buzz's bundled Goose 1.57.0, so protected agents lag unprotected ones until the pin moves. The review found no sessions-schema change in the gap.stoppingslot, which can leave an untracked worker; andprotectionContextcallsentry.worker.get()directly, so a tool call during the old worker's kill grace can start the new worker early. Both are tracked in the follow-up issue, with the Goose pin plan and the cold-start measurement.enabledneeds a companion engine change.statusandreviewcommands and itscheckerprocess asenvresolved onPATH([1/5] Add native agent protection and policy boundary #1875 finding A). Thereviewcommand also records approval decisions. The host refuses absolute program paths, and only host processes can declare a hash-checked artifact. So thecheckerprocess could move to one in the companion, while thestatusandreviewcommands need a Buzz host change first. Tracked in squareup/buzz-plugin-security#4.Validation
Restack. Rebased with the stack onto main
066e34f12: nowcef48c69c, still one commit on #1879 (6a2529319). One conflict, with main's saved session prompts (#1855): an opened session records its prompt first, then is closed if the harness was removed meanwhile. At the stack tip (b85084bff):pnpm check(Biome,tsc, design checks), the integration suite (248 passed, 1 skipped), the full Vitest run (16,072 passed, 22 skipped),cargo test -p buzzodz-plugins(64),cargo test --libinsrc-tauri(841 passed, 2 ignored), Clippy (-D warnings) andcargo fmt --checkpass. Onesrc-tauritest was left out:window_state::tests::onboarding_snapshot_is_kept_until_next_launch_restorecrashes (SIGSEGV) on main as well (seen at7cb3cb2c1), and this stack doesn't changewindow_state.Round 2, checked in the fix worktrees before the restack:
tsc --noEmitis clean. Their three tests fail on the unbounded waits, and the shared-worker one also fails when the worker is forgotten.tsc --noEmitandbiome checkon the changed files are clean in both.cargo test --lib host_process(35, including the new protection tests), Clippy (-D warnings) andcargo fmt --checkpass.github_copilot,codexandcursor-agentare refused before a worker starts, an unnamed provider uses Goose's default read in the review worker, and the review-model list hides the refused providers;get()meanwhile starts no second goose;5b56d68: 126 Node tests pass. The Python engine tests ran inside a sandbox that blocks the real plugin storage; the 4 that failed there need/bin/ps, a nested sandbox or that storage, and the 3 engine tests among them pass outside it. The relocated-bundle install test hasn't run outside the sandbox. The engine patch has not been compiled.The checks below ran before round 2.
Rebased onto main
61a67d28f: now1b4cd9990(stack tip80471b41c). At the new tip, the agent, Buzz MCP, relay, Builderlab, Inbox, Channels and scripts suites (6,882 passed, 22 skipped),tsc --noEmit,cargo test --lib host_process(33),cargo test -p buzzodz-plugins(64), Clippy (-D warnings) andcargo fmt --checkpass. The checks below ran before the rebases.At
47b80eb6f(stack tip6b514fff4), on main6dc462a2d:tsc --noEmitis clean. At the stack tip, the Claude, Codex, Goose and Pi plugins, agent harness, agent protection, agents, Buzz MCP, Builderlab and scripts: 3,000 passed, 18 skipped.cargo test --lib host_process(33) andcargo test -p buzzodz-plugins(64) pass, and Clippy (-D warnings),cargo fmt --checkandbiome checkare clean.46e5fc5: 117 Node and 38 Python engine tests pass, including the new protection tests. I didn't run the install tests locally, because they would touch the real plugin storage.Not yet done
goose-acpis not rebuilt from it or its provenance regenerated.goose-acp, and what a denied or unanswered prompt does; protected Goose never writingconfig.yaml; a protected launch with the missing Goose paths listed (it should start, and a worker should be unable to create them); the Copilot,codexandcursor-agentrefusals against real providers; and review cancellation and deadlines against a real model.Code flow
Goose gates native conversation input, including results, and checks tool arguments with history before dispatch. Policy changes require strict ACP retirement.
Illustrative flow; checks shown enabled. Source: runtime.ts, agent-sessions.ts.
Ownership across the stack
Approved media from earlier snapshots
These recordings and screenshots predate the current fixes. The two-agent demo used host
831386161d561678c92c9ce59883162e7261a68cand plugin6d1528a; the earlier single-agent clips used the snapshots named below. They illustrate those flows and do not validate this head.Independent Claude/Codex approvals; approving Codex leaves Claude paused.
01-two-agent-approval.mp4
Codex continues to its requested summary.
02-codex-reply.mp4
Earlier single-agent Ready reply (
7c3dba764).demo-ready-success.mp4
Earlier quoted-injection denial (
9ed4ebda2).demo-injection-denied.mp4
Original approved demo post.
Review and merge bottom to top:
Above these: #2088 (exact history records) and #2105 (denial reasons for the owner).
🤖 Generated with Claude Code