Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (8)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{tsx,ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/**/*.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{test,spec}.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughResume recovery now restores agent-listing state. Local transcripts retain listing attachments, while external transcript and feedback paths filter them and repair parent links. MCP and deferred-tool delta handling validates malformed records. Release tests cover version ChangesResume attachment handling
External transcript egress
Release metadata
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/attachments.ts`:
- Around line 1648-1657: Add regression tests for the attachment resume flow
around the suppressNextAgent latch: verify an unchanged listing is suppressed
only once, changed listings preserve and emit agent additions/removals, and
resetSentSkillNames() clears the suppression state. Run the targeted tests, bun
run typecheck, and bun run typecheck:type-tests when available.
- Around line 1648-1657: The suppressNextAgent branch in the attachment listing
flow should consume the latch and return an empty delta only when the current
filtered agent set is unchanged from the persisted listing. When agent types
were removed, added, or the filtered set is empty, preserve the latch for normal
processing so the existing removedTypes/addedTypes calculation emits the
legitimate delta; update announced only for the unchanged duplicate case.
In `@src/utils/conversationRecovery.ts`:
- Around line 650-654: Add an end-to-end resume regression test covering the
agent_listing_delta branch in conversation recovery: restore a transcript
containing this attachment, verify the next attachment pass suppresses only the
duplicate listing, and confirm it emits a delta when the available agent set
changes. Place the test alongside the existing resume/attachment coverage and
preserve unrelated attachment behavior.
In `@src/utils/sessionStorage.ts`:
- Around line 4775-4791: Add regression coverage in sessionStorage tests for all
four allowed attachment types—skill_listing, agent_listing_delta,
deferred_tools_delta, and mcp_instructions_delta—while preserving the
hook_additional_context environment gate and verifying unrelated attachments
remain filtered. Run the targeted tests, bun run check, and bun run typecheck.
- Around line 4783-4791: Update the attachment-type allowlist in the transcript
filtering logic around m.type and getUserType() so it retains only skill_listing
and agent_listing_delta; remove deferred_tools_delta and mcp_instructions_delta
from the accepted types to prevent sensitive MCP instruction payloads from being
persisted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 65b37ce1-c504-46db-b9a5-4e784ca1afd1
📒 Files selected for processing (3)
src/utils/attachments.tssrc/utils/conversationRecovery.tssrc/utils/sessionStorage.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/conversationRecovery.tssrc/utils/sessionStorage.tssrc/utils/attachments.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/conversationRecovery.tssrc/utils/sessionStorage.tssrc/utils/attachments.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/conversationRecovery.tssrc/utils/sessionStorage.tssrc/utils/attachments.ts
🔇 Additional comments (2)
src/utils/attachments.ts (1)
2938-2938: LGTM!Also applies to: 2962-2973
src/utils/conversationRecovery.ts (1)
21-24: LGTM!
| // Same for agent_listing_delta — mid-history re-announce busts Moonshot | ||
| // / OpenAI automatic prefix cache on every --resume. | ||
| if (attachment.type === 'agent_listing_delta') { | ||
| suppressNextAgentListing() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add an end-to-end resume regression test for this trigger.
Restore a transcript containing agent_listing_delta, then verify the next attachment pass suppresses only the duplicate listing and still emits a delta when the available agent set changes.
As per coding guidelines, TypeScript behavior changes must add/update tests. As per path instructions, resume/attachment behavior must be covered by tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/conversationRecovery.ts` around lines 650 - 654, Add an end-to-end
resume regression test covering the agent_listing_delta branch in conversation
recovery: restore a transcript containing this attachment, verify the next
attachment pass suppresses only the duplicate listing, and confirm it emits a
delta when the available agent set changes. Place the test alongside the
existing resume/attachment coverage and preserve unrelated attachment behavior.
Sources: Coding guidelines, Path instructions
CodeRabbit follow-up for PR Gitlawb#2070: suppressNextAgentListing only skips when the filtered agent set matches the transcript announced set; otherwise emit a normal add/remove delta. Keep all four prefix-cache listing types on the external allowlist (dropping mcp/deferred without a resume latch re-busts cache). Add unit coverage for latch and isLoggableMessage.
CodeRabbit follow-up (373d413)Addressing the review on this PR under the agreed policy: 1. Latch - fixed (not always return [])
2. Tests - added
3. Allowlist - keeping all four types (intentional)We are not dropping
Privacy note in 4. Policy changeYes - this is an intentional, narrow policy change for non-ant transcript persistence of turn-0 listing deltas only, required for OpenAI-compatible automatic prefix cache (Moonshot measured: resume first-call cache 0% -> 98.9%). |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/attachments.agentListingResume.test.ts`:
- Around line 86-112: Update both suppressNextAgentListing tests to verify
one-shot latch consumption: after the existing assertion, call
getAgentListingDeltaAttachment with the same context and an empty messages
array, then assert the current agents are emitted as an initial listing. Apply
this follow-up check to both the unchanged-set and changed-set cases, preserving
their existing assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b500241f-9d2c-43bd-9c43-0aa973af598b
📒 Files selected for processing (4)
src/utils/attachments.agentListingResume.test.tssrc/utils/attachments.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/sessionStorage.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/sessionStorage.listingAllowlist.test.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/sessionStorage.tssrc/utils/attachments.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/sessionStorage.listingAllowlist.test.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/sessionStorage.tssrc/utils/attachments.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/sessionStorage.listingAllowlist.test.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/sessionStorage.tssrc/utils/attachments.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/sessionStorage.listingAllowlist.test.tssrc/utils/attachments.agentListingResume.test.ts
🔇 Additional comments (4)
src/utils/attachments.ts (1)
1648-1672: LGTM!Also applies to: 2951-2951, 2981-2988
src/utils/attachments.agentListingResume.test.ts (1)
1-152: 📐 Maintainability & Code QualityRun the focused validation before merge.
No check results were provided. Run the targeted Bun test,
bun run typecheck, andbun run check; runbun run typecheck:type-testsif that script applies.As per coding guidelines, changed TypeScript requires relevant validation. As per path instructions, narrow Bun tests and type checks should be run for resume/attachment changes.
Sources: Coding guidelines, Path instructions
src/utils/sessionStorage.ts (1)
4783-4787: Enforce the “already sent at turn 0” privacy invariant.The allowlist accepts every
mcp_instructions_delta, whose payload contains raw instruction blocks. This comment assumes those blocks are always already-sent and non-sensitive, but the predicate does not enforce that. Confirm the attachment is initial-only; otherwise narrow or redact the external transcript payload while preserving the data required for cache reconstruction.src/utils/sessionStorage.listingAllowlist.test.ts (1)
1-120: LGTM!
CodeRabbit follow-up (latch test)Addressed the remaining actionable comment:
Non-blocking: keeping all four allowlist types remains intentional; mcp/deferred are turn-0 model-visible catalogs needed for Moonshot prefix-cache reconstruction. Commit: cdcd966 |
|
@coderabbitai please do intensive review on this again |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/attachments.ts`:
- Around line 2987-2990: Persist the recovered announced agent-type set
alongside the suppression latch in src/utils/attachments.ts:2987-2990, compare
against it in the attachment-generation logic at
src/utils/attachments.ts:1657-1671 when messages are missing or unhydrated, and
clear it during reset at src/utils/attachments.ts:2951. In
src/utils/conversationRecovery.ts:650-654, reconstruct and pass/store the final
set across transcript deltas instead of only arming the boolean. Add the
regression coverage in src/utils/attachments.agentListingResume.test.ts:148-170
for restoring a listing followed by attachment generation with missing or
unhydrated messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 043dc734-ed76-4bb3-a844-c16ad33c08b2
📒 Files selected for processing (5)
src/utils/attachments.agentListingResume.test.tssrc/utils/attachments.tssrc/utils/conversationRecovery.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/sessionStorage.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: smoke-and-tests (24.11.x)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/sessionStorage.tssrc/utils/attachments.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/conversationRecovery.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/sessionStorage.tssrc/utils/attachments.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/conversationRecovery.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/sessionStorage.tssrc/utils/attachments.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/conversationRecovery.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/attachments.agentListingResume.test.tssrc/utils/sessionStorage.listingAllowlist.test.ts
🔇 Additional comments (3)
src/utils/sessionStorage.ts (1)
4769-4807: LGTM!src/utils/sessionStorage.listingAllowlist.test.ts (2)
1-119: LGTM!
29-119: 📐 Maintainability & Code QualityConfirm required Bun validation.
No check results were provided. Run and report the focused test plus
bun run typecheck; runbun run typecheck:type-testsif this repository enables it.Sources: Coding guidelines, Path instructions
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep externally persisted listing payloads behind the privacy boundary
src/utils/sessionStorage.ts:4791
Before this change, external sessions filtered all attachments. The new allow-list instead sends these four payload classes throughcleanMessagesForLoggingandrecordTranscript, including remote persistence, even though the surrounding policy explicitly excludes non-ant attachments because they can contain sensitive data. They are not a structured safe catalog:skill_listing.contentincludes local skill descriptions,agent_listing_delta.addedLinesincludes custom agent descriptions and tool policy, andmcp_instructions_delta.addedBlockscopies arbitrary server-providedInitializeResult.instructions. MCP servers can also connect after turn zero, so the stated turn-zero invariant is not enforced. The existing CodeRabbit privacy request is therefore valid on the current head. Keep raw payloads out of external transcripts, or introduce explicit opt-in/redaction with an enforceable safe-data contract and a separate resume-cache mechanism. -
[P1] Refresh same-named MCP instructions after resume
src/utils/sessionStorage.ts:4794
Persistingmcp_instructions_deltamakes the next process reconstruct an announced set from server names alone. A resumed session creates a fresh MCP connection, so a server can return differentInitializeResult.instructionsunder the same configured name after a deployment or configuration/auth-policy change; however,getMcpInstructionsDeltasees the old name and returns no attachment. I verified this exact old-block/new-block case returnsnull. The model therefore keeps following stale server instructions. This is newly reachable for external sessions because this PR starts retaining those deltas. Compare the rendered instruction block (or a connection/version fingerprint) and emit an update when it changes; a name-only scan is not sufficient across a process boundary. -
[P2] Re-announce changed definitions for an existing agent type
src/utils/attachments.ts:1648
The new resume path considers an agent listing unchanged when its set ofagentTypestrings matches the transcript. That ignores the content the listing actually gives the model:formatAgentLineincludeswhenToUseplus the effective tool allow/deny policy. Editing a project/plugin agent while the CLI is stopped, while retaining its type name, now resumes with the old description and capability contract and emits no corrective delta; before this PR the external transcript did not carry the prior delta, so the current listing was sent again. Fingerprint the rendered listing data as well as the type names and add coverage for a same-type description/tool-policy change. -
[P2] Serialize the new environment-mutating test
src/utils/sessionStorage.listingAllowlist.test.ts:29
These tests repeatedly change process-globalUSER_TYPEandCLAUDE_CODE_SAVE_HOOK_ADDITIONAL_CONTEXT, but unlike the adjacent attachment-resume test and the repository's other environment-mutating tests they never acquiresharedMutationLock. Bun can execute test files concurrently, so another test observing or changing either variable can race these assertions and leak state across files. Acquire and release the shared mutation lock in setup/teardown before mutating the environment.
|
P2 (sharedMutationLock / env-mutating listingAllowlist test) — addressed
Remaining review feedback — in progress (not in this commit) Still working locally on:
Will follow up with commits for those once the resume-cache path is wired end-to-end. |
How P1-2 and P2-1 were fixed (
|
| Reviewer case | Result after fix |
|---|---|
Old block in transcript for docs, fresh handshake returns new instructions, same name |
Delta is not null; addedNames: ['docs'] with the new addedBlocks |
| Same name, same block | Still null (no spurious cache bust) |
| Server disconnected | Still in removedNames |
Test that matches the reviewer’s “I verified old-block/new-block returns null” case:
src/utils/mcpInstructionsDelta.test.ts → getMcpInstructionsDelta re-announces when same-name instructions change after resume
P2-1 — Re-announce changed definitions for an existing agent type
Reviewer finding (verbatim intent):
The resume path treated an agent listing as unchanged when the set of agentType strings matched the transcript. That ignores what the model actually sees: formatAgentLine includes whenToUse plus the effective tool allow/deny policy. Editing a project/plugin agent while the CLI is stopped (same type name) resumed with the old description/capability contract and emitted no corrective delta. Fingerprint the rendered listing data as well as type names; add coverage for same-type description/tool-policy change.
What was wrong before:
const announced = new Set<string>() // types only
// suppress latch: unchanged iff Set equality of types
const added = filtered.filter(a => !announced.has(a.agentType)) // same type → never re-announceHow it is fixed now (src/utils/attachments.ts → getAgentListingDeltaAttachment):
- Reconstruct
Map<agentType, formatAgentLine>from prioragent_listing_deltaattachments (addedLinespaired withaddedTypes). - Suppress latch (
suppressNextAgent): skip only when every current agent’sformatAgentLine(a)equals the announced line for that type (size match + content match). Same types with different whenToUse/tool policy → latch does not swallow the delta. - Normal delta path: add when type is new or
announced.get(type) !== formatAgentLine(a).
formatAgentLine is exactly: `- ${type}: ${whenToUse} (Tools: ${toolsDescription})` — so both description and tool policy are in the fingerprint.
Does this solve the reviewed case?
| Reviewer case | Result after fix |
|---|---|
Same agentType, whenToUse changed across resume (+ suppress latch armed) |
Emits delta with new addedLines containing the new description |
Same type, tool allowlist/denylist changed (formatAgentLine Tools: …) |
Emits delta; line contains the new tool policy |
| Same type, same rendered line | Suppress latch still returns [] (prefix-stable path preserved) |
Coverage:
src/utils/attachments.agentListingResume.test.ts
suppressNextAgentListing re-announces when same-type whenToUse changed across resumegetAgentListingDeltaAttachment re-announces when same-type tool policy changedsuppressNextAgentListing still skips when same-type content matches transcript
Summary vs review asks
| Ask | Addressed? | Mechanism |
|---|---|---|
| P1-2: compare rendered MCP instruction block, not name alone | Yes | Map<name, block> + prev !== block → re-announce |
| P1-2: old-block / new-block after resume must not return null | Yes | Explicit unit test for that case |
P2-1: fingerprint rendered listing (formatAgentLine), not type names alone |
Yes | Map<type, line> in latch + added filter |
| P2-1: coverage for same-type description / tool-policy change | Yes | Two dedicated tests + latch still-skips when content matches |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/mcpInstructionsDelta.test.ts`:
- Around line 70-78: Add a test alongside “getMcpInstructionsDelta removes
disconnected servers by name” covering a server that remains connected but
returns no instructions after resume; assert the delta reports that server in
removedNames while addedNames remains empty, locking in the behavior of
getMcpInstructionsDelta.
In `@src/utils/mcpInstructionsDelta.ts`:
- Around line 128-135: Update the removal logic in the loop over
announced.keys() to remove a server when it is either no longer connected or has
no current entry in blocks, while preserving announced blocks for connected
servers that still provide instructions. Use the existing connectedNames and
blocks symbols so a reconnected server without instructions clears its stale
announcement.
In `@src/utils/resumeCache.ts`:
- Around line 232-303: Add focused tests for
hydrateListingAttachmentsFromResumeCache covering existing-listing skip
behavior, deferred-to-agent-to-mcp-to-skill injection order, and empty-cache
no-op behavior. Add tests for updateResumeCacheFromMessages verifying cache data
merges correctly across repeated message slices. Use the project’s established
test patterns, and report the focused bun test command plus bun run typecheck.
- Around line 169-179: Update the skill_listing branch in
updateResumeCacheFromMessages to deduplicate entries using contentHash before
pushing to cache.skillListings. Reuse the computed hash to detect an existing
listing and skip appending duplicates, while preserving the current metadata and
dirty-flag behavior for newly added listings.
- Around line 165-168: Update the attachment loop in
updateResumeCacheFromMessages to skip records whose m.attachment is null or not
an object before reading a.type, matching the guard used by
hasListingAttachment. Preserve processing for valid attachment records and
prevent malformed legacy transcript entries from throwing during
recordTranscript.
- Around line 305-345: Remove getAnnouncedMcpInstructionHashes and
getAnnouncedAgentListingLines from resumeCache.ts, then reuse
getAnnouncedMcpInstructionBlocks from mcpInstructionsDelta.ts and the existing
agent-listing reconstruction helper from attachments.ts. Preserve the MCP
hash-map result by applying hashContent(block) to each reconstructed block, and
update all local callers/imports accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: adbbdbea-2a58-43c1-98ec-72ed8d8b7d24
📒 Files selected for processing (9)
src/utils/attachments.agentListingResume.test.tssrc/utils/attachments.tssrc/utils/cachePaths.tssrc/utils/conversationRecovery.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/resumeCache.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/sessionStorage.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/cachePaths.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/attachments.tssrc/utils/conversationRecovery.tssrc/utils/sessionStorage.tssrc/utils/resumeCache.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/cachePaths.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/attachments.tssrc/utils/conversationRecovery.tssrc/utils/sessionStorage.tssrc/utils/resumeCache.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/cachePaths.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/attachments.tssrc/utils/conversationRecovery.tssrc/utils/sessionStorage.tssrc/utils/resumeCache.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/mcpInstructionsDelta.test.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/attachments.agentListingResume.test.ts
🔇 Additional comments (10)
src/utils/attachments.ts (2)
1667-1681: The latch is still consumed when the comparison falls through, so an attachment pass that receivesundefinedor un-hydratedmessagesburns it and re-injects a full initial listing. This was raised previously.
1639-1655: LGTM!Also applies to: 1683-1690
src/utils/cachePaths.ts (1)
21-21: LGTM!src/utils/mcpInstructionsDelta.ts (1)
46-67: LGTM!Also applies to: 118-126
src/utils/conversationRecovery.ts (1)
25-25: LGTM!Also applies to: 809-819
src/utils/sessionStorage.ts (1)
85-85: LGTM!Also applies to: 1522-1525, 4780-4787
src/utils/attachments.agentListingResume.test.ts (1)
10-10: LGTM!Also applies to: 68-85, 89-89, 110-110, 151-159, 174-222
src/utils/mcpInstructionsDelta.test.ts (1)
1-68: LGTM!Also applies to: 80-100
src/utils/sessionStorage.listingAllowlist.test.ts (1)
38-53: LGTM!Also applies to: 64-82, 132-153
src/utils/resumeCache.ts (1)
72-77: 🩺 Stability & AvailabilityNo change needed. Resume switches the active session to
asSessionId(result.sessionId)before--resumewrites, sogetResumeCachePath()resolves using the same resumed session ID.
| export async function hydrateListingAttachmentsFromResumeCache( | ||
| messages: Message[], | ||
| sessionId: string = getSessionId(), | ||
| ): Promise<Message[]> { | ||
| if (hasListingAttachment(messages)) { | ||
| return messages | ||
| } | ||
|
|
||
| const cache = await loadResumeCache(sessionId) | ||
| const injected: Message[] = [] | ||
|
|
||
| // Match turn-0 collection order in attachments.ts (deferred → agent → mcp → skill). | ||
| if (cache.deferredTools && Object.keys(cache.deferredTools.lines).length > 0) { | ||
| const names = Object.keys(cache.deferredTools.lines).sort() | ||
| injected.push( | ||
| createAttachmentMessage({ | ||
| type: 'deferred_tools_delta', | ||
| addedNames: names, | ||
| addedLines: names.map(n => cache.deferredTools!.lines[n]!), | ||
| removedNames: [], | ||
| }), | ||
| ) | ||
| } | ||
|
|
||
| if (cache.agentListing && Object.keys(cache.agentListing.lines).length > 0) { | ||
| const types = Object.keys(cache.agentListing.lines).sort() | ||
| injected.push( | ||
| createAttachmentMessage({ | ||
| type: 'agent_listing_delta', | ||
| addedTypes: types, | ||
| addedLines: types.map(t => cache.agentListing!.lines[t]!), | ||
| removedTypes: [], | ||
| isInitial: true, | ||
| showConcurrencyNote: cache.agentListing.showConcurrencyNote, | ||
| }), | ||
| ) | ||
| } | ||
|
|
||
| if ( | ||
| cache.mcpInstructions && | ||
| Object.keys(cache.mcpInstructions.blocks).length > 0 | ||
| ) { | ||
| const names = Object.keys(cache.mcpInstructions.blocks).sort() | ||
| injected.push( | ||
| createAttachmentMessage({ | ||
| type: 'mcp_instructions_delta', | ||
| addedNames: names, | ||
| addedBlocks: names.map(n => cache.mcpInstructions!.blocks[n]!), | ||
| removedNames: [], | ||
| }), | ||
| ) | ||
| } | ||
|
|
||
| for (const listing of cache.skillListings) { | ||
| injected.push( | ||
| createAttachmentMessage({ | ||
| type: 'skill_listing', | ||
| content: listing.content, | ||
| skillCount: listing.skillCount, | ||
| isInitial: listing.isInitial, | ||
| }), | ||
| ) | ||
| } | ||
|
|
||
| if (injected.length === 0) { | ||
| return messages | ||
| } | ||
|
|
||
| // Prepend so normalizeAttachmentForAPI rebuilds the same early prefix the | ||
| // original session sent before the first real user turn content. | ||
| return [...injected, ...messages] | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add tests for cache hydration.
No test file for resumeCache.ts appears in this change set. Cover: hydration skipped when a listing attachment already exists, injection order (deferred → agent → mcp → skill), empty-cache no-op, and merge behavior of updateResumeCacheFromMessages across repeated slices. Report the exact commands, for example the focused bun test path plus bun run typecheck.
As per coding guidelines, "Add or update tests when a TypeScript or TSX change affects behavior." As per path instructions, add or update tests for cache hydration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/resumeCache.ts` around lines 232 - 303, Add focused tests for
hydrateListingAttachmentsFromResumeCache covering existing-listing skip
behavior, deferred-to-agent-to-mcp-to-skill injection order, and empty-cache
no-op behavior. Add tests for updateResumeCacheFromMessages verifying cache data
merges correctly across repeated message slices. Use the project’s established
test patterns, and report the focused bun test command plus bun run typecheck.
Sources: Coding guidelines, Path instructions
| /** Content hash map for MCP instruction blocks currently announced in messages. */ | ||
| export function getAnnouncedMcpInstructionHashes( | ||
| messages: Message[], | ||
| ): Map<string, string> { | ||
| const announced = new Map<string, string>() | ||
| for (const msg of messages) { | ||
| if (msg.type !== 'attachment') continue | ||
| if (msg.attachment.type !== 'mcp_instructions_delta') continue | ||
| const { addedNames, addedBlocks, removedNames } = msg.attachment | ||
| // Removals first so same-name updates keep the new block's hash. | ||
| for (const n of removedNames) announced.delete(n) | ||
| for (let i = 0; i < addedNames.length; i++) { | ||
| const name = addedNames[i] | ||
| const block = addedBlocks[i] | ||
| if (name && block !== undefined) { | ||
| announced.set(name, hashContent(block)) | ||
| } | ||
| } | ||
| } | ||
| return announced | ||
| } | ||
|
|
||
| /** agentType → rendered line for attachments already in the conversation. */ | ||
| export function getAnnouncedAgentListingLines( | ||
| messages: Message[], | ||
| ): Map<string, string> { | ||
| const announced = new Map<string, string>() | ||
| for (const msg of messages) { | ||
| if (msg.type !== 'attachment') continue | ||
| if (msg.attachment.type !== 'agent_listing_delta') continue | ||
| const { addedTypes, addedLines, removedTypes } = msg.attachment | ||
| // Removals first so same-type content updates keep the new line. | ||
| for (const t of removedTypes) announced.delete(t) | ||
| for (let i = 0; i < addedTypes.length; i++) { | ||
| const type = addedTypes[i] | ||
| const line = addedLines[i] | ||
| if (type && line !== undefined) announced.set(type, line) | ||
| } | ||
| } | ||
| return announced | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find callers of the announced-scan helpers.
rg -n 'getAnnouncedMcpInstructionHashes|getAnnouncedAgentListingLines|getAnnouncedMcpInstructionBlocks' srcRepository: Gitlawb/openclaude
Length of output: 813
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file sizes =="
wc -l src/utils/resumeCache.ts src/utils/attachments.ts src/utils/mcpInstructionsDelta.ts
echo "== resumeCache relevant export area =="
sed -n '280,360p' src/utils/resumeCache.ts | cat -n
echo "== mcpInstructionsDelta relevant function area =="
sed -n '1,120p' src/utils/mcpInstructionsDelta.ts | cat -n
echo "== attachments referenced area =="
rg -n -C 4 'get.+Announced|announced|agent_listing_delta|mcp_instructions_delta' src/utils/attachments.ts
echo "== diff stat and changed file markers if available =="
git diff --stat || true
git diff -- src/utils/resumeCache.ts src/utils/mcpInstructionsDelta.ts src/utils/attachments.ts | sed -n '1,220p' || true
echo "== AST outline candidates =="
ast-grep outline src/utils/resumeCache.ts --match getAnnouncedAgentListingLines --view expanded || true
ast-grep outline src/utils/mcpInstructionsDelta.ts --match getAnnouncedMcpInstructionBlocks --view expanded || trueRepository: Gitlawb/openclaude
Length of output: 15773
Unify the announced-attachment reconstruction helpers.
getAnnouncedAgentListingLines duplicates the agent listing delta reconstruction in src/utils/attachments.ts, while getAnnouncedMcpInstructionHashes duplicates getAnnouncedMcpInstructionBlocks in src/utils/mcpInstructionsDelta.ts. Neither new helper has callers outside src/utils/resumeCache.ts, so remove them and reuse the existing helper plus hashContent(block) where needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/resumeCache.ts` around lines 305 - 345, Remove
getAnnouncedMcpInstructionHashes and getAnnouncedAgentListingLines from
resumeCache.ts, then reuse getAnnouncedMcpInstructionBlocks from
mcpInstructionsDelta.ts and the existing agent-listing reconstruction helper
from attachments.ts. Preserve the MCP hash-map result by applying
hashContent(block) to each reconstructed block, and update all local
callers/imports accordingly.
|
Thanks for the latest review on Actionable items
VerificationP1-1 (privacy / local resume-cache boundary) remains in progress — this commit tightens Bot-flagged correctness on top of that work; please re-review when convenient. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/attachments.ts (1)
1644-1655: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSkip malformed attachments before reading
msg.attachment.type. Runtime transcript data can haveattachmentset tonull,undefined, or a non-object; these two attachment-scanning loops currently try to read.typebefore those guards would skip the message. Add the same guard used inupdateResumeCacheFromMessagestogetAgentListingDeltaAttachment(src/utils/attachments.ts#L1640-L1642) and togetAnnouncedMcpInstructionBlocks/getMcpInstructionsDelta(src/utils/mcpInstructionsDelta.ts#L54-L92).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/attachments.ts` around lines 1644 - 1655, Guard attachment values before accessing attachment.type in getAgentListingDeltaAttachment in src/utils/attachments.ts (lines 1644-1655) and getMcpInstructionsDelta/getAnnouncedMcpInstructionBlocks in src/utils/mcpInstructionsDelta.ts (lines 86-92), reusing the existing updateResumeCacheFromMessages malformed-attachment guard so null, undefined, and non-object attachments are skipped safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/resumeCache.test.ts`:
- Around line 1-179: Update the resume cache test cleanup around
resetResumeCacheForTesting and the asynchronous updateResumeCacheFromMessages
flow so afterEach waits for all pending scheduled writes before clearing state.
Ensure tests that trigger cache saves do not leave dangling writeChain or
scheduleSave promises after completion, including tests that do not await
hydration.
---
Outside diff comments:
In `@src/utils/attachments.ts`:
- Around line 1644-1655: Guard attachment values before accessing
attachment.type in getAgentListingDeltaAttachment in src/utils/attachments.ts
(lines 1644-1655) and getMcpInstructionsDelta/getAnnouncedMcpInstructionBlocks
in src/utils/mcpInstructionsDelta.ts (lines 86-92), reusing the existing
updateResumeCacheFromMessages malformed-attachment guard so null, undefined, and
non-object attachments are skipped safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 762e2ccc-a2d0-48bb-a078-046410182593
📒 Files selected for processing (7)
src/utils/attachments.agentListingResume.test.tssrc/utils/attachments.tssrc/utils/conversationRecovery.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/resumeCache.test.tssrc/utils/resumeCache.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/resumeCache.test.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/conversationRecovery.tssrc/utils/attachments.tssrc/utils/resumeCache.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/resumeCache.test.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/conversationRecovery.tssrc/utils/attachments.tssrc/utils/resumeCache.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/resumeCache.test.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/conversationRecovery.tssrc/utils/attachments.tssrc/utils/resumeCache.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/mcpInstructionsDelta.test.tssrc/utils/resumeCache.test.tssrc/utils/attachments.agentListingResume.test.ts
🔇 Additional comments (8)
src/utils/resumeCache.ts (2)
165-188: LGTM!
189-234: LGTM!src/utils/attachments.ts (2)
1656-1698: LGTM!
2978-2983: LGTM!Also applies to: 3006-3032
src/utils/mcpInstructionsDelta.ts (1)
118-136: LGTM!src/utils/mcpInstructionsDelta.test.ts (1)
80-86: LGTM!src/utils/conversationRecovery.ts (1)
619-685: LGTM!src/utils/attachments.agentListingResume.test.ts (1)
224-243: LGTM!
|
Addressed the latest CodeRabbit follow-ups on
Targeted tests: |
CodeRabbit follow-up for PR Gitlawb#2070: suppressNextAgentListing only skips when the filtered agent set matches the transcript announced set; otherwise emit a normal add/remove delta. Keep all four prefix-cache listing types on the external allowlist (dropping mcp/deferred without a resume latch re-busts cache). Add unit coverage for latch and isLoggableMessage.
44ed8b8 to
3c1c78f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/attachments.ts (1)
1647-1656: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate complete attachment schemas at the runtime boundary.
Both paths accept partial objects with recognized
typevalues. These records can throw during reconstruction and can produce incorrect MCP diagnostics.
src/utils/attachments.ts#L1647-L1656: validateremovedTypes,addedTypes, andaddedLinesbefore iteration.src/utils/mcpInstructionsDelta.ts#L57-L66: validateremovedNames,addedNames, andaddedBlocksbefore iteration.src/utils/mcpInstructionsDelta.ts#L93-L95: apply the same validity rule before incrementing diagnostics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/attachments.ts` around lines 1647 - 1656, Validate the complete recognized attachment schema before processing: in src/utils/attachments.ts lines 1647-1656, require removedTypes, addedTypes, and addedLines to be valid arrays before iteration; in src/utils/mcpInstructionsDelta.ts lines 57-66, require removedNames, addedNames, and addedBlocks to be valid arrays before reconstruction; and in src/utils/mcpInstructionsDelta.ts lines 93-95, apply the same validity rule before incrementing diagnostics. Anchor these checks to the existing attachment reconstruction and MCP delta diagnostic functions, skipping malformed records without throwing or counting them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/resumeCache.test.ts`:
- Around line 15-22: Update the cleanup in the beforeEach hook around
resetResumeCacheForTesting and unlink(getResumeCachePath(SESSION)) to ignore
only ENOENT missing-file errors; rethrow all other unlink failures so
stale-cache or isolation problems fail the test. Apply the same handling to the
other catch block covering this cleanup.
---
Outside diff comments:
In `@src/utils/attachments.ts`:
- Around line 1647-1656: Validate the complete recognized attachment schema
before processing: in src/utils/attachments.ts lines 1647-1656, require
removedTypes, addedTypes, and addedLines to be valid arrays before iteration; in
src/utils/mcpInstructionsDelta.ts lines 57-66, require removedNames, addedNames,
and addedBlocks to be valid arrays before reconstruction; and in
src/utils/mcpInstructionsDelta.ts lines 93-95, apply the same validity rule
before incrementing diagnostics. Anchor these checks to the existing attachment
reconstruction and MCP delta diagnostic functions, skipping malformed records
without throwing or counting them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 101b47b5-b9e7-47c8-91b9-6a43f87476bc
📒 Files selected for processing (5)
src/utils/attachments.tssrc/utils/mcpInstructionsDelta.tssrc/utils/resumeCache.test.tssrc/utils/resumeCache.tssrc/utils/sessionStorage.listingAllowlist.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/sessionStorage.listingAllowlist.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/attachments.tssrc/utils/resumeCache.test.tssrc/utils/resumeCache.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/sessionStorage.listingAllowlist.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/attachments.tssrc/utils/resumeCache.test.tssrc/utils/resumeCache.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/sessionStorage.listingAllowlist.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/attachments.tssrc/utils/resumeCache.test.tssrc/utils/resumeCache.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/sessionStorage.listingAllowlist.test.tssrc/utils/resumeCache.test.ts
🔇 Additional comments (4)
src/utils/sessionStorage.listingAllowlist.test.ts (1)
43-45: LGTM!src/utils/resumeCache.ts (1)
12-15: LGTM!Also applies to: 127-137
src/utils/attachments.ts (1)
1662-1702: LGTM!Also applies to: 2985-2986, 3012-3038
src/utils/mcpInstructionsDelta.ts (1)
78-92: LGTM!Also applies to: 94-99, 124-139
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/sessionStorage.ts (1)
4788-4797: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard malformed attachment records here too.
m.attachment.typeis read without checking thatm.attachmentis a non-null object.resumeCache.ts,attachments.ts, andmcpInstructionsDelta.tsall addif (!m.attachment || typeof m.attachment !== 'object') continue/returnfor the same reason: legacy transcripts can carry malformed attachment records.isLoggableMessageruns insidecleanMessagesForLogging, called on everyrecordTranscript, so one malformed record crashes transcript recording with aTypeErrorinstead of just being skipped.Consider adding a regression test in `src/utils/sessionStorage.listingAllowlist.test.ts` for a malformed `attachment: null` record, matching the coverage already added in `resumeCache.test.ts`. Would you like me to draft that test?🛡️ Proposed fix
if (m.type === 'attachment' && getUserType() !== 'ant') { + // Legacy transcripts may carry malformed attachment records + // (null/undefined/non-object payload). Treat as non-loggable instead + // of throwing. + if (!m.attachment || typeof m.attachment !== 'object') return false const t = m.attachment.type if ( t === 'hook_additional_context' &&🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/sessionStorage.ts` around lines 4788 - 4797, Update the attachment branch in isLoggableMessage to return false when m.attachment is null or not an object before reading m.attachment.type. Preserve the existing hook_additional_context and environment-flag checks for valid attachment objects, and add regression coverage for an attachment: null record if the surrounding tests support it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/conversationRecovery.ts`:
- Around line 837-848: Remove the redundant if/else around
hydrateListingAttachmentsFromResumeCache in the conversation recovery flow and
make a single call with messages. Rely on the function’s default sessionId
parameter to obtain the session when no explicit value is available, preserving
the existing message hydration behavior.
---
Outside diff comments:
In `@src/utils/sessionStorage.ts`:
- Around line 4788-4797: Update the attachment branch in isLoggableMessage to
return false when m.attachment is null or not an object before reading
m.attachment.type. Preserve the existing hook_additional_context and
environment-flag checks for valid attachment objects, and add regression
coverage for an attachment: null record if the surrounding tests support it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 273e663b-72f2-4b02-9285-d73c4683731f
📒 Files selected for processing (10)
src/utils/attachments.agentListingResume.test.tssrc/utils/attachments.tssrc/utils/cachePaths.tssrc/utils/conversationRecovery.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/resumeCache.test.tssrc/utils/resumeCache.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/sessionStorage.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/mcpInstructionsDelta.tssrc/utils/resumeCache.test.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/cachePaths.tssrc/utils/conversationRecovery.tssrc/utils/sessionStorage.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/attachments.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/resumeCache.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/mcpInstructionsDelta.tssrc/utils/resumeCache.test.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/cachePaths.tssrc/utils/conversationRecovery.tssrc/utils/sessionStorage.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/attachments.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/resumeCache.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/mcpInstructionsDelta.tssrc/utils/resumeCache.test.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/cachePaths.tssrc/utils/conversationRecovery.tssrc/utils/sessionStorage.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/attachments.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/resumeCache.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/resumeCache.test.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/attachments.agentListingResume.test.ts
🔇 Additional comments (12)
src/utils/resumeCache.test.ts (2)
15-22: 🩺 Stability & Availability | ⚡ Quick winCatch only
ENOENTduring resume-cache file cleanup.Both
beforeEach(Lines 15-22) andafterEach(Lines 24-34) swallow everyunlinkerror, not just "file does not exist." Ifunlinkfails for another reason (permissions, I/O), the stale resume-cache file remains on disk, and the next test can load leftover data instead of a clean cache.Catch only
ENOENTand rethrow other errors so cleanup failures fail the test loudly instead of silently corrupting isolation.🛡️ Proposed fix
beforeEach(async () => { resetResumeCacheForTesting() try { await unlink(getResumeCachePath(SESSION)) - } catch { - // missing is fine + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err } }) afterEach(async () => { await flushResumeCacheWritesForTesting() resetResumeCacheForTesting() try { await unlink(getResumeCachePath(SESSION)) - } catch { - // missing is fine + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err } })
37-196: LGTM! Test cases exercise the no-op, skip-when-present, injection order, skill dedup, agent-delta merge, and malformed-attachment tolerance paths, andafterEachcorrectly flusheswriteChainbefore reset to avoid dangling writes.src/utils/cachePaths.ts (1)
21-23: LGTM!src/utils/resumeCache.ts (2)
139-241: LGTM! The malformed-attachment guards andcontentHash-based skill dedup requested in prior reviews are in place, and hydration order matches the documented turn-0 sequence.Also applies to: 243-319
63-137: 🗄️ Data Integrity & IntegrationNo resume-cache cross-process issue to raise.
The existing background-session code treats running sessions as already-active and does not start a second process against the same live
sessionId, so the module-level cache mirrors the intended single-OS-process writer per active session.> Likely an incorrect or invalid review comment.src/utils/attachments.ts (1)
1639-1730: LGTM! The suppression latch now falls back to the recovered baseline only when the in-memory scan is empty, the unchanged check compares rendered line content (not just type membership), and the latch is consumed correctly in both the unchanged and changed-set paths. This matches the regression tests inattachments.agentListingResume.test.ts.Also applies to: 2982-2987, 3010-3038
src/utils/mcpInstructionsDelta.ts (1)
46-162: LGTM! Removal now correctly covers both the disconnected case and the connected-but-instructionless case, and content changes under the same server name are re-announced via block comparison rather than name comparison.src/utils/mcpInstructionsDelta.test.ts (1)
1-108: LGTM!src/utils/conversationRecovery.ts (1)
21-25: LGTM! Restoration correctly reconstructs the recovered agent-line baseline (removals before additions) and arms the one-shot latch with it, matching the consumer logic inattachments.ts.Also applies to: 619-685
src/utils/sessionStorage.ts (1)
85-85: LGTM!updateResumeCacheFromMessagesruns on the full pre-filter message list beforecleanMessagesForLogging, matching the documented privacy boundary (listing payloads go to the local cache, not the public transcript).Also applies to: 1522-1525
src/utils/sessionStorage.listingAllowlist.test.ts (1)
1-156: LGTM! Environment mutation is properly isolated withsharedMutationLockand restored inafterEach, and coverage spans all four listing types plus the hook-context gate for both external andantusers.src/utils/attachments.agentListingResume.test.ts (1)
1-244: LGTM!
| // External transcripts omit listing deltas (privacy). Reinject from the | ||
| // local resume-cache before announced-set / latch restoration so --resume | ||
| // keeps a byte-stable API prefix for OpenAI/Moonshot automatic caching. | ||
| if (sessionId) { | ||
| messages = await hydrateListingAttachmentsFromResumeCache( | ||
| messages!, | ||
| sessionId, | ||
| ) | ||
| } else { | ||
| messages = await hydrateListingAttachmentsFromResumeCache(messages!) | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Simplify the redundant sessionId branch.
hydrateListingAttachmentsFromResumeCache declares sessionId: string = getSessionId() as a default parameter. Passing sessionId explicitly, even when it is undefined, produces the same result as omitting it. The if (sessionId) {...} else {...} split is unnecessary.
♻️ Proposed simplification
- if (sessionId) {
- messages = await hydrateListingAttachmentsFromResumeCache(
- messages!,
- sessionId,
- )
- } else {
- messages = await hydrateListingAttachmentsFromResumeCache(messages!)
- }
+ messages = await hydrateListingAttachmentsFromResumeCache(
+ messages!,
+ sessionId,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // External transcripts omit listing deltas (privacy). Reinject from the | |
| // local resume-cache before announced-set / latch restoration so --resume | |
| // keeps a byte-stable API prefix for OpenAI/Moonshot automatic caching. | |
| if (sessionId) { | |
| messages = await hydrateListingAttachmentsFromResumeCache( | |
| messages!, | |
| sessionId, | |
| ) | |
| } else { | |
| messages = await hydrateListingAttachmentsFromResumeCache(messages!) | |
| } | |
| // External transcripts omit listing deltas (privacy). Reinject from the | |
| // local resume-cache before announced-set / latch restoration so --resume | |
| // keeps a byte-stable API prefix for OpenAI/Moonshot automatic caching. | |
| if (sessionId) { | |
| messages = await hydrateListingAttachmentsFromResumeCache( | |
| messages!, | |
| sessionId, | |
| ) | |
| } else { | |
| messages = await hydrateListingAttachmentsFromResumeCache(messages!) | |
| } | |
| messages = await hydrateListingAttachmentsFromResumeCache( | |
| messages!, | |
| sessionId, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/conversationRecovery.ts` around lines 837 - 848, Remove the
redundant if/else around hydrateListingAttachmentsFromResumeCache in the
conversation recovery flow and make a single call with messages. Rely on the
function’s default sessionId parameter to obtain the session when no explicit
value is available, preserving the existing message hydration behavior.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/conversationRecovery.ts (1)
659-680: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject partial
agent_listing_deltaattachments before restoring state.This code defaults missing fields to empty arrays. A partial attachment can then change
recoveredAgentLinesand armsuppressNextAgentListing().getAgentListingDeltaAttachment()rejects the same partial schema, so a resume race can use a baseline that no valid transcript delta established.Require
addedTypes,addedLines, andremovedTypesto all be arrays before settingsawAgentListingor applying removals and additions. Add a regression test for a partial delta withremovedTypesoraddedTypesmissing.Proposed fix
if (attachment.type === 'agent_listing_delta') { - sawAgentListing = true - const addedTypes = Array.isArray(attachment.addedTypes) - ? attachment.addedTypes - : [] - const addedLines = Array.isArray(attachment.addedLines) - ? attachment.addedLines - : [] - const removedTypes = Array.isArray(attachment.removedTypes) - ? attachment.removedTypes - : [] + if ( + !Array.isArray(attachment.addedTypes) || + !Array.isArray(attachment.addedLines) || + !Array.isArray(attachment.removedTypes) + ) { + continue + } + sawAgentListing = true + const { addedTypes, addedLines, removedTypes } = attachment🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/conversationRecovery.ts` around lines 659 - 680, Update the agent_listing_delta handling in conversation recovery to validate that addedTypes, addedLines, and removedTypes are all arrays before setting sawAgentListing or mutating recoveredAgentLines. Ignore partial attachments entirely, matching getAgentListingDeltaAttachment() validation, and add a regression test covering a delta missing removedTypes or addedTypes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/utils/conversationRecovery.ts`:
- Around line 659-680: Update the agent_listing_delta handling in conversation
recovery to validate that addedTypes, addedLines, and removedTypes are all
arrays before setting sawAgentListing or mutating recoveredAgentLines. Ignore
partial attachments entirely, matching getAgentListingDeltaAttachment()
validation, and add a regression test covering a delta missing removedTypes or
addedTypes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3be1d77a-752d-4efb-8cff-bc15369ad9c5
📒 Files selected for processing (4)
src/utils/attachments.tssrc/utils/conversationRecovery.tssrc/utils/mcpInstructionsDelta.tssrc/utils/resumeCache.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/resumeCache.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/conversationRecovery.tssrc/utils/attachments.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/resumeCache.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/conversationRecovery.tssrc/utils/attachments.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/resumeCache.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/conversationRecovery.tssrc/utils/attachments.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/resumeCache.test.ts
🔇 Additional comments (4)
src/utils/resumeCache.test.ts (2)
15-35: LGTM!
15-35: 📐 Maintainability & Code QualityProvide the required validation results.
The supplied context does not include command output for the changed TypeScript behavior. Before merge, provide results for the focused resume-cache, agent-listing, and MCP tests,
bun run typecheck, andbun run typecheck:type-testswhen that script exists.As per coding guidelines, “Run the relevant TypeScript validation checks for changed code, including
bun run typecheckand, when applicable,bun run typecheck:type-tests.” As per path instructions, “Run the narrowest relevant Bun test suites plus typecheck/build checks, and document exact validation commands in the PR.”Sources: Coding guidelines, Path instructions
src/utils/attachments.ts (1)
1650-1670: LGTM!Also applies to: 3005-3012, 3121-3124
src/utils/mcpInstructionsDelta.ts (1)
60-80: LGTM!Also applies to: 108-117, 146-161
|
Addressed the latest CodeRabbit actionable items on
Targeted tests: 26 pass ( |
P1-1 Countermeasures Report: Privacy boundary for listing deltasPR: #2070 This report answers the P1-1 privacy concern raised in review: putting 1. Problem statement (what P1-1 required)These attachment types carry local catalogs, not generic UI chrome:
Privacy requirement: for non-ant ( Prefix-cache requirement (same PR, separate mechanism): on 2. Design chosen (honest chronology)Earlier commits on this PR ( Final design (
So: privacy is enforced by 3. Write path (record → local cache → public filter)In // External transcripts filter listing deltas (privacy). Persist announced
// catalogs to the local resume-cache so --resume can reinject them without
// putting sensitive payloads in the public transcript / remote ingress.
updateResumeCacheFromMessages(messages)
const cleanedMessages = cleanMessagesForLogging(messages, allMessages)Order matters and is intentional:
4. Privacy gate (what never becomes “loggable” for external)In
Consequence: Ant (first-party) sessions are unchanged: attachments remain loggable for 5. Local resume-cache (what holds the sensitive payloads)
Stored shape (
Path helper: 6. Read / resume path (hydrate without public log)
Hydrate reinjects into the in-memory message list used for continuation. It does not route those payloads through 7. Remote ingress relationship
8. Automated verification (P1-1 privacy)
Tests serialize env mutations via Additional coverage for resume behavior (agent latch / MCP / resume-cache) lives in:
Those files support the cache-stability half of the PR; the allowlist test file is the direct P1-1 privacy lock. 9. What this report does not claimTo stay accurate:
10. Summary (P1-1 status)
Bottom line: P1-1 is addressed by keeping the external privacy filter and adding a local-only resume-cache write/hydrate path. Prefix-cache stability no longer depends on shipping sensitive listing catalogs into the public transcript. |
Keep skill/agent/deferred-tools/mcp listing attachments in non-ant transcripts and suppress agent listing re-injection on --resume so automatic prefix cache is not busted.
CodeRabbit follow-up for PR Gitlawb#2070: suppressNextAgentListing only skips when the filtered agent set matches the transcript announced set; otherwise emit a normal add/remove delta. Keep all four prefix-cache listing types on the external allowlist (dropping mcp/deferred without a resume latch re-busts cache). Add unit coverage for latch and isLoggableMessage.
…script CodeRabbit follow-up: after suppressNextAgentListing is consumed, a call with messages=[] must re-emit an initial agent_listing_delta. Covers both the unchanged-set and changed-set resume paths.
Address P2 review: serialize USER_TYPE / CLAUDE_CODE_SAVE_HOOK_ADDITIONAL_CONTEXT mutations via sharedMutationLock in beforeEach/afterEach, matching other environment-mutating tests in this repo.
Persist listing deltas in a local resume-cache (not the public transcript) and reinject on --resume for prefix-cache stability. Diff MCP instruction blocks and formatAgentLine content so same-name/same-type updates after resume are re-announced.
CodeRabbit close-out —
|
| Finding | Change |
|---|---|
agent_listing_delta restore fail-closed (length + all strings) |
conversationRecovery.ts — ignore partial / mismatched / non-string addedTypes/addedLines before arming suppress |
getAnnouncedMcpInstructionBlocks same contract |
mcpInstructionsDelta.ts — same fail-closed parse |
In-REPL resume() must clear process-local sentSkillNames before restore |
prepareInReplResumeListingState(messages) = resetSentSkillNames() then restoreSkillStateFromMessages; wired in REPL.tsx |
| Extract shared subagent egress filter | filterSubagentTranscriptsForExternalEgress next to filterMessagesForExternalEgress in sessionStorage.ts; wired in submitTranscriptShare.ts + Feedback.tsx |
Outside-diff: rebuild remoteEgressOmittedParents after local resume |
Project.rebuildRemoteEgressOmittedParentsFromLocalTranscript() called from adoptResumedSessionFile() |
Obsolete / N/A on this PR
| Finding | Reason |
|---|---|
Outside-diff background-session block in REPL.tsx (~2873–3028) |
Not in this PR’s REPL delta vs main; no change required for Option A prefix-cache resume |
Tests
bun test ./src/utils/attachments.agentListingResume.test.ts ./src/utils/mcpInstructionsDelta.test.ts ./src/utils/sessionStorage.listingAllowlist.test.ts ./src/components/FeedbackSurvey/submitTranscriptShare.test.ts
bun run typecheckResult: 52 pass / 0 fail; tsc --noEmit clean.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Feedback.tsx (1)
164-180: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAdd a focused regression test for the
Feedbackupload path.
Feedback.test.tsonly covers issue-draft URL generation.submitTranscriptShare.egresscovers the survey/share code path.Add an intercepted
Feedbacksubmission test with main messages, subagent transcripts, and raw JSONL containing permission/attachment listings. Assert the postedcontentexcludes listing payloads and retains normal conversation turns.Run
bun run typecheckandbun run typecheck:type-testsafter adding the test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Feedback.tsx` around lines 164 - 180, Add a focused intercepted upload regression test for the Feedback submission flow, distinct from issue-draft and submitTranscriptShare.egress tests. Exercise main messages, subagent transcripts, and raw JSONL containing permission/attachment listings, then assert the posted content excludes those listing payloads while retaining normal conversation turns. Run bun run typecheck and bun run typecheck:type-tests afterward.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/FeedbackSurvey/submitTranscriptShare.test.ts`:
- Around line 33-40: Restore process-wide test state after execution: in
submitTranscriptShare test setup, capture the original process.env.USER_TYPE
before changing it and capture whether globalThis.MACRO existed, then add
afterAll cleanup that restores or deletes each value accordingly. Keep
postedBodies restoration in the individual test rather than shared setup, while
preserving the existing shared mutation-lock cleanup.
---
Outside diff comments:
In `@src/components/Feedback.tsx`:
- Around line 164-180: Add a focused intercepted upload regression test for the
Feedback submission flow, distinct from issue-draft and
submitTranscriptShare.egress tests. Exercise main messages, subagent
transcripts, and raw JSONL containing permission/attachment listings, then
assert the posted content excludes those listing payloads while retaining normal
conversation turns. Run bun run typecheck and bun run typecheck:type-tests
afterward.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8a85d50e-3542-4339-a594-1ed3d7a593df
📒 Files selected for processing (10)
src/components/Feedback.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/screens/REPL.tsxsrc/utils/attachments.agentListingResume.test.tssrc/utils/conversationRecovery.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/sessionStorage.listingAllowlist.test.tssrc/utils/sessionStorage.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode and ESM imports throughout the source code.
Run
bun run typecheckandbun run typecheck:type-testsfor TypeScript changes when applicable.
Files:
src/screens/REPL.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/conversationRecovery.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/sessionStorage.tssrc/utils/sessionStorage.listingAllowlist.test.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use React and Ink patterns for terminal UI components.
Files:
src/screens/REPL.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/conversationRecovery.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/sessionStorage.tssrc/utils/sessionStorage.listingAllowlist.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not add new Python code, Python provider paths, or Python dependencies without explicit maintainer approval.
**/*.{ts,tsx,js,jsx}: Follow the existing code style in touched source files, prefer small readable changes, avoid unrelated reformatting, and keep comments useful and concise.
Preserve existing repository patterns unless intentionally refactoring them, and avoid broad rewrites or unnecessary generated changes.
Review AI-assisted code for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting it.
Files:
src/screens/REPL.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/conversationRecovery.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/sessionStorage.tssrc/utils/sessionStorage.listingAllowlist.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update documentation when setup, commands, or user-facing behavior changes.
Files:
src/screens/REPL.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/conversationRecovery.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/sessionStorage.tssrc/utils/sessionStorage.listingAllowlist.test.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/screens/REPL.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/conversationRecovery.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/sessionStorage.tssrc/utils/sessionStorage.listingAllowlist.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Prefer existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Usechalkfor terminal color andexecafor child-process execution when those capabilities are needed.
Files:
src/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/conversationRecovery.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/sessionStorage.tssrc/utils/sessionStorage.listingAllowlist.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Add or update tests when behavior changes, and run the narrowest useful focused test checks.
Files:
src/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/sessionStorage.listingAllowlist.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Add or update tests when a code change affects behavior.
Use focused tests such asbun test ./path/to/test-file.test.tswhen validating a narrowly scoped change.
Files:
src/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/sessionStorage.listingAllowlist.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/sessionStorage.listingAllowlist.test.ts
🔇 Additional comments (9)
src/utils/sessionStorage.listingAllowlist.test.ts (1)
17-26: LGTM!Also applies to: 866-885, 887-935
src/components/FeedbackSurvey/submitTranscriptShare.ts (1)
14-14: LGTM!Also applies to: 44-62
src/components/FeedbackSurvey/submitTranscriptShare.test.ts (1)
140-171: LGTM!src/utils/sessionStorage.ts (1)
7-7: LGTM!Also applies to: 918-936, 1235-1280, 1948-1969, 2267-2285, 5609-5627
src/utils/conversationRecovery.ts (1)
22-22: LGTM!Also applies to: 662-681, 691-702
src/screens/REPL.tsx (1)
178-178: LGTM!Also applies to: 1985-1988
src/utils/mcpInstructionsDelta.ts (1)
65-82: LGTM!src/utils/attachments.agentListingResume.test.ts (1)
16-19: LGTM!Also applies to: 239-252, 287-334
src/utils/mcpInstructionsDelta.test.ts (1)
146-181: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Feedback.egress.test.ts`:
- Around line 76-97: Update the Feedback egress test’s upload-body assertions to
verify that content contains “feedback main turn,” ensuring the raw JSONL
fixture is included and filtered correctly. Add this assertion alongside the
existing main and subagent transcript checks.
- Around line 47-125: Update the Feedback egress test setup and teardown to
restore every module overridden by mock.module(), not only Axios. Capture stable
originals for providers, auth, HTTP, privacy, and session storage before
mocking, then re-register those originals in afterAll; apply the same cleanup
pattern to the transcript-share egress test.
In `@src/components/Feedback.tsx`:
- Around line 214-220: Update the submitReport callback’s dependency array to
include backgroundTasks, ensuring it uses the latest task transcripts when
submitting. Add a focused regression test that changes the task transcript
before submission and verifies the updated backgroundTasks are uploaded.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a700a90b-497c-4ca5-8534-d3c29c94afeb
📒 Files selected for processing (3)
src/components/Feedback.egress.test.tssrc/components/Feedback.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode and ESM imports throughout the source code.
Run
bun run typecheckandbun run typecheck:type-testsfor TypeScript changes when applicable.
Files:
src/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.tsxsrc/components/Feedback.egress.test.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use React and Ink patterns for terminal UI components.
Files:
src/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.tsxsrc/components/Feedback.egress.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Prefer existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Usechalkfor terminal color andexecafor child-process execution when those capabilities are needed.
Files:
src/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.egress.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Add or update tests when behavior changes, and run the narrowest useful focused test checks.
Files:
src/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.egress.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not add new Python code, Python provider paths, or Python dependencies without explicit maintainer approval.
**/*.{ts,tsx,js,jsx}: Follow the existing code style in touched source files, prefer small readable changes, avoid unrelated reformatting, and keep comments useful and concise.
Preserve existing repository patterns unless intentionally refactoring them, and avoid broad rewrites or unnecessary generated changes.
Review AI-assisted code for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting it.
Files:
src/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.tsxsrc/components/Feedback.egress.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Add or update tests when a code change affects behavior.
Use focused tests such asbun test ./path/to/test-file.test.tswhen validating a narrowly scoped change.
Files:
src/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.egress.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update documentation when setup, commands, or user-facing behavior changes.
Files:
src/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.tsxsrc/components/Feedback.egress.test.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.tsxsrc/components/Feedback.egress.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/components/FeedbackSurvey/submitTranscriptShare.test.tssrc/components/Feedback.egress.test.ts
🔇 Additional comments (2)
src/components/Feedback.tsx (1)
67-132: LGTM!Also applies to: 167-167, 511-512
src/components/FeedbackSurvey/submitTranscriptShare.test.ts (1)
14-16: LGTM!Also applies to: 39-41, 131-140
| const realProviders = await import('../utils/model/providers.js') | ||
| mock.module('../utils/model/providers.js', () => ({ | ||
| ...realProviders, | ||
| getAPIProvider: () => 'firstParty', | ||
| isFirstPartyAnthropicBaseUrl: () => true, | ||
| })) | ||
|
|
||
| const realAuth = await import('../utils/auth.js') | ||
| mock.module('../utils/auth.js', () => ({ | ||
| ...realAuth, | ||
| checkAndRefreshOAuthTokenIfNeeded: async () => {}, | ||
| })) | ||
|
|
||
| const realHttp = await import('../utils/http.js') | ||
| mock.module('../utils/http.js', () => ({ | ||
| ...realHttp, | ||
| getAuthHeaders: () => ({ | ||
| headers: { Authorization: 'Bearer test' }, | ||
| error: undefined, | ||
| }), | ||
| getUserAgent: () => 'test-agent', | ||
| })) | ||
|
|
||
| const realPrivacy = await import('../utils/privacyLevel.js') | ||
| mock.module('../utils/privacyLevel.js', () => ({ | ||
| ...realPrivacy, | ||
| isEssentialTrafficOnly: () => false, | ||
| })) | ||
|
|
||
| tempDir = await mkdtemp(join(tmpdir(), 'openclaude-feedback-egress-')) | ||
| const transcriptPath = join(tempDir, 'session.jsonl') | ||
| await writeFile( | ||
| transcriptPath, | ||
| `${JSON.stringify({ | ||
| type: 'user', | ||
| uuid: '00000000-0000-4000-8000-00000000f001', | ||
| parentUuid: null, | ||
| timestamp: '2026-08-07T00:00:00.000Z', | ||
| message: { role: 'user', content: 'feedback main turn' }, | ||
| })}\n${JSON.stringify({ | ||
| type: 'attachment', | ||
| uuid: '00000000-0000-4000-8000-00000000f002', | ||
| parentUuid: '00000000-0000-4000-8000-00000000f001', | ||
| timestamp: '2026-08-07T00:00:00.000Z', | ||
| attachment: { | ||
| type: 'skill_listing', | ||
| content: 'Available skills:\n- /leak-me-please', | ||
| skillCount: 1, | ||
| isInitial: true, | ||
| }, | ||
| })}\n`, | ||
| ) | ||
|
|
||
| const realSession = await import('../utils/sessionStorage.js') | ||
| mock.module('../utils/sessionStorage.js', () => ({ | ||
| ...realSession, | ||
| getTranscriptPath: () => transcriptPath, | ||
| loadAllSubagentTranscriptsFromDisk: async () => ({ | ||
| 'agent-leak': [ | ||
| { | ||
| type: 'attachment', | ||
| uuid: '00000000-0000-4000-8000-00000000a201', | ||
| attachment: { | ||
| type: 'agent_listing_delta', | ||
| addedTypes: ['Explore'], | ||
| addedLines: ['- Explore: /leak-agent-listing'], | ||
| removedTypes: [], | ||
| isInitial: true, | ||
| showConcurrencyNote: false, | ||
| }, | ||
| }, | ||
| { | ||
| type: 'user', | ||
| uuid: '00000000-0000-4000-8000-00000000a202', | ||
| message: { role: 'user', content: 'subagent turn' }, | ||
| }, | ||
| ], | ||
| }), | ||
| })) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
.set -euo pipefail
echo "== repo files of interest =="
git ls-files | rg '(^|/)(Feedback\.egress\.test\.ts|submitTranscriptShare\.test\.ts|AGENTS\.md)$|package\.json$' || true
echo
echo "== target test excerpt =="
if [ -f src/components/Feedback.egress.test.ts ]; then
wc -l src/components/Feedback.egress.test.ts
sed -n '1,230p' src/components/Feedback.egress.test.ts
fi
echo
echo "== related test excerpt =="
if [ -f src/components/FeedbackSurvey/submitTranscriptShare.test.ts ]; then
wc -l src/components/FeedbackSurvey/submitTranscriptShare.test.ts
sed -n '1,220p' src/components/FeedbackSurvey/submitTranscriptShare.test.ts
fi
echo
echo "== mock.module patterns in tests =="
rg -n "mock\.module|mock\.restore|afterAll|beforeAll|process\.exitCode|globalTestId|describe|it\(" src/components src/components/FeedbackSurvey src -g '*.test.ts' | head -200Repository: Gitlawb/openclaude
Length of output: 31534
🌐 Web query:
Bun test mock.module module cache restore module override mock restore documentation
💡 Result:
In Bun's test runner, mock.module() is used to override the behavior of a module, replacing its exports with the result of a factory function [1][2]. Regarding the restoration of module mocks: 1. Restoration Behavior: While mock.restore() is designed to restore mocks to their original implementation, historically it primarily targeted function spies/mocks [3][4]. Recent updates have improved mock.restore() to better handle mock.module() overrides, specifically by reverting module namespaces and cache entries [5][6]. 2. Preload Survival: Mocks created using mock.module() during a --preload phase are considered persistent [5]. These are designed to survive calls to mock.restore(), allowing developers to keep global test setup intact while resetting other mocks between tests [5][7]. 3. Recommended Pattern: The standard practice is to use afterEach(mock.restore) in your test files to clean up transient mocks [4][5]. If you encounter issues with mocks persisting between tests, ensure you are not relying on side effects from --preload and that your environment is utilizing recent versions of Bun that support proper mock.module() eviction [5][7]. 4. Alternative Strategy: If mock.restore() does not meet your specific requirements, you can scope mocks to individual files by using bun test --isolate (experimental), which provides a fresh environment per test file, preventing cross-file mock leakage [4]. In summary, for most use cases, mock.restore() is the intended method for cleanup [5]. If transient module mocks are not being cleared as expected, verify your Bun version and consider the afterEach pattern [4][5].
Citations:
- 1: https://bun.sh/docs/test/mocks
- 2: https://bun.com/docs/test/mocks
- 3: https://github.com/oven-sh/bun/blob/74e191b3/packages/bun-types/test.d.ts
- 4: Restore mock.module using mock.restore not work as expect oven-sh/bun#7823
- 5: bun:test: make mock.restore() revert mock.module() overrides oven-sh/bun#35669
- 6: fix:
mock.restore()properly handlesmock.module()oven-sh/bun#18171 - 7: bun test: scope mock.module() to the test file that registered it oven-sh/bun#31319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Feedback.egress cleanup mock presence =="
sed -n '130,155p' src/components/Feedback.egress.test.ts
echo
echo "== submission test cleanup mock presence =="
sed -n '138,150p' src/components/FeedbackSurvey/submitTranscriptShare.test.ts
echo
echo "== nearby cleanup patterns documenting process-wide mock.module"
sed -n '1,40p' src/components/ModelPicker.switchProfile.test.ts
sed -n '65,80p' src/hooks/fileSuggestions.test.ts
echo
echo "== test runner config =="
if [ -f package.json ]; then
node -e "const p=require('./package.json'); console.log(JSON.stringify({scripts:p.scripts, devDependencies:p.devDependencies?.bun?:" (not available to avoid parse crash?)"}, null, 2))"
fi
echo
echo "== imports of affected tests in other tests =="
rg -n "Feedback\.egress|submitTranscriptShare|mock\.restore\(" src -g '*.test.ts' | head -120Repository: Gitlawb/openclaude
Length of output: 3909
Restore the non-axios mock.module() overrides.
beforeAll() registers modules for providers, auth, HTTP, privacy, session storage, and Axios, but afterAll() restores only Axios. Bun keeps mock.module() overrides process-wide unless each override is explicitly remapped back to its captured original module. Capture stable module values for all overridden modules before mocking, and re-register each original module in afterAll. The same cleanup applies to the transcript-share egress test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Feedback.egress.test.ts` around lines 47 - 125, Update the
Feedback egress test setup and teardown to restore every module overridden by
mock.module(), not only Axios. Capture stable originals for providers, auth,
HTTP, privacy, and session storage before mocking, then re-register those
originals in afterAll; apply the same cleanup pattern to the transcript-share
egress test.
Source: Path instructions
CodeRabbit close-out (complete)HEAD:
TestsNo unfinished Outside-diff items left open on this review round. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Rebuild remote egress omission map on print-mode resume when remote persistence is active
src/cli/print.ts:5163
This PR addsremoteEgressOmittedParentsand rebuilds it from adopted local JSONL insideadoptResumedSessionFile()(sessionStorage.ts:2264-2270). Interactive resume paths call adopt afterresetSessionFilePointer();openclaude -p --resume/-p --continueonly reset the pointer (print.ts:5163,5401) and never rebuild. That gap was latent onmainbecause external users did not persist listing attachments locally; this PR makes local JSONL parent chains reference withheld listing UUIDs, so the omission map matters on resume.Scope is narrower than “all print resume”:
persistToRemoteis a no-op unless CCR v2’sinternalEventWriteris registered or bothENABLE_SESSION_PERSISTENCEandremoteIngressUrlare set (sessionStorage.ts:2042-2047). Plain local UUID print resume without URL/CCR hydrate often never hits remote persistence. The failure is real on paths that do — e.g. print resume from local JSONL that still contains listing attachments while CCR v2 or session-ingress persistence is enabled — where the first post-resume remote append can ship aparentUuidpointing at a listing UUID never stored remotely. URL-based hydrate writes egress-filtered remote logs (no listings), so an empty omission map is usually correct there.Please call
adoptResumedSessionFile()(or equivalent rebuild) on print resume whenever persistence can reach remote/CCR, and add a focused test: local JSONL with a listing attachment, resume via print, assert the next remote append is reparented. -
[P2] Hold
deferred_tools_deltaremovals until the deferred tool pool has settled on resume
src/utils/toolSearch.ts:750
The removal loop here is unchanged frommain; this PR newly makes it reachable for external users by persistingdeferred_tools_deltain local JSONL. Runtime check on head: transcript announcesmcp__docs__search,toolsis still[], andgetDeferredToolsDeltareturnsremovedNames: ['mcp__docs__search']. That is the same resume race class already fixed in this PR formcp_instructions_deltaand MCP-gated agent listings.Reach is feature-gated for externals:
getDeferredToolsDeltaAttachmentonly runs whenisDeferredToolsDeltaEnabled()is true (USER_TYPE === 'ant'or GrowthBooktengu_glacier_2xr). When that gate is on and MCP tools join after the first attachment pass, a false removal followed by re-add is another mid-history listing rewrite and prefix-cache bust. Please mirror the MCP/agent hold logic for deferred tools and add resume-race coverage under the enabled gate. -
[P3] Treat
needs-authMCP clients as unsettled for instruction removals
src/utils/mcpInstructionsDelta.ts:171
This PR correctly holds removals whilemcpClientsis empty or a server is stillpending(mcpInstructionsDelta.test.ts).clientSetSettledForRemovalsalso becomes true when the only client isneeds-auth(type !== 'pending'), andserverStillPendingdoes not include that state. Runtime check on head: announceddocswithmcpClients = [{ type: 'needs-auth', name: 'docs' }]returnsremovedNames: ['docs']whilemcpClients: []correctly returnsnull. Auth-window spurious removal/re-add is a narrower edge than empty/pending, but it is the same delta churn class. Please hold name-based removals while the required server ispendingorneeds-auth, and add a focused test.
Call adoptResumedSessionFile after reset on print -p --continue/--resume so remoteEgressOmittedParents rebuilds before the first CCR append (Finding 1). Add contract tests: happy path, no-adopt negative control, local vs remote asymmetry, multi-listing chain.
[P2] Rebuild remote egress omission map on print-mode resume when remote persistence is active — closedHEAD: What this P2 isPrint-mode After this PR persists listing attachments locally, those listings stay in the local Scope jatmn called out: when remote persistence can run (CCR v2 Code change
Same order as interactive resume: reset → restore metadata → adopt ( Test commandbun test ./src/utils/sessionStorage.listingAllowlist.test.tsDetailed resultsCoverage for this P2
Remaining jatmn items (not in this push — submitting next)
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/sessionStorage.listingAllowlist.test.ts`:
- Line 952: Update enablePrintResumePersistenceForTest to save and restore
process.env.USER_TYPE alongside its existing state, then route all four affected
tests through the helper without direct USER_TYPE assignments:
src/utils/sessionStorage.listingAllowlist.test.ts lines 952, 1148, 1230, and
1321. Preserve each test’s external-user behavior while ensuring the environment
variable is restored after every test.
- Around line 953-1046: Update this test to reuse the existing
enablePrintResumePersistenceForTest setup and restore helpers, and replace the
inline post-resume assistant message construction with
postResumeAssistantMessage. Preserve the test’s existing assertions and flow
while removing the duplicated environment setup, cleanup, and message
definition.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e338b432-1fd0-46e2-96f7-092fce18bd36
📒 Files selected for processing (2)
src/cli/print.tssrc/utils/sessionStorage.listingAllowlist.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode and ESM imports throughout the source code.
Run
bun run typecheckandbun run typecheck:type-testsfor TypeScript changes when applicable.
Files:
src/cli/print.tssrc/utils/sessionStorage.listingAllowlist.test.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use React and Ink patterns for terminal UI components.
Files:
src/cli/print.tssrc/utils/sessionStorage.listingAllowlist.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Prefer existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Usechalkfor terminal color andexecafor child-process execution when those capabilities are needed.
Files:
src/cli/print.tssrc/utils/sessionStorage.listingAllowlist.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not add new Python code, Python provider paths, or Python dependencies without explicit maintainer approval.
**/*.{ts,tsx,js,jsx}: Follow the existing code style in touched source files, prefer small readable changes, avoid unrelated reformatting, and keep comments useful and concise.
Preserve existing repository patterns unless intentionally refactoring them, and avoid broad rewrites or unnecessary generated changes.
Review AI-assisted code for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting it.
Files:
src/cli/print.tssrc/utils/sessionStorage.listingAllowlist.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update documentation when setup, commands, or user-facing behavior changes.
Files:
src/cli/print.tssrc/utils/sessionStorage.listingAllowlist.test.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/cli/print.tssrc/utils/sessionStorage.listingAllowlist.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Add or update tests when behavior changes, and run the narrowest useful focused test checks.
Files:
src/utils/sessionStorage.listingAllowlist.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Add or update tests when a code change affects behavior.
Use focused tests such asbun test ./path/to/test-file.test.tswhen validating a narrowly scoped change.
Files:
src/utils/sessionStorage.listingAllowlist.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/sessionStorage.listingAllowlist.test.ts
🔇 Additional comments (3)
src/utils/sessionStorage.listingAllowlist.test.ts (3)
1005-1005:switchSession(sessionId as never, dir)still uses anas nevercast. Preferas unknown as SessionId.
1389-1392: LGTM!
1295-1300: 🎯 Functional CorrectnessNo duplicate
JSON.parsecall is present.The chain runs
JSON.parse(line)only once before.find(...), so this concern does not apply.> Likely an incorrect or invalid review comment.
CodeRabbit review
|
| Actionable | Change |
|---|---|
USER_TYPE leak in four print-resume tests (discussion) |
enablePrintResumePersistenceForTest now saves/restores process.env.USER_TYPE (sets external). Direct USER_TYPE assignments removed from all four print-resume tests. |
| Deduplicate first print-resume test setup (discussion) | Same test now uses enablePrintResumePersistenceForTest + postResumeAssistantMessage + persist.restore() instead of inline env/message/cleanup. |
Tests: bun test ./src/utils/sessionStorage.listingAllowlist.test.ts — 23 pass.
Finding 2 (jatmn P2) — deferred_tools_delta resume holdHEAD: ChangeHold
Wired TestsCloses jatmn review Finding 2 / P2 on toolSearch removal loop. |
Add alternate-angle cases (all-pending MCP, undeferred silent, two-pass resume, reappear after settle, ant attachment wrapper). Align comments with jatmn [P2] wording.
Addressed: jatmn [P2] — deferred tools hold + expanded resume-race testsPR: #2070 Code (already on tip from
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/deferredToolsDelta.resumeRace.test.ts`:
- Around line 125-135: Update getDeferredToolsDelta so MCP clients with a
needs-auth status are treated as unsettled alongside pending clients, preventing
removals until authorization is resolved. Add a focused regression test in the
deferred-tools resume race tests using a needs-auth client and assert the result
is null, while preserving the existing pending-client behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b183b241-565a-4e62-8771-29b1abfb50c3
📒 Files selected for processing (1)
src/utils/deferredToolsDelta.resumeRace.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode and ESM imports throughout the source code.
Run
bun run typecheckandbun run typecheck:type-testsfor TypeScript changes when applicable.
Files:
src/utils/deferredToolsDelta.resumeRace.test.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use React and Ink patterns for terminal UI components.
Files:
src/utils/deferredToolsDelta.resumeRace.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Prefer existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Usechalkfor terminal color andexecafor child-process execution when those capabilities are needed.
Files:
src/utils/deferredToolsDelta.resumeRace.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Add or update tests when behavior changes, and run the narrowest useful focused test checks.
Files:
src/utils/deferredToolsDelta.resumeRace.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not add new Python code, Python provider paths, or Python dependencies without explicit maintainer approval.
**/*.{ts,tsx,js,jsx}: Follow the existing code style in touched source files, prefer small readable changes, avoid unrelated reformatting, and keep comments useful and concise.
Preserve existing repository patterns unless intentionally refactoring them, and avoid broad rewrites or unnecessary generated changes.
Review AI-assisted code for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting it.
Files:
src/utils/deferredToolsDelta.resumeRace.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Add or update tests when a code change affects behavior.
Use focused tests such asbun test ./path/to/test-file.test.tswhen validating a narrowly scoped change.
Files:
src/utils/deferredToolsDelta.resumeRace.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update documentation when setup, commands, or user-facing behavior changes.
Files:
src/utils/deferredToolsDelta.resumeRace.test.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/deferredToolsDelta.resumeRace.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/deferredToolsDelta.resumeRace.test.ts
🔇 Additional comments (1)
src/utils/deferredToolsDelta.resumeRace.test.ts (1)
1-18: LGTM!Also applies to: 59-121, 137-211
| test('holds MCP removals while every MCP client is still pending (sibling of instructions hold)', () => { | ||
| const messages = [ | ||
| deltaMessage(['mcp__docs__search'], ['mcp__docs__search']), | ||
| ] | ||
| const tools = [{ name: 'SomeBuiltin', isMcp: false }] as unknown as Tools | ||
| const delta = getDeferredToolsDelta(tools, messages, undefined, [ | ||
| pending('docs'), | ||
| pending('other'), | ||
| ]) | ||
| expect(delta).toBeNull() | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep needs-auth MCP clients unsettled.
This test covers only pending clients. getDeferredToolsDelta currently treats every non-pending client as settled, so a needs-auth client can emit a removal while its MCP server still requires authorization. Add a needs-auth regression case that expects null, and exclude needs-auth from the settlement predicate.
As per coding guidelines, add or update tests when behavior changes. As per path instructions, block risky runtime changes that lack focused regression coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/deferredToolsDelta.resumeRace.test.ts` around lines 125 - 135,
Update getDeferredToolsDelta so MCP clients with a needs-auth status are treated
as unsettled alongside pending clients, preventing removals until authorization
is resolved. Add a focused regression test in the deferred-tools resume race
tests using a needs-auth client and assert the result is null, while preserving
the existing pending-client behavior.
Sources: Coding guidelines, Path instructions
Hold deferred_tools_delta, mcp_instructions_delta, and agent-listing removals while a required MCP client is needs-auth (same as pending). Add regression tests; Feedback submitReport deps + egress assert.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/toolSearch.ts (1)
750-755: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winApply deferred-tool removals before additions.
Lines 750-755 add names before they remove names. If one valid delta removes and re-adds the same tool name, reconstruction leaves that tool absent. The next pass then emits an unnecessary listing delta.
Process
removedNamesfirst. Then processaddedNames. Add a focused regression test with the same name in both arrays.As per coding guidelines, add or update tests when behavior changes. As per path instructions, preserve regression coverage for changed resume behavior.
Proposed fix
- for (const n of addedNames) { - announced.add(n) - } for (const n of removedNames) { announced.delete(n) } + for (const n of addedNames) { + announced.add(n) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/toolSearch.ts` around lines 750 - 755, In the deferred-tool reconstruction loop, process every name in removedNames before processing names in addedNames so a name present in both arrays remains announced. Add a focused regression test covering the same tool name in both deltas and verify the next pass emits no unnecessary listing delta.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Feedback.egress.test.ts`:
- Line 192: Run the required validation checks for the Feedback changes: bun
test ./src/components/Feedback.egress.test.ts, bun run typecheck, bun run
typecheck:type-tests, and the repository-defined build check in AGENTS.md.
Report the exact commands and their results before merge.
In `@src/components/Feedback.tsx`:
- Line 242: Update the dependency array for the submitReport callback to include
abortSignal alongside the existing dependencies, ensuring both submission and
title generation use the current signal when it changes.
---
Outside diff comments:
In `@src/utils/toolSearch.ts`:
- Around line 750-755: In the deferred-tool reconstruction loop, process every
name in removedNames before processing names in addedNames so a name present in
both arrays remains announced. Add a focused regression test covering the same
tool name in both deltas and verify the next pass emits no unnecessary listing
delta.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bd55f7f7-89a1-4c6c-b7f2-7e61a6cdc884
📒 Files selected for processing (9)
src/components/Feedback.egress.test.tssrc/components/Feedback.tsxsrc/services/mcp/types.tssrc/utils/attachments.agentListingResume.test.tssrc/utils/attachments.tssrc/utils/deferredToolsDelta.resumeRace.test.tssrc/utils/mcpInstructionsDelta.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/toolSearch.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode and ESM imports throughout the source code.
Run
bun run typecheckandbun run typecheck:type-testsfor TypeScript changes when applicable.
Files:
src/utils/toolSearch.tssrc/utils/deferredToolsDelta.resumeRace.test.tssrc/utils/attachments.agentListingResume.test.tssrc/services/mcp/types.tssrc/components/Feedback.tsxsrc/utils/attachments.tssrc/components/Feedback.egress.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/mcpInstructionsDelta.test.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use React and Ink patterns for terminal UI components.
Files:
src/utils/toolSearch.tssrc/utils/deferredToolsDelta.resumeRace.test.tssrc/utils/attachments.agentListingResume.test.tssrc/services/mcp/types.tssrc/components/Feedback.tsxsrc/utils/attachments.tssrc/components/Feedback.egress.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/mcpInstructionsDelta.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Prefer existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Usechalkfor terminal color andexecafor child-process execution when those capabilities are needed.
Files:
src/utils/toolSearch.tssrc/utils/deferredToolsDelta.resumeRace.test.tssrc/utils/attachments.agentListingResume.test.tssrc/services/mcp/types.tssrc/utils/attachments.tssrc/components/Feedback.egress.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/mcpInstructionsDelta.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not add new Python code, Python provider paths, or Python dependencies without explicit maintainer approval.
**/*.{ts,tsx,js,jsx}: Follow the existing code style in touched source files, prefer small readable changes, avoid unrelated reformatting, and keep comments useful and concise.
Preserve existing repository patterns unless intentionally refactoring them, and avoid broad rewrites or unnecessary generated changes.
Review AI-assisted code for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting it.
Files:
src/utils/toolSearch.tssrc/utils/deferredToolsDelta.resumeRace.test.tssrc/utils/attachments.agentListingResume.test.tssrc/services/mcp/types.tssrc/components/Feedback.tsxsrc/utils/attachments.tssrc/components/Feedback.egress.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/mcpInstructionsDelta.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update documentation when setup, commands, or user-facing behavior changes.
Files:
src/utils/toolSearch.tssrc/utils/deferredToolsDelta.resumeRace.test.tssrc/utils/attachments.agentListingResume.test.tssrc/services/mcp/types.tssrc/components/Feedback.tsxsrc/utils/attachments.tssrc/components/Feedback.egress.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/mcpInstructionsDelta.test.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/toolSearch.tssrc/utils/deferredToolsDelta.resumeRace.test.tssrc/utils/attachments.agentListingResume.test.tssrc/services/mcp/types.tssrc/components/Feedback.tsxsrc/utils/attachments.tssrc/components/Feedback.egress.test.tssrc/utils/mcpInstructionsDelta.tssrc/utils/mcpInstructionsDelta.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Add or update tests when behavior changes, and run the narrowest useful focused test checks.
Files:
src/utils/deferredToolsDelta.resumeRace.test.tssrc/utils/attachments.agentListingResume.test.tssrc/components/Feedback.egress.test.tssrc/utils/mcpInstructionsDelta.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Add or update tests when a code change affects behavior.
Use focused tests such asbun test ./path/to/test-file.test.tswhen validating a narrowly scoped change.
Files:
src/utils/deferredToolsDelta.resumeRace.test.tssrc/utils/attachments.agentListingResume.test.tssrc/components/Feedback.egress.test.tssrc/utils/mcpInstructionsDelta.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/deferredToolsDelta.resumeRace.test.tssrc/utils/attachments.agentListingResume.test.tssrc/components/Feedback.egress.test.tssrc/utils/mcpInstructionsDelta.test.ts
src/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use existing service and provider integration patterns when implementing API, MCP, OAuth, wiki, voice, or related integrations.
Files:
src/services/mcp/types.ts
src/{skills,utils/plugins,services/mcp}/**
⚙️ CodeRabbit configuration file
src/{skills,utils/plugins,services/mcp}/**: Review skill/plugin/MCP behavior as a trust boundary. Check registry fetches, local and remote installs, path normalization, hash verification, revocation/trust metadata, tools_required handling, config-home behavior, and startup-time loading. Block on path traversal risk, unverified downloads, silent trust promotion, or unexpected code/tool activation.
Files:
src/services/mcp/types.ts
🔇 Additional comments (8)
src/utils/attachments.ts (1)
190-193: LGTM!Also applies to: 1741-1759
src/services/mcp/types.ts (1)
228-237: LGTM!src/utils/mcpInstructionsDelta.ts (1)
3-6: LGTM!Also applies to: 166-187
src/utils/attachments.agentListingResume.test.ts (1)
436-480: LGTM!src/utils/mcpInstructionsDelta.test.ts (1)
93-104: LGTM!Also applies to: 127-144
src/utils/deferredToolsDelta.resumeRace.test.ts (1)
59-65: LGTM!Also applies to: 109-133
src/components/Feedback.tsx (1)
28-28: LGTM!Also applies to: 67-132, 167-167, 214-220, 511-512
src/components/Feedback.egress.test.ts (1)
1-156: LGTM!Also applies to: 158-196
| expect(postedBodies).toHaveLength(1) | ||
| const content = postedBodies[0]?.content ?? '' | ||
| expect(content).toContain('plain turn') | ||
| expect(content).toContain('feedback main turn') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial
Run the required checks before merge.
No test, typecheck, or build results are included in this review. Run:
bun test ./src/components/Feedback.egress.test.tsbun run typecheckbun run typecheck:type-tests- The repository-defined build check from
AGENTS.md
As per coding guidelines, “Run bun run typecheck and bun run typecheck:type-tests for TypeScript changes when applicable.” As per path instructions, “Run narrow Bun tests plus typecheck/build checks and report exact commands.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Feedback.egress.test.ts` at line 192, Run the required
validation checks for the Feedback changes: bun test
./src/components/Feedback.egress.test.ts, bun run typecheck, bun run
typecheck:type-tests, and the repository-defined build check in AGENTS.md.
Report the exact commands and their results before merge.
Sources: Coding guidelines, Path instructions
Bot close-out (
|
| Item | Change |
|---|---|
| [P3] Treat needs-auth MCP clients as unsettled for instruction removals | isMcpClientUnsettledForRemovals (pending | needs-auth) in src/services/mcp/types.ts; used by getMcpInstructionsDelta, getDeferredToolsDelta, and agent-listing hold in attachments.ts |
CodeRabbit: getDeferredToolsDelta needs-auth regression |
Tests in deferredToolsDelta.resumeRace.test.ts |
Feedback submitReport deps |
backgroundTasks added to useCallback deps |
| Feedback egress body assert | Posted content must include feedback main turn |
Tests
bun test src/utils/deferredToolsDelta.resumeRace.test.ts \
src/utils/mcpInstructionsDelta.test.ts \
src/utils/attachments.agentListingResume.test.ts \
src/components/Feedback.egress.test.ts
→ 49 pass / 0 fail
N/A (already on HEAD from prior commits)
- Print-resume
USER_TYPErestore / helper inlining — covered by5f9fd15d - Recovered-baseline corrective agent-listing delta — already in
attachments.agentListingResume.test.ts - Malformed / partial delta parser hardening — already on branch
…ignal in Feedback deps
Bot close-out —
|
| Item | Change |
|---|---|
Outside-diff src/utils/toolSearch.ts (~750–755) — process removedNames before addedNames so a name in both stays announced |
Reconstruction loop now removes first, then adds. Regression: reconstruction applies removals before additions for same name (CodeRabbit outside-diff) in deferredToolsDelta.resumeRace.test.ts |
Inline Feedback.tsx — abortSignal missing from submitReport useCallback deps |
Deps: [abortSignal, backgroundTasks, description, envInfo.isGit, messages] |
Inline Feedback.egress.test.ts — run required checks and report |
Ran (see below) |
Checks run (this turn / prior same close-out)
bun test ./src/utils/deferredToolsDelta.resumeRace.test.ts \
./src/utils/deferredToolsDelta.malformed.test.ts \
./src/components/Feedback.egress.test.ts \
./src/utils/mcpInstructionsDelta.test.ts \
./src/utils/attachments.agentListingResume.test.ts
→ 56 pass / 0 fail
No open “in progress” items remaining for review 4888222038.
[P3] Treat
|
| Item | Status |
|---|---|
needs-auth unsettled for instruction removals |
Done (isMcpClientUnsettledForRemovals) |
Empty mcpClients still holds (null / no attachment) |
Done |
Mixed: connected other + needs-auth docs does not remove docs |
Done |
| Focused + alternate-angle tests | Done |
Detailed test results (just re-run)
bun test \
./src/utils/mcpInstructionsDelta.test.ts \
./src/utils/mcpInstructionsDelta.needsAuthAngle.test.ts \
./src/utils/deferredToolsDelta.resumeRace.test.ts \
./src/utils/attachments.agentListingResume.test.ts
→ 58 pass / 0 fail
116 expect() calls
Ran 58 tests across 4 files. [1092.00ms]
A. Direct getMcpInstructionsDelta (includes jatmn [P3] cases)
| Test | Result |
|---|---|
| does not remove when mcpClients empty | pass |
| does not remove while every client is still pending | pass |
| does not remove while every client needs-auth (jatmn [P3]) | pass |
| does not remove docs while docs is pending and other is connected | pass |
| does not remove docs while docs needs-auth and other is connected | pass |
| removes disconnected servers once client set is settled | pass |
| (+ remaining mcpInstructionsDelta unit cases) | pass |
B. Alternate angles (mcpInstructionsDelta.needsAuthAngle.test.ts) — 9/9 pass
| Angle | Test | Result |
|---|---|---|
| Production attachment wrapper | needs-auth docs alone → no mcp_instructions_delta |
pass |
| Production attachment wrapper | empty mcpClients → no removal attachment | pass |
| Production attachment wrapper | settled other + missing docs → removal attachment | pass |
| Old buggy predicate contrast | type !== 'pending' would remove needs-auth docs; current holds |
pass |
| Old buggy predicate contrast | mixed connected + needs-auth: old removes docs; current holds | pass |
| Two-pass resume | needs-auth hold → same-name connected does not remove | pass |
| Two-pass resume | needs-auth hold → only other connected removes docs | pass |
| Settled contrast | failed docs alone authorizes removal | pass |
| Settled contrast | disabled docs alone authorizes removal | pass |
C. Sibling resume-hold paths (same unsettled helper)
| Suite | Notable [P3]-related cases | Result |
|---|---|---|
deferredToolsDelta.resumeRace.test.ts |
holds MCP removals while server needs-auth; every client needs-auth | pass (14) |
attachments.agentListingResume.test.ts |
holds MCP-gated agent removals while required server needs-auth | pass (22) |
Runtime check (jatmn scenario)
| Input | Result |
|---|---|
announced docs + [{ type: 'needs-auth', name: 'docs' }] |
null (no removedNames: ['docs']) |
mcpClients: [] |
null |
Bot / maintainer close-out —
|
| Finding | Change on HEAD |
|---|---|
| jatmn [P2] Rebuild remote egress omission map on print-mode resume | src/cli/print.ts — adoptResumedSessionFile() after resume paths so remoteEgressOmittedParents is rebuilt against the resumed JSONL |
jatmn [P2] Hold deferred_tools_delta removals until tool / MCP pool settled |
Hold gates + resume-race / attachment-wrapper tests in deferredToolsDelta.resumeRace.test.ts |
jatmn [P3] Treat needs-auth MCP clients as unsettled for resume removals |
isMcpClientUnsettledForRemovals in src/services/mcp/types.ts; instructions + deferred-tools hold; alternate-angle tests in mcpInstructionsDelta.needsAuthAngle.test.ts |
| CodeRabbit Process deferred_tools removals before additions (same name) | src/utils/toolSearch.ts — apply removedNames then addedNames; covered in resume-race test |
CodeRabbit Feedback abortSignal dependency |
src/components/Feedback.tsx — abortSignal included in effect deps |
Tip since prior comment (3fb14c4c)
e770e64e— alternate-angle coverage for MCPneeds-authinstruction hold [P3] (attachment wrapper, two-pass resume, failed/disabled contrast, old-predicate regression)
Obsolete / not a #2070 actionable
- Soft “Possibly related to fix(code-reviewer): require inline diff input and preserve read-only search in embedded-search builds #2102” — informational only; no additional change required on this PR.
Validation
bun test src/utils/mcpInstructionsDelta.needsAuthAngle.test.ts \
src/utils/mcpInstructionsDelta.test.ts \
src/utils/deferredToolsDelta.resumeRace.test.ts \
src/components/Feedback.egress.test.ts
→ 37 pass, 0 fail
Ready for re-review.
|
Closing this PR rather than requesting another patch round. The underlying report is valid: local resume needs to preserve the same model-visible ordered history when prefix caching depends on it. But this branch is no longer a reviewable fix for that problem. It has accumulated serial follow-ups across transcript privacy, remote/CCR persistence, feedback and sharing, MCP lifecycle policy, compaction, print-mode resume, and an unrelated web release test. Continuing to apply individual review-bot suggestions here has made the design and regression surface too broad to merge safely. Please do not keep amending this branch. Reopen the work as a small ordered series, with each PR independently reviewable and revertible:
Explicitly leave For future PRs, treat cross-cutting automated-review suggestions as candidates for a follow-up issue/PR unless they are necessary to the narrow contract under review. A large test matrix does not make an otherwise cross-cutting patch focused. |
3 participants
Summary
Fix catastrophic automatic prefix-cache busts on
--resumefor OpenAI-compatible providers that cache by byte-stable leadingmessagesprefix (most visibly Moonshot / Kimi).Root cause: for non-
antusers,isLoggableMessagedropped almost allattachmenttypes from the transcript. Turn-0 listing attachments (skill_listing,agent_listing_delta,deferred_tools_delta,mcp_instructions_delta) were therefore missing after resume, so the next turn re-injected agent/skill catalogs into the wire history. That rewrote earlymessages[N]content → shared prefix collapsed to ~messages[0]→ Moonshot reportedcached_tokens → 0on the first post-resume call.This PR:
suppressNextSkillListingpattern withsuppressNextAgentListingso resume does not re-announce agents when the delta is already in the transcript.Why this is Moonshot-shaped (but not Moonshot-only)
cached_tokens/prompt_tokens_details.cached_tokensantcache_controlbreakpointsgetUserType() === 'ant'still logs all attachments as beforetoolsEqual: truebefore and after)So the fix is scoped to transcript fidelity for listing deltas — the same invariant ants already enjoy — not a Moonshot-special-case API quirk or a provider
#ifdef.Privacy / training-surface consistency
The historical filter comment still stands: most attachments stay out of public transcripts.
Allowed through for non-ants (this PR only):
skill_listingagent_listing_deltadeferred_tools_deltamcp_instructions_deltaThese carry agent/skill/tool catalog text already sent to the model at turn 0. They do not include project file contents.
Still blocked unless explicitly opted in:
hook_additional_context(unchanged — still requiresCLAUDE_CODE_SAVE_HOOK_ADDITIONAL_CONTEXT)Code changes (minimal)
src/utils/sessionStorage.tsisLoggableMessagefor non-antssrc/utils/attachments.tssuppressNextAgentListing+ consume latch ingetAgentListingDeltaAttachment(same shape as skills)src/utils/conversationRecovery.tsagent_listing_delta, callsuppressNextAgentListing()No changes to openaiShim request shaping, tool definitions, or provider routing.
Measurement method
Real CLI path:
dist/cli.mjs+ Moonshotkimi-k3, localhost forward-proxy dumping everychat/completionsbody + SSE response.Protocol:
--resume <session>with another tool turn--resumewith “Do not use tools” follow-upCompared:
messages[]first divergence index (prefix stability)toolsfingerprint equalitychoices[0].usage.cached_tokens/prompt_tokens)Note: OpenClaude’s own Anthropic-shaped usage fields stay
0whenOPENAI_BASE_URLpoints at localhost (self-hosted cache metrics path). Moonshot’s raw SSEcached_tokensis the ground truth below.Results — BEFORE (broken)
Dump:
oc-reqdump-1785444725950· modelkimi-k3· session921bae3f-4a94-49b2-baf4-daf4a310d60bWire prefix
toolsEqual--resume)messages[1].contentmessages[7].contentConcrete A/B snippet at
messages[1](resume):Available agent types for the Agent tool: …# claudeMd…) — different SHA, different length (11746 vs 8856 chars in one captured pair)Moonshot
cached_tokens(SSE)Interpretation: within a process, cache works. The first resume request that rewrites early history pays full prompt price (
cached_tokens = 0).Results — AFTER (this PR)
Dump:
oc-reqdump-1785447394725· modelkimi-k3· sessione458eefd-1e29-4946-b0e3-78ffed319a5c· build including this commitWire prefix
toolsEqualmessages[2]length only (2 → 6)--resume)messages[6]length only (6 → 8)messages[12]length only (12 → 14)Moonshot
cached_tokens(SSE)--resumefirstHead-to-head (the money shot)
messages[1].content(rewrite)messages[6](append-only)cached=0/prompt≈23.5k)cached=23296/prompt=23551)Test plan
--resumeT2 → T5 on Moonshotkimi-k3(before/after)toolsSHA identical across compared callscached_tokenson resume first call recovers from 0 → ~99%Risks / non-goals
cache_controlfor Moonshot — unnecessary once the prefix is stable.Checklist for reviewers
suppressNextSkillListingfor agents the right resume latch (vs reconstructing announced set only from transcript)?messages[1]and should join the allow-list?Commit:
b53a9fdfonussoewwin/openclaudeSummary by CodeRabbit
Bug Fixes
Privacy
Reliability