Repository navigation
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| langchain_messages.extend( | ||
| convert_to_messages([msg]) # type: ignore[list-item] | ||
| ) |
There was a problem hiding this comment.
Returning a fixed AIMessage skips later formatting of its content. An assistant history message with nonempty tool_calls and content "Checking {{location}}" keeps those braces even after format_messages(location="Paris"). Previously, that content went through _get_langchain_prompt_string, so the supplied value replaced the variable.
Preserve later content formatting while retaining the tool fields, and add a test for this case.
Knowledge Base Used: Prompt retrieval, compilation, and caching
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/model.py
Line: 472-474
Comment:
**History variables stay unfilled**
Returning a fixed `AIMessage` skips later formatting of its content. An assistant history message with nonempty `tool_calls` and content `"Checking {{location}}"` keeps those braces even after `format_messages(location="Paris")`. Previously, that content went through `_get_langchain_prompt_string`, so the supplied value replaced the variable.
Preserve later content formatting while retaining the tool fields, and add a test for this case.
**Knowledge Base Used:** [Prompt retrieval, compilation, and caching](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/prompt-retrieval-compilation-and-cache.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 25e2048. Tool history now uses a LangChain message template: it formats the content at format_messages() time and copies the original message with only that content replaced, preserving tool_calls, tool_call_id, and name. The real LangChain regression failed on the previous head (Checking {{location}} remained literal) and now passes for assistant and tool content. The two related prompt test files pass 61 tests; full Ruff, formatting, and mypy checks pass.
| msg.get("tool_calls") or msg.get("tool_call_id") | ||
| ): | ||
| try: | ||
| from langchain_core.messages import convert_to_messages # noqa: PLC0415, I001 |
There was a problem hiding this comment.
The new convert_to_messages import sits inside get_langchain_prompt. This violates the repository instruction, “Move imports to the top of the module instead of placing them within functions or methods.” Move it to module scope, keeping the dependency optional if needed. This repository requirement must be satisfied before merging.
Rule Used: Move imports to the top of the module instead of placing them within functions or methods. (source)
Learned From
langfuse/langfuse-python#1387
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/model.py
Line: 467
Comment:
**Import breaks repository rule**
The new `convert_to_messages` import sits inside `get_langchain_prompt`. This violates the repository instruction, “Move imports to the top of the module instead of placing them within functions or methods.” Move it to module scope, keeping the dependency optional if needed. This repository requirement must be satisfied before merging.
**Rule Used:** Move imports to the top of the module instead of placing them within functions or methods. ([source](https://app.greptile.com/personal-org-4986/-/custom-context?memory=c960fc07-9928-409f-a18b-a780cbdded12))
**Learned From**
[langfuse/langfuse-python#1387](https://github.com/langfuse/langfuse-python/pull/1387)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
I checked the current AGENTS.md and code_review.md; neither contains the quoted import rule. langchain-core remains optional for the SDK, and the existing MessagesPlaceholder path also imports it lazily. I kept the new tool path lazy for the same reason. A fresh-process check with all langchain imports blocked confirms ordinary SDK/text/tuple paths still work and actual tool conversion raises the documented ImportError.
There was a problem hiding this comment.
You're right that the quoted rule isn't in AGENTS.md or code_review.md, and I was wrong to call it a repository requirement. It comes from an active configured review rule: View rule.
Your technical rationale is sound: keeping this import lazy preserves langchain-core as an optional dependency, matches the existing MessagesPlaceholder path, and your fresh-process check verifies that ordinary SDK paths still work without it. The remaining objection is only that the configured rule is unconditional about module-level imports; it doesn't account for optional, feature-specific dependencies. So my earlier wording overstated the repository basis for the comment, but the rule conflict itself remains until that configured rule is changed or scoped to allow this pattern.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
What does this PR do?
A resolved tool-call history passed to
get_langchain_prompt()losestool_callsandtool_call_idwhen every message becomes a(role, content)tuple.ChatPromptTemplate.from_messages()then raisesValueError: Unexpected message type: tool.Convert tool calls and results to LangChain message templates, preserving their IDs and other tool metadata while formatting content at
format_messages()time. Ordinary messages retain their tuple path, including histories withtool_calls=Noneor[]. LangChain imports stay lazy, preserving its optional dependency boundary.Related: #1840 handles input message objects, #1894 handles non-string content, and #2060 handles literal substitution. This fixes the output conversion of tool metadata.
Type of change
Verification
The extended existing tool-history test fails on main with the exception above. On the initial PR head, an additional real LangChain regression failed because tool-history content
Checking {{location}}was not formatted. After the follow-up fix, both prompt test files pass (61 tests), including delayed assistant/tool content formatting and preservation of complete tool metadata. Full Ruff lint/format and mypy pass. A fresh process blocking every LangChain import also verifies ordinary SDK/text/tuple paths work without that optional dependency.Validation environment and limits
Validated on Python 3.13.3 and langchain-core 1.2.22 after
uv sync --frozen. The cache fixtures use CI dummy keys and mocked API; no backend/live-provider tests or multi-Python matrix were run.Checklist
code_review.md..env.templateif needed. (Return docstring updated.)Restore later formatting for tool-call content and satisfy the import rule before merging.
Summary
The PR preserves tool-call IDs when converting resolved history for LangChain.
Reviews (1) · Last reviewed commit: "fix(prompts): preserve tool messages in ..." · Reviewed by Greptile