- 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).
Math.random().toString(36).slice(2,6) can produce fewer than 4 chars
for short base-36 representations (e.g. 0.5 → '0.i' → 'i').
Append '0000' before slicing to guarantee the suffix is always 4 chars.
The test drainPostHookActions helper iterated the live array, but
triggerInternalHook snapshots with [...(event.postHookActions ?? [])]
before draining. Align test helper to match production semantics.
1. Math.random() === 0 guard: (0).toString(36).slice(2,6) returns ''
producing a trailing-hyphen slug. Added || '0000' fallback.
2. Misleading ENOENT comment: said 'when blockSessionSave was set before
writeFileWithinRoot' but that branch requires writtenEntry !== null
(blockSessionSave was false). Real scenario: external file deletion
between inline write and post-hook drain. Comment corrected.
3. Live-array drain: for...of on postHookActions is a live iterator —
a self-scheduling action could loop infinitely. Snapshot the array
with [...(event.postHookActions ?? [])] before draining.
Split the single catch block into two distinct error-handling paths:
1. Pre-existing content restore (writeFileWithinRoot): errors are NOT
swallowed — ENOENT here means memoryDir was removed after our inline
write, which is a real filesystem inconsistency that must surface.
2. Unlink (no pre-existing content): ENOENT is expected when the inline
write didn't happen. Non-ENOENT errors (EACCES, EROFS) are re-thrown
for triggerInternalHook to log. Added comment clarifying that the
retraction guarantee is best-effort under adversarial filesystem
conditions (per-action isolation absorbs the error).
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.
The resolveDisplaySessionKey function correctly resolved workspace-based
agent IDs (e.g. agent:main:main → agent:navi:main when workspaceDir
matches the navi agent), but the entry template used event.sessionKey
directly, bypassing the resolution.
When an earlier hook pre-sets sessionSaveContent and a later hook clears
it, the transcript is not available for fallback — it was never loaded.
Hooks wanting to override should set their own content, not clear it.
The InternalHookEvent interface now requires postHookActions (added by
this PR). The boot-md test's makeEvent helper was missing it, causing
a tsc error that fails CI.
Second-resolution (HHMMSS) fallback slugs can collide when automated or
multi-channel setups emit rapid /new or /reset commands within the same
second — both writes target the same filename and the later one silently
overwrites the earlier memory entry.
Append a 4-char random alphanumeric suffix (e.g. 103022-x7f2) to make
collisions effectively impossible without LLM slug generation.
The slug generator embeds up to 2000 chars of raw conversation content
in its prompt. Without disableTools, the embedded agent inherits the
full tool set (exec, file write, messaging), meaning a crafted
conversation could prompt-inject the slug call into executing arbitrary
side-effects before the slug text is extracted.
Slug generation is pure text — it never needs tool access. Add
disableTools: true to close this injection surface.
- 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
Remove early return in triggerInternalHook that skipped the post-hook
drain when allHandlers was empty. This was a latent footgun: any
postHookActions pre-populated on the event before calling
triggerInternalHook would be silently dropped if no handlers were
registered for that event type.
Update test to verify post-hooks now drain in the no-handlers case.
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 blockSessionSave is already true, the handler was still loading
session content and potentially calling generateSlugViaLLM — sending
sensitive transcript text to a model provider despite an explicit
block. Short-circuit before transcript loading when the flag is set.
Credit: Codex review.
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)
Custom content and no-session paths fell through to HHMM (4-char)
timestamp slug. Two /new events in the same minute would overwrite.
HHMMSS makes collisions require same-second timing.
Bundled hooks register before managed/workspace hooks in FIFO order,
so blockSessionSave only works when set by typed plugin hooks (which
fire earlier in the lifecycle) or extraDirs hooks. Document this
limitation honestly rather than claiming incorrect ordering.
- 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.
