Uh oh!
There was an error while loading. Please reload this page.
[dead-code] chore: remove dead functions — 5 functions removed - #47598
Conversation
Remove unreachable functions identified by deadcode analyzer: - htmlEscape (pkg/cli/bootstrap_profile_helpers.go) - PrintListItem (pkg/console/print.go) - PrintErrorChain (pkg/console/print.go) - grypeCache.reset (pkg/cli/grype.go) - buildCopilotSettingsCleanupTrap (pkg/workflow/copilot_engine_execution.go) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
✅ PR Code Quality Reviewer completed the code quality review. |
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47598 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. No test files were added or modified in this PR. PR #47598 removes 5 dead functions; the test files listed in test-files.txt are existing files but show no diff changes (test-diff.txt and diff-numstat.txt are both empty). Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
Removes five repository-local dead functions and their obsolete tests.
Changes:
- Removes unused CLI/cache helpers.
- Removes two exported console print wrappers.
- Removes obsolete Copilot cleanup helper tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/bootstrap_profile_helpers.go | Removes htmlEscape. |
pkg/cli/bootstrap_profile_helpers_test.go | Removes its assertion. |
pkg/cli/grype.go | Removes cache reset helper. |
pkg/cli/grype_test.go | Removes reset test. |
pkg/console/print.go | Removes two exported print APIs. |
pkg/console/print_test.go | Removes corresponding tests. |
pkg/workflow/copilot_engine_execution.go | Removes standalone cleanup trap helper. |
pkg/workflow/copilot_home_expansion_test.go | Removes cleanup trap tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Medium
| fmt.Fprintln(stderr, FormatListItemStderr(item)) | ||
| } | ||
| // PrintSectionHeader formats and prints a section header to stderr. |
| // PrintSectionHeader formats and prints a section header to stderr. | ||
| func PrintSectionHeader(header string) { | ||
| fmt.Fprintln(stderr, FormatSectionHeaderStderr(header)) | ||
| } |
| } | ||
| } | ||
| // TestBashIntegration_MCPConfigExport drives the generated export block |
| // buildCopilotSettingsCleanupAndExitCodeTrap returns an EXIT trap that: | ||
| // 1. persists the execution step exit code for setup/post OTLP conclusion spans, and | ||
| // 2. removes the temporary Copilot settings file. |
There was a problem hiding this comment.
Clean dead-code removal. All 5 removed functions were unused, their tests are correctly removed alongside them, and the buildCopilotSettingsCleanupAndExitCodeTrap replacement already covers the more important trap behaviour. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 13.8 AIC · ⌖ 5.12 AIC · ⊞ 5K
There was a problem hiding this comment.
Review: Dead Code Removal
One non-blocking issue found; otherwise the removals are clean.
All 5 deleted functions (, , , , ) have no remaining callers in the repo — the deletions are safe. The / functions are exported but unused outside the package, so their removal carries no API risk in this internal repo.
One finding (non-blocking): the comment on line 94 of pkg/workflow/copilot_engine_execution.go still names the deleted function. See the inline comment for a suggested fix.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 30 AIC · ⌖ 4.56 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/copilot_engine_execution.go:94
Stale comment references the deleted buildCopilotSettingsCleanupTrap: After this PR removes that function, the phrase "matching buildCopilotSettingsCleanupTrap behavior" on line 94 becomes a dangling cross-reference that confuses future readers who search for that name and find nothing.
<details>
<summary>💡 Suggested fix</summary>
Replace the cross-reference with a self-contained description:
// The body is single-quoted so $HOME in copilotSettingsPath is expanded at// trap-fir…</details>There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on one coverage gap.
📋 Key Themes & Highlights
Key Themes
- Missing test port: Three integration tests that pinned
$HOMEexpansion timing were deleted alongsidebuildCopilotSettingsCleanupTrap, but their invariant applies equally to the supersedingbuildCopilotSettingsCleanupAndExitCodeTrapand was not ported.
Positive Highlights
- ✅ Clean, surgical removals — each deleted function had zero call sites
- ✅ PR description is thorough: lists every removed function and test by name
- ✅ All removed tests genuinely tested the deleted code rather than shared behaviour
- ✅ Build verification (
go build,go vet,make fmt) is confirmed
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 28.5 AIC · ⌖ 4.61 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/copilot_home_expansion_test.go:96
[/tdd] Three removed tests pinned a critical invariant: $HOME expands at trap-fire time, not definition time. The replacement function buildCopilotSettingsCleanupAndExitCodeTrap carries the same single-quoting contract but no equivalent tests were ported.
<details>
<summary>💡 Suggestion</summary>
Port at minimum a unit test and the bash-integration TrapFiresWithRuntimeHome variant to buildCopilotSettingsCleanupAndExitCodeTrap. The bash test that changes HOME mid-script is e…
🎉 This pull request is included in a new release. Release: |
Summary
Removes 5 dead functions and their associated tests across three packages. No behaviour changes — all removed symbols were unused.
Changes
pkg/cli/bootstrap_profile_helpers.gohtmlEscape()helper and its"html"importpkg/cli/grype.go(*grypeCache).reset()method (was test-only)pkg/console/print.goPrintListItem()andPrintErrorChain()pkg/workflow/copilot_engine_execution.gobuildCopilotSettingsCleanupTrap()Corresponding tests in
*_test.gofiles are removed alongside each function.Impact
PrintListItem,PrintErrorChainwere exported but unusedMotivation
Dead code elimination to reduce maintenance surface and avoid confusion for future contributors.