fix(memory): persist tool executions on every memory backend

[pdogra1299]

Description

storeToolExecution existed only on RedisConversationMemoryManager, and NeuroLink.storeToolExecutions reached it with a cast:

const redisMemory = this.conversationMemory as RedisConversationMemoryManager;
await redisMemory.storeToolExecution(...);

isToolExecutionStorageAvailable() compounded it, requiring STORAGE_TYPE === "redis" and a class-name match. So on in-memory storage tool activity never became tool_call / tool_result messages at all.

That left two divergent history shapes. Everything downstream that reasons about tool batches — compaction, tool-output pruning, and the id-based pair repair from #1280 — behaved differently depending on STORAGE_TYPE, and the in-memory shape was the barely-exercised one.

Type of Change

• Bug fix (non-breaking change which fixes an issue)

Changes

• storeToolExecution declared on IConversationMemoryManager (optional, so third-party backends stay valid)
• implemented on ConversationMemoryManager, mirroring the Redis contract exactly: calls written before results, toolCallId carried on both sides so repairToolPairs can match by id rather than adjacency, tool name resolved onto the result via that id, non-string output serialized with a guarded fallback, result.success / result.error recorded
• caller now dispatches through the interface instead of casting
• isToolExecutionStorageAvailable() is capability-based rather than Redis-specific

Testing

pnpm run test:tool-storage-parity — 10/10, no Redis required. Covers the contract, batch ordering, id propagation, serialization and error recording, session creation, empty batches, appending onto an existing turn, and a parallel batch round-tripping through repairToolPairs untouched.

Verified to fail loudly (✗, exit 1) on a deliberately broken assertion, per the CLAUDE.md hazard where isExpectedProviderError() can downgrade a real failure to ⊘.

Regressions, all green: test:tool-pairing 15/15, test:token-accounting 10/10, test:step-guard 6/6, test:loop-guard-core 11/11, test:openai-compat-guard 9/9, test:anthropic-guard 10/10. tsc --noEmit --strict 0 errors, eslint 0 errors, prettier clean.

Scope: append-only Redis storage is NOT in this PR

The planned O(n²) fix for storeConversationTurn is deliberately excluded. I measured it against local Redis first:

Conversation profile	first turns	last turns	degradation
200 turns x 2KB messages	2ms	2ms	1.0x — no problem
400 turns x 20KB messages (~16MB)	2ms	61ms	30.5x, 10.9s total

So the problem is real but only at multi-MB scale — which is the agentic tool-output profile, so it is worth fixing.

I built the split-storage design ({key}:msgs LIST + small metadata blob, with a marker so legacy blobs are read as-is and converted on next write) and then reverted it, because implementing it surfaced a hazard the plan had not accounted for: getStats() and listAllSessions() scan keys by prefix and GET each one. New :msgs LIST keys live under that same prefix, so those scans would hit WRONGTYPE. The code already special-cases :sessions keys, which shows this collision class is a known trap in this file.

Combined with 12 varied read sites, two-key TTL, and clearSession needing to delete both keys, it is a larger and more delicate change than one PR should absorb alongside this one. A half-migrated storage layer would make split-written sessions read as empty on unrouted paths — strictly worse than not starting. It deserves its own focused PR with a real-Redis test suite.

Top of the stack: #1280 → #1281 → #1282 → #1283 → this.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Tool calls and results can now be persisted in supported conversation memory backends, including in-memory storage.
  • Stored tool activity preserves execution order, identifiers, names, outputs, and success or error details.
  • Sessions are created automatically when needed, and new tool activity can be appended to existing sessions.

• Tests

  • Added coverage validating tool-storage behavior, ordering, serialization, error handling, and session management.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 998a5342-6ceb-4a61-b935-123877454a3a

📥 Commits

Reviewing files that changed from the base of the PR and between e296cdd and 5825991.

📒 Files selected for processing (5)

• package.json
• src/lib/core/conversationMemoryManager.ts
• src/lib/neurolink.ts
• src/lib/types/conversationMemoryInterface.ts
• test/continuous-test-suite-tool-storage-parity.ts

🚧 Files skipped from review as they are similar to previous changes (5)

• package.json
• src/lib/neurolink.ts
• src/lib/core/conversationMemoryManager.ts
• src/lib/types/conversationMemoryInterface.ts
• test/continuous-test-suite-tool-storage-parity.ts

---

📝 Walkthrough

Walkthrough

The change adds optional tool execution storage to conversation memory backends. The in-memory manager persists ordered tool calls and results. NeuroLink uses capability detection. A continuous test suite validates storage behavior.

