src/services/AgentSummary/__tests__/summaryContext.test.ts (1)

103-132: Consider asserting the empty-input contract for getSummaryContextFingerprint.

agentSummary.ts relies on getSummaryContextFingerprint(summaryContext) returning null for empty bounded contexts (the transcriptFingerprint && … guard). A small test pinning that contract would prevent a regression where, e.g., switching to '' would silently make every empty tick "match" the previously stored fingerprint of ''.

Suggested addition

   test('changes when message content changes under the same uuid', () => {
     ...
   })
+
+  test('returns null for an empty transcript', () => {
+    expect(getSummaryContextFingerprint([])).toBeNull()
+  })
 })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/AgentSummary/__tests__/summaryContext.test.ts` around lines 103
- 132, Add a test asserting the empty-input contract for
getSummaryContextFingerprint: call getSummaryContextFingerprint with an
empty/zero-length bounded context (e.g., [] or the minimal empty summaryContext
shape used by agentSummary.ts) and assert it returns null; this ensures
agentSummary.ts's guard (transcriptFingerprint && …) keeps working and prevents
regressions where an empty fingerprint becomes '' or another non-null value.
Locate the tests in summaryContext.test.ts and add the assertion alongside the
existing cases referencing getSummaryContextFingerprint so it runs with the
other fingerprint tests.

src/services/AgentSummary/summaryContext.ts (1)

55-62: Truncated string contributions can mask changes to the tail of a large value.

hash.update(text.slice(0, limit.remaining)) only feeds the first remaining chars of a large primitive into the digest, while limit.remaining -= text.length accounts for the full length. If a single string value at the very edge of the budget changes only past the truncation point, the fingerprint won't reflect it (the rest of the walk is also short-circuited because remaining is already ≤ 0). With MAX_SUMMARY_CONTEXT_CHARS = 200_000 and lastMessage.uuid baked into the returned key this is unlikely to matter in practice — calling it out as a minor robustness consideration only.

Possible tweak to deduct only what was hashed (preserves total walk continuation behavior for callers)

-  if (value === null || typeof value !== 'object') {
-    const text = String(value)
-    hash.update(typeof value)
-    hash.update(':')
-    hash.update(text.slice(0, limit.remaining))
-    limit.remaining -= text.length
-    return
-  }
+  if (value === null || typeof value !== 'object') {
+    const text = String(value)
+    const consumed = Math.min(text.length, limit.remaining)
+    hash.update(typeof value)
+    hash.update(':')
+    hash.update(text.slice(0, consumed))
+    // Mark whether the value was truncated so siblings can't collide by content alone.
+    if (consumed < text.length) hash.update(`#${text.length}`)
+    limit.remaining -= text.length
+    return
+  }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/AgentSummary/summaryContext.ts` around lines 55 - 62, The code
truncates large primitive strings but subtracts the full text length from
limit.remaining, which hides tail changes; in the block handling primitives
(check for value === null || typeof value !== 'object'), compute the actual
hashed length first (e.g., sliceLen = Math.min(text.length, limit.remaining)),
call hash.update(text.slice(0, sliceLen)), then decrement limit.remaining by
sliceLen (not text.length), and bail early if sliceLen is 0; refer to the
variables/identifiers limit.remaining, value, text, and hash.update to locate
and fix the logic.

src/utils/teammateMailbox.ts (1)

152-165: Clean up the temp file when atomic write fails.

If writeFile partially writes or rename fails (e.g., EXDEV on a different filesystem, ENOSPC, EACCES on the target directory), the .tmp file is leaked. Over time these can accumulate next to active inboxes and confuse cleanup tooling. Wrap the rename in try/catch and unlink the temp on failure.

♻️ Suggested fix

-  const tempPath = `${inboxPath}.${process.pid}.${randomBytes(8).toString('hex')}.tmp`
-  await writeFile(tempPath, content, 'utf-8')
-  await rename(tempPath, inboxPath)
+  const tempPath = `${inboxPath}.${process.pid}.${randomBytes(8).toString('hex')}.tmp`
+  try {
+    await writeFile(tempPath, content, 'utf-8')
+    await rename(tempPath, inboxPath)
+  } catch (error) {
+    await unlink(tempPath).catch(() => {})
+    throw error
+  }

Add the import:

-import { mkdir, readFile, rename, stat, writeFile } from 'fs/promises'
+import { mkdir, readFile, rename, stat, unlink, writeFile } from 'fs/promises'

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/teammateMailbox.ts` around lines 152 - 165, The writeMailboxAtomic
function can leak the temp file if writeFile/rename fails; update
writeMailboxAtomic to wrap the rename (and the write+rename sequence) in a
try/catch and, on any failure after creating tempPath, attempt to
unlink(tempPath) (using the existing fs promises API or importing unlink) before
rethrowing the original error; reference the tempPath variable and the
rename/writeFile calls and ensure the unlink call itself is swallowed or logged
so cleanup failures don't mask the original error.

packages/builtin-tools/src/tools/SendMessageTool/__tests__/udsRecipientSanitization.test.ts (1)

3-5: Use the shared bun:bundle mock instead of redefining it inline.

This adds another ad-hoc feature-flag mock surface. Please wire this test through the shared mock in tests/mocks/ so bun:bundle behavior stays consistent across suites.

