Split BrainBar daemon and UI#298
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@BugBot review Please check for launchd lifecycle regressions, socket ownership/races, helper subprocess startup issues, and single-writer violations. |
|
@codex review Focus on the daemon/UI process boundary, launchd plist/install behavior, Phase D helper compatibility, and whether any UI path still starts an in-process server or writes directly. |
| } | ||
|
|
||
| # Install LaunchAgents (expands path to actual APP_DIR) | ||
| if [ "$DEV_BUNDLE_BUILD" -eq 0 ]; then |
There was a problem hiding this comment.
🟢 Low brain-bar/build-app.sh:321
The success message "[build-app] LaunchAgents installed — daemon and UI restart independently" at line 328 prints unconditionally, even when neither $DAEMON_PLIST_SRC nor $UI_PLIST_SRC exists and no LaunchAgent was actually installed. Consider only printing the message when at least one plist file exists and install_launchagent was invoked.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file brain-bar/build-app.sh around line 321:
The success message `"[build-app] LaunchAgents installed — daemon and UI restart independently"` at line 328 prints unconditionally, even when neither `$DAEMON_PLIST_SRC` nor `$UI_PLIST_SRC` exists and no LaunchAgent was actually installed. Consider only printing the message when at least one plist file exists and `install_launchagent` was invoked.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 983a1a353a
ℹ️ 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".
| let collector = BrainBarAppSupport.makeUIStatsCollector( | ||
| dbPath: dbPath, | ||
| targetPID: ProcessInfo.processInfo.processIdentifier, | ||
| brainBusEvents: BrainBusClient() | ||
| ) |
There was a problem hiding this comment.
Stop opening the database from the UI collector
In the split UI process, this still constructs a StatsCollector, whose initializer creates a BrainDatabase(path:); that connection opens READWRITE | CREATE, runs PRAGMAs, and calls ensureMigrations() on startup. So every BrainBar UI launch can still acquire write locks and mutate schema directly instead of only watching the daemon/socket, which violates the new single-writer process boundary and can race the daemon on the canonical WAL DB.
Useful? React with 👍 / 👎.
| runtime.install( | ||
| collector: collector, | ||
| injectionStore: nil, | ||
| database: nil | ||
| ) |
There was a problem hiding this comment.
Restore a daemon-backed command bar model
With runtime.database permanently installed as nil, BrainBarWindowRootView never creates its QuickCaptureViewModel because commandBarProvider.viewModel(database:) only becomes ready for a non-nil database. In menuBarWindow mode, the Capture/Search menu commands and hotkey now only show the popover with the “Warming memory…” placeholder forever, so the UI quick-capture/search path is disabled unless it is replaced with a socket-backed model.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 983a1a3. Configure here.
| targetPID: 0, | ||
| brainBusEvents: brainBusEvents | ||
| ) | ||
| } |
There was a problem hiding this comment.
UI dashboard permanently stuck in degraded state
High Severity
makeUIStatsCollector passes targetPID: 0 to DaemonHealthMonitor. Since DaemonHealthMonitor.sample() has a guard targetPID > 0 else { return nil } check, it always returns nil. This causes PipelineState.derive(daemon: nil, ...) to always evaluate as .degraded, and DashboardFlowSummary to always show "Pipeline visibility is degraded" with all pipeline indicators as .unavailable — regardless of actual daemon health.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 983a1a3. Configure here.
| server.onDatabaseReady = { [weak self] database in | ||
| Task { @MainActor in | ||
| guard let self else { return } | ||
| self.sharedDatabase = database |
There was a problem hiding this comment.
UI process writes to database violating single-writer design
Medium Severity
The UI process's StatsCollector creates a BrainDatabase(path: dbPath) that calls openAndConfigure() → ensureMigrations(), which executes DDL statements (CREATE VIEW IF NOT EXISTS, ALTER TABLE ADD COLUMN, etc.) against the same database the daemon owns. This violates the single-writer architecture the PR establishes, where BrainBarDaemon is documented as owning "the single-writer path."
Reviewed by Cursor Bugbot for commit 983a1a3. Configure here.
| NSLog("[BrainBar] Starting UI shell; daemon owns %@", BrainBarServer.defaultSocketPath()) | ||
| let collector = BrainBarAppSupport.makeUIStatsCollector( | ||
| dbPath: dbPath, | ||
| targetPID: ProcessInfo.processInfo.processIdentifier, |
There was a problem hiding this comment.
Stale log message claims socket ownership the UI lacks
Low Severity
The log message "Socket ready; database will self-heal" is a leftover from the pre-split code. The UI process no longer owns a socket or database — the daemon does. This makes diagnostic logs actively misleading during incident response.
Reviewed by Cursor Bugbot for commit 983a1a3. Configure here.
* fix: route BrainBar search through Python hybrid helper * fix: harden BrainBar hybrid helper startup * fix: address BrainBar hybrid review followups * fix: report hybrid helper launch failures * fix: keep source all unfiltered for entity routing * fix: bound hybrid helper socket waits * fix: harden hybrid helper fallback cleanup * fix: sanitize hybrid helper responses * feat: stream BrainBar brain bus events * refactor: use NSStatusItem popover shell * fix: prevent SIGPIPE from hybrid helper socket writes * fix: route BrainBar search through Python hybrid helper * fix: harden BrainBar hybrid helper startup * fix: address BrainBar hybrid review followups * fix: report hybrid helper launch failures * fix: keep source all unfiltered for entity routing * fix: bound hybrid helper socket waits * fix: harden hybrid helper fallback cleanup * fix: sanitize hybrid helper responses * feat: stream BrainBar brain bus events * refactor: use NSStatusItem popover shell * fix: prevent SIGPIPE from hybrid helper socket writes * fix: serialize BrainBus socket writes * refactor: split BrainBar daemon and UI (#298) * fix: refresh BrainBar stats before bus events * test: wait for BrainBar socket readiness * test: relax arbitration drain deadline * fix: open BrainBar UI stats database read-only (#300)
After PR #312 removed the FastAPI daemon, the UI process must open SQLite directly. BrainBarApp.applicationDidFinishLaunching was still calling runtime.install(injectionStore: nil, database: nil) — a holdover from when the daemon owned the DB and the UI consumed via socket. Since BrainBarRuntime.database / .injectionStore are @published private(set) and only mutable via install(), they stayed nil forever, leaving the command bar stuck on "Warming memory…" and the Injections tab on the "feed not wired" placeholder. Extract a testable BrainBarAppSupport.wireRuntime(_:dbPath:collector:) helper that opens BrainDatabase read-only (so the writer pidfile stays uncontended with the Python supervisor) and constructs an InjectionStore (which owns its own writable connection for ack writes). Replace the nil-hardcoded install call in BrainBarApp.swift with this helper. Add BrainBarRuntimeWiringTests as a regression guard — if anyone re- introduces the nil install, the test fails with a comment pointing back to PR #312 and the gating UI placeholders. Regression history: - fed97bb (PR #298): refactor split daemon + UI; UI consumed DB via daemon socket - 692839c / e609d84 (PR #312): removed FastAPI daemon — UI was left waiting for a daemon that no longer exists Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>


Summary
BrainBarDaemonSwiftPM executable that owns the BrainBar MCP socket, brain bus, single-writer server path, and hybrid helper lifecycle.BrainBaras the LSUIElement UI process withNSStatusItem/NSPopoverfrom item test: QA integration tests + fix FTS5 query escaping #6 and a reconnectingBrainBusClientsubscriber from item feat(engine): Think/Recall/Sessions intelligence layer #5.build-app.shto build both products, embed both binaries intoBrainBar.app, and install twoProcessType=InteractiveLaunchAgents.schema_migrations.detailspresent.README.md.30-second lecture demo
Expected: killing
BrainBardoes not remove/tmp/brainbar.sock; the daemon still answers MCP; kickstarting the UI relaunches the menu-bar process.Verification
swift test --package-path brain-bar --filter DatabaseTests/testSchemaMigrationsTableIncludesDetailsForHybridHelperParityfailed before the Swift schema migration addeddetails.pytest tests/test_brainbar_build_app_guards.py -q-> 13 passed.swift build --package-path brain-bar --product BrainBar-> passed.swift build --package-path brain-bar --product BrainBarDaemon-> passed.BrainBarDaemonwith tempBRAINBAR_SOCKET_PATH+BRAINBAR_DB_PATH;initializesucceeded andtools/listreturned 16 tools over UDS.swift test --package-path brain-bar-> 363 passed.Note
Medium Risk
Splits BrainBar into two cooperating processes and updates launchd packaging/build scripts, which can impact startup/IPC reliability and deployment behavior. Also adjusts SQLite schema migration compatibility, which could affect fresh DB initialization if incorrect.
Overview
BrainBar is split into two SwiftPM executables:
BrainBarDaemonnow owns the MCP server and/tmp/brainbar.sock, whileBrainBarbecomes a UI-only menu bar shell that no longer starts/stops the server or requires database readiness for URL handling.The build and install flow is updated to build/embed both binaries into
BrainBar.appand install twoProcessType=InteractiveLaunchAgents (com.brainlayer.brainbar+com.brainlayer.brainbar-daemon), with new tests and a new daemon LaunchAgent plist.SQLite startup migrations are tweaked to ensure
schema_migrations.detailsexists (and is backfilled viaALTER TABLEwhen missing) to maintain compatibility with the hybrid helper.Reviewed by Cursor Bugbot for commit 983a1a3. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Split BrainBar into separate UI and headless BrainBarDaemon processes
BrainBarDaemonexecutable (BrainBarDaemonMain.swift) that owns the MCP server, Unix domain socket (/tmp/brainbar.sock), SQLite DB, and helper process lifecycle, running as a persistent launchd agent via com.brainlayer.brainbar-daemon.plist.BrainBarUI app (BrainBarApp.swift) no longer starts or manages the server, database, orInjectionStore; it assumes the daemon is already running.Sources/BrainBarDaemon/rather than duplicated.BrainDatabase.ensureChunkColumnsnow adds adetailscolumn toschema_migrationsif absent.isReadyToHandleBrainBarURLalways returnstrue.📊 Macroscope summarized 983a1a3. 7 files reviewed, 2 issues evaluated, 0 issues filtered, 1 comment posted
🗂️ Filtered Issues