fix: correct {step_N} placeholder regex in executor#161
Merged
ShuxinLin merged 4 commits intofeat/156-fmsr-mcp-serverfrom Feb 20, 2026
Merged
fix: correct {step_N} placeholder regex in executor#161ShuxinLin merged 4 commits intofeat/156-fmsr-mcp-serverfrom
ShuxinLin merged 4 commits intofeat/156-fmsr-mcp-serverfrom
Conversation
- Clear stale "in progress" comment from mcp/servers/fmsr/__init__.py - Add python-dotenv as explicit runtime dependency in pyproject.toml - Remove stale Pydantic V1 warning note from INSTRUCTIONS.md (warning is now suppressed) - Document get_failure_mode_sensor_mapping scalability limitation in tool docstring (sequential LLM calls; keep lists small) Signed-off-by: Shuxin Lin <shuxin.lin@ibm.com> Signed-off-by: Shuxin Lin <linshuhsin@gmail.com>
_PLAN_PROMPT uses str.format(), so {{step_N}} in the template renders
to {step_N} (single braces) in the prompt the LLM receives. The LLM
therefore generates args like {"asset_id": "{step_2}"}, but the old
regex r"\{\{step_(\d+)\}\}" only matched double-brace {{step_N}} —
so _has_placeholders() always returned False and the raw placeholder
string was forwarded directly to the MCP tool.
Fix: change _PLACEHOLDER_RE to r"\{step_(\d+)\}" (single braces).
Update all test fixtures from "{{step_N}}" to "{step_N}" to reflect
the actual format produced by the LLM.
Signed-off-by: Shuxin Lin <shuxin.lin@ibm.com>
Signed-off-by: Shuxin Lin <linshuhsin@gmail.com>
The original bug was undetected because all test fixtures built PlanStep
objects directly with "{{step_N}}" (double braces), which matched the
buggy double-brace regex. No test exercised the real path where
parse_plan() receives LLM output containing {step_N} (single braces).
Add three tests that would have caught the bug:
test_planner.py:
- test_placeholder_args_preserved_as_string: verifies parse_plan stores
{step_N} placeholder strings verbatim in tool_args
- test_placeholder_in_parsed_args_detected: verifies _has_placeholders()
returns True for args produced by parse_plan with {step_N} placeholders
test_runner.py:
- test_pipeline_resolves_placeholder_from_planner_output: end-to-end
pipeline test — planner LLM returns a plan with {step_N} in args,
executor resolves it via LLM, tool is called with the resolved value
not the literal placeholder string
Signed-off-by: Shuxin Lin <shuxin.lin@ibm.com>
Signed-off-by: Shuxin Lin <linshuhsin@gmail.com>
Add tool and tool_args fields to StepResult so the executor records
which tool was called and with what arguments (after placeholder
resolution). The CLI --show-history flag now prints:
[OK ] Step 2 (IoTAgent): List all assets at site MAIN
tool: assets args: {'site_name': 'MAIN'}
{"site_name": "MAIN", "total_assets": 1, ...}
The --json output also gains tool and tool_args per history entry.
Signed-off-by: Shuxin Lin <shuxin.lin@ibm.com>
Signed-off-by: Shuxin Lin <linshuhsin@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #160.
_PLAN_PROMPTinplanner.pyis rendered withstr.format(). The template contains{{step_N}}(double braces), which Python's.format()converts to{step_N}(single braces) in the prompt text the LLM receives. The LLM therefore generates args such as:{"asset_id": "{step_2}"}The old placeholder detection regex was:
This never matched the single-brace
{step_2}the LLM produced, so_has_placeholders()always returnedFalseand the raw placeholder string was forwarded directly to the MCP tool — causing errors like:Fix: change
_PLACEHOLDER_REto match single braces:All test fixtures updated from
"{{step_N}}"to"{step_N}"to match actual LLM output format.Test plan
mcp/plan_execute/tests/unit tests passtest_has_placeholders_true—"{step_1}"correctly detectedtest_executor_resolves_placeholder_via_llm— placeholder resolved via LLM calltest_executor_no_placeholder_skips_llm— no-placeholder steps still skip LLM