Based on learnings: Applies to /tests//*.test.ts : Mock only modules with side effects (dependencies with I/O, state, or network access). Do not mock pure functions or pure data modules. Use shared mocks from tests/mocks/ for log.ts, debug.ts, bun:bundle, settings.js, config.ts, and auth.ts

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@packages/builtin-tools/src/tools/SendMessageTool/__tests__/udsRecipientSanitization.test.ts`
around lines 3 - 5, The test currently redefines the bun:bundle feature-flag
inline via mock.module('bun:bundle', ...) which creates an ad-hoc mock; remove
that inline mock and instead wire the test to use the shared bun:bundle mock
from the tests/mocks/ directory so feature flags remain consistent (ensure the
test imports or invokes the shared mock setup before running and configures the
UDS_INBOX flag via that shared mock); update the test to rely on the shared mock
entry (rather than mock.module) and remove any local feature-flag logic so other
suites use the same bun:bundle behavior.

src/utils/udsMessaging.ts (1)

311-318: Use a constant-time comparison for the auth token.

getMessageAuthToken(message) === authToken short-circuits on first differing byte. The local-UDS attack surface limits the practical impact (the capability file is already 0o600 and the comparison runs in the recipient's own process), but a constant-time check is trivial here and avoids the question entirely.

🛡️ Suggested fix using crypto.timingSafeEqual

 function isAuthorizedMessage(message: UdsMessage): boolean {
-  return getMessageAuthToken(message) === authToken
+  const provided = getMessageAuthToken(message)
+  if (!provided || !authToken) return false
+  const a = Buffer.from(provided, 'utf8')
+  const b = Buffer.from(authToken, 'utf8')
+  if (a.length !== b.length) return false
+  return timingSafeEqual(a, b)
 }

Add timingSafeEqual to the existing crypto import on line 11.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/udsMessaging.ts` around lines 311 - 318, Replace the direct
equality check in isAuthorizedMessage by performing a constant-time comparison
using crypto.timingSafeEqual: import timingSafeEqual (or crypto) into the
module, retrieve the token via getMessageAuthToken(message), and if both token
and authToken are defined and have the same byte length, compare
Buffer.from(token) and Buffer.from(authToken) with timingSafeEqual; return false
for missing tokens or length mismatch. Ensure you reference getMessageAuthToken
and isAuthorizedMessage when making the change.

src/utils/udsClient.ts (1)

46-50: Consider extracting a shared NDJSON response reader.

getChunkBytes is duplicated verbatim in src/utils/udsMessaging.ts (line 565), and the read/settle/parse loop in sendToUdsSocket (lines 220-275) is nearly identical to the one in sendUdsMessage (lines 594-639 of udsMessaging.ts): same buffer/settled/finish pattern, same per-frame byte cap, same NDJSON splitting, and same response/error/pong settling rules. Extracting a helper would prevent the two implementations from drifting on future changes (e.g., when the framing or termination rules evolve).

