fix(webui): hide rework_only steps from DAG visualization

[nextlevelshit]

Summary

• Filter rework_only steps from DAG visualization so they don't clutter the pipeline graph at layer 0
• Add filterReworkOnlySteps helper in dag.go that removes steps with ReworkOnly: true
• Apply the filter in both handlers_pipelines.go (pipeline detail view) and handlers_runs.go (run detail view) before computing DAG layout
• Add comprehensive test coverage for the filtering logic
• Include spec and plan artifacts for the fix

Related to #691

Changes

• internal/webui/dag.go — added filterReworkOnlySteps() function to filter steps before DAG layout computation
• internal/webui/dag_test.go — added tests covering filtering of rework-only steps (mixed, all rework, none rework, empty input)
• internal/webui/handlers_pipelines.go — apply rework-only filter before ComputeDAGLayout call
• internal/webui/handlers_runs.go — apply rework-only filter before ComputeDAGLayout call
• specs/691-dag-rework-only/ — spec, plan, and task artifacts

Test Plan

• New unit tests in dag_test.go validate filtering behavior
• Existing DAG layout tests continue to pass
• go test ./internal/webui/... passes

[nextlevelshit]

Code Review (Wave Pipeline)

Verdict: APPROVE

PR #693 is a clean, well-scoped change that filters rework_only steps from the DAG visualization in the web UI. The code operates on trusted internal data, uses auto-escaped templates, and introduces no new attack surface. All tests pass.

---

Critical Issues

None.

---

In-Diff Findings

Severity	File	Finding
Minor	internal/pipeline/executor.go	Removed //nolint:errcheck from two RunHooks calls without actually handling the returned errors. Linters will now flag these, but runtime behavior is unchanged (errors silently dropped). Suggestion: Either handle the errors (log them) or re-add the //nolint with an explanatory comment.
Suggestion	internal/webui/dag.go:272	stripExcludedDeps removes references from Dependencies but does not strip entries from the Edges field. If a non-rework step has a graph-mode edge targeting a rework-only step, the EdgeInfo tooltip will reference the excluded step. Suggestion: Add edge stripping for completeness, though ComputeDAGLayout already silently drops edges to nodes without positions.
Suggestion	internal/webui/dag_test.go	Missing test case for edges pointing to excluded steps and for stripExcludedDeps with an empty excluded set. Suggestion: Add test cases to document the edge-skipping contract and exercise the early-return branch.

---

Pre-Existing Findings (out of scope — recommend follow-up issues)

Severity	File	Finding
Minor	internal/webui/handlers_webhooks.go:193	handleAPIUpdateWebhook does not validate webhook URLs with isPublicURL(), allowing SSRF bypass by updating a webhook to a private/loopback address after creation. Suggestion: Add isPublicURL validation when req.URL is set.
Minor	internal/webui/handlers_webhooks.go:160	Webhook secret (HMAC key) is returned in API responses for create, update, and detail endpoints, risking exposure if responses are logged or intercepted. Suggestion: Omit or redact the Secret field after initial creation.

---

Summary

The in-diff issues are minor and non-blocking. The pre-existing security findings should be tracked as separate issues. Approving this PR.

---

Generated by Wave pr-review pipeline

[nextlevelshit]

Code Review: APPROVE

PR #693 filters rework_only steps from the DAG visualization in the web UI. The security review found no vulnerabilities -- changes are purely presentational, operate on trusted server-side YAML data, and are protected by existing input validation and template escaping. The quality review found zero blocking issues. The architectural decision to filter at the call site rather than inside ComputeDAGLayout is sound.

---

Findings

Minor

File	Line	Description
internal/webui/handlers_pipelines.go	172	Rework-only filtering logic is duplicated between handlePipelineDetailPage and handleRunDetailPage. Both handlers construct DAGStepInput with the same exclude-and-strip pattern. Suggestion: Extract a shared helper like buildDAGStepsFromPipeline() that encapsulates both filtering and DAGStepInput construction. Evaluate whether the run handler's extra complexity (status, tokens, gate info) makes this worthwhile.

