fix(state): use merge_dicts reducer for scratchpad to prevent key loss on concurrent writes - #209
Conversation
When multiple save_note tool calls run in the same executor_tools superstep, they all read the same base state and each produce a full merged-scratchpad update. The previous take_last reducer discarded all but the last update, silently losing keys written by earlier tool calls. merge_dicts accumulates every key across all concurrent updates, preserving all writes. Fixes minitap-ai#208
📝 WalkthroughWalkthroughThis PR adds a dictionary merge helper and changes ChangesScratchpad merge behavior
🎯 1 (Trivial) | ⏱️ ~2 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
minitap/mobile_use/graph/state.py (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant docstring on Line 21.
This sentence restates exactly what the implementation already makes obvious; prefer code clarity over explanatory restatement comments.
As per path instructions, "Comments: flag any comment that restates what the code already says."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@minitap/mobile_use/graph/state.py` at line 21, Remove the redundant docstring in the state module: the brief description on the merge helper is just restating what the implementation already makes obvious. Delete the docstring near the dict-merge logic in state.py and leave the function/class code unchanged so the intent comes from the code itself.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@minitap/mobile_use/graph/state.py`:
- Line 21: Remove the redundant docstring in the state module: the brief
description on the merge helper is just restating what the implementation
already makes obvious. Delete the docstring near the dict-merge logic in
state.py and leave the function/class code unchanged so the intent comes from
the code itself.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3b9d1d8-c220-40d9-bb25-2c8790ed1074
📒 Files selected for processing (1)
minitap/mobile_use/graph/state.py
plfavreau
left a comment
There was a problem hiding this comment.
Thank you @citizen204 for your contribution ! Merging your PR.
Summary
When the executor calls
save_notemultiple times in the sameexecutor_toolssuperstep (i.e. the LLM emits several tool calls atonce), every
save_noteinvocation reads the same basestate.scratchpadand produces a complete merged-copy update:The previous
take_lastreducer then keeps only the last update,silently discarding all keys written by earlier calls in the same
batch. With
merge_dictsas the reducer, LangGraph accumulates eachbranch's output via
{**a, **b}, so every key written by every toolcall in the same superstep survives.
Fixes #208
Changes
minitap/mobile_use/graph/state.py: addmerge_dictshelper and useit as the reducer for
scratchpadinstead oftake_lastSummary by CodeRabbit