-
Notifications
You must be signed in to change notification settings - Fork 7
feat: doc-to-code linkage manifest for deterministic selection #69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| """Doc-to-code linkage: deterministic file selection from front-matter declarations.""" | ||
|
|
||
| import re | ||
| from pathlib import Path | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] import-style Missing blank line between stdlib and third-party import groups. |
||
| import yaml | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [high] missing-dependency linkage.py imports yaml (PyYAML) but pyyaml is not declared in pyproject.toml production dependencies. PyYAML is only available as a transitive dependency of pre-commit (dev-only). The Dockerfile installs only main dependencies, so this will cause an ImportError at runtime when find_declared_docs is called for .md files. Suggested fix: Add pyyaml to the dependencies list in pyproject.toml. |
||
|
|
||
|
|
||
| def parse_doc_frontmatter(file_path): | ||
| """Extract code-to-docs front-matter from a documentation file. | ||
|
|
||
| Supports YAML front-matter (--- delimiters) for .md files, and | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] path-traversal parse_doc_frontmatter reads files via Path(file_path).read_text() without calling validate_file_path() from security_utils.py, which is used for all file I/O in generation.py. While paths come from rglob (not direct user input), the inconsistency with the codebase's existing security pattern is a gap. Suggested fix: Add validate_file_path(file_path, base_dir=doc_root) before reading, consistent with generation.py. |
||
| comment-based declarations for .rst and .adoc files. | ||
|
|
||
| Returns a dict with a "covers" key (list of source paths), or | ||
| empty dict if no declaration is found. | ||
| """ | ||
| try: | ||
| content = Path(file_path).read_text(encoding="utf-8") | ||
| except Exception: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] error-handling-idiom Bare except Exception silently swallows errors without logging, inconsistent with the logging pattern in discovery.py and generation.py. |
||
| return {} | ||
|
|
||
| suffix = Path(file_path).suffix | ||
|
|
||
| if suffix == ".md": | ||
| return _parse_yaml_frontmatter(content) | ||
| elif suffix == ".rst": | ||
| return _parse_rst_directive(content) | ||
| elif suffix == ".adoc": | ||
| return _parse_adoc_comment(content) | ||
| return {} | ||
|
|
||
|
|
||
| def _parse_yaml_frontmatter(content): | ||
| """Parse YAML front-matter between --- delimiters.""" | ||
| if not content.startswith("---"): | ||
| return {} | ||
| parts = content.split("---", 2) | ||
| if len(parts) < 3: | ||
| return {} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] edge-case Split-based front-matter delimiter parsing has standard limitations with consecutive --- sequences. No data corruption; declarations silently ignored in edge cases. |
||
| try: | ||
| fm = yaml.safe_load(parts[1]) | ||
| if isinstance(fm, dict) and "code-to-docs" in fm: | ||
| ctd = fm["code-to-docs"] | ||
| if isinstance(ctd, dict) and "covers" in ctd: | ||
| covers = ctd["covers"] | ||
| if isinstance(covers, list): | ||
| return {"covers": [str(p) for p in covers]} | ||
| except yaml.YAMLError: | ||
| pass | ||
| return {} | ||
|
|
||
|
|
||
| def _parse_rst_directive(content): | ||
| """Parse .. code-to-docs:: covers: path1, path2 from rst.""" | ||
| match = re.search(r"^\.\.\s+code-to-docs::\s*covers:\s*(.+)$", content, re.MULTILINE) | ||
| if match: | ||
| paths = [p.strip() for p in match.group(1).split(",") if p.strip()] | ||
| return {"covers": paths} if paths else {} | ||
| return {} | ||
|
|
||
|
|
||
| def _parse_adoc_comment(content): | ||
| """Parse // code-to-docs: covers: path1, path2 from adoc.""" | ||
| match = re.search(r"^//\s*code-to-docs:\s*covers:\s*(.+)$", content, re.MULTILINE) | ||
| if match: | ||
| paths = [p.strip() for p in match.group(1).split(",") if p.strip()] | ||
| return {"covers": paths} if paths else {} | ||
| return {} | ||
|
|
||
|
|
||
| def extract_changed_paths(diff_text): | ||
| """Extract all file paths changed in a unified diff.""" | ||
| paths = set() | ||
| for match in re.finditer(r"^diff --git a/(.+?) b/", diff_text, re.MULTILINE): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] function-docstring extract_changed_paths and find_declared_docs lack Args: and Returns: sections used by other public functions in the codebase. |
||
| paths.add(match.group(1)) | ||
| return paths | ||
|
|
||
|
|
||
| def find_declared_docs(diff_text, doc_root="."): | ||
| """Find doc files whose declared covers paths intersect with the diff. | ||
|
|
||
| Returns a list of (doc_path, "declared") tuples for docs that match, | ||
| and scans all doc files in doc_root. | ||
| """ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] return-type-consistency find_declared_docs returns tagged tuples (str, 'declared') but the call site immediately discards the tag. No other discovery function uses this pattern. |
||
| changed = extract_changed_paths(diff_text) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] symlink-following find_declared_docs uses Path(doc_root).rglob('*') which follows symlinks by default. In the GitHub Actions context where untrusted PRs are processed, a PR could introduce a symlink pointing outside the doc root; file contents would be read by parse_doc_frontmatter. Suggested fix: Add if doc.is_symlink(): continue before processing each path. |
||
| if not changed: | ||
| return [] | ||
|
|
||
| doc_extensions = {".md", ".rst", ".adoc"} | ||
| declared = [] | ||
|
|
||
| for doc in Path(doc_root).rglob("*"): | ||
| if not doc.is_file() or doc.suffix not in doc_extensions: | ||
| continue | ||
| if ".doc-index" in str(doc): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] path-traversal covers paths in front-matter are attacker-controlled values with no validation for .., absolute paths, or null bytes. Currently only used in string matching (no I/O), but lacks defense-in-depth. |
||
| continue | ||
| fm = parse_doc_frontmatter(str(doc)) | ||
| covers = fm.get("covers", []) | ||
| if not covers: | ||
| continue | ||
| for covered_path in covers: | ||
| if any(c == covered_path or c.startswith(covered_path + "/") for c in changed): | ||
| declared.append((str(doc), "declared")) | ||
| break | ||
|
|
||
| return declared | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -514,6 +514,16 @@ def main(): | |
| print("Asking AI for relevant files...") | ||
| relevant_files = ask_ai_for_relevant_files(diff, file_previews) | ||
|
|
||
| # Merge in declared docs (front-matter linkage). These skip the LLM call. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] import-placement Function-level from linkage import find_declared_docs inside main() is inconsistent with the codebase convention of module-level imports. |
||
| from linkage import find_declared_docs | ||
|
|
||
| declared_docs = find_declared_docs(diff) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] logic-error The declared docs merge runs unconditionally after file discovery, including when relevant_files was set from a previous interactive review's accepted files. If a user ran [review-docs], rejected a doc file with a covers front-matter declaration, then ran [update-docs], the merge re-adds the rejected file — overriding the user's explicit rejection and breaking the [update-docs] contract documented in CLAUDE.md. Suggested fix: Filter rejected files from declared_paths before merging, or skip linkage merge when honoring a previous review. |
||
| declared_paths = {d[0] for d in declared_docs} | ||
| if declared_paths: | ||
| print(f"Declared docs (front-matter linkage): {sorted(declared_paths)}") | ||
| existing = set(relevant_files) if relevant_files else set() | ||
| relevant_files = sorted(existing | declared_paths) | ||
|
|
||
| if not relevant_files: | ||
| print("AI did not suggest any files.") | ||
| if review_mode or update_mode or feature_mode: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| """Tests for doc-to-code linkage front-matter parsing.""" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] test-coverage-gap Tests do not cover directory prefix matching (c.startswith(covered_path + '/')), malformed YAML, or integration with suggest_docs.py. |
||
|
|
||
| from linkage import ( | ||
| extract_changed_paths, | ||
| find_declared_docs, | ||
| parse_doc_frontmatter, | ||
| ) | ||
|
|
||
|
|
||
| class TestParseDocFrontmatter: | ||
| def test_md_yaml_frontmatter(self, tmp_path): | ||
| doc = tmp_path / "guide.md" | ||
| doc.write_text( | ||
| "---\ncode-to-docs:\n covers:\n - src/cli.py\n - src/config.py\n---\n# Guide\n", | ||
| encoding="utf-8", | ||
| ) | ||
| result = parse_doc_frontmatter(str(doc)) | ||
| assert result == {"covers": ["src/cli.py", "src/config.py"]} | ||
|
|
||
| def test_md_no_frontmatter(self, tmp_path): | ||
| doc = tmp_path / "guide.md" | ||
| doc.write_text("# Guide\n\nNo front-matter here.\n", encoding="utf-8") | ||
| assert parse_doc_frontmatter(str(doc)) == {} | ||
|
|
||
| def test_rst_directive(self, tmp_path): | ||
| doc = tmp_path / "guide.rst" | ||
| doc.write_text( | ||
| ".. code-to-docs:: covers: src/cli.py, src/config.py\n\nGuide\n=====\n", | ||
| encoding="utf-8", | ||
| ) | ||
| result = parse_doc_frontmatter(str(doc)) | ||
| assert result == {"covers": ["src/cli.py", "src/config.py"]} | ||
|
|
||
| def test_adoc_comment(self, tmp_path): | ||
| doc = tmp_path / "guide.adoc" | ||
| doc.write_text( | ||
| "// code-to-docs: covers: src/cli.py, src/config.py\n= Guide\n", | ||
| encoding="utf-8", | ||
| ) | ||
| result = parse_doc_frontmatter(str(doc)) | ||
| assert result == {"covers": ["src/cli.py", "src/config.py"]} | ||
|
|
||
| def test_missing_file(self): | ||
| assert parse_doc_frontmatter("/nonexistent/file.md") == {} | ||
|
|
||
| def test_no_covers_key(self, tmp_path): | ||
| doc = tmp_path / "guide.md" | ||
| doc.write_text("---\ntitle: Guide\n---\n# Guide\n", encoding="utf-8") | ||
| assert parse_doc_frontmatter(str(doc)) == {} | ||
|
|
||
|
|
||
| class TestExtractChangedPaths: | ||
| def test_extracts_paths(self): | ||
| diff = "diff --git a/src/cli.py b/src/cli.py\n+new\ndiff --git a/src/config.py b/src/config.py\n+new\n" | ||
| assert extract_changed_paths(diff) == {"src/cli.py", "src/config.py"} | ||
|
|
||
| def test_empty_diff(self): | ||
| assert extract_changed_paths("") == set() | ||
|
|
||
|
|
||
| class TestFindDeclaredDocs: | ||
| def test_finds_matching_doc(self, tmp_path): | ||
| doc = tmp_path / "guide.md" | ||
| doc.write_text( | ||
| "---\ncode-to-docs:\n covers:\n - src/cli.py\n---\n# Guide\n", | ||
| encoding="utf-8", | ||
| ) | ||
| diff = "diff --git a/src/cli.py b/src/cli.py\n+new\n" | ||
| result = find_declared_docs(diff, str(tmp_path)) | ||
| assert len(result) == 1 | ||
| assert result[0][1] == "declared" | ||
|
|
||
| def test_no_match(self, tmp_path): | ||
| doc = tmp_path / "guide.md" | ||
| doc.write_text( | ||
| "---\ncode-to-docs:\n covers:\n - src/other.py\n---\n# Guide\n", | ||
| encoding="utf-8", | ||
| ) | ||
| diff = "diff --git a/src/cli.py b/src/cli.py\n+new\n" | ||
| assert find_declared_docs(diff, str(tmp_path)) == [] | ||
|
|
||
| def test_skips_undeclared_docs(self, tmp_path): | ||
| doc = tmp_path / "guide.md" | ||
| doc.write_text("# Guide\n\nNo declaration.\n", encoding="utf-8") | ||
| diff = "diff --git a/src/cli.py b/src/cli.py\n+new\n" | ||
| assert find_declared_docs(diff, str(tmp_path)) == [] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] module-docstring
Single-line module docstring where most source modules use multi-line format with an elaborating paragraph.