Also applies to: 220-275

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/udsClient.ts` around lines 46 - 50, The duplicated NDJSON
response-reading logic (including getChunkBytes) across getChunkBytes,
sendToUdsSocket, and sendUdsMessage should be extracted into a shared helper to
avoid drift; create a new function (e.g., readNdjsonResponses or
parseNdjsonStream) that encapsulates the buffer/settled/finish loop, per-frame
byte cap, NDJSON splitting, and the response/error/pong settling rules, replace
the duplicated implementations in sendToUdsSocket and sendUdsMessage to call
this helper, and keep the existing getChunkBytes either moved into the helper or
exported for reuse so both modules use the same utility.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

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

58-71: Hooks are file-scoped and unnecessarily applied to getLastPeerDmSummary.

beforeEach/afterEach are declared at the file level, so the temp directory creation and CLAUDE_CONFIG_DIR mutation also run for every getLastPeerDmSummary test even though those tests don't touch the filesystem. Consider scoping these hooks inside the two describe blocks that need them, to keep tests independent and slightly faster.

🤖 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 58 - 71, The
file-level beforeEach/afterEach that create tempHome and mutate
process.env.CLAUDE_CONFIG_DIR are running for all tests (including ones for
getLastPeerDmSummary) and should be moved into the two describe blocks that
actually need filesystem setup; locate the beforeEach/afterEach functions that
reference previousConfigDir, tempHome, mkdtempSync,
process.env.CLAUDE_CONFIG_DIR and rm and cut them out of the file scope and
paste them inside the specific describe(...) blocks that exercise filesystem
behavior, leaving tests for getLastPeerDmSummary unaffected.

---

1-22: Consider using the src/* path alias for imports.

Per coding guidelines, the src/* path alias should be preferred over deep relative imports. This is consistent with the rest of the codebase and easier to refactor.

♻️ Proposed change

-import type { Message } from '../../types/message.js'
+import type { Message } from 'src/types/message'
 import {
   compactMailboxMessages,
   getLastPeerDmSummary,
   ...
-} from '../teammateMailbox.js'
+} from 'src/utils/teammateMailbox'

As per coding guidelines: "Path alias src/* is available via tsconfig and maps to ./src/*. Use this for imports throughout the codebase".

🤖 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 1 - 22, Replace
deep relative imports in this test with the src/* path alias: update imports for
types (Message), all functions and types from ../teammateMailbox.js
(compactMailboxMessages, getLastPeerDmSummary, getInboxPath,
markMessageAsReadByIndex, markMessageAsReadByIdentity, markMessagesAsRead,
markMessagesAsReadByPredicate, MAX_MAILBOX_MESSAGE_TEXT_BYTES,
MAX_MAILBOX_MESSAGES, MAX_READ_MAILBOX_MESSAGES,
MAX_UNREAD_PROTOCOL_MAILBOX_MESSAGES, readMailbox, TeammateMessage,
writeToMailbox) and any node:fs/node:path/node:os imports to use the project
alias where appropriate (e.g., import from "src/utils/teammateMailbox" or
"src/types/message" instead of relative paths) so the file follows the src/*
tsconfig path-alias convention.

src/services/AgentSummary/__tests__/agentSummary.test.ts (3)

83-85: Simplify the scheduled type — TimerHandler & (...) => void is meaningless.

TimerHandler is string | Function, so intersecting it with a function type doesn’t add useful constraints and makes Parameters<...> resolve in a confusing way. The captured callback is always invoked with no args (Line 119, 126), so a simple signature is clearer and equivalent.

♻️ Proposed simplification

-  let scheduled:
-    | ((...args: Parameters<TimerHandler & ((...args: unknown[]) => void)>) => void)
-    | undefined
+  let scheduled: ((...args: unknown[]) => void | Promise<void>) | undefined

🤖 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 83 -
85, The variable `scheduled` currently uses an overcomplicated type involving
`Parameters<TimerHandler & ((...args: unknown[]) => void)>`; replace its
declaration with a simple function signature `(() => void) | undefined` (i.e.,
make `scheduled` type `() => void` or `undefined`) and update any places that
assign or call it (the setTimeout/mock callback assignments and invocations
around `scheduled` at lines where `scheduled` is assigned and invoked) to use
zero-argument calls (e.g., `scheduled && scheduled()` or optional chaining
`scheduled?.()`). This removes the meaningless intersection with `TimerHandler`
and clarifies that the captured callback takes no arguments.

---

43-48: Avoid mocking the pure filterIncompleteToolCalls helper.

This factory replaces filterIncompleteToolCalls with an identity function. Per the test guidelines, only modules with side effects (I/O, state, network) should be mocked — not pure helpers. The existing transcriptMessages are already “clean” (no orphan tool calls), so the real implementation would behave identically and would also catch regressions if the helper’s contract changes.

Consider dropping this mock and letting the real implementation run.

As per coding guidelines: "Mock only modules with side effects (dependencies with I/O, state, or network access). Do not mock pure functions or pure data modules."

🤖 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 43 -
48, The test currently replaces the pure helper filterIncompleteToolCalls with
an identity function via
mock.module('@claude-code-best/builtin-tools/tools/AgentTool/runAgent.js'),
which hides regressions; remove that mock so the real filterIncompleteToolCalls
implementation runs for agentSummary tests (delete or comment out the
mock.module block that returns filterIncompleteToolCalls: <T>(messages: T) =>
messages) and re-run the tests—this keeps the pure helper unmocked while still
allowing you to mock only side‑effecting modules.

---

29-31: Replace require() with ESM import for the sessionStorage snapshot and restoration.

The test file uses proper ESM imports throughout (lines 1–13), but lines 29–31 and 74–78 use require('../../../utils/sessionStorage.ts'), which bypasses Bun's ESM resolution and relies on CommonJS interop. Additionally, there's a path inconsistency: require() uses .ts while mock.module() expects .js.

Replace with:

import * as sessionStorageModule from '../../../utils/sessionStorage.js'

const sessionStorageSnapshot = {
  ...sessionStorageModule,
}

Then update the afterAll factory (lines 74–78) to:

afterAll(() => {
  mock.module('src/utils/sessionStorage.js', () => sessionStorageModule)
})

This aligns with the coding guideline: "All imports, builds, and execution must use Bun APIs, not Node.js APIs. Module system must be ESM."

🤖 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 29 -
31, Replace the CommonJS require call that builds sessionStorageSnapshot with an
ESM import: import the sessionStorage module as sessionStorageModule from
'../../../utils/sessionStorage.js' and spread that into sessionStorageSnapshot
(replace the require('../../../utils/sessionStorage.ts') usage). Then update the
afterAll factory that registers the mock to call mock.module with the '.js' path
('src/utils/sessionStorage.js') and return the sessionStorageModule so the mock
restores the ESM module correctly; update references to the
sessionStorageSnapshot and afterAll factory accordingly.

src/services/AgentSummary/summaryContext.ts (1)

1-1: Use the node: prefix for built-in modules to align with Bun best practices.

Bun's documentation recommends the node: prefix for built-in modules to ensure reliable resolution in strict ESM contexts and avoid potential conflicts with userland packages. While the codebase is currently inconsistent (mostly using 'crypto', but increasingly preferring 'node:crypto' in packages), adopting the prefix improves clarity and future compatibility.

♻️ Proposed change

-import { createHash } from 'crypto'
+import { createHash } from 'node:crypto'

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/AgentSummary/summaryContext.ts` at line 1, Replace the built-in
module import to use the node: prefix so Bun resolves it reliably: update the
import that currently pulls createHash (symbol createHash) from 'crypto' to
import createHash from 'node:crypto' across this module (summaryContext.ts) so
the built-in crypto module uses the node: namespace consistently with other
packages.

packages/builtin-tools/src/tools/SendMessageTool/SendMessageTool.ts (2)

858-890: Duplicate inline-token rejection inside the UDS branch is unreachable.

The earlier guard at Lines 809-823 already returns before this branch is entered for any plain-text + uds: + inline-token combination, and validateInput rejects the same case upstream. The check on Lines 860-868 is dead defensive code; consider removing it or, if you prefer to keep the defense-in-depth, drop the earlier 809-823 block and consolidate here so reviewers can see one authoritative gate.

🤖 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 858 - 890, The duplicate inline-token rejection inside the uds branch is
unreachable because validateInput and the earlier guard already return for
plain-text uds addresses with inline tokens; remove the inner defensive check
that calls hasInlineUdsToken and wasInlineUdsTokenRejected (the if-block that
returns the "uds addresses must not include inline auth tokens" error) from the
SendMessageTool uds handling, leaving the recipientForDisplay,
require('src/utils/udsClient.js') import, sendToUdsSocket call, and try/catch
intact; alternatively, if you prefer one authoritative gate, delete the earlier
guard in validateInput and keep this check here — but do not keep both.

---

593-604: Redundant recipientForDisplay after markAndRedactInlineUdsToken.

By Line 593/604, input.to has already been redacted at Line 583-585 (markAndRedactInlineUdsToken mutates input.to), so recipientForDisplay(input.to) is effectively input.to. Either drop the redundant call or skip the in-place mutation in markAndRedactInlineUdsToken — currently both layers run and the intent (which one is authoritative) is muddled.

