Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 34 additions & 2 deletions plugin/pi/compaction-recovery.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
const SUMMARY_FIELD_PATHS = [
// Current Pi session_compact event shape:
// { compactionEntry: CompactionEntry, fromExtension, reason, willRetry }
// where CompactionEntry.summary holds the summary text. Listed first so the
// documented shape wins over generic legacy fallbacks.
["compactionEntry", "summary"],
["summary"],
["compactedSummary"],
["compacted_summary"],
Expand Down Expand Up @@ -47,6 +52,11 @@ export function extractCompactedSummary(event) {
return undefined;
}

/**
* Manual fallback used when the compaction summary could NOT be archived to
* Engram during the session_compact handler (no summary extracted, no session,
* or the Engram write failed). Asks the agent to save it on the next turn.
*/
export function recoveryInstruction(project) {
return (
`CRITICAL INSTRUCTION FOR COMPACTED SUMMARY:\n` +
Expand All @@ -57,8 +67,30 @@ export function recoveryInstruction(project) {
);
}

export function buildRecoveryNotice(project, context) {
const instruction = recoveryInstruction(project);
/**
* 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);
Comment on lines +70 to +93

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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 interpreting saved !== null and 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

const trimmedContext = typeof context === "string" ? context.trim() : "";
return trimmedContext ? `${trimmedContext}\n\n${instruction}` : instruction;
}
9 changes: 7 additions & 2 deletions plugin/pi/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -887,8 +887,9 @@ export default function registerEngram(pi: ExtensionAPI) {
if (sessionId) await ensureSessionBestEffort(sessionId);

const summary = extractCompactedSummary(event);
let persistedSummary = false;
if (sessionId && summary) {
await bestEffortEngramFetch("/observations", {
const saved = await bestEffortEngramFetch("/observations", {
method: "POST",
body: {
session_id: sessionId,
Expand All @@ -900,10 +901,14 @@ export default function registerEngram(pi: ExtensionAPI) {
topic_key: "session/compaction-recovery",
},
});
// bestEffortEngramFetch returns null on a failed/unreachable write. Treat
// only a confirmed response as persisted so a failed archive still falls
// back to the manual FIRST ACTION REQUIRED save instruction.
persistedSummary = saved !== null;
}

const data = await bestEffortEngramFetch<ContextResponse>(`/context?project=${encodeURIComponent(project)}`);
pendingRecoveryNotice = buildRecoveryNotice(project, data?.context);
pendingRecoveryNotice = buildRecoveryNotice(project, data?.context, persistedSummary);
});

pi.on("before_agent_start", async (event: AgentStartEvent, ctx: SessionContext) => {
Expand Down
32 changes: 32 additions & 0 deletions plugin/pi/test/compaction-recovery.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,26 @@ test("extractCompactedSummary supports top-level and nested summary fields", ()
assert.equal(extractCompactedSummary({ compaction: { content: "content summary" } }), "content summary");
});

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);
});
Comment on lines +18 to +36

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


test("recoveryInstruction keeps manual FIRST ACTION REQUIRED fallback", () => {
const notice = recoveryInstruction("engram");
assert.match(notice, /FIRST ACTION REQUIRED/);
Expand All @@ -27,3 +47,15 @@ test("buildRecoveryNotice prefixes context when available", () => {
assert.equal(buildRecoveryNotice("engram", "existing context").startsWith("existing context\n\nCRITICAL"), true);
assert.equal(buildRecoveryNotice("engram", "").startsWith("CRITICAL"), true);
});

test("buildRecoveryNotice drops the manual save instruction once persisted", () => {
// Default (persisted=false): manual save instruction is present.
const fallback = buildRecoveryNotice("engram", undefined);
assert.match(fallback, /FIRST ACTION REQUIRED/);

// After a successful archive: no manual save instruction, context still surfaced.
const persisted = buildRecoveryNotice("engram", "recent project memory", true);
assert.doesNotMatch(persisted, /FIRST ACTION REQUIRED/);
assert.match(persisted, /recent project memory/);
assert.match(persisted, /already saved to Engram/);
});