chore: add HOL plugin scanner CI - #346
Conversation
- Run the pinned static scanner with read-only permissions and strict gates. - Preserve failed-scan reports and document local reproduction. Refs #344
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR successfully implements the requested HOL Plugin Scanner CI workflow with appropriate security configurations and comprehensive documentation. The implementation follows security best practices with SHA-pinned actions, read-only permissions, disabled credential persistence, and strict quality gates. The documentation provides clear guidance for both CI usage and local reproduction. No blocking defects were identified.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Reviewed head: 8a7d6da
-
[P2] Publish the job summary explicitly for JSON scans —
.github/workflows/hol-plugin-scanner.yml:30–31With
format: json, this configuration does not produce the job summary promised in README anddocs/development.md. The pinned action overridesGITHUB_STEP_SUMMARYwith an empty value in this workflow, disabling the runner's summary writer; its separate publishing step only handles Markdown and also skips after scanner failure. The run for this head confirms the empty variable and skipped publisher after the 78/100 gate failure, while the JSON artifact uploads successfully. Maintainers therefore have to download the artifact to inspect findings instead of seeing them in the promised job summary. Add a follow-up step that renders the saved JSON intoGITHUB_STEP_SUMMARYwith a failure-tolerant condition like the artifact step's, preserving both the failed gate and JSON retention.
Render saved JSON report metadata in the job summary after scanner gate failures. - Cover metadata rendering and escaping with a workflow regression check. - Keep strict scanner gates and report retention unchanged. Refs #344
|
/q review |
There was a problem hiding this comment.
This PR adds a well-structured HOL Plugin Scanner CI workflow with SHA-pinned actions, read-only permissions, proper artifact retention, and comprehensive documentation. The security posture is strong with persist-credentials: false and minimal permissions.
One critical issue requires attention: the scanner summary script lacks error handling for malformed reports, which could crash the CI step and hide the actual scanner results. The suggested fix adds proper exception handling to ensure the workflow remains informative even when the scanner output is unexpected.
Once the error handling is addressed, the implementation will be production-ready and provide reliable security scanning for the agent-plugin package.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| with open(sys.argv[1], encoding="utf-8") as report_file: | ||
| print(render_summary(json.load(report_file))) |
There was a problem hiding this comment.
🛑 Crash Risk: Missing error handling for required keys causes runtime crash when report structure is unexpected. The script directly accesses nested dictionary keys without validation, which will raise KeyError if the scanner produces a malformed report or changes its output schema.
| with open(sys.argv[1], encoding="utf-8") as report_file: | |
| print(render_summary(json.load(report_file))) | |
| if __name__ == "__main__": | |
| try: | |
| with open(sys.argv[1], encoding="utf-8") as report_file: | |
| print(render_summary(json.load(report_file))) | |
| except (KeyError, TypeError) as e: | |
| print(f"Error: Invalid report structure - {e}", file=sys.stderr) | |
| sys.exit(1) | |
| except (IndexError, FileNotFoundError) as e: | |
| print(f"Error: {e}", file=sys.stderr) | |
| sys.exit(1) |
There was a problem hiding this comment.
Addressed in 8d0823c9feea2a09e0df4b861580562a053fdc3b: unreadable JSON and invalid report structures now produce an error summary and exit 1. Regression checks cover invalid JSON, missing keys, null nested data, and invalid finding entries. make test, make build, and hosted regression checks pass; artifact retention remains independent of publisher failure.
Write a concise job-summary error for unreadable or malformed reports while preserving a failing exit status. - Cover malformed JSON and invalid nested report structures. - Document the error-summary behavior. Refs #344
|
/q review |
There was a problem hiding this comment.
Review Summary
This PR adds a HOL plugin scanner CI workflow with a Python-based summary renderer. The implementation is well-structured with comprehensive test coverage and appropriate security practices (read-only permissions, SHA-pinned actions, no persisted credentials).
Critical Finding
One logic error blocks merge: the workflow uses inconsistent environment variable syntax for the scanner report path (${{ runner.temp }} vs $RUNNER_TEMP), which could cause file path mismatches and publish step failures. A code suggestion has been provided to standardize the syntax.
Additional Observations
The Python scripts demonstrate solid security practices with HTML escaping for untrusted report content and safe error handling. The test coverage includes both happy path and error cases. Documentation updates are thorough and align with the implementation.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| output: ${{ runner.temp }}/hol-plugin-scanner-report.json | ||
| - name: Publish scanner summary | ||
| if: ${{ !cancelled() && steps.scan.outputs.report_path != '' }} | ||
| run: python3 .github/scripts/scanner_summary.py "$RUNNER_TEMP/hol-plugin-scanner-report.json" >> "$GITHUB_STEP_SUMMARY" |
There was a problem hiding this comment.
🛑 Logic Error: Inconsistent environment variable reference between lines 33 and 36. Line 33 uses ${{ runner.temp }} (GitHub Actions expression syntax) while line 36 uses $RUNNER_TEMP (shell environment variable syntax). Both reference the same location but must use identical syntax for correct file path matching.1
| output: ${{ runner.temp }}/hol-plugin-scanner-report.json | |
| - name: Publish scanner summary | |
| if: ${{ !cancelled() && steps.scan.outputs.report_path != '' }} | |
| run: python3 .github/scripts/scanner_summary.py "$RUNNER_TEMP/hol-plugin-scanner-report.json" >> "$GITHUB_STEP_SUMMARY" | |
| output: ${{ runner.temp }}/hol-plugin-scanner-report.json | |
| - name: Publish scanner summary | |
| if: ${{ !cancelled() && steps.scan.outputs.report_path != '' }} | |
| run: python3 .github/scripts/scanner_summary.py "${{ runner.temp }}/hol-plugin-scanner-report.json" >> "$GITHUB_STEP_SUMMARY" |
Footnotes
-
The scan step's
outputparameter defines where the JSON report is written. The publish step must reference the exact same path to read that report. Using${{ runner.temp }}in output and$RUNNER_TEMPin the shell command could cause path mismatches if GitHub Actions resolves these differently, leading to "file not found" failures in the publish step. ↩
There was a problem hiding this comment.
This is not a path mismatch in this workflow. GitHub documents both RUNNER_TEMP and runner.temp as the runner's job temporary directory. These steps run in the same Ubuntu job without a container boundary or an override. The completed run for this head confirms the publisher read the report successfully after the scanner gate failed, and artifact retention succeeded. Keeping the quoted shell variable; no corrective change is needed. Leaving the thread unresolved for reviewer visibility.
|
Addressed the missing-summary review in Validation: Follow-up The PR remains draft for the existing scanner findings. No review thread was resolved or dismissed. |
2 participants
Summary
Add the source scanner CI requested in issue #344. A SHA-pinned HOL action scans PRs to
mainand pushes tomainwith read-only permissions, a minimum score of 80, and failure on high/critical findings. Save the full JSON report for 14 days even after a failed gate.A separate post-scan step publishes the saved report's score, analyzer status, and finding rules/locations to the job summary. This addresses the review finding that the pinned action's JSON mode does not publish its own summary. The renderer uses Python's standard library, escapes report metadata, and leaves finding descriptions in the JSON artifact. Unreadable or malformed reports produce an error summary and a failing exit status. README and development guidance document the workflow and local reproduction.
The PR remains draft because the existing scanner findings fail its gate. PR #345 separately pins existing mutable action references. No gate is weakened or bypassed.
Related Issues
Refs #344. Implements the source scanner CI requested in upstream PR #259. Finding follow-up and the existing listing's rebase/re-review remain outstanding.
Validation
make testandmake buildpassed for the review fix and both binaries. Sandbox-only cache/localhost restrictions were resolved by rerunning outside the sandbox.python3 -m unittest discover -s .github/scripts -p 'test_*.py'passed; the workflow runs this regression check before scanning.git diff --checkpassed.8d0823c, standard CI passed. The hosted scanner run passed the renderer regression test, summary publisher, and JSON artifact upload after the scan gate failed. The downloaded report remains at 78/100, with the exact prior rule/severity/location counts (0 critical, 10 high, 16 medium, 2 low, 6 informational); both optional Cisco analyzers remain unavailable.8d0823c. Its earlier malformed-report finding is fixed and covered by regression checks. Its remaining path-syntax comment is not actionable: the documented runner temporary directory is shared by both steps, and hosted publishing succeeds. A reply with documentation and run evidence records the disposition; threads remain unresolved for reviewer visibility. CodeRabbit skips review while this PR is draft.Checklist
docs/branch-naming-harness.mddocs/documentation-harness.md)