🤖 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 593 - 604, The code calls recipientForDisplay(input.to) after
markAndRedactInlineUdsToken has already mutated input.to, making the
recipientForDisplay call redundant; either remove the redundant
recipientForDisplay call in SendMessageTool (the lines setting input.recipient =
recipientForDisplay(input.to)) or change markAndRedactInlineUdsToken to return a
redacted value instead of mutating input.to and then use that return value when
setting input.recipient — pick one approach and update usages (references:
recipientForDisplay, markAndRedactInlineUdsToken, input.to, input.recipient) so
only one authoritative redaction/display conversion is performed.

src/utils/udsResponseReader.ts (1)

45-54: Frame-size check uses O(n) buffer measurement per chunk.

Buffer.byteLength(buffer, 'utf8') rescans the entire pending buffer on every 'data' event. With many small chunks this is O(n²) over the response stream. Since maxFrameBytes is bounded at 64 KiB this is not a real problem, but a tracked counter (bufferBytes += getChunkBytes(chunk), decremented when lines are extracted) would be cheaper and easier to reason about.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/udsResponseReader.ts` around lines 45 - 54, Replace the repeated
full-buffer size calls with a tracked byte counter: introduce a variable (e.g.,
bufferBytes) in the scope of the socket data handler, increment it by
getChunkBytes(chunk) when a chunk arrives, use that counter to compare against
options.maxFrameBytes and call finish(new Error('UDS response frame exceeded
size limit')) if exceeded, and when you remove/extract lines from buffer
decrement bufferBytes by the number of bytes removed; update logic around
socket.on('data', chunk => { ... }) and any line-extraction code so the counter
stays consistent (referencing bufferBytes, getChunkBytes, options.maxFrameBytes
and finish).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/builtin-tools/src/tools/AgentTool/filterIncompleteToolCalls.ts (1)

17-17: Remove redundant type assertions; type narrowing already applies.

Since Message is a discriminated union on type (from @ant/model-provider), the checks message?.type === 'user' and message?.type === 'assistant' automatically narrow the type to UserMessage and AssistantMessage respectively. The as UserMessage and as AssistantMessage assertions are redundant and can be removed, simplifying the code to align with the guideline: "Use type guards (type narrowing) with union types instead of forced type casting".

♻️ Proposed simplification

   for (const message of messages) {
     if (message?.type === 'user') {
-      const userMessage = message as UserMessage
-      const content = userMessage.message.content
+      const content = message.message.content
       if (Array.isArray(content)) {

   for (const message of messages) {
     if (message?.type === 'assistant') {
-      const assistantMessage = message as AssistantMessage
-      const content = assistantMessage.message.content
+      const content = message.message.content
       if (Array.isArray(content)) {
         let changed = false
@@
         if (filteredContent.length > 0) {
           withoutOrphanToolUses.push({
-            ...assistantMessage,
+            ...message,
             message: {
-              ...assistantMessage.message,
+              ...message.message,
               content: filteredContent,
             },
           })

     if (message?.type !== 'user') {
       filteredMessages.push(message)
       continue
     }
-    const userMessage = message as UserMessage
-    const content = userMessage.message.content
+    const content = message.message.content
     if (!Array.isArray(content)) {
@@
     if (filteredContent.length > 0) {
       filteredMessages.push({
-        ...userMessage,
+        ...message,
         message: {
-          ...userMessage.message,
+          ...message.message,
           content: filteredContent,
         },
       })
     }

Applies to lines 17, 34, and 77.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/builtin-tools/src/tools/AgentTool/filterIncompleteToolCalls.ts` at
line 17, Remove the unnecessary type assertions in filterIncompleteToolCalls.ts:
where the code uses "const userMessage = message as UserMessage" and similar "as
AssistantMessage" casts (occurring around the variables assigned at the checks
for message?.type === 'user' and message?.type === 'assistant'), rely on the
discriminated-union type narrowing instead and change those assignments to use
the already-narrowed type (e.g., const userMessage = message) so you eliminate
the redundant casts in the function(s) handling message type checks.

src/commands/poor/__tests__/poorMode.test.ts (2)

16-45: Extract the expanded settings mock into tests/mocks/settings.ts instead of inlining a full-surface mock.

This change grows the inline mock from a few functions to ~16 exports — exactly the situation the shared-mocks guideline is meant to prevent. The author's own comment on lines 34–35 hints at the leakage risk that motivates the rule. Putting the surface in tests/mocks/settings.ts keeps it in one place, lets every settings consumer test reuse it, and avoids drift each time settings.ts adds an export.

As per coding guidelines: "Use shared mocks from tests/mocks/ for log.ts, debug.ts, bun:bundle, settings.js, config.ts, and auth.ts" and "Never inline mock definitions; update shared mock files instead".

♻️ Suggested shape

tests/mocks/settings.ts (new or extended) exports a factory:

// tests/mocks/settings.ts
export function makeSettingsMock(initial: Record<string, unknown> = {}) {
  let state = initial
  let lastUpdate: { source: string; patch: Record<string, unknown> } | null = null
  return {
    state: () => state,
    setState: (s: Record<string, unknown>) => { state = s },
    lastUpdate: () => lastUpdate,
    resetLastUpdate: () => { lastUpdate = null },
    exports: {
      loadManagedFileSettings: () => ({ settings: null, errors: [] }),
      // ...full surface...
      getInitialSettings: () => state,
      updateSettingsForSource: (source: string, patch: Record<string, unknown>) => {
        lastUpdate = { source, patch }
        state = { ...state, ...patch }
      },
    },
  }
}

Then in this test file:

