fix(pi): archive current Pi compaction shape and avoid duplicate save - #673
Open
noxos-os wants to merge 1 commit into
Open
fix(pi): archive current Pi compaction shape and avoid duplicate save#673noxos-os wants to merge 1 commit into
noxos-os wants to merge 1 commit into
Conversation
Pi's session_compact event carries the summary at event.compactionEntry.summary, but extractCompactedSummary only inspected legacy top-level/nested paths, so the auto-archive to Engram never fired on current Pi and the agent was always asked to save the summary manually. - Add ['compactionEntry','summary'] as the first SUMMARY_FIELD_PATH so the documented event shape is extracted. - Track whether the archive POST actually succeeded and pass a persisted flag to buildRecoveryNotice. When persisted, inject a continuity-only notice and drop the manual FIRST ACTION REQUIRED: mem_session_summary instruction so the next turn does not duplicate the save it just completed. A failed or unreachable write still falls back to the manual instruction. - Cover both the new event shape and the persisted flag in tests.
📝 WalkthroughWalkthroughCompaction recovery now reads the current ChangesCompaction recovery flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SessionCompactHandler
participant EngramObservations
participant buildRecoveryNotice
SessionCompactHandler->>EngramObservations: Persist compaction summary
EngramObservations-->>SessionCompactHandler: Return response or null
SessionCompactHandler->>buildRecoveryNotice: Pass persistedSummary
buildRecoveryNotice-->>SessionCompactHandler: Build acknowledgement or recovery notice
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@plugin/pi/compaction-recovery.js`:
- Around line 70-93: Move persisted-versus-manual recovery policy out of
plugin/pi/compaction-recovery.js: remove persistedAcknowledgement and the
persisted branch in buildRecoveryNotice, and consume the status or notice
produced by the core layer instead. In plugin/pi/index.ts, forward the archive
result from the compaction flow without interpreting saved !== null or selecting
user-facing recovery behavior; both affected sites should remain thin adapters
that parse input, call the core API/tool, and return results.
In `@plugin/pi/test/compaction-recovery.test.mjs`:
- Around line 18-36: Update the extractCompactedSummary tests to include a
conflicting legacy summary alongside compactionEntry.summary, and assert that
the modern compactionEntry.summary value is returned. Keep the existing native,
extension, and empty-summary coverage unchanged.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 178120ad-0224-411e-9a22-4d01e81c9cb1
📒 Files selected for processing (3)
plugin/pi/compaction-recovery.jsplugin/pi/index.tsplugin/pi/test/compaction-recovery.test.mjs
Comment on lines
+70
to
+93
| /** | ||
| * Used when the compaction summary was already archived to Engram by | ||
| * gentle-engram. Still surfaces retrieved project context for continuity, but | ||
| * omits the manual `FIRST ACTION REQUIRED: mem_session_summary` instruction so | ||
| * the agent does not duplicate the save it just completed. | ||
| */ | ||
| function persistedAcknowledgement(project) { | ||
| return ( | ||
| `Compaction recovery summary was already saved to Engram (${project}) by gentle-engram. ` + | ||
| `No manual mem_session_summary call is needed for this compaction. ` + | ||
| `Call mem_context if you need additional recent project memory.` | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Build the recovery notice injected before the next agent turn. | ||
| * | ||
| * @param {string} project Engram project name. | ||
| * @param {string|undefined} context Retrieved Engram project context, if any. | ||
| * @param {boolean} [persisted=false] True when the compaction summary was | ||
| * already archived to Engram; suppresses the manual save instruction. | ||
| */ | ||
| export function buildRecoveryNotice(project, context, persisted = false) { | ||
| const instruction = persisted ? persistedAcknowledgement(project) : recoveryInstruction(project); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move compaction persistence policy into the core layer.
The PR adds the persisted/manual recovery decision across two plugin/** modules. Keep the plugin adapter limited to parsing input, invoking the core Go API/tool, and returning results.
plugin/pi/compaction-recovery.js#L70-L93: remove the plugin-owned persisted acknowledgement and recovery-mode branch; consume a core-produced status or notice.plugin/pi/index.ts#L890-L911: forward the archive result instead of interpretingsaved !== nulland selecting the user-facing recovery behavior.
As per path instructions, adapters under plugin/** must stay thin: “parse input, call the core Go API/tool, return. No business logic, no external runtime deps.”
📍 Affects 2 files
plugin/pi/compaction-recovery.js#L70-L93(this comment)plugin/pi/index.ts#L890-L911
🤖 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 `@plugin/pi/compaction-recovery.js` around lines 70 - 93, Move
persisted-versus-manual recovery policy out of plugin/pi/compaction-recovery.js:
remove persistedAcknowledgement and the persisted branch in buildRecoveryNotice,
and consume the status or notice produced by the core layer instead. In
plugin/pi/index.ts, forward the archive result from the compaction flow without
interpreting saved !== null or selecting user-facing recovery behavior; both
affected sites should remain thin adapters that parse input, call the core
API/tool, and return results.
Source: Path instructions
Comment on lines
+18
to
+36
| test("extractCompactedSummary reads the current Pi session_compact event shape", () => { | ||
| // Documented Pi event: { compactionEntry: CompactionEntry, ... } where | ||
| // CompactionEntry.summary holds the summary text. | ||
| assert.equal( | ||
| extractCompactedSummary({ compactionEntry: { summary: "native pi summary" } }), | ||
| "native pi summary", | ||
| ); | ||
| assert.equal( | ||
| extractCompactedSummary({ | ||
| compactionEntry: { summary: " extension summary " }, | ||
| fromExtension: true, | ||
| reason: "threshold", | ||
| willRetry: false, | ||
| }), | ||
| "extension summary", | ||
| ); | ||
| // Empty compactionEntry.summary is ignored (falls through / returns undefined). | ||
| assert.equal(extractCompactedSummary({ compactionEntry: { summary: " " } }), undefined); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test modern-field precedence explicitly.
These cases prove that compactionEntry.summary is readable, but not that it takes precedence over legacy fields. Add a conflicting legacy summary and assert that the modern value wins.
Suggested test
assert.equal(
extractCompactedSummary({ compactionEntry: { summary: "native pi summary" } }),
"native pi summary",
);
+ assert.equal(
+ extractCompactedSummary({
+ compactionEntry: { summary: "modern summary" },
+ payload: { summary: "legacy summary" },
+ }),
+ "modern summary",
+ );📝 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.
Suggested change
| test("extractCompactedSummary reads the current Pi session_compact event shape", () => { | |
| // Documented Pi event: { compactionEntry: CompactionEntry, ... } where | |
| // CompactionEntry.summary holds the summary text. | |
| assert.equal( | |
| extractCompactedSummary({ compactionEntry: { summary: "native pi summary" } }), | |
| "native pi summary", | |
| ); | |
| assert.equal( | |
| extractCompactedSummary({ | |
| compactionEntry: { summary: " extension summary " }, | |
| fromExtension: true, | |
| reason: "threshold", | |
| willRetry: false, | |
| }), | |
| "extension summary", | |
| ); | |
| // Empty compactionEntry.summary is ignored (falls through / returns undefined). | |
| assert.equal(extractCompactedSummary({ compactionEntry: { summary: " " } }), undefined); | |
| }); | |
| test("extractCompactedSummary reads the current Pi session_compact event shape", () => { | |
| // Documented Pi event: { compactionEntry: CompactionEntry, ... } where | |
| // CompactionEntry.summary holds the summary text. | |
| assert.equal( | |
| extractCompactedSummary({ compactionEntry: { summary: "native pi summary" } }), | |
| "native pi summary", | |
| ); | |
| assert.equal( | |
| extractCompactedSummary({ | |
| compactionEntry: { summary: "modern summary" }, | |
| payload: { summary: "legacy summary" }, | |
| }), | |
| "modern summary", | |
| ); | |
| assert.equal( | |
| extractCompactedSummary({ | |
| compactionEntry: { summary: " extension summary " }, | |
| fromExtension: true, | |
| reason: "threshold", | |
| willRetry: false, | |
| }), | |
| "extension summary", | |
| ); | |
| // Empty compactionEntry.summary is ignored (falls through / returns undefined). | |
| assert.equal(extractCompactedSummary({ compactionEntry: { summary: " " } }), undefined); | |
| }); |
🤖 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 `@plugin/pi/test/compaction-recovery.test.mjs` around lines 18 - 36, Update the
extractCompactedSummary tests to include a conflicting legacy summary alongside
compactionEntry.summary, and assert that the modern compactionEntry.summary
value is returned. Keep the existing native, extension, and empty-summary
coverage unchanged.
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.
Problem
gentle-engram'ssession_compacthandler never auto-archived the compaction summary to Engram on current Pi, becauseextractCompactedSummaryonly inspected legacy top-level / nested paths and missed Pi's documented event shape:As a result,
summarywasundefined, the/observationsPOST was skipped, and the agent was always asked to save the summary manually via theFIRST ACTION REQUIRED: mem_session_summarynotice on the next turn — even though that was meant to be a fallback.Separately,
buildRecoveryNoticealways appended the manual-save instruction, so once extraction is fixed the agent would duplicate the save it just completed.Fix
plugin/pi/compaction-recovery.js["compactionEntry", "summary"]as the firstSUMMARY_FIELD_PATHso the documented event shape is extracted (legacy fallbacks are preserved).buildRecoveryNotice(project, context, persisted = false): whenpersisted, emit a continuity-only acknowledgement (still surfaces retrieved Engram context) and omit the manualFIRST ACTION REQUIRED: mem_session_summaryinstruction. Defaultfalsekeeps existing behavior for any caller.plugin/pi/index.tssession_compact, treat the archive POST's confirmed response aspersistedand pass it tobuildRecoveryNotice.bestEffortEngramFetchreturnsnullon a failed/unreachable write, so a failed archive still falls back to the manual save instruction — only a confirmed success suppresses it.Behavior matrix
FIRST ACTION REQUIRED(retry)FIRST ACTION REQUIRED(unchanged)Tests
extractCompactedSummarynow covers{ compactionEntry: { summary } }(native + extension-provided, plus whitespace-only ignored).buildRecoveryNotice(..., true)asserts noFIRST ACTION REQUIREDwhile still surfacing context.Full local run:
node --test plugin/pi/test/*.test.mjs→ 44 pass / 0 fail.Backward compatible:
buildRecoveryNoticethird arg defaults tofalse, and the new extraction path is strictly additive.Summary by CodeRabbit
Bug Fixes
Tests