host-service: integration test suite + 3 v2 bug fixes#3915
Merged
Conversation
…10 routers Boots `createApp` against a `bun:sqlite`-backed drizzle db with fake providers and drives a tRPC client over `app.fetch` (no port, no network). Covers health, host, workspace, workspace-creation, workspace-cleanup, project, filesystem, cloud, notifications, github, ports — including 401 paths, CORS allowlist, and real-git fixtures for branch + status flows. `CreateAppOptions` now accepts optional `db` and `api` overrides so the harness can inject the bun:sqlite db (better-sqlite3 isn't loadable under Bun; bundled-Node still uses better-sqlite3 in prod) and a fake cloud api. `CreateAppResult.dispose()` lets tests cleanly stop the PR runtime, event bus, and owned db.
…kspace-creation misc Adds 18 more tests covering: - git router: listBranches, getStatus (clean/dirty), getBaseBranch + setBaseBranch round-trip, renameBranch - pullRequests router: getByWorkspaces hydration with linked PR rows, refreshByWorkspaces no-op - workspace-creation misc: getContext, getProgress (in-memory store), generateBranchName empty/unknown-project guards
…ion tests +17 tests across three more files: - terminal: listSessions empty, killSession NOT_FOUND for unknown workspace + terminal, auth required - project.setup: clone with no/unparseable repoCloneUrl, repoint without allowRelocate, project.create empty/template NOT_IMPLEMENTED, idempotent remove for missing project - git history: listCommits empty/with-commits, getCommitFiles, getDiff staged content, getBranchSyncStatus no-remote + detached HEAD
… tests +9 tests across two more files: - workspace.create + workspace.delete: success path persists worktree+row, cloud failure rolls back the on-disk worktree, main-by-path rejection, delete removes worktree + row + calls cloud, auth required - ws auth: /events 401 without token, 401 with wrong token, accepted with valid token query param, /terminal/* 401 without auth
+5 tests covering: PROJECT_NOT_SETUP when project missing, no-managed-worktree guard, NOT_FOUND for unregistered explicit worktreePath, successful adopt of an explicit worktree path with cloud + local row creation, and recordBaseBranch persisting `branch.<name>.base` in git config.
Adds optional \`github\` factory override to \`CreateAppOptions\` so tests can inject a fake Octokit-shaped object instead of hitting api.github.com. +9 tests across two files: - github router (mocked): getPRStatus head filter, getPR pull_number, listPRs pagination forwarding, getRepo - workspaceCreation github procedures: searchGitHubIssues direct \`#N\` lookup, free-text search fallback, repoMismatch for cross-repo URLs; searchPullRequests direct \`#N\` lookup + PR filter on search results
…project cloud fallback +6 tests: - github (mocked Octokit): listDeployments + listDeploymentStatuses param forwarding, getUser, mergePR with mergeMethod - workspaceCleanup: deleteBranch=true removes the branch after worktree teardown - project: findByPath cloud-fallback path when no local project + parseable GitHub remote
Adds optional \`chatRuntime\` (ChatRuntimeManager) and \`chatService\` (ChatService) overrides to \`CreateAppOptions\` so tests can stub the mastra- backed runtimes that previously blocked coverage of these routers. +16 tests: - chat router: getDisplayState / listMessages / getSnapshot / sendMessage (including fire-and-forget cloud lastActiveAt update) / endSession / respondToApproval (zod enum guard) / getSlashCommands / resolveSlashCommand / auth required - auth router: getAnthropicStatus, startAnthropicOAuth, completeOAuth, setAnthropicApiKey (zod min(1) guard + happy path), OpenAI status + setOpenAIApiKey, both disconnect endpoints, auth required
…n tests +6 tests on the validation surface of the two heaviest workspaceCreation procedures (full happy paths still need extensive cloud + git mocks): - create: empty branchName / workspaceName rejected, PROJECT_NOT_SETUP for unknown project - checkout: exactly-one-of branch|pr refine guard, negative PR number rejected at zod, PROJECT_NOT_SETUP for unknown project
Adds 4 deliberately-aggressive bug-hunt files (38 tests, 1 todo) that probe defenses rather than pin behavior: - bug-hunt.test.ts: filesystem sandbox (writeFile/readFile/deletePath/ movePath/createDirectory '..' traversal), git-flag injection through setBaseBranch/renameBranch, idempotency, auth-header parsing edge cases, SQL/identifier injection - bug-hunt-2.test.ts: symlink-based sandbox escape (rejected), copyPath/createDirectory traversal, partial-failure consistency, manual worktree deletion graceful handling, detached HEAD edges - bug-hunt-3.test.ts: branch-name path traversal (incidentally blocked by git's refname rules), workspace.delete legacy dirty-skip behavior, setBaseBranch race todo - bug-hunt-4.test.ts: cross-project worktree adoption confusion, double-call cloud propagation Found one real bug (marked test.todo so the suite stays green and the fix removes the .todo): BUG: parallel git.setBaseBranch calls race on .git/config.lock and one returns a 500 with "error: could not lock config file .git/config: File exists". A renderer double-click during a slow request hits this. Fix: catch lock-contention errors and retry, or serialize per-workspace config writes. Defenses verified to hold: - workspace-fs realpath checks block all traversal + symlink escapes - git's refname rules incidentally block `../` in branch names (used by workspace.create which doesn't validate before path.join) - drizzle parameterizes against SQL injection - timing-safe PSK comparison, multi-token query rejected - cross-project worktree adoption properly rejected by git scope - workspaceCleanup.destroy idempotent on missing rows
…kspaceCreation
Two new v2 bugs (test.todo) on top of the .git/config.lock race already
documented:
1. workspaceCreation.create leaks an 'active' progress entry when the
project isn't set up locally. setProgress(pendingId, 'ensuring_repo')
runs BEFORE requireLocalProject throws PROJECT_NOT_SETUP, and the
throw path doesn't call clearProgress. Renderer shows a stuck spinner
for up to 5 minutes (until sweepStaleProgress catches it).
2. Same pattern with whitespace-only branchName: setProgress reaches
'creating_worktree' before the .trim() check throws BAD_REQUEST.
clearProgress is only called inside the worktree-add try/catch
downstream; everything in between leaks.
Same leak shape exists in workspaceCreation.checkout for any throw
between setProgress(ensuring_repo) at line 21 and requireLocalProject
at line 23 — covered by the same fix surface.
Suggested fix: wrap the procedure body in `try { ... } finally
{ clearProgress(pendingId) }` and remove the scattered clearProgress
calls. Or make setProgress/clearProgress a tRPC middleware bound to
the procedure lifecycle.
…te + checkout Wraps both procedures in a try/finally so every throw path clears the pending-id progress entry. Previously, errors thrown between \`setProgress(...)\` and the scattered \`clearProgress(...)\` call sites (project-not-setup, whitespace branch name, listBranchNames / safeResolveWorktreePath / mkdirSync failures, etc.) left a stale "active" step that the renderer kept polling for up to 5 minutes — a stuck spinner on the Pending Workspace page until \`sweepStaleProgress\` caught it. The scattered \`clearProgress\` calls inside the body are now redundant but harmless (the call is idempotent); leaving them for diff minimalism. Flips two .todo regressions in bug-hunt-v2.test.ts to real \`test()\` asserting \`getProgress\` returns null after each early-throw path.
…tion Concurrent \`git config\` writes to the same repo race on .git/config.lock and one fails with "error: could not lock config file .git/config: File exists". Reproduced via two parallel \`git.setBaseBranch\` calls (renderer double-click on the base-branch picker during a slow request). Adds \`gitConfigWrite\` helper that retries on lock contention with exponential backoff (30/60/120/240ms, 4 attempts max). Routes the three in-tree config writers through it: - git.setBaseBranch - workspaceCreation.create base-branch record - workspaceCreation.adopt recordBaseBranch Flips the previously-todo bug-hunt-3 regression test to a real \`test()\` asserting two parallel setBaseBranch calls converge without a 500.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds DI-capable app creation with an async dispose lifecycle, a retrying git-config write utility, consolidated cleanup and safer checkout/create flows, and a comprehensive test harness plus 20+ new integration test suites exercising host-service behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Tester
participant TestHost as TestHost
participant App as HostApp
participant DB as DrizzleDB
participant Git as SimpleGit
participant API as CloudApi
participant GH as Octokit
participant Chat as ChatRuntime
Tester->>TestHost: createTestHost(options with overrides)
TestHost->>App: createApp(injected db?, api?, github?, chatRuntime?, chatService?)
App->>DB: init/connect (if not injected)
App->>Git: create/configure git client
App->>API: attach ApiClient (fake or injected)
App->>GH: use Octokit factory (if injected)
App->>Chat: attach chat runtime/service (if injected)
TestHost-->>Tester: returns TestHost + trpc clients + dispose()
Tester->>App: trpc request (e.g., workspace.create, git.setBaseBranch)
App->>Git: perform git ops (may call gitConfigWrite with retries)
App->>DB: persist/read workspaces, progress, etc.
App->>API: call cloud procedures/overrides
App->>GH: call Octokit mock (if used)
App->>Chat: update session (async)
Tester->>TestHost: dispose()
TestHost->>App: app.dispose()
App->>Chat: stop runtimes
App->>DB: close DB if internally created
App-->>TestHost: disposed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~80 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryAdds a comprehensive integration-test harness for Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/robustness suggestions with no correctness impact on the changed paths. The three bug fixes are correct and each has a regression test that would fail on the prior code. The test harness is well-structured (fail-loud fake API, real git repos, isolated sqlite). All P2 comments (dispose error ordering, internal Drizzle $client, fetchApp init forwarding, gitConfigWrite arg guard) are quality suggestions that do not affect current runtime behaviour. packages/host-service/src/app.ts — dispose robustness and $client internal API usage worth a follow-up. The pre-existing pull-requests.test.ts:71 failure (#3884) should be tracked separately.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/app.ts | Adds optional dependency injection overrides (db, api, github, chatRuntime, chatService) and a dispose() method; minor robustness gaps in dispose error handling and use of internal Drizzle $client API. |
| packages/host-service/src/trpc/router/git/utils/config-write.ts | New gitConfigWrite helper with bounded exponential-backoff retry on .git/config.lock contention; logic is correct for all current call sites. |
| packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts | Wraps body in try/finally to guarantee clearProgress on every exit path, fixing Bug 2 & 3 progress leaks; gitConfigWrite replaces raw git config call. |
| packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts | Large refactor: PR-path and branch-path unified under a single try/finally progress cleanup seam, fixing the same progress-leak shape as create.ts. |
| packages/host-service/test/helpers/createTestHost.ts | Bootstraps an isolated bun:sqlite-backed app with fake providers and an in-process tRPC client; fetchApp incorrectly passes RequestInit as Hono env when input is already a Request. |
| packages/host-service/test/helpers/fakes.ts | Provides fake auth, credential, model-resolver providers and a Proxy-based ApiClient that throws on unmocked paths — good fail-loud test design. |
| packages/host-service/test/helpers/git-fixture.ts | Creates real on-disk git repos in realpathSync-resolved tmpdir with isolated user config and an initial commit; correct macOS symlink handling. |
| packages/host-service/test/integration/bug-hunt-v2.test.ts | Regression tests for the three fixed bugs (progress leak on PROJECT_NOT_SETUP, whitespace branch, and destroy phase ordering); correctly structured to fail on old code. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["workspaceCreation.create / checkout"] -->|outer try| B["setProgress('ensuring_repo')"]
B --> C{"requireLocalProject"}
C -->|throws PROJECT_NOT_SETUP| F
C -->|ok| D["setProgress('creating_worktree')"]
D --> E{"branchName.trim() empty?"}
E -->|yes — throws BAD_REQUEST| F
E -->|no| G["git worktree add"]
G --> H["cloud API calls"]
H --> I["local DB insert"]
I --> J["clearProgress + return result"]
J --> F
subgraph F["finally block"]
direction LR
F1["clearProgress(pendingId)"]
end
subgraph LockRetry["gitConfigWrite lock retry"]
R1["git config write"] -->|config.lock error| R2["sleep 30/60/120 ms"]
R2 --> R1
R1 -->|success| R3["return"]
R1 -->|other error OR retries exhausted| R4["throw"]
end
G -.->|setBaseBranch / recordBaseBranch| LockRetry
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
packages/host-service/src/app.ts:191-201
**`dispose` may silently skip db close on runtime stop errors**
`pullRequestRuntime.stop()` and `eventBus.close()` are called without try-catch. If either throws, the `if (ownsDb)` branch that closes the SQLite connection is never reached, leaving the file handle open. In tests the `createTestHost` wrapper still closes `sqlite` directly, but in production this means a crash in `.stop()` can leave the db handle dangling.
```suggestion
const dispose = async (): Promise<void> => {
try { pullRequestRuntime.stop(); } catch {}
try { eventBus.close(); } catch {}
if (ownsDb) {
```
### Issue 2 of 4
packages/host-service/src/app.ts:196-198
**Relying on Drizzle's undocumented `$client` internal**
`(db as unknown as { $client?: { close: () => void } }).$client?.close()` reaches into an undocumented internal of drizzle-orm. There is no public contract for `$client`, so a minor Drizzle upgrade could silently skip the close. Consider wrapping `db` in a thin interface that exposes a `close()` method at construction time, or exporting a typed `HostDb` that includes the close method.
### Issue 3 of 4
packages/host-service/src/trpc/router/git/utils/config-write.ts:18-22
**No guard that `args` actually target `git config`**
The function is documented and named as a `git config` write helper but accepts arbitrary `args`. A caller passing `["fetch", "--all"]` would silently get lock-contention retry behaviour for an unrelated command. Adding an assertion or renaming to something generic (e.g. `gitRawWithLockRetry`) would prevent misuse. Current call sites are all correct.
### Issue 4 of 4
packages/host-service/test/helpers/createTestHost.ts:97-102
**`init` forwarded as Hono `env` when `input` is already a `Request`**
When `input` is already a `Request` object, `fetchApp` still passes `init` (a `RequestInit`) as the second argument to `result.app.fetch`. Hono's second argument is `env` (Cloudflare-style environment bindings), not request options. Callers that supply a pre-built `Request` plus `init` headers/body will see no effect — the init is silently ignored.
```suggestion
const fetchApp = async (
input: Request | string,
init?: RequestInit,
): Promise<Response> => {
const req =
typeof input === "string" ? new Request(input, init) : input;
return result.app.fetch(req);
};
```
Reviews (1): Last reviewed commit: "fix(host-service): retry git config writ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
191
to
+201
|
|
||
| return { app, injectWebSocket, api }; | ||
| const ownsDb = options.db === undefined; | ||
| const dispose = async (): Promise<void> => { | ||
| pullRequestRuntime.stop(); | ||
| eventBus.close(); | ||
| if (ownsDb) { | ||
| try { | ||
| (db as unknown as { $client?: { close: () => void } }).$client?.close(); | ||
| } catch { | ||
| // best-effort close; tests should not fail on teardown | ||
| } |
There was a problem hiding this comment.
dispose may silently skip db close on runtime stop errors
pullRequestRuntime.stop() and eventBus.close() are called without try-catch. If either throws, the if (ownsDb) branch that closes the SQLite connection is never reached, leaving the file handle open. In tests the createTestHost wrapper still closes sqlite directly, but in production this means a crash in .stop() can leave the db handle dangling.
Suggested change
| return { app, injectWebSocket, api }; | |
| const ownsDb = options.db === undefined; | |
| const dispose = async (): Promise<void> => { | |
| pullRequestRuntime.stop(); | |
| eventBus.close(); | |
| if (ownsDb) { | |
| try { | |
| (db as unknown as { $client?: { close: () => void } }).$client?.close(); | |
| } catch { | |
| // best-effort close; tests should not fail on teardown | |
| } | |
| const dispose = async (): Promise<void> => { | |
| try { pullRequestRuntime.stop(); } catch {} | |
| try { eventBus.close(); } catch {} | |
| if (ownsDb) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/app.ts
Line: 191-201
Comment:
**`dispose` may silently skip db close on runtime stop errors**
`pullRequestRuntime.stop()` and `eventBus.close()` are called without try-catch. If either throws, the `if (ownsDb)` branch that closes the SQLite connection is never reached, leaving the file handle open. In tests the `createTestHost` wrapper still closes `sqlite` directly, but in production this means a crash in `.stop()` can leave the db handle dangling.
```suggestion
const dispose = async (): Promise<void> => {
try { pullRequestRuntime.stop(); } catch {}
try { eventBus.close(); } catch {}
if (ownsDb) {
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+196
to
+198
| if (ownsDb) { | ||
| try { | ||
| (db as unknown as { $client?: { close: () => void } }).$client?.close(); |
There was a problem hiding this comment.
$client internal
(db as unknown as { $client?: { close: () => void } }).$client?.close() reaches into an undocumented internal of drizzle-orm. There is no public contract for $client, so a minor Drizzle upgrade could silently skip the close. Consider wrapping db in a thin interface that exposes a close() method at construction time, or exporting a typed HostDb that includes the close method.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/app.ts
Line: 196-198
Comment:
**Relying on Drizzle's undocumented `$client` internal**
`(db as unknown as { $client?: { close: () => void } }).$client?.close()` reaches into an undocumented internal of drizzle-orm. There is no public contract for `$client`, so a minor Drizzle upgrade could silently skip the close. Consider wrapping `db` in a thin interface that exposes a `close()` method at construction time, or exporting a typed `HostDb` that includes the close method.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+18
to
+22
| export async function gitConfigWrite( | ||
| git: SimpleGit, | ||
| args: string[], | ||
| options: { retries?: number; baseDelayMs?: number } = {}, | ||
| ): Promise<string> { |
There was a problem hiding this comment.
args actually target git config
The function is documented and named as a git config write helper but accepts arbitrary args. A caller passing ["fetch", "--all"] would silently get lock-contention retry behaviour for an unrelated command. Adding an assertion or renaming to something generic (e.g. gitRawWithLockRetry) would prevent misuse. Current call sites are all correct.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/git/utils/config-write.ts
Line: 18-22
Comment:
**No guard that `args` actually target `git config`**
The function is documented and named as a `git config` write helper but accepts arbitrary `args`. A caller passing `["fetch", "--all"]` would silently get lock-contention retry behaviour for an unrelated command. Adding an assertion or renaming to something generic (e.g. `gitRawWithLockRetry`) would prevent misuse. Current call sites are all correct.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+97
to
+102
|
|
||
| const fakeApi = createFakeApiClient(options.apiOverrides); | ||
|
|
||
| const createOptions: CreateAppOptions = { | ||
| config: { | ||
| organizationId: |
There was a problem hiding this comment.
init forwarded as Hono env when input is already a Request
When input is already a Request object, fetchApp still passes init (a RequestInit) as the second argument to result.app.fetch. Hono's second argument is env (Cloudflare-style environment bindings), not request options. Callers that supply a pre-built Request plus init headers/body will see no effect — the init is silently ignored.
Suggested change
| const fakeApi = createFakeApiClient(options.apiOverrides); | |
| const createOptions: CreateAppOptions = { | |
| config: { | |
| organizationId: | |
| const fetchApp = async ( | |
| input: Request | string, | |
| init?: RequestInit, | |
| ): Promise<Response> => { | |
| const req = | |
| typeof input === "string" ? new Request(input, init) : input; | |
| return result.app.fetch(req); | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/test/helpers/createTestHost.ts
Line: 97-102
Comment:
**`init` forwarded as Hono `env` when `input` is already a `Request`**
When `input` is already a `Request` object, `fetchApp` still passes `init` (a `RequestInit`) as the second argument to `result.app.fetch`. Hono's second argument is `env` (Cloudflare-style environment bindings), not request options. Callers that supply a pre-built `Request` plus `init` headers/body will see no effect — the init is silently ignored.
```suggestion
const fetchApp = async (
input: Request | string,
init?: RequestInit,
): Promise<Response> => {
const req =
typeof input === "string" ? new Request(input, init) : input;
return result.app.fetch(req);
};
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
packages/host-service/test/integration/chat.test.ts (1)
140-142: ⚡ Quick winReplace
as anywith a non-anycast for the invalid-enum test case.On Line 141, this violates the TS typing guideline and can be rewritten without
any.As per coding guidelines `**/*.{ts,tsx}`: Avoid using `any` type; prefer explicit type safety in TypeScript.💡 Suggested fix
- // biome-ignore lint/suspicious/noExplicitAny: testing zod rejection - payload: { decision: "garbage" as any }, + payload: { + decision: "garbage" as unknown as "approve" | "reject", + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/test/integration/chat.test.ts` around lines 140 - 142, The test uses an unsafe "as any" cast for the invalid-enum payload; replace it with a proper non-any cast by asserting the string as unknown then as the enum/type used by the code (e.g., replace `payload: { decision: "garbage" as any }` with `payload: { decision: "garbage" as unknown as Decision }`), referencing the test's payload object and its decision field so TypeScript rules are satisfied while still producing an invalid enum value for the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/host-service/src/trpc/router/git/utils/config-write.ts`:
- Around line 23-39: The retry loop can leave lastErr undefined if
options.retries is 0 and the backoff count is confusing; normalize retries to at
least 1 (e.g., retries = Math.max(1, options.retries ?? 4)) and initialize
lastErr to a proper Error (or meaningful fallback Error) so throw lastErr is
never undefined; keep the existing loop using attempt < retries and exponential
backoff but ensure retries is clamped and lastErr is set before the loop ends so
git.raw failures always result in a proper Error being thrown.
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts`:
- Around line 169-214: The post-worktree local setup (calls to
enablePushAutoSetupRemote and gitConfigWrite) can throw after the worktree was
created, leaving an orphaned worktree on disk; wrap those local setup steps so
any error triggers the existing rollbackWorktree() and then rethrows to surface
the failure. Concretely, after the worktree add succeeds, execute
enablePushAutoSetupRemote(...) and the gitConfigWrite(...) call inside a
try/catch that calls await rollbackWorktree() in the catch block before throwing
the caught error (or returning a rejected promise), ensuring rollbackWorktree is
awaited so the on-disk worktree is removed on local setup failure. Ensure you
reference the existing rollbackWorktree function and preserve the current
logging behavior when rollback fails.
In `@packages/host-service/test/helpers/createTestHost.ts`:
- Around line 152-163: The current dispose function awaits result.dispose()
first which, if it throws, prevents sqlite.close() and rmSync(dataDir, ...) from
running; change the control flow so sqlite.close() and rmSync(...) always run
regardless of result.dispose() outcome (e.g., call await result.dispose() inside
a try/catch and move sqlite.close() and rmSync(...) into a finally block or
execute them after catching errors), keeping the best-effort try/catch semantics
around sqlite.close() and rmSync and ensuring any error from result.dispose() is
either rethrown or logged after cleanup; refer to the dispose function,
result.dispose, sqlite.close, rmSync and dataDir to locate the code.
In `@packages/host-service/test/integration/bug-hunt-2.test.ts`:
- Around line 397-415: The test name and comment are misleading because it never
verifies visibility from a second host; after disposing the first host
(a.dispose()) instantiate a new host (e.g., const b = await createTestHost())
pointed at the same fixture/repo (use repo.repoPath or same DB setup used when
inserting via a.db) and assert the row is visible via
b.trpc.project.list.query(); then dispose b. If the harness truly cannot share
persistence, instead update the test name/comment to reflect it only verifies
same-host visibility and remove the implication of restart/second-host behavior.
- Around line 200-214: The current assertion uses a loose condition (errorThrown
|| !worktreeStillThere) which incorrectly passes when an error is thrown but the
on-disk worktree remains; change the assertion to require that an error maps to
a cleaned-up worktree and success maps to a present worktree by replacing the
check with a direct equality between errorThrown and !worktreeStillThere (i.e.,
assert errorThrown === !worktreeStillThere) so that result, errorThrown,
expectedWorktree and worktreeStillThere enforce the correct rollback semantics.
In `@packages/host-service/test/integration/bug-hunt.test.ts`:
- Around line 377-380: The test currently swallows any error by using .catch(()
=> null) on host.trpc.workspace.get.query which can mask route/table failures;
change the test to explicitly assert the NOT_FOUND trpc error instead of
returning null (e.g., await the query and assert the thrown error has data.code
=== 'NOT_FOUND' or equivalent), and if you need to prove the table still exists
add a direct DB read (e.g., call the underlying DB client or model read used in
other tests) to verify the workspace table/schema is present; remove the
.catch(() => null) and replace expect(row).toBeNull() with explicit error
assertion plus an optional direct DB check.
- Around line 298-313: The current assertion uses fulfilled.length <= 2 which is
meaningless for Promise.allSettled and the later uniqueness check allows
duplicate feature/race rows to slip by; replace these with a direct assertion
that there is at most one workspace row for branch "feature/race": query rows
via host.db.select().from(workspaces).where(eq(workspaces.projectId,
projectId)).all(), filter to featureRows by r.branch === "feature/race", and
assert expect(featureRows.length).toBeLessThanOrEqual(1); remove or tighten the
existing fulfilled-length assertion and keep the uniqueness check only if you
still want to guard against duplicate ids, referencing variables a, b,
fulfilled, workspaces, projectId, and featureRows to locate the code.
In `@packages/host-service/test/integration/cloud.test.ts`:
- Around line 35-37: The test currently calls
expect(host.unauthenticatedTrpc.cloud.whoami.query()).rejects.toBeInstanceOf(TRPCClientError)
but does not await or return the assertion, which can cause false positives;
update the test to await the rejection assertion (await
expect(...).rejects.toBeInstanceOf(TRPCClientError)) so the test runner waits
for the promise rejection, referencing
host.unauthenticatedTrpc.cloud.whoami.query() and TRPCClientError to locate the
assertion to change.
In `@packages/host-service/test/integration/filesystem.test.ts`:
- Around line 56-62: The test is asserting a rejected promise but doesn't await
the assertion, so change the assertion to await the rejects matcher: await
expect(host.trpc.filesystem.listDirectory.query({ workspaceId: "no-such-ws",
absolutePath: repo.repoPath })).rejects.toBeInstanceOf(TRPCClientError);; update
the test case around host.trpc.filesystem.listDirectory.query and
TRPCClientError to ensure the promise rejection is properly awaited.
In `@packages/host-service/test/integration/project.test.ts`:
- Around line 52-55: The test "get rejects non-uuid projectId via zod" currently
calls host.trpc.project.get.query({ projectId: "not-a-uuid" }) without awaiting
the rejects assertion, so change the assertion to await
expect(host.trpc.project.get.query({ projectId: "not-a-uuid"
})).rejects.toBeInstanceOf(TRPCClientError) to ensure the promise rejection is
properly awaited and the test reliably fails if the query does not reject as
expected.
In `@packages/host-service/test/integration/smoke.test.ts`:
- Around line 26-29: The test "protected procedure rejects requests without
bearer token" is missing an await on the rejection assertion, which can cause
false positives; update the assertion that calls
host.unauthenticatedTrpc.host.info.query() to await the
expect(...).rejects.toBeInstanceOf(TRPCClientError) chain so the test runner
waits for the rejection assertion to resolve.
In `@packages/host-service/test/integration/workspace.test.ts`:
- Around line 47-50: The test is missing await when asserting promise
rejections, so update the failing assertions that call
host.trpc.workspace.get.query(...) to await the expect(...).rejects chain (e.g.,
change expect(host.trpc.workspace.get.query({ id: "no-such-id"
})).rejects.toBeInstanceOf(TRPCClientError) to await
expect(...).rejects.toBeInstanceOf(...)); do the same for the second occurrence
around lines 76–79 to ensure the rejection assertions for TRPCClientError are
properly awaited.
In `@packages/host-service/test/integration/ws-auth.test.ts`:
- Around line 25-32: The test "test(\"/events accepts a valid token via query
param (no upgrade header → 426/200/etc, just not 401)\")" currently only asserts
expect(res.status).not.toBe(401), which is too weak; change it to assert a
concrete expected status (or a small allowed set) based on what the test harness
actually returns—for example replace the negative check with a positive
assertion like expect([200,426]).toContain(res.status) or
expect(res.status).toBe(200) so the test fails on unrelated 404/500 errors;
update the assertion around the res object returned by
host.fetch(`.../events?token=${encodeURIComponent(host.psk)}`) accordingly.
---
Nitpick comments:
In `@packages/host-service/test/integration/chat.test.ts`:
- Around line 140-142: The test uses an unsafe "as any" cast for the
invalid-enum payload; replace it with a proper non-any cast by asserting the
string as unknown then as the enum/type used by the code (e.g., replace
`payload: { decision: "garbage" as any }` with `payload: { decision: "garbage"
as unknown as Decision }`), referencing the test's payload object and its
decision field so TypeScript rules are satisfied while still producing an
invalid enum value for the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84ae24a4-963f-469a-8699-4aced0385178
📒 Files selected for processing (38)
packages/host-service/src/app.tspackages/host-service/src/trpc/router/git/git.tspackages/host-service/src/trpc/router/git/utils/config-write.tspackages/host-service/src/trpc/router/workspace-creation/procedures/adopt.tspackages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tspackages/host-service/src/trpc/router/workspace-creation/procedures/create.tspackages/host-service/test/helpers/createTestHost.tspackages/host-service/test/helpers/fakes.tspackages/host-service/test/helpers/git-fixture.tspackages/host-service/test/integration/auth.test.tspackages/host-service/test/integration/bug-hunt-2.test.tspackages/host-service/test/integration/bug-hunt-3.test.tspackages/host-service/test/integration/bug-hunt-4.test.tspackages/host-service/test/integration/bug-hunt-v2.test.tspackages/host-service/test/integration/bug-hunt.test.tspackages/host-service/test/integration/chat.test.tspackages/host-service/test/integration/cloud.test.tspackages/host-service/test/integration/filesystem.test.tspackages/host-service/test/integration/git-history.test.tspackages/host-service/test/integration/git.test.tspackages/host-service/test/integration/github-mocked.test.tspackages/host-service/test/integration/github.test.tspackages/host-service/test/integration/notifications.test.tspackages/host-service/test/integration/ports.test.tspackages/host-service/test/integration/project-setup.test.tspackages/host-service/test/integration/project.test.tspackages/host-service/test/integration/pull-requests.test.tspackages/host-service/test/integration/smoke.test.tspackages/host-service/test/integration/terminal.test.tspackages/host-service/test/integration/workspace-cleanup.test.tspackages/host-service/test/integration/workspace-create-delete.test.tspackages/host-service/test/integration/workspace-creation-adopt.test.tspackages/host-service/test/integration/workspace-creation-github.test.tspackages/host-service/test/integration/workspace-creation-misc.test.tspackages/host-service/test/integration/workspace-creation-validation.test.tspackages/host-service/test/integration/workspace-creation.test.tspackages/host-service/test/integration/workspace.test.tspackages/host-service/test/integration/ws-auth.test.ts
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…elpers
Pulled the four-step beforeEach (host + repo + projects.insert + workspaces.insert)
that was duplicated across 12 test files into three composable helpers:
- test/helpers/seed.ts — \`seedProject\`, \`seedWorkspace\`, \`seedTerminalSession\`,
\`seedPullRequest\`. Tests express *what* they need rather than the drizzle
insert shape, so future schema changes only touch this file.
- test/helpers/cloud-fakes.ts — pre-canned cloud-API factories
(\`cloudOk.hostEnsure\`, \`cloudOk.workspaceCreate\`, \`cloudOk.workspaceDelete\`,
\`cloudOk.workspaceGetFromHost\`, etc.) plus bundled \`cloudFlows.workspaceCreateOk\`
/ \`workspaceDeleteOk\` for whole-flow setups. Replaces 5+ inline copies of
\`(input: unknown) => ({ id: randomUUID(), ...echo })\`.
- test/helpers/scenarios.ts — high-level lifecycle bundles:
\`createBasicScenario\` (host + repo + project + main workspace),
\`createFeatureWorktreeScenario\` (basic + real \`git worktree add\` + feature
workspace row), \`createProjectScenario\` (project only). Each owns its
\`dispose()\` so afterEach is one-line.
Migrated 13 test files. Bug-hunt suites kept as-is — single-purpose probes
read better with explicit setup. Net diff: -205 lines across migrated tests
(helpers add +412 net for the suite, paid back as the suite grows).
182 integration tests still pass. Typecheck + lint clean.
… assertions
7 valid findings from coderabbit on the latest commit (greptile re-flagged
4 already-fixed issues; ignoring those).
- 8 test files: \`afterEach\` now disposes via \`scenario?.dispose()\` (or
\`host?.dispose()\`) so a thrown \`beforeEach\` doesn't mask the original
error with a teardown crash. Optional chain only — types stay
non-optional so test bodies don't need \`!\` everywhere.
- bug-hunt-v2: renamed test from "TEARDOWN_FAILED" to "destroy rejects a
main workspace BEFORE running teardown or cloud-delete" — the body
only verifies the phase-0 main-workspace guard, not real
TEARDOWN_FAILED mapping (which needs a PTY-enabled harness).
- smoke CORS: \`not.toBe("http://evil.example")\` would also pass for a
misconfigured wildcard \`*\`. Replaced with \`toBeNull()\` — Hono's CORS
middleware omits the header entirely for non-allowlisted origins.
- workspaceCreation.adopt PROJECT_NOT_SETUP test was passing on "any
throw"; now asserts \`{ data: { code: "PRECONDITION_FAILED" } }\`
(the cause shape that \`projectNotSetupError\` produces).
- workspace.get / gitStatus NOT_FOUND tests now assert
\`{ data: { code: "NOT_FOUND" } }\` instead of just \`TRPCClientError\`
instance.
Skipped (greptile re-flags of already-fixed issues):
- app.ts:212 dispose isolation (fixed in 5812eaf)
- app.ts:209 \`$client\` undocumented (acknowledged tradeoff)
- config-write.ts:22 args guard (rename suggestion, low value)
- createTestHost.ts:102 fetchApp env (fixed in 5812eaf)
- createTestHost.ts:173 cleanup finally (fixed in 5812eaf)
- workspace-creation-misc.ts:78 progress cleanup (fixed in 705f924)
Pass over my own additions following the deslop guide: - app.ts: collapsed 5 individual JSDoc blocks for the test-harness override fields into one umbrella comment + concise per-field types. Same for createTestHost.ts. - scenarios.ts: removed unused \`withBasicScenario\` helper (no callers); trimmed restate-the-obvious field doc on \`hostOptions\`. - cloud-fakes.ts: removed unused \`organization\`, \`userMe\`, \`chatUpdateSession\` factories; tightened the top JSDoc + workspaceCreate comment. - fakes.ts: removed unused \`ProcedureKind\` type export. - bug-hunt-v2: dropped a "Today: progress is non-null …" comment that described pre-fix behavior the test no longer demonstrates; trimmed meta-explanations of test design choices to focus on the non-obvious assertion. - bug-hunt: pruned setBaseBranch / renameBranch test bodies to one intent comment each (down from 3 multi-line prose blocks). - workspace-cleanup: tightened a "switching hosts" inline. No behavior change. 398 tests still pass; typecheck + lint clean.
Kitenite
added a commit
that referenced
this pull request
May 1, 2026
Merging origin/main brought in the new integration test suite (PR #3915) which had three stale assertions against my saga refactor: - "create() with empty mode returns NOT_IMPLEMENTED": dropped — empty mode is now implemented via the saga, the test guarded the old throw. - "create() with template mode returns NOT_IMPLEMENTED": same, dropped. - "remove() is idempotent when project doesn't exist" + "project.remove is idempotent across two calls": both updated. project.remove now calls cloud v2Project.delete first (kill point), so apiOverrides needs to mock that mutation. The new project.remove return shape is { success: true, repoPath: string | null } not { success: true } — expectations updated to match. Lint auto-fix from CI also wrapped the initEmptyRepo call onto multiple lines; included.
Kitenite
added a commit
that referenced
this pull request
May 1, 2026
#3913) * docs(desktop): v2 create-project audit + v1-untouchable constraint Audit of the v2 create-project flow: surfaces the two parallel backends (electronTrpc.projects.* vs host-service client.project.*), implicit states across loading/error/disk/DB truth, the clone-from-git shortfalls vs empty-repo, and a "cloud is reality" transactional model for any future implementation. Adds a hard constraint at the top: the /new-project page and its three tabs are reached by v1 surfaces (StartView, WorkspaceSidebarFooter), so they are off-limits for any rewire even in the name of v2 consolidation. The earlier attempt to migrate those tabs (8 commits, host-service saga, three rewired tabs) was reset because it silently changed v1's create-project backend. The path forward documented in the doc: build v2-only alternatives (new route, new procedures) that v2 callers migrate to; leave the v1-reached path completely alone until the v1 sunset PR. * docs(desktop): lock in post-reset decisions + revised impl sequence After the reset, re-walked every decision under the "never touch v1" rule. Strategy is now: extend NewProjectModal as the canonical v2 create surface (no new route), keep empty/template as coming-soon stubs for now, ship cloud prereqs + host-service saga + delete-saga + DeleteProjectSection fixup as four small v2-only commits. The v1-reached surfaces (/new-project page, electronTrpc.projects.*, useOpenProject family) stay completely untouched. * feat(api): v2Project.delete via JWT, create accepts client-supplied id Cloud-side prereqs for the v2 host-service create-project saga and the dormant Delete-project UI. All v2-only changes; no v1 surface calls these procedures. - v2Project.delete switches from protectedProcedure to jwtProcedure. Existing form was unreachable from apiTrpcClient (JWT-only) so DeleteProjectSection was a dormant UI. Idempotent on missing project (or wrong org) so the cloud-first delete saga can retry post-rollback without spurious errors. - v2Project.create accepts an optional client-supplied uuid so the host service can mint a project id locally and persist downstream rows that reference it before this commit-point insert. PK collisions surface as TRPCError CONFLICT 409 (cause-chain walk finds the constraint name on Drizzle's nested pg error). v4 UUID collision is essentially impossible but the CONFLICT mapping is symmetric with the existing slug-retry pattern. DeleteProjectSection caller fixup ships in a follow-up commit. * feat(host-service): cloud-as-reality create saga + 4 modes Single host-service-only commit with all four create modes routed through one persistFromResolved saga: 1. local DB project row (client-supplied UUID) 2. cloud v2Project.create (FK-required before workspace) 3. cloud v2Workspace.create + local workspace (strict) On any failure the saga unwinds in reverse — including a cloud v2Project.delete to roll back the cloud commit if the workspace step throws. The saga is the commit unit; the user either sees the project fully exist everywhere, or doesn't see it at all. - ensureMainWorkspaceStrict: new strict variant for the saga. Existing log-and-continue ensureMainWorkspace stays for project.setup and the startup sweep. - initEmptyRepo: atomic mkdir + git init --initial-branch=main + initial commit. Surfaces PRECONDITION_FAILED when the local git user is unconfigured. - cloneTemplateInto: shallow-clones, strips .git, re-inits. - utils/templates.ts: authoritative templateId -> URL mapping. Single real template (nextjs-chatbot) for now. - createFromClone / createFromImportLocal inverted to local-first saga; createFromEmpty + createFromTemplate added. - importLocal sets cleanupRepoPathOnFailure=false (user pointed us at their folder; never rm). All other modes set true (we own the freshly-created dir). No v1 surface calls these procedures; safe to ship. * feat(host-service): cloud-first project.remove saga, never auto-rm Reworks project.remove to match the v2 delete-workspace saga's "cloud is reality" pattern: - Cloud v2Project.delete first (kill point). Cloud cascade handles v2_workspaces; the per-workspace deleteMainForHost loop is gone. - Local DB cleanup is best-effort. If cloud-delete fails the saga aborts cleanly with the cloud's error and local stays intact for retry. - The on-disk repo dir is NEVER auto-rm'd. The user's working tree must be removed by an explicit user action, not as a side-effect. Returns repoPath in the result so a future UI can offer an explicit "delete files too" follow-up. No v1 callers; safe to ship. * fix(desktop): route DeleteProjectSection through host-service saga The Delete button in V2ProjectSettings was dormant — apiTrpcClient sends only a JWT, but the cloud v2Project.delete was protectedProcedure (session-only) so the call always 401'd. Now that delete is jwtProcedure and the host-service has a cloud-first project.remove saga, route the UI through the saga instead of calling cloud directly. Result: delete now removes cloud project + cloud workspaces (cascade) + local DB rows + local worktrees, leaves the on-disk repo dir untouched. Single canonical v2 delete path. Pure v2 surface (settings/v2-project/...); no v1 contact. * fix(api,host-service,desktop): slug conflict retry + sanitize toast Manual QA hit "Failed query: insert into v2_projects ..." raw SQL in the create-project toast when the slug already existed. Three causes: 1. Cloud v2Project.create only mapped PK conflicts to CONFLICT; slug conflicts passed through as raw pg errors. Now the same cause-chain walk also detects v2_projects_org_slug_unique and throws CONFLICT with the stable message "Project slug already exists". 2. Host-service createCloudProjectWithSlugRetry's isSlugConflict() tested err.message for the constraint name, which was always false because the message is Drizzle's "Failed query: ..." envelope (the constraint name is on the underlying pg cause). Updated to match the new stable cloud-side error message so slug retry actually fires — user sees the project created at slug-2/-3/etc, no toast. 3. NewProjectModal's toast dumped the raw "Failed query: ..." string when the saga gave up. Now hides that envelope behind a friendly message and logs the full error to console for devs. Found during manual QA after a previous test left "hello-world" in ORG_B; the saga should have transparently used "hello-world-2". * fix(host-service,docs): drop write-only visibility, friendly detached-HEAD error Addresses PR review: 1. visibility was schema-validated for empty/template modes but never read — handlers dropped it on the floor. Per audit decision 4 the project lives local-only with no GitHub remote until first push, but no first-push integration exists today, so visibility is purely aspirational. Dropped from the schema until there's a real persistence target. UI tabs are still stubs (decision 4 deferred); when someone implements them and the first-push flow at the same time, visibility comes back with a real consumer. 2. ensureMainWorkspaceStrict threw a raw Error on no-branch which surfaced to the user as INTERNAL_SERVER_ERROR after rollback. Now throws TRPCError PRECONDITION_FAILED with actionable instructions ("Repository is in detached-HEAD state. Check out a branch..."). Detached-HEAD imports/clones now get a useful saga-failure toast. 3. Audit doc body still described the original "rewire shared /new-project + delete electronTrpc.projects.* + repoint v2 dropdown" consolidation plan as the path forward — but all of that touches v1-reached code (forbidden after the 2026-04-30 incident). Replaced the deprecated sections with a deprecation banner pointing at the Locked-in decisions section above. Also removed the duplicate "Decisions reaffirmed" table. * test(host-service): align integration tests with new saga semantics Merging origin/main brought in the new integration test suite (PR #3915) which had three stale assertions against my saga refactor: - "create() with empty mode returns NOT_IMPLEMENTED": dropped — empty mode is now implemented via the saga, the test guarded the old throw. - "create() with template mode returns NOT_IMPLEMENTED": same, dropped. - "remove() is idempotent when project doesn't exist" + "project.remove is idempotent across two calls": both updated. project.remove now calls cloud v2Project.delete first (kill point), so apiOverrides needs to mock that mutation. The new project.remove return shape is { success: true, repoPath: string | null } not { success: true } — expectations updated to match. Lint auto-fix from CI also wrapped the initEmptyRepo call onto multiple lines; included. * refactor(host-service): deslop project saga utils Code/comment cleanup pass on the create-project saga work: - Extract `claimEmptyTargetDir`, `gitInitMainBranch`, `asInitialCommitTrpcError` in resolve-repo.ts so initEmptyRepo and cloneTemplateInto stop duplicating ~25 lines of mkdir/EEXIST, git-init-with-fallback, and "empty ident" → PRECONDITION_FAILED handling. - Hoist SLUG_CONFLICT_MESSAGE to a const in handlers.ts so the cross- package coordination with cloud v2Project.create is one place instead of an opaque string in a one-line predicate. - Tighten JSDocs that restated the function name or referenced callers ("e.g. project.setup", "deferred per audit decision 4"). Keep the comments that capture non-obvious WHY: shallow-clone rationale, atomic-claim TOCTOU rationale, Drizzle envelope walk. No behavior change. 409/409 host-service tests still pass.
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
1 participant
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
@superset/host-serviceand 200+ tests covering every v2 router via real-DB + real-git fixtures + injected fakes for cloud API / Octokit / mastra.Bugs fixed
1.
git.setBaseBranchconfig-lock race (fix: retry git config writes on .git/config.lock contention). Two concurrentsetBaseBranchcalls on the same repo race on.git/config.lockand one returns 500error: could not lock config file .git/config: File exists. Reproduced via renderer double-click on the base-branch picker during a slow request. Fixed with agitConfigWritehelper that retries on lock contention with exponential backoff (30/60/120/240ms). Same helper applied to the other two in-tree config writers (workspaceCreation.createbase-branch record,workspaceCreation.adoptrecordBaseBranch).2.
workspaceCreation.createprogress leak onPROJECT_NOT_SETUP(fix: plug progress-store leak).setProgress(pendingId, 'ensuring_repo')ran beforerequireLocalProjectthrew; the throw path didn't callclearProgress. Renderer kept polling a stale "active" step for up to 5 minutes (untilsweepStaleProgresscaught it). Fixed by wrapping the procedure body intry { … } finally { clearProgress(pendingId) }— same pattern applied toworkspaceCreation.checkoutwhich had the same shape.3.
workspaceCreation.createprogress leak on whitespace branch. Same shape, deeper in the procedure:setProgress(creating_worktree)ran beforebranchName.trim()empty-check threw. Same fix.Each bug has a regression test that fails on the previous code and passes on the new code.
Test infrastructure
test/helpers/createTestHost.ts— bootscreateAppagainst abun:sqlite-backed drizzle db with fake providers, returns an in-process tRPC client overapp.fetch(no port, no network).bun:sqliteis used becausebetter-sqlite3's native binding isn't loadable under Bun (Support V8 C++ APIs for "nan" addons and other packages to work oven-sh/bun#4290); production still usesbetter-sqlite3in the bundled-Node host process.test/helpers/fakes.ts—FakeApiAuthProvider,FakeHostAuthProvider,MemoryGitCredentialProvider,FakeModelResolver, plus a Proxy-basedcreateFakeApiClientthat records calls and throws on unmocked procedure paths so test gaps fail loud instead of returning undefined.test/helpers/git-fixture.ts— real on-disk git repos in tmpdir withrealpathSync(matters for workspace-fs's realpath-based root checks on macOS where tmpdir is symlinked).src/app.ts—CreateAppOptionsaccepts optionaldb,api,github,chatRuntime,chatServiceoverrides;CreateAppResultexposesdispose(). Production path is unchanged when overrides are absent.Coverage
24 integration files / 182 tests covering every router in
appRouter:searchBranches,getContext,getProgress,generateBranchName,adopthappy + error paths,create/checkoutvalidation, mocked-Octokit GitHub searchlistBranches,getStatus(clean/dirty/detached),getBaseBranchround-trip,renameBranch,listCommits,getCommitFiles,getDiff,getBranchSyncStatusgetByWorkspaceshydration,refreshByWorkspacesno-opChatRuntimeManager(no mastra dependency)ChatServiceTest plan
bun test packages/host-service— 361 pass, 1 pre-existing failure unrelated (pull-requests.test.ts:71shape mismatch from fix(host-service): dedupe PR refresh calls with repo-keyed cache #3884; should be a separate fix)bun run --filter @superset/host-service typecheck— cleanbun run lint:fix packages/host-service— clean (2 unrelated warnings)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Testing