-mock.module('src/utils/settings/settings.js', () => ({
-  loadManagedFileSettings: () => ({ settings: null, errors: [] }),
-  ...
-}))
+import { makeSettingsMock } from '../../../../tests/mocks/settings'
+const settingsMock = makeSettingsMock()
+mock.module('src/utils/settings/settings.js', () => settingsMock.exports)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/poor/__tests__/poorMode.test.ts` around lines 16 - 45, Extract
the inline full-surface settings mock into a shared factory in
tests/mocks/settings.ts (e.g. export function makeSettingsMock(initial = {}) {
... }) that maintains internal state and lastUpdate and returns an object with
.exports containing the same exported functions used here
(loadManagedFileSettings, getManagedFileSettingsPresence, parseSettingsFile,
getInitialSettings, getSettingsForSource, getSettingsWithErrors,
getSettingsWithSources, getSettings_DEPRECATED, settingsMergeCustomizer,
getManagedSettingsKeysForLogging, hasAutoModeOptIn,
hasSkipDangerousModePermissionPrompt, getAutoModeConfig,
getUseAutoModeDuringPlan, rawSettingsContainsKey, updateSettingsForSource,
getSettingsRootPathForSource, getSettingsFilePathForSource,
getRelativeSettingsFilePathForSource, getPolicySettingsOrigin). Then update this
test (poorMode.test.ts) to import makeSettingsMock(), call it to get the mock
instance, and pass mockInstance.exports into mock.module instead of the large
inline object; ensure the factory exposes helpers (state, setState, lastUpdate,
resetLastUpdate) if the tests rely on reading/updating mockSettings.

---

47-50: Re-mocking with the real namespace after mock.restore() is the right guard against leakage into sibling suites. However, import * as settingsModule returns a frozen module namespace; passing it directly as the factory result works in Bun today but will throw if any consumer attempts to mutate exports. When moving this to the recommended shared tests/mocks/settings.ts factory, use a plain object spread (() => ({ ...settingsModule })) to avoid that footgun. Additionally, per the coding guidelines, all tests mocking settings should consolidate to a single shared mock in tests/mocks/settings.ts rather than maintaining divergent inline mocks across providers.test.ts, effort.test.ts, and others—this eliminates the leakage risk entirely.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/poor/__tests__/poorMode.test.ts` around lines 47 - 50, The test
re-mocks the real settings namespace after mock.restore() using the frozen
imported namespace (settingsModule), which can throw if consumers mutate
exports; change the factory passed to
mock.module('src/utils/settings/settings.js', ...) to return a plain mutable
object copy instead of the frozen namespace (e.g., use a factory that spreads
settingsModule into a new object), and move/consolidate all inline settings
mocks from this and other tests (providers.test.ts, effort.test.ts, etc.) into
the shared tests/mocks/settings.ts mock factory so every test uses the same
mutable mock implementation and avoids leakage.

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

126-143: Tighten the req-1 substring assertion to avoid masking off-by-one regressions.

'req-1' is also a substring of 'req-10', 'req-100', 'req-1000', etc., so this assertion would still pass if compaction kept [req-10..req-2000] instead of the intended [req-1..req-2000]. Match against the JSON-quoted form (or check both bookends) so the cap boundary is genuinely exercised.

Proposed refinement

     expect(compacted).toHaveLength(MAX_UNREAD_PROTOCOL_MAILBOX_MESSAGES)
-    expect(compacted[0]?.text).toContain('req-1')
+    expect(compacted[0]?.text).toContain('"req-1"')
+    expect(compacted.at(-1)?.text).toContain(
+      `"req-${MAX_UNREAD_PROTOCOL_MAILBOX_MESSAGES}"`,
+    )

🤖 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 126 - 143, The test
for compactMailboxMessages is using a loose substring 'req-1' which can match
'req-10' etc.; update the assertion to assert the exact JSON-quoted request_id
(e.g. expect(compacted[0]?.text).toContain('"request_id":"req-1"') or otherwise
check both the first and last items to verify the true cap boundary) so the cap
at MAX_UNREAD_PROTOCOL_MAILBOX_MESSAGES is exercised; locate the test that
constructs messages via message(...) and the compactMailboxMessages(...) call
and tighten the assertion accordingly.

src/utils/udsMessaging.ts (3)

81-86: Exported limits should be documented as part of the public surface.

MAX_UDS_INBOX_ENTRIES, MAX_UDS_FRAME_BYTES, MAX_UDS_INBOX_BYTES, MAX_UDS_CLIENTS, UDS_AUTH_TIMEOUT_MS, UDS_IDLE_TIMEOUT_MS are now exported and consumed by the test suite (and by udsClient/udsResponseReader indirectly via MAX_UDS_FRAME_BYTES). Adding a one-line JSDoc to each — particularly the rationale for the byte/entry caps — would make future tuning safer; right now the only documentation is the bare numeric literal.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/udsMessaging.ts` around lines 81 - 86, Add one-line JSDoc comments
above each exported constant (MAX_UDS_INBOX_ENTRIES, MAX_UDS_FRAME_BYTES,
MAX_UDS_INBOX_BYTES, MAX_UDS_CLIENTS, UDS_AUTH_TIMEOUT_MS, UDS_IDLE_TIMEOUT_MS)
describing what the limit controls and the rationale (e.g., memory/throughput
protection, preventing oversized frames, expected client concurrency, or
auth/idle timeout behavior) so consumers and tests understand why the numeric
value was chosen; keep each JSDoc short (one sentence) and include units for
time/bytes where applicable to make future tuning safe and self-documenting.

---

211-228: Redundant pre-check duplicates assertPrivateDirectory logic.

Lines 215-219 already check !stat.isDirectory() || stat.isSymbolicLink() and throw, but the very next line calls assertPrivateDirectory (Line 175) which performs the same check with a slightly different message. The pre-check is dead code; just call assertPrivateDirectory directly.

♻️ Optional cleanup

   try {
     const stat = await lstat(dir)
-    if (!stat.isDirectory() || stat.isSymbolicLink()) {
-      throw new Error(
-        `[udsMessaging] socket parent is not a directory: ${dir}`,
-      )
-    }
     assertPrivateDirectory(stat, dir, 'socket parent')
     return
   } catch (error) {
     if (!isNotFound(error)) throw error
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/udsMessaging.ts` around lines 211 - 228, In
ensureSocketParent(path) remove the redundant pre-check that inspects
stat.isDirectory() and stat.isSymbolicLink() and throws, and instead call
assertPrivateDirectory(stat, dir, 'socket parent') directly after obtaining stat
from lstat; keep the existing try/catch around lstat and the subsequent
mkdir/chmod behavior unchanged so the same validation and error messages come
from assertPrivateDirectory rather than the duplicated manual check.