Changes

Tool execution storage parity

Layer / File(s)	Summary
Storage contract and in-memory implementation src/lib/types/conversationMemoryInterface.ts, src/lib/core/conversationMemoryManager.ts	The interface adds optional storeToolExecution. The in-memory manager stores ordered calls and results, preserves identifiers and names, records metadata, handles serialization failures, and updates session activity.
Capability-based backend integration src/lib/neurolink.ts	NeuroLink delegates storage to backends that expose storeToolExecution. Availability checks use capability detection.
Parity validation and test execution test/continuous-test-suite-tool-storage-parity.ts, package.json	The test suite validates persistence, ordering, metadata, serialization, pairing, session creation, and appending. A package script runs the suite.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant NeuroLink
  participant MemoryBackend
  participant ConversationSession
  NeuroLink->>MemoryBackend: storeToolExecution(sessionId, toolCalls, toolResults)
  MemoryBackend->>ConversationSession: persist tool-call messages
  MemoryBackend->>ConversationSession: persist tool-result messages
  MemoryBackend-->>NeuroLink: complete or report failure

Loading

Possibly related PRs

• juspay/neurolink#1142: Modifies conversation-session message persistence and ordering.
• juspay/neurolink#1280: Modifies tool-call and tool-result persistence and pairing behavior.
• juspay/neurolink#1277: Adds related continuous tsx test-suite and package-script changes.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: persisting tool executions across all configured memory backends.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/session-persistence

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[murdore]

Not mergeable yet, and I want to flag why the checks look the way they do, because the picture is misleading.

Base is not release

