fix: add diagnostic for cleared sessionSaveContent, export drainPostHookActions

- Add log.debug when a post-hook clears pre-set sessionSaveContent, making
  the silent no-op visible to plugin authors (symmetric with blockSessionSave
  cleared warning)
- Extract drainPostHookActions as an exported utility from internal-hooks.ts
  so handler tests share the exact production drain semantics instead of
  maintaining a divergent copy
- Update handler.test.ts to import and use the shared drain utility

Addresses greptile review feedback for confidence score improvement.
This commit is contained in:
zeroaltitude 2026-03-09 11:42:20 -07:00
parent 45ec7ec672
commit f0958de191
No known key found for this signature in database
GPG Key ID: 77592FB1C703882E
3 changed files with 62 additions and 36 deletions

View File

@ -573,24 +573,12 @@ describe("session-memory hook", () => {
expect(memoryContent).toContain("assistant: Only message 2");
});
// Helper to drain postHookActions with per-action error isolation,
// matching triggerInternalHook's actual drain behaviour.
async function drainPostHookActions(event: {
postHookActions: Array<() => Promise<void> | void>;
}) {
// Snapshot before draining — matches triggerInternalHook's production
// semantics (prevents self-scheduling actions from executing in the
// same drain cycle).
const pending = [...event.postHookActions];
// Clear source array so re-drain is a no-op (matches production).
event.postHookActions.length = 0;
for (const action of pending) {
try {
await action();
} catch {
// Per-action isolation — one failure doesn't block others.
}
}
// Uses the exported drain utility from internal-hooks.ts so tests share
// the exact same drain semantics as production (snapshot → clear → sequential
// await → per-action error isolation).
async function drainActions(event: { postHookActions: Array<() => Promise<void> | void> }) {
const { drainPostHookActions } = await import("../../internal-hooks.js");
await drainPostHookActions(event.postHookActions);
}
it("blockSessionSave (pre-set) prevents memory file creation", async () => {
@ -610,7 +598,7 @@ describe("session-memory hook", () => {
event.context.blockSessionSave = true;
await handler(event);
await drainPostHookActions(event);
await drainActions(event);
const memoryDir = path.join(tempDir, "memory");
const memoryFiles = await fs.readdir(memoryDir).catch(() => [] as string[]);
@ -643,7 +631,7 @@ describe("session-memory hook", () => {
event.context.blockSessionSave = true;
// Post-hook action retracts the file
await drainPostHookActions(event);
await drainActions(event);
memoryFiles = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md"));
expect(memoryFiles).toHaveLength(0);
@ -678,7 +666,7 @@ describe("session-memory hook", () => {
});
event1.timestamp = fixedTimestamp;
await handler(event1);
await drainPostHookActions(event1);
await drainActions(event1);
const memoryDir = path.join(tempDir, "memory");
const files1 = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md"));
@ -707,7 +695,7 @@ describe("session-memory hook", () => {
// Late-block: retraction should restore the FIRST session's content.
event2.context.blockSessionSave = true;
await drainPostHookActions(event2);
await drainActions(event2);
const restoredContent = await fs.readFile(collidingPath, "utf-8");
expect(restoredContent).toContain("first session");
@ -734,7 +722,7 @@ describe("session-memory hook", () => {
event.context.sessionSaveContent = "Custom summary from upstream hook";
await handler(event);
await drainPostHookActions(event);
await drainActions(event);
const memoryDir = path.join(tempDir, "memory");
const files = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md"));
@ -766,7 +754,7 @@ describe("session-memory hook", () => {
event.context.sessionSaveContent = "Redacted by policy";
// Post-hook action overwrites
await drainPostHookActions(event);
await drainActions(event);
const memoryDir = path.join(tempDir, "memory");
const files = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md"));
@ -792,7 +780,7 @@ describe("session-memory hook", () => {
event.context.sessionSaveContent = "";
await handler(event);
await drainPostHookActions(event);
await drainActions(event);
const memoryDir = path.join(tempDir, "memory");
const files = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md"));
@ -861,7 +849,7 @@ describe("session-memory hook", () => {
event.context.sessionSaveContent = "Replacement content from policy hook";
// Post-hook should create the directory and write the file
await drainPostHookActions(event);
await drainActions(event);
const files = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md"));
expect(files.length).toBeGreaterThan(0);
@ -887,7 +875,7 @@ describe("session-memory hook", () => {
event.context.sessionSaveContent = "Should not appear";
await handler(event);
await drainPostHookActions(event);
await drainActions(event);
const memoryDir = path.join(tempDir, "memory");
const memoryFiles = await fs.readdir(memoryDir).catch(() => [] as string[]);
@ -916,7 +904,7 @@ describe("session-memory hook", () => {
event.context.blockSessionSave = true;
event.context.sessionSaveContent = "Should not appear";
await drainPostHookActions(event);
await drainActions(event);
const memoryDir = path.join(tempDir, "memory");
const memoryFiles = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md"));
@ -947,7 +935,7 @@ describe("session-memory hook", () => {
// Since the transcript was never loaded, no file can be produced.
event.context.blockSessionSave = false;
await drainPostHookActions(event);
await drainActions(event);
// No memory file should exist — the transcript was never loaded
const memoryDir = path.join(tempDir, "memory");

View File

@ -495,6 +495,22 @@ const saveSessionToMemory: HookHandler = async (event) => {
"execution. To write a file after clearing blockSessionSave, also " +
"provide sessionSaveContent with the desired content.",
);
} else if (
event.context.blockSessionSave !== true &&
writtenEntry !== null &&
typeof postContent !== "string" &&
hasCustomContent
) {
// sessionSaveContent was pre-set (inline write used custom content),
// then a later hook cleared it. The file retains the pre-set content.
// This is a no-op — to revert to transcript content, the clearing hook
// must provide its own sessionSaveContent. Log for diagnostics so
// plugin authors know their clearing was silently ignored.
log.debug(
"sessionSaveContent was cleared by a post-hook but the inline write " +
"already used the pre-set content. File retains pre-set content. " +
"To override, set sessionSaveContent to the desired replacement.",
);
}
});
} catch (err) {

View File

@ -297,16 +297,38 @@ export async function triggerInternalHook(event: InternalHookEvent): Promise<voi
// callbacks do not execute in this drain cycle. Without this, a self-
// scheduling action (one that pushes another action) could loop infinitely
// because Array's for...of iterator is live and re-reads length each step.
const pendingActions = [...event.postHookActions];
// Clear the source array so re-draining the same event is a no-op.
// Without this, passing an event twice would re-execute every action.
event.postHookActions.length = 0;
for (const action of pendingActions) {
await drainPostHookActions(event.postHookActions, (err) => {
const message = err instanceof Error ? err.message : String(err);
log.error(`Post-hook action error [${event.type}:${event.action}]: ${message}`);
});
}
/**
* Drain an array of post-hook action callbacks.
*
* Snapshots the array before iterating so that self-scheduling actions
* (ones that push new callbacks) do not execute in the same drain cycle.
* Clears the source array after snapshotting so re-draining is a no-op.
* Errors are caught per-action so one failure doesn't block others.
*
* Exported for use in tests that need to drain post-hook actions without
* going through the full triggerInternalHook pipeline, ensuring they
* share the same drain semantics as production.
*
* @param actions - The postHookActions array to drain (mutated: cleared after snapshot).
* @param onError - Optional per-action error handler. Defaults to swallowing errors silently.
*/
export async function drainPostHookActions(
actions: Array<() => Promise<void> | void>,
onError?: (err: unknown) => void,
): Promise<void> {
const pending = [...actions];
actions.length = 0;
for (const action of pending) {
try {
await action();
} catch (err) {
const message = err instanceof Error ? err.message : String(err);
log.error(`Post-hook action error [${event.type}:${event.action}]: ${message}`);
onError?.(err);
}
}
}