---

299-316: inbox full error message conflates three distinct rejection reasons.

enqueueInboxEntry returns false for (a) per-message frame oversize, (b) entry-count overflow, and (c) byte-budget overflow, but logs all three as inbox full. The framer should already reject (a) before this path, so it's mostly defensive — yet operators reading the log can't distinguish "we're being flooded" from "a giant single message slipped past the framer cap" (which would indicate a bug). Splitting the log message would make production triage cheaper without changing behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/udsMessaging.ts` around lines 299 - 316, The log in
enqueueInboxEntry currently lumps three different rejection causes into one
"inbox full" error; update enqueueInboxEntry to detect which condition failed
(entryBytes > MAX_UDS_FRAME_BYTES, inbox.length >= MAX_UDS_INBOX_ENTRIES, or
inboxBytes + entryBytes > MAX_UDS_INBOX_BYTES) and log a distinct, descriptive
message for each case (e.g., "message too large (exceeds MAX_UDS_FRAME_BYTES)",
"inbox entry count limit reached", or "inbox byte quota exceeded") while keeping
the same boolean return behavior; use the existing variables (entryBytes,
MAX_UDS_FRAME_BYTES, inbox.length, MAX_UDS_INBOX_ENTRIES, inboxBytes,
MAX_UDS_INBOX_BYTES) and the same logError call site so only the message string
changes.

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

137-149: Sentinel error path swallows the diagnostic message.

If MAX_UDS_INBOX_BYTES enforcement ever regresses, the inner throw new Error('byte cap was not enforced') is caught by the same catch and tested against 'inbox full', so the failure surfaces as a confusing assertion failure on the wrong message rather than the intended diagnostic. Re-throwing the sentinel out of the try (or asserting outside the loop) would make the regression mode obvious.

♻️ Optional restructure

-    for (;;) {
-      try {
-        await sendUdsMessage(path, { type: 'text', data: payload })
-        accepted++
-        if (accepted > MAX_UDS_INBOX_BYTES / payload.length + 20) {
-          throw new Error('byte cap was not enforced')
-        }
-      } catch (error) {
-        expect(error).toBeInstanceOf(Error)
-        expect((error as Error).message).toContain('inbox full')
-        break
-      }
-    }
+    const limit = MAX_UDS_INBOX_BYTES / payload.length + 20
+    let lastError: Error | undefined
+    while (accepted <= limit) {
+      try {
+        await sendUdsMessage(path, { type: 'text', data: payload })
+        accepted++
+      } catch (error) {
+        lastError = error as Error
+        break
+      }
+    }
+    expect(lastError).toBeInstanceOf(Error)
+    expect(lastError?.message).toContain('inbox full')

🤖 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 137 - 149, The test
loop around sendUdsMessage and MAX_UDS_INBOX_BYTES currently throws a sentinel
Error('byte cap was not enforced') inside the try which is then caught and
asserted as an 'inbox full' error; change the flow so the sentinel cannot be
mistaken for the expected inbox-full error: either move the assertion that the
cap was exceeded outside the try/catch (count accepted and after the loop assert
accepted did not exceed the threshold) or throw a distinct Error subclass or
unique sentinel that you re-throw from the catch when it originates from your
own check; update the loop in udsMessaging.test.ts referencing sendUdsMessage
and MAX_UDS_INBOX_BYTES so only genuine inbox-full errors are asserted inside
the catch and the sentinel failure surfaces separately.

packages/builtin-tools/src/tools/SendMessageTool/SendMessageTool.ts (1)

671-681: Redundant scheme check.

hasInlineUdsToken already calls parseAddress and gates on addr.scheme === 'uds', so the outer addr.scheme === 'uds' && is dead-code-ish. Also worth noting the third parseAddress(input.to) invocation in this function (Line 658, this block, and Line 690) — three parses of the same string in a single validation call.

♻️ Optional simplification

-      if (
-        addr.scheme === 'uds' &&
-        hasInlineUdsToken(input.to)
-      ) {
+      if (hasInlineUdsToken(input.to)) {
         return {
           result: false,
           message:
             'uds addresses must not include inline auth tokens; use the ListPeers address',
           errorCode: 9,
         }
       }

🤖 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 671 - 681, The redundant outer scheme check should be removed because
hasInlineUdsToken already calls parseAddress and checks addr.scheme; instead
call hasInlineUdsToken(input.to) directly and rely on its internal parseAddress
logic, and avoid multiple parseAddress calls by parsing input.to once (reuse the
resulting addr variable) or by using the existing addr from parseAddress where
present; update the validation in SendMessageTool (the block referencing addr,
hasInlineUdsToken, and parseAddress) to use the single parsed address or the
direct hasInlineUdsToken call to eliminate the third parseAddress invocation.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

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

126-143: Tighten the protocol-cap assertion to avoid prefix collisions.

toContain('req-1') is also satisfied by req-10, req-11, …, so this assertion does not actually prove that the dropped element is req-0 (i.e. the oldest). With MAX_UNREAD_PROTOCOL_MAILBOX_MESSAGES + 1 inputs you typically generate req-0…req-N where N >= 10, so the substring match is ambiguous for any cap ≥ 11.

♻️ Proposed stricter assertion

-    expect(compacted).toHaveLength(MAX_UNREAD_PROTOCOL_MAILBOX_MESSAGES)
-    expect(compacted[0]?.text).toContain('req-1')
+    expect(compacted).toHaveLength(MAX_UNREAD_PROTOCOL_MAILBOX_MESSAGES)
+    const firstId = JSON.parse(compacted[0]?.text ?? '{}').request_id
+    expect(firstId).toBe('req-1')
+    expect(compacted.some(m => m.text.includes('"request_id":"req-0"'))).toBe(
+      false,
+    )

🤖 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 126 - 143, The
assertion is too loose because expect(compacted[0]?.text).toContain('req-1') can
match 'req-10' etc.; update the test in the "caps unread protocol messages with
an independent bound" case to assert the exact dropped item by parsing or
exact-matching the protocol payload: use compactMailboxMessages, the generated
messages via message and MAX_UNREAD_PROTOCOL_MAILBOX_MESSAGES to build
req-0…req-N, then decode or parse compacted[0].text (or use a strict string
equality / regex with word-boundaries) and assert the request_id equals "req-1"
(or that the serialized JSON contains `"request_id":"req-1"`), ensuring the test
verifies the precise ID rather than a substring.

src/services/AgentSummary/__tests__/agentSummary.test.ts (1)

84-92: Consider properly restoring all mock.module() registrations; Bun's mock.restore() does not reset them.

mock.restore() does not reset modules registered via mock.module(), per Bun documentation. This leaves poorMode.js, LocalAgentTask.js, and forkedAgent.js mocked for the remainder of the in-process test runtime. While no other tests in this suite currently import these modules (verified via codebase search), proper cleanup via explicit mock restoration (e.g., mock.restoreModule() or re-importing the real modules) is better practice and protects against future test additions. Alternatively, move these mocks to per-test setup via beforeEach() to scope their lifetime.

🤖 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 84 -
92, The afterEach cleanup currently calls mock.restore() but leaves mock.module
registrations in place; update the afterEach in the test to explicitly undo
those module mocks (e.g., call mock.restoreModule() for each mocked path or
re-import the real modules) for the modules you registered earlier (poorMode.js,
LocalAgentTask.js, forkedAgent.js and the sessionStorage.js registration), or
move the mock.module(...) calls into beforeEach so their scope is per-test;
ensure you reference the existing afterEach block and the mock.module(...) calls
when making the change.

src/utils/ndjsonFramer.ts (1)

86-98: ⚠️ Potential issue | 🟠 Major

Multibyte UTF-8 sequences are still split at chunk boundaries on the tail path.

A previous review flagged this and was marked "Addressed in commit c9ddecc", but line 96 still calls chunk.subarray(start).toString('utf8') and the file does not import StringDecoder. When a chunk ends mid-multibyte-sequence, the trailing bytes are decoded to U+FFFD and the next chunk's leading continuation bytes decode to another U+FFFD. The corrupted buffer then fails JSON.parse and triggers a spurious invalid-frame rejection (or, with destroyOnInvalidFrame: true, kills the socket). Line 79 is safe because LF (0x0A) cannot appear inside a multibyte sequence, so only the tail accumulation needs the fix. udsResponseReader.ts already uses StringDecoder('utf8') for exactly this reason — adopt the same pattern here.

♻️ Suggested fix using `StringDecoder`

 import type { Socket } from 'net'
+import { StringDecoder } from 'node:string_decoder'
@@
 export function attachNdjsonFramer<T = unknown>(
   socket: Socket,
   onMessage: (msg: T) => void,
   parse: (text: string) => T = text => JSON.parse(text) as T,
   options: NdjsonFramerOptions = {},
 ): void {
   let buffer = ''
   let bufferBytes = 0
+  const decoder = new StringDecoder('utf8')
   const maxFrameBytes = options.maxFrameBytes ?? Number.POSITIVE_INFINITY
@@
-      buffer += chunk.subarray(start, index).toString('utf8')
+      buffer += decoder.write(chunk.subarray(start, index))
       emitLine(buffer)
       buffer = ''
       bufferBytes = 0
       start = index + 1
     }
@@
     if (tailBytes > 0) {
-      buffer += chunk.subarray(start).toString('utf8')
+      buffer += decoder.write(chunk.subarray(start))
       bufferBytes += tailBytes
     }
   })
 }

You may also want to call decoder.end() on socket 'end'/'close' to flush any trailing bytes through the framer (and reject if it produces a non-empty residual without a newline).

Verification — confirm the issue is still live and that udsResponseReader already handles it:

#!/bin/bash
# Expect: ndjsonFramer has no StringDecoder; udsResponseReader does.
rg -nP "StringDecoder|string_decoder|toString\(['\"]utf8['\"]\)" src/utils/ndjsonFramer.ts src/utils/udsResponseReader.ts

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/ndjsonFramer.ts` around lines 86 - 98, The tail path in
ndjsonFramer still decodes trailing bytes with
chunk.subarray(start).toString('utf8'), which corrupts multibyte UTF‑8
sequences; fix by importing and instantiating a StringDecoder('utf8') (same
pattern used in udsResponseReader) and replace the toString call with
decoder.write(...) when appending to buffer (update the code around buffer +=
chunk.subarray(start)... and bufferBytes handling); also ensure decoder.end() is
called on socket 'end'/'close' to flush any remaining bytes and reject if the
flushed residual produces a non-empty leftover without a newline (so
rejectOversizedFrame/invalid-frame handling remains correct).

