src/utils/__tests__/teammateMailbox.test.ts (1)

345-360: Test asserts rejection but not the failure mode — consider tightening.

Pre‑creating a directory at inboxPath will cause writeToMailbox to throw, but the failure can originate from at least three different places (initial writeFile(..., { flag: 'wx' }) rejecting with EISDIR/EEXIST, proper-lockfile failing 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 stat at the top alongside the other node:fs/promises imports rather than dynamic-importing inline.)

Also worth noting: per src/utils/teammateMailbox.ts:372-385, only EEXIST is swallowed; on Linux, writeFile against an existing directory typically reports EISDIR, 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 loggedErrors and updateCalls. Per runSummary's finally block, after a thrown error the summarizer should call scheduleNext (which goes through the injected setTimeout and overwrites scheduled). To match the name and actually cover the rescheduling path, consider asserting that setTimeout was 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 scheduled reference 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: handle is only meaningfully used in the stop test.

Most tests assign handle = startTestSummarization(...) but never read it. You can drop the outer let handle declaration and the unused assignments, and just call startTestSummarization() 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 a case block.

The added const planApprovalDecision is declared directly inside case 'plan_approval_response': without braces. This is the pattern Biome's noSwitchDeclarations (and ESLint's no-case-declarations) flag, and it’s also inconsistent with the sibling shutdown_response case 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).

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/updateCalls and the debug log). Since runSummary calls scheduleNext() after the poor-mode early return, you can cheaply confirm the reschedule by capturing setTimeout invocations and asserting a new timer was registered after scheduled!() resolves — otherwise a future regression that drops scheduleNext() 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:

1. 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.

2. File permissions: The capability JSON is written via writeFile(...) with no mode option, so it lands with default permissions (typically 0o644). Currently, readUdsCapabilityToken does not validate file mode, so the test passes. However, if the code ever tightens to enforce 0o600 on capability files (as exists for directories), this test would silently start failing with a permissions error instead of the intended connect-failure path. Setting mode: 0o600 on 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.

src/utils/__tests__/teammateMailbox.test.ts (1)

345-363: Cross-platform note on EISDIR.

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 asserting error is truthy with a regex on the message.

Also, the inner callback parameter error shadows the outer const 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 with sha256 and 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 to udsClient.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 from udsClient.ts (or matching on a cause/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 verifies loggedErrors and updateCalls. The interesting behavior in agentSummary.ts's finally block (scheduleNext() after the catch) — i.e., that a new timer is registered post-error — is not exercised. Since setTimeout is mocked to overwrite scheduled, capturing the pre-error reference and asserting it was replaced (or simply that scheduled is 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.

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 fake setTimeout returns scheduledCount as the handle and stop() happens to fire after exactly one schedule. If the production path ever schedules an additional timer before stop() (e.g., a future warm-up tick), this expectation flips silently from "stop cleared the right handle" to "stop cleared handle #1 instead 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 scheduledCount increased by one — that proves a timer was scheduled, not that the rescheduled callback is the summarizer's own. Consider also asserting that scheduled was reassigned to a fresh function (e.g., capture the pre-call reference and assert scheduled !== 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 native Error.cause options bag to avoid shadowing the inherited property.

The explicit readonly cause field shadows Error.prototype.cause. Since your project targets ESNext and requires Bun ≥1.2.0 (which fully supports native Error.cause), you can remove the field and pass { cause } to the super() 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.

src/utils/udsClient.ts (1)

255-257: Consider also wrapping the connection-timeout path in UdsPeerConnectionError.

The socket-error path (Line 252-253) now rejects with UdsPeerConnectionError, but the setTimeout branch at Line 256 still rejects with a generic Error('Connection timed out'). Callers that branch on instanceof UdsPeerConnectionError (and read socketPath) 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 that forkCalls/updateCalls don'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.