mirror of
https://github.com/claude-code-best/claude-code.git
synced 2026-06-22 16:25:51 +00:00
test: keep review assertions tied to real failure paths
CodeRabbit flagged three non-blocking but valid review gaps: platform-specific mailbox errno checks, brittle UDS connection-failure message assertions, and missing AgentSummary reschedule proof after fork errors. This keeps the fixes narrow by tightening the affected assertions and adding a structured UDS connection error for tests to assert behavior instead of prose. Constraint: PR #374 is a review follow-up and must not hide warnings, skip tests, or merge the PR. Rejected: Matching the UDS failure message literal | preserves the brittle coupling CodeRabbit flagged. Rejected: Asserting only that mailbox writes throw | would allow unrelated pre-path failures to pass. Confidence: high Scope-risk: narrow Directive: Keep UDS connection-failure tests on structured error data, not display wording. Tested: bun test src/services/AgentSummary/__tests__/agentSummary.test.ts src/utils/__tests__/teammateMailbox.test.ts src/utils/__tests__/udsMessaging.test.ts Tested: bunx tsc --noEmit --pretty false Tested: bun run lint Tested: bun run test:all Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage Tested: bun run build Tested: bun run build:vite Not-tested: GitHub-hosted CodeRabbit refresh until pushed.
This commit is contained in:
@@ -33,6 +33,7 @@ describe('startAgentSummarization', () => {
|
|||||||
let debugLogs: string[]
|
let debugLogs: string[]
|
||||||
let loggedErrors: Error[]
|
let loggedErrors: Error[]
|
||||||
let clearedHandles: unknown[]
|
let clearedHandles: unknown[]
|
||||||
|
let scheduledCount: number
|
||||||
|
|
||||||
function startTestSummarization(
|
function startTestSummarization(
|
||||||
dependencies: AgentSummaryDependencies = {},
|
dependencies: AgentSummaryDependencies = {},
|
||||||
@@ -81,8 +82,9 @@ describe('startAgentSummarization', () => {
|
|||||||
if (typeof callback !== 'function') {
|
if (typeof callback !== 'function') {
|
||||||
throw new Error('Expected timer callback')
|
throw new Error('Expected timer callback')
|
||||||
}
|
}
|
||||||
|
scheduledCount += 1
|
||||||
scheduled = callback as () => void | Promise<void>
|
scheduled = callback as () => void | Promise<void>
|
||||||
return 1 as unknown as ReturnType<typeof setTimeout>
|
return scheduledCount as unknown as ReturnType<typeof setTimeout>
|
||||||
}) as unknown as typeof setTimeout,
|
}) as unknown as typeof setTimeout,
|
||||||
updateAgentSummary: (taskId: string, summary: string) => {
|
updateAgentSummary: (taskId: string, summary: string) => {
|
||||||
updateCalls.push({ taskId, summary })
|
updateCalls.push({ taskId, summary })
|
||||||
@@ -101,6 +103,7 @@ describe('startAgentSummarization', () => {
|
|||||||
debugLogs = []
|
debugLogs = []
|
||||||
loggedErrors = []
|
loggedErrors = []
|
||||||
clearedHandles = []
|
clearedHandles = []
|
||||||
|
scheduledCount = 0
|
||||||
})
|
})
|
||||||
|
|
||||||
test('summarizes bounded transcript once and skips unchanged fingerprints', async () => {
|
test('summarizes bounded transcript once and skips unchanged fingerprints', async () => {
|
||||||
@@ -175,6 +178,7 @@ describe('startAgentSummarization', () => {
|
|||||||
})
|
})
|
||||||
|
|
||||||
expect(typeof scheduled).toBe('function')
|
expect(typeof scheduled).toBe('function')
|
||||||
|
const initialScheduledCount = scheduledCount
|
||||||
await scheduled!()
|
await scheduled!()
|
||||||
|
|
||||||
expect(forkCalls).toEqual([])
|
expect(forkCalls).toEqual([])
|
||||||
@@ -182,6 +186,7 @@ describe('startAgentSummarization', () => {
|
|||||||
expect(debugLogs).toContain(
|
expect(debugLogs).toContain(
|
||||||
'[AgentSummary] Skipping summary — poor mode active',
|
'[AgentSummary] Skipping summary — poor mode active',
|
||||||
)
|
)
|
||||||
|
expect(scheduledCount).toBe(initialScheduledCount + 1)
|
||||||
})
|
})
|
||||||
|
|
||||||
test('logs summary errors and keeps the next timer owned by the summarizer', async () => {
|
test('logs summary errors and keeps the next timer owned by the summarizer', async () => {
|
||||||
@@ -193,10 +198,12 @@ describe('startAgentSummarization', () => {
|
|||||||
})
|
})
|
||||||
|
|
||||||
expect(typeof scheduled).toBe('function')
|
expect(typeof scheduled).toBe('function')
|
||||||
|
const initialScheduledCount = scheduledCount
|
||||||
await scheduled!()
|
await scheduled!()
|
||||||
|
|
||||||
expect(loggedErrors).toEqual([error])
|
expect(loggedErrors).toEqual([error])
|
||||||
expect(updateCalls).toEqual([])
|
expect(updateCalls).toEqual([])
|
||||||
|
expect(scheduledCount).toBe(initialScheduledCount + 1)
|
||||||
})
|
})
|
||||||
|
|
||||||
test('stop clears the pending summary timer', () => {
|
test('stop clears the pending summary timer', () => {
|
||||||
|
|||||||
@@ -1,9 +1,10 @@
|
|||||||
import { afterEach, beforeEach, describe, expect, test } from 'bun:test'
|
import { afterEach, beforeEach, describe, expect, test } from 'bun:test'
|
||||||
import { mkdir, readFile, rm, writeFile } from 'node:fs/promises'
|
import { mkdir, readFile, rm, stat, writeFile } from 'node:fs/promises'
|
||||||
import { mkdtempSync } from 'node:fs'
|
import { mkdtempSync } from 'node:fs'
|
||||||
import { tmpdir } from 'node:os'
|
import { tmpdir } from 'node:os'
|
||||||
import { dirname, join } from 'node:path'
|
import { dirname, join } from 'node:path'
|
||||||
import type { Message } from 'src/types/message.js'
|
import type { Message } from 'src/types/message.js'
|
||||||
|
import { getErrnoCode } from 'src/utils/errors.js'
|
||||||
import {
|
import {
|
||||||
compactMailboxMessages,
|
compactMailboxMessages,
|
||||||
getLastPeerDmSummary,
|
getLastPeerDmSummary,
|
||||||
@@ -356,10 +357,16 @@ describe('teammate mailbox retention', () => {
|
|||||||
'alpha',
|
'alpha',
|
||||||
).then(
|
).then(
|
||||||
() => undefined,
|
() => undefined,
|
||||||
error => error as NodeJS.ErrnoException,
|
err => err,
|
||||||
)
|
)
|
||||||
|
|
||||||
expect(error?.code).toBe('EISDIR')
|
const code = getErrnoCode(error)
|
||||||
|
expect(code).toBeDefined()
|
||||||
|
if (code === undefined) {
|
||||||
|
throw new Error('Expected filesystem errno code')
|
||||||
|
}
|
||||||
|
expect(['EISDIR', 'EPERM', 'EACCES']).toContain(code)
|
||||||
|
expect((await stat(inboxPath)).isDirectory()).toBe(true)
|
||||||
})
|
})
|
||||||
|
|
||||||
test('readMailbox fails closed on corrupt mailbox content', async () => {
|
test('readMailbox fails closed on corrupt mailbox content', async () => {
|
||||||
|
|||||||
@@ -227,11 +227,20 @@ describe('UDS inbox retention', () => {
|
|||||||
JSON.stringify({ socketPath: path, authToken: 'test-token' }),
|
JSON.stringify({ socketPath: path, authToken: 'test-token' }),
|
||||||
'utf-8',
|
'utf-8',
|
||||||
)
|
)
|
||||||
const { sendToUdsSocket } = await import('../udsClient.js')
|
const { sendToUdsSocket, UdsPeerConnectionError } = await import(
|
||||||
|
'../udsClient.js'
|
||||||
await expect(sendToUdsSocket(path, 'hello')).rejects.toThrow(
|
|
||||||
'Failed to connect to peer',
|
|
||||||
)
|
)
|
||||||
|
|
||||||
|
const error = await sendToUdsSocket(path, 'hello').then(
|
||||||
|
() => undefined,
|
||||||
|
err => err,
|
||||||
|
)
|
||||||
|
expect(error).toBeInstanceOf(UdsPeerConnectionError)
|
||||||
|
if (!(error instanceof UdsPeerConnectionError)) {
|
||||||
|
throw new Error('Expected UDS peer connection error')
|
||||||
|
}
|
||||||
|
expect(error.socketPath).toBe(path)
|
||||||
|
expect(error.message).not.toContain('test-token')
|
||||||
})
|
})
|
||||||
|
|
||||||
test('sendUdsMessage fails closed before connecting without an auth token', async () => {
|
test('sendUdsMessage fails closed before connecting without an auth token', async () => {
|
||||||
|
|||||||
@@ -36,6 +36,18 @@ export type PeerSession = {
|
|||||||
alive: boolean
|
alive: boolean
|
||||||
}
|
}
|
||||||
|
|
||||||
|
export class UdsPeerConnectionError extends Error {
|
||||||
|
readonly socketPath: string
|
||||||
|
readonly cause: unknown
|
||||||
|
|
||||||
|
constructor(socketPath: string, cause: unknown) {
|
||||||
|
super(`Failed to connect to peer at ${socketPath}: ${errorMessage(cause)}`)
|
||||||
|
this.name = 'UdsPeerConnectionError'
|
||||||
|
this.socketPath = socketPath
|
||||||
|
this.cause = cause
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
// Session directory
|
// Session directory
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
@@ -237,9 +249,7 @@ export async function sendToUdsSocket(
|
|||||||
maxFrameBytes: MAX_UDS_FRAME_BYTES,
|
maxFrameBytes: MAX_UDS_FRAME_BYTES,
|
||||||
onSettled: finish,
|
onSettled: finish,
|
||||||
formatSocketError: err =>
|
formatSocketError: err =>
|
||||||
new Error(
|
new UdsPeerConnectionError(target.socketPath, err),
|
||||||
`Failed to connect to peer at ${target.socketPath}: ${errorMessage(err)}`,
|
|
||||||
),
|
|
||||||
})
|
})
|
||||||
conn.setTimeout(5000, () => {
|
conn.setTimeout(5000, () => {
|
||||||
finish(new Error('Connection timed out'))
|
finish(new Error('Connection timed out'))
|
||||||
|
|||||||
Reference in New Issue
Block a user