fix: desktop harness bundling + council top-5 harness fixes - #26
Conversation
…ap detection
1. Add Elixir tests to CI (erlef/setup-beam, mix test, dep cache)
2. Supervisor :one_for_one → :rest_for_one (Storage crash cascades correctly)
3. Emit session/closed in terminate/2 for all 5 providers (fixes stale snapshots)
4. Authenticate /api/* HTTP routes with harness_secret (parity with WebSocket auth)
5. Activate gap detection — replay_from_sql_or_empty returns {:gap,...} on SQL failure
All findings verified against current code via multi-agent council analysis.
88 Elixir tests pass, 0 failures.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 56 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR integrates an Elixir harness release into the T3Code desktop application. It adds CI/build steps for Elixir toolchain and testing, implements desktop process management to launch and monitor a bundled harness, configures harness runtime and database paths, adds session closure event emissions across all provider sessions, enables release builds, and introduces new council dispatch tooling ( Changes
Sequence DiagramssequenceDiagram
participant Desktop as Desktop App
participant Harness as Harness Process
participant Backend as Backend Server
participant FileSystem as File System
Desktop->>Desktop: bootstrap()
Desktop->>Harness: startHarness()
Harness->>Harness: spawn harness release
Harness-->>Desktop: harnessPort, harnessSecret
Desktop->>FileSystem: write desktop-port file
Desktop->>FileSystem: write desktop-token file
Desktop->>Backend: startBackend(with harness config)
Backend-->>Desktop: backendPort, backendAuthToken
Desktop->>Desktop: application ready
Note over Desktop,Harness: On shutdown...
Desktop->>Harness: stopHarness()
Harness->>Harness: terminate process
Desktop->>Backend: stop backend
sequenceDiagram
participant Script as council-dispatch.ts
participant T3Client as T3Client
participant Server as t3code Server
participant Provider1 as Provider 1
participant Provider2 as Provider 2
Script->>T3Client: connect()
T3Client->>Server: WebSocket handshake
Server-->>T3Client: server.welcome push
T3Client-->>Script: connected
Script->>Script: parse CLI args (prompt, providers)
Script->>T3Client: send(thread.create)
T3Client->>Server: request with id
Server-->>T3Client: response with id
T3Client-->>Script: threadId
Script->>T3Client: send(thread.turn.start, prompt)
T3Client->>Server: dispatch to Provider 1
T3Client->>Server: dispatch to Provider 2
Server->>Provider1: execute with prompt
Server->>Provider2: execute with prompt
Script->>T3Client: disconnect()
Script->>FileSystem: write manifest.json
Script-->>Script: exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
size:XL
vouch:trusted
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| // Desktop mode may not include bootstrapProjectId in welcome — look it up | ||
| if (!projectId) { | ||
| const snapshot = (await client.request("orchestration.getSnapshot", {})) as { |
There was a problem hiding this comment.
🔴 Calling non-existent client.request() method causes runtime TypeError
scripts/council-dispatch.ts:141 calls client.request("orchestration.getSnapshot", {}), but the T3Client class (scripts/lib/t3-client.ts:47-202) does not have a request method. The available methods are send, dispatch, connect, onPush, and disconnect. This will throw TypeError: client.request is not a function at runtime when the fallback project lookup path is reached. The scripts directory typecheck is skipped in CI (scripts/package.json has "typecheck": "echo 'scripts typecheck skipped in CI...'").
| const snapshot = (await client.request("orchestration.getSnapshot", {})) as { | |
| const snapshot = (await client.send("orchestration.getSnapshot", {})) as { |
Was this helpful? React with 👍 or 👎 to provide feedback.
| return false; | ||
| } | ||
|
|
||
| harnessPort = 4321; |
There was a problem hiding this comment.
🟡 Hardcoded harness port 4321 causes port conflict with multiple desktop instances
When starting a bundled harness release, harnessPort is hardcoded to 4321 at apps/desktop/src/main.ts:988. If a user launches multiple desktop instances, all harness processes attempt to bind the same port, causing the second (and subsequent) instances to fail. The backend port is properly dynamically allocated via NetService.reserveLoopbackPort() (apps/desktop/src/main.ts:1441-1444), but the harness doesn't receive the same treatment. The port should be dynamically allocated, similar to how the backend port is handled.
Was this helpful? React with 👍 or 👎 to provide feedback.
Matches the before-quit handler pattern. Prevents orphaned harness child process if app.quit() fails after signal delivery. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ap logging
1. Use Plug.Crypto.secure_compare for harness secret (timing attack mitigation)
2. Move session/closed emission after maybe_complete_turn in cursor terminate/2
(prevents state-changed→ready firing after session/closed)
3. Log SQL replay failure reason before returning {:gap,...}
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| stopHarness(); | ||
| stopBackend(); |
There was a problem hiding this comment.
🔴 SIGINT/SIGTERM handlers stop harness before backend, opposite of before-quit handler
The new stopHarness() calls in the SIGINT and SIGTERM handlers are placed before stopBackend(), but the existing before-quit handler at apps/desktop/src/main.ts:1514-1515 uses the correct order: stopBackend() first, then stopHarness(). Since the backend is a client of the harness (it receives harnessPort and harnessSecret during bootstrap at line 1103-1105 and connects to it), killing the harness first leaves the backend with a dead upstream connection, potentially causing error storms, reconnect attempts, or unclean shutdown of provider sessions before the backend itself is terminated.
| stopHarness(); | |
| stopBackend(); | |
| stopBackend(); | |
| stopHarness(); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| stopHarness(); | ||
| stopBackend(); |
There was a problem hiding this comment.
🔴 SIGTERM handler also stops harness before backend (same issue as SIGINT)
Same ordering bug as in the SIGINT handler — stopHarness() is called before stopBackend(), inconsistent with the before-quit handler at apps/desktop/src/main.ts:1514-1515 which correctly stops the backend (the harness client) before stopping the harness (the upstream service).
| stopHarness(); | |
| stopBackend(); | |
| stopBackend(); | |
| stopHarness(); |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
erlef/setup-beam,mix test, dep cache) — gates 88 tests / 2,249 lines:one_for_one→:rest_for_one— Storage crash now cascades correctly to SnapshotServersession/closedinterminate/2for all 5 providers — fixes stale "running" snapshots after explicit stop/api/*HTTP routes withharness_secret— parity with WebSocket authreplay_from_sql_or_emptyreturns{:gap,...}on SQL failure, activates dead recovery handlerTest plan
mix test— 88 tests, 0 failures (verified locally)bun typecheck/bun run test/bun fmt/bun lint— all pass/api/snapshotreturns 401 without secret, 200 with?secret=<harness_secret>:closedstatus in snapshot (not stale:running)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores