feat: docs build verification and code sample checking - #70
Conversation
The pipeline can only produce plausible text. An optional docs-build-command input runs the build against the generated tree before opening a PR. On failure, reports the error. Also extracts fenced code blocks and syntax-checks python, json, and yaml using stdlib parsers (compile, json.loads, yaml.safe_load). Reports failures but does not execute anything.
|
🤖 Finished Review · ✅ Success · Started 5:51 AM UTC · Completed 6:07 AM UTC Commit: |
ReviewFindingsHigh
Medium
Low
Labels: PR adds a new feature module and modifies GitHub Action inputs Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| @@ -0,0 +1,87 @@ | |||
| """Pre-commit verification for generated documentation. | |||
There was a problem hiding this comment.
[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.
| try: | ||
| shutil.copytree(docs_root, tmpdir, dirs_exist_ok=True) | ||
| result = subprocess.run( | ||
| build_command, |
There was a problem hiding this comment.
[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')}
| try: | ||
| shutil.copytree(docs_root, tmpdir, dirs_exist_ok=True) | ||
| result = subprocess.run( | ||
| build_command, |
There was a problem hiding this comment.
[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.
|
|
||
| _FENCE_PATTERN = re.compile( | ||
| r"^```(\w+)\s*\n(.*?)^```\s*$", | ||
| re.MULTILINE | re.DOTALL, |
There was a problem hiding this comment.
[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'^(?:|~~~)(\w+)\s*\n(.*?)^(?:|~~~)\s*$'
| 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.
[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.
|
|
||
|
|
||
| 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.
[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.
|
|
||
| def check_code_samples(content, file_path=""): | ||
| """Syntax-check fenced code blocks in generated documentation. | ||
|
|
There was a problem hiding this comment.
[low] function docstring style
Both check_code_samples and run_docs_build omit Args: blocks. Many multi-parameter functions in the codebase include them.
| timeout=timeout, | ||
| ) | ||
| if result.returncode != 0: | ||
| error = (result.stderr or result.stdout or "Build failed").strip() |
There was a problem hiding this comment.
[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].
| @@ -0,0 +1,59 @@ | |||
| """Tests for build_check.py -- docs build verification and code sample checking.""" | |||
There was a problem hiding this comment.
[low] test adequacy
The test suite does not cover tilde-fenced code blocks, YAML validation, empty code blocks, or blocks without a language specifier.
| """ | ||
| if not build_command: | ||
| return True, "" | ||
|
|
There was a problem hiding this comment.
[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.
fullsend-ai-review
Bot
added
feature
python
1 participant
Summary
Adds pre-commit verification for generated documentation.
src/build_check.py): optionaldocs-build-commandinput runs the docs build in a temp copy before opening a PR. Hard timeout, sanitized output.python(compile),json(json.loads),yaml(yaml.safe_load). Reports failures without executing anything.Test plan
uv run pytest -vpasses (428 tests)