Compare commits

..

5 Commits

Author SHA1 Message Date
unraid
f3f8c9339b fix: keep UDS peer failures structured
CodeRabbit and Claude cross-review identified that timeout and raw peer connection failures should share one observable error contract. UDS peer failures now use UdsPeerConnectionError consistently, and connectToPeer hands the socket lifecycle back to the caller after a successful connection instead of retaining an internal timeout or error listener.

The tests cover the real socket paths with capability files, timeout behavior, connection failure structure, post-connect listener handoff, AgentSummary rescheduling observations, and platform-specific mailbox directory errno handling.

Constraint: Preserve the 5000ms production timeout default while allowing tests to exercise timeout paths quickly.

Rejected: Suppress CodeRabbit warnings in tests | would hide the real timeout/error contract gap.

Rejected: Keep connectToPeer post-connect error listener | it would silently swallow caller-owned socket errors.

Confidence: high

Scope-risk: narrow

Directive: Keep UDS send/connect timeout and socket-error paths on the same structured peer error contract.

Tested: bun test src/utils/__tests__/udsMessaging.test.ts src/services/AgentSummary/__tests__/agentSummary.test.ts src/utils/__tests__/teammateMailbox.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

Tested: omx ask claude simplify review artifact .omx/artifacts/claude-review-only-cross-check-for-pr-374-on-branch-codex-codecov-r-2026-04-27T08-17-47-309Z.md

Tested: omx ask claude security review artifact .omx/artifacts/claude-security-review-cross-check-for-pr-374-current-working-tree--2026-04-27T08-26-54-079Z.md

Not-tested: GitHub-hosted CodeRabbit refresh until pushed.
2026-04-27 16:31:02 +08:00
unraid
3305da0d49 test: enforce structured UDS timeout failures
CodeRabbit's follow-up surfaced a real consistency gap: UDS send socket errors used UdsPeerConnectionError while response timeouts still rejected a generic Error. Timeouts now use the same structured peer failure contract, and the test exercises that path through a short explicit timeout instead of waiting for the production default.

The AgentSummary unchanged-fingerprint test now also asserts that the second unchanged tick does not log errors, preserving the existing behavior checks without changing production scheduling semantics.

Constraint: Keep the production timeout default at 5000ms while allowing tests to exercise the timeout path quickly.

Rejected: Leave timeout failures as generic Error | callers would need separate handling for the same peer connection failure class.

Confidence: high

Scope-risk: narrow

Directive: Keep UDS send timeout and socket-error branches on the same structured error contract.

Tested: bun test src/services/AgentSummary/__tests__/agentSummary.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.
2026-04-27 16:13:04 +08:00
unraid
2c7131cea6 test: remove brittle review follow-up assumptions
CodeRabbit's second pass found two valid brittleness issues and one suggested callback-reference assertion that would not match production behavior. This keeps the production behavior unchanged: timers still schedule the summarizer closure, tests now assert timer-handle identity, and UDS connection errors use native Error.cause instead of shadowing it.

Constraint: Do not manufacture behavior just to satisfy a review hint; assertions must match the real AgentSummary scheduling contract.

Rejected: Assert a fresh scheduled callback function | scheduleNext intentionally passes the same runSummary closure each time.

Rejected: Store a custom cause field on UdsPeerConnectionError | native Error.cause is available under ESNext/Bun.

Confidence: high

Scope-risk: narrow

Directive: Timer tests should assert returned handle identity for ownership, not incidental numeric values.

Tested: bun test src/services/AgentSummary/__tests__/agentSummary.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.
2026-04-27 16:02:49 +08:00
unraid
5ad3b316d5 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.
2026-04-27 15:54:13 +08:00
unraid
bc72dc2b09 test: keep Codecov coverage on real agent communication paths
PR #369 was merged before the final Codecov coverage fix landed, so this follow-up carries only the incremental real-path tests needed on top of main. The tests exercise AgentSummary lifecycle branches, mailbox fail-closed behavior, UDS client connection failure through a real capability file, and UDS response-reader framing without mock.module, warning suppression, feature fallback, or production-code churn.

Constraint: PR #369 is already merged; this branch must contain only the incremental Codecov repair on top of latest main

Rejected: Reopen or keep pushing the merged PR branch | merged PR refs do not update and would leave Codecov stale

Rejected: Mock bun:bundle or hide warnings | would reintroduce cross-test pollution and pseudo coverage

Rejected: Keep unrelated SendMessageTool production diff | it created avoidable patch-coverage debt without improving the runtime path

Confidence: high

Scope-risk: narrow

Directive: Keep these coverage tests on real paths; do not replace them with output suppression or feature-flag mocks

Tested: bunx tsc --noEmit --pretty false

Tested: bun run lint

Tested: bun test src\utils\__tests__\teammateMailbox.test.ts

