diff --git a/backend/packages/harness/deerflow/skills/review/resource_graph.py b/backend/packages/harness/deerflow/skills/review/resource_graph.py index 7ff837eb61..13718c6307 100644 --- a/backend/packages/harness/deerflow/skills/review/resource_graph.py +++ b/backend/packages/harness/deerflow/skills/review/resource_graph.py @@ -2,7 +2,9 @@ from __future__ import annotations +import posixpath import re +from collections.abc import Set as AbstractSet from pathlib import PurePosixPath from typing import Any @@ -11,7 +13,7 @@ from deerflow.skills.review.models import make_finding, normalize_relative_path _MARKDOWN_LINK_RE = re.compile(r"!?\[[^\]]*]\(([^)\s]+)(?:\s+\"[^\"]*\")?\)") _CODE_SPAN_RE = re.compile(r"`([^`]+)`") -_PATH_TOKEN_RE = re.compile(r"(? tuple[dict[str, Any], list continue content = str(entry.get("content") or "") for raw_ref in _extract_references(content): - resolved = _resolve_reference(path, raw_ref) + resolved = _resolve_reference(path, raw_ref, files.keys()) if resolved is None: continue if resolved == "__ESCAPES__": @@ -96,18 +98,32 @@ _TRAILING_SENTENCE_PUNCTUATION = ".?!" def _extract_references(content: str) -> set[str]: refs: set[str] = set() + # Markdown link syntax fixes the fragment semantics: the text after '#' + # in a link target is ALWAYS a URL fragment, never part of the filename + # — a link to a file literally named "faq.md#pricing" would have to + # percent-encode it. So links always strip the fragment, and their full + # construct is blanked out of the residual text: the code-span and + # bare-path passes below are literal-path contexts where '#' may be + # part of a real filename, and they must never see link-internal text. + # Trailing sentence punctuation is still stripped here (#5739). + residual = content for match in _MARKDOWN_LINK_RE.finditer(content): refs.add(match.group(1).split("#", 1)[0].rstrip(_TRAILING_SENTENCE_PUNCTUATION)) - for match in _CODE_SPAN_RE.finditer(content): + residual = residual.replace(match.group(0), " " * len(match.group(0))) + for match in _CODE_SPAN_RE.finditer(residual): token = match.group(1).strip() if "/" in token: refs.add(token.rstrip(_TRAILING_SENTENCE_PUNCTUATION)) - for match in _PATH_TOKEN_RE.finditer(content): + for match in _PATH_TOKEN_RE.finditer(residual): refs.add(match.group(0).rstrip(_TRAILING_SENTENCE_PUNCTUATION)) return refs -def _resolve_reference(source_path: str, raw_ref: str) -> str | None: +def _resolve_reference( + source_path: str, + raw_ref: str, + files: AbstractSet[str] | None = None, +) -> str | None: ref = raw_ref.strip().strip("\"'") if not ref or ref.startswith("#") or re.match(r"^[A-Za-z][A-Za-z0-9+.-]*:", ref): return None @@ -117,7 +133,23 @@ def _resolve_reference(source_path: str, raw_ref: str) -> str | None: base = PurePosixPath(source_path).parent if "://" in ref: return None - candidate = (base / ref).as_posix() - return normalize_relative_path(candidate) + if "#" in ref: + pre_hash, fragment = ref.split("#", 1) + # A '..' segment in the post-'#' text must never participate in + # normalization: it would collapse the hash-bearing segment and + # retarget the edge ("faq.md#/../other.md" must resolve to + # faq.md). Any other fragment gets its chance first: the whole + # token may name a real package file — '#' is legal in file AND + # directory names ("references/C#.md", "references/C#/readme.md"), + # from nested sources too ("../C#.md"). Only when the + # canonicalized whole token matches no file does the suffix + # become a section fragment and get stripped. + if "/.." in f"/{fragment}" or fragment.startswith(".."): + ref = pre_hash + elif files is not None and posixpath.normpath((base / ref).as_posix()) in files: + return normalize_relative_path((base / ref).as_posix()) + else: + ref = pre_hash + return normalize_relative_path((base / ref).as_posix()) except ValueError: return "__ESCAPES__" diff --git a/backend/tests/test_skill_review_core.py b/backend/tests/test_skill_review_core.py index 66c29ff34d..3f02a5cb0e 100644 --- a/backend/tests/test_skill_review_core.py +++ b/backend/tests/test_skill_review_core.py @@ -152,6 +152,133 @@ def test_resource_graph_keeps_real_dotted_filenames(tmp_path): assert not any(f["rule_id"] == "resource.missing" and f["path"] == "SKILL.md" for f in facts["findings"]) +def test_resource_graph_strips_fragment_from_code_span_refs(tmp_path): + # A code span can carry a section anchor just like a markdown link + # target ("`references/faq.md#pricing`"). The anchor is not part of + # the path: it must not turn a valid reference into a + # resource.missing finding. + _write( + tmp_path / "SKILL.md", + _valid_skill() + "\nSee `references/faq.md#pricing` for details.\n", + ) + _write(tmp_path / "references" / "faq.md", "# FAQ\n") + + facts = analyze_skill_package(LocalDirectoryReader(tmp_path).read()) + + assert {"source": "SKILL.md", "target": "references/faq.md"} in facts["resources"]["edges"] + assert "references/faq.md" not in facts["resources"]["orphans"] + assert not any(f["rule_id"] == "resource.missing" and f["path"] == "SKILL.md" for f in facts["findings"]) + + +def test_resource_graph_keeps_real_dots_when_stripping_code_span_fragments(tmp_path): + # Stripping the fragment must not eat real dotted filenames: the + # extension dots of a fragment-bearing code span reference survive. + _write( + tmp_path / "SKILL.md", + _valid_skill() + "\nRead `references/v1.0.md#notes`.\n", + ) + _write(tmp_path / "references" / "v1.0.md", "# v1.0\n") + + facts = analyze_skill_package(LocalDirectoryReader(tmp_path).read()) + + assert {"source": "SKILL.md", "target": "references/v1.0.md"} in facts["resources"]["edges"] + assert not any(f["rule_id"] == "resource.missing" and f["path"] == "SKILL.md" for f in facts["findings"]) + + +def test_resource_graph_prefers_hash_filenames_over_fragments(tmp_path): + # A package filename may legally contain '#': a code-span reference to + # `references/C#.md` must resolve to the real file, not be truncated to + # `references/C` by fragment stripping. The suffix is only treated as a + # fragment when the exact path does not exist. + _write( + tmp_path / "SKILL.md", + _valid_skill() + "\nSee `references/C#.md` and `references/faq.md#pricing`.\n", + ) + _write(tmp_path / "references" / "C#.md", "# C#\n") + _write(tmp_path / "references" / "faq.md", "# FAQ\n") + + facts = analyze_skill_package(LocalDirectoryReader(tmp_path).read()) + + assert {"source": "SKILL.md", "target": "references/C#.md"} in facts["resources"]["edges"] + assert {"source": "SKILL.md", "target": "references/faq.md"} in facts["resources"]["edges"] + assert not any(f["rule_id"] == "resource.missing" and f["path"] == "SKILL.md" for f in facts["findings"]) + + +def test_resource_graph_prefers_hash_filenames_from_nested_sources(tmp_path): + # The literal-file check must canonicalize leading relative segments + # before comparing against the snapshot keys: from + # references/sub/guide.md, `../C#.md` joins to + # `references/sub/../C#.md`, which matches no key verbatim — without + # canonicalization the reference is truncated to `../C` and produces a + # false resource.missing plus an orphan report for the real file. + _write(tmp_path / "SKILL.md", _valid_skill()) + _write( + tmp_path / "references" / "sub" / "guide.md", + _valid_skill("guide") + "\nSee `../C#.md`.\n", + ) + _write(tmp_path / "references" / "C#.md", "# C#\n") + + facts = analyze_skill_package(LocalDirectoryReader(tmp_path).read()) + + assert {"source": "references/sub/guide.md", "target": "references/C#.md"} in facts["resources"]["edges"] + assert not any(f["rule_id"] in {"resource.missing", "resource.escaping-link"} and f["path"] == "references/sub/guide.md" for f in facts["findings"]) + + +def test_resource_graph_prefers_hash_directory_paths_over_stripping(tmp_path): + # '#' is legal in directory names too: the whole token + # `references/C#/readme.md` names a real file even though the text + # after '#' contains '/'. It must not be truncated to `references/C` + # — only a '..' segment in the post-'#' text forces the strip-first + # fallback ("faq.md#/../other.md"). + _write( + tmp_path / "SKILL.md", + _valid_skill() + "\nSee `references/C#/readme.md`.\n", + ) + _write(tmp_path / "references" / "C#" / "readme.md", "# C#\n") + + facts = analyze_skill_package(LocalDirectoryReader(tmp_path).read()) + + assert {"source": "SKILL.md", "target": "references/C#/readme.md"} in facts["resources"]["edges"] + assert not any(f["rule_id"] == "resource.missing" and f["path"] == "SKILL.md" for f in facts["findings"]) + + +def test_resource_graph_strips_fragment_before_normalizing_fallback(tmp_path): + # The exact-path preference must check the literal token: normalizing + # first collapses a hash-bearing segment ("faq.md#/.." -> "other.md") + # and can silently retarget the edge and orphan faq.md. The fragment is + # dropped first when the literal token is not a real file. + _write( + tmp_path / "SKILL.md", + _valid_skill() + "\nSee `references/faq.md#/../other.md`.\n", + ) + _write(tmp_path / "references" / "faq.md", "# FAQ\n") + _write(tmp_path / "references" / "other.md", "# Other\n") + + facts = analyze_skill_package(LocalDirectoryReader(tmp_path).read()) + + assert {"source": "SKILL.md", "target": "references/faq.md"} in facts["resources"]["edges"] + assert not any(f["rule_id"] == "resource.missing" and f["path"] == "SKILL.md" for f in facts["findings"]) + + +def test_resource_graph_always_strips_markdown_link_fragments(tmp_path): + # In Markdown link syntax the text after '#' is always a URL fragment — + # a link to a file literally named "faq.md#pricing" would have to + # percent-encode it. The link must resolve to faq.md and stay broken + # (resource.missing) even when a file literally named + # references/faq.md#pricing exists; the bare-path pass must not see the + # link-internal text and resurrect the literal edge. + _write( + tmp_path / "SKILL.md", + _valid_skill() + "\nSee [FAQ](references/faq.md#pricing).\n", + ) + _write(tmp_path / "references" / "faq.md#pricing", "# trap\n") + + facts = analyze_skill_package(LocalDirectoryReader(tmp_path).read()) + + assert not any(e["target"] == "references/faq.md#pricing" for e in facts["resources"]["edges"]) + assert any(f["rule_id"] == "resource.missing" and f["path"] == "SKILL.md" and "references/faq.md" in f["message"] for f in facts["findings"]) + + def test_resource_graph_ignores_eval_fixture_references(tmp_path): _write(tmp_path / "SKILL.md", _valid_skill()) _write(