test(web): Add unit tests for daemon web HTTP layer - #109
Merged
Conversation
Expand tests for grpc-web-control RPC helpers, http-security middleware, OpenAI-compat routes, validate-body helpers, API schemas, and createWebApp routes to exercise auth, CORS, CSRF, and config validation paths.
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded broad Vitest coverage for daemon web schemas, validation, security middleware, gRPC control, OpenAI-compatible routes, and embedded-server behavior. ChangesDaemon web test coverage
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
sudhirverma
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #109 +/- ##
==========================================
+ Coverage 63.70% 67.78% +4.08%
==========================================
Files 37 37
Lines 4973 4973
Branches 1568 1568
==========================================
+ Hits 3168 3371 +203
+ Misses 1262 1056 -206
- Partials 543 546 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/daemon/src/daemon/web/http-security.test.ts (2)
593-613: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a negative CSRF case with a mismatched header.
The current test covers "no CSRF" and "matching CSRF". It does not cover a present-but-wrong CSRF header with no allowlisted origin. That path is the main CSRF bypass risk in
createAuthMiddleware.♻️ Proposed extra case
expect(withCsrf.nextCalled).toBe(true); + + const mismatched = runMiddleware(createAuthMiddleware(token, corsOrigins), { + method: 'POST', + headers: { + cookie: `${API_TOKEN_COOKIE}=${encodeURIComponent(token)}; ${CSRF_COOKIE}=${encodeURIComponent(csrf)}`, + [CSRF_HEADER]: 'other-value', + }, + } as Partial<Request>); + expect(mismatched.nextCalled).toBe(false); + expect(mismatched.statusCode).toBe(403);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/daemon/src/daemon/web/http-security.test.ts` around lines 593 - 613, Add a negative assertion in the test “requires CSRF or allowlisted origin for cookie auth on POST” using the existing CSRF cookie but a mismatched CSRF header and no allowlisted origin. Verify the middleware does not call next and returns status 403, alongside the existing missing-token and matching-token cases.
447-451: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten two assertions to lock the contract.
Line 449 checks only the file name suffix. The test name states "points under the config directory", so assert the directory too. Line 573 checks the 401 status and body, but not the
WWW-Authenticateheader thatcreateAuthMiddlewaresets. A regression that drops that header stays undetected.♻️ Proposed assertions
- expect(getHttpApiTokenPath()).toMatch(/http-api-token$/); + expect(getHttpApiTokenPath()).toBe(path.join(getConfigDir(), 'http-api-token'));expect(res.statusCode).toBe(401); + expect(res.headers['www-authenticate']).toBe('Bearer'); expect(res.body).toEqual({ error: 'Unauthorized' });Use the mocked config dir helper already available in this test file if
getConfigDiris not imported.Also applies to: 573-581
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/daemon/src/daemon/web/http-security.test.ts` around lines 447 - 451, Strengthen the getHttpApiTokenPath test to assert the returned path is inside the mocked config directory, not only that it ends with http-api-token; use the existing config-directory helper if needed. In the unauthorized-response test around createAuthMiddleware, also assert the WWW-Authenticate header is present with the expected value while preserving the existing 401 status and body assertions.packages/daemon/src/daemon/web/server.test.ts (4)
984-990: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose the server in a
finallyblock.If the
expectat line 988 fails, line 989 never runs. The HTTP server stays open and holds the event loop, so the run can hang or report unrelated failures. Other tests in this file already usetry/finallyfor this.♻️ Proposed fix
const { httpServer, baseUrl } = await startTestApp(state); - const res = await httpRequest(baseUrl, 'GET', '/api/providers'); - expect(res.statusCode).toBe(500); - await stopTestApp(httpServer); + try { + const res = await httpRequest(baseUrl, 'GET', '/api/providers'); + expect(res.statusCode).toBe(500); + } finally { + await stopTestApp(httpServer); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/daemon/src/daemon/web/server.test.ts` around lines 984 - 990, Wrap the request and status assertion in a try/finally block within the “GET /api/providers returns 500 when listProviders throws” test, and call stopTestApp(httpServer) from the finally block so the server is always closed when assertions or requests fail.
27-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
tmpConfigDirintovi.hoistedand clean it up.
vi.mockcalls are hoisted above line 27. The factory runs when../../core/paths.jsis first imported, which happens during the hoisted import of./server.js. The mocked functions only readtmpConfigDirlazily, so this works today. It breaks with aReferenceErrorif any module readsgetConfigDir()at import time.vi.hoistedremoves that dependency on evaluation order.Also,
tmpConfigDiris never removed. Every run leaves a temp directory behind.♻️ Proposed change
-const tmpConfigDir = fs.mkdtempSync(path.join(os.tmpdir(), 'abbenay-server-unit-config-')); +const { tmpConfigDir } = vi.hoisted(() => { + const nodeFs = require('node:fs') as typeof import('node:fs'); + const nodeOs = require('node:os') as typeof import('node:os'); + const nodePath = require('node:path') as typeof import('node:path'); + return { + tmpConfigDir: nodeFs.mkdtempSync(nodePath.join(nodeOs.tmpdir(), 'abbenay-server-unit-config-')), + }; +}); vi.mock('../../core/paths.js', async (importOriginal) => {Add a global teardown for the directory:
+afterAll(() => { + fs.rmSync(tmpConfigDir, { recursive: true, force: true }); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/daemon/src/daemon/web/server.test.ts` around lines 27 - 36, Move tmpConfigDir initialization into vi.hoisted so the mocked getUserConfigPath, getWorkspaceConfigPath, and getConfigDir functions can safely access it during module evaluation. Add global teardown for the test suite that recursively removes tmpConfigDir after tests complete.
831-855: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests share one mutable
config.yamland depend on execution order.Several tests in this suite write to
path.join(tmpConfigDir, 'config.yaml'): thePOST /api/configtest at line 541, the provider configure tests at lines 805 and 823, and this test. This test then deletes the file at line 853. Any later test that expects the earlier config content fails. The suite passes only because of the current declaration order.Give this test its own config directory, or write the file with a unique name and point the mocked
getUserConfigPathat a per-test path. That removes the order coupling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/daemon/src/daemon/web/server.test.ts` around lines 831 - 855, Update the GET /api/mcp-servers test to use an isolated per-test configuration path instead of the shared tmpConfigDir/config.yaml. Point the mocked getUserConfigPath (or equivalent test configuration) to that unique directory or filename, and keep cleanup scoped to the isolated config so other tests’ configuration remains unaffected.
102-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a typed route-state fixture instead of broad casts.
The routes read
secretStore,sessionStore,toolRegistry,mcpClientPool,mcpServer, chat, provider/model, and client/workspace members. Define the mock withPick<DaemonState, ...>and explicit nested types. UseClientType.CLIinstead of'CLI' as never.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/daemon/src/daemon/web/server.test.ts` around lines 102 - 198, Update createMockState to use a typed Pick<DaemonState, ...> fixture covering the route-consumed secretStore, sessionStore, toolRegistry, mcpClientPool, mcpServer, chat, provider/model, and client/workspace members, with explicit types for nested mocks. Remove the broad unknown cast and type assertions, and represent CLI clients with ClientType.CLI rather than 'CLI' as never.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/daemon/src/daemon/web/grpc-web-control.test.ts`:
- Around line 50-55: Update the “constructs a client with default unix socket
when address is omitted” test to capture the gRPC client constructor arguments,
then assert the address is unix://${getDefaultSocketPath()} and
sslTargetNameOverride matches the configured value while preserving the existing
client and close assertions.
In `@packages/daemon/src/daemon/web/http-security.test.ts`:
- Around line 627-638: The test around createAuthMiddleware should exercise the
production middleware chain by running createCorsMiddleware before
authentication, and include https://abbenay.example.com in the configured
corsOrigins so the request reaches auth. Preserve the POST cookie-auth
assertion, and avoid relying on x-forwarded-proto unless the test explicitly
establishes the trusted proxy boundary required by isOriginAllowed.
---
Nitpick comments:
In `@packages/daemon/src/daemon/web/http-security.test.ts`:
- Around line 593-613: Add a negative assertion in the test “requires CSRF or
allowlisted origin for cookie auth on POST” using the existing CSRF cookie but a
mismatched CSRF header and no allowlisted origin. Verify the middleware does not
call next and returns status 403, alongside the existing missing-token and
matching-token cases.
- Around line 447-451: Strengthen the getHttpApiTokenPath test to assert the
returned path is inside the mocked config directory, not only that it ends with
http-api-token; use the existing config-directory helper if needed. In the
unauthorized-response test around createAuthMiddleware, also assert the
WWW-Authenticate header is present with the expected value while preserving the
existing 401 status and body assertions.
In `@packages/daemon/src/daemon/web/server.test.ts`:
- Around line 984-990: Wrap the request and status assertion in a try/finally
block within the “GET /api/providers returns 500 when listProviders throws”
test, and call stopTestApp(httpServer) from the finally block so the server is
always closed when assertions or requests fail.
- Around line 27-36: Move tmpConfigDir initialization into vi.hoisted so the
mocked getUserConfigPath, getWorkspaceConfigPath, and getConfigDir functions can
safely access it during module evaluation. Add global teardown for the test
suite that recursively removes tmpConfigDir after tests complete.
- Around line 831-855: Update the GET /api/mcp-servers test to use an isolated
per-test configuration path instead of the shared tmpConfigDir/config.yaml.
Point the mocked getUserConfigPath (or equivalent test configuration) to that
unique directory or filename, and keep cleanup scoped to the isolated config so
other tests’ configuration remains unaffected.
- Around line 102-198: Update createMockState to use a typed Pick<DaemonState,
...> fixture covering the route-consumed secretStore, sessionStore,
toolRegistry, mcpClientPool, mcpServer, chat, provider/model, and
client/workspace members, with explicit types for nested mocks. Remove the broad
unknown cast and type assertions, and represent CLI clients with ClientType.CLI
rather than 'CLI' as never.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46eca344-ebbb-485f-8d77-44d79339c063
📒 Files selected for processing (7)
packages/daemon/src/daemon/web/api-schemas.test.tspackages/daemon/src/daemon/web/grpc-web-control.rpc.test.tspackages/daemon/src/daemon/web/grpc-web-control.test.tspackages/daemon/src/daemon/web/http-security.test.tspackages/daemon/src/daemon/web/openai-compat.test.tspackages/daemon/src/daemon/web/server.test.tspackages/daemon/src/daemon/web/validate-body.test.ts
Strengthen grpc-web-control, http-security, and server test assertions from PR review: constructor args, CORS/auth chain, CSRF negatives, and isolated config fixtures.
Lazy-init tmp config dir inside the paths mock via dynamic import so vi.hoisted stays compatible with @typescript-eslint/no-require-imports.
sathyapramod
approved these changes
Aug 11, 2026
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
3 participants
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
packages/daemon/src/daemon/web/covering grpc-web-control RPC helpers, http-security (CORS/auth/CSRF), validate-body, API schemas, OpenAI-compat routes, and createWebApp HTTP routes.grpc-web-control.rpc.test.tsforsendStartWebServer/sendStopWebServerwithout a live daemon.finallyblocks.Commits
test(web): Add unit tests for daemon web HTTP layer— initial web-layer unit test coverageMerge branch 'main' into test/daemon-web-unit-coverage— sync with maintest(web): address CodeRabbit review feedback on web unit tests— review-driven test hardening (bbe0a4f)fix(test): avoid require() in server.test vi.hoisted for eslint— CI lint fix for lazy config dir init (32e72b2)Test plan
npm run lint -w packages/daemonnpm run test -w packages/daemon -- --run src/daemon/web/(226+ tests)npm test(all workspaces)