[dead-code] chore: remove dead functions — 5 functions removed#41731
Conversation
Remove unreachable functions identified by deadcode analyzer: - permissionLevelRank (call_workflow_permissions.go) - findUncoveredWorkerPermissions (call_workflow_permissions.go) - extractCallWorkflowPermissions (call_workflow_permissions.go) - hasNonAllowedExpressionInRunContent (template_injection_validation.go) - hasExpressionInRunContent (template_injection_validation.go) Also remove their exclusive test functions and unused imports. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41731 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Removes unused internal helper functions and their now-obsolete unit tests in pkg/workflow, reducing maintenance overhead while keeping the currently-used implementations intact (e.g., scanRunContentExpressions and extractCallWorkflowPermissionImport).
Changes:
- Removed dead helper functions from template injection validation and call-workflow permissions logic.
- Removed tests that only exercised the deleted helpers and cleaned up related imports.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/template_injection_validation.go | Removes unused hasNonAllowedExpressionInRunContent / hasExpressionInRunContent wrappers. |
| pkg/workflow/template_injection_validation_test.go | Drops tests that only covered the removed wrappers and removes the unused regexp import. |
| pkg/workflow/call_workflow_permissions.go | Removes unused permission comparison / compatibility helper functions and drops the sort import. |
| pkg/workflow/call_workflow_permissions_test.go | Removes tests that depended on the deleted helpers (leaving remaining import/behavior coverage in place). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Low
| // TestExtractCallWorkflowPermissions_LockYMLPriorityOverYML tests that .lock.yml takes | ||
| // priority over .yml when both exist |
🧪 Test Quality Sentinel Report
Test files changed (deletions only):
📊 Metrics & Test Classification (0 tests analyzed)
Go: 0 new/modified ( The two test files were modified but only to remove tests for the 5 dead functions deleted from production ( Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out — two minor stale comments in adjacent files, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Stale cross-file doc comments (2 instances): deleting the wrapper functions left dangling references in
compiler.goandcall_workflow_secrets.gothat were not part of the diff - Functional correctness is unaffected — the build, vet, and tests all pass with the deletions
Positive Highlights
- ✅ Pure deletion PR with zero additions — minimal blast radius
- ✅ All wrapper functions were genuinely thin (e.g.
hasNonAllowedExpressionInRunContentwas a one-liner delegating toscanRunContentExpressions) - ✅ Deleted tests cleanly mirror deleted functions; no orphaned test helpers
- ✅ Full verification included (go build, go vet × 2, make fmt, go test) with passing CI run
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 65 AIC · ⌖ 9.77 AIC · ⊞ 6.5K
| return walkRunBlockLines(yamlContent, expressionRegex.MatchString) | ||
| } | ||
|
|
||
| // scanRunContentExpressions performs a single pass over run: blocks to detect both |
There was a problem hiding this comment.
[/zoom-out] The two deleted functions are still referenced by name in the validateTemplateInjection doc comment in compiler.go (~line 308), which now describes a stale two-step scan strategy. This comment was not updated as part of this PR.
💡 Suggested doc update in compiler.go
The Path B doc in compiler.go currently reads:
// 1. hasExpressionInRunContent(UnsafeContextPattern) — detects user-controlled
// contexts (github.event.*, steps.*.outputs.*, inputs.*) ...
// 2. hasNonAllowedExpressionInRunContent — detects compiler-regression cases ...
Update to reference the consolidating replacement:
// Uses scanRunContentExpressions to detect both user-controlled contexts
// (scan.hasUnsafe) and non-allowlisted expressions (scan.hasDisallowed) in a
// single pass, avoiding a duplicate run-block walk.
| return imported.permissions, nil | ||
| } | ||
|
|
||
| func extractCallWorkflowPermissionImport(workflowName, markdownPath string) (*callWorkflowPermissionImport, error) { |
There was a problem hiding this comment.
[/zoom-out] The now-deleted extractCallWorkflowPermissions is still referenced in a cross-reference comment in call_workflow_secrets.go (line 16): "Priority: .lock.yml > .yml (same as extractCallWorkflowPermissions)." A reader following that comment will find no such function.
💡 Suggested fix in call_workflow_secrets.go
Change:
// Priority: .lock.yml > .yml (same as extractCallWorkflowPermissions).To:
// Priority: .lock.yml > .yml (same as extractCallWorkflowPermissionImport).
pelikhan
deleted the
chore/remove-dead-functions-2026-06-26-ec876ab64c256a14
branch
There was a problem hiding this comment.
Dead code removal is valid: all 5 deleted functions are confirmed unreferenced in production code, go build/vet/test pass, and the deleted wrapper tests are adequately superseded by existing TestExtractCallWorkflowPermissionImport_* and TestCallWorkflowPermissions_EndToEnd coverage.
One inline comment for a documentation artifact introduced by this diff. Two additional stale references in unchanged files are noted below as non-blocking follow-ups.
Non-blocking: stale function names in compiler.go doc comment
pkg/workflow/compiler.go lines 308–312 (unchanged file) still name hasExpressionInRunContent and hasNonAllowedExpressionInRunContent as the two text scans used by Path B of validateTemplateInjection. Both functions are removed by this PR; the actual call at line 343 is scanRunContentExpressions(yamlContent). The comment should be updated to describe scanRunContentExpressions and its .hasUnsafe / .hasDisallowed fields instead.
Non-blocking: stale cross-reference in call_workflow_secrets.go
pkg/workflow/call_workflow_secrets.go line 16 reads // Priority: .lock.yml > .yml (same as extractCallWorkflowPermissions). — the referenced helper is deleted. Update to reference extractCallWorkflowPermissionImport or just describe the priority rule directly.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 88.4 AIC · ⌖ 6.86 AIC · ⊞ 5.2K
| @@ -645,48 +470,6 @@ jobs: | |||
|
|
|||
| // TestExtractCallWorkflowPermissions_LockYMLPriorityOverYML tests that .lock.yml takes | |||
| // priority over .yml when both exist | |||
There was a problem hiding this comment.
Orphan comment left behind after function deletion: the doc comment for TestExtractCallWorkflowPermissions_LockYMLPriorityOverYML has no function body below it and will mislead anyone reading the file.
💡 Suggested fix
Delete the two dangling lines (lines 471–472):
-// TestExtractCallWorkflowPermissions_LockYMLPriorityOverYML tests that .lock.yml takes
-// priority over .yml when both exist
-
// TestCallWorkflowPermissions_EndToEnd tests...The .lock.yml > .yml priority is still exercised end-to-end inside TestBuildCallWorkflowJobs_SetsPermissionsFromLockYML and TestCallWorkflowPermissions_EndToEnd—no coverage gap, just remove the stale comment.
Functions Removed
permissionLevelRankpkg/workflow/call_workflow_permissions.gofindUncoveredWorkerPermissionspkg/workflow/call_workflow_permissions.goextractCallWorkflowPermissionspkg/workflow/call_workflow_permissions.gohasNonAllowedExpressionInRunContentpkg/workflow/template_injection_validation.gohasExpressionInRunContentpkg/workflow/template_injection_validation.goTests Removed
TestFindUncoveredWorkerPermissionsTestExtractCallWorkflowPermissions_FromLockYMLTestExtractCallWorkflowPermissions_FromYMLTestExtractCallWorkflowPermissions_FromMDTestExtractCallWorkflowPermissions_FileNotFoundTestExtractCallWorkflowPermissions_LockYMLPriorityOverYMLTestHasNonAllowedExpressionInRunContentTestHasExpressionInRunContentVerification
go build ./...go vet ./...go vet -tags=integration ./...make fmtgo test ./pkg/...(pre-existingpkg/clifailure unrelated)https://github.com/github/gh-aw/actions/runs/28246514898