Suggestions (non-blocking)

File	Line	Description
internal/webui/dag.go	272	stripExcludedDeps removes inbound Dependencies referencing excluded steps but does not strip outgoing Edges targeting excluded steps, leaving dangling edge targets. Currently safe because ComputeDAGLayout gracefully skips edges with missing endpoints, but explicit cleanup would be more robust.
internal/webui/handlers_pipelines.go	85	HTML DAG hides rework_only steps, but JSON API endpoints (buildPipelineDetail, handleAPIRunDetail) still expose them. If intentional, consider adding a comment documenting the design choice. If not, apply the same filtering to API responses or add a ?include_rework=true query parameter.
internal/webui/dag_test.go	--	stripExcludedDeps has an early return for len(excluded) == 0, but no test exercises this fast path. Consider adding a subtest with a nil or empty excluded set.
internal/webui/dag_test.go	--	No test verifies behavior when DAGStepInput.Edges reference excluded steps. Consider adding a test with graph-mode edges pointing to excluded steps.
internal/webui/dag_test.go	180	TestComputeDAGLayout_ReworkOnlyExcluded uses a rework-only step with zero dependents. Consider adding a variant where a non-excluded step lists the rework-only step in its Dependencies.
internal/webui/handlers_runs.go	770	Pre-existing: failure class is looked up twice for failed steps -- once inside the stepMap block and again unconditionally. The second overwrites the first. Not introduced by this PR.

---

Reviewed at 2026-04-03T21:00:52Z

[nextlevelshit]

Review Resolution Summary

Total findings: 10 | Accepted & fixed: 3 | Rejected: 3 | Deferred: 4

---

Accepted & Fixed (commit 4ec8b8b)

Severity	Finding	Resolution
Suggestion	stripExcludedDeps does not strip Edges targeting excluded steps (dag.go:272)	Extended stripExcludedDeps to also filter DAGStepInput.Edges where the target is in the excluded set.
Suggestion	Missing test for stripExcludedDeps with empty excluded set (dag_test.go)	Added subtest exercising the len(excluded) == 0 early-return path.
Suggestion	Missing test for Edges referencing excluded steps (dag_test.go)	Added test verifying ComputeDAGLayout gracefully skips edges to non-existent nodes.

Rejected

Finding	Rationale
Removed //nolint:errcheck from RunHooks calls (executor.go)	File is not in the PR diff — finding references code not changed by this PR.
Rework-only filtering logic duplicated between handlers (handlers_pipelines.go:172)	The shared pattern is only ~4 lines; the DAGStepInput construction differs significantly (5 fields vs 15+ fields). Extracting a helper would be a premature abstraction.
Test missing variant where non-excluded step depends on rework-only step (dag_test.go:180)	Already covered by TestStripExcludedDeps, which verifies dependency stripping for steps depending on excluded steps.

Deferred (pre-existing / out-of-scope)

Finding	Reason	Follow-up
JSON API endpoints still expose rework_only steps	Design decision requiring product input — out of scope for this PR.	Decide whether API should filter rework_only steps or add ?include_rework=true parameter.
handleAPIUpdateWebhook missing isPublicURL() validation (SSRF)	Pre-existing, not introduced by this PR.	File security issue to add URL validation on webhook updates.
Webhook secret returned in API responses	Pre-existing, not introduced by this PR.	File security issue to redact secret after initial creation.
Failure class looked up twice for failed steps	Pre-existing, not introduced by this PR.	File minor bug to deduplicate the lookup in handleRunDetailPage.

---

Generated by Wave pr-fix-review pipeline

[nextlevelshit]

Review Resolution Summary

Total findings: 10 | Fixed (prior commit): 3 | Rejected: 3 | Deferred: 4

---

Fixed in commit 4ec8b8b (prior to triage)

