Skip to content

fix(pi): protect bridge JSON-RPC stdout - #1208

Merged
SawyerHood merged 3 commits into
get-bb:mainfrom
raincodes64:fix/pi-bridge-stdout
Aug 8, 2026
Merged

fix(pi): protect bridge JSON-RPC stdout#1208
SawyerHood merged 3 commits into
get-bb:mainfrom
raincodes64:fix/pi-bridge-stdout

Conversation

@raincodes64

@raincodes64 raincodes64 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Reserve the Pi bridge stdout pipe for protocol frames and redirect extension output to stderr. Add regression coverage for OSC notification output.

Fixes #1180

@SawyerHood

SawyerHood commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

🚨 SLOP COP 🚨 · review

I am SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and test coverage.

SawyerHood
SawyerHood reviewed Aug 8, 2026
View reviewed changes

@SawyerHood SawyerHood left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 SLOP COP 🚨 · review

ELI5: The bridge needs a clean JSON message line. This change moves extension text away, but the new path can stop and fill memory.

I found two merge blockers:

  1. High — The redirect breaks stdout flow control. It reports stderr backpressure through stdout. A caller can then wait for drain on the wrong stream and stop forever.

  2. High — Redirected output has no memory limit. The parent stores every stderr line for the process lifetime. Newline-free OSC text also stays in the readline buffer. The exit formatter joins all data before it keeps the last 4,000 characters.

Security and architecture:

  • The Pi-specific guard has the correct scope because Pi loads extensions in its bridge process.
  • The guard does not isolate hostile code. Direct file-descriptor writes, saved writers, and inherited stdout can bypass it.
  • I treat those bypasses as a documented limit, not a blocker for this transport fix.
  • The shared provider-process boundary should use the existing ACP or Claude bounded-tail pattern.

Validation:

  • Three independent GPT-5.6 reviews covered security, code quality, architecture, and performance.
  • The final GPT-5.6 gate confirmed both findings.
  • GitHub CI passed all required jobs.
  • git diff --check passed.
  • I did not execute code from this untrusted fork.
  • This backend bridge change has no browser route, so browser end-to-end testing does not apply.

I submitted this as a comment-only review. Fix both stream issues and add tests for backpressure and bounded capture.