Tested: bun test src\services\AgentSummary\__tests__\agentSummary.test.ts src\services\AgentSummary\__tests__\summaryContext.test.ts src\utils\__tests__\teammateMailbox.test.ts src\utils\__tests__\udsMessaging.test.ts src\utils\__tests__\udsResponseReader.test.ts packages\builtin-tools\src\tools\SendMessageTool\__tests__\udsRecipientSanitization.test.ts

Tested: bun run test:all

Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage

Tested: bun run build

Tested: bun run build:vite

Tested: bun audit

Tested: git diff --check

Tested: Claude simplify review GO (.omx/artifacts/claude-simplify-codecov-20260427-1521.md)

Tested: Claude security review GO (.omx/artifacts/claude-security-codecov-20260427-1522.md)

Not-tested: GitHub-hosted Codecov upload after this amended commit until PR checks rerun
2026-04-27 15:14:38 +08:00
4 changed files with 33 additions and 69 deletions

File diff suppressed because one or more lines are too long

Before

Width:  |  Height:  |  Size: 1.7 MiB

After

Width:  |  Height:  |  Size: 1.6 MiB

View File

@@ -161,9 +161,7 @@ describe('startAgentSummarization', () => {
expect(forkCalls).toEqual([])
expect(updateCalls).toEqual([])
expectDebugLogContaining(
'[AgentSummary] Skipping summary for task-1: no bounded context available',
)
expectDebugLogContaining('no bounded context available')
})
test('skips summarization before building context when transcript is too short', async () => {
@@ -175,9 +173,7 @@ describe('startAgentSummarization', () => {
expect(forkCalls).toEqual([])
expect(updateCalls).toEqual([])
expectDebugLogContaining(
'[AgentSummary] Skipping summary for task-1: not enough messages (2)',
)
expectDebugLogContaining('not enough messages (2)')
})
test('skips and reschedules while poor mode is active', async () => {
@@ -192,7 +188,7 @@ describe('startAgentSummarization', () => {
expect(forkCalls).toEqual([])
expect(updateCalls).toEqual([])
expectDebugLogContaining('[AgentSummary] Skipping summary — poor mode active')
expectDebugLogContaining('poor mode active')
expect(scheduledCount).toBe(initialScheduledCount + 1)
expect(lastTimerHandle).not.toBe(initialTimerHandle)
})
@@ -222,7 +218,7 @@ describe('startAgentSummarization', () => {
handle.stop()
expectDebugLogContaining('[AgentSummary] Stopping summarization for task-1')
expectDebugLogContaining('Stopping summarization for task-1')
expect(clearedHandles).toEqual([pendingHandle])
})
})

View File

@@ -307,9 +307,7 @@ describe('UDS inbox retention', () => {
'../udsClient.js'
)
const error = await connectToPeer(path, () => {
throw new Error('Unexpected post-connect socket error')
}).then(
const error = await connectToPeer(path).then(
() => undefined,
err => err,
)
@@ -340,24 +338,13 @@ describe('UDS inbox retention', () => {
})
let client: Socket | undefined
const socketErrors: Error[] = []
try {
const { connectToPeer } = await import('../udsClient.js')
client = await connectToPeer(
path,
error => {
socketErrors.push(error)
},
1000,
)
client = await connectToPeer(path, 50)
await new Promise(resolve => setTimeout(resolve, 100))
expect(client.destroyed).toBe(false)
expect(client.listenerCount('error')).toBe(1)
const socketError = new Error('post-connect failure')
client.emit('error', socketError)
expect(socketErrors).toEqual([socketError])
expect(client.listenerCount('error')).toBe(0)
} finally {
client?.destroy()
for (const socket of sockets) {

View File

@@ -266,48 +266,33 @@ export async function sendToUdsSocket(
/**
* Connect to a peer and return the raw socket for bidirectional communication.
* The caller owns the post-connect lifecycle through onSocketError, which is
* attached before the Promise resolves so peer socket errors cannot be
* swallowed or surface through a listener handoff window.
* Pre-connect failures reject with UdsPeerConnectionError.
* This only opens the transport; callers still own any capability handshake.
* The caller is responsible for managing the connection lifecycle.
*/
export function connectToPeer(
socketPath: string,
onSocketError: (error: Error) => void,
timeoutMs = 5000,
): Promise<Socket> {
return new Promise<Socket>((resolve, reject) => {
const conn = createConnection(socketPath)
let settled = false
const timeout = setTimeout(
fail,
timeoutMs,
new Error('Connection timed out'),
)
function cleanupListeners(): void {
clearTimeout(timeout)
conn.off('error', fail)
}
function fail(cause: unknown): void {
const fail = (cause: unknown) => {
if (settled) {
return
}
settled = true
cleanupListeners()
conn.destroy()
reject(new UdsPeerConnectionError(socketPath, cause))
}
conn.once('connect', () => {
if (settled) {
return
}
settled = true
cleanupListeners()
conn.on('error', onSocketError)
conn.setTimeout(0)
conn.off('error', fail)
resolve(conn)
})
conn.on('error', fail)
conn.setTimeout(timeoutMs, () => {
fail(new Error('Connection timed out'))
})
})
}