Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes dead safe-output “output job builder” methods from the workflow compiler (and associated tests), continuing the deadcode cleanup plan.
Changes:
- Deleted several unused
Compiler.buildCreateOutput*Jobmethods (comments/discussions/code-scanning/PR review comments/agent session) plus missing-data/tool job builders. - Removed tests that exclusively covered those dead builders (including deleting
add_comment_dependencies_test.go). - Updated
DEADCODE.mdplanning/tracking content and extended.serena/project.ymlconfiguration.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/security_reports_test.go | Removes tests/imports tied to the deleted code-scanning-alert job builder. |
| pkg/workflow/safe_output_refactor_test.go | Drops helper-usage tests that depended on removed standalone builders. |
| pkg/workflow/missing_tool_test.go | Removes job-construction assertions for the deleted missing-tool job builder. |
| pkg/workflow/missing_tool.go | Deletes buildCreateOutputMissingToolJob, leaving config parsing intact. |
| pkg/workflow/missing_issue_reporting.go | Deletes shared job builder + helper (envVarPrefix) used only by removed code. |
| pkg/workflow/missing_data_test.go | Removes tests for the deleted missing-data job builder. |
| pkg/workflow/missing_data.go | Deletes buildCreateOutputMissingDataJob, leaving config parsing intact. |
| pkg/workflow/create_pr_review_comment.go | Deletes dead PR review comment job builder, keeps config parsing. |
| pkg/workflow/create_discussion_dependencies_test.go | Removes job dependency test for deleted discussion job builder. |
| pkg/workflow/create_discussion.go | Deletes dead discussion job builder, keeps config parsing + helpers. |
| pkg/workflow/create_code_scanning_alert.go | Deletes dead code-scanning-alert job builder, keeps config parsing. |
| pkg/workflow/create_agent_session_test.go | Removes tests for the deleted agent session job builder. |
| pkg/workflow/create_agent_session.go | Deletes dead agent session job builder, keeps config parsing. |
| pkg/workflow/add_comment_dependencies_test.go | Deleted file; it only tested the removed add-comment job builder. |
| pkg/workflow/add_comment.go | Deletes dead add-comment job builder, keeps config parsing + permissions helper. |
| DEADCODE.md | Updates deadcode plan content (needs tracker accuracy tweaks per comments). |
| .serena/project.yml | Adds optional Serena config keys (symbol_info_budget, language_backend). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+47
to
+55
| ## Batch plan (85 dead functions as of 2026-03-02, after phases 5–8) | ||
|
|
||
| Batches 1–4 have been completed. The original batches 5–16 are superseded by this plan; | ||
| many of those functions were removed during prior work, and the remainder (plus newly | ||
| discovered dead code) are redistributed below into 30 focused phases. | ||
| Phases 5–8 are complete. The original phases 9–34 are superseded by this revised plan, | ||
| which groups work by domain, includes LOC estimates, and skips functions too small to | ||
| be worth the disruption. | ||
|
|
||
| Each phase: delete the dead functions, delete tests that exclusively test them, | ||
| run verification, commit, open PR. | ||
| run verification, commit, open PR. Branches are stacked: dc-9 based on dc-8, dc-10 | ||
| based on dc-9, etc. |
There was a problem hiding this comment.
DEADCODE.md is now out of date for this PR: it still says “85 dead functions … after phases 5–8” and “Phases 5–8 are complete”, but this PR is phase 9 and removes additional dead functions (per PR description 85→76). Please update this header/blurb to reflect the post-phase-9 count/status (or explicitly note that the counts are pre-phase-9 if that’s intentional).
Comment on lines
+69
to
+75
| **Not worth deleting (< 10 lines, isolated):** | ||
| `envVarPrefix` (2), `GenerateSafeInputGoToolScriptForInspector` (2), `MapToolConfig.GetAny` (3), | ||
| `HasErrors` (5), `extractMapFromFrontmatter` (5), `GetDefaultMaxForType` (5), | ||
| `GetValidationConfigForType` (6), `getPlaywrightMCPPackageVersion` (6), `RunGit` (6), | ||
| `ExecGHWithOutput` (7), `convertGoPatternToJavaScript` (7), `ValidateExpressionSafetyPublic` (7), | ||
| `IsSafeInputsHTTPMode` (7), `HasSafeJobsEnabled` (7), `renderSafeOutputsMCPConfig` (8), | ||
| `renderPlaywrightMCPConfig` (8), `SecurityFinding.String` (8), `GetAllEngines` (11) |
There was a problem hiding this comment.
This “Not worth deleting” list still includes envVarPrefix, but envVarPrefix was deleted in this PR (and no longer exists in the Go codebase). Please remove it from the list (or replace with another still-present skipped function) so the tracker remains accurate.
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.
Remove 8 dead
buildCreateOutput*JobCompiler methods and 2 dead missing-data/tool builders (~480 LOC removed, 85→76 dead functions).Functions removed:
Compiler.buildCreateOutputAddCommentJob(116 LOC, add_comment.go)Compiler.buildCreateOutputDiscussionJob(78 LOC, create_discussion.go)Compiler.buildCreateOutputCodeScanningAlertJob(73 LOC, create_code_scanning_alert.go)Compiler.buildCreateOutputPullRequestReviewCommentJob(63 LOC, create_pr_review_comment.go)Compiler.buildIssueReportingJob(61 LOC, missing_issue_reporting.go)Compiler.buildCreateOutputAgentSessionJob(53 LOC, create_agent_session.go)Compiler.buildCreateOutputMissingDataJob(17 LOC, missing_data.go)Compiler.buildCreateOutputMissingToolJob(17 LOC, missing_tool.go)Test changes: deleted
add_comment_dependencies_test.go; removed dead test functions fromcreate_agent_session_test.go,create_discussion_dependencies_test.go,missing_data_test.go,missing_tool_test.go,security_reports_test.go,safe_output_refactor_test.go.