raincodes64 and others added 3 commits August 8, 2026 23:12
@raincodes64 @SawyerHood
fix(pi): protect bridge JSON-RPC stdout
ead59fb 
Reserve the Pi bridge stdout pipe for protocol frames and redirect extension output to stderr. Add regression coverage for OSC notification output and bump the host daemon protocol version.
@SawyerHood
fix(pi): address stdout guard review findings
3765b9d
@SawyerHood
test(db): allow time for client request key batching
0dcda51
@SawyerHood
SawyerHood force-pushed the fix/pi-bridge-stdout branch from ada53e7 to 0dcda51 Compare August 8, 2026 23:13
@SawyerHood
SawyerHood merged commit 0cd5011 into get-bb:main Aug 8, 2026
9 checks passed
@tymonTe tymonTe mentioned this pull request Aug 11, 2026
Merged
SawyerHood pushed a commit that referenced this pull request Aug 11, 2026
@tymonTe @claude
fix(host-daemon,db): stop one undeliverable event from wedging every …
5f71d2c 
…thread (#1321)

# Problem

Every thread on the host freezes at "waiting" and only a full app
restart clears it. This has now happened **four times on bb-app
0.36.0**, each time triggered by a single event the server could never
store.

The daemon holds **one in-memory event queue for the whole host** and
reposts it as a single batch. When the head of that queue is an event
the server deterministically refuses, the batch can never succeed — so
every other thread's `turn/started`, `item/*` and `turn/completed`
events pile up behind it and never reach the database. The UI reads the
database, so every thread looks stuck.

### From the logs

`~/.bb/logs/server.3.log` — the first rejection, then the same one
repeating verbatim:

```json
{"level":40,"time":1786360836499,"eventType":"provider/unhandled","scopeKind":"turn",
 "threadId":"thr_fpx3vkax5h","turnId":"auto-compact-1",
 "errorMessage":"Cannot append provider/unhandled for turn auto-compact-1 before turn/started is stored",
 "errorName":"MissingStoredTurnStartedError","msg":"Rejected daemon event before turn/started"}
{"level":40,"time":1786360836616,"eventType":"provider/unhandled","scopeKind":"turn",
 "threadId":"thr_fpx3vkax5h","turnId":"auto-compact-1", ... }
```

Every occurrence, grouped by the turn that poisoned the queue:

| Thread | Turn | Rejections | Window |
|---|---|---:|---|
| `thr_dwmzmanhn5` | `auto-compact-2` | 1911 | 08-07 14:57:19 → 15:27:06
(29.8 min) |
| `thr_fpx3vkax5h` | `auto-compact-1` | 505 | 08-10 13:20:36 → 13:25:57
(5.3 min) |
| `thr_sdc5dy277m` | `auto-compact-3` | 171 | 08-10 13:48:47 → 13:52:43
(3.9 min) |
| `thr_qifimqh4a6` | `auto-compact-1` | 260 | 08-10 15:08:46 → 15:14:00
(5.2 min) |

Every window ends at a restart, never at a recovery.

During the 13:20 window the server logged **no thread activity
whatsoever** — only the rejections:

```
1  [plugin:connect] rpc listAccountServers failed: not_paired
5  Skipping malformed prompt history row
1  [plugin:agent-limits] disposed        <- the restart
```

The 15:08 occurrence is visible directly in the database. Rows inserted
per minute across all threads, spanning that window:

```
15:03 |  90 events | 4 threads
15:04 |  91        | 1
15:05 |  66        | 1
15:06 |  22        | 1
15:07 |  32        | 1
15:08 |  35        | 2     <- poison event lands at 15:08:46
15:09 |   0        | 0
15:10 |   0        | 0
15:11 |   1        | 1
15:12 |   0        | 0
15:13 |   4        | 2
15:14 |  29        | 4     <- restart at 15:14:00
15:15 |  48        | 3
```

Five minutes in which the whole machine persisted essentially nothing,
then instant recovery on restart. Those events are gone: the queue is
in-memory, so the restart that clears the wedge also discards everything
held behind it, leaving a hole in each affected thread's transcript.

# Root cause

1. **A provider-minted turn id is trusted.**
`createUnhandledProviderEvent` falls back to reading `turnId` out of the
raw provider event when the caller does not supply one:

   ```ts
   const turnId = args.turnId ?? getTurnIdFromRawEvent(args.rawEvent);
   ```

Codex labels its automatic-compaction traffic `auto-compact-N`. The
string `auto-compact` appears nowhere in bb's source — it is entirely
provider-minted, and every `provider/unhandled` event on all four
affected threads carries `providerId: "codex"`. bb never opened that
turn, so it never emitted a `turn/started` for it. Critically, every
caller supplies `turnId` from bb's own turn registry and omits it *only
when bb has no active turn* — precisely the case where a scraped id is
guaranteed wrong.

2. **The server hard-rejects the orphan.**
`resolveDaemonTurnStartDisposition` finds no stored `turn/started`; the
escape hatch `ORPHAN_DROPPABLE_TURN_EVENT_TYPES` held only the two
usage-snapshot types, so it throws `MissingStoredTurnStartedError`.

3. **The whole batch dies with it.** `/session/events` appends every
event in one `immediate` transaction, so the throw rolls all of them
back and returns `409 invalid_request`.

4. **The daemon reposts it forever.** The drain loop takes the entire
queue as one batch and splices only on success:

   ```ts
   try { response = await options.postEvents(batch) }
   catch (error) {
logger.error(..., "Failed to post daemon events; will retry on the next
flush");
     return;                       // queue untouched
   }
   queue.splice(0, batch.length);  // only reached on success
   ```

The daemon already knows this class of error is permanent —
`defaultRetryableForStatus(409)` is `false`, and
`ServerResponseError.retryable` carries that verdict — but nothing
consults it.

# Fix

**1. `apps/host-daemon/src/event-sink.ts` — never repost a batch the
server permanently refused.** On a non-retryable `invalid_request`, the
sink bisects the batch, drops the events that are undeliverable by
construction, and delivers the rest. Since the server appends in one
transaction and rolls back entirely on refusal, nothing was committed
and re-posting the halves cannot duplicate. Isolating k bad events costs
O(k log n) posts.

The `invalid_request` code check is what keeps this narrow:
`/session/events` also fails non-retryably with `401 unauthorized` and
`401 inactive_session`, and those say nothing about the events
themselves. Those must stay queued for the session the daemon is about
to reopen, not be discarded one at a time — there is a regression test
for exactly this.

**2. `packages/agent-runtime/src/shared/provider-unhandled-event.ts` —
stop trusting provider turn ids.** Only a turn id the caller vouched for
scopes the event; the raw-event fallback is gone.

**3. `packages/db/src/data/events.ts` — `provider/unhandled` becomes
orphan-droppable.** A backstop, in the spirit of the existing comment
about fork usage snapshots. An unhandled passthrough event is diagnostic
only: losing one is a non-event, failing the batch it rode in with is
not. Turn-content events still require a stored `turn/started`, so
genuine ordering bugs are still caught.

**4. The queue-backup tripwire logs at `warn`, not `debug`.** It never
once fired in any of the four incidents, so there was no signal short of
noticing the UI had stopped moving.

Fix 1 is the load-bearing one. Fixes 2 and 3 close this particular
trigger; only fix 1 stops the *next* unknown orphan event from wedging
the host.

## Note on ordering of fixes 1 and 3

Fix 3 alone repairs already-enrolled daemons: an old daemon talking to a
new server stops receiving 409s, so the wedge cannot recur even before
it updates. Fix 1 is what makes the daemon resilient to the next unknown
case.

# Protocol version

Bumped `HOST_DAEMON_PROTOCOL_VERSION` 99 → 100, matching the convention
used by #1224, #1208, #1232, #1314 and #1236 for daemon-behaviour
changes. Nothing in the wire *schema* changed, and both directions are
compatible (old daemon + new server is in fact the repair path above) —
the bump is here to push fix 1 out to enrolled machines rather than
leave them on a build that wedges. Happy to drop it if you would rather
not force an update cycle for this.

# Tests

Written as reproductions first, and confirmed failing against the base
commit before the fix:

| Test | Package | Reproduces |
|---|---|---|
| `ignores a provider-supplied turn id the caller did not vouch for` |
`@bb/agent-runtime` | root cause — `auto-compact-1` scraped from raw
params |
| `drops orphan provider/unhandled events instead of failing the batch`
| `@bb/db` | the batch-wide rollback |
| `drops a permanently rejected event instead of retrying it forever` |
`@bb/host-daemon` | the infinite repost |
| `delivers events queued behind a permanently rejected event` |
`@bb/host-daemon` | **the wedge itself** — healthy traffic from other
threads gets through |
| `accepts a batch carrying a provider/unhandled event for a turn bb
never started` | `@bb/server` | end-to-end at the
`/internal/session/events` route that produced the 409 |

Plus guards against over-correcting:

- `keeps events queued when the session, not the batch, is rejected` — a
401 must not bisect the queue away.
- `keeps retrying a batch that fails for a retryable reason` — 5xx
behaviour unchanged.

One existing expectation changed: `codex/adapter.test.ts >
translateEvent unknown codex notifications fall back to
provider/unhandled` now expects thread scope. That path handles
notifications which *failed* schema parsing, so nothing there vouches
for the turn id; Codex notifications bb does parse still carry turn
scope. Comment in the test explains it.

## Verification

Rebased onto `d07c1ce28` and re-verified there. `pnpm exec turbo run
test` on `@bb/db`, `@bb/agent-runtime`, `@bb/host-daemon`,
`@bb/host-daemon-contract`, `@bb/server`, `@bb/integration-tests`:

```
@bb/host-daemon-contract    49 passed (49)
@bb/host-daemon            526 passed (526)
@bb/db                     378 passed (378)
@bb/server                1405 passed (1405)
@bb/integration-tests       55 passed (55)
@bb/agent-runtime          894 passed | 1 failed (895)
```

`typecheck` and `lint` clean across all of them.

The single `@bb/agent-runtime` failure —
`runtime.process-lifecycle.test.ts > bounds provider stderr while data
arrives without a newline` — **fails identically on unmodified
`origin/main`** and is unrelated to this change.


---

Fixes #1320

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
amadad pushed a commit to amadad/bb that referenced this pull request Aug 14, 2026
@raincodes64 @SawyerHood @amadad
fix(pi): protect bridge JSON-RPC stdout (get-bb#1208)
c945640 
Reserve the Pi bridge stdout pipe for protocol frames and redirect
extension output to stderr. Add regression coverage for OSC notification
output.

Fixes get-bb#1180

---------

Co-authored-by: Sawyer Hood <sawyerjhood@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SawyerHood SawyerHood SawyerHood left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

pi: thread stuck on Working...

2 participants

@raincodes64 @SawyerHood