fix(amp): capture agent-end messages incrementally - #497
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe Amp plugin now passes ChangesMessage-aware synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AgentEnd as agent.end event
participant Sync as SessionSyncManager
participant Capture as captureThread
participant Threads as /threads
AgentEnd->>Sync: scheduleSync(threadId, messages)
Sync->>Capture: captureThread(threadId, messages)
Capture->>Threads: write supplied messages
Sync->>Sync: retain pending messages during coalescing
Sync->>Capture: reschedule with pending messages
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ 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 |
67e50f3 to
b601261
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nowledge-mem-amp-plugin/tests/sync.test.ts (1)
303-315: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest multiple supplied batches.
This test covers one batch and verifies that the transcript reader is bypassed. It does not detect loss of an earlier
agent.endbatch during debounce or an in-flight capture.Schedule two distinct batches before the next capture completes. Assert that both batches reach the persistence request in order, and that
readThreadMessagesis not called.🤖 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 `@nowledge-mem-amp-plugin/tests/sync.test.ts` around lines 303 - 315, Update the test around SessionSyncManager.scheduleSync to submit two distinct agent.end batches before the next capture completes, then allow the capture to finish and assert the persistence request contains both batches in order. Retain the assertion that readThreadMessages is not called, and ensure the scenario covers batches queued during debounce or an in-flight capture.
🤖 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 `@nowledge-mem-amp-plugin/src/sync.ts`:
- Around line 63-64: Replace the single latest-batch pendingMessages slot and
associated debounce rescheduling in the sync flow with a FIFO queue that retains
every agent.end.messages batch, including batches received while a capture is in
flight. Drain the queue in order without allowing completion of an older capture
to cancel a newer scheduled batch, and add a regression test covering two
distinct event batches to verify neither is lost.
---
Nitpick comments:
In `@nowledge-mem-amp-plugin/tests/sync.test.ts`:
- Around line 303-315: Update the test around SessionSyncManager.scheduleSync to
submit two distinct agent.end batches before the next capture completes, then
allow the capture to finish and assert the persistence request contains both
batches in order. Retain the assertion that readThreadMessages is not called,
and ensure the scenario covers batches queued during debounce or an in-flight
capture.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b906410-9bc4-4cd0-ad2a-4478a97ae1ed
📒 Files selected for processing (4)
nowledge-mem-amp-plugin/src/index.tsnowledge-mem-amp-plugin/src/sync.tsnowledge-mem-amp-plugin/tests/index.test.tsnowledge-mem-amp-plugin/tests/sync.test.ts
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 `@nowledge-mem-amp-plugin/src/index.ts`:
- Around line 86-91: Update both manual save paths that invoke
SessionSyncManager.syncNow to limit the transcript to the latest 20
non-compacted messages before synchronization. Ensure the bounded, filtered
message collection is passed to syncNow while leaving automatic agent.end
capture unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b894a098-6fb5-4075-bb93-d08da0a81273
📒 Files selected for processing (3)
nowledge-mem-amp-plugin/src/index.tsnowledge-mem-amp-plugin/src/sync.tsnowledge-mem-amp-plugin/tests/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- nowledge-mem-amp-plugin/src/sync.ts
- nowledge-mem-amp-plugin/tests/index.test.ts
| * Reads the full message transcript for an explicit manual save through the | ||
| * Amp SDK. | ||
| * | ||
| * Automatic capture uses the messages already included in the `agent.end` | ||
| * event, avoiding a Plugin RPC entirely. Manual saves retain their full-history | ||
| * contract even though that explicit operation may require a large RPC read. |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '1,230p' nowledge-mem-amp-plugin/src/index.ts
printf '\n--- related symbols ---\n'
rg -n "SessionSyncManager|thread\.messages|full: true|agent\.end|manual|save" nowledge-mem-amp-plugin -g '*.{ts,tsx,js}'Repository: nowledge-co/community
Length of output: 33178
🏁 Script executed:
# Trace the manual-save and automatic-capture call paths without executing repository code.
rg -n -C 8 "SessionSyncManager|thread\.messages|full: true|agent\.end|manual|save" nowledge-mem-amp-plugin
printf '\n--- candidate files ---\n'
git ls-files | rg 'SessionSyncManager|amp-plugin|sync'Repository: nowledge-co/community
Length of output: 50378
🏁 Script executed:
printf '%s\n' '--- sync capture path ---'
sed -n '123,350p' nowledge-mem-amp-plugin/src/sync.ts
printf '%s\n' '--- index integration tests ---'
sed -n '1,180p' nowledge-mem-amp-plugin/tests/index.test.ts
printf '%s\n' '--- bounded/non-compacted references ---'
rg -n -i -C 3 "20|compact|non.?comp|latest|full history|full-history" nowledge-mem-amp-plugin/src nowledge-mem-amp-plugin/tests nowledge-mem-amp-plugin/README.md nowledge-mem-amp-plugin/CHANGELOG.mdRepository: nowledge-co/community
Length of output: 46028
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
index = Path("nowledge-mem-amp-plugin/src/index.ts").read_text()
sync = Path("nowledge-mem-amp-plugin/src/sync.ts").read_text()
tools = Path("nowledge-mem-amp-plugin/src/tools.ts").read_text()
commands = Path("nowledge-mem-amp-plugin/src/commands.ts").read_text()
checks = {
"manual SDK read is full": 'thread.messages({ full: true })' in index,
"manual sync reads injected transcript when no batch is supplied":
'const rawMessages = options.messages ?? await this.ports.readThreadMessages(threadId)' in sync,
"manual sync calls captureThread without a message batch":
'return this.captureThread(threadId, { force: true })' in sync,
"save-thread tool calls syncNow":
'const result = await deps.syncManager.syncNow(threadId)' in tools,
"save-thread command calls syncNow":
'syncManager.syncNow(threadId)' in commands,
"manual path applies a 20-message limit":
'.slice(-20)' in index or '.slice(-20)' in sync or '.slice(-20)' in tools or '.slice(-20)' in commands,
"manual path filters compacted messages":
'compacted' in index.lower() or 'compacted' in sync.lower() or
'compacted' in tools.lower() or 'compacted' in commands.lower(),
}
for name, value in checks.items():
print(f"{name}: {value}")
PYRepository: nowledge-co/community
Length of output: 490
Bound manual saves to the latest 20 non-compacted messages.
Both manual save paths call SessionSyncManager.syncNow, which reads thread.messages({ full: true }) without a message limit or compacted-message filter. Bound the transcript before synchronization to avoid unbounded Plugin RPC 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 `@nowledge-mem-amp-plugin/src/index.ts` around lines 86 - 91, Update both
manual save paths that invoke SessionSyncManager.syncNow to limit the transcript
to the latest 20 non-compacted messages before synchronization. Ensure the
bounded, filtered message collection is passed to syncNow while leaving
automatic agent.end capture unchanged.
c7ffe5e to
157d753
Compare
2 participants
Summary
agent.endpayload instead of querying the thread transcript.agent.endpayload when another capture is already in flight.This removes the expensive Plugin RPC read from the automatic
agent.endpath, which can stall long Amp sessions. Manual save retains its full-history contract.Summary by CodeRabbit
Performance Improvements
Bug Fixes