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 () => {