fix(compression): fallback to synthetic compression when memory provider is noop#757
Open
Chewji9875 wants to merge 1 commit into
Open
fix(compression): fallback to synthetic compression when memory provider is noop#757Chewji9875 wants to merge 1 commit into
Chewji9875 wants to merge 1 commit into
Conversation
|
@Chewji9875 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a synthetic compression fallback to the compression handler: when the memory provider is ChangesSynthetic Compression Fallback for Noop Provider
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Chewji9875
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
test/auto-compress.test.ts (1)
82-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe “unset default” test is no longer testing an unset env.
Line 82 forces
AGENTMEMORY_AUTO_COMPRESS="false"before the test at Line 88, so this case now overlaps with the explicit-false scenario and no longer validates true default behavior.Suggested fix
it("default (AGENTMEMORY_AUTO_COMPRESS unset): does NOT fire mem::compress", async () => { + delete process.env["AGENTMEMORY_AUTO_COMPRESS"]; const { registerObserveFunction } = await import( "../src/functions/observe.js" );🤖 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/auto-compress.test.ts` around lines 82 - 89, The test labeled "default (AGENTMEMORY_AUTO_COMPRESS unset): does NOT fire mem::compress" is not actually running with the env unset because beforeEach/afterEach force AGENTMEMORY_AUTO_COMPRESS="false"; update the test setup so the env is truly unset when importing registerObserveFunction: either remove the global beforeEach/afterEach assignment of process.env["AGENTMEMORY_AUTO_COMPRESS"] = "false" or explicitly delete process.env["AGENTMEMORY_AUTO_COMPRESS"] (e.g., delete process.env["AGENTMEMORY_AUTO_COMPRESS"]) at the start of that specific test before the dynamic import of registerObserveFunction so the default behavior is exercised.
🧹 Nitpick comments (2)
test/auto-compress.test.ts (1)
255-259: ⚡ Quick winAssert that noop fallback does not call provider methods.
To lock the fallback contract, add explicit expectations that
compress/summarizewere not invoked.Suggested assertions
expect(mockMetricsStore.record).not.toHaveBeenCalled(); + expect(mockProvider.compress).not.toHaveBeenCalled(); + expect(mockProvider.summarize).not.toHaveBeenCalled();Also applies to: 297-297
🤖 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/auto-compress.test.ts` around lines 255 - 259, Add explicit assertions that the noop fallback does not invoke provider methods by asserting mockProvider.compress and mockProvider.summarize were not called (e.g., expect(mockProvider.compress).not.toHaveBeenCalled() and expect(mockProvider.summarize).not.toHaveBeenCalled()) in the test that constructs mockProvider (the noop provider case) and duplicate the same assertions in the other test covering the noop fallback (the one around the second occurrence), ensuring you reference the mockProvider object used in those tests.src/functions/compress.ts (1)
110-142: ⚡ Quick winParallelize independent async side effects in noop path.
Line 110 awaits vector indexing before starting stream publishes at Line 117. These operations are independent, so running them together (e.g., a single
Promise.allSettled) will reduce latency and isolate failures better.As per coding guidelines:
Use parallel operations where possible (Promise.all for independent kv writes/reads).🤖 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/functions/compress.ts` around lines 110 - 142, The current code awaits vectorIndexAddGuarded(...) before firing the two sdk.trigger(...) calls, but those are independent; change to run all three side-effects concurrently by removing the separate await and using a single Promise.allSettled([...]) that includes the Promise returned by vectorIndexAddGuarded(synthetic.id, synthetic.sessionId, ...) and the two sdk.trigger(...) calls (which reference STREAM and TriggerAction.Void()); this will parallelize the operations and keep failures isolated by handling their settled results.
🤖 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/functions/compress.ts`:
- Around line 93-163: The noop-fallback branch persists data and publishes
streams but never records an audit entry; add a call to recordAudit(...) before
returning success to satisfy the "state-changing operations" guideline.
Specifically, after the streamResults loop (and before logger.info and the
return) invoke recordAudit with the session id (data.sessionId), the observation
id (data.observationId or synthetic.id), an action string like
"compress:no-op-fallback" and relevant metadata (e.g., type: synthetic.type,
importance: synthetic.importance, compressed: true, confidence:
synthetic.confidence); await the call and handle/log any non-fatal errors
similarly to other async ops so failures don’t block the main flow.
---
Outside diff comments:
In `@test/auto-compress.test.ts`:
- Around line 82-89: The test labeled "default (AGENTMEMORY_AUTO_COMPRESS
unset): does NOT fire mem::compress" is not actually running with the env unset
because beforeEach/afterEach force AGENTMEMORY_AUTO_COMPRESS="false"; update the
test setup so the env is truly unset when importing registerObserveFunction:
either remove the global beforeEach/afterEach assignment of
process.env["AGENTMEMORY_AUTO_COMPRESS"] = "false" or explicitly delete
process.env["AGENTMEMORY_AUTO_COMPRESS"] (e.g., delete
process.env["AGENTMEMORY_AUTO_COMPRESS"]) at the start of that specific test
before the dynamic import of registerObserveFunction so the default behavior is
exercised.
---
Nitpick comments:
In `@src/functions/compress.ts`:
- Around line 110-142: The current code awaits vectorIndexAddGuarded(...) before
firing the two sdk.trigger(...) calls, but those are independent; change to run
all three side-effects concurrently by removing the separate await and using a
single Promise.allSettled([...]) that includes the Promise returned by
vectorIndexAddGuarded(synthetic.id, synthetic.sessionId, ...) and the two
sdk.trigger(...) calls (which reference STREAM and TriggerAction.Void()); this
will parallelize the operations and keep failures isolated by handling their
settled results.
In `@test/auto-compress.test.ts`:
- Around line 255-259: Add explicit assertions that the noop fallback does not
invoke provider methods by asserting mockProvider.compress and
mockProvider.summarize were not called (e.g.,
expect(mockProvider.compress).not.toHaveBeenCalled() and
expect(mockProvider.summarize).not.toHaveBeenCalled()) in the test that
constructs mockProvider (the noop provider case) and duplicate the same
assertions in the other test covering the noop fallback (the one around the
second occurrence), ensuring you reference the mockProvider object used in those
tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dca6d6a0-bd48-4416-846f-6fed180d6c14
📒 Files selected for processing (2)
src/functions/compress.tstest/auto-compress.test.ts
Comment on lines
+93
to
+163
| await kv.set( | ||
| KV.observations(data.sessionId), | ||
| data.observationId, | ||
| synthetic, | ||
| ); | ||
|
|
||
| try { | ||
| getSearchIndex().add(synthetic); | ||
| } catch (err) { | ||
| logger.warn("Failed to index compressed observation into BM25", { | ||
| obsId: synthetic.id, | ||
| sessionId: synthetic.sessionId, | ||
| title: synthetic.title, | ||
| error: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| } | ||
|
|
||
| await vectorIndexAddGuarded( | ||
| synthetic.id, | ||
| synthetic.sessionId, | ||
| synthetic.title + " " + (synthetic.narrative || ""), | ||
| { kind: "observation", logId: synthetic.id }, | ||
| ); | ||
|
|
||
| const streamResults = await Promise.allSettled([ | ||
| sdk.trigger({ | ||
| function_id: "stream::set", | ||
| payload: { | ||
| stream_name: STREAM.name, | ||
| group_id: STREAM.group(data.sessionId), | ||
| item_id: data.observationId, | ||
| data: { type: "compressed", observation: synthetic }, | ||
| }, | ||
| }), | ||
| sdk.trigger({ | ||
| function_id: "stream::send", | ||
| payload: { | ||
| stream_name: STREAM.name, | ||
| group_id: STREAM.viewerGroup, | ||
| id: `compressed-${data.observationId}`, | ||
| type: "compressed_observation", | ||
| data: { | ||
| type: "compressed", | ||
| observation: synthetic, | ||
| sessionId: data.sessionId, | ||
| }, | ||
| }, | ||
| action: TriggerAction.Void(), | ||
| }), | ||
| ]); | ||
|
|
||
| for (const res of streamResults) { | ||
| if (res.status === "rejected") { | ||
| logger.warn("Non-fatal stream publish failure after compress", { | ||
| sessionId: data.sessionId, | ||
| observationId: data.observationId, | ||
| error: | ||
| res.reason instanceof Error | ||
| ? res.reason.message | ||
| : String(res.reason), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| logger.info("Observation compressed (synthetic noop fallback)", { | ||
| obsId: data.observationId, | ||
| type: synthetic.type, | ||
| importance: synthetic.importance, | ||
| }); | ||
|
|
||
| return { success: true, compressed: synthetic, qualityScore: synthetic.confidence * 100 }; |
Contributor
There was a problem hiding this comment.
Add audit logging for noop fallback mutations.
Line 93 persists data and Lines 117-142 publish stream events, but this branch never calls recordAudit(). Please record an audit entry for this state-changing path before returning success.
As per coding guidelines: Use recordAudit() for state-changing operations.
🤖 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/functions/compress.ts` around lines 93 - 163, The noop-fallback branch
persists data and publishes streams but never records an audit entry; add a call
to recordAudit(...) before returning success to satisfy the "state-changing
operations" guideline. Specifically, after the streamResults loop (and before
logger.info and the return) invoke recordAudit with the session id
(data.sessionId), the observation id (data.observationId or synthetic.id), an
action string like "compress:no-op-fallback" and relevant metadata (e.g., type:
synthetic.type, importance: synthetic.importance, compressed: true, confidence:
synthetic.confidence); await the call and handle/log any non-fatal errors
similarly to other async ops so failures don’t block the main flow.
…ory provider is noop
Chewji9875
force-pushed
the
fix/compress-noop-fallback
branch
from
ee68089 to
97c69c4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHAT
Implements an early return/fallback to local zero-LLM synthetic compression when the active memory provider is
NoopProvider(noop).WHERE
src/functions/compress.ts(Adds theprovider.name === "noop"check to generate and write synthetic observations).test/auto-compress.test.ts(Adds unit tests verifying the graceful fallback to synthetic compression and zero metrics pollution, while correcting environment variable cleanups in test lifecycles).WHEN
When
mem::compressis called (either manually or automatically via theobservehook) and the system has no active/valid API keys (defaulting to thenoopprovider).WHY
Previously, the noop provider blindly passed empty strings to the LLM compressor, causing XML validation failures 100% of the time, polluting OTel metrics with failures, and leading to complete data loss since observations were never saved. This fallback ensures data integrity and clean metric logs.
Summary by CodeRabbit
Release Notes
New Features
Tests