[dead-code] chore: remove dead functions — 5 functions removed - #58996
Conversation
Remove 5 unreachable functions identified by the deadcode static analyzer, along with their exclusive tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! 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.
|
|
✅ PR Code Quality Reviewer completed the code quality review. No actionable review comments: this PR removes genuinely unreferenced helper wrappers and their orphaned tests without changing remaining call sites or behavior. 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.
|
|
✅ 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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nice cleanup overall. Remaining cuts are mostly now-duplicate tests that existed only to exercise removed wrapper helpers; removing them keeps the suite focused on observable command behavior and shrinks maintenance surface.
net: -180 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #58996 · codex · gpt53codex · 5.19 AIC · ⌖ 3.28 AIC · ⊞ 12.8K
Comment /ponytail to run again
Comments that could not be inline-anchored
pkg/cli/add_command_test.go:419
L419: delete: whole test for compileDispatchWorkflowDependencies wrapper. Nothing replaces it; wrapper is removed and behavior is already covered via command-level compilation tests.
pkg/cli/add_command_test.go:457
L457: delete: whole test for compileCallWorkflowDependencies wrapper. Nothing replaces it; wrapper is removed and behavior stays exercised through top-level add workflow paths.
pkg/cli/file_tracker_test.go:253
L253: delete: TestCompileWorkflowWithTracking_SharedActions integration-style setup around a removed wrapper. Nothing replaces it; tracking behavior is already validated by direct FileTracker tests.
pkg/cli/update_command_test.go:981
L981: delete: TestCompileWorkflowWithRefresh function-existence test for a removed wrapper. Nothing replaces it; update behavior is already asserted in TestUpdateWorkflow_* cases.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — clean dead-code removal, one nit on stale doc comments.
📋 Details
Key Themes
- Removal is well-scoped: each deleted function had no remaining callers and its exclusive tests were removed alongside it (verified via grep across the repo).
- One doc comment (
compileWorkflow, add_workflow_compilation.go:23) still referencescompileWorkflowWithRefresh, which this PR deletes — left as an inline comment.
Positive Highlights
- ✅ Verification steps (
go build,go vet,go vet -tags=integration) are documented in the PR body. - ✅ Tests removed are exclusively those exercising the deleted functions — no unrelated test coverage lost.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 22.7 AIC · ⌖ 14.6 AIC · ⊞ 10.3K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/add_workflow_compilation.go:23
[/codebase-design] This doc comment for compileWorkflow still references compileWorkflowWithRefresh, which was just removed in this PR — the comment now points to a non-existent function.
<details>
<summary>💡 Suggested fix</summary>
Update the comment to reference compileWorkflowWithRefreshAndActionRef (the actual delegate), e.g.:
// compileWorkflow compiles a workflow file without refreshing stop time.
// This is a convenience wrapper around compileWorkflowWithRefreshAndActi…
</details>
There was a problem hiding this comment.
Reviewed with Impeccable distill lens (refactor/cleanup change). This is a clean, mechanical dead-code removal — verified no remaining call sites for any of the 5 removed functions. One minor doc-comment drift flagged inline (stale reference to a deleted function name in a doc comment that now describes a different function). Non-blocking.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 28.5 AIC · ⌖ 13.3 AIC · ⊞ 8.3K
Comments that could not be inline-anchored
pkg/cli/add_workflow_compilation.go:155
The doc comment above (lines 146-154) still refers to the now-deleted compileCallWorkflowDependencies wrapper by name ("compileCallWorkflowDependencies compiles any call-workflow .md worker dependencies..."), but that function was removed in this PR — the comment is now attached to compileCallWorkflowDependenciesWithActionRef. Please update the comment to reference the correct (remaining) function name so it doesn't describe a function that no longer exists.
@copilot please address this.
There was a problem hiding this comment.
🟡 Changes recommended
Preserve the exported compatibility wrapper and applicable tests, and correct stale documentation references.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Removes five reported dead-code functions and associated tests from CLI workflow compilation and ShellCheck tooling.
Changes:
- Removes four workflow-compilation wrappers.
- Removes an exported ShellCheck compatibility wrapper.
- Deletes five tests and unused imports.
File summaries
| File | Review |
|---|---|
pkg/cli/update_command_test.go |
Removes a wrapper-specific refresh test. |
pkg/cli/file_tracker_test.go |
Removes coverage still applicable to the surviving action-ref helper; migrate the test instead. |
pkg/cli/compile_external_tools.go |
Removes a documented exported helper, creating a downstream compatibility risk. |
pkg/cli/add_workflow_compilation.go |
Removes wrappers but leaves two documentation comments referencing deleted symbols. |
pkg/cli/add_command_test.go |
Removes tests for behavior still present in action-ref variants; migrate those tests instead. |
Review details
- Files reviewed: 5/5 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.
| assert.Contains(t, string(recompiled), "name: \"Worker\"", "recompiled lock should contain worker name") | ||
| } | ||
|
|
||
| func TestValidateWorkflowDestination_SkipsExistingWorkflowFromSameSource(t *testing.T) { |
| return compileWorkflowWithRefreshAndActionRef(ctx, filePath, verbose, quiet, engineOverride, "", refreshStopTime, approve) | ||
| } | ||
|
|
||
| func compileWorkflowWithRefreshAndActionRef(ctx context.Context, filePath string, verbose bool, quiet bool, engineOverride, actionRef string, refreshStopTime bool, approve bool) error { |
| return compileCallWorkflowDependenciesWithActionRef(ctx, workflowFile, verbose, quiet, engineOverride, "", force, tracker) | ||
| } | ||
|
|
||
| func compileCallWorkflowDependenciesWithActionRef(ctx context.Context, workflowFile string, verbose, quiet bool, engineOverride, actionRef string, force bool, tracker *FileTracker) error { |
| // we don't expect any reaction action files to be created or tracked | ||
| } | ||
|
|
||
| func TestFileTracker_StageAllFiles_NonGitRepo(t *testing.T) { |
@copilot Please take the next forward-progress pass on this PR.
Recent review blockers:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit |
|
🎉 This pull request is included in a new release. Release: |
Dead Code Removal
Removed 5 unreachable functions identified by the
deadcodestatic analyzer (./cmd/... ./internal/tools/...), plus their exclusive tests.Functions Removed
compileWorkflowWithRefreshpkg/cli/add_workflow_compilation.gocompileWorkflowWithTrackingpkg/cli/add_workflow_compilation.gocompileDispatchWorkflowDependenciespkg/cli/add_workflow_compilation.gocompileCallWorkflowDependenciespkg/cli/add_workflow_compilation.goRunShellcheckOnLockFilespkg/cli/compile_external_tools.goTests Removed
TestCompileWorkflowWithRefresh(pkg/cli/update_command_test.go)TestCompileWorkflowWithTracking_SharedActions(pkg/cli/file_tracker_test.go)TestCompileDispatchWorkflowDependencies_FallsBackToRawFrontmatter(pkg/cli/add_command_test.go)TestCompileCallWorkflowDependencies_PropagatesError(pkg/cli/add_command_test.go)TestCompileCallWorkflowDependencies_ForceRecompilesStale(pkg/cli/add_command_test.go)Verification
go build ./...go vet ./...go vet -tags=integration ./...make fmt—fmt-jsontarget fails pre-existing onmain(unrelated to this change); Go/JS formatting portions passed.https://github.com/github/gh-aw/actions/runs/34038688676
Run: https://github.com/github/gh-aw/actions/runs/34042322138