Repository navigation
Apply Claude turn settings without replacing live sessions - #1236
Merged
Merged
Conversation
sholub-dev
force-pushed
the
per-turn-permission-escalation
branch
from
August 9, 2026 18:39
4aa085b to
1c28080
Compare
Collaborator
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request for security, code quality, architecture, performance, and end-to-end behavior. |
SawyerHood
reviewed
Aug 10, 2026
| return; | ||
| } | ||
|
|
||
| threadSession.permissionEscalation = params.permissionEscalation; |
Collaborator
There was a problem hiding this comment.
🚨 slopcop/review — A shared session value applies the newest turn policy to all background work.
Background tasks outlive their start turn. A later turn overwrites this shared value. An older system task can then read ask from a user turn and enter the approval path. The reverse change can also deny older user work. Please attach the policy to the work that created each tool request. Add overlap tests for both policy directions.
Contributor
Author
There was a problem hiding this comment.
The SDK gives canUseTool no link to the originating turn or task, so per-work policy isn't possible at this layer. The old behavior rebuilt the session on every escalation flip and killed background work — the bug this PR fixes. Both crossovers fail safe: ask only happens while a user turn is active, deny auto-denies.
SawyerHood
reviewed
Aug 10, 2026
| return; | ||
| } | ||
|
|
||
| threadSession.permissionEscalation = params.permissionEscalation; |
Collaborator
There was a problem hiding this comment.
🚨 slopcop/review — A failed steer leaves the new permission policy active.
This assignment occurs before pushInput confirms the steer. The error path returns an error without restoring the old value. A rejected ask steer can therefore loosen the policy for existing deny work. Update the value after acceptance, or restore it in the error path. Add a failed-steer test.
Contributor
Author
There was a problem hiding this comment.
Fixed in d4bf20e: escalation is set only after pushInput succeeds. Added a failed-steer test.
SawyerHood
reviewed
Aug 10, 2026
SawyerHood
left a comment
SawyerHood
left a comment
Collaborator
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5: This change keeps old Claude jobs alive. But it gives all jobs one permission switch. It is like one light switch for every room.
I found two defects. I recommend fixes before merge.
-
High — Background work crosses the per-turn permission boundary.
Background tasks outlive their start turn. Each new turn overwrites one session-wide
permissionEscalationvalue. An older system task can later readaskfrom a user turn and enter the approval path. The reverse change can deny older user work. Attach the policy to the work that created each tool request. Add overlap tests for both policy directions. -
Medium — A failed steer keeps its permission change.
The bridge changes the value before
pushInputsucceeds. The error path does not restore the old value. A rejectedasksteer can loosen existingdenywork. Update the value after acceptance, or restore it after failure.
Architecture and performance
The adapter capability is an acceptable abstraction. The two Claude permission checks serve different SDK enforcement points. I found no useful refactor requirement. The change removes session replacement and adds constant-time state access.
Validation
I verified the exact head SHA and reviewed all 14 changed files. Three newer base commits do not touch these files. git diff --check passed. All reported GitHub CI checks passed. The change does not require a host-daemon protocol increase.
The pull request comes from an untrusted fork, and this backend change has no browser route. I did not start a server or execute the pull-request code locally.
This is a comment-only review. I did not approve the pull request or request changes.
sholub-dev
force-pushed
the
per-turn-permission-escalation
branch
from
August 10, 2026 16:18
9269d62 to
d4bf20e
Compare
ymichael
force-pushed
the
per-turn-permission-escalation
branch
from
August 10, 2026 23:48
d4bf20e to
3b81ef3
Compare
ymichael
changed the title
ymichael
force-pushed
the
per-turn-permission-escalation
branch
from
August 11, 2026 02:30
3b81ef3 to
f79aeb7
Compare
Escalation follows the turn initiator, but it was pinned into Claude session construction, so every user/system turn alternation replaced the session and killed the background tasks inside it. Turn commands now carry escalation, the bridge adopts it in place, and the runtime records escalation-only changes without a session-replacing reconfigure.
ymichael
added a commit
to sholub-dev/bb
that referenced
this pull request
Aug 11, 2026
## Summary - resolve PRs through the configured upstream remote and branch when a BB managed local branch has a different name - preserve distinct no-PR and unavailable GitHub CLI outcomes - add real-Git regression coverage for fork remotes, renamed branches, no-PR, authentication, and missing CLI cases - bump the host-daemon protocol to 97 so enrolled hosts receive the corrected lookup behavior ## Validation - pnpm exec turbo run test --filter=@bb/host-workspace --force - pnpm exec turbo run test --filter=@bb/host-daemon-contract --force - pnpm exec turbo run typecheck --filter=@bb/host-workspace - pnpm exec turbo run typecheck --filter=@bb/host-daemon-contract --filter=@bb/host-daemon - live read-only lookup against env_yikdjjfryt resolved get-bb#1236
ymichael
force-pushed
the
per-turn-permission-escalation
branch
from
August 11, 2026 16:51
551feeb to
daae198
Compare
SawyerHood
pushed a commit
that referenced
this pull request
Aug 11, 2026
…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>
ymichael
added a commit
that referenced
this pull request
Aug 12, 2026
## Motivation The four provider adapters (claude-code, codex, pi, acp) and three bridges carry near-verbatim copies of the same machinery: stdio harness, session registry, adapter identity/command/translation boilerplate, turn lifecycle, item-id scoping, and tool item translation. That duplication has a measurable history of making cross-provider work slow and lossy: - **Manual compaction (#1152)** landed through 19 commits, more than 11 of them iterating the same feature provider-by-provider ("Make OpenCode ACP compaction reliable", "Avoid false Pi compaction success", "Show ACP compaction lifecycle", "Fix compaction lifecycle races", ...). One feature, re-fought per adapter. - **Rate-limit normalization (#1027)** only ever shipped to 2 of 4 providers. On main today, `ProviderRateLimitState` is referenced by claude-code and codex translation only — Pi and ACP have zero references. Duplication didn't just slow the change down; it silently dropped half the matrix. - **`classifyExecutionSettingsChange` (#1236)** had to touch all four adapters to change one provider's behavior, because the member wiring is copied everywhere. After this PR, the shared machinery lives in `packages/agent-runtime/src/shared/` and a cross-provider change of this kind is a one-place change. ## Honest accounting Repo-wide this is roughly **−300 lines net** (2,204 insertions, 2,512 deletions, all inside `packages/agent-runtime`). The ~1,640-line figure from the de-slop audit counts *duplicated* lines, not deletable ones — extracting a shared implementation still costs its one copy plus parameterization. The value of this PR is **one-place-change for cross-provider behavior**, not raw deletion. ## Commit-by-commit review guide The 8 commits are logically staged; each is independently revertable and behavior-preserving: 1. **`fff1c494c` share bridge stdio harness** — the three bridges' identical stdin/stdout JSON-RPC loop (readline wiring, dispatch, error replies) moves to `shared/bridge-harness.ts`. 2. **`e1e7615ba` share bridge session registry** — the per-bridge `Map`-of-sessions bookkeeping (create/get/stop/dispose) moves to `shared/bridge-session-registry.ts`. 3. **`66fee1a30` share bridge environment helpers** — env-override construction/parsing dedup in `shared/adapter-utils.ts` (round-trip covered by a new test), plus `shared/mime-types.ts`. 4. **`6e6e9fbc1` share standard adapter members** — the biggest one. Each adapter's copied identity/launch members, `initialize` plan, unsupported-command fallbacks, accepted-command translation, model-list parsing, and tool-call decoding collapse into `createStandardAdapterMembers()` (`shared/standard-adapter-members.ts`); adapters keep only their provider-specific `buildProviderCommandPlan`. Review focus: confirm each adapter's per-command plans are unchanged (they are moved, not edited). 5. **`4c6a3c65b` centralize provider turn hooks** — per-adapter `ensureXTurnStarted` wrappers and the copied error-envelope-to-events helper become `onTurnStart`/`onTurnFinish` hooks and `buildErrorEvents` on the shared turn-state registry; `shared/provider-error-events.ts` is deleted. 6. **`276516e32` share scoped item id factories** — the copied open/completed scoped-item-id resolution moves to `shared/scoped-item-ids.ts`. 7. **`fbd671c2a` share tool item translation** — the copied toolUse/toolResult item builders move to `shared/tool-item-translation.ts`. 8. **`86a313d7b` remove dead exports and helpers** — deletes exports and helpers made unreferenced by the extractions (verified by repo-wide typecheck). ## Wire compatibility (server ↔ host daemon) **No `HOST_DAEMON_PROTOCOL_VERSION` bump needed, and none is included.** The diff is confined to `packages/agent-runtime`; `packages/host-daemon-contract` (session payloads, WS messages, host RPC commands/results) is untouched and the protocol version stays at 106 on both sides. The refactor changes where adapter/bridge code lives, not what crosses the wire: the 900+ adapter tests that assert exact translated `ThreadEvent` output (ids, scopes, payloads) pass without any expectation changes, so the event stream the daemon posts to the server is byte-identical. ## Rebase provenance This branch originated in archived thread `thr_jyia7bkdqn` and was rebased onto current main (original authorship preserved in the commits). The rebase crossed #1280 (sent-message editing) and #1321 (provider-unhandled-event fix), both of which touched the same adapters; conflicts were resolved by preserving main's shipped behavior and re-grafting it into the extracted structure: - `thread/discard` command plans re-added per provider (acp: noop, claude-code: `thread/stop`, pi: `thread/discard`) inside each `buildProviderCommandPlan`. - Claude Code's `latestProviderCheckpointId` reset moved into the shared `onTurnStart` hook (fires only for newly opened turns, same guard as the removed wrapper). - `sourceProviderCheckpointId` forwarding re-added to the claude-code and pi `thread/fork` plans. The rebase delta vs the original branch inside `packages/agent-runtime` is exactly the sum of #1280 + #1321 (+1,124/−36), i.e. nothing from main was lost and nothing beyond the refactor was introduced. ## Verification - `pnpm exec turbo run typecheck` — whole repo, 58/58 tasks pass. - `pnpm exec turbo run test --filter=@bb/agent-runtime` — 907 passed, 1 failed: `runtime.process-lifecycle.test.ts > bounds provider stderr while data arrives without a newline`, which fails identically on unmodified origin/main (reproduced during this work; also documented as pre-existing in #1321). The lifecycle suites (`runtime.lifecycle.test.ts` 28/28, `runtime.process-lifecycle.test.ts` 21/21 excluding the pre-existing failure) covering the Codex recovery paths pass. - `pnpm exec turbo run test --filter=@bb/host-daemon` (the only dependent of `@bb/agent-runtime`) — 541/541 pass. > AGENT GENERATED: by Claude Fable 5 🤖 Generated with [Claude Code](https://claude.com/claude-code)
ymichael
added a commit
that referenced
this pull request
Aug 17, 2026
Third conformant bridge, same four moves (templates cab42e59a, 7c44fda2a, df8ede082). translate-message.ts and the adapter's translation closures move into event-translation.ts behind createClaudeEventTranslator (byte-identity verified, deviations enumerated); interactions.ts and session-params.ts extracted and shared by both dialects; the adapter shrinks 1373 -> 413 lines. Hygiene needed only constant/error-detail gaps - this bridge is where #853/#859 were fixed, and no test pinned a drop. Handshake facts with evidence: sessionRestore true (SDK resume), fork "checkpoint" (forkSession upToMessageId - also evidence for the plan's fork/rewind merge candidate), manualCompaction FALSE (finding: claude has no compact wire method; /compact rides prompt text), approvalRequestPolicy "provider" (canUseTool pre-filters, #1236 - the runtime skips reclassification via the handshake getter). Canonical surface: per-session entropy translators, identity-before- events, canonical options feeding the same applyLiveSessionSettings machinery (providerOptions omission = keep current), release with no fabricated interruption and NO background-task settlement (session stays resumable, the opaqueTaskIds rule), interrupt keeps full settlement, session/replaced wired at both rebuild sites with settlement strictly preceding it, and approval/AskUserQuestion/ExitPlanMode all as canonical interaction/request round trips. Conformance 11/11. Suites 969 default + 252 isolated (966+251 baseline all unchanged); daemon bundle builds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1235.
Claude background commands, workflows, and agents all live inside one Agent SDK session. bb previously treated execution-option drift as construction-time configuration, dispatched
thread/resume, and replaced that session. Alternating user/system turns therefore interrupted background work, and several other setting changes had the same destructive behavior.This PR makes reconfiguration provider-specific. Claude settings that can safely change in place now travel with each turn; only actual session-construction changes rebuild the provider session.
What changes
Agent/legacyTasktool chain, including SDK callbacks that omit agent metadata. Permission-request hooks provide an additional tool-level provenance boundary.setModelcontrol.applyFlagSettings, including transitions to and frommaxreasoning.Agent/legacyTaskandWorkflowtoggles are enforced live. They no longer depend on stale construction-timedisallowedTools, and disabling them rejects new launches without replacing the session or killing existing work.serviceTier: "fast"is normalized to"default"before validation, comparison, persistence, and provider command construction, so it cannot cause a rebuild or reach the Claude bridge.The permission path remains conservative: readonly enforcement resolves escalation at tool-call time, unsandboxed retries are automatically denied on escalation-denied turns, and that denial runs before cached session grants so an approval from an earlier user turn cannot leak into later system work.
The following remain session-scoped and still rebuild when changed:
HOST_DAEMON_PROTOCOL_VERSIONis bumped to 99 because the server no longer sends construction-time ClaudedisallowedToolsvalues for workflow/subagent settings. Version 98 is already used by qualified upstream PR lookup on the updated base.Validation
serviceTier: "fast"on both start and agent-origin steer. It retained one provider thread, preserved the background child's originalaskpolicy after the steer changed the current thread todeny, surfaced the dangerous write as a pending approval, and completed it afterallow_once.@bb/agent-runtime,@bb/server,@bb/host-daemon, and@bb/host-daemon-contract.bounds provider stderr while data arrives without a newlinelifecycle test, which is outside this diff.