Repository navigation
fix(context): pair tool calls by id instead of adjacency - #1280
Conversation
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe change adds identifier-based tool pairing, contiguous batch repair, safe summarization boundaries, conversation-history repair, and a network-free continuous test suite. ChangesTool Pair Repair
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConversationMemory
participant repairToolPairs
participant StructuredSummarizer
participant SummarizationEngine
ConversationMemory->>repairToolPairs: repair filtered conversation history
repairToolPairs-->>ConversationMemory: return balanced messages
StructuredSummarizer->>StructuredSummarizer: snap split to batch boundary
SummarizationEngine->>SummarizationEngine: reject full-array snapped split
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
test/continuous-test-suite-tool-pairing.ts (2)
200-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the duplicate result is actually dropped.
The current assertion only checks balance. A batch of 1 call and 2 duplicate results is also "balanced" if repair adds a synthetic call for the duplicate. Assert the result count and
repairedso the test locks the documentedchangedcontract inrepairBatch.💚 Proposed stronger assertions
const out = repairToolPairs(input); const c = counts(out.messages); assert(c.calls === c.results, "duplicate id left the batch unbalanced"); + assert(out.repaired, "dropped duplicate did not mark the history repaired"); + assert(c.calls === 1 && c.results === 1, "duplicate result was not dropped"); + assert( + out.orphanedCallsFixed === 0 && out.orphanedResultsFixed === 0, + "duplicate removal produced synthetic placeholders", + ); });🤖 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 `@test/continuous-test-suite-tool-pairing.ts` around lines 200 - 209, Strengthen the test named “duplicate result ids never over-pair a call” to assert that the duplicate result is dropped rather than balanced by adding a synthetic call. Verify the output contains one result and that the repair result’s `repaired`/`changed` contract is set as documented by `repairBatch`, while preserving the existing input and balance checks.
274-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for two adjacent tool batches with no text between them.
Every snap test separates batches with a text message or uses one batch. The suite therefore does not cover a contiguous run that holds two batches, which is the case where
snapSplitToBatchBoundaryskips more than one batch. Add a case such as[call, result, call, result, text]withsplitIndex = 2, and a case where the whole array is one tool run, to assert the returned index stays inside the array. This locks the boundary behavior discussed onsrc/lib/context/toolPairRepair.tsLines 191-219.🤖 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 `@test/continuous-test-suite-tool-pairing.ts` around lines 274 - 284, Extend the snapSplitToBatchBoundary tests with adjacent tool batches lacking an intervening text message, using [call, result, call, result, text] and splitIndex 2 to verify snapping past both batches. Also add a case where the entire array is one tool run and assert the returned index remains within the array, preserving the boundary behavior.
🤖 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/lib/context/summarizationEngine.ts`:
- Around line 210-213: Cap the value passed to snapSplitToBatchBoundary in the
summarization split calculation so it never reaches messages.length, preserving
at least one recent message. Update the return expression in the surrounding
split-index logic and keep the existing minimum of one message summarized.
In `@src/lib/context/toolPairRepair.ts`:
- Around line 9-14: Replace the stale flushPendingToolExecutions reference with
flushPendingToolData in the module header of src/lib/context/toolPairRepair.ts
lines 9-14 and the toolCallId documentation in src/lib/types/conversation.ts
lines 354-364; no implementation changes are required.
- Around line 191-219: Update snapSplitToBatchBoundary so its backward and
forward walks stop at a tool_result-to-tool_call transition rather than treating
the entire contiguous tool run as one batch. Preserve the existing boundary
validation and return the closest legal split, including a transition within a
run and avoiding messages.length when an earlier batch boundary is available.
---
Nitpick comments:
In `@test/continuous-test-suite-tool-pairing.ts`:
- Around line 200-209: Strengthen the test named “duplicate result ids never
over-pair a call” to assert that the duplicate result is dropped rather than
balanced by adding a synthetic call. Verify the output contains one result and
that the repair result’s `repaired`/`changed` contract is set as documented by
`repairBatch`, while preserving the existing input and balance checks.
- Around line 274-284: Extend the snapSplitToBatchBoundary tests with adjacent
tool batches lacking an intervening text message, using [call, result, call,
result, text] and splitIndex 2 to verify snapping past both batches. Also add a
case where the entire array is one tool run and assert the returned index
remains within the array, preserving the boundary 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: CHILL
Plan: Pro Plus
Run ID: 85c03c0e-64f1-4e3c-b063-0df0ef402805
📒 Files selected for processing (9)
package.jsonsrc/lib/context/stages/structuredSummarizer.tssrc/lib/context/summarizationEngine.tssrc/lib/context/toolPairRepair.tssrc/lib/core/redisConversationMemoryManager.tssrc/lib/types/context.tssrc/lib/types/conversation.tssrc/lib/utils/conversationMemory.tstest/continuous-test-suite-tool-pairing.ts
|
🔍 Yama Code Review Summary I've completed a thorough review of PR #1280 "batch-aware tool pair repair" following the Yama methodology. Here's my assessment: Decision: APPROVED ✅Review Scope & Methodology
FindingsNo blocking issues found. All CRITICAL/MAJOR focus areas pass:
Detailed AnalysisFiles Reviewed:
Key Implementation Highlights:
Impact on Existing Code
RecommendationThis PR is safe to merge. It addresses a real bug where adjacency-based repair was incorrectly modifying healthy parallel tool batches, introduces a robust fix with proper testing, and maintains full backward compatibility. Reviewed by Yama autonomous code review agent following NeuroLink enterprise standards. |
|
🔒 MAJOR: Types not exported from barrel — The new types RepairToolBatch and RepairResult are defined in src/lib/types/context.ts but the barrel (src/lib/types/index.ts) does not export './context.js'. This means types cannot be imported via the standard barrel pattern (e.g., import type { X } from '../types/index.js') and must be imported directly from the source file, violating CLAUDE.md Rule 13 ("Barrel-only imports for internal types"). Fix: Add 'export * from "./context.js";' to src/lib/types/index.ts to include the newly added types in the barrel exports. |
Review Summary for PR #1280: "fix(context): pair tool calls by id instead of adjacency"Decision: CHANGES_REQUESTEDFindings (1 MAJOR issue):
Inline Comment Posted:A comment has been posted on Fix Required:Add Positive Aspects:
Impact Analysis:The code knowledge graph shows this change affects ~8200 nodes across the codebase, primarily through the context summarization pipeline and conversation memory management. The changes are self-contained to the context/toolPairRepair module with no unexpected side effects. Review Scope:This review covered all 9 changed files:
Recommendation:Request changes until the barrel export issue is fixed. Once resolved, this PR should be approved as it provides a valuable bug fix that correctly handles parallel tool calls and maintains backward compatibility. |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Actionable comments posted: 1
🔒 MAJOR — Types not exported from barrel (src/lib/types/index.ts)
The new types RepairToolBatch and RepairResult are defined in src/lib/types/context.ts but the barrel does not export './context.js', violating CLAUDE.md Rule 13.
Fix required: Add export * from "./context.js"; to src/lib/types/index.ts
Review Summary for PR #1280: "fix(context): pair tool calls by id instead of adjacency"
Decision: CHANGES_REQUESTED
Findings:
| Severity | Category | Title | File | Description |
|---|---|---|---|---|
| 🔒 MAJOR | Architecture & CLAUDE.md Critical Rules | Types not exported from barrel | src/lib/types/index.ts | The new types RepairToolBatch and RepairResult are defined in src/lib/types/context.ts but the barrel does not export './context.js', violating CLAUDE.md Rule 13 ("Barrel-only imports for internal types"). |
Positive Aspects:
- ✅ Core fix (ID-based tool pairing) correctly addresses a real bug where parallel tool calls were misidentified as orphans
- ✅ Batch processing properly handles parallel tool calls within a single step
- ✅ Backward compatibility maintained via optional
toolCallIdfield - ✅ Comprehensive test coverage with edge cases for various message patterns
- ✅ No breaking changes to existing APIs
- ✅ Type changes are additive and safe
Impact Analysis:
The code knowledge graph shows this change affects ~8,200 nodes across the codebase, primarily through the context summarization pipeline and conversation memory management. Changes are self-contained to the context/toolPairRepair module with no unexpected side effects.
Files Reviewed (9 total):
- package.json - ✅ Approved (test script addition)
- structuredSummarizer.ts - ✅ Approved (summary snapping logic)
- summarizationEngine.ts - ✅ Approved (summary snapping logic)
- toolPairRepair.ts - ✅ Approved (core ID-based repair logic)
- redisConversationMemoryManager.ts - ✅ Approved (type import updated)
- context.ts -
⚠️ Missing barrel export (see inline comment) - conversation.ts - ✅ Approved (ChatMessage augmentation)
- conversationMemory.ts - ✅ Approved (no functional changes)
- continuous-test-suite-tool-pairing.ts - ✅ Approved (comprehensive tests)
Recommendation:
Request changes until the barrel export issue is fixed. Once resolved, this PR should be approved as it provides a valuable bug fix that correctly handles parallel tool calls and maintains backward compatibility.
🛡️ Yama Review Verdict: CHANGES_REQUESTEDSeverity counts — 🔒 CRITICAL: 0 · PR #1280 fixes a critical bug in tool pairing logic (changing from adjacency-based to ID-based matching) and adds summary snapping to prevent cutting through tool batches. The fix is sound and well-tested, but there's one MAJOR issue: new types RepairToolBatch and RepairResult defined in context.ts are not exported from the barrel (src/lib/types/index.ts), violating CLAUDE.md Rule 13. Fix: Add 'export * from "./context.js";' to src/lib/types/index.ts. Findings behind this verdict
|
|
Reviewed this closely because six PRs are stacked on it. The pairing rewrite itself I think is right — ID mode with a legacy positional fallback, keeping the first result on duplicate ids, and the separate Two issues in 1. The forward snap can empty the recent window
// all tool-role, three complete batches
[call1, res1, call2, res2, call3, res3]
snapSplitToBatchBoundary(msgs, 2) // => 6 === messages.lengthConsumer: const messagesToSummarize = recentMessages.slice(0, splitIndex);
const recentToKeep = recentMessages.length - messagesToSummarize.length; // 0So the entire recent window is summarized and nothing is kept verbatim, which defeats Reachable whenever history ends mid tool-run — an assistant turn whose results came back with no trailing text. 2. It snaps boundaries that were already clean// six complete batches, then two text messages
snapSplitToBatchBoundary(msgs, 6) // => 12Index 6 sits between A 3. Doc reference points at a function that does not exist
Status
|
78a90da to
0cecdbb
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
Looking at the previous review finding about types not being exported from barrel in src/lib/types/index.ts. Let me verify if this is still an issue. |
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 `@test/continuous-test-suite-tool-pairing.ts`:
- Around line 303-306: Remove the interpolated split value from the assertion
messages in the assertions around snapSplitToBatchBoundary and the corresponding
assertions near lines 356–364. Replace each dynamic message with a static
structural description while preserving the existing assertions and failure
conditions.
- Around line 201-210: Update the “duplicate result ids never over-pair a call”
test around repairToolPairs and counts to assert that exactly one call and one
result remain, rather than only checking equal counts. Preserve the existing
input and verify duplicate result removal directly in the output.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 53cce57a-716e-4e96-a541-20bbb2f69937
📒 Files selected for processing (9)
package.jsonsrc/lib/context/stages/structuredSummarizer.tssrc/lib/context/summarizationEngine.tssrc/lib/context/toolPairRepair.tssrc/lib/core/redisConversationMemoryManager.tssrc/lib/types/context.tssrc/lib/types/conversation.tssrc/lib/utils/conversationMemory.tstest/continuous-test-suite-tool-pairing.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- package.json
- src/lib/core/redisConversationMemoryManager.ts
- src/lib/types/conversation.ts
- src/lib/context/summarizationEngine.ts
- src/lib/context/stages/structuredSummarizer.ts
- src/lib/utils/conversationMemory.ts
- src/lib/types/context.ts
- src/lib/context/toolPairRepair.ts
| assert( | ||
| snapSplitToBatchBoundary(messages, split) === split, | ||
| `legal batch boundary at index ${split} was moved`, | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove dynamic values from assertion messages.
Line 305, Line 358, and Line 363 interpolate ${split} into assertion messages. Use structural messages with no actual values.
Proposed fix
- `legal batch boundary at index ${split} was moved`,
+ "legal batch boundary was moved",
@@
- `orphaned tool_result survived at split index ${split}`,
+ "orphaned tool_result survived after split repair",
@@
- `unbalanced tool batch at split index ${split}`,
+ "tool batch was unbalanced after split repair",As per coding guidelines, test/**/*.ts must not include actual values in assertion messages.
Also applies to: 356-364
🤖 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 `@test/continuous-test-suite-tool-pairing.ts` around lines 303 - 306, Remove
the interpolated split value from the assertion messages in the assertions
around snapSplitToBatchBoundary and the corresponding assertions near lines
356–364. Replace each dynamic message with a static structural description while
preserving the existing assertions and failure conditions.
Source: Coding guidelines
|
Thanks both — addressed. Summary of what changed and one finding I could not reproduce. Fixed — batch boundary detection (CodeRabbit, Major). Confirmed: This is the same bug class as Fixed — forward snap could consume the whole recent window (CodeRabbit, Major). Confirmed: an all-tool array snapped forward to Fixed — stale doc reference (CodeRabbit, Minor). The method is Not reproduced — types not exported from barrel (@Tara-ag, Major). Verification: |
Review Summary for PR #1280Decision: APPROVED ✅This PR implements batch-aware tool call/result pairing for the context compaction pipeline, fixing a critical bug where parallel tool calls were incorrectly repaired when results arrived out of order. Findings SummaryNo issues found. All code quality, security, and correctness checks pass. What This PR Does
Impact on Existing Code
Key Design Decisions
Verification Checklist
ConclusionThis is a well-designed, thoroughly tested fix that resolves a real bug in the context compaction pipeline. The implementation follows NeuroLink's patterns, maintains backward compatibility, and includes comprehensive regression tests. Ready to merge. |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary for PR #1280
Decision: APPROVED ✅
This PR implements batch-aware tool call/result pairing for the context compaction pipeline, fixing a critical bug where parallel tool calls were incorrectly repaired when results arrived out of order.
Findings Summary
No issues found. All code quality, security, and correctness checks pass.
What This PR Does
-
Batch-Aware Tool Pairing: Rewrites
toolPairRepair.tsto use ID-based matching (toolCallId) instead of positional adjacency, fixing the bug where parallel batches would create orphaned placeholders over valid results. -
Type Updates: Adds
RepairToolBatchtype insrc/lib/types/context.tsand extendsChatMessagewith optionaltoolCallIdfield insrc/lib/types/conversation.ts. -
Memory Persistence: Redis memory manager now persists
toolCallIdfor both calls and results, enabling correct re-pairing after compaction. -
Split Protection: Batch-aware split logic in
summarizationEngine.tsusessnapSplitToBatchBoundary()to prevent cutting tool batches in half during summarization. -
Comprehensive Tests: New test suite
continuous-test-suite-tool-pairing.tscovers 19 test cases including healthy batches, genuine orphans, multiple independent batches, split boundaries, and edge cases.
Impact on Existing Code
- Blast Radius: Changes affect 76 functions directly, 500+ nodes within 3 hops, and 67 files indirectly.
- Key Dependencies:
SummarizationEngine,ContextCompactor,RedisConversationMemoryManager,StructuredSummarizerall integrate correctly with new batch-aware logic. - Backward Compatibility: Fallback to positional pairing maintained for legacy sessions without
toolCallId. - Test Coverage: Comprehensive regression tests added; no existing tests broken.
Key Design Decisions
- ID-Based Pairing: Uses unique
toolCallIdfor parallel batches where position is meaningless. - Sentinel Handling: Unknown/empty IDs fall back gracefully to legacy positional pairing.
- Non-Destructive: Healthy batches (complete pairs) are left untouched.
- Atomic Batches: Split boundaries snap to batch boundaries to avoid orphaning results.
Verification Checklist
- ✅ Types exported from barrel (
src/lib/types/index.ts) - ✅ No interface declarations (all use
type) - ✅ No hardcoded secrets or credentials
- ✅ Proper error handling (returns, never throws)
- ✅ Backward compatibility maintained
- ✅ Type safety preserved (no
anyor double assertions) - ✅ Test coverage comprehensive
- ✅ CLI vs SDK concerns properly separated
Conclusion
This is a well-designed, thoroughly tested fix that resolves a real bug in the context compaction pipeline. The implementation follows NeuroLink's patterns, maintains backward compatibility, and includes comprehensive regression tests. Ready to merge.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary for PR #1280
Decision: APPROVED ✅
This PR implements batch-aware tool call/result pairing for the context compaction pipeline, fixing a critical bug where parallel tool calls were incorrectly repaired when results arrived out of order.
Findings Summary
No issues found. All code quality, security, and correctness checks pass.
What This PR Does
-
Batch-Aware Tool Pairing: Rewrites
toolPairRepair.tsto use ID-based matching (toolCallId) instead of positional adjacency, fixing the bug where parallel batches would create orphaned placeholders over valid results. -
Type Updates: Adds
RepairToolBatchtype insrc/lib/types/context.tsand extendsChatMessagewith optionaltoolCallIdfield insrc/lib/types/conversation.ts. -
Memory Persistence: Redis memory manager now persists
toolCallIdfor both calls and results, enabling correct re-pairing after compaction. -
Split Protection: Batch-aware split logic in
summarizationEngine.tsusessnapSplitToBatchBoundary()to prevent cutting tool batches in half during summarization. -
Comprehensive Tests: New test suite
continuous-test-suite-tool-pairing.tscovers 19 test cases including healthy batches, genuine orphans, multiple independent batches, split boundaries, and edge cases.
Impact on Existing Code
- Blast Radius: Changes affect 76 functions directly, 500+ nodes within 3 hops, and 67 files indirectly.
- Key Dependencies:
SummarizationEngine,ContextCompactor,RedisConversationMemoryManager,StructuredSummarizerall integrate correctly with new batch-aware logic. - Backward Compatibility: Fallback to positional pairing maintained for legacy sessions without
toolCallId. - Test Coverage: Comprehensive regression tests added; no existing tests broken.
Key Design Decisions
- ID-Based Pairing: Uses unique
toolCallIdfor parallel batches where position is meaningless. - Sentinel Handling: Unknown/empty IDs fall back gracefully to legacy positional pairing.
- Non-Destructive: Healthy batches (complete pairs) are left untouched.
- Atomic Batches: Split boundaries snap to batch boundaries to avoid orphaning results.
Verification Checklist
- ✅ Types exported from barrel (
src/lib/types/index.ts) - ✅ No interface declarations (all use
type) - ✅ No hardcoded secrets or credentials
- ✅ Proper error handling (returns, never throws)
- ✅ Backward compatibility maintained
- ✅ Type safety preserved (no
anyor double assertions) - ✅ Test coverage comprehensive
- ✅ CLI vs SDK concerns properly separated
Conclusion
This is a well-designed, thoroughly tested fix that resolves a real bug in the context compaction pipeline. The implementation follows NeuroLink's patterns, maintains backward compatibility, and includes comprehensive regression tests. Ready to merge.
0cecdbb to
8344a88
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
Second pass — @murdore — all three were real, all three fixed in the previous pushYour comment landed just before that push, so for the record, against 1 + 2 — 3 — doc reference. Three regression tests cover it: a @coderabbitai — one taken, one declined with evidenceDuplicate-result assertion (lines 201-210) — taken. You are right that assert(c.calls === 1, "duplicate result caused a call to be synthesized");
assert(c.results === 1, "duplicate result was not dropped");
const survivor = out.messages.find((m) => m.role === "tool_result");
assert(survivor?.id === "r1", "the wrong duplicate result was kept");The third line also pins the first-wins tie-break, which nothing asserted before. Interpolated split index in assertion messages — declined. The hazard is real but the rule is narrower than the finding reads. A loop index is a locator, not a payload. Checked empirically rather than by reading — every message those two loops can emit, run through the real matcher: These are Verification
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Revision =='
git rev-parse --short HEAD
echo '== Relevant declarations and functions =='
ast-grep outline src/lib/context/toolPairRepair.ts --items all --type function
ast-grep outline src/lib/context/summarizationEngine.ts --items all --type function
ast-grep outline test/continuous-test-suite-tool-pairing.ts --items all --type function
echo '== Batch split implementation =='
rg -n -A35 -B12 'cutsBatch|snapSplitToBatchBoundary' \
src/lib/context/toolPairRepair.ts \
src/lib/context/summarizationEngine.ts \
src/lib/context/stages/structuredSummarizer.ts
echo '== Duplicate-result regression assertions =='
rg -n -A22 -B12 'duplicate result|duplicate-result|first.*wins|first-wins' \
test/continuous-test-suite-tool-pairing.ts
echo '== Stale and current documentation references =='
rg -n -i -C2 'flushPendingToolExecutions|flushPendingToolData' src test || true
echo '== Barrel export =='
rg -n -C2 'context\.js' src/lib/types/index.tsLength of output: 33148
The duplicate-result regression test now prevents both over-pairing and incorrect duplicate selection. It asserts exactly one call, exactly one result, and keeps The split logic now preserves The stale I accept the clarification on interpolated loop indices. The index is a structural locator and does not expose payload content. I withdraw that concern. ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
Re-checked against
One residual edgeThe forward walk can still return Downstream that is On the assertion-message thread — my guidance, applied too broadlyThat thread traces back to a note I added to CLAUDE.md, and I had the mechanism wrong. Correcting it here since it is steering this PR. The harness turns a failed assertion into a skip when The actual trigger in my case was the literal token So for this PR: I will send a follow-up to fix the CLAUDE.md wording so the next reviewer is not led the same way. Everything else here still looks good to me — the ID/legacy pairing split, first-result-wins on duplicate ids, and the |
8344a88 to
2271f0f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
🎉 This PR is included in version 10.10.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Summary
This PR fixes issue #749 by implementing batch-aware, ID-based tool call/result pairing in the context compaction system. The previous adjacency-based matching would incorrectly pair tools in parallel calls (e.g., call1, call2, result1 would match call1→result1 and orphan call2). The new implementation correctly pairs within batches using tool IDs when available, falling back to positional matching within the batch.
Files changed: 9 files
- Core implementation:
src/lib/context/toolPairRepair.ts,summarizationEngine.ts,structuredSummarizer.ts - Integration:
src/lib/utils/conversationMemory.ts - Tests:
test/continuous-test-suite-tool-pairing.ts - Documentation:
package.json,docs/features/context-compaction.md
Impact: All conversation memory operations now properly repair orphaned tool calls/results that occur when history is sliced mid-batch (via pointers after summarization). This affects ~500 code nodes but is a critical correctness fix for multi-turn conversations with tool use.
Code quality: All changes follow CLAUDE.md standards - proper error handling, correct type usage, logger guards on expensive serialization, comprehensive tests covering edge cases.
Impact on existing code
The change is backward-compatible - it only repairs messages that previously had orphans (which would have caused provider rejections). No API changes, no breaking changes. All callers of getConversationMessages, buildContextFromPointer, compactSession, and related functions automatically benefit from the fix.
Verdict: APPROVED
This is a well-implemented bug fix with:
- ✓ Correct algorithm design (batch-aware, ID-first pairing)
- ✓ Comprehensive test coverage
- ✓ Proper error handling and logging
- ✓ Zero breaking changes
- ✓ Follows all project standards
3 participants
Description
repairToolPairsmatchedtool_calltotool_resultby adjacency. A single agent step with parallel tool calls is persisted as every call followed by every result (flushPendingToolExecutions), sotool_callis routinely followed by anothertool_callin perfectly healthy history. The repair read that as an orphan and injected a[Tool result unavailable]placeholder over a result that was present two entries later.Measured on the exact shape the Redis manager writes:
This ran after every compaction, so any session using parallel tool calls was corrupted the moment it compacted.
Root cause:
ChatMessagehad notoolCallId. The id was computed influshPendingToolExecutions, used to build a name lookup, then discarded — leaving position as the only signal, and position is wrong for every parallel batch.Type of Change
Motivation and Context
Providers hard-reject an unmatched
tool_call/tool_result, so a corrupted batch turns into a failed turn. Two further paths could emit orphans that nothing caught:buildContextFromPointerslices history at the summary pointer with no repair on readtool_resultChanges
toolCallIdadded toChatMessage(optional — backward compatible) and persisted on both sidesrepairToolPairsrewritten batch- and ID-aware, with positional fallback scoped to the batch for pre-existing sessionssnapSplitToBatchBoundarykeeps the summary pointer off a batch boundary, applied in both summarizers (including after the clamp instructuredSummarizer, which could otherwise pull the boundary back in)toolCallIdresults are dropped rather than over-pairing a callTesting
New no-API suite
pnpm run test:tool-pairing— 15/15. Covers parallel batches (id and legacy), five-wide batches, out-of-order results, genuine orphans in both directions, duplicate ids, empty-content results, multiple independent batches, and randomized split points.Verified the suite fails loudly (
✗, exit 1) on a deliberately broken assertion — per the CLAUDE.md hazard whereisExpectedProviderError()can downgrade a real failure to⊘.tsc --noEmit --strict: 0 errorseslint: 0 errorstest:context: 36 pass / 5 fail / 18 skip — byte-identical failure set torelease(all 5 pre-existing and environmental: 4x LiteLLM model-access denial, 1xdist/lib/layout mismatch)Notes for reviewers
Stacked: this is the base of a 3-PR stack.
fix/token-accountingmust not merge before this one — correct token accounting makes compaction fire far more often, which would amplify this corruption without the fix underneath.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements
Tests