docs: update the README actions/checkout pin example to v7.0.0 (#53)#63
docs: update the README actions/checkout pin example to v7.0.0 (#53)#63d-morrison wants to merge 1 commit into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| - name: Comment with AI summary | ||
| # Skip when the model returns nothing, so we never post an empty comment. | ||
| if: steps.inference.outputs.response != '' |
There was a problem hiding this comment.
The != '' check catches a truly empty string but won't guard against a whitespace-only response (e.g. " \n"), which would still post a blank-looking comment. GitHub Actions expressions don't support a trim() function, so the only clean fix would be to route through an intermediate shell step that trims and re-exports the value. Minor, but worth noting if blank comments have been observed.
There was a problem hiding this comment.
Keeping != '' as-is for parity with qwt's standalone summary.yml (the explicit goal of #43 — so qwt can migrate without behavior drift). A whitespace-only response from the summarizer is a negligible risk, and the only clean fix (an intermediate shell step that trims and re-exports) is more machinery than this edge case warrants. Happy to add the trim step if you'd prefer the extra robustness over parity.
Generated by Claude Code
… limit From the automated review of PR #63 (all non-blocking): - check-phi tests: remove the unused `import sys`. - check-phi tests: tearDown now removes the per-test tmpdir (shutil.rmtree), matching stdlib convention instead of leaking it. - summary.yml: document that the ===ISSUE=== markers are a best-effort guard (a crafted body can still spoof the closing marker; a full fix needs structured ai-inference inputs, which the action doesn't expose). Kept the `response != ''` guard as-is for parity with qwt's standalone workflow (#43); a whitespace-only AI summary is a negligible risk that doesn't warrant an extra trim step. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GKLYLEuXjYyvnmfXdwdpdG
There was a problem hiding this comment.
Pull request overview
This PR closes several small backlog follow-ups by adding automated unit coverage for the check-phi detectors, hardening the summary reusable workflow against prompt-injection/blank outputs, and applying a couple of documentation/comment cleanups to keep guidance aligned with current behavior.
Changes:
- Add a stdlib
unittestsuite forcheck-phidetectors (including a git-backed diff-scope fixture) and run it in CI via a new_selftest.ymljob. - Harden
.github/workflows/summary.ymlby wrapping issue content in explicit untrusted-data markers and skipping comment posting on empty model output. - Minor doc/comment updates (README checkout pin example; condensed
WORKFLOW_TOKENcomment; changelog entries).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
README.md |
Updates the example actions/checkout SHA pin comment to match current usage. |
check-phi/tests/test_check_phi.py |
Adds unit tests for all check-phi detectors plus diff-scope coverage for csv_phi_header. |
CHANGELOG.md |
Records the new test harness and summary hardening in the unreleased changelog. |
.github/workflows/summary.yml |
Adds prompt-injection hardening markers and prevents posting blank comments. |
.github/workflows/claude.yml |
Condenses WORKFLOW_TOKEN inline documentation to a short pointer. |
.github/workflows/_selftest.yml |
Excludes PHI test fixtures from whole-tree scans and adds a dedicated phi-tests job to run the new unit suite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # The ===ISSUE=== markers are a best-effort prompt-injection guard: a | ||
| # crafted body could still spoof the closing marker. A stronger fix needs | ||
| # structured (non-interpolated) inputs, which actions/ai-inference doesn't | ||
| # expose. Worst case here is a manipulated summary comment — this job only | ||
| # holds issues:write + models:read. |
| _spec = importlib.util.spec_from_file_location("check_phi", _MODULE_PATH) | ||
| check_phi = importlib.util.module_from_spec(_spec) | ||
| _spec.loader.exec_module(check_phi) |
This comment has been minimized.
This comment has been minimized.
| (path, lineno) | ||
| for path, lineno, text in rows | ||
| for _ in check_phi._detect_csv_phi_header(path, lineno, text) | ||
| ] |
There was a problem hiding this comment.
The inner comprehension for _ in check_phi._detect_csv_phi_header(...) iterates once per hit per column, not once per row. If a CSV row has two PHI-suggestive columns (e.g. "ssn,mrn\n"), _csv_hits returns [("patients.csv", 1), ("patients.csv", 1)] — a duplicated tuple per column hit. The current tests only exercise single-column hits so they pass either way, but a future test asserting == [("patients.csv", 1)] on a two-column fixture would fail with a confusing duplicate.
If "at least one hit on this row" is the intended contract, deduplicate with dict.fromkeys:
| ] | |
| def _csv_hits(self, base_ref): | |
| rows = check_phi._scan_lines_diff(base_ref, []) | |
| self.assertIsNotNone(rows, "diff scan should succeed in a real repo") | |
| return list(dict.fromkeys( | |
| (path, lineno) | |
| for path, lineno, text in rows | |
| if check_phi._detect_csv_phi_header(path, lineno, text) | |
| )) |
The diff-scan helper iterated once per column hit, so a row with multiple PHI-suggestive headers would return duplicate (path, lineno) tuples. Collapse to one entry per row with >=1 hit via dict.fromkeys, matching the intended "rows where a PHI header was detected" contract (review nit on #63). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GKLYLEuXjYyvnmfXdwdpdG
The "Pinning third-party actions" example still showed `# v4.3.1` while every workflow in the repo now pins `actions/checkout` at `# v7.0.0`; update the doc example to match so it reads like the actual code. (#60/#43/#62/#61 — the rest of this PR's original scope — landed on main independently via #69/#66/#64/#65, so this branch is reduced to just the #53 doc fix.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GKLYLEuXjYyvnmfXdwdpdG
d-morrison
force-pushed
the
claude/focused-lovelace-70fd6s
branch
from
7eace3c to
41c9457
Compare
|
Claude finished @d-morrison's task in 1m 12s —— View job Code Review — PR #63
Overall verdict: LGTMThe PR currently contains a single change: updating the illustrative Hallucination check — clean SHA
No mismatch between the example and the actual usage. The No issues found. Nothing to block merge.
|
d-morrison
changed the title
Reduced to a single doc fix.
This PR originally bundled #60 / #43 / #62 / #53, but while it was open those landed on
mainindependently via #69 (check-phi tests), #66 (summary hardening), #64 (WORKFLOW_TOKEN comment), and #65 (quarto example). After rebasing ontomain, the only change left thatmaindoesn't already have is the #53 doc nit:README.md's "Pinning third-party actions" example still showedactions/checkout@… # v4.3.1, while every workflow + composite in the repo now pins# v7.0.0. Updated the example to match so the doc reads like the actual code.One line; no functional change.