Add bb thread import for existing ACP sessions - #1047
Conversation
|
TY for the pr @mgpai22 will take a look tomorrow |
1fa96a2 to
d624d6d
Compare
mgpai22
changed the title
d624d6d to
e137874
Compare
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request for security, code quality, architecture, performance, and practical end-to-end behavior. |
| deps: Pick<ThreadImportDeps, "db">, | ||
| args: { hostId: string; providerSessionId: string }, | ||
| ): void { | ||
| const existingThreadId = findLiveThreadIdByProviderThreadId(deps.db, { |
There was a problem hiding this comment.
🚨 slopcop/review — Make the provider-session claim atomic across environments.
This lookup only sees identity events that a completed start already stored. Two imports for different project workspaces can both pass before either identity event exists. Each environment owns a separate runtime and ACP bridge, so the process-local reservation does not protect this race. Both threads can then load and continue the same external session. Add a server-owned reservation with a unique key on (hostId, providerId, providerSessionId). Claim it in the thread-create transaction. Release it after failed creation or permanent deletion. Add a concurrent test that uses two environments.
| ); | ||
| }, | ||
| get: getThread, | ||
| async import(input) { |
There was a problem hiding this comment.
🚨 slopcop/review — Preserve automatic plugin attribution for imports.
This SDK method defaults the origin to sdk. The plugin SDK wrapper adds plugin attribution only for spawn and fork. Thus, bb.sdk.threads.import() creates a thread without origin: "plugin" and originPluginId. Extend wrapSdkForPlugin with an import wrapper that uses the same rules. Add a plugin API test for default and explicit origins.
| command: Extract< | ||
| AdapterCommand, | ||
| { type: "thread/start" | "thread/resume" } | ||
| { type: "thread/start" | "thread/resume" | "thread/import" } |
There was a problem hiding this comment.
🚨 slopcop/review — Use the validated import directory for this command.
The server validates the request directory and uses it for the thread environment. This shared builder then selects profile.cwd ?? command.cwd. A custom ACP agent with a configured directory therefore sends a different path to session/load. The thread attaches to one project while the provider loads another directory. Use command.cwd for thread/import, or validate the effective profile directory at the server boundary. Add a custom-agent directory test.
| */ | ||
| export function findLiveThreadIdByProviderThreadId( | ||
| db: DbQueryConnection, | ||
| args: { hostId: string; providerThreadId: string }, |
There was a problem hiding this comment.
🚨 slopcop/review — Include the provider ID in this lookup key.
Provider session IDs belong to a provider namespace. This query uses only the host and session ID. A session named abc in acp-omp can therefore block a valid abc import from acp-opencode on the same host. Pass providerId into this function and filter on threads.providerId. Use the same provider dimension in the atomic reservation.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5
This pull request lets BB adopt a conversation that started in an ACP agent. BB copies the old conversation into a thread, then lets the user continue it.
Findings
I found four issues that need fixes:
- Two imports can bind the same provider session when they use different environments. The database check is not atomic, and each environment has a separate bridge process.
- The duplicate lookup omits the provider ID. Equal session IDs from two different ACP providers can block each other.
- A custom ACP agent can replace the validated import directory with its configured directory. The provider can load a path that the server did not validate.
- The new SDK import method loses automatic plugin attribution. The plugin SDK wrapper covers spawn and fork, but it does not cover import.
The server should own a persistent provider-session reservation. The reservation key should include the host, provider, and provider session. This design also removes the duplicate event-log lookup policy from environment-local bridge processes.
I found no command injection, path traversal, new secret exposure, or direct authorization bypass. The daemon protocol version correctly moves from 77 to 78. The CLI, SDK, route, guide, and built-in skill surfaces exist.
Validation
- All selected tests passed: 840 agent-runtime, 361 database, 49 host-daemon-contract, 41 server-contract, 86 SDK, and 1,347 server tests.
- All seven selected package type checks passed.
- The selected package builds passed.
- The built
bb thread import --helpcommand passed.
I did not use a browser test. This change adds an API, SDK, and CLI creation path, but it adds no browser import action. A full live check also needs a real external ACP session. The adapter and server integration tests cover replay, lifecycle, and route behavior with the fake ACP agent.
I posted this as a comment-only review. I did not approve the pull request or request changes through GitHub.
e137874 to
976b590
Compare
Adds supportsSessionImport to provider capabilities (true for ACP providers, whose session/load maps to import; verified live at session open), the /threads/import request contract, the thread.start sessionImport wire descriptor, and the historical marker on turn framing events so replayed history persists without lifecycle side effects. Bumps HOST_DAEMON_PROTOCOL_VERSION to 72 for the thread.start payload change.
POST /threads/import validates the imported session's cwd against the project source (or an existing project workspace) and creates a thread carrying a sessionImport descriptor through provisioning to thread.start. The runtime maps it to a new thread/import adapter command; the ACP bridge opens the session via session/load (no fresh-session fallback, clear errors when the agent lacks loadSession or the load fails) and forwards the replayed history as historical updates instead of dropping them behind the loading gate. Historical turn framing settles the thread idle with no live turn state or server lifecycle effects, and replayed user_message_chunk updates become userMessage items.
bb thread import --project --provider --provider-session creates a thread bound to an existing external ACP session, with optional --host, --cwd, --title, --permission-mode, and --visibility. The SDK exposes threads.import and the bb-cli skill documents the workflow.
The fake ACP agent can now fail session/load and replay a scripted history during it. Bridge tests prove the replay is forwarded as historical updates, that imports refuse agents without session/load, and that load failures surface a clear error instead of a fresh session. Adapter tests cover the historical replay translation; route tests cover the sessionImport thread.start dispatch, cwd mismatch refusal, and the capability gate.
…agents Refuse importing a provider session another live thread already binds (server-side 409 via a new findLiveThreadIdByProviderThreadId reverse lookup, plus a bridge-side guard so the process-local routing map is never silently overwritten). Derive supportsSessionImport from the agent's live `initialize` handshake instead of trusting the static ACP-family constant, so an agent without session/load is refused before an environment is provisioned and a doomed thread.start is dispatched; this needed a new provider.list_models result field, bumping HOST_DAEMON_PROTOCOL_VERSION. Also close two historical-replay gaps: exclude historical turn/completed rows from start-activation staleness so a replayed frame can never strand a thread in "starting", and close the synthetic historical turn with a cancelled turn/completed when session/load fails after a partial replay so it doesn't stay open forever.
The in-CLI guide (bb-guide-threads.md) documented Forking but never gained an Importing section when the import verb shipped, so agents reading it in-app never learn the command exists. Add it alongside the other flags in the same style, and regenerate the derived template and plugin-sdk bundle outputs.
…nd lossy replay Move the duplicate-binding check before session/load so a rejected import never forwards history for a provider session it won't end up owning, and close any historical turn that still slips through a later race instead of leaving it open forever. Probe supportsSessionImport for ACP agents whose model list comes from a CLI command too, since they previously never reached the session-discovery path that populates the capability cache. Replace dropped non-text replayed user message chunks with a placeholder instead of losing them, since a later resume drops replay entirely. Require an explicit --cwd for `bb thread import`: bb has no way to read the external session's real working directory back from it, so a silently-defaulted cwd made the mismatch refusal unreachable for the common case. Add a runtime-level test covering the historical-replay bypass that skips turn/background/idle/goal state machines for imported history.
findLiveThreadIdByProviderThreadId ordered by events.sequence, a per-thread counter, so across multiple matching threads it picked whichever had logged the most events rather than the one most recently touched. Order by createdAt instead. The lookup also had no supporting index and scanned the full events table on every thread import; add a partial index on provider_thread_id.
…import A stranded replayed user message survived a torn-down historical turn (e.g. thread/stop racing ahead of the trailing turn/completed) and leaked into the next live turn as a phantom userMessage that was never actually sent; clear the accumulator alongside the other per-turn state. Concurrent thread/import requests for the same provider session both passed the pre-load unbound check (neither was bound yet) and both forwarded replayed history before the loser was rejected. Reserve the provider session id right after the pre-load check, before session/load goes out, so a losing concurrent import fails before it can dispatch session/load at all. The session/load capability cache expired independently of, and earlier than, the model-discovery cache it's learned alongside, silently dropping supportsSessionImport from model/list in the gap. Re-stamp both caches together. Also add negative caching and a bounded timeout to the CLI-agent capability probe so a broken or slow agent isn't re-spawned on every model/list call and can't push an otherwise-fast CLI catalog reply toward the server's command timeout. The capability probe resolved its launch spec from the static known-agent table only, so a custom ACP agent shadowing a built-in provider id was probed against the wrong binary, and a purely custom agent skipped the probe entirely and fell back to the static ACP-family allow. Share the same custom-agent-aware resolution thread.start uses. Fix the @bb/server-contract allowlist claiming importThreadRequestSchema.cwd is optional when the schema requires it, and cover the previously-untested workspace and cross-project branches of the import cwd check.
…nd scope duplicate checks by provider
…nt pins a directory
…isting thread/import's readiness check needs a fresh session/load capability answer before binding a thread to an external session, but every other model/list caller (picker, default-model resolution) should keep answering from the capability cache instead of paying an extra agent spawn. Thread the opt-in through the wire contract, bridge, and adapter as `probeSessionImport`, gated only on the import's own call. Also release a thread's provider-session reservation when its async thread.start fails before the import ever binds, so retrying the same import doesn't 409 forever against an orphaned reservation, and keep resolving import cwd the same way thread.start does so a configured ACP agent's pinned directory can't diverge between import and resume.
…ls before thread.start An import's reservation is claimed synchronously when the thread is created, before provisioning ever runs. failThreadProvisioning and the environment.provision failure path only marked the thread errored without releasing it, so a provisioning failure before thread.start was ever dispatched left the reservation stranded and every retry of the same import 409ed forever.
When a configured custom ACP agent pins its own cwd, the adapter always resolves profile.cwd over the caller's asserted --cwd, so a mismatched assertion was silently discarded instead of refused — violating the required cwd field's own contract that an import can never silently bind to the wrong directory. Reject the import instead, and update the CLI, contract, and docs to describe the pin-must-match-assertion requirement.
A resume whose session/load fails falls back to a fresh provider session, but the bridge still emits thread/identity for it, leaving the thread's original import reservation pointing at a session it no longer holds. Release the reservation when a recorded identity supersedes it, and stop findLiveThreadIdByProviderThreadId from treating a session a thread has since rebound away from as still live, so a stranded session can be re-imported once its thread moves on.
executeLiveDaemonCommandBody only flushed the event sink after dispatchCommand resolved, so a thread.start failure (e.g. a session/load timeout) could report back to the server before an already-buffered thread/identity reached it. That let the server infer "bind never completed" from a stale event log and release an import reservation the bridge was about to finish claiming. Flush in a finally block so buffered events are always reported ahead of the command result.
976b590 to
e662fa5
Compare
|
All four findings are addressed, and the branch is rebased onto current main (protocol version now 84). F1 — atomic reservation. New I kept the event-log check alongside the reservation rather than replacing it: sessions bound by plain start/resume, and imports created before this migration, exist only in the event log. The reservation is checked first and is the race-safe source of truth. F2 — plugin attribution. F3 — validated cwd. F4 — provider-scoped key. Protocol version is bumped only for the earlier I also dropped two commits that were only working around slowness on my machine: the |
Implements #1028.
Adds
bb thread import, which adopts an existing external agent session as a bb thread: the replayed history lands in the timeline, and the thread continues that same provider session.How it works
The ACP bridge already calls
session/loadwhen resuming a bb-created thread, and drops the replayedsession/updatestream behind thesession.loadinggate. That is correct for resume, because bb already holds that history.Import reuses the same call with a caller-supplied session id, and forwards the replay instead of dropping it. Replayed events are marked
historical: they persist as timeline events but skip turn lifecycle effects, so the thread lands idle without running a turn. Live turns after that behave normally.Also included:
supportsSessionImport, derived fromagentCapabilities.loadSessionat initialize, with a live per-agent probe on top of the static ACP-family default. Providers without it are refused.HOST_DAEMON_PROTOCOL_VERSIONgoes to 75.thread.startgained thesessionImportdescriptor andprovider.list_modelsgained the optionalsupportsSessionImportresult field; an old daemon would drop the descriptor and silently start a fresh, history-less session.Verification
Tests cover the bridge (replay persisted, unsupported agent refused, load failure surfaced), adapter translation, and the route (cwd mismatch, unsupported provider) against real in-memory sqlite.
Live run against omp 17.2.9: a 2,404-message session imported as 1,994 persisted events (993 completed items, including tool calls and reasoning), thread idle, no model calls. Bogus session id and cwd mismatch both refuse with a clear error.
Known limits
session/loadtakes cwd as input and it cannot be probed from the agent, so validation is against the project's own paths.Two commits touch test infrastructure unrelated to the feature: a
@bb/servervitest timeout raise, and hermeticity fixes ininstall-machine-script(a globally installedbb-appon PATH leaks into the test that asserts its absence) andinternal-skill-trees(hardcoded 0644 vs a 0002 umask). Happy to split those out.