fix(langchain): handle NotRequired fields in InjectedState without KeyError#35684
Conversation
…yError When InjectedState references a TypedDict field marked as NotRequired, the upstream langgraph ToolNode accesses state[key] which raises KeyError if the field is absent from the runtime state. This introduces a ToolNode subclass in langchain that overrides _inject_tool_args to use .get() (dict state) and getattr with a default (object state) so that missing optional fields resolve to None instead of crashing. The factory's create_agent now uses this patched ToolNode automatically. Fixes langchain-ai#35585
96399ce to
258b24d
Compare
- Add type parameters to generic dict (dict[str, Any]) - Annotate state as Any to prevent unreachable-code warnings from isinstance checks (StateT defaults to dict, hiding list/object branches)
|
Friendly ping — CI is green, tests pass, rebased on latest. Ready for review whenever convenient. Happy to address any feedback. 🙏 |
✅ Verified with unit testsEnvironment: Python 3.13.12, macOS, The |
|
ccurme (@ccurme) Fixes |
Resolve the failing langchain_v1 lint job on PR langchain-ai#35684 by auto-formatting the test import block and replacing the mutable class attribute used in ObjState with an instance attribute. Targeted validation in libs/langchain_v1/.venv now passes: ruff check, ruff format --check, and pytest tests/unit_tests/tools/test_tool_node.py -q.
Resolve the failing langchain_v1 lint job on PR langchain-ai#35684 by auto-formatting the test import block and replacing the mutable class attribute used in ObjState with an instance attribute. Targeted validation in libs/langchain_v1/.venv now passes: ruff check, ruff format --check, and pytest tests/unit_tests/tools/test_tool_node.py -q. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
I verified this locally on the actual Using a local class CustomAgentState(AgentState[Any]):
city: NotRequired[str]Observed locally:
I also ran an end-to-end variant where the tool accepts
Separately, I reran targeted |
|
Friendly ping — rebased on latest and ready for review. Happy to address any feedback! |
Alvin Tang (alvinttang)
left a comment
There was a problem hiding this comment.
Review: Handle NotRequired fields in InjectedState without KeyError
Approach: full method override vs. minimal patch
The fix works by subclassing langgraph.prebuilt.ToolNode and completely re-implementing _inject_tool_args. This is ~80 lines of duplicated logic from the upstream class. While it solves the immediate problem, it creates a maintenance burden: any future changes to langgraph.prebuilt.ToolNode._inject_tool_args (new injection types, behavioral fixes, performance improvements) will not be reflected in this subclass. The two implementations will silently diverge.
Alternative approaches worth considering:
- Fix this upstream in
langgraphitself (the root cause is there), and pin/require the fixed version. - Use a
try/except KeyErrorwrapper aroundsuper()._inject_tool_args()with fallback logic — much less code duplication. - Monkey-patch just the dict access pattern rather than copying the entire method.
Semantic concern: None vs. raising
When a NotRequired field is absent, this fix injects None as the argument value. This is a reasonable default, but it changes the contract: the tool function now receives None for a field it declared as Annotated[str, InjectedState("city")] — note the type is str, not str | None. This means:
- The tool author may not expect
Noneand could get aTypeErrordownstream (e.g.,f"Sunny in {city}"would produce"Sunny in None"instead of failing clearly). - A more explicit approach might be to use
Optional[str]orstr | Nonein the tool signature when referencingNotRequiredfields, and document that convention.
This isn't necessarily a blocker, but worth documenting the behavior.
Code correctness
The reimplemented logic itself looks correct:
state.get(state_field)for dict state — correct, returnsNonefor missing keysgetattr(state, state_field, None)for object state — correct- Store and runtime injection logic matches upstream
- List-state handling with error messages matches upstream
Tests are good
The 4 test cases cover the key scenarios well:
- Present field (baseline)
- Absent
NotRequiredfield (the regression) - Full state injection
- Object-based state with missing attribute
One missing test: what happens when store injection is combined with a missing state field? The current tests only exercise state injection in isolation.
Nit
from langgraph.prebuilt.tool_node import _get_all_injected_argsImporting a private function (_get_all_injected_args) from langgraph creates a coupling to an internal API that could change without notice. This is another argument for fixing this upstream.
Summary
The fix is correct and the tests are solid, but the full method duplication is a maintenance risk. I'd recommend either pushing the .get() fix upstream to langgraph, or at minimum adding a comment noting the upstream version this was forked from so future maintainers know when to sync.
|
Thanks for the thorough review Alvin Tang (@alvinttang) — these are all valid concerns. On approach (full override vs minimal patch): If the maintainers prefer, I can:
On On private API import: Happy to take whichever direction the maintainers prefer. What would you recommend — upstream fix + close this, or keep this as a minimal workaround? |
ee9b5ac to
5e7bb37
Compare
Alvin Tang (alvinttang)
left a comment
There was a problem hiding this comment.
Good fix for a real pain point — NotRequired fields in TypedDict state are a natural pattern, and crashing with KeyError is unexpected. A few observations:
1. The core fix (.get() / getattr(..., None)) is correct
Using state.get(state_field) for dict state and getattr(state, state_field, None) for object state is the right approach. It matches how NotRequired / Optional fields should behave — absent means None, not an error.
2. This is a full copy of _inject_tool_args from upstream langgraph
The override copies ~50 lines of logic from langgraph.prebuilt.ToolNode._inject_tool_args. This creates a maintenance burden: any future changes to the upstream method (new injection types, bug fixes, refactoring) won't be picked up by this subclass. A few alternatives to consider:
- Could a targeted
try/except KeyErrorwrapper aroundsuper()._inject_tool_args()achieve the same result with less code duplication? - Alternatively, could this fix be contributed upstream to
langgraphdirectly, so the langchain wrapper isn't needed? - If the full override is necessary, adding a comment noting the upstream source and version would help future maintainers know when to re-sync.
3. The retrieve and aretrieve overrides in the diff appear to be from the wrong PR
Looking at the diff more carefully — the retrieve and aretrieve standalone functions at the bottom of fusion_retriever.py don't appear to be part of this PR's changes (they look like they might have been merged from another branch). Could you verify this? They define module-level functions with self as a parameter, which would fail at runtime since they're not bound to any class.
Edit: I see these might be from a different file — ignore if this is a diff artifact.
4. Tests are thorough and well-structured
The four test cases cover the key scenarios: field present, field absent (the regression), full-state injection, and object-state with missing attribute. Good use of MagicMock(spec=ToolRuntime).
5. Minor: the factory.py import change
Switching from from langgraph.prebuilt.tool_node import ToolNode to from langchain.tools.tool_node import ToolNode ensures create_agent uses the fixed subclass. This is correct, but it means any code that imports ToolNode directly from langgraph won't get the fix. Worth noting in the docs.
Clean fix overall. The main concern is the long-term maintenance cost of duplicating the upstream method — ideally this would be fixed in langgraph itself.
Summary
Fixes #35585.
When
InjectedState("field")references aTypedDictfield marked asNotRequired, the upstreamlanggraph.prebuilt.ToolNode._inject_tool_argsmethod accessesstate[field]directly — raisingKeyErrorwhen that field is absent from the runtime state.What changed
langchain/tools/tool_node.py— Introduced aToolNodesubclass that overrides_inject_tool_argsto use.get()(for dict state) andgetattr(…, None)(for object state), so missing optional fields resolve toNoneinstead of crashing.langchain/agents/factory.py— Updatedcreate_agentto import and use the patchedToolNodefromlangchain.tools.tool_noderather than importing directly fromlanggraph.tests/unit_tests/tools/test_tool_node.py— Added 4 tests covering:NotRequiredfield absent → returnsNone(the core regression)Reproducer (from issue)
Before this fix, invoking the tool when
citywas never set in state raised:Testing
All 734 existing unit tests pass (
tests/unit_tests/agents/+tests/unit_tests/tools/), plus the 4 new tests.