Severity	Finding	Resolution
Suggestion	stripExcludedDeps does not strip outgoing Edges targeting excluded steps (dag.go:272)	Extended stripExcludedDeps to also filter DAGStepInput.Edges where the target is in the excluded set and rebuild EdgeInfo from remaining edges.
Suggestion	Missing test for stripExcludedDeps with empty excluded set (dag_test.go)	Added TestStripExcludedDeps_EmptyExcluded exercising both nil and empty-map excluded set paths.
Suggestion	Missing test for Edges referencing excluded steps (dag_test.go)	Added TestComputeDAGLayout_EdgesSkipMissingNodes verifying edges to non-existent nodes are silently dropped.

Rejected

Finding	Rationale
Removed //nolint:errcheck from RunHooks calls (executor.go)	File is not in the PR diff — finding references code not changed by this PR.
Rework-only filtering logic duplicated between handlers (handlers_pipelines.go:172)	The shared pattern is ~4 lines; DAGStepInput construction differs significantly (5 fields vs 15+ fields). Extracting a helper would be premature abstraction.
Missing test variant where non-excluded step depends on rework-only step (dag_test.go:180)	Already covered by TestStripExcludedDeps, which verifies dependency stripping for steps depending on excluded steps.

Deferred (pre-existing / out-of-scope)

Finding	Reason	Suggested Follow-up
JSON API endpoints still expose rework_only steps	Design decision requiring product input — out of scope for this PR.	Decide whether API should filter or add ?include_rework=true parameter.
handleAPIUpdateWebhook missing isPublicURL() validation (SSRF)	Pre-existing, not in PR diff.	File security issue for URL validation on webhook updates.
Webhook secret returned in API responses	Pre-existing, not in PR diff.	File security issue to redact secret after initial creation.
Failure class looked up twice for failed steps (handlers_runs.go)	Pre-existing duplicate lookup, not introduced by this PR.	File minor bug to deduplicate in handleRunDetailPage.

---

Generated by Wave pr-fix-review pipeline

[nextlevelshit]

Review Resolution Summary

Total findings: 10 | Accepted & fixed: 0 | Rejected: 6 | Deferred: 4

Rejected (6)

#	Finding	Rationale
1	Removed //nolint:errcheck from RunHooks without handling errors	False positive — executor.go is not in the PR diff
2	stripExcludedDeps does not strip outgoing Edges	Already fixed in 4ec8b8b — stripExcludedDeps now filters Edges targeting excluded steps
3	Missing test for empty excluded set	Already fixed in 4ec8b8b — TestStripExcludedDeps_EmptyExcluded added
4	Missing test for Edges referencing excluded steps	Already fixed in 4ec8b8b — TestComputeDAGLayout_EdgesSkipMissingNodes added
5	Rework-only filtering logic duplicated between handlers	Premature abstraction — shared pattern is ~4 lines but DAGStepInput construction differs significantly (5 vs 15+ fields)
6	Test uses rework-only step with zero dependents	Already covered by TestStripExcludedDeps which verifies dependency stripping for steps referencing excluded steps

Deferred (4)

#	Finding	Severity	Reason	Follow-up
1	handleAPIUpdateWebhook missing isPublicURL() validation (SSRF)	minor	Pre-existing — handlers_webhooks.go not modified by this PR	File security issue to add isPublicURL validation on webhook update
2	Webhook secret returned in API responses	minor	Pre-existing — handlers_webhooks.go not modified by this PR	File security issue to redact Secret field after creation
3	JSON API still exposes rework_only steps	suggestion	Design decision requiring product input — out of scope	Discuss whether API should filter rework_only steps
4	Failure class looked up twice for failed steps	minor	Pre-existing duplicate at handlers_runs.go:756-783 — not introduced by this PR	File minor bug to remove duplicate GetStepAttempts call

Result

No code changes required for this PR. Three suggestions were already addressed in commit 4ec8b8b, three were rejected as not applicable, and four pre-existing/out-of-scope issues are deferred for separate tracking.