mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-11-07 19:20:34 +00:00
fix(skills): pair a tool result with its own response's calls (#7929)
Tool-call ids are unique per assistant response, not per conversation. extract_skills built a single id -> path map over the whole message list, so a later read that reused an id overwrote the earlier one: the earlier skill silently dropped out of the ledger and its correct ToolMessage logged "mismatched skill read metadata". Rebuild the map from each AIMessage's own calls and resolve a ToolMessage against the nearest preceding response, matching the pairing rule tool_call_args.pair_tool_call_results already applies. Fixes #7858 Signed-off-by: Batianxieshen1 <280706568+Batianxieshen1@users.noreply.github.com> Co-authored-by: Batianxieshen1 <280706568+Batianxieshen1@users.noreply.github.com>
This commit is contained in:
parent
3067b5a5bd
commit
07b4e06697
@ -140,25 +140,36 @@ def extract_skills(
|
||||
skills_root: str,
|
||||
read_tool_names: Collection[str],
|
||||
) -> list[SkillEntry]:
|
||||
"""Enumerate skill-file reads (AI read_file call + paired ToolMessage result)."""
|
||||
"""Enumerate skill-file reads (AI read_file call + paired ToolMessage result).
|
||||
|
||||
A result pairs only with a read call from the nearest preceding AIMessage,
|
||||
because tool-call ids are unique per response, not per conversation; a
|
||||
result that ended up non-adjacent to its call is dropped.
|
||||
"""
|
||||
normalized_root = posixpath.normpath(skills_root.rstrip("/") or "/")
|
||||
read_names = frozenset(read_tool_names)
|
||||
|
||||
skill_paths_by_id: dict[str, str] = {}
|
||||
for message in messages:
|
||||
if not isinstance(message, AIMessage):
|
||||
continue
|
||||
for tool_call in message.tool_calls or []:
|
||||
if _tool_call_name(tool_call) not in read_names:
|
||||
continue
|
||||
tool_call_id = _tool_call_id(tool_call)
|
||||
raw_path = _tool_call_path(tool_call)
|
||||
path = _normalize_under_root(raw_path, normalized_root) if raw_path else None
|
||||
if tool_call_id and path and _is_skill_file(path):
|
||||
skill_paths_by_id[tool_call_id] = path
|
||||
|
||||
entries: list[SkillEntry] = []
|
||||
# Tool-call ids are unique per assistant response, not per conversation, so
|
||||
# this map is rebuilt from each AIMessage's own calls and a ToolMessage is
|
||||
# resolved against the nearest preceding response — the pairing rule
|
||||
# ``tool_call_args.pair_tool_call_results`` applies. A single map for the
|
||||
# whole conversation let a later read overwrite an earlier read that reused
|
||||
# the same id, which dropped the earlier skill from the ledger and logged a
|
||||
# mismatch for a read that was correct.
|
||||
skill_paths_by_id: dict[str, str] = {}
|
||||
for index, message in enumerate(messages):
|
||||
if isinstance(message, AIMessage):
|
||||
skill_paths_by_id = {}
|
||||
for tool_call in message.tool_calls or []:
|
||||
if _tool_call_name(tool_call) not in read_names:
|
||||
continue
|
||||
tool_call_id = _tool_call_id(tool_call)
|
||||
raw_path = _tool_call_path(tool_call)
|
||||
path = _normalize_under_root(raw_path, normalized_root) if raw_path else None
|
||||
if tool_call_id and path and _is_skill_file(path):
|
||||
skill_paths_by_id[tool_call_id] = path
|
||||
continue
|
||||
if not isinstance(message, ToolMessage):
|
||||
continue
|
||||
if getattr(message, "status", "success") == "error":
|
||||
|
||||
@ -221,6 +221,98 @@ class TestExtractSkills:
|
||||
out = extract_skills(msgs, skills_root=_ROOT, read_tool_names=_READ)
|
||||
assert [e["name"] for e in out] == ["a", "b"]
|
||||
|
||||
def test_same_tool_call_id_reused_by_a_later_read_keeps_both_skills(self):
|
||||
msgs = [
|
||||
_ai_read("call_0", "/mnt/skills/public/a/SKILL.md"),
|
||||
ToolMessage(
|
||||
content="A",
|
||||
tool_call_id="call_0",
|
||||
id="tm1",
|
||||
additional_kwargs=_skill_metadata("/mnt/skills/public/a/SKILL.md", "A"),
|
||||
),
|
||||
_ai_read("call_0", "/mnt/skills/custom/b/SKILL.md"),
|
||||
ToolMessage(
|
||||
content="B",
|
||||
tool_call_id="call_0",
|
||||
id="tm2",
|
||||
additional_kwargs=_skill_metadata("/mnt/skills/custom/b/SKILL.md", "B"),
|
||||
),
|
||||
]
|
||||
|
||||
out = extract_skills(msgs, skills_root=_ROOT, read_tool_names=_READ)
|
||||
|
||||
assert [e["name"] for e in out] == ["a", "b"]
|
||||
assert [e["path"] for e in out] == [
|
||||
"/mnt/skills/public/a/SKILL.md",
|
||||
"/mnt/skills/custom/b/SKILL.md",
|
||||
]
|
||||
|
||||
def test_same_tool_call_id_reused_does_not_warn_for_the_earlier_read(self, caplog):
|
||||
msgs = [
|
||||
_ai_read("call_0", "/mnt/skills/public/a/SKILL.md"),
|
||||
ToolMessage(
|
||||
content="A",
|
||||
tool_call_id="call_0",
|
||||
id="tm1",
|
||||
additional_kwargs=_skill_metadata("/mnt/skills/public/a/SKILL.md", "A"),
|
||||
),
|
||||
_ai_read("call_0", "/mnt/skills/custom/b/SKILL.md"),
|
||||
ToolMessage(
|
||||
content="B",
|
||||
tool_call_id="call_0",
|
||||
id="tm2",
|
||||
additional_kwargs=_skill_metadata("/mnt/skills/custom/b/SKILL.md", "B"),
|
||||
),
|
||||
]
|
||||
|
||||
with caplog.at_level("WARNING", logger="deerflow.agents.middlewares.skill_context"):
|
||||
out = extract_skills(msgs, skills_root=_ROOT, read_tool_names=_READ)
|
||||
|
||||
assert len(out) == 2
|
||||
assert "mismatched skill read metadata" not in caplog.text
|
||||
|
||||
def test_tool_result_whose_call_is_only_in_an_earlier_response_is_skipped(self, caplog):
|
||||
# A tool-call-less AIMessage between the read and its late result must
|
||||
# still close the response: the result cannot pair across it.
|
||||
msgs = [
|
||||
_ai_read("call_0", "/mnt/skills/public/a/SKILL.md"),
|
||||
AIMessage(content="thinking"),
|
||||
ToolMessage(
|
||||
content="A",
|
||||
tool_call_id="call_0",
|
||||
id="tm1",
|
||||
additional_kwargs=_skill_metadata("/mnt/skills/public/a/SKILL.md", "A"),
|
||||
),
|
||||
]
|
||||
|
||||
with caplog.at_level("WARNING", logger="deerflow.agents.middlewares.skill_context"):
|
||||
out = extract_skills(msgs, skills_root=_ROOT, read_tool_names=_READ)
|
||||
|
||||
assert out == []
|
||||
assert "mismatched skill read metadata" not in caplog.text
|
||||
assert "missing skill read metadata" not in caplog.text
|
||||
|
||||
def test_missing_metadata_warning_points_at_the_later_reads_path(self, caplog):
|
||||
msgs = [
|
||||
_ai_read("call_0", "/mnt/skills/public/a/SKILL.md"),
|
||||
ToolMessage(
|
||||
content="A",
|
||||
tool_call_id="call_0",
|
||||
id="tm1",
|
||||
additional_kwargs=_skill_metadata("/mnt/skills/public/a/SKILL.md", "A"),
|
||||
),
|
||||
_ai_read("call_0", "/mnt/skills/custom/b/SKILL.md"),
|
||||
ToolMessage(content="B", tool_call_id="call_0", id="tm2"),
|
||||
]
|
||||
|
||||
with caplog.at_level("WARNING", logger="deerflow.agents.middlewares.skill_context"):
|
||||
out = extract_skills(msgs, skills_root=_ROOT, read_tool_names=_READ)
|
||||
|
||||
assert [e["name"] for e in out] == ["a"]
|
||||
assert "missing skill read metadata" in caplog.text
|
||||
assert "/mnt/skills/custom/b/SKILL.md" in caplog.text
|
||||
assert "/mnt/skills/public/a/SKILL.md" not in caplog.text
|
||||
|
||||
def test_extract_skills_prefers_metadata_only_when_path_matches_read_call(self):
|
||||
msgs = [
|
||||
_ai_read("r1", "/mnt/skills/public/data-analysis/SKILL.md"),
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user