Skip to content

fix(agent): run post_llm_call hook before session persistence - #14913

Open
aniruddhaadak80 wants to merge 3 commits into
NousResearch:mainfrom
aniruddhaadak80:fix/post-llm-call-persist-order
Open

aniruddhaadak80 wants to merge 3 commits into
NousResearch:mainfrom
aniruddhaadak80:fix/post-llm-call-persist-order

Conversation

@aniruddhaadak80

@aniruddhaadak80 aniruddhaadak80 commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

The transform_llm_output plugin hook ran after the assistant turn had already been persisted. Consequences: (a) plugins that mutate the final response could not influence what got saved, so session replay/search showed pre-transform text that never matched what the user saw; (b) a hook raising mid-flight left the persisted row and the in-memory history inconsistent.

This PR moves the hook to run inside the persist try-block, immediately after the closing assistant row is filled but before the row is stamped/persisted � so a transformed response is what reaches the session DB and the returned final_response. When a transform occurs, the pending assistant message is rewritten with the transformed content and the DB flush cursor invalidated (_db_flush_scan_prefix = None) so the updated row is re-persisted; timestamps stay consistent via stamp_message_timestamp. post_llm_call still fires after the turn-completion explainer, unchanged.

Recreated fresh off current main (the original branch was based on an old tree where the hook lived in plugins; it now lives in hermes_cli.lifecycle.invoke_hook).

Related Issue

N/A � reviewer-requested rebuild of #14913 itself.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • agent/turn_finalizer.py: hoisted _response_transformed / _pre_transform_response init next to _cleanup_errors; moved the transform_llm_output invocation from the post-persist block into the persist try-block after the tail-closing fill; on transform, rewrite the last assistant message (restamp + _db_persisted pop + flush-prefix invalidation); removed the late post-persist block.
  • tests/agent/test_transform_before_persist.py: new � 5 behavior tests using a stub agent (transform result replaces persisted + returned text; untransformed turns untouched; hook errors don't break persistence; ordering vs persist asserted).
  • Existing tests/test_transform_llm_output_hook.py passes unchanged.

How to Test

  1. pytest tests/agent/test_transform_before_persist.py tests/test_transform_llm_output_hook.py -q
  2. Manual: register a transform_llm_output lifecycle hook that appends a marker string, run one CLI turn with a fresh session, then /search the marker � before this change the session DB only contained pre-transform text; now the transformed text is searchable.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature
  • I've run pytest tests/ -q and all tests pass (9 targeted tests green; unrelated Windows-local failures reproduce on clean main)
  • I've added tests for my changes
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • Documentation � or N/A
  • cli-config.yaml.example � or N/A
  • CONTRIBUTING.md/AGENTS.md � or N/A
  • Cross-platform impact considered (no platform-specific code touched)
  • Tool descriptions/schemas � or N/A

Screenshots / Logs

N/A.

Copilot AI review requested due to automatic review settings April 24, 2026 04:49
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/plugins Plugin system and bundled plugins labels Apr 24, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused ordering investigation.

Problems

  • post_llm_call is presently an observer: agent/turn_finalizer.py:368-378 discards its return, and the public contract says observer returns are ignored (website/docs/user-guide/features/hooks.md:374,384,615). Treating { "override_response": ... } as authoritative would introduce new hook semantics rather than repair an existing override path. The maintainer resolution for [Bug]: post_llm_call response overrides are applied after persistence, causing final_response/history mismatch #14894 reached the same conclusion.
  • The actual supported output-replacement hook is transform_llm_output (website/docs/user-guide/features/hooks.md:1240-1265). On current main it runs after _persist_session (agent/turn_finalizer.py:207,339-357), so this PR would not make transformed user-visible text durable.

Suggested changes

  • Preserve post_llm_call as an observer unless maintainers explicitly approve a new return-value contract.
  • If targeting durable transformed output, re-scope the change to transform before persistence and synchronize the current turn's eligible assistant text before _persist_session.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 12, 2026
@aniruddhaadak80
aniruddhaadak80 force-pushed the fix/post-llm-call-persist-order branch from 30b4cf6 to 605f368 Compare July 15, 2026 03:41
@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026
The hook fired after _persist_session, so a plugin's rewritten response
was delivered to the user but never became durable: /resume replayed the
original text and post_llm_call observers synced the untransformed
transcript.

Move the hook into the finalize path right after the closing-assistant-row
block and before the persist override/micro-compaction/persist sequence.
When a transformation applies, rewrite the turn's closing assistant row in
place using the same durability steps as the pure-tool-tail fill (restamp,
drop _db_persisted so the next flush re-writes content, invalidate the
bounded flush-scan cursor). post_llm_call now observes the transformed
response, matching what the user saw.

Adds regression tests pinning: hook-before-persist ordering, transformed
text in the persisted transcript, no-op results leaving everything intact,
post_llm_call receiving the transformed text, and hook failures staying
contained.

Fixes feedback on NousResearch#14913 (rebased onto current main).
@aniruddhaadak80

Copy link
Copy Markdown
Contributor Author

@teknium1 Both points from your review are addressed in bb82198 (pushed to this branch):

  1. post_llm_call stays an observer - the hook's return value remains discarded; nothing in the new code treats override_response or any return as authoritative. No new hook semantics introduced.
  2. Re-scoped onto transform_llm_output, before persistence - the hook now fires ahead of _persist_session in finalize_turn, and when a plugin rewrites the text, the closing assistant row is rewritten with the same durability steps used by the tail-close fill above it (restamp timestamp, drop the _db_persisted flush marker so the next persist actually writes the content, invalidate the bounded flush-scan cursor). Transformed text is therefore durable across /resume and visible to post_llm_call observers.

Regression coverage in tests/agent/test_transform_before_persist.py pins the ordering contract: hook-before-persist ordering, transformed text present in the persisted snapshot with the flush marker dropped, no-op returns leaving transcript untouched, post_llm_call receiving the transformed response, and hook exceptions contained without breaking the turn.

Happy to adjust if the durability rewrite should use a different mechanism than the flush-marker path.

@aniruddhaadak80

Copy link
Copy Markdown
Contributor Author

Thanks for the precise review - both readings are correct, and they leave this PR at a genuine fork that needs a maintainer call rather than a mechanical fix:

  1. As written, the PR treats { "override_response": ... }\ from \post_llm_call\ as authoritative, which contradicts the documented observer contract (\hooks.md: returns ignored) and the [Bug]: post_llm_call response overrides are applied after persistence, causing final_response/history mismatch #14894 resolution. I will not push new hook semantics without an explicit OK.

  2. The underlying durability gap stands regardless: \ ransform_llm_output\ currently runs after _persist_session, so even sanctioned output transforms never reach storage for the live turn.

Two paths forward:

  • A (no semantic change): close this PR and open a narrow one that moves the eligible-assistant-text transform ahead of persistence, keeping \post_llm_call\ strictly observational.
  • B (approved semantics change): keep this PR and document the override-response contract as intentional, with gateway/CLI surfaces updated accordingly.

Happy to execute either immediately - @teknium1 / @austinpickett, which way do you want it?

…ersist-order

# Conflicts:
#	agent/turn_finalizer.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/plugins Plugin system and bundled plugins P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants