[dead-code] chore: remove dead functions — 4 functions removed - #48917
Conversation
Remove unreachable functions identified by deadcode analyzer: - FormatProgressMessageStderr, FormatPromptMessageStderr, FormatVerboseMessageStderr (pkg/console) - GetValidationConfigJSON (pkg/workflow) — thin wrapper only used by tests, updated tests to call GetValidationConfigJSONWithDataSchema directly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Caution agentic threat detected DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. No test files were added or modified in this PR. Test Quality Sentinel skipped. This is a dead-code removal PR (#48917) that removes 4 unused functions and refactors test calls to match the removal. No new behavioral tests were introduced. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48917 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (30 additions detected, threshold is 100). |
There was a problem hiding this comment.
Pull request overview
Removes four functions identified as unreachable while preserving validation coverage.
Changes:
- Removes three unused console stderr formatters, including WASM stubs.
- Replaces validation-wrapper test calls with the lower-level API.
Show a summary per file
| File | Description |
|---|---|
pkg/console/console.go |
Removes unused stderr formatters. |
pkg/console/console_wasm.go |
Removes corresponding WASM stubs. |
pkg/workflow/safe_outputs_validation_config.go |
Removes the validation wrapper. |
pkg/workflow/safe_output_validation_config_test.go |
Updates wrapper call sites. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
| func TestGetValidationConfigJSON(t *testing.T) { | ||
| // Test with nil (all types) | ||
| jsonStr, err := GetValidationConfigJSON(nil, nil) | ||
| jsonStr, err := GetValidationConfigJSONWithDataSchema(nil, nil, false, nil) |
| // GetValidationConfigJSONWithDataSchema behaves like GetValidationConfigJSONWithDataSchema and additionally | ||
| // injects a normalized data schema into body-bearing safe-output types. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean dead-code removal overall. One minor doc issue found (inline comment): the updated doc comment on GetValidationConfigJSONWithDataSchema became self-referential after removing the wrapper function. The rest of the changes look correct and complete.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21 AIC · ⌖ 4.81 AIC · ⊞ 5.3K
Comments that could not be inline-anchored
pkg/workflow/safe_outputs_validation_config.go:496
The doc comment is now self-referential: it says "behaves like GetValidationConfigJSONWithDataSchema" — the function itself. Please update it to describe what the function actually does, e.g.:
// GetValidationConfigJSONWithDataSchema returns the validation configuration as indented JSON.
// If enabledTypes is empty or nil, returns all validation configs.
// If mentions is non-empty, a top-level "mentions" key is included.
// If dataEnabled is true and dataSchema is non-nil, injects a n…
</details>
There was a problem hiding this comment.
🧪 Test Quality Sentinel Report
✅ Test Quality Score: N/A — Maintenance Only
Dead-code removal PR with no new behavioral tests. Test modifications are purely mechanical (function call updates).
📊 Analysis Summary
| Metric | Value |
|---|---|
| Test files modified | 1 (safe_output_validation_config_test.go) |
| New test functions | 0 |
| Modified test functions | 6 (mechanical updates only) |
| New behavioral tests | 0 |
| Violations | 0 |
Changes:
- Removed 4 dead functions:
GetValidationConfigJSON,FormatProgressMessageStderr,FormatPromptMessageStderr,FormatVerboseMessageStderr - Updated 6 test calls from
GetValidationConfigJSON()→GetValidationConfigJSONWithDataSchema() - No functional test logic changes
Verdict
✅ passed. Maintenance PR with no new test coverage to evaluate. Existing tests mechanically updated to match code changes.
There was a problem hiding this comment.
Clean, correctly-verified dead code removal. No functional or correctness issues found.
Review notes
- Verified none of the 4 removed functions (
FormatProgressMessageStderr,FormatPromptMessageStderr,FormatVerboseMessageStderr,GetValidationConfigJSON) have remaining callers anywhere in the codebase. - Test file was correctly updated to call
GetValidationConfigJSONWithDataSchema(..., false, nil)directly instead of dropping coverage. go build ./pkg/console/... ./pkg/workflow/...passes.- One low-severity nit filed inline: a doc comment became self-referential after the removal (mentions itself instead of describing behavior directly). Non-blocking.
🔎 Code quality review by PR Code Quality Reviewer · aut00 · 24 AIC · ⌖ 7.14 AIC · ⊞ 7.8K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/safe_outputs_validation_config.go:177
Doc comment is now self-referential and no longer explains the relationship to the removed function.
<details>
<summary>💡 Details</summary>
The comment reads: "GetValidationConfigJSONWithDataSchema behaves like GetValidationConfigJSONWithDataSchema and additionally..." — it should instead describe what the function does directly, since the original target of the comparison (GetValidationConfigJSON) was deleted in this PR.
Suggested fix:
// GetValidationConfigJSONWithDataSchema retur…
</details>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — approving with one minor doc fix requested.
📋 Key Themes & Highlights
Key Themes
- Self-referential doc comment: after removing the
GetValidationConfigJSONwrapper, itsGetValidationConfigJSONWithDataSchemadoc was updated to say "behaves like GetValidationConfigJSONWithDataSchema" — a no-op description. The prose from the deleted wrapper should move here.
Positive Highlights
- ✅ Clean, mechanical removal — no logic changes
- ✅ Tests updated to call the underlying function directly rather than deleted
- ✅ Cache comment updated alongside the rename
- ✅ WASM stub alignment is a nice housekeeping win
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 20.9 AIC · ⌖ 4.9 AIC · ⊞ 7K
Comment /matt to run again
| // GetValidationConfigJSONWithDataSchema behaves like GetValidationConfigJSONWithDataSchema and additionally | ||
| // injects a normalized data schema into body-bearing safe-output types. | ||
| func GetValidationConfigJSONWithDataSchema(enabledTypes []string, mentions map[string]any, dataEnabled bool, dataSchema map[string]any) (string, error) { | ||
| safeOutputValidationLog.Printf("Getting validation config JSON for %d types (mentions=%t)", len(enabledTypes), len(mentions) > 0) |
There was a problem hiding this comment.
[/codebase-design] The updated doc comment is now self-referential: it says behaves like GetValidationConfigJSONWithDataSchema — referencing the function itself.
💡 Suggested fix
Move the description from the deleted GetValidationConfigJSON wrapper here:
// GetValidationConfigJSONWithDataSchema returns the validation configuration as
// indented JSON, optionally filtered to enabledTypes and augmented with a
// normalized data schema injected into body-bearing safe-output types.
// If enabledTypes is empty or nil, all configs are returned.
// If mentions is non-empty, a top-level "mentions" key is included so that
// collect_ndjson_output.cjs honours the configured `@mention` allowlist.@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
Overview
Removes 4 dead/unused functions from the
consoleandworkflow(safe-outputs validation) packages, and updates internal callers/tests to match. All changes are internal to this repository (unexported or package-scoped call sites); no external/public API surface is affected.Files Changed
pkg/console/console.gopkg/console/console_wasm.gopkg/workflow/safe_outputs_validation_config.gopkg/workflow/safe_output_validation_config_test.goRemoved Functions
FormatProgressMessageStderr(pkg/console/console.go,pkg/console/console_wasm.go)FormatPromptMessageStderr(pkg/console/console.go,pkg/console/console_wasm.go)FormatVerboseMessageStderr(pkg/console/console.go)GetValidationConfigJSON(pkg/workflow/safe_outputs_validation_config.go) — thin wrapper aroundGetValidationConfigJSONWithDataSchema(enabledTypes, mentions, false, nil)Impact
safe_output_validation_config_test.gowere switched fromGetValidationConfigJSON(...)toGetValidationConfigJSONWithDataSchema(..., false, nil)directly.GetValidationConfigJSONWithDataSchemaretains identical behavior for the default (non-data-schema) case.console_wasm.goone-line function declarations were realigned (gofmt) after removals.*Stderrconsole formatters orGetValidationConfigJSONmust switch to the retained equivalents.Verification
GetValidationConfigJSONWithDataSchemadirectly, keeping existing coverage intact.