src/utils/udsMessaging.ts (1)

650-666: ⚠️ Potential issue | 🟡 Minor

authToken reset still nested inside if (socketPath) — fix not yet applied.

Per the prior review on this hunk, authToken = null (Line 657) should be hoisted out of the if (socketPath) block so a partial-failure stop path that leaves server set but socketPath null still wipes the in-memory token. capabilityFilePath and defaultSocketPath are already reset unconditionally; this completes that symmetry.

🛡️ Proposed reorder

   if (socketPath) {
     await removeSocketPath(socketPath)
     delete process.env.CLAUDE_CODE_MESSAGING_SOCKET
     logForDebugging(
       `[udsMessaging] server stopped, socket removed: ${socketPath}`,
     )
     socketPath = null
-    authToken = null
   }
+  authToken = null
   if (capabilityFilePath) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/udsMessaging.ts` around lines 650 - 666, The authToken reset is
incorrectly placed inside the if (socketPath) block; move the statement
authToken = null so it executes unconditionally after the socketPath conditional
(matching how capabilityFilePath and defaultSocketPath are cleared), i.e., keep
removing the socket and nulling socketPath inside the if (socketPath) block but
hoist authToken = null out of that block so any partial-stop path still wipes
the in-memory token.

packages/builtin-tools/src/tools/AgentTool/runAgent.ts (1)

89-93: LGTM — clean extraction with backwards-compatible re-export.

The inline filterIncompleteToolCalls logic has been moved to its own module and re-exported here, preserving the public surface for any existing consumers that import it from runAgent.ts. The behavior at line 380 (filtering forkContextMessages before forwarding) is unchanged, and agentSummary.ts is already wired to import directly from the new file (per context snippet 4).

One minor follow-up worth verifying: the rationale for filtering here directly contradicts the explicit warning in src/utils/forkedAgent.ts (context snippet 2), which deliberately skips this call to avoid orphaning paired tool_results and triggering API 400s. This pre-dates the PR, so it's not a regression — but if both code paths can produce the subagent's initialMessages, the divergence may be worth a comment explaining why runAgent is safe to filter while forkedAgent is not.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/builtin-tools/src/tools/AgentTool/runAgent.ts` around lines 89 - 93,
Add a short explanatory comment near the use of filterIncompleteToolCalls in
runAgent (the call handling forkContextMessages before forwarding) stating why
it is safe here but must be skipped in src/utils/forkedAgent.ts to avoid
orphaning paired tool_results and 400s; reference the symbols
filterIncompleteToolCalls, forkContextMessages, runAgent and forkedAgent to
locate the logic and ensure the comment explains the differing input
shapes/source of subagent initialMessages that justify the divergence.

