Skip to content

[dead-code] chore: remove dead functions — 4 functions removed#25798

Merged
pelikhan merged 1 commit intomainfrom
dead-code/remove-batch-2026-04-11-9617ccb9652489c5
Apr 11, 2026
Merged

[dead-code] chore: remove dead functions — 4 functions removed#25798
pelikhan merged 1 commit intomainfrom
dead-code/remove-batch-2026-04-11-9617ccb9652489c5

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot commented Apr 11, 2026

Dead Code Removal

This PR removes unreachable Go functions identified by the deadcode static analyzer.

Functions Removed

Function File
ReadFileFromHEAD pkg/gitutil/gitutil.go
normalizeContainerPins + containerPinRE pkg/workflow/compiler.go
ResolveAgentFilePath pkg/workflow/engine_helpers.go
BuildInvalidAgentPathStep pkg/workflow/engine_helpers.go

Tests Removed

  • TestReadFileFromHEAD — exclusively tested the deleted ReadFileFromHEAD function
  • TestReadFileFromHEADWithRoot/returns same result as ReadFileFromHEAD — sub-test that called the deleted function
  • TestResolveAgentFilePath — exclusively tested the deleted ResolveAgentFilePath function
  • TestResolveAgentFilePathFormat — exclusively tested the deleted ResolveAgentFilePath function
  • TestShellVariableExpansionInAgentPath — exclusively tested the deleted ResolveAgentFilePath function

The container pin normalization regex was inlined into wasm_golden_test.go as a test-local variable (testContainerPinRE) since it is still needed for golden test stability (native compilation includes @sha256: pins; WASM does not).

Verification

  • go build ./... — passes
  • go vet ./... — passes
  • go vet -tags=integration ./... — passes
  • make fmt — Go code formatted (fmt-cjs pre-existing failure unrelated to this change)
  • go test ./pkg/gitutil/... ./pkg/workflow/... — passes

Dead Function Count

  • Before this batch: ~12 functions
  • Removed in this PR: 4 functions
  • Remaining: ~8 functions (some skipped: live in WASM binary, or large test-helper migration needed)

Automated by Dead Code Removal workflow — https://github.com/github/gh-aw/actions/runs/24281896384

Generated by Dead Code Removal Agent · ● 3.1M ·

  • expires on Apr 14, 2026, 12:05 PM UTC


✨ PR Review Safe Output Test - Run 24282404926

💥 [THE END] — Illustrated by Smoke Claude · ● 202.5K ·

Remove unreachable Go functions identified by the deadcode static analyzer:
- ReadFileFromHEAD (pkg/gitutil/gitutil.go)
- normalizeContainerPins + containerPinRE (pkg/workflow/compiler.go)
- ResolveAgentFilePath (pkg/workflow/engine_helpers.go)
- BuildInvalidAgentPathStep (pkg/workflow/engine_helpers.go)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💥 Automated smoke test review - all systems nominal! This PR removes dead code functions across gitutil, compiler.go, and engine_helpers.go. The changes look clean and focused.

💥 [THE END] — Illustrated by Smoke Claude · ● 202.5K

return ReadFileFromHEADWithRoot(filePath, gitRoot)
}

// ReadFileFromHEADWithRoot is like ReadFileFromHEAD but accepts a pre-computed git
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Good cleanup — ReadFileFromHEAD was a thin wrapper around ReadFileFromHEADWithRoot. Removing the wrapper reduces the public API surface and makes callers use the more explicit version directly. This is a clean dead-code removal.

@@ -35,17 +35,6 @@ func normalizeHeredocDelimiters(content string) string {
return heredocDelimiterRE.ReplaceAllString(content, "GH_AW_${1}_NORM_EOF")
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Removing containerPinRE and normalizeContainerPins here makes sense if they're no longer used. Worth confirming no test helpers or other callers reference these functions before finalizing the PR.

@pelikhan pelikhan marked this pull request as ready for review April 11, 2026 13:48
Copilot AI review requested due to automatic review settings April 11, 2026 13:48
@pelikhan pelikhan merged commit bcbc787 into main Apr 11, 2026
54 checks passed
@pelikhan pelikhan deleted the dead-code/remove-batch-2026-04-11-9617ccb9652489c5 branch April 11, 2026 13:48
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes dead/unreachable Go helpers identified by deadcode, and keeps WASM golden tests stable by moving container pin normalization into the test.

Changes:

  • Removed unused helpers and their dedicated tests (ReadFileFromHEAD, ResolveAgentFilePath, BuildInvalidAgentPathStep, container pin normalizer).
  • Inlined container pin normalization into wasm_golden_test.go via a test-local regex to preserve golden stability.
Show a summary per file
File Description
pkg/workflow/wasm_golden_test.go Adds a test-local regex to strip @sha256: digest pins during golden comparison
pkg/workflow/engine_helpers.go Removes unused agent-path helpers
pkg/workflow/engine_helpers_test.go Removes tests that exclusively covered the deleted agent-path helper
pkg/workflow/compiler.go Removes unused container pin normalization helper/regex
pkg/gitutil/gitutil.go Removes unused ReadFileFromHEAD wrapper
pkg/gitutil/gitutil_test.go Removes tests that exclusively covered the deleted wrapper

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 6/6 changed files
  • Comments generated: 1

"github.com/stretchr/testify/require"
)

// containerPinRE matches Docker image digest pins of the form @sha256:<64 hex chars>.
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment refers to containerPinRE, but the actual regex variable here is testContainerPinRE. This can be confusing when grepping or when reading the test; consider updating the comment to match the identifier (or rename the variable/comment consistently).

Suggested change
// containerPinRE matches Docker image digest pins of the form @sha256:<64 hex chars>.
// testContainerPinRE matches Docker image digest pins of the form @sha256:<64 hex chars>.

Copilot uses AI. Check for mistakes.
@github-actions github-actions bot mentioned this pull request Apr 11, 2026
@github-actions
Copy link
Copy Markdown
Contributor Author

🧪 Test Quality Sentinel Report

Test Quality Score: N/A (Pure deletion PR)

Excellent — no deficiencies detected

Metric Value
New test functions analyzed 0
Deleted test functions 4 functions / ~185 lines
Coding-guideline violations None
Build tags present on all test files ✅ Yes
Test inflation detected No

Summary of Test Changes

This PR removes 4 dead production functions and their corresponding tests. All test deletions are correct and appropriate — no orphaned test code remains.

File Change Details
pkg/gitutil/gitutil_test.go 30 lines deleted Removed TestReadFileFromHEAD (2 sub-tests: happy path + error path) and 1 sub-test from TestReadFileFromHEADWithRoot that cross-compared the now-deleted ReadFileFromHEAD wrapper
pkg/workflow/engine_helpers_test.go 148 lines deleted Removed TestResolveAgentFilePath (table-driven, 11 cases including shell-injection rejections), TestResolveAgentFilePathFormat, TestShellVariableExpansionInAgentPath, and the comment for TestShellEscapeArgWithFullyQuotedAgentPath
pkg/workflow/wasm_golden_test.go 5 lines added / 1 deleted ✅ Positive refactoring: moved containerPinRE from production scope into the test file as testContainerPinRE, keeping test utilities test-scoped

Notable Quality Observations

The deleted tests were well-written: TestResolveAgentFilePath was a proper table-driven test with both happy-path cases and security-sensitive rejection cases (double quotes, $, backticks, semicolons, pipes, newlines), and all assertions included descriptive messages. Their removal is justified solely because the functions they tested have been identified as dead code.

The wasm_golden_test.go refactoring is good housekeeping — moving a regex used only in test helpers into test scope prevents it from cluttering the production API.


Language Support

Tests analyzed:

  • 🐹 Go (*_test.go): 0 new functions (deletions only) — all files carry the required //go:build !integration tag

Verdict

Check passed. No new tests were added — all test-file changes are deletions of tests for dead production code. Zero coding-guideline violations detected.


📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

  • Assert on observable outputs, return values, or state changes
  • Cover error paths and boundary conditions
  • Would catch a behavioral regression if deleted
  • Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

  • Assert on internal function calls (mocking internals)
  • Only test the happy path with typical inputs
  • Break during legitimate refactoring even when behavior is correct
  • Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

🧪 Test quality analysis by Test Quality Sentinel · ● 771.1K ·

Copy link
Copy Markdown
Contributor Author

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: N/A (pure deletion PR). All test changes are deletions of tests for removed dead-code functions. Zero violations detected — build tags present, no mocks, deleted tests were well-written. Approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants