Conversation
Remove validatePiEngineRequirements which is only reachable from CompileToYAML, itself unreachable from any production entry point. Also remove its exclusive test file pi_validation_test.go and the dead call site in compiler_string_api.go. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out — appropriate for a cleanup/dead-code removal PR.
What was reviewed
This PR removes validatePiEngineRequirements: the implementation in engine_validation.go, its call site in compiler_string_api.go, and the corresponding test file pi_validation_test.go.
Assessment
- ✅ Clean and complete removal — all three touch points (call site, implementation, tests) are removed together, leaving no dangling references.
- ✅ Tests deleted with the code — the
pi_validation_test.gofile is correctly removed; keeping dead tests would be misleading. - ✅ No collateral damage — the surrounding validation chain in
ParseWorkflowStringis intact and unaffected. - ✅ DEADCODE.md pattern — consistent with the project's convention of tracking and pruning dead code.
Verdict
No issues found. This is a straightforward, well-scoped dead-code removal. Approving.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 1.2M
There was a problem hiding this comment.
Pull request overview
Removes Pi-engine-specific compile-time validation code and its dedicated unit tests as part of a dead-code cleanup pass.
Changes:
- Removed
Compiler.validatePiEngineRequirementsfrompkg/workflow/engine_validation.go. - Removed the
ParseWorkflowStringinvocation of that validation frompkg/workflow/compiler_string_api.go. - Deleted
pkg/workflow/pi_validation_test.go, which exclusively tested the removed function.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_string_api.go | Removes the Pi engine requirements validation step from the string-based parsing pipeline. |
| pkg/workflow/engine_validation.go | Deletes the validatePiEngineRequirements helper implementation and its error messages. |
| pkg/workflow/pi_validation_test.go | Removes unit tests that covered only the deleted Pi validation helper. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/workflow/compiler_string_api.go:151
- Removing the Pi engine requirement validation here changes ParseWorkflowString behavior (Wasm/browser path) to no longer emit a clear compile-time error when
engine: piis used withouttools.github.mode: gh-proxyandtools.cli-proxy: true. If those requirements are still intended, consider keeping this validation (or moving it into the shared validation pipeline so file-based compilation and string-based compilation behave consistently). If they’re no longer required, please update the Pi engine docs/comments that still state they are enforced at compile time (e.g., pkg/workflow/pi_engine.go mentionsvalidatePiEngineRequirements).
// Validate GitHub tool configuration
if err := validateGitHubToolConfig(workflowData.ParsedTools, workflowData.Name); err != nil {
return nil, fmt.Errorf("%s: %w", cleanPath, err)
}
- Files reviewed: 3/3 changed files
- Comments generated: 0
Dead Code Removal
This PR removes unreachable Go functions identified by the
deadcodestatic analyzer.Functions Removed
Compiler.validatePiEngineRequirementspkg/workflow/engine_validation.goTests Removed
pkg/workflow/pi_validation_test.go— exclusively testedvalidatePiEngineRequirementsVerification
go build ./...— passesgo vet ./...— passesgo vet -tags=integration ./...— passesmake fmt— no changes neededDead Function Count
Automated by Dead Code Removal workflow — https://github.com/github/gh-aw/actions/runs/25504175612