Guard CGO/CJS workflow purity - #53680
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
There was a problem hiding this comment.
Pull request overview
Adds a shared purity guard for CGO/CJS test workflows.
Changes:
- Checks secret references and write permissions before caching checkouts.
- Reduces CJS artifact permissions.
- Removes CGO failure issue creation.
Show a summary per file
| File | Description |
|---|---|
scripts/check-cgo-cjs-workflow-purity.sh |
Implements purity validation. |
.github/workflows/cjs.yml |
Runs validation and reduces permissions. |
.github/workflows/cgo.yml |
Runs validation and removes notifications. |
Review details
Suppressed comments (1)
scripts/check-cgo-cjs-workflow-purity.sh:48
- The scanner only enters block mappings written as a bare
permissions:key and only recognizes an unquotedwritevalue. Semantically equivalent valid YAML such aspermissions: { issues: write }, anchored mappings,issues: "write", or quotedwrite-allbypasses the purity check. Parse the YAML structure and recursively validate workflow- and job-levelpermissionsvalues instead of matching formatting.
/^[[:space:]]*permissions:[[:space:]]*write-all([[:space:]]*(#.*)?)?$/ {
print FILENAME ":" FNR ":" $0
}
/^[[:space:]]*permissions:[[:space:]]*$/ {
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
| if [ "$#" -eq 0 ]; then | ||
| set -- .github/workflows/cgo.yml .github/workflows/cjs.yml | ||
| fi |
| # GITHUB_TOKEN and the repository's SCIENCE telemetry secret. | ||
| disallowed_secrets_file="$tmp_dir/disallowed-secrets.txt" | ||
| if ! perl -ne ' | ||
| while (/\$\{\{\s*secrets\.([A-Za-z_][A-Za-z0-9_]*)\b/g) { |
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes
The new purity guard is moving in the right direction, but its diagnostics are already lossy: when both workflows fail in one run, the script overwrites the first workflow's scratch output and only reports the last file's details.
Blocking theme
- The new checker reuses fixed temp filenames across the loop, so multi-file failures produce incomplete and misleading CI output.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 7.27 AIC · ⌖ 8.03 AIC · ⊞ 4.5K
Comment /review to run again
|
|
||
| # These workflows should stay pure test workflows: allow only the built-in | ||
| # GITHUB_TOKEN and the repository's SCIENCE telemetry secret. | ||
| disallowed_secrets_file="$tmp_dir/disallowed-secrets.txt" |
There was a problem hiding this comment.
This guard script writes its scratch files with fixed names inside a shared temporary directory, so the second workflow you scan overwrites the first workflow's diagnostics. If both cgo.yml and cjs.yml are dirty in one run, the output will only show the last file's findings, which makes debugging failures needlessly misleading.
💡 Keep per-workflow diagnostics separate.
Because disallowed-secrets.txt and write-permissions.txt are reused for every loop iteration, the script loses the earlier file's output as soon as the next workflow is processed. Give each workflow its own result files, for example:
safe_name=${workflow//\//_}
disallowed_secrets_file="$tmp_dir/${safe_name}-disallowed-secrets.txt"
write_permissions_file="$tmp_dir/${safe_name}-write-permissions.txt"That preserves all failing matches and keeps CI output trustworthy when multiple workflows break at once.
There was a problem hiding this comment.
The changes look correct and well-structured.
- New purity script (
check-cgo-cjs-workflow-purity.sh): Theperlregex for secrets and theawkindent-tracking for write permissions handle edge cases correctly (blank lines, comments,write-all, per-permissionwritevalues, multi-file resets viaFNR==1). notify-failureremoval: The deleted job hadissues: write— exactly the kind of write permission the new guard would have caught. Removing it is the right fix.actions: write→actions: readincjs.yml: Clean least-privilege improvement.- Trigger path for the new script added to both push/pull_request filters — correct.
No actionable issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21.2 AIC · ⌖ 8.82 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Ponytail review — over-engineering check only.
One finding on the new purity-check script: it uses a temp dir + trap just to hold two scan outputs it immediately checks with [ -s ... ]. Capturing to variables and checking [ -n ... ] drops the tmp_dir/trap plumbing entirely.
net: -10 lines possible.
Generated by ✂️ Ponytail Reviewer for #53680 · auto · 22.2 AIC · ⌖ 3.19 AIC · ⊞ 7.3K
Comment /ponytail to run again
| set -- .github/workflows/cgo.yml .github/workflows/cjs.yml | ||
| fi | ||
|
|
||
| tmp_dir="$(mktemp -d)" |
There was a problem hiding this comment.
L8-9: shrink: mktemp -d + trap only to stash two throwaway command outputs in files. Capture with disallowed=$(perl ... "$workflow") and writes=$(awk ... "$workflow"), then check [ -n "$disallowed" ] — drops tmp_dir/trap and both *_file vars, ~10 lines.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /improve-codebase-architecture — commenting (no blocking issues, but the purity script needs a test).
📋 Key Themes & Highlights
Key Themes
- Missing test coverage:
check-cgo-cjs-workflow-purity.shhas no automated test; a silent regression would disable the guard entirely. - Fragile YAML parsing: The
awkindent-tracking heuristic can miss inlinepermissions: {key: write}style and will need updating if the project adopts flow-style YAML. - Undocumented allowlist: The
SCIENCEsecret is permitted but has no inline explanation. - Deleted observability:
notify-failureis removed without a recorded rationale or replacement, creating an implicit gap.
Positive Highlights
- ✅ Excellent principle: purity is enforced at cache-save time, making the guard self-policing rather than advisory.
- ✅ Clean
set -euo pipefailandtrapcleanup — robust shell hygiene. - ✅ Reducing CJS
actionspermission fromwritetoreadis the right least-privilege move. - ✅ The trigger-path addition in
cjs.ymlensures the check re-runs whenever the script itself changes.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 34.3 AIC · ⌖ 10.2 AIC · ⊞ 7.8K
Comment /matt to run again
| fi | ||
| done | ||
|
|
||
| exit "$failed" |
There was a problem hiding this comment.
[/tdd] No regression test for the purity script — if the perl regex or awk pattern silently breaks, the guard becomes a no-op.
💡 Suggested approach
Add a test script (e.g., scripts/test-check-cgo-cjs-workflow-purity.sh) that:
- Creates a synthetic workflow YAML with a known-disallowed secret expression and asserts the script exits non-zero.
- Creates a clean workflow YAML and asserts the script exits zero.
- Wires the test into
make test-unitor a dedicatedmake test-scriptstarget.
Without this, a regex typo or tooling change can silently disable the guard.
@copilot please address this.
| } | ||
| ' "$workflow" >"$write_permissions_file"; then | ||
| echo "Failed to scan permissions in $workflow" | ||
| failed=1 |
There was a problem hiding this comment.
[/codebase-design] The awk permissions scanner uses indent-level heuristics to track permissions: blocks, which is fragile against valid YAML indentation variants and aliased blocks.
💡 Concrete risk
A workflow like:
jobs:
build:
permissions:
contents: write # inside a job — should be caught...is caught today, but inline permissions: {contents: write} on a single line is not matched by the current awk pattern (it only matches permissions: followed by a newline). Consider using a proper YAML parser (python3 -c "import yaml, sys; ..." or yq) for correctness and to reduce maintenance burden.
@copilot please address this.
| close ARGV if eof; | ||
| ' "$workflow" >"$disallowed_secrets_file"; then | ||
| echo "Failed to scan secrets expressions in $workflow" | ||
| failed=1 |
There was a problem hiding this comment.
[/codebase-design] The hardcoded allowlist (GITHUB_TOKEN, SCIENCE) lives only inside the script with no comment referencing where SCIENCE is documented or why it is permitted.
💡 Suggestion
Add a brief inline comment explaining the rationale, e.g.:
# GITHUB_TOKEN: standard built-in token — always safe
# SCIENCE: repo-level telemetry secret — write-only sink, no sensitive data
unless $1 eq "GITHUB_TOKEN" || $1 eq "SCIENCE";If the allowlist grows, consider a variable at the top of the script so it is visible to reviewers without reading the perl one-liner.
@copilot please address this.
| @@ -2508,216 +2510,6 @@ jobs: | |||
| path: conformance-output.txt | |||
| retention-days: 7 | |||
|
|
|||
There was a problem hiding this comment.
[/improve-codebase-architecture] The notify-failure job is deleted without a documented decision or replacement strategy — if failure alerting on main is still needed, the rationale for removing it is not captured.
💡 Suggestion
Either:
- Add a short comment in the PR body or a commit message noting that failure notification is intentionally dropped (e.g., superseded by another alerting system), or
- File a follow-up issue tracking the gap if alerting was valuable.
The deletion is a significant loss of observability. Without a record, a future contributor may re-add the exact pattern that introduced the issues: write permission.
@copilot please address this.
|
@copilot review failing tests and move to separate workflow if write token is needed |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in |
| import sys | ||
|
|
||
| workflow = sys.argv[1] | ||
| allowed = {"GITHUB_TOKEN", "SCIENCE"} |
There was a problem hiding this comment.
Addressed in 374a619: SCIENCE is no longer allowlisted, and the script test now asserts secrets.SCIENCE is rejected.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
CGO and CJS checkout-cache jobs should stay pure test paths with nothing sensitive to leak. These workflows should not reference secrets beyond
GITHUB_TOKEN/SCIENCE, and should not request write permissions.Shared purity check
scripts/check-cgo-cjs-workflow-purity.sh..github/workflows/cgo.ymland.github/workflows/cjs.ymlfor disallowedsecrets.*expressions.permissions: write-alland any mapped permission set towrite.Checkout-cache enforcement
checkout-cachejobs before saving the repository cache.Workflow permission cleanup
actions: writetoactions: read.issues: write.