Conversation
Removes unreachable Go functions identified by the deadcode static analyzer. Functions removed: - ResolveRefToSHA (pkg/parser/remote_fetch.go) - ExtractPermissionsFromContent (pkg/parser/content_extractor.go) - SemanticVersion.IsPrecise (pkg/semverutil/semverutil.go) - SemanticVersion.IsNewer (pkg/semverutil/semverutil.go) - writeArgsToYAMLInline (pkg/workflow/args.go) - higherIntegrityLevels (pkg/workflow/cache_integrity.go) - GenerateHeredocDelimiter (pkg/workflow/strings.go) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Removes Go dead code (and the now-obsolete unit tests) based on deadcode analyzer output.
Changes:
- Deleted 7 unreachable helper functions across
pkg/parser,pkg/semverutil, andpkg/workflow. - Removed unit tests that exclusively exercised the deleted functions (including deleting an entire test file).
- Cleaned up related imports in the affected test files.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/strings.go | Removes GenerateHeredocDelimiter; keeps seeded delimiter helper. |
| pkg/workflow/strings_test.go | Removes tests that only covered GenerateHeredocDelimiter. |
| pkg/workflow/permissions_import_test.go | Removes test/import that only covered ExtractPermissionsFromContent. |
| pkg/workflow/mcp_config_utils_test.go | Deletes test file that only covered writeArgsToYAMLInline. |
| pkg/workflow/cache_integrity.go | Removes unused higherIntegrityLevels helper. |
| pkg/workflow/cache_integrity_test.go | Removes test that only covered higherIntegrityLevels. |
| pkg/workflow/args.go | Removes unused writeArgsToYAMLInline helper. |
| pkg/semverutil/semverutil.go | Removes unused exported methods IsPrecise / IsNewer. |
| pkg/semverutil/semverutil_test.go | Removes tests that only covered IsPrecise / IsNewer. |
| pkg/parser/remote_fetch.go | Removes exported wrapper ResolveRefToSHA. |
| pkg/parser/content_extractor.go | Removes exported wrapper ExtractPermissionsFromContent. |
Comments suppressed due to low confidence (2)
pkg/workflow/strings.go:268
- In the GenerateHeredocDelimiterFromSeed docstring, the later comment text still references GenerateHeredocDelimiter, which is removed in this PR. Please update the docstring to avoid referencing a non-existent function and describe the empty-seed fallback directly.
// GenerateHeredocDelimiterFromSeed creates a stable heredoc delimiter derived from a seed
// (typically the workflow frontmatter hash hex string) so that repeated compilations of the
// same workflow produce identical lock files.
pkg/parser/remote_fetch.go:707
- This PR removes the exported parser.ResolveRefToSHA symbol, leaving only ResolveRefToSHAForHost. Removing an exported function is a breaking API change for external importers; if you want to preserve backward compatibility, consider keeping ResolveRefToSHA as a deprecated wrapper that calls ResolveRefToSHAForHost(owner, repo, ref, "").
return downloadFileFromGitHubWithDepth(owner, repo, path, ref, 0, host)
}
// ResolveRefToSHAForHost resolves a git ref to its full commit SHA on a specific GitHub host.
// Use this when the target repository is on a different host than the one configured via GH_HOST.
// host is the hostname without scheme (e.g., "github.com", "myorg.ghe.com").
// An empty host uses the default configured host (GH_HOST or github.com).
func ResolveRefToSHAForHost(owner, repo, ref, host string) (string, error) {
return resolveRefToSHA(owner, repo, ref, host)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -263,45 +263,6 @@ func ShortenCommand(command string) string { | |||
| return shortened | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
This removes the exported GenerateHeredocDelimiter API from pkg/workflow. Even if it’s currently unused within this repo, it is a breaking change for any external consumers of the module; consider keeping a deprecated wrapper that delegates to GenerateHeredocDelimiterFromSeed(name, "") (or equivalent) if backward compatibility matters.
This issue also appears on line 266 of the same file.
Suggested change
| // GenerateHeredocDelimiter is retained for backward compatibility. | |
| // | |
| // Deprecated: Use GenerateHeredocDelimiterFromSeed with an explicit seed instead. | |
| func GenerateHeredocDelimiter(name string) (string, error) { | |
| return GenerateHeredocDelimiterFromSeed(name, "") | |
| } |
Comment on lines
54
to
58
| return strings.TrimSpace(string(extractedJSON)), nil | ||
| } | ||
|
|
||
| // ExtractPermissionsFromContent extracts permissions section from frontmatter as JSON string | ||
| func ExtractPermissionsFromContent(content string) (string, error) { | ||
| return extractFrontmatterField(content, "permissions", "{}") | ||
| } | ||
|
|
||
| // extractFrontmatterField extracts a specific field from frontmatter as JSON string | ||
| func extractFrontmatterField(content, fieldName, emptyValue string) (string, error) { |
There was a problem hiding this comment.
Removing the exported parser.ExtractPermissionsFromContent function is a breaking API change for any external consumers of this module. If the parser package is treated as public, consider leaving a deprecated wrapper (or documenting an alternative) rather than deleting the symbol outright.
Comment on lines
98
to
103
| return ver | ||
| } | ||
|
|
||
| // IsPrecise returns true if the version has explicit minor and patch components. | ||
| // For example, "v6.0.0" is precise, but "v6" is not. | ||
| func (v *SemanticVersion) IsPrecise() bool { | ||
| versionPart := strings.TrimPrefix(v.Raw, "v") | ||
| dotCount := strings.Count(versionPart, ".") | ||
| return dotCount >= 2 // Require at least major.minor.patch | ||
| } | ||
|
|
||
| // IsNewer returns true if v is strictly newer than other. | ||
| // Uses golang.org/x/mod/semver.Compare for proper semantic version comparison. | ||
| func (v *SemanticVersion) IsNewer(other *SemanticVersion) bool { | ||
| v1 := EnsureVPrefix(v.Raw) | ||
| v2 := EnsureVPrefix(other.Raw) | ||
| isNewer := semver.Compare(v1, v2) > 0 | ||
| log.Printf("Version comparison: %s vs %s, isNewer=%v", v.Raw, other.Raw, isNewer) | ||
| return isNewer | ||
| } | ||
|
|
||
| // Compare compares two semantic versions and returns 1 if v1 > v2, -1 if v1 < v2, | ||
| // or 0 if they are equal. A bare version without a leading "v" is accepted. | ||
| func Compare(v1, v2 string) int { |
There was a problem hiding this comment.
This PR removes exported methods (*SemanticVersion).IsPrecise and (*SemanticVersion).IsNewer. Even if they’re unused internally, deleting exported APIs is a breaking change for external consumers; consider keeping deprecated methods that delegate to existing helpers (e.g., Compare/EnsureVPrefix) if API stability is a goal.
pelikhan
deleted the
dead-code/remove-unreachable-functions-20260331-3c2560ccf6189c7c
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dead Code Removal
This PR removes unreachable Go functions identified by the
deadcodestatic analyzer.Functions Removed
ResolveRefToSHApkg/parser/remote_fetch.goExtractPermissionsFromContentpkg/parser/content_extractor.goSemanticVersion.IsPrecisepkg/semverutil/semverutil.goSemanticVersion.IsNewerpkg/semverutil/semverutil.gowriteArgsToYAMLInlinepkg/workflow/args.gohigherIntegrityLevelspkg/workflow/cache_integrity.goGenerateHeredocDelimiterpkg/workflow/strings.goTests Removed
TestSemanticVersionIsPrecise(pkg/semverutil/semverutil_test.go) — exclusively testedIsPreciseTestSemanticVersionIsNewer(pkg/semverutil/semverutil_test.go) — exclusively testedIsNewerTestWriteArgsToYAMLInline(pkg/workflow/mcp_config_utils_test.go) — exclusively testedwriteArgsToYAMLInline(entire file deleted)TestHigherIntegrityLevels(pkg/workflow/cache_integrity_test.go) — exclusively testedhigherIntegrityLevelsTestGenerateHeredocDelimiter(pkg/workflow/strings_test.go) — exclusively testedGenerateHeredocDelimiterTestGenerateHeredocDelimiter_Usage(pkg/workflow/strings_test.go) — exclusively testedGenerateHeredocDelimiterTestGenerateHeredocDelimiter_Uniqueness(pkg/workflow/strings_test.go) — exclusively testedGenerateHeredocDelimiterTestExtractPermissionsFromContent(pkg/workflow/permissions_import_test.go) — exclusively testedExtractPermissionsFromContentVerification
go build ./...— passesgo vet ./...— passesgo vet -tags=integration ./...— passesmake fmt— no changes neededgo test ./pkg/parser/... ./pkg/semverutil/... ./pkg/workflow/...— all passDead Function Count
Skipped (safe WASM functions)
WithSkipValidation,WithNoEmit,WithWorkflowIdentifier,CompileToYAML,ParseWorkflowString— all referenced incmd/gh-aw-wasm/main.go(build-constrainedjs && wasm).Automated by Dead Code Removal workflow — https://github.com/github/gh-aw/actions/runs/23796136562