test: keep Codecov coverage on real agent communication paths#374
test: keep Codecov coverage on real agent communication paths#374claude-code-best merged 4 commits intomainfrom
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
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:
📝 WalkthroughWalkthroughRefactors and adds unit tests across AgentSummary and various utils; introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/utils/__tests__/teammateMailbox.test.ts (1)
345-360: Test asserts rejection but not the failure mode — consider tightening.Pre‑creating a directory at
inboxPathwill causewriteToMailboxto throw, but the failure can originate from at least three different places (initialwriteFile(..., { flag: 'wx' })rejecting withEISDIR/EEXIST,proper-lockfilefailing to lock, or the subsequent content write). A bare.rejects.toThrow()will pass for any of them, including unrelated regressions (e.g., a future change that throws a generic error before even touching the path).Consider asserting on a more specific signal so the test continues to cover the intended branch — e.g. matching the expected errno code or message fragment, and verifying the directory wasn't clobbered:
♻️ Suggested tightening
test('writeToMailbox rejects when the inbox path is already a directory', async () => { const inboxPath = getInboxPath('worker', 'alpha') await mkdir(inboxPath, { recursive: true }) await expect( writeToMailbox( 'worker', { from: 'team-lead', text: 'new', timestamp: new Date(5).toISOString(), }, 'alpha', ), - ).rejects.toThrow() + ).rejects.toThrow(/EISDIR|EEXIST|directory/i) + + // Directory should remain a directory; no file was written in its place. + expect((await import('node:fs/promises')).stat(inboxPath).then(s => s.isDirectory())).resolves.toBe(true) })(Or import
statat the top alongside the othernode:fs/promisesimports rather than dynamic-importing inline.)Also worth noting: per
src/utils/teammateMailbox.ts:372-385, onlyEEXISTis swallowed; on Linux,writeFileagainst an existing directory typically reportsEISDIR, so the rethrow path is what actually drives this test. A platform note or a code-level assertion would make that intent explicit and protect against silent regressions if the swallow set is ever broadened.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/teammateMailbox.test.ts` around lines 345 - 360, The test currently only checks that writeToMailbox rejects but not why; update the test for the inbox-directory case to assert the specific failure mode (match error.errno or error.message for EISDIR/EEXIST) and also verify the inboxPath remains a directory (use stat from node:fs/promises to assert isDirectory()), and import stat at the top with the other fs imports instead of dynamic-importing it inline; reference the writeToMailbox and getInboxPath calls and the EISDIR/EEXIST errno symbols when tightening the assertion.src/services/AgentSummary/__tests__/agentSummary.test.ts (2)
187-200: Test name promises more than it asserts.The test is named "logs summary errors and keeps the next timer owned by the summarizer", but only asserts
loggedErrorsandupdateCalls. PerrunSummary'sfinallyblock, after a thrown error the summarizer should callscheduleNext(which goes through the injectedsetTimeoutand overwritesscheduled). To match the name and actually cover the rescheduling path, consider asserting thatsetTimeoutwas re-invoked after the failure.♻️ Suggested assertion
expect(loggedErrors).toEqual([error]) expect(updateCalls).toEqual([]) + // finally → scheduleNext should reinstall a timer owned by the summarizer + expect(typeof scheduled).toBe('function')If you want to be stricter, capture the previous
scheduledreference before invoking and assert the new one is a different function instance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/AgentSummary/__tests__/agentSummary.test.ts` around lines 187 - 200, The test currently asserts only loggedErrors and updateCalls but does not verify that runSummary's finally path reschedules the next timer; update the test in agentSummary.test.ts (the test that calls startTestSummarization / scheduled) to capture the current scheduled reference before invoking scheduled!, then after awaiting scheduled! assert that the injected setTimeout was re-invoked by checking that the new scheduled is a function and not the same reference (or that a mocked setTimeout call count increased); reference startTestSummarization, scheduled, runSummary, scheduleNext and the injected setTimeout when making the assertion so the test covers the rescheduling path.
29-29:handleis only meaningfully used in thestoptest.Most tests assign
handle = startTestSummarization(...)but never read it. You can drop the outerlet handledeclaration and the unused assignments, and just callstartTestSummarization()directly (capturing the result only in the stop test). Minor cleanup; safe to defer.Also applies to: 107-107, 146-146, 160-160, 173-173, 189-193, 203-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/AgentSummary/__tests__/agentSummary.test.ts` at line 29, Remove the outer `let handle: { stop: () => void } | undefined` declaration and any test assignments that set `handle = startTestSummarization(...)` when the returned handle is never used; instead, call `startTestSummarization()` directly in those tests. Keep capturing the return value into `handle` only in the dedicated "stop" test where `handle.stop()` is invoked. Update references to `handle` elsewhere in the file (`startTestSummarization`, `stop` test) accordingly so only the stop test holds the handle and all other tests simply call `startTestSummarization()` without assignment.packages/builtin-tools/src/tools/SendMessageTool/SendMessageTool.ts (1)
618-623: Inline the decision to avoid a lexical declaration inside acaseblock.The added
const planApprovalDecisionis declared directly insidecase 'plan_approval_response':without braces. This is the pattern Biome'snoSwitchDeclarations(and ESLint'sno-case-declarations) flag, and it’s also inconsistent with the siblingshutdown_responsecase immediately above which uses an inline ternary. Inlining keeps the diff minimal (matches the PR’s stated "avoid unnecessary changed lines" goal) and keeps the switch lint-clean.♻️ Proposed simplification
case 'plan_approval_response': - const planApprovalDecision = input.message.approve - ? 'approve' - : 'reject' - return `plan_approval ${planApprovalDecision} to ${recipient}` + return `plan_approval ${input.message.approve ? 'approve' : 'reject'} to ${recipient}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/builtin-tools/src/tools/SendMessageTool/SendMessageTool.ts` around lines 618 - 623, In the switch inside SendMessageTool.ts for the 'plan_approval_response' case, avoid declaring const planApprovalDecision inside the case block; instead inline the ternary directly in the returned string (like the sibling shutdown_response) so you remove the planApprovalDecision binding and return `plan_approval ${input.message.approve ? 'approve' : 'reject'} to ${recipient}` (i.e., replace the planApprovalDecision reference with an inline conditional).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/builtin-tools/src/tools/SendMessageTool/SendMessageTool.ts`:
- Around line 618-623: In the switch inside SendMessageTool.ts for the
'plan_approval_response' case, avoid declaring const planApprovalDecision inside
the case block; instead inline the ternary directly in the returned string (like
the sibling shutdown_response) so you remove the planApprovalDecision binding
and return `plan_approval ${input.message.approve ? 'approve' : 'reject'} to
${recipient}` (i.e., replace the planApprovalDecision reference with an inline
conditional).
In `@src/services/AgentSummary/__tests__/agentSummary.test.ts`:
- Around line 187-200: The test currently asserts only loggedErrors and
updateCalls but does not verify that runSummary's finally path reschedules the
next timer; update the test in agentSummary.test.ts (the test that calls
startTestSummarization / scheduled) to capture the current scheduled reference
before invoking scheduled!, then after awaiting scheduled! assert that the
injected setTimeout was re-invoked by checking that the new scheduled is a
function and not the same reference (or that a mocked setTimeout call count
increased); reference startTestSummarization, scheduled, runSummary,
scheduleNext and the injected setTimeout when making the assertion so the test
covers the rescheduling path.
- Line 29: Remove the outer `let handle: { stop: () => void } | undefined`
declaration and any test assignments that set `handle =
startTestSummarization(...)` when the returned handle is never used; instead,
call `startTestSummarization()` directly in those tests. Keep capturing the
return value into `handle` only in the dedicated "stop" test where
`handle.stop()` is invoked. Update references to `handle` elsewhere in the file
(`startTestSummarization`, `stop` test) accordingly so only the stop test holds
the handle and all other tests simply call `startTestSummarization()` without
assignment.
In `@src/utils/__tests__/teammateMailbox.test.ts`:
- Around line 345-360: The test currently only checks that writeToMailbox
rejects but not why; update the test for the inbox-directory case to assert the
specific failure mode (match error.errno or error.message for EISDIR/EEXIST) and
also verify the inboxPath remains a directory (use stat from node:fs/promises to
assert isDirectory()), and import stat at the top with the other fs imports
instead of dynamic-importing it inline; reference the writeToMailbox and
getInboxPath calls and the EISDIR/EEXIST errno symbols when tightening the
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8238c0d-9dca-4311-b396-cf35bcb313fe
📒 Files selected for processing (6)
packages/builtin-tools/src/tools/SendMessageTool/SendMessageTool.tssrc/services/AgentSummary/__tests__/agentSummary.test.tssrc/services/AgentSummary/__tests__/summaryContext.test.tssrc/utils/__tests__/teammateMailbox.test.tssrc/utils/__tests__/udsMessaging.test.tssrc/utils/__tests__/udsResponseReader.test.ts
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
amDosion
force-pushed
the
codex/codecov-real-path-coverage
branch
from
fb30d00 to
74d9d14
Compare
amDosion
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/services/AgentSummary/__tests__/agentSummary.test.ts (1)
172-185: Optional: assert the reschedule side of "skips and reschedules while poor mode is active".The test name promises both skip and reschedule semantics, but the assertions only cover the skip path (empty
forkCalls/updateCallsand the debug log). SincerunSummarycallsscheduleNext()after the poor-mode early return, you can cheaply confirm the reschedule by capturing setTimeout invocations and asserting a new timer was registered afterscheduled!()resolves — otherwise a future regression that dropsscheduleNext()from the poor-mode branch would not fail this test.♻️ Suggested tightening
test('skips and reschedules while poor mode is active', async () => { + let setTimeoutCalls = 0 handle = startTestSummarization({ isPoorModeActive: () => true, + setTimeout: ((callback: TimerHandler) => { + setTimeoutCalls += 1 + scheduled = callback as () => void | Promise<void> + return 1 as unknown as ReturnType<typeof setTimeout> + }) as unknown as typeof setTimeout, }) expect(typeof scheduled).toBe('function') + const callsBeforeRun = setTimeoutCalls await scheduled!() expect(forkCalls).toEqual([]) expect(updateCalls).toEqual([]) expect(debugLogs).toContain( '[AgentSummary] Skipping summary — poor mode active', ) + expect(setTimeoutCalls).toBeGreaterThan(callsBeforeRun) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/AgentSummary/__tests__/agentSummary.test.ts` around lines 172 - 185, The test "skips and reschedules while poor mode is active" currently only asserts the skip behavior; update it to also assert the reschedule by spying on timer registration: in the test that calls startTestSummarization({ isPoorModeActive: () => true }) and invokes scheduled!(), instrument global timers (e.g., jest.useFakeTimers() or spy on setTimeout) before calling scheduled!(), then assert that scheduleNext() (the code path that registers a timeout) was invoked by checking that a new timer was created after scheduled!() resolves; reference the test helper startTestSummarization, the scheduled function returned, and the scheduleNext/runSummary behavior to locate where to add the timer spy and the additional expectation.src/utils/__tests__/udsMessaging.test.ts (1)
220-235: Strengthen test assertion to match the test name; consider adding file mode for future-proofing.The test is functionally correct but can be tightened in two ways:
Assertion mismatch: The name claims "without leaking token state," but the test only verifies the error message contains
'Failed to connect to peer'. It doesn't check that the token ('test-token') is absent from the error. Add.not.toContain('test-token')to match the intent.File permissions: The capability JSON is written via
writeFile(...)with nomodeoption, so it lands with default permissions (typically0o644). Currently,readUdsCapabilityTokendoes not validate file mode, so the test passes. However, if the code ever tightens to enforce0o600on capability files (as exists for directories), this test would silently start failing with a permissions error instead of the intended connect-failure path. Settingmode: 0o600on the file makes the test resilient to that future change.♻️ Proposed changes
await mkdir(capabilityDir, { recursive: true, mode: 0o700 }) await writeFile( join(capabilityDir, capabilityName), JSON.stringify({ socketPath: path, authToken: 'test-token' }), - 'utf-8', + { encoding: 'utf-8', mode: 0o600 }, ) const { sendToUdsSocket } = await import('../udsClient.js') - await expect(sendToUdsSocket(path, 'hello')).rejects.toThrow( - 'Failed to connect to peer', - ) + const error = await sendToUdsSocket(path, 'hello').then( + () => { + throw new Error('expected sendToUdsSocket to reject') + }, + (err: unknown) => err as Error, + ) + expect(error.message).toContain('Failed to connect to peer') + expect(error.message).not.toContain('test-token')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/udsMessaging.test.ts` around lines 220 - 235, Update the test "udsClient send reports connection failures without leaking token state" to assert that the thrown error does not include the capability token and make the capability file mode strict: when writing the capability JSON (the writeFile call that creates the capabilityName with socketPath and authToken), add mode: 0o600 so the file has owner-only permissions, and after calling sendToUdsSocket(path, 'hello') assert the rejection message contains the connection-failure text AND .not.toContain('test-token') to ensure the token is not leaked from sendToUdsSocket's error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/services/AgentSummary/__tests__/agentSummary.test.ts`:
- Around line 172-185: The test "skips and reschedules while poor mode is
active" currently only asserts the skip behavior; update it to also assert the
reschedule by spying on timer registration: in the test that calls
startTestSummarization({ isPoorModeActive: () => true }) and invokes
scheduled!(), instrument global timers (e.g., jest.useFakeTimers() or spy on
setTimeout) before calling scheduled!(), then assert that scheduleNext() (the
code path that registers a timeout) was invoked by checking that a new timer was
created after scheduled!() resolves; reference the test helper
startTestSummarization, the scheduled function returned, and the
scheduleNext/runSummary behavior to locate where to add the timer spy and the
additional expectation.
In `@src/utils/__tests__/udsMessaging.test.ts`:
- Around line 220-235: Update the test "udsClient send reports connection
failures without leaking token state" to assert that the thrown error does not
include the capability token and make the capability file mode strict: when
writing the capability JSON (the writeFile call that creates the capabilityName
with socketPath and authToken), add mode: 0o600 so the file has owner-only
permissions, and after calling sendToUdsSocket(path, 'hello') assert the
rejection message contains the connection-failure text AND
.not.toContain('test-token') to ensure the token is not leaked from
sendToUdsSocket's error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dd28baa-f94c-48b0-affe-199bc1932b0b
📒 Files selected for processing (5)
src/services/AgentSummary/__tests__/agentSummary.test.tssrc/services/AgentSummary/__tests__/summaryContext.test.tssrc/utils/__tests__/teammateMailbox.test.tssrc/utils/__tests__/udsMessaging.test.tssrc/utils/__tests__/udsResponseReader.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/services/AgentSummary/tests/summaryContext.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/tests/teammateMailbox.test.ts
- src/utils/tests/udsResponseReader.test.ts
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
amDosion
force-pushed
the
codex/codecov-real-path-coverage
branch
from
74d9d14 to
bc72dc2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/utils/__tests__/teammateMailbox.test.ts (1)
345-363: Cross-platform note onEISDIR.The assertion
error?.code === 'EISDIR'relies on libuv/Bun surfacing the POSIX error code for writes targeting a directory. This is reliable on Linux/macOS but can differ on Windows (e.g.,EPERM/EACCES). If CI is Linux-only this is fine; otherwise consider matching against a small allow-list or assertingerroris truthy with a regex on the message.Also, the inner callback parameter
errorshadows the outerconst error; renaming the parameter (e.g.,err) would marginally improve readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/teammateMailbox.test.ts` around lines 345 - 363, The test 'writeToMailbox rejects when the inbox path is already a directory' relies on a platform-specific errno ('EISDIR') and shadows the outer const error with the inner promise rejection parameter; update the assertion to be cross-platform by either checking that an error was thrown and matching its code against an allow-list (e.g., ['EISDIR','EPERM','EACCES']) or asserting error is truthy and matching its message with a regex, and rename the inner promise rejection parameter from error to err to avoid shadowing; ensure the test still captures the rejected value from writeToMailbox and performs the new, platform-robust assertion.src/utils/__tests__/udsMessaging.test.ts (1)
220-235: LGTM — connection-failure path is exercised cleanly.The test correctly seeds a valid capability file (so the
'No auth token found'early-return is not the rejection reason) and then asserts the rejection comes from the actual connect step. Hashing the socket path withsha256and writing{ socketPath, authToken }matches the capability lookup convention used elsewhere in this file (see line 477), so the assertion truly targets the post-auth-load branch.One small thing to keep in mind: the rejected message
'Failed to connect to peer'is a string-literal coupling toudsClient.ts. If that wording is ever rephrased the test will silently start failing on the right code path for the wrong reason — consider importing the message constant fromudsClient.ts(or matching on acause/error code) if such a constant becomes available. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/udsMessaging.test.ts` around lines 220 - 235, Test currently asserts the rejection message string literal 'Failed to connect to peer' which couples the test to udsClient.ts wording; update the test in udsMessaging.test.ts to import the error message constant (or exported error code) from ../udsClient.js and assert against that constant (or alternatively assert on the Error.cause or an error.code property) when calling sendToUdsSocket so the test verifies the correct failure path without brittle string matching.src/services/AgentSummary/__tests__/agentSummary.test.ts (1)
187-200: Test name promises a rescheduling assertion that isn't made.The test is named
…and keeps the next timer owned by the summarizer, but the body only verifiesloggedErrorsandupdateCalls. The interesting behavior inagentSummary.ts'sfinallyblock (scheduleNext()after the catch) — i.e., that a new timer is registered post-error — is not exercised. SincesetTimeoutis mocked to overwritescheduled, capturing the pre-error reference and asserting it was replaced (or simply thatscheduledis still a function) would close the gap and prevent a regression where the summarizer silently stops rescheduling on errors.♻️ Proposed assertion to match the test name
test('logs summary errors and keeps the next timer owned by the summarizer', async () => { const error = new Error('fork failed') handle = startTestSummarization({ runForkedAgent: async () => { throw error }, }) expect(typeof scheduled).toBe('function') + const initialScheduled = scheduled await scheduled!() expect(loggedErrors).toEqual([error]) expect(updateCalls).toEqual([]) + // finally { scheduleNext() } must re-arm the timer even after an error + expect(typeof scheduled).toBe('function') + expect(scheduled).not.toBe(initialScheduled) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/AgentSummary/__tests__/agentSummary.test.ts` around lines 187 - 200, The test currently checks loggedErrors and updateCalls but doesn't assert that the summarizer re-registers a timer after an error; update the test that uses startTestSummarization and the mocked setTimeout/scheduled to capture the scheduled function reference before invoking scheduled!, then after awaiting scheduled! assert that scheduled has been replaced (e.g., is a different function or remains a function) to confirm scheduleNext in agentSummary.ts's finally block ran and a new timer was set; reference the test helpers startTestSummarization, the mocked scheduled variable, and the scheduleNext behavior in agentSummary.ts when adding this assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/services/AgentSummary/__tests__/agentSummary.test.ts`:
- Around line 187-200: The test currently checks loggedErrors and updateCalls
but doesn't assert that the summarizer re-registers a timer after an error;
update the test that uses startTestSummarization and the mocked
setTimeout/scheduled to capture the scheduled function reference before invoking
scheduled!, then after awaiting scheduled! assert that scheduled has been
replaced (e.g., is a different function or remains a function) to confirm
scheduleNext in agentSummary.ts's finally block ran and a new timer was set;
reference the test helpers startTestSummarization, the mocked scheduled
variable, and the scheduleNext behavior in agentSummary.ts when adding this
assertion.
In `@src/utils/__tests__/teammateMailbox.test.ts`:
- Around line 345-363: The test 'writeToMailbox rejects when the inbox path is
already a directory' relies on a platform-specific errno ('EISDIR') and shadows
the outer const error with the inner promise rejection parameter; update the
assertion to be cross-platform by either checking that an error was thrown and
matching its code against an allow-list (e.g., ['EISDIR','EPERM','EACCES']) or
asserting error is truthy and matching its message with a regex, and rename the
inner promise rejection parameter from error to err to avoid shadowing; ensure
the test still captures the rejected value from writeToMailbox and performs the
new, platform-robust assertion.
In `@src/utils/__tests__/udsMessaging.test.ts`:
- Around line 220-235: Test currently asserts the rejection message string
literal 'Failed to connect to peer' which couples the test to udsClient.ts
wording; update the test in udsMessaging.test.ts to import the error message
constant (or exported error code) from ../udsClient.js and assert against that
constant (or alternatively assert on the Error.cause or an error.code property)
when calling sendToUdsSocket so the test verifies the correct failure path
without brittle string matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a043c96b-d6b4-4cec-b757-0efdca4290b7
📒 Files selected for processing (5)
src/services/AgentSummary/__tests__/agentSummary.test.tssrc/services/AgentSummary/__tests__/summaryContext.test.tssrc/utils/__tests__/teammateMailbox.test.tssrc/utils/__tests__/udsMessaging.test.tssrc/utils/__tests__/udsResponseReader.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/tests/udsResponseReader.test.ts
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.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/services/AgentSummary/__tests__/agentSummary.test.ts (2)
81-88: Brittle coupling: assert on the actual timer handle, not the counter value.
clearedHandles).toEqual([1])works only because the fakesetTimeoutreturnsscheduledCountas the handle andstop()happens to fire after exactly one schedule. If the production path ever schedules an additional timer beforestop()(e.g., a future warm-up tick), this expectation flips silently from "stop cleared the right handle" to "stop cleared handle#1instead of the latest one" — a false pass on identity, or a confusing failure on count. Capture the handle returned to the caller and assert identity.♻️ Proposed refactor
let clearedHandles: unknown[] let scheduledCount: number + let lastTimerHandle: unknown @@ setTimeout: ((callback: TimerHandler) => { if (typeof callback !== 'function') { throw new Error('Expected timer callback') } scheduledCount += 1 scheduled = callback as () => void | Promise<void> - return scheduledCount as unknown as ReturnType<typeof setTimeout> + lastTimerHandle = scheduledCount + return lastTimerHandle as unknown as ReturnType<typeof setTimeout> }) as unknown as typeof setTimeout, @@ test('stop clears the pending summary timer', () => { handle = startTestSummarization() + const pendingHandle = lastTimerHandle handle.stop() expect(debugLogs).toContain( '[AgentSummary] Stopping summarization for task-1', ) - expect(clearedHandles).toEqual([1]) + expect(clearedHandles).toEqual([pendingHandle]) })Also applies to: 209-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/AgentSummary/__tests__/agentSummary.test.ts` around lines 81 - 88, The test's fake setTimeout returns a numeric counter (scheduledCount) and the assertion checks for that counter in clearedHandles, which is brittle; instead have the fake setTimeout return a concrete handle object/value, capture the handle assigned when scheduling (e.g., the value assigned to scheduledHandle from the mocked setTimeout), and assert that clearedHandles contains that exact handle (identity) after calling stop(); update both occurrences (the mock around setTimeout, and the assertion at clearedHandles) so you capture and assert the actual returned handle rather than the counter (refer to the mocked setTimeout, the scheduled/scheduledCount variables, clearedHandles array, and the stop() call).
192-207: Minor: assertion doesn't fully back the test name.The test is titled "keeps the next timer owned by the summarizer" but only verifies
scheduledCountincreased by one — that proves a timer was scheduled, not that the rescheduled callback is the summarizer's own. Consider also asserting thatscheduledwas reassigned to a fresh function (e.g., capture the pre-call reference and assertscheduled !== prev) to lock down the ownership claim. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/AgentSummary/__tests__/agentSummary.test.ts` around lines 192 - 207, The test should also assert that the rescheduled callback reference changed so it really "keeps the next timer owned by the summarizer": before calling await scheduled!() capture the current scheduled reference (e.g., const prevScheduled = scheduled), then after the call assert scheduled !== prevScheduled in addition to the existing scheduledCount check; locate this change in the test block that uses startTestSummarization, scheduled, scheduledCount and loggedErrors to ensure the scheduled function was reassigned to a fresh function owned by the summarizer.src/utils/udsClient.ts (1)
39-49: Consider using the ES2022 nativeError.causeoptions bag to avoid shadowing the inherited property.The explicit
readonly causefield shadowsError.prototype.cause. Since your project targetsESNextand requires Bun ≥1.2.0 (which fully supports nativeError.cause), you can remove the field and pass{ cause }to thesuper()constructor, letting the Error engine populate the property natively.♻️ Suggested refactor
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)}`) + super( + `Failed to connect to peer at ${socketPath}: ${errorMessage(cause)}`, + { cause }, + ) this.name = 'UdsPeerConnectionError' this.socketPath = socketPath - this.cause = cause } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/udsClient.ts` around lines 39 - 49, The UdsPeerConnectionError class currently defines a readonly cause property that shadows the native Error.prototype.cause; remove the explicit readonly cause field and instead pass the cause via the Error options bag in the constructor (i.e., call super(message, { cause })) so the runtime populates the native cause; update the constructor of UdsPeerConnectionError (and keep socketPath and name assignment) to build the message using errorMessage(cause) and pass { cause } to super rather than storing cause on the instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/services/AgentSummary/__tests__/agentSummary.test.ts`:
- Around line 81-88: The test's fake setTimeout returns a numeric counter
(scheduledCount) and the assertion checks for that counter in clearedHandles,
which is brittle; instead have the fake setTimeout return a concrete handle
object/value, capture the handle assigned when scheduling (e.g., the value
assigned to scheduledHandle from the mocked setTimeout), and assert that
clearedHandles contains that exact handle (identity) after calling stop();
update both occurrences (the mock around setTimeout, and the assertion at
clearedHandles) so you capture and assert the actual returned handle rather than
the counter (refer to the mocked setTimeout, the scheduled/scheduledCount
variables, clearedHandles array, and the stop() call).
- Around line 192-207: The test should also assert that the rescheduled callback
reference changed so it really "keeps the next timer owned by the summarizer":
before calling await scheduled!() capture the current scheduled reference (e.g.,
const prevScheduled = scheduled), then after the call assert scheduled !==
prevScheduled in addition to the existing scheduledCount check; locate this
change in the test block that uses startTestSummarization, scheduled,
scheduledCount and loggedErrors to ensure the scheduled function was reassigned
to a fresh function owned by the summarizer.
In `@src/utils/udsClient.ts`:
- Around line 39-49: The UdsPeerConnectionError class currently defines a
readonly cause property that shadows the native Error.prototype.cause; remove
the explicit readonly cause field and instead pass the cause via the Error
options bag in the constructor (i.e., call super(message, { cause })) so the
runtime populates the native cause; update the constructor of
UdsPeerConnectionError (and keep socketPath and name assignment) to build the
message using errorMessage(cause) and pass { cause } to super rather than
storing cause on the instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ed5722f-1f92-45b7-a6de-48bcb7ff211b
📒 Files selected for processing (4)
src/services/AgentSummary/__tests__/agentSummary.test.tssrc/utils/__tests__/teammateMailbox.test.tssrc/utils/__tests__/udsMessaging.test.tssrc/utils/udsClient.ts
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.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/utils/udsClient.ts (1)
255-257: Consider also wrapping the connection-timeout path inUdsPeerConnectionError.The socket-error path (Line 252-253) now rejects with
UdsPeerConnectionError, but thesetTimeoutbranch at Line 256 still rejects with a genericError('Connection timed out'). Callers that branch oninstanceof UdsPeerConnectionError(and readsocketPath) won't get the structured error for timeout-induced connection failures, which is arguably the same failure class from the caller's perspective.♻️ Optional: route timeouts through the same error type
conn.setTimeout(5000, () => { - finish(new Error('Connection timed out')) + finish( + new UdsPeerConnectionError( + target.socketPath, + new Error('Connection timed out'), + ), + ) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/udsClient.ts` around lines 255 - 257, The timeout branch uses a plain Error; change it to reject with the same structured type as socket errors by creating and passing a UdsPeerConnectionError in the conn.setTimeout callback (the same error class used in the socket 'error' handler), including the socketPath and a descriptive message (e.g. 'Connection timed out') when calling finish so callers can reliably instanceof-check UdsPeerConnectionError; update the conn.setTimeout(...) => finish(...) invocation accordingly.src/services/AgentSummary/__tests__/agentSummary.test.ts (1)
133-137: Optional: tighten the unchanged-fingerprint assertion.The second
await scheduled!()validates thatforkCalls/updateCallsdon't grow, but doesn't assert observable behavior on the unchanged-fingerprint path (e.g., a debug log entry, or that no new error was logged / no extra reschedule occurred beyond the initial). Adding one such assertion would make this test fail-loud if the contract regresses (e.g., if the implementation later starts re-forking on identical fingerprints but produces the same summary).♻️ Suggested tightening
await scheduled!() expect(forkCalls).toHaveLength(1) expect(updateCalls).toHaveLength(1) + expect(loggedErrors).toEqual([]) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/AgentSummary/__tests__/agentSummary.test.ts` around lines 133 - 137, After the second await scheduled!() add an assertion that validates observable unchanged-fingerprint behavior beyond counts: for example, assert the mock logger (e.g., logger.debug or processLogger.debug used in this test) did not receive an extra entry or assert the rescheduler/mockReschedule function was not called; keep the existing checks on forkCalls and updateCalls and add one extra expect (referencing scheduled, forkCalls, updateCalls and the test's mock logger or reschedule mock) so the test fails if identical fingerprints trigger additional actions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/services/AgentSummary/__tests__/agentSummary.test.ts`:
- Around line 133-137: After the second await scheduled!() add an assertion that
validates observable unchanged-fingerprint behavior beyond counts: for example,
assert the mock logger (e.g., logger.debug or processLogger.debug used in this
test) did not receive an extra entry or assert the rescheduler/mockReschedule
function was not called; keep the existing checks on forkCalls and updateCalls
and add one extra expect (referencing scheduled, forkCalls, updateCalls and the
test's mock logger or reschedule mock) so the test fails if identical
fingerprints trigger additional actions.
In `@src/utils/udsClient.ts`:
- Around line 255-257: The timeout branch uses a plain Error; change it to
reject with the same structured type as socket errors by creating and passing a
UdsPeerConnectionError in the conn.setTimeout callback (the same error class
used in the socket 'error' handler), including the socketPath and a descriptive
message (e.g. 'Connection timed out') when calling finish so callers can
reliably instanceof-check UdsPeerConnectionError; update the
conn.setTimeout(...) => finish(...) invocation accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13a5f124-40c0-413f-96a3-5679d5dec938
📒 Files selected for processing (2)
src/services/AgentSummary/__tests__/agentSummary.test.tssrc/utils/udsClient.ts
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.
3 participants
Follow-up to #369, which has already been merged. This PR carries only the incremental Codecov repair on top of latest main: real AgentSummary lifecycle tests, mailbox fail-closed coverage with explicit EISDIR assertion, UDS client connection-failure coverage through a real capability file, and UDS response-reader framing coverage. Final diff intentionally contains no production-code churn, no warning suppression, no feature fallback, and no mock.module residue. Verification completed locally after the final cleanup: bunx tsc --noEmit --pretty false; bun run lint; targeted bun tests for AgentSummary/mailbox/UDS/SendMessage; bun run test:all; bun test --coverage --coverage-reporter lcov --coverage-dir coverage; bun run build; bun run build:vite; bun audit; git diff --check. Claude cross-validation: simplify GO at .omx/artifacts/claude-simplify-codecov-20260427-1521.md; security-review GO at .omx/artifacts/claude-security-codecov-20260427-1522.md.
Summary by CodeRabbit
Tests
Bug Fixes