-
Notifications
You must be signed in to change notification settings - Fork 7
feat: detect-only mode for docs drift checking #68
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,51 @@ | ||
| """Detect-only mode: identify docs affected by a diff without generating anything.""" | ||
|
|
||
| import re | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
|
|
||
| def extract_changed_doc_paths(diff_text): | ||
| """Extract documentation file paths that were modified in the diff.""" | ||
| doc_extensions = {".md", ".rst", ".adoc"} | ||
| paths = set() | ||
| for match in re.finditer(r"^diff --git a/(.+?) b/", diff_text, re.MULTILINE): | ||
| path = match.group(1) | ||
| if Path(path).suffix in doc_extensions: | ||
| paths.add(path) | ||
| return paths | ||
|
|
||
|
|
||
| def run_detect_only(diff, relevant_files, changed_docs): | ||
| """Compare affected docs against docs actually changed in the PR. | ||
|
|
||
|
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] logic-error run_detect_only compares relevant_files (paths relative to docs root) against changed_docs (repo-relative paths from extract_changed_doc_paths). In same-repo mode with DOCS_SUBFOLDER, these path namespaces differ. The set subtraction will never find matches, so every affected file will be reported as untouched even when updated in the PR. Suggested fix: Normalize paths before comparison. Strip the DOCS_SUBFOLDER prefix from changed_docs paths or prepend it to relevant_files. |
||
| Returns (affected_but_untouched, summary_lines). | ||
| """ | ||
| affected_set = set(relevant_files) if relevant_files else set() | ||
| untouched = affected_set - changed_docs | ||
|
|
||
| lines = [] | ||
| if untouched: | ||
| lines.append(f"Found {len(untouched)} doc file(s) that may need updates:") | ||
| for f in sorted(untouched): | ||
| lines.append(f" - {f}") | ||
| lines.append("") | ||
| lines.append( | ||
| "Comment [review-docs] on the PR to review suggested changes, " | ||
| "or [update-docs] to generate updates directly." | ||
| ) | ||
| else: | ||
|
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 When relevant_files is empty and changed_docs is also empty, run_detect_only reports 'All affected documentation files are already updated in this PR.' This is misleading -- no docs were identified as affected. Suggested fix: Add a check: if affected_set is empty, output a distinct message like 'No documentation files were identified as affected by this change.' |
||
| lines.append("All affected documentation files are already updated in this PR.") | ||
|
|
||
| return untouched, lines | ||
|
|
||
|
|
||
| def exit_with_severity(untouched, severity): | ||
| """Exit with the appropriate code based on severity setting.""" | ||
| if not untouched: | ||
| return | ||
|
|
||
| severity = (severity or "warn").lower() | ||
| if severity == "error": | ||
| print(f"Error: {len(untouched)} doc file(s) may need updates but were not changed.") | ||
| sys.exit(1) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -258,6 +258,31 @@ def main(): | |
| print(f"Index build complete: {result['status']}") | ||
| return | ||
|
|
||
| # Handle detect-only mode (runs on pull_request events, not comments) | ||
|
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] scope-coherence The detect-only mode block sits between the --build-index handler and COMMENT_BODY parsing. The existing comment partially explains this but could be more explicit about why the position matters. |
||
| mode = os.environ.get("MODE", "comment") | ||
|
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] import-style The detect-only mode block uses a lazy inline import. Every other module import in suggest_docs.py is at the top of the file. No other conditional/deferred import exists in the file, and detect.py has no heavy dependencies justifying deferral. Suggested fix: Move the import to the top-level imports section alongside the other from-module-import statements. |
||
| if mode == "detect-only": | ||
| from detect import exit_with_severity, extract_changed_doc_paths, run_detect_only | ||
|
|
||
| print("Mode: detect-only") | ||
| if not setup_docs_environment(): | ||
| print("Failed to set up docs environment") | ||
| return | ||
| diff = get_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. [high] logic-error In detect-only mode, setup_docs_environment() is called before get_diff(). setup_docs_environment() changes the working directory to the docs subfolder or a cloned docs repo. get_diff() then runs git diff in that new CWD. In separate-repo mode, this produces the docs repo's diff instead of the source code diff. In the comment-based flow, get_diff() correctly runs before setup_docs_environment(). Suggested fix: Move diff = get_diff() and the empty-diff guard before the setup_docs_environment() call in the detect-only block, matching the ordering used by the comment-based flow. |
||
| if not diff: | ||
| print("No diff found.") | ||
| return | ||
| changed_docs = extract_changed_doc_paths(diff) | ||
| relevant_files = find_relevant_files_optimized(diff) | ||
| if relevant_files is None: | ||
| file_previews = get_file_content_or_summaries() | ||
| relevant_files = ask_ai_for_relevant_files(diff, file_previews) if file_previews else [] | ||
| untouched, summary = run_detect_only(diff, relevant_files, changed_docs) | ||
| for line in summary: | ||
| print(line) | ||
| severity = os.environ.get("DOCS_DRIFT_SEVERITY", "warn") | ||
| exit_with_severity(untouched, severity) | ||
| return | ||
|
|
||
| # Detect which command was used | ||
| comment_body = os.environ.get("COMMENT_BODY", "") | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| """Tests for detect-only mode.""" | ||
|
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-docstring-convention The test docstring references the feature rather than the module. Other test files follow the convention of referencing the source module filename. |
||
|
|
||
| from detect import extract_changed_doc_paths, run_detect_only | ||
|
|
||
|
|
||
| class TestExtractChangedDocPaths: | ||
| def test_finds_doc_files(self): | ||
| diff = ( | ||
| "diff --git a/src/main.py b/src/main.py\n" | ||
| "+added\n" | ||
| "diff --git a/docs/guide.md b/docs/guide.md\n" | ||
| "+updated\n" | ||
| "diff --git a/docs/api.rst b/docs/api.rst\n" | ||
| "+updated\n" | ||
| ) | ||
| paths = extract_changed_doc_paths(diff) | ||
| assert paths == {"docs/guide.md", "docs/api.rst"} | ||
|
|
||
| def test_ignores_non_doc_files(self): | ||
| diff = "diff --git a/src/main.py b/src/main.py\n+added\n" | ||
| assert extract_changed_doc_paths(diff) == set() | ||
|
|
||
| def test_empty_diff(self): | ||
| assert extract_changed_doc_paths("") == set() | ||
|
|
||
|
|
||
| class TestRunDetectOnly: | ||
| def test_reports_untouched_files(self): | ||
| untouched, lines = run_detect_only( | ||
| diff="", | ||
| relevant_files=["docs/guide.md", "docs/api.md"], | ||
| changed_docs={"docs/guide.md"}, | ||
| ) | ||
| assert untouched == {"docs/api.md"} | ||
| assert any("docs/api.md" in line for line in lines) | ||
|
|
||
| def test_all_updated(self): | ||
| untouched, lines = run_detect_only( | ||
| diff="", | ||
| relevant_files=["docs/guide.md"], | ||
| changed_docs={"docs/guide.md"}, | ||
| ) | ||
| assert untouched == set() | ||
| assert any("already updated" in line for line in lines) | ||
|
|
||
| def test_no_relevant_files(self): | ||
| untouched, lines = run_detect_only( | ||
| diff="", | ||
| relevant_files=[], | ||
| changed_docs=set(), | ||
| ) | ||
| assert untouched == set() | ||
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-style
The module docstring is a single line. Most other source modules in src/ use a multi-line docstring with summary, blank line, and elaboration.