From 65e0158bf86af2f32fa2ca8f5008fb192a5e2c32 Mon Sep 17 00:00:00 2001
From: zeroaltitude <zeroaltitude@gmail.com>
Date: Mon, 9 Mar 2026 12:15:25 -0700
Subject: [PATCH] fix: rethrow in test drain helper, fix slug comment, clarify
 drain JSDoc

- drainActions test helper now rethrows errors so test failures surface
  the actual error instead of a confusing downstream assertion
- Fix misleading comment: slug collision is via fallback timestamp+random
  path (LLM slug generation disabled in VITEST=true), not LLM mock
- Clarify drainPostHookActions JSDoc: re-drain is only a no-op when no
  actions push new callbacks during execution; mid-drain pushes persist

Addresses greptile review feedback for confidence score improvement.
---
 .../bundled/session-memory/handler.test.ts    | 22 ++++++++++---------
 src/hooks/internal-hooks.ts                   |  6 ++++-
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/src/hooks/bundled/session-memory/handler.test.ts b/src/hooks/bundled/session-memory/handler.test.ts
index 79e47ffba3b..61aa3e2205d 100644
--- a/src/hooks/bundled/session-memory/handler.test.ts
+++ b/src/hooks/bundled/session-memory/handler.test.ts
@@ -575,10 +575,13 @@ describe("session-memory hook", () => {
 
   // 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).
+  // await). Errors are rethrown (rather than swallowed) so test failures surface
+  // the actual error message instead of a confusing downstream assertion failure.
   async function drainActions(event: { postHookActions: Array<() => Promise<void> | void> }) {
     const { drainPostHookActions } = await import("../../internal-hooks.js");
-    await drainPostHookActions(event.postHookActions);
+    await drainPostHookActions(event.postHookActions, (err) => {
+      throw err;
+    });
   }
 
   it("blockSessionSave (pre-set) prevents memory file creation", async () => {
@@ -647,14 +650,13 @@ describe("session-memory hook", () => {
       content: createMockSessionContent([{ role: "user", content: "first session" }]),
     });
 
-    // Pin Math.random AND timestamp to force deterministic slug — both
-    // handler calls produce the same LLM-generated slug ("simple-math"
-    // via the mock), exercising the slug-collision restoration path
-    // (preExistingContent !== null).  Math.random is pinned as a backstop
-    // for the edge case where sessionContent is null and the LLM path
-    // never fires.  Without pinning the clock, a wall-clock second
-    // boundary between event1 and event2 would produce different HHMMSS
-    // prefixes → no collision.
+    // Pin Math.random AND timestamp to force deterministic fallback slug —
+    // both handler calls produce the same HHMMSS prefix (fixed timestamp)
+    // and the same random suffix (pinned Math.random). LLM slug generation
+    // is disabled in the test environment (VITEST=true), so the collision
+    // is exercised entirely through the fallback path.  Without pinning
+    // the clock, a wall-clock second boundary between event1 and event2
+    // would produce different HHMMSS prefixes → no collision.
     vi.spyOn(Math, "random").mockReturnValue(0.5);
     const fixedTimestamp = new Date("2024-01-15T12:34:56.000Z");
 
diff --git a/src/hooks/internal-hooks.ts b/src/hooks/internal-hooks.ts
index 5ae1e127695..6d2610c670b 100644
--- a/src/hooks/internal-hooks.ts
+++ b/src/hooks/internal-hooks.ts
@@ -308,7 +308,11 @@ export async function triggerInternalHook(event: InternalHookEvent): Promise<voi
  *
  * 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.
+ * Clears the source array after snapshotting so an immediate re-drain with
+ * no intervening pushes is a no-op.  Note: if an action pushes new callbacks
+ * *during* its own execution, those entries land in the already-cleared source
+ * array and will persist after this function returns — a subsequent drain
+ * call will execute them.
  * 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