packages/builtin-tools/src/tools/SendMessageTool/__tests__/udsRecipientSanitization.test.ts (1)

5-5: Optional: hoist the SendMessageTool import to module scope.

Each of the 7 tests repeats const { SendMessageTool } = await import('../SendMessageTool.js'). There are no module-level mocks here that require lazy loading, so a single top-level import { SendMessageTool } from '../SendMessageTool.js' would remove the boilerplate and let each test callback be synchronous when no other awaits are needed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@packages/builtin-tools/src/tools/SendMessageTool/__tests__/udsRecipientSanitization.test.ts`
at line 5, Hoist the dynamic import so tests stop repeating it: remove the
repeated "const { SendMessageTool } = await import('../SendMessageTool.js')"
lines from each of the seven tests and add a single top-level import like
"import { SendMessageTool } from '../SendMessageTool.js'" at module scope in
udsRecipientSanitization.test.ts; update the individual test callbacks to be
synchronous where no other awaits are used so they no longer need to await the
module load.

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

126-143: Tighten the protocol-cap assertion to avoid loose substring matches.

toContain('req-1') also matches req-10, req-11, …, req-19, req-100, etc. The test would still pass if compaction incorrectly kept req-10 (or any req-1X) as the first retained entry instead of req-1, masking an off-by-one in the protocol cap. Pin the exact identifier.

♻️ Proposed refactor

-    expect(compacted).toHaveLength(MAX_UNREAD_PROTOCOL_MAILBOX_MESSAGES)
-    expect(compacted[0]?.text).toContain('req-1')
+    expect(compacted).toHaveLength(MAX_UNREAD_PROTOCOL_MAILBOX_MESSAGES)
+    // Oldest (req-0) must be dropped; first retained should be exactly req-1.
+    expect(JSON.parse(compacted[0]!.text)).toMatchObject({ request_id: 'req-1' })
+    expect(JSON.parse(compacted.at(-1)!.text)).toMatchObject({
+      request_id: `req-${MAX_UNREAD_PROTOCOL_MAILBOX_MESSAGES}`,
+    })

🤖 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 126 - 143, The test
uses a loose substring assertion that can match req-10 etc.; update the
assertion to pin the exact retained identifier: locate the test "caps unread
protocol messages with an independent bound" and change the final assertion on
compactMailboxMessages' result (function compactMailboxMessages, constant
MAX_UNREAD_PROTOCOL_MAILBOX_MESSAGES, helper message) to verify the exact
request_id for the first retained message—either by parsing compacted[0].text as
JSON and asserting request_id === 'req-1' or by checking for the exact key/value
string (e.g. '"request_id":"req-1"') so off-by-one matches like req-10 cannot
satisfy the test.

src/services/AgentSummary/summaryContext.ts (1)

33-41: Optional: prefer Object.keys over for...in + Object.hasOwn filter.

for (const key in record) { if (!Object.hasOwn(record, key)) continue; ... } is functionally equivalent to for (const key of Object.keys(record)) and avoids walking the prototype chain. Same nit applies at lines 84–86 inside updateFingerprintHash. Purely stylistic.

♻️ Proposed minor refactor

       } else {
         const record = value as Record<string, unknown>
-        for (const key in record) {
-          if (!Object.hasOwn(record, key)) continue
+        for (const key of Object.keys(record)) {
           total += key.length + 3
           total += estimateJsonChars(record[key], limit - total, seen)
           if (total > limit) return total
         }
       }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/AgentSummary/summaryContext.ts` around lines 33 - 41, Replace
the for...in + Object.hasOwn check with a direct Object.keys iteration to avoid
prototype-chain traversal: in summaryContext.ts update the loop inside
estimateJsonChars that currently uses "for (const key in record) { if
(!Object.hasOwn(record, key)) continue; ... }" to "for (const key of
Object.keys(record)) { ... }", and make the same change in updateFingerprintHash
where a similar for...in + Object.hasOwn pattern is used; keep the inner logic
identical (accumulating total/processing values) and preserve existing calls to
estimateJsonChars/updateFingerprintHash and the seen/limit handling.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes