fix(cli): stats never hides a failed task behind a cancelled sibling - #1213
Merged
Merged
Conversation
workflowCancelled treated a missing Smithers state as the mark of a fan-out aggregate, so it also skipped a failed task record that had no state. A fan-out node with one cancelled task and one such failed task was then reported canceled, and the failure dropped out of totals.status_counts. That contradicts the documented rule that a node is canceled only when Smithers cancelled every failed task of it. Skip only records that carry aggregate_attempt_statuses, and count any other failed record without a state as not cancelled. No writer on main produces such a task record today (Greptile, on #1200), so this is a correctness fix for the rule rather than for an observed run. Co-Authored-By: Claude Opus 5.5 <[email protected]>
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.
Follow-up to #1200, from Greptile's P1 on #1200.
Problem
statsreports a failed node ascanceledwhen Smithers cancelled every failed task of it (#1087, #1200).workflowCancelledinpackages/cli/src/run-statistics.tsused a missing Smithersstateas the mark of a fan-out aggregate record. So it also skipped a failed task record without a state. For a fan-out node with one cancelled task and one failed task without a state, it saw onlycancelledand reported the nodecanceled. The failure disappeared fromtotals.status_counts, which contradictsdocs/reference/cli.md("When Smithers cancelled every failed task of a node").A triage of the finding found no writer on
main, or in past releases, that records a failed task without a state. So this fixes the rule, not an observed run.Change
aggregate_attempt_statuses(the fan-out aggregate).failed.Verification
stats counts a failed node whose task Smithers cancelled as canceledgains the mixed case: a cancelled task plus a failed task without a state. Onorigin/mainit fails withactual: ['canceled', 0, 1],expected: ['failed', 1, 0]. It passes here.run-statistics.test.ts: 18/18.lint:strict:ciand CLItsc --noEmitpass.Risk
A failed record without a state or provenance that is not an aggregate now keeps its node
failed, as the documentation says. The only records skipped are aggregates, and they always carryaggregate_attempt_statuses(required by the run-state schema).🤖 Generated with Claude Code
The PR appears safe to merge.
Summary
The PR changes
statsto exclude fan-out aggregate records explicitly when deciding whether Smithers canceled every failed task. It adds a mixed canceled/stateless-failure test and documents the correction in the changelog.Reviews (1) · Last reviewed commit: "fix(cli): stats never hides a failed tas..."