Greptile Summary

This PR introduces the postHookActions deferred-execution mechanism on InternalHookEvent, adds blockSessionSave/sessionSaveContent context fields to the session-memory handler, hardens slug generation against prompt injection by setting disableTools: true, and upgrades all memory filenames to include an 8-char random hex suffix to eliminate silent-overwrite collisions. The design is well-considered and the new integration tests cover the key invariants (pre-set/late-set block, content replacement, precedence, fail-safe, mkdir edge case, CWE-834 guard).

Confidence Score: 5/5

Safe to merge; only two minor P2 findings.

No P0 or P1 issues found. The core postHookActions mechanism, fail-safe write pattern, TOCTOU ownership checks, and CWE-834 snapshot guard are all correctly implemented and well-tested. The two findings are P2 only: a stale '4-char' comment in the test and a PR-description claim about HHMMSS that the code doesn't actually implement (collision avoidance is fully addressed by the 8-char suffix regardless).

No files require special attention.

This is a comment left during a code review.
Path: src/hooks/bundled/session-memory/handler.test.ts
Line: 73-77

Comment:
**Stale comment says "4-char" but suffix is 8 chars**

The comment on line 73 reads "4-char hex random suffix" while the assertion on line 77 tests for exactly 8 hex characters (`[0-9a-f]{8}`), which matches the implementation (`slice(0, 8)`). The comment was not updated when the suffix was widened from 4 to 8 chars.

```suggestion
      // Filename includes an 8-char hex random suffix on all paths
      // (LLM and fallback) to guarantee uniqueness; the timestamp portion
      // is what we assert here.
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/hooks/bundled/session-memory/handler.ts
Line: 83-86

Comment:
**PR description claims HHMMSS fallback slug but code still produces HHMM**

The PR description states: *"Collision-resistant fallback slug — `HHMMSS` timestamp (previously `HHMM`) for the no-LLM path."* However, `timeSlug` is still built as `${hour}${minute}` (4 digits, HHMM), and the test regex `2330` confirms this. The `second` variable is computed (line 75) but not included in the slug. Since the 8-char unique suffix provides all necessary collision avoidance, this doesn't affect correctness — but the PR description's claim is inaccurate and `second` goes unused.

```suggestion
    timeSlug: `${hour}${minute}${second}`,
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (39): Last reviewed commit: "fix(session-memory): address gpt-5.5 dee..." | Re-trigger Greptile}

@greptile-apps: Please re-score after minor fixes!

@greptile-apps: resolved minor issue, please re-score

@greptile-apps: Please re-score, fixes added

@greptile-apps Please rescore.

@greptile-apps Please rescore.

@greptile-apps: We have re-thought this feature and believe that we need a slightly more powerful hooks feature: postHookActions. A generic deferred-execution mechanism on InternalHookEvent. Any handler can push async callbacks onto event.postHookActions. After all handlers in the FIFO loop have completed, triggerInternalHook drains the queue sequentially.

This is not session-memory-specific. Any hook that needs "act after everyone has spoken" semantics can use it.

Design constraint: fail-safe: The critical invariant: if anything goes wrong in the hooks system, session memory must still be written. Data preservation beats policy enforcement.

This means the session-memory handler cannot defer its write to postHookActions — if the drain never runs (bug, exception, crash), the file would never exist. Instead:

1. Write inline (as today) — the file hits disk during the handler's normal execution
2. Push a post-hook action for retraction (blockSessionSave) or replacement (sessionSaveContent)
3. If postHookActions never drains → file stays on disk with best available content
4. If a later hook sets blockSessionSave → post-hook action deletes the file
5. If a later hook sets sessionSaveContent → post-hook action overwrites with new content

Pre-set flags (set before the handler runs by an upstream hook) still work inline — the handler checks blockSessionSave before writing and uses sessionSaveContent directly. The post-hook action is a second pass that catches late-set flags.

Please consider this design change and re-score!

Excellent design! The postHookActions mechanism is elegant and the fail-safe approach is exactly right. However, I found a bug in the edge case handling.

The Issue:

When both blockSessionSave and sessionSaveContent are set (especially if blockSessionSave is pre-set), the post-hook action creates a file that shouldn't exist:

// In the post-hook action:
if (event.context.blockSessionSave === true && writtenEntry !== null) {
  // delete and return
}

// This runs even if blockSessionSave is true (but writtenEntry was null)
if (typeof postContent === "string" && postContent !== writtenEntry) {
  await writeFileWithinRoot(...); // ❌ Creates file despite blockSessionSave!
}

Trace:

• blockSessionSave pre-set → inline write skipped → writtenEntry = null
• Post-hook: delete check fails (writtenEntry is null)
• Post-hook: overwrite check succeeds → creates new file (violates blockSessionSave intent)

Fix:

The sessionSaveContent logic should respect blockSessionSave:

const postContent = event.context.sessionSaveContent;
if (
  event.context.blockSessionSave !== true &&  // ✅ Add this check
  typeof postContent === "string" &&
  postContent !== writtenEntry
) {
  await writeFileWithinRoot({
    rootDir: memoryDir,
    relativePath: filename,
    data: postContent,
    encoding: "utf-8",
  });
  log.debug("Session save content replaced by post-hook (sessionSaveContent)", {
    length: postContent.length,
  });
}

Missing Test Coverage:

None of the tests set both flags. Add:

it("blockSessionSave takes precedence over sessionSaveContent", async () => {
  // Test both pre-set and one late-set scenarios
});

Otherwise: Implementation is solid, tests are comprehensive, and the fail-safe guarantees are preserved. This is a strong architecture improvement.

@darfaz Tagging you in the split. We've improved the architecture considerably while positing a powerful new hooks mechanism: postHookActions.

@greptile-apps: fixed blockSessionSave semantics and added test; please re-score!

@greptile-apps Please rescore.

@clawsweeper — addressed the open maintainer-level questions you flagged on this PR. Recent commits:

bd14845 — public-surface docs, log clarity, privacy posture

1. Public hook-runtime surface (InternalHookEvent.postHookActions). Previous JSDoc was a single line, inadequate for the API-compatibility / plugin-migration concern you raised. Replaced with comprehensive docs covering: when to use the field, snapshot+clear drain semantics with explicit CWE-834 callout, per-action error isolation, optional/additive nature (existing handlers unchanged), and the explicit contract that actions pushed during drain are queued for the next cycle, NOT extended into the current one. Plugin authors and maintainers now have the full contract in one place.

2. Premature save logging. The pre-existing Memory file written successfully / Session context saved to log lines emitted before the post-hook drain decided whether to retract or replace the file — misleading audit trail. Renamed to make staging vs final disposition explicit: Memory file written inline (pre-post-hook, may be retracted/replaced) and Session context staged at <path> (final disposition decided by post-hook drain). Operators grepping for save completion should filter on post-drain lines (Session save retracted / replaced).

3. Late-blockSessionSave privacy semantics. Tightened the warning to match the loud-and-specific posture you asked for:

   • Uses WARN level with a PRIVACY: prefix for grep-ability.
   • Includes the transcript byte count so operators can size the egress.
   • Explicitly states that retraction does NOT un-send the data from the slug-generation provider's logs / training pipeline.
   • Re-emphasizes the mitigation (set blockSessionSave BEFORE the handler runs).

e812c0f — filename suffix entropy (already pushed earlier today)

4. 4-hex character suffix entropy. Bumped from 4 hex chars (16 bits, ~50% birthday-paradox collision at 256 same-day same-slug saves) to 8 hex chars (32 bits, < 1 in 1e8 collision probability for realistic loads). Test regex updated to match.

6f63e10 — CWE-834 self-append guard tests (also already pushed)

Explicit regression tests for the snapshot+clear drain pattern, including a self-appending action test (with safety cap to prevent test runner hang) and a multi-handler stacking test.

On your split-the-PR suggestion — I'd defer to maintainer preference. The three logical pieces (slug-generation hardening with disableTools, postHookActions mechanism, session-memory block/content controls with stronger filename uniqueness) are coherent enough to land together as one focused PR, and the test surface across them is interleaved (the post-hook callback's behavior depends on the inline write semantics, which depend on the slug-generation outcome). If you want them split, happy to do that — just say the word.

On testing readiness — local vitest run src/hooks/bundled/session-memory/ src/hooks/internal-hooks.test.ts shows 70/70 passing. Full suite at the relevant scopes is green.

Ready for fresh review.

@greptile-apps: please re-review, and thank you

Closing without merge — superseded by the upstream session-memory rework on main, which addresses the same concerns through a different (opt-in) design:

• This PR's always-on slug + UUID-suffix uniqueness vs. main's opt-in hooks.internal.entries.session-memory.llmSlug: true + resolveAvailableMemoryFilename helper for collision avoidance.
• The postHookActions / blockSessionSave / sessionSaveContent surface is not preserved here. If those control points are still desired, they can be re-proposed against the new handler shape in a fresh PR.

No downstream consumers in our ecosystem depend on this PR's specific API surface, so closing without follow-up. The branch is preserved as the annotated tag archive/feat/hook-session-memory-block-content on https://github.com/zeroaltitude/openclaw if recovery is ever needed.