Issue #212: feat: Implement LangChain message trimming - #1148
Issue #212: feat: Implement LangChain message trimming#1148dereck-symmetry wants to merge 3 commits into
Conversation
bjagg
left a comment
bjagg
left a comment
There was a problem hiding this comment.
Overview
Implements token-budget trimming in the Advisor's pre_model_hook, replacing the # TODO Add safe trim_messages here placeholder with a real _safe_trim_messages helper, plus a new test_memory.py (6 tests) and an ADR update.
The trimming implementation itself is good, and one comment in particular is the kind of thing that only comes from actually thinking about the failure mode:
# Note: start_on is deliberately NOT set - it would force the
# trimmed history to end on a HumanMessage, dropping trailing AI/tool messages
# even when within budget and breaking multi-step tool use.That is a subtle trap correctly avoided, and test_safe_trim_keeps_tool_call_tail_within_budget locks it in. include_system=True protecting the summary at index 0, strategy="last", and the empty-trim fallback with a warning are all the right calls.
Must fix: this PR deletes the #772 KT artifact
diff --git a/.claude/plans/772-import-transformation-groups.md
deleted file mode 100644
That file is Chris Beach's knowledge-transfer document for the remaining transformation-import work (Layers 2-4, now tracked as #1140/#1141/#1142). It was deliberately corrected and merged ten days ago in #1143 specifically so the remaining layers would not live only in one person's head — Chris's hours on the project have run out.
The deletion isn't mentioned in the PR description, and it's paired with a .gitignore addition of .claude/plans/ labelled "Local AI-agent working files (not source-controlled)". I understand the instinct — agent scratch space generally shouldn't be committed — but this particular file was promoted to a tracked artifact on purpose.
Please drop both the deletion and the .gitignore hunk from this PR. If the convention is worth having, it deserves its own PR and a decision about where KT documents should live instead — the same .gitignore hunk is also in #1149, so it's currently being proposed in three places at once.
Design question: the token budget only applies post-summarization
state["messages"] = _safe_trim_messages(...) sits inside if summarized_messages:, which is only reachable when len(state["messages"]) > max_messages triggers summarization. So a conversation at or below MESSAGES_TO_KEEP (default 4) messages is never trimmed regardless of its token count — four long tool results, or one very large message, pass through untouched.
test_short_conversation_is_not_summarized encodes this (assert result["messages"] == conversation), so it reads as intentional. But it's worth stating explicitly, because #718 — "Failed to trim messages to fit within max_tokens limit before summarization" — is precisely that uncovered path. The PR body says #718's fix is deliberately out of scope, which is a reasonable scoping call. Two suggestions:
- Say so in the code, next to the
if summarized_messages:branch — the fact that the token budget is not enforced on the short-conversation path is non-obvious and will otherwise be re-derived by whoever picks up #718. - Confirm
#212should close on this PR. If #212 is "implement trimming" then yes; if it's understood as "the message list sent to the LLM always fits the budget," then it's partially addressed and #718 carries the rest.
Configuration note
TRIMMED_MESSAGES_SIZE defaults to 384 (core.py:46), which is the same value as the summarizer's max_conversation_size default. Since the trim runs on summary + retained messages after summarization, having the two budgets coincide means the trim will rarely bind. Worth confirming the number was chosen rather than inherited — and a brief note on how the two budgets relate would help.
Test coverage
Six tests, and they cover the right things: trims when over budget, no-ops when under, preserves the system message, preserves the latest human message, keeps the tool-call tail, and falls back on empty. Asserting on caplog for the fallback warning is a nice touch.
Verdict
Request changes — solely for the KT artifact deletion, which is a merge blocker. The trimming work itself is ready and I'd approve it on its own.
Recommend removing the .claude/plans/ deletion and .gitignore hunk, then adding a comment on the post-summarization scoping. Happy to re-review promptly.
…g and budget ceiling
|
Thanks for the thorough review — all three points addressed in d909e74: KT artifact deletion (blocker) — Agreed, and apologies. Trim scoping (#718) — Documented rather than left to be re-derived: a comment above
Also: PR body changed Re-requesting your review. |
…1163) ##### Description of Change **Problem.** The demo Advisor has been failing its synthetic monitor since 2026-08-22, and the alerts are still firing. Every run dies the same way: the typing indicator never clears because the Advisor is getting a 429 from OpenAI. ``` openai.RateLimitError: 429 - Request too large for gpt-4.1-mini on tokens per min (TPM): Limit 2000000, Requested 3200202 -> 3406017 -> 3503940 ``` **Cause.** `pre_model_hook` mutated the state dict in place and returned the whole `ChatState`. LangGraph treats a `pre_model_hook` return value as a *state update*, and `AgentState.messages` is declared `Annotated[Sequence[BaseMessage], add_messages]` — an **append** reducer that only replaces entries whose message IDs already exist. Our hook returned a plain list with no `RemoveMessage(REMOVE_ALL_MESSAGES)` sentinel, so the retained messages were ID-matched no-ops while **each new summary carried a fresh ID and was appended**. Nothing was ever removed. Every turn the model received the full prior history *plus* one more summary block, so the hook that exists to bound the context was growing it instead. The framework says so directly, in `langgraph/prebuilt/chat_agent_executor.py` (langgraph-prebuilt 0.2.3, the version we ship): > At least one of `messages` or `llm_input_messages` MUST be provided. > `"messages"` — *If provided, will UPDATE the `messages` in the state*. > `"llm_input_messages"` — *If provided, will be used as the input to the LLM, and will NOT UPDATE `messages`*. > **Warning:** if returning `messages`, you should OVERWRITE the key: `{"messages": [RemoveMessage(id=REMOVE_ALL_MESSAGES), *new_messages]}` There was a standing tell in the logs, once per agent invocation, easy to read as noise — it is the signature of returning a whole state where a scoped update is required: ``` WARNING | langgraph | Task pre_model_hook ... wrote to unknown channel remaining_steps, ignoring it. ``` **Solution.** Return a scoped update that hands the trimmed list to the model via `llm_input_messages`, which feeds the LLM without mutating `messages`, and return only the keys that must persist. `context` must, so the summarizer can tell it already summarized. `remaining_steps` is a managed value and must never be written back. Two latent problems in the same function are fixed alongside it: - `messages_to_retain` was bound inside the `if len(...) > max_messages:` branch but read outside it — safe only because it was read solely when a summary existed, and an `UnboundLocalError` waiting on any future edit. - The no-summarization path returned no LLM input at all, which `create_react_agent` rejects. The pre-existing `# ty: ignore[invalid-assignment]` is gone too, since we no longer assign into the TypedDict. **Side effects / limitations.** Behavior-only change to how the hook reports its result; no API, schema, or config change. Because trimming now actually takes effect, the model sees a genuinely shorter history than it did before — that is the intent, but it means summarization quality (`LIF_ADVISOR_MAX_CONVERSATION_SIZE`, default 384) is now load-bearing in a way it previously was not, since the untrimmed history was silently backstopping it. **How to test.** `uv run pytest test/components/lif/langchain_agent/` — and to see the regression, stash `memory.py` and re-run: 5 of the 6 new tests fail against the old code. ##### Related Issues Closes #1162 Refs #212, Refs #718 ##### Type of Change - [x] Bug fix (non-breaking change which fixes an issue) ##### Project Area(s) Affected - [x] components/ - [x] test/ or e2e/ --- ##### Checklist - [x] commit message follows commit guidelines (see commitlint.config.mjs) - [x] tests are included (unit and/or integration tests) - [x] code passes linting checks (`uv run ruff check`) - [x] code passes formatting checks (`uv run ruff format`) - [x] code passes type checking (`uv run ty check`) - [x] pre-commit hooks have been run successfully ##### Testing - [x] Automated tests added/updated The regression test merges the hook's update through the **real** `add_messages` reducer and reads the LLM input the way `call_model` does (`llm_input_messages` if present, else `messages`), so it reproduces the production failure rather than asserting on an idealized contract: | | LLM input after 30 turns | |---|---| | before | **88 messages**, growing linearly | | after | **5**, flat | Full suite: 684 passed. ##### Additional Notes This supersedes #212/#718 as the immediate cause of the demo outage. Worth flagging for the review queue: **#1148's trimming would not have prevented this**, because it runs downstream of the same broken return path. Once merged this needs promotion to demo to actually clear the alerts — the demo advisor runs a pinned image tag.
2 participants
This issue implements a new feature where LangChain message trimming is implemented.
The changes can be tested by Claude in an automated fashion by deployment. It can also be manually tested.
#718 is related to this issue. However the fix for that is NOT implemented here.
Refs #212
Refs #718
Type of Change
to not work as expected)
Project Area(s) Affected
Checklist
uv run ruff check)uv run ruff format)uv run ty check)docs/and project README updated
Testing
Additional Notes
Testing found a new possible bug with 1-org deployments. Author will create a new issue and discuss with the dev team.
Note that testing for this PR uses LLM for automated testing.