- Import crypto from 'node:crypto' for consistency with codebase
conventions (every other file uses explicit import, not global)
- Tighten late-block privacy warning to only fire when sessionContent
was actually loaded (non-null) — prevents misleading warning when
no transcript was ever read from disk or sent to LLM
- Add matching crypto import in test file so vi.spyOn mocks the
correct module reference
- 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.
- 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.
- Replace direct Math.random mutation with vi.spyOn(Math, 'random').mockReturnValue(0.5)
for idiomatic Vitest cleanup integration
- Fix comment: collision is driven by identical LLM slug, not timestamp fallback;
Math.random pin is a backstop for null sessionContent edge case
- Remove unnecessary nullish-coalescing and conditional guard on postHookActions
(field is required in interface and always initialized by createInternalHookEvent)
Addresses greptile review feedback for confidence score improvement.
1. Slug-collision test: Pin event.timestamp to a fixed Date in addition
to Math.random, preventing flaky behavior when wall-clock crosses a
second boundary between event1 and event2.
2. handler.ts: Skip entry construction when blockPreSet is true (the
value is discarded — writtenEntry will be null regardless). Avoids
misleading dead code.
3. handler.ts: Reuse blockPreSet in the inline-write guard instead of
re-evaluating context.blockSessionSave === true. Single source of
truth for the pre-set block condition.
1. Clear postHookActions array after draining (both production
triggerInternalHook and test helper). Re-triggering the same event
is now a safe no-op instead of re-executing every action.
2. Expand warning log in block-then-clear scenario to explain that
transcript was skipped because sessionSaveContent OR blockSessionSave
was pre-set, and that plugin authors must provide sessionSaveContent
when clearing blockSessionSave.
3. Add test: 'clears postHookActions after drain — re-trigger is a no-op'
verifying the idempotency contract.
The previous test couldn't trigger a slug collision because the random
suffix made same-filename generation virtually impossible. Pin Math.random
to a fixed value so both handler calls produce the same fallback slug,
exercising the preExistingContent !== null restoration branch.
Verifies: second handler overwrites first file → late blockSessionSave
retracts second write → first session's content is restored (not deleted).
The test drainPostHookActions helper iterated the live array, but
triggerInternalHook snapshots with [...(event.postHookActions ?? [])]
before draining. Align test helper to match production semantics.
When blockSessionSave is pre-set, the handler skips transcript loading
and LLM slug generation (avoiding unnecessary I/O and model calls).
If a later hook clears blockSessionSave without setting sessionSaveContent,
no file can be produced because the transcript was never loaded.
Previously this was a silent no-op. Now emits log.warn so plugin authors
know to supply sessionSaveContent when un-blocking a pre-set block.
Adds test: 'blockSessionSave pre-set then cleared without sessionSaveContent
warns and writes nothing' — verifies the edge case produces no file.
When a later hook sets blockSessionSave=true, the retraction previously
unconditionally deleted memoryFilePath. If the filename collided with a
pre-existing memory file (e.g. same LLM slug on the same day), the prior
session's content was erased.
Now snapshots any pre-existing file content before the inline write and
restores it on retraction instead of deleting. Non-colliding writes still
get deleted as before.
- Remove redundant length guard on postHookActions drain loop
- Add per-action error isolation to drainPostHookActions test helper,
matching actual triggerInternalHook behaviour
- Remove double-catch in session-memory post-hook callback; let errors
propagate to the framework's per-action catch for consistent logging
When blockSessionSave is true initially, the inline write is skipped —
including the fs.mkdir that creates memoryDir. If a later hook clears
blockSessionSave and sets sessionSaveContent, the post-hook
writeFileWithinRoot call would fail with ENOENT because the directory
was never created. The error was silently swallowed, causing the content
override to be lost.
Add fs.mkdir(memoryDir, { recursive: true }) before the post-hook write.
Add regression test for the block-then-clear-with-content scenario.
When both flags are set, blockSessionSave must win — a blocked save
should never create a file, even if sessionSaveContent is also present.
Bug: if blockSessionSave was pre-set (writtenEntry=null), the post-hook
sessionSaveContent check would pass and create a new file, violating
the block intent.
Fix: guard the sessionSaveContent overwrite with blockSessionSave !== true.
Tests: 2 new tests covering both-flags-pre-set and both-flags-late-set.
Credit: Greptile review.
Adds a generic postHookActions mechanism to InternalHookEvent:
handlers push deferred callbacks that triggerInternalHook drains
after all handlers complete. This eliminates FIFO registration-order
dependencies — any hook can set context flags regardless of when it
registered.
Session-memory handler updated to use fail-safe pattern:
- Writes file inline (preserves data if postHookActions never runs)
- Pushes post-hook action for retraction (blockSessionSave) and
content replacement (sessionSaveContent)
- Pre-set flags still work inline; late-set flags work via post-hook
New tests:
- 5 tests for postHookActions mechanism (ordering, error isolation,
late-context visibility)
- 2 tests for late-set blockSessionSave/sessionSaveContent
- 1 fail-safe test (file preserved when postHookActions not drained)
- Add log.debug when sessionSaveContent override is used (symmetric with blockSessionSave)
- Change override test assertion from toContain to toBe for precision
Two new context fields for upstream hooks (e.g. security plugins) to
control session memory persistence:
- blockSessionSave (boolean): prevent session from being saved to memory
- sessionSaveContent (string): override saved content with custom text
(empty string is valid — persists a blank marker without transcript)
When sessionSaveContent is set, LLM slug generation and session content
loading are skipped (unnecessary when content is overridden).
Split from #35567 — sessionSaveRedirectPath follows separately as it
requires path canonicalization, symlink resolution, and filesystem
write policy review.
When /new rotates <session>.jsonl to <session>.jsonl.reset.*, the session-memory hook may read an empty active transcript and write header-only memory entries.
Add fallback logic to read the latest .jsonl.reset.* sibling when the primary file has no usable content.
Also add a unit test covering the rotated transcript path.
Fixes#18088
Refs #17563
Adds `messages` config option to session-memory hook (default: 15).
Fixes filter order bug - now filters user/assistant messages first,
then slices to get exactly N messages. Previously sliced first which
could result in fewer messages when non-message entries were present.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
