Fix TYPE-008 conformance checker permission expectations - #49324
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes TYPE-008 to recognize the least-privilege permission builders used by create_check_run.
Changes:
- Updates TYPE-008 permission checks.
- Removes unused permission factories.
- Removes their generated documentation entries.
Show a summary per file
| File | Description |
|---|---|
scripts/check-safe-outputs-conformance.sh |
Updates expected permission builders. |
pkg/workflow/permissions_factory.go |
Removes obsolete factories. |
pkg/workflow/README.md |
Removes stale factory references. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| # adds pull-requests:read when target is configured. | ||
| if [ -f "$handler_registry" ]; then | ||
| if ! grep -q "NewPermissionsContentsReadChecksWrite" "$handler_registry"; then | ||
| if ! grep -q "NewPermissionsChecksWrite" "$handler_registry"; then |
There was a problem hiding this comment.
Fixed in commit f2e80d3. The grep pattern now uses NewPermissionsChecksWrite() (with parentheses), which cannot match NewPermissionsChecksWritePRRead(), so the two profile checks are now independently verified.
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49324 does not have the implementation label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. No test files were added or modified in this PR. This is a production code fix (permissions_factory.go) and documentation update. Test Quality Sentinel analysis not applicable. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: Fix TYPE-008 conformance checker permission expectations
The changes are correct — the two deleted factory helpers (NewPermissionsContentsReadChecksWrite / NewPermissionsContentsReadChecksWritePRRead) were unused, the handler already uses the leaner NewPermissionsChecksWrite / NewPermissionsChecksWritePRRead profiles, and the conformance script and README are updated accordingly.
One pre-existing comment (line 1625) already flags that grep -q "NewPermissionsChecksWrite" is a substring match that would also match NewPermissionsChecksWritePRRead, so the base-profile assertion could pass even if the no-target branch were removed. That concern is worth addressing but is not a blocker for this fix. No new issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.6 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs — requesting changes on one correctness issue.
📋 Key Themes & Highlights
Key Issue
- Substring-match ambiguity (line 1625):
grep -q "NewPermissionsChecksWrite"matches bothNewPermissionsChecksWrite(base profile) andNewPermissionsChecksWritePRRead(extended profile), so the base-profile check can silently pass even when the no-target branch no longer calls the correct helper.
Positive Highlights
- ✅ Dead-code removal is clean and well-scoped — the two
ContentsRead*helpers were truly unused. - ✅ README regeneration is consistent with the code change.
- ✅ PR description is clear and maps directly to the spec section.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 13.9 AIC · ⊞ 7K
Comment /matt to run again
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Approve — no new blocking issues
Small, correct cleanup: conformance script now greps for the actual active permission builders (NewPermissionsChecksWrite, NewPermissionsChecksWritePRRead), and the deleted factory functions/doc entries were confirmed unused (no callers anywhere in the repo).
Review notes
- Verified
NewPermissionsContentsReadChecksWrite/...PRReadhave zero remaining references after removal. - Verified
safe_output_handlers.gocreate-check-run branch already calls the two functions the script now checks for. - A pre-existing review comment already flags the substring-match weakness in the new grep check (
NewPermissionsChecksWriteis a substring ofNewPermissionsChecksWritePRRead), so I am not duplicating that finding. - No new correctness, security, or maintainability issues found in this diff by either my independent pass or the sub-agent first-pass.
🔎 Code quality review by PR Code Quality Reviewer · auto · 34.1 AIC · ⊞ 7.8K
Comment /review to run again
PR TriageCategory: chore (conformance checker fix) Rationale: Small, targeted 3-file diff to align a conformance checker regex with active permission-builder names. CI is green across all checks, but PR is currently Labels applied:
|
|
@copilot run pr-finisher skill |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the
Please address any remaining review feedback, resolve any now-addressed review threads if possible, and rerun the relevant checks once ready.
|
…rens Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The grep pattern now uses |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Hey 👋 — thanks for this conformance checker fix! The PR correctly addresses the root cause of issue #49287 by updating TYPE-008 to grep for the right permission function names and cleaning up the unused dead-code functions. Here are a couple of things that would strengthen this:
If you would like to add a simple validation, here is a prompt you can assign to your coding agent:
|
|
🎉 This pull request is included in a new release. Release: |
The TYPE-008 conformance check was flagging
create_check_runas missing required permissions because it searched for stale helper names that includedcontents: read. The handler already uses the least-privilege profile required by the spec:checks: write, pluspull-requests: readonly whentargetis set.Conformance checker
Permission factory cleanup
contents: read + checks: writefactory helpers that no production code referenced.Generated reference docs
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.