Repository navigation
Conversation
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am now reviewing this pull request for security, code quality, performance, architecture, and test coverage. |
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5: A parent agent gave the BB daemon one thread folder as if it were the folder for all threads. This change stops that mistake.
I found one blocking regression.
The production launcher removes BB_THREAD_ID and BB_THREAD_STORAGE from runtime.env. The same environment then starts the bundled bb CLI. This breaks commands that use the current thread, including bb thread show --self and bb thread spawn --parent-self.
I reproduced the problem. The normal bb status command found this thread. The packaged launcher path reported Thread: (not set). The packaged bb thread show --self path failed because BB_THREAD_ID was absent.
Please keep thread context for the CLI path. Remove it only from long-lived server and daemon environments. Add a launcher test that starts the CLI child and verifies a context command.
The security review found no security defect. The performance review found only small extra environment copies. No daemon protocol version change is necessary because the wire contract does not change.
The architecture scan found similar root logic in the server and daemon. The active server path already forces data-directory root selection with an empty environment. The remaining ambient server fallback looks stale, but it does not block this fix.
A browser test does not apply because this change has no browser route. Launcher, daemon, and focused QA tests provide the correct coverage.
Checks passed:
- 708 affected package tests
- 12 focused QA tests
- Turbo type checks for
@bb/config,bb-app, and@bb/host-daemon - The
bb-appTurbo build git diff --check
I posted this as a comment-only review. I did not approve the pull request or request changes through GitHub.
|
Nested dev-server QA completed against the reporter-confirmed activation path.
A/B result:
The unpatched server log contains the issue exact error and stack: ApiError status 502, code invalid_path, message Thread storage path escapes the storage root, logged as Live thread start command failed through callHostOnlineRpcWithRetry -> runLiveHostCommand. Process inspection showed the outer agent variables in the unpatched server and daemon children. In the PR stack, BB_THREAD_ID, BB_THREAD_STORAGE, and BB_ENVIRONMENT_ID were absent from the server/daemon children while the launcher retained them for its CLI context. All isolated and dev processes were stopped after capture. |
Summary
bbCLIBB_THREAD_STORAGEConfirmed mechanism
Commit
1688734bb1bfdb26a844f868bf7f3d07ab9b0c2bmade the server derive per-host thread paths from the host session data directory instead of ambientBB_THREAD_STORAGE, but the daemon still adopted ambientBB_THREAD_STORAGEas its global root. When bb starts with a parent thread's environment, the daemon root narrows to the parent thread directory while the server sends a sibling worker path.requireConfinedPathcorrectly rejects that sibling as escaping the narrowed root.The standalone QA launcher and dev launcher already strip this context. The production launcher did not. This PR fixes both layers: production child processes are sanitized and the daemon no longer treats ambient per-thread context as global configuration.
Reproduction
I ran the issue's exact
pathprobeworkflow with Claude Haiku from a real bb-managed worktree:{"r":"ok"}{"r":"ok"}bb-app@0.37.0, clean environment{"r":"ok"}bb-app@0.36.0, clean environment{"r":"ok"}open -awith inherited thread contextThread storage path escapes the storage rootlogbb-app@0.37.0with the same inherited contextbb-app@0.36.0with the same inherited context{"r":"ok"}The reporter subsequently reconstructed the timeline and confirmed both the failing and passing runs were on 0.37.0. The failing instance was installed and launched with
open -aby a detached script started from a bb agent shell, which propagated that thread's environment into Desktop. The passing instance was cleanly relaunched by the built-in updater. This matches the controlled matrix exactly: the split is contaminated agent launch versus clean updater launch, not 0.36.0 versus 0.37.0.The divergence was introduced by commit
1688734bb1bfdb26a844f868bf7f3d07ab9b0c2b(Inject global app skills into agent runtimes): the server began deriving storage from the host session data directory while the daemon still adopted ambientBB_THREAD_STORAGE. The confinement check predates that commit. No commit in the 0.36.0..0.37.0 range introduced this failure.Users do not need to roll back: fully quit the contaminated Desktop instance and relaunch it from Finder, Dock, the updater, or another environment without thread-scoped
BB_*variables.Verification
pnpm exec turbo run test --filter=@bb/host-daemon --force— 537 passedpnpm exec turbo run test --filter=bb-app --filter=@bb/config --force— 171 passedpnpm exec turbo run typecheck --filter=@bb/host-daemon --filter=bb-app --filter=@bb/config<dataDir>/thread-storageNo
HOST_DAEMON_PROTOCOL_VERSIONbump is needed because no server/daemon message or RPC contract changes.Fixes #1366