-
Notifications
You must be signed in to change notification settings - Fork 7
feat: docs build verification and code sample checking #70
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,87 @@ | ||
| """Pre-commit verification for generated documentation. | ||
|
|
||
| Runs an optional docs build command and syntax-checks fenced code samples | ||
| before the generated content is committed or pushed. | ||
| """ | ||
|
|
||
| import json | ||
| import re | ||
| import shutil | ||
| import subprocess | ||
| import tempfile | ||
|
|
||
| from security_utils import sanitize_output | ||
|
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 Other modules in src/ use grouping comments for internal imports (e.g., # Import security utilities). This module omits the comment. |
||
|
|
||
|
|
||
| def run_docs_build(build_command, docs_root, timeout=120): | ||
| """Run a docs build command in a temp copy of the docs tree. | ||
|
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 style The run_docs_build docstring uses a terse Returns (success, error_output) format rather than the Returns: block style used elsewhere. |
||
|
|
||
| Returns (success, error_output). | ||
| """ | ||
| if not build_command: | ||
| return True, "" | ||
|
|
||
|
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] secrets handling shutil.copytree() copies the entire docs_root into a temp directory. If docs_root contains sensitive files, they are accessible during the build window. |
||
| tmpdir = tempfile.mkdtemp(prefix="code-to-docs-build-") | ||
| try: | ||
| shutil.copytree(docs_root, tmpdir, dirs_exist_ok=True) | ||
| result = subprocess.run( | ||
|
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] edge case When subprocess.run raises TimeoutExpired with shell=True, the shell process is killed but its child processes may continue running as orphans, consuming CPU/memory on the runner even after the timeout is reported. Suggested fix: Use start_new_session=True with os.killpg to ensure the entire process tree is terminated on timeout. |
||
| build_command, | ||
|
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] secrets handling The subprocess.run() call with shell=True inherits the full process environment, including GH_TOKEN, MODEL_API_KEY, JIRA_API_TOKEN, and GOOGLE_SA_KEY. While the build_command is set by the repo owner (trusted input), the command runs against a docs tree that may contain LLM-generated content from untrusted PR diffs. A sanitized environment would reduce the blast radius. Suggested fix: Pass a sanitized environment to subprocess.run() that strips sensitive variables: env = {k: v for k, v in os.environ.items() if k not in ('GH_TOKEN', 'MODEL_API_KEY', 'JIRA_API_TOKEN', 'GOOGLE_SA_KEY')} 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] command injection subprocess.run() is called with shell=True and the build_command string is passed directly. While docs-build-command comes from workflow YAML (trusted input), using shell=True means the command is interpreted by /bin/sh. The existing run_command_safe() in security_utils.py does not use shell=True. Suggested fix: Add a comment documenting the trust boundary. Consider validating against an allowlist of known build tools or using shlex.split() and shell=False. |
||
| shell=True, | ||
| cwd=tmpdir, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=timeout, | ||
| ) | ||
| if result.returncode != 0: | ||
| error = (result.stderr or result.stdout or "Build failed").strip() | ||
|
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 Error output is truncated to 2000 characters before sanitize_output is called. If a sensitive token straddles the boundary, it could avoid scrubbing. Suggested fix: Call sanitize_output before truncating: sanitize_output(error)[:2000]. |
||
| return False, sanitize_output(error[:2000]) | ||
| return True, "" | ||
| except subprocess.TimeoutExpired: | ||
| return False, f"Build timed out after {timeout}s" | ||
| except Exception as e: | ||
| return False, sanitize_output(str(e)) | ||
| finally: | ||
| shutil.rmtree(tmpdir, ignore_errors=True) | ||
|
|
||
|
|
||
| _FENCE_PATTERN = re.compile( | ||
| r"^```(\w+)\s*\n(.*?)^```\s*$", | ||
| re.MULTILINE | re.DOTALL, | ||
|
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 _FENCE_PATTERN regex only matches backtick-delimited code fences. The codebase recognizes both backtick and tilde fences (comments.py handles both), and tilde fences are valid Markdown. Any fenced code block using ~~~python or ~~~json will be silently skipped by check_code_samples, producing false negatives. Suggested fix: Update _FENCE_PATTERN to also match tilde fences: r'^(?: |
||
| ) | ||
|
|
||
|
|
||
| def check_code_samples(content, file_path=""): | ||
| """Syntax-check fenced code blocks in generated documentation. | ||
|
|
||
|
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 style Both check_code_samples and run_docs_build omit Args: blocks. Many multi-parameter functions in the codebase include them. |
||
| Returns a list of (line_number, language, error) tuples. | ||
| Does not execute any code. | ||
| """ | ||
| issues = [] | ||
| for match in _FENCE_PATTERN.finditer(content): | ||
| lang = match.group(1).lower() | ||
| code = match.group(2) | ||
| line_num = content[: match.start()].count("\n") + 1 | ||
|
|
||
| if lang == "python": | ||
| try: | ||
| compile(code, f"{file_path}:line{line_num}", "exec") | ||
| except SyntaxError as e: | ||
| issues.append((line_num, lang, str(e))) | ||
|
|
||
| elif lang == "json": | ||
| try: | ||
| json.loads(code) | ||
| except json.JSONDecodeError as e: | ||
| issues.append((line_num, lang, str(e))) | ||
|
|
||
| elif lang == "yaml": | ||
| try: | ||
| import yaml | ||
|
|
||
| yaml.safe_load(code) | ||
| except ImportError: | ||
| pass | ||
| except yaml.YAMLError as e: | ||
| issues.append((line_num, lang, str(e))) | ||
|
|
||
| return issues | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| """Tests for build_check.py -- docs build verification and code sample checking.""" | ||
|
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 adequacy The test suite does not cover tilde-fenced code blocks, YAML validation, empty code blocks, or blocks without a language specifier. |
||
|
|
||
| from build_check import check_code_samples, run_docs_build | ||
|
|
||
|
|
||
| class TestRunDocsBuild: | ||
| def test_no_command_succeeds(self, tmp_path): | ||
| ok, err = run_docs_build("", str(tmp_path)) | ||
| assert ok is True | ||
| assert err == "" | ||
|
|
||
| def test_successful_build(self, tmp_path): | ||
| ok, err = run_docs_build("true", str(tmp_path)) | ||
| assert ok is True | ||
|
|
||
| def test_failed_build(self, tmp_path): | ||
| ok, err = run_docs_build("echo 'broken directive' && exit 1", str(tmp_path)) | ||
| assert ok is False | ||
| assert "broken directive" in err | ||
|
|
||
| def test_timeout(self, tmp_path): | ||
| ok, err = run_docs_build("sleep 10", str(tmp_path), timeout=1) | ||
| assert ok is False | ||
| assert "timed out" in err.lower() | ||
|
|
||
|
|
||
| class TestCheckCodeSamples: | ||
| def test_valid_python(self): | ||
| content = '# Guide\n\n```python\nprint("hello")\n```\n' | ||
| assert check_code_samples(content) == [] | ||
|
|
||
| def test_invalid_python(self): | ||
| content = "# Guide\n\n```python\ndef broken(\n```\n" | ||
| issues = check_code_samples(content) | ||
| assert len(issues) == 1 | ||
| assert issues[0][1] == "python" | ||
|
|
||
| def test_valid_json(self): | ||
| content = '# Config\n\n```json\n{"key": "value"}\n```\n' | ||
| assert check_code_samples(content) == [] | ||
|
|
||
| def test_invalid_json(self): | ||
| content = "# Config\n\n```json\n{broken}\n```\n" | ||
| issues = check_code_samples(content) | ||
| assert len(issues) == 1 | ||
| assert issues[0][1] == "json" | ||
|
|
||
| def test_unknown_language_skipped(self): | ||
| content = '```ruby\nputs "hello"\n```\n' | ||
| assert check_code_samples(content) == [] | ||
|
|
||
| def test_multiple_blocks(self): | ||
| content = '```python\nx = 1\n```\n\n```json\n{"a": 1}\n```\n' | ||
| assert check_code_samples(content) == [] | ||
|
|
||
| def test_line_number_tracking(self): | ||
| content = "Line 1\nLine 2\nLine 3\n\n```python\ndef broken(\n```\n" | ||
| issues = check_code_samples(content) | ||
| assert issues[0][0] == 5 | ||
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.
[high] dead code / consumer completeness
The new module build_check.py is never imported or called by any other module in the codebase. The DOCS_BUILD_COMMAND environment variable is plumbed through action.yml to the container, but suggest_docs.py (the main orchestrator) does not import run_docs_build or check_code_samples. Setting docs-build-command in a workflow has no effect -- the build check will never run. The action.yml input creates a false contract.
Suggested fix: Import and call run_docs_build and check_code_samples at the appropriate point in suggest_docs.py. Read DOCS_BUILD_COMMAND from os.environ and invoke run_docs_build(build_command, docs_root). Handle failure by feeding the error output back into regeneration or aborting the push.