This targets fix/loop-guard-parity (#1283), so it can only merge after everything below it. Order for the stack:

release
 └─ #1280  fix/tool-pair-identity
     └─ #1281  fix/token-accounting
         └─ #1282  fix/cache-aware-compaction
             └─ #1283  fix/loop-guard-parity
                 └─ #1285  fix/session-persistence
                     └─ #1286  fix/redis-append-only
                         └─ #1287  fix/loop-guard-completion

This PR is #5 of 7.

The Single Commit failure is a stacking artifact, not a real violation

single-commit-enforcement.yml triggers on push with branches-ignore: [release, main, develop], so it runs on the branch and measures against release rather than against this PR's own base. fix/loop-guard-parity… this branch is 5 commits ahead of release because it carries its parents'. Against its own base it is a single commit, which is what the policy actually asks for. It should clear itself as the parents merge.

The bigger issue: almost no CI has run here

Three checks are present on this PR. Release-targeting PRs get fifteen. The difference is not incidental — the workflows are branch-gated:

workflow	pull_request: branches:
ci.yml (test (20), build-check, lint, format)	[release]
yama-review.yml	[release]
copilot-review.yml	[release, main]

So on this PR nothing has been built, no tests have run, and neither AI reviewer has looked at it. Only CodeRabbit, SentinelOne and the push-triggered commit check fire. The absence of review comments here reflects the absence of a review, not a clean bill of health.

Across #1281, #1282, #1283, #1285, #1286 and #1287 that is roughly 3,900 added lines that have never been compiled or tested by CI.

Nothing to do on this PR specifically — it resolves itself once the parents land and the base becomes release. Worth knowing before anyone reads the green ticks as coverage. If you want real signal sooner, temporarily retargeting to release (or adding the stack branches to ci.yml's filter) would run the full gate.

[pdogra1299]

@murdore — confirmed both points, verified rather than taken on trust.

Single Commit failure is a stacking artifact. single-commit-enforcement.yml fires on push with branches-ignore: [release, main, develop], and in the push branch of that script BASE_BRANCH is hard-coded to origin/release — so it measures against release, not the PR's own base. Against its own base every branch in the stack is exactly one commit:

$ git rev-list --count HEAD ^<own-base>
fix/token-accounting        1
fix/cache-aware-compaction  1
fix/loop-guard-parity       1
fix/session-persistence     1
fix/redis-append-only       1
fix/loop-guard-completion   1

Nothing to squash. It clears itself as parents land.

The CI gap is real and I am not treating the green ticks as coverage. Verified the gating you listed:

workflow	pull_request: branches:
ci.yml	[release]
yama-review.yml	[release]
copilot-review.yml	[release, main]

So on this PR nothing has been built or tested by CI, and that is the accurate reading.

What has run, locally on the stack tip (so every branch's code compiled together) after the latest rebase:

• pnpm run check → 4961 files, 0 errors, 0 warnings
• all nine new suites → 95 tests, 0 failures, 0 skips
  (tool-pairing 18, token-accounting 10, step-guard 6, loop-guard-core 11, openai-compat-guard 9, anthropic-guard 10, tool-storage-parity 10, redis-append-only 10, gemini-guard 11)

That is local signal, not CI signal, and I would rather say so plainly than let the tick count imply otherwise. Merging bottom-up closes the gap for real: each PR's base becomes release as its parent lands, at which point it picks up the full fifteen-check gate before it can merge — so no branch reaches release without having been through it.

[pdogra1299]

#1280 is merged (a0f49e40), so the stack has been rebased onto the new release (now at 10.10.2 — it had moved two releases since these branches were cut). Still one commit per branch.

#1281 was CONFLICTING after the merge — its branch still carried the pre-squash copy of #1280's commit. Replanted with git rebase --onto origin/release 8344a885 so only its own commit survives; now MERGEABLE against release.

One real conflict, in providers/anthropic/client.ts. release had gained the structured-output buffering from the final_result work (bufferedText / finalResultText) at the same point where #1283 adds estimateAnthropicFixedOverhead. Both-added, both wanted — kept both. Verified afterwards that the release-side path is still fully wired (bufferedText accumulates at the text-delta branch, finalResultText is set from the final_result tool call, and the buffered turn is still delivered at the end of the loop), not just that it compiles.

The build was broken, and pnpm run check did not catch it

Worth recording since it is exactly the CI gap @murdore flagged. Building the tip surfaced two errors in #1286's code that the typechecker passes clean:

redisConversationMemoryManager.ts(987,7): error TS2345:
  '(string | Buffer)[]' is not assignable to 'string[]'
redisConversationMemoryManager.ts(1000,5): error TS2322:
  'number | `${number}`' is not assignable to 'number'

pnpm run check uses the main tsconfig; build:react-hooks compiles with --module nodenext --moduleResolution nodenext, which resolves the redis types differently and widens both return types. Fixed at the source rather than with an assertion (Rule 14): lRange entries are normalized via .toString(), lLen through Number(). Both are real defects, not just type noise — a binary-mode client would have thrown on parse, and a RESP3 string count would have concatenated where callers add.

Verification

Each branch was built independently, since each merges on its own:

branch	build
fix/token-accounting	✅
fix/cache-aware-compaction	✅
fix/loop-guard-parity	✅
fix/session-persistence	✅
fix/redis-append-only	✅
fix/loop-guard-completion	✅

On the tip: pnpm run check → 4939 files, 0 errors; pnpm run lint → 0 errors, 48 warnings, all pre-existing max-lines-per-function in files this stack does not touch (claudeProxyRoutes.ts, toolRoutes.ts, csvProcessor.ts); all nine suites → 95 tests, 0 failures, 0 skips.

Still local signal, not CI signal — except on #1281, which now targets release and is running the full gate.

[pdogra1299]

#1281 is merged (613709b3), so release is now at both stack heads #1280 and #1281. The remaining stack has been rebased onto it.

Rebase — one commit each, no conflicts:

94370cfb  #1287  fix(providers): complete in-turn context guard coverage across native loops
31f043cd  #1286  fix(memory): store session messages in an append-only redis list
ca47f289  #1285  fix(memory): persist tool executions on every memory backend
451fe2ba  #1283  fix(providers): guard native agent loops against mid-turn overflow
f722c458  #1282  fix(context): reclaim to a low-water mark in the step budget guard
613709b3  release

#1282 was DIRTY because it still carried the pre-squash copy of #1281's commit; that is resolved.

Verification — each branch independently, not just the tip:

branch	pnpm run check	pnpm run build	own suites
fix/cache-aware-compaction	4935 files, 0 errors	pass	step-guard 6/6
fix/loop-guard-parity	4938 files, 0 errors	pass	anthropic-guard 10/10, loop-guard-core 11/11, openai-compat-guard 9/9
fix/session-persistence	4938 files, 0 errors	pass	tool-storage-parity 10/10
fix/redis-append-only	4938 files, 0 errors	pass	redis-append-only 10/10
fix/loop-guard-completion	4939 files, 0 errors	pass	gemini-guard 11/11

67 tests, 0 failures, 0 skips. pnpm run build is run in full because pnpm run check alone does not catch everything — the nodenext resolution used by build:react-hooks surfaced two real redis typing defects in #1286 that check passed over.

Review comments: no unresolved inline findings on this PR. The only review activity to date is CodeRabbit's auto-generated walkthrough and @murdore's note about the check count, which is answered above — that gap closes as each PR's base becomes release. #1281 bore this out: once it targeted release it ran all 16 checks and passed, and that run triggered a CodeRabbit pass with three findings. Those landed after #1281 was already merged, so they are carried in a separate follow-up rather than retro-fitted here.

[pdogra1299]

Re-cascaded onto the amended #1283.

#1283 was amended to carry three CodeRabbit findings (batch splitting at the protected-tail boundary, byte-vs-String.length preview eligibility, and a calibration ratio whose two halves described different requests), so its head moved 6659c024 → 90d393cc. This branch was replanted with git rebase --onto 90d393cc 6659c024; 31edd8e2 is still the single commit it contributes.

Rebase fidelity — git diff df82fe3b..31edd8e2 (old tip → new tip) is exactly the #1283 amendment and nothing else: 9 files, all under context/, providers/anthropic, providers/openaiChatCompletionsBase and their three suites. This PR's own change is untouched.

Verified after the rebase: tsc --noEmit 0 errors, eslint 0 errors (48 pre-existing warnings, same as release), build clean, test:tool-storage-parity 10/10, test:loop-guard-core 12/12, test:openai-compat-guard 12/12, test:anthropic-guard 13/13.

No review comments to resolve. CodeRabbit posted "Review skipped — auto reviews are disabled on base/target branches other than the default branch", so the absence of findings here is absence of review, not approval. It will review for real once the base becomes release.

🔒 Single Commit Policy Validation is still red for the reason explained above — the push trigger hard-codes BASE_BRANCH="origin/release", so a stacked branch is measured against release and counts its parents' commits too. Against its own base this branch is one commit. #1287 was merged with the same check red.

[pdogra1299]

#1283 merged (56088a1f, released in 10.10.5) — and it merged with the amendment, so all three CodeRabbit findings are on release; git diff 56088a1f 90d393cc is empty.

GitHub auto-retargeted this PR to release when fix/loop-guard-parity was deleted, which left it CONFLICTING: the branch still carried the pre-squash copy of #1283's commit. Replanted with git rebase --onto origin/release 90d393cc, so only its own commit survives.

7a65b200 fix(memory): persist tool executions on every memory backend — one commit against release.

Content is unchanged by the rebase: git diff 31edd8e2 7a65b200 is CHANGELOG.md + package.json only, i.e. the 10.10.5 bump.

Verified at the rebased tree: tsc --noEmit 0 errors, eslint 0 errors (48 pre-existing warnings, same as release), build clean, test:tool-storage-parity 10/10, test:loop-guard-core 12/12, test:openai-compat-guard 12/12, test:anthropic-guard 13/13, test:step-guard 6/6.

Now that the base is release, CodeRabbit's auto-review applies for the first time on this PR — its earlier "Review skipped — auto reviews are disabled on base/target branches other than the default branch" comment was the reason there were no findings, not a clean bill. I'll resolve whatever it raises.

[github-actions]

✅ Single Commit Policy - COMPLIANT

Status: Policy requirements met • 1 commit • Valid format • Ready for merge

📊 View validation details

📝 Commit Details

• Hash: 58259917a891bed303fda243135aa891c1a2a99b
• Message: fix(memory): persist tool executions on every memory backend
• Author: Parth Dogra

✅ Validation Results

• Single commit requirement met
• No merge commits in branch
• Semantic commit message format verified
• Ready for squash merge to release branch

---

🤖 Automated validation by NeuroLink Single Commit Enforcement

[github-actions]

🤖 AI Review & Build Compliance ✅

Status: AI analysis complete • Build rules validated • Ready for review

📊 View detailed analysis results

🛡️ Analysis Complete

• ✅ Security scan (vulnerabilities, API keys)
• ✅ TypeScript safety & code quality
• ✅ Error handling & best practices
• ✅ Build rule enforcement validated
• ✅ Commit format & compliance checks

📋 Ready for Merge When

• All CI checks passing
• Manual review approved
• Any AI-flagged issues resolved

---

🤖 AI analysis complete - check individual code comments for specific feedback

[Tara-ag]

Review Summary for PR #1285: Tool Storage Parity Fix

Decision: ✅ APPROVED

This PR fixes a critical parity issue where tool execution was only persisted on Redis storage backend, causing different history shapes depending on STORAGE_TYPE. The in-memory manager now implements the same contract as the Redis backend.

Findings Summary

No issues found. This is a straightforward correctness fix with comprehensive test coverage.

Changes Overview

• Interface: Added optional storeToolExecution? method to IConversationMemoryManager
• Implementation: Added storeToolExecution to in-memory ConversationMemoryManager
• Caller: Changed from unsafe Redis type cast to capability-based check
• Helper: Updated isToolExecutionStorageAvailable() to be capability-based
• Tests: Added 10 comprehensive tests covering all contract requirements

Impact Analysis

• Blast Radius: Affects conversation memory management (208 flows touched per graph analysis)
• Affected Flows: testExternalMCPConnection (criticality 0.85), workflow/compaction/pruning paths
• Backward Compatibility: ✅ No breaking changes - method is optional on interface
• Risk Level: LOW - Pure data consistency fix, no API changes, comprehensive tests

Quality Assessment

✅ TypeScript types properly defined
✅ Null handling with nullish coalescing (??)
✅ Error handling for serialization failures
✅ Follows project conventions (no interfaces except required memory interface)
✅ Comprehensive test suite (10 tests, pure in-memory)
✅ Proper documentation/comments

Conclusion

This is a solid, well-tested parity fix that brings in-memory storage up to Redis storage's contract. All downstream paths that reason about tool batches (compaction, pruning, pair repair) will now see consistent history regardless of STORAGE_TYPE.

Recommendation: APPROVE and merge

[coderabbitai]

🧹 Nitpick comments (1)

test/continuous-test-suite-tool-storage-parity.ts (1)

157-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Verify ID-to-result mapping for parallel batches.

This test only checks role ordering and equal call/result counts. It passes if parallel results use swapped toolCallId values or tool names. Assert that each tool_result has the expected toolCallId and resolved tool name.

🤖 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-storage-parity.ts` around lines 157 - 174,
Extend the parallel-batch test around storeBatch and repairToolPairs to validate
each tool_result’s toolCallId and resolved tool name, not just counts and roles.
Assert that t1, t2, and t3 retain their corresponding read_file, grep, and ls
mappings after repair, while preserving the existing healthy-batch assertions.

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

Nitpick comments:
In `@test/continuous-test-suite-tool-storage-parity.ts`:
- Around line 157-174: Extend the parallel-batch test around storeBatch and
repairToolPairs to validate each tool_result’s toolCallId and resolved tool
name, not just counts and roles. Assert that t1, t2, and t3 retain their
corresponding read_file, grep, and ls mappings after repair, while preserving
the existing healthy-batch assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e6de29c7-d18c-4c15-b472-36b3c187efec

📥 Commits

Reviewing files that changed from the base of the PR and between e296cdd and 7a65b20.

📒 Files selected for processing (5)

• package.json
• src/lib/core/conversationMemoryManager.ts
• src/lib/neurolink.ts
• src/lib/types/conversationMemoryInterface.ts
• test/continuous-test-suite-tool-storage-parity.ts

[coderabbitai]

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.

[pdogra1299]

CodeRabbit's nitpick is resolved — it was a real gap, not just style.

The finding. The parallel-batch test asserted role ordering and equal call/result counts only, so it would have passed with two parallel results holding each other's toolCallId — which is exactly the failure the id-based repair exists to prevent. The single-pair test above it cannot catch that: with one entry, position and id agree.

The fix. a parallel batch round-trips through repairToolPairs now pins the toolCallId -> tool mapping on both the tool_call and tool_result sides after repair, and rejects any id that was never issued.

A/B-verified against the real defect class, not just re-run. Patching the in-memory storeToolExecution to resolve the tool name positionally (names[ri]) instead of via toolNameById.get(toolCallId) makes the suite report ✗ … RESULT: FAIL — while tool name is resolved onto the result via id still passes, confirming the gap was genuine. It fails loudly (✗, not ⊘), so it is not being downgraded by the isExpectedProviderError() hazard in defineSuite. Source restored clean afterwards.

Amended into the single commit — 58259917, still one commit against release.

tsc --noEmit 0 errors, eslint 0 errors, test:tool-storage-parity 10/10, test:tool-pairing 18/18.

[github-actions]

🤖 AI Review & Build Compliance ✅

Status: AI analysis complete • Build rules validated • Ready for review

📊 View detailed analysis results

🛡️ Analysis Complete

• ✅ Security scan (vulnerabilities, API keys)
• ✅ TypeScript safety & code quality
• ✅ Error handling & best practices
• ✅ Build rule enforcement validated
• ✅ Commit format & compliance checks

📋 Ready for Merge When

• All CI checks passing
• Manual review approved
• Any AI-flagged issues resolved

---

🤖 AI analysis complete - check individual code comments for specific feedback

[github-actions]

🎉 This PR is included in version 10.10.6 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