From 944859838a7e53b6fe76de3392bfa46f1587f9d7 Mon Sep 17 00:00:00 2001 From: "Bowen M." Date: Mon, 10 Aug 2026 15:44:15 +0800 Subject: [PATCH] fix: harden exit summary (skip empty summaries, bound shutdown work) Two robustness fixes for the exit summary on session_shutdown, raised in the follow-up discussion on #26: 1. Persist gate: summaries whose every section is "None." (trivial sessions with nothing worth recording) are no longer written to the daily log. The prompt tells the model to emit "None." per section when there is nothing to record, but the write path only checked for a non-empty string, so boilerplate entries were appended, re-injected at every session start, and indexed by qmd. New isExitSummaryEmpty() helper completes the curated-write gate. 2. Timeout: summary generation is now raced against a self-imposed timeout (PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS, default 10s). Pi core awaits shutdown handlers with no timeout, so a hanging provider previously blocked quitting indefinitely. On expiry nothing is persisted; the late result is dropped. Refs #26 Signed-off-by: Bowen M. Signed-off-by: Zeng, Jay --- CHANGELOG.md | 12 +++++ README.md | 1 + index.ts | 46 ++++++++++++++++++- test/unit.test.ts | 115 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 172 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ba9232..ef03320 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,18 @@ follows [Semantic Versioning](https://semver.org/). summaries to a cheaper/faster model. Unresolvable specs fall back to the session model with a warning. (#26) +### Fixed + +- Exit summaries whose every section is "None." (trivial sessions with nothing + worth recording) are no longer persisted to the daily log, completing the + curated-write gate — previously such boilerplate entries were appended, + re-injected at every session start, and indexed by qmd. (#26) +- Exit-summary generation on `session_shutdown` is now bounded by a + self-imposed timeout (`PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS`, default 10s). + Pi core awaits shutdown handlers with no timeout, so a hanging provider + previously blocked quitting indefinitely. On expiry nothing is persisted. + (#26) + ### Changed - Reduced pull-request CI duplication and setup overhead by consolidating the diff --git a/README.md b/README.md index 1963c5b..819156e 100644 --- a/README.md +++ b/README.md @@ -193,6 +193,7 @@ This ensures in-progress context survives compaction and is visible in the next | `PI_MEMORY_SUMMARIZE_TRANSITIONS` | `1`, `true`, `yes`, `on` | unset | Also write exit summaries during lifecycle transitions (`/reload`, `/new`, `/resume`, `/fork`). By default these transitions skip summaries for speed. | | `PI_MEMORY_EXIT_SUMMARY` | `0`, `off`, `false`, `no` to disable | unset (enabled) | Disable the exit summary on real quit (Ctrl+D, `/quit`, session end). Quitting then does no LLM call and no `qmd update`, so it is instant; explicit `memory_write` during sessions is unaffected. | | `PI_MEMORY_EXIT_SUMMARY_MODEL` | `provider/model-id` | unset (session model) | Model used to write the exit summary, e.g. a cheaper/faster one. Unresolvable specs fall back to the session model with a warning. | +| `PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS` | positive integer (milliseconds) | `10000` | Self-imposed timeout for exit-summary generation on quit. Pi awaits shutdown handlers with no timeout, so a hanging provider would otherwise block quitting indefinitely. On expiry nothing is persisted. | ## Troubleshooting diff --git a/index.ts b/index.ts index 07610b9..3ad496d 100644 --- a/index.ts +++ b/index.ts @@ -508,6 +508,35 @@ export function isExitSummaryEnabled(): boolean { return !(value === "0" || value === "off" || value === "false" || value === "no"); } +/** + * True when a generated exit summary carries no actual content — every section + * is empty or "None.". The summary prompt instructs the model to write "None." + * under each heading when nothing is worth recording; persisting those blocks + * would pollute the daily log (re-injected every session start) and the qmd + * index with boilerplate. + */ +export function isExitSummaryEmpty(summary: string): boolean { + const contentLines = summary + .split("\n") + .map((line) => line.trim()) + .filter((line) => line.length > 0 && !line.startsWith("#")); + if (contentLines.length === 0) return true; + return contentLines.every((line) => /^none\.?$/i.test(line.replace(/^[-*+]\s*/, ""))); +} + +const DEFAULT_EXIT_SUMMARY_TIMEOUT_MS = 10_000; + +/** + * Self-imposed timeout for the exit-summary work on session_shutdown. Pi core + * awaits shutdown handlers with no timeout, and generateExitSummary() is only + * bounded by the provider's own timeout — a hanging provider would otherwise + * block quitting indefinitely. Override with PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS. + */ +export function getExitSummaryTimeoutMs(): number { + const configured = Number(process.env.PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS); + return Number.isInteger(configured) && configured > 0 ? configured : DEFAULT_EXIT_SUMMARY_TIMEOUT_MS; +} + export function shouldSkipExitSummaryForReason(reason: string | undefined): boolean { if (!reason) return false; if (shouldSummarizeLifecycleTransitions()) return false; @@ -1458,15 +1487,26 @@ export default function (pi: ExtensionAPI) { const reason = exitSummaryReason ?? "session-end"; exitSummaryReason = null; + let summaryTimer: ReturnType | undefined; try { if (reason) { ensureDirs(); - const result = await generateExitSummary(ctx); + // Race the summary against a self-imposed timeout: pi core awaits + // shutdown handlers with no timeout, so a hanging provider would + // otherwise block quitting indefinitely. On expiry nothing is + // persisted (the late result, if any, is simply dropped). + const summaryWork = generateExitSummary(ctx); + const expired = new Promise((resolve) => { + summaryTimer = setTimeout(() => resolve(null), getExitSummaryTimeoutMs()); + }); + const result = await Promise.race([summaryWork, expired]); // Only persist real summaries. The old fallback appended an // all-"None." boilerplate block on every failed summarization // (no API key, empty response, …), polluting the daily log — // which is then re-injected into context every session start. - if (result.hasMessages && result.summary) { + // Successful-but-empty summaries (every section "None.") are + // filtered out for the same reason. + if (result?.hasMessages && result.summary && !isExitSummaryEmpty(result.summary)) { const summary = result.summary; const sid = shortSessionId(ctx.sessionManager.getSessionId()); const ts = nowTimestamp(); @@ -1480,6 +1520,7 @@ export default function (pi: ExtensionAPI) { } } } finally { + if (summaryTimer) clearTimeout(summaryTimer); if (updateTimer) { clearTimeout(updateTimer); updateTimer = null; @@ -2371,6 +2412,7 @@ export default function (pi: ExtensionAPI) { `- PI_MEMORY_DIR: ${process.env.PI_MEMORY_DIR ? "set" : "default"}`, `- PI_MEMORY_EXIT_SUMMARY: ${isExitSummaryEnabled() ? "enabled" : "disabled"}`, `- PI_MEMORY_EXIT_SUMMARY_MODEL: ${process.env.PI_MEMORY_EXIT_SUMMARY_MODEL?.trim() || "session model"}`, + `- PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS: ${getExitSummaryTimeoutMs()}`, ); return { diff --git a/test/unit.test.ts b/test/unit.test.ts index 3dd7478..f66f08b 100644 --- a/test/unit.test.ts +++ b/test/unit.test.ts @@ -31,7 +31,9 @@ import { ensureDirs, ensureQmdEmbed, forgetBlocks, + getExitSummaryTimeoutMs, getQmdSearchTimeoutMs, + isExitSummaryEmpty, isExitSummaryEnabled, nowTimestamp, parseScratchpad, @@ -1662,6 +1664,119 @@ describe("lifecycle hooks", () => { }); }); + describe("isExitSummaryEmpty", () => { + test("treats all-None summaries as empty", () => { + const summary = [ + "### Decisions", + "- None.", + "### Lessons Learned", + "- None.", + "### Notes", + "- None.", + "### Follow-ups", + "- None.", + ].join("\n"); + expect(isExitSummaryEmpty(summary)).toBe(true); + }); + + test("tolerates formatting variations (bullets, case, missing period)", () => { + expect(isExitSummaryEmpty("### Decisions\nNone\n### Notes\n* none.")).toBe(true); + expect(isExitSummaryEmpty("None.")).toBe(true); + expect(isExitSummaryEmpty("### Decisions\n### Notes")).toBe(true); + }); + + test("keeps summaries with any real content", () => { + const summary = [ + "### Decisions", + "- None.", + "### Lessons Learned", + "- None.", + "### Notes", + "- User prefers dark mode.", + "### Follow-ups", + "- None.", + ].join("\n"); + expect(isExitSummaryEmpty(summary)).toBe(false); + expect(isExitSummaryEmpty("### Notes\n- Discussed None. vs null semantics")).toBe(false); + }); + }); + + describe("exit summary shutdown timeout", () => { + const fourMessageBranch = () => [ + { + type: "message", + message: { + role: "user", + content: [{ type: "text", text: "Please remember we chose dark mode." }], + timestamp: Date.now(), + }, + }, + { + type: "message", + message: { + role: "assistant", + content: [{ type: "text", text: "Noted, using it for the storage layer." }], + timestamp: Date.now(), + }, + }, + { + type: "message", + message: { + role: "user", + content: [{ type: "text", text: "Also migrate the config to match." }], + timestamp: Date.now(), + }, + }, + { + type: "message", + message: { + role: "assistant", + content: [{ type: "text", text: "Done — config migrated and tests pass." }], + timestamp: Date.now(), + }, + }, + ]; + + let savedTimeout: string | undefined; + beforeEach(() => { + savedTimeout = process.env.PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS; + }); + afterEach(() => { + if (savedTimeout === undefined) delete process.env.PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS; + else process.env.PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS = savedTimeout; + }); + + test("getExitSummaryTimeoutMs parses env with fallback to default", () => { + delete process.env.PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS; + expect(getExitSummaryTimeoutMs()).toBe(10_000); + process.env.PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS = "250"; + expect(getExitSummaryTimeoutMs()).toBe(250); + process.env.PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS = "not-a-number"; + expect(getExitSummaryTimeoutMs()).toBe(10_000); + process.env.PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS = "-5"; + expect(getExitSummaryTimeoutMs()).toBe(10_000); + }); + + test("session_shutdown stays responsive when summary generation hangs", async () => { + // Pi core awaits session_shutdown handlers with no timeout; a hanging + // provider must not block quitting forever. The API-key lookup never + // resolves here, so without a self-imposed timeout this test only fails + // via bun's per-test timeout. + process.env.PI_MEMORY_EXIT_SUMMARY_TIMEOUT_MS = "50"; + const getApiKey = mock(() => new Promise(() => {})); + const ctx = createShutdownCtx({ + branch: fourMessageBranch(), + model: { provider: "openai", id: "gpt-4o-mini" }, + modelRegistry: { getApiKey }, + }); + + await hooks.session_shutdown({ reason: "quit" }, ctx); + + expect(getApiKey).toHaveBeenCalled(); + expect(fs.existsSync(dailyPath(todayStr()))).toBe(false); + }); + }); + // -- session_before_compact -- test("session_before_compact appends handoff when scratchpad has open items", async () => {