fix(desktop): stop cancelled desktop CI runs reporting as failures - #91373
fix(desktop): stop cancelled desktop CI runs reporting as failures#91373posthog[bot] wants to merge 1 commit into
Conversation
The five desktop `*Pass` gates run with `if: always()`, so they run during a cancel-in-progress cancellation and read their cancelled dependencies as failures. A superseded run then posts a red required check with no test failure behind it. Add a `Note run cancellation` step guarded by `if: cancelled()` that writes a flag to $GITHUB_ENV, then short-circuit the `Check results` step to exit 0 when the flag is set. cancelled() is true only for a whole-run cancel, so a job-level cancel or timeout leaves the run uncancelled and still reaches the per-dependency guards. The gate keeps `if: always()` and every guard, so WF007 still passes. Generated-By: PostHog Desktop Task-Id: 2fd66c79-a5de-4c09-9913-72c7fa7e82ae
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🦔 PostHog Review reviewed this pull requestFound 2 must fix, 0 should fix, 0 consider. Published 2 findings (view the review). Resolved comments: 2 left for you |
🤖 CI report
|
|
Gates denied this PR (CI workflow changes hit the infra/CI deny-list and tier classification), and it modifies required-gate pass/fail logic across five CI workflows with zero human or agent reviews — that's risky CI/infra territory with no independent assurance.
Gate mechanics and policy version
|
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| The cost of that `always()` gate is a red required check on every superseded run: the gate runs during the cancel, reads its cancelled dependencies, and fails. | ||
| That red sits on a dead SHA — the newer commit that superseded it runs its own gate — but it still reads as a hard failure and trains people to reflex-rerun. | ||
| To drop it without opening a false-green hole, add a `Note run cancellation` step guarded by `if: ${{ cancelled() }}` that writes `RUN_CANCELLED=true` to `$GITHUB_ENV`, then short-circuit the `Check results` step to `exit 0` when that flag is set (the desktop suite gates do this). | ||
| `cancelled()` is true only for a whole-run cancel, so a job-level cancel or timeout leaves the run uncancelled and still reaches the per-dependency guards. | ||
| The `Check results` step keeps no `if:` and keeps every guard, so `WF007` still sees each dependency reach a fail-closed guard. |
There was a problem hiding this comment.
Guidance publishes an unsafe required-gate recipe
Why we think it's a valid issue
- Checked: the five gates the PR edits, the ruleset comment that names the required check, the merge_group path, the WF007 linter source, and existing run-cancel automation in the repo.
- Found:
.github/workflows/desktop-ci.yml:105-113writesRUN_CANCELLED=trueon${{ cancelled() }}and thenexit 0before every dependency guard. GitHub'scancelled()reports that the run was cancelled; it carries no cause, so a person clicking Cancel, an API/gh run cancelcall, and acancel-in-progresssupersede are indistinguishable inside the expression. The PR's stated property, "Only a supersede — which lands on a dead SHA — passes", is therefore false. - Found: the gate's own conclusion drives the required check. The PR's premise is that a cancelled run posts a red
Desktop * Pass, which only happens because the gate job exits nonzero while the run conclusion iscancelled. By the same path,exit 0posts green..github/workflows/desktop-ci.yml:91-93callsDesktop Tests Pass"the one check name to require in the master ruleset". - Found: nothing re-tests the code later.
.github/workflows/desktop-ci.yml:15-19and the child conditions (for example.github/workflows/desktop-build.yml:67) skip every real job onmerge_group, and the gate treatsskippedas success. The PR run is the only place desktop tests execute. - Found: the repo already cancels its own runs on purpose.
.github/workflows/ci-backend.yml:1348and:2375rungh run cancel ${{ github.run_id }}after a deterministic repo-check or OpenAPI failure. If the published recipe reaches a gate in that workflow, those deliberate hard failures convert to a green required check. - Found: WF007 cannot catch this.
tools/hogli-commands/hogli_commands/workflow_lint/checks/required_gates.py:332-345only asks whether eachneeds.<dep>.resulttextually reaches a guard inside some step; it models no unconditional earlyexit 0that makes those guards unreachable. Its docstring names the exact outcome it exists to block — "green, with zero tests run" — which this recipe reintroduces by a different route. So the PR's "WF007 still passes" evidence proves the guards are present, not that they run. - Impact: a manual or scripted cancel on a live head SHA leaves
Desktop Tests Passgreen with no desktop build, lint, typecheck, or test executed, and the merge queue does not repeat that work. The change converts a fail-closed merge gate into a fail-open one, and.agents/skills/authoring-ci-workflows/SKILL.md:110-112publishes it as the general pattern for required-check gates. - Priority: raised to
must_fix. The reviewer framed the risk as future copying of unsafe guidance, but the bypass is already live in five gates in this PR, it defeats a required merge gate, and an ordinary human action triggers it.
Issue description
This skill presents the cancellation short-circuit as fail-closed guidance. GitHub defines cancelled() for any workflow cancellation, including a manual cancellation. The recipe can mark an untested required gate green. Future CI authors can copy this bypass into other workflows.
Suggested fix
Remove this recipe until the workflow can distinguish a superseded run from other cancellations. Document the broad cancelled() behavior. Extend WF007 before approving any early success path that bypasses dependency guards.
Prompt to fix with AI (copy-paste)
## Context
@.agents/skills/authoring-ci-workflows/SKILL.md#L108-112
<issue_description>
This skill presents the cancellation short-circuit as fail-closed guidance. GitHub defines `cancelled()` for any workflow cancellation, including a manual cancellation. The recipe can mark an untested required gate green. Future CI authors can copy this bypass into other workflows.
</issue_description>
<issue_validation>
- **Checked:** the five gates the PR edits, the ruleset comment that names the required check, the merge_group path, the WF007 linter source, and existing run-cancel automation in the repo.
- **Found:** `.github/workflows/desktop-ci.yml:105-113` writes `RUN_CANCELLED=true` on `${{ cancelled() }}` and then `exit 0` before every dependency guard. GitHub's `cancelled()` reports that the run was cancelled; it carries no cause, so a person clicking Cancel, an API/`gh run cancel` call, and a `cancel-in-progress` supersede are indistinguishable inside the expression. The PR's stated property, "Only a supersede — which lands on a dead SHA — passes", is therefore false.
- **Found:** the gate's own conclusion drives the required check. The PR's premise is that a cancelled run posts a *red* `Desktop * Pass`, which only happens because the gate job exits nonzero while the run conclusion is `cancelled`. By the same path, `exit 0` posts green. `.github/workflows/desktop-ci.yml:91-93` calls `Desktop Tests Pass` "the one check name to require in the master ruleset".
- **Found:** nothing re-tests the code later. `.github/workflows/desktop-ci.yml:15-19` and the child conditions (for example `.github/workflows/desktop-build.yml:67`) skip every real job on `merge_group`, and the gate treats `skipped` as success. The PR run is the only place desktop tests execute.
- **Found:** the repo already cancels its own runs on purpose. `.github/workflows/ci-backend.yml:1348` and `:2375` run `gh run cancel ${{ github.run_id }}` after a deterministic repo-check or OpenAPI failure. If the published recipe reaches a gate in that workflow, those deliberate hard failures convert to a green required check.
- **Found:** WF007 cannot catch this. `tools/hogli-commands/hogli_commands/workflow_lint/checks/required_gates.py:332-345` only asks whether each `needs.<dep>.result` textually reaches a guard inside some step; it models no unconditional early `exit 0` that makes those guards unreachable. Its docstring names the exact outcome it exists to block — "green, with zero tests run" — which this recipe reintroduces by a different route. So the PR's "WF007 still passes" evidence proves the guards are present, not that they run.
- **Impact:** a manual or scripted cancel on a live head SHA leaves `Desktop Tests Pass` green with no desktop build, lint, typecheck, or test executed, and the merge queue does not repeat that work. The change converts a fail-closed merge gate into a fail-open one, and `.agents/skills/authoring-ci-workflows/SKILL.md:110-112` publishes it as the general pattern for required-check gates.
- **Priority:** raised to `must_fix`. The reviewer framed the risk as future copying of unsafe guidance, but the bypass is already live in five gates in this PR, it defeats a required merge gate, and an ordinary human action triggers it.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Remove this recipe until the workflow can distinguish a superseded run from other cancellations. Document the broad `cancelled()` behavior. Extend WF007 before approving any early success path that bypasses dependency guards.
</potential_solution>
There was a problem hiding this comment.
Confirmed — this is a real must_fix, and it's inseparable from the workflow change it documents, so I'm escalating rather than editing the skill in isolation.
What I verified: GitHub's cancelled() is true for any whole-run cancellation and carries no cause. A superseded run (cancel-in-progress), a person clicking Cancel, gh run cancel, and an API cancel all set it. So the recipe's premise that 'only a supersede passes' does not hold: a manual or scripted cancel on a live head SHA also hits the exit 0 short-circuit before any dependency guard runs, leaving Desktop Tests Pass green with no desktop build, quality, typecheck, or test executed — and on merge queue entries the real jobs are skipped, so nothing re-runs them. That converts a fail-closed required gate into a fail-open one. The skill then publishes this as the general pattern, which is riskier still given the repo already self-cancels on purpose in other workflows.
Why I'm not auto-fixing it: a correct fix has to change the five .github/workflows/** gate files, not just this doc — editing the skill alone would leave the hole live and the PR self-contradictory. Workflow files are outside what this automated fixer may change, and the right fix is a design decision, not a mechanical one: either drop the cancelled() short-circuit, or add a discriminator that GitHub doesn't expose natively (for example comparing the run's head SHA against the branch head to tell a supersede from a live cancel), likely alongside a WF007 extension so the early exit 0 can't bypass the guards unnoticed. Its correctness can only be proven by a real superseded-run race on GitHub, which this PR did not run.
What a human needs to decide: whether to abandon the short-circuit approach or how to safely distinguish a supersede from other cancellations — and only then what this skill should document. This shares its root cause with the companion thread on desktop-ci.yml.
| # A superseded run (cancel-in-progress) cancels its jobs. That is not a | ||
| # test failure, so note it and let the check below pass instead of | ||
| # posting a red required check. cancelled() is true only for a whole-run | ||
| # cancel, so a job-level cancel or timeout still reaches the guards. | ||
| - name: Note run cancellation | ||
| if: ${{ cancelled() }} | ||
| run: echo "RUN_CANCELLED=true" >> "$GITHUB_ENV" | ||
| - name: Check results | ||
| run: | | ||
| if [[ "${RUN_CANCELLED:-}" == "true" ]]; then | ||
| echo "Run was cancelled (superseded); not treating it as a failure." | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Manual cancellation can make required gates pass
Why we think it's a valid issue
- Checked: the umbrella gate and all four child gates, the ruleset comment naming the required check, the
merge_grouppath, and the WF007 linter that the PR cites as proof of safety. - Found: the short-circuit runs before every allowlist guard.
.github/workflows/desktop-ci.yml:110-113exits zero ahead of theneeds.build,needs.quality,needs.typecheck,needs.test, andneeds.backend-couplingchecks at lines 114-133.cancelled()reports only that the run was cancelled; it carries no cause, so a Cancel click, agh run cancel, and acancel-in-progresssupersede are identical inside the expression. - Found: the gate job's own conclusion is what branch protection reads, so
exit 0posts green. The PR's own premise proves this: it reports that today a cancelled run posts a redDesktop * Pass, which can only come from the gate exiting nonzero while the run conclusion iscancelled..github/workflows/desktop-ci.yml:91-93callsDesktop Tests Pass"the one check name to require in the master ruleset". - Found: the child-gate claim holds. The same three lines appear verbatim at
.github/workflows/desktop-build.yml:139-141,.github/workflows/desktop-quality.yml:117-119,.github/workflows/desktop-typecheck.yml:136-138, and.github/workflows/desktop-test.yml:363-365, each ahead of its guards. The children are reusable workflows called from.github/workflows/desktop-ci.yml:32-49, so their jobs belong to the caller's run and one cancel flips all five gates green together. - Found: nothing re-tests the code afterwards.
.github/workflows/desktop-ci.yml:15-19and the child conditions (for example.github/workflows/desktop-build.yml:67) skip every real job onmerge_group, and the gates treatskippedas success. The PR run is the only place desktop build, lint, typecheck, and tests execute. - Found: WF007 cannot detect this.
tools/hogli-commands/hogli_commands/workflow_lint/checks/required_gates.py:332-345only asks whether eachneeds.<dep>.resulttextually reaches a guard inside a step; it models no unconditional earlierexit 0that makes those guards unreachable. Its docstring names the outcome it exists to block, "green, with zero tests run", which this recipe reintroduces by a new route. The PR's "WF007 passes" evidence therefore proves the guards exist, not that they run. - Impact: an ordinary human Cancel on the live head SHA leaves all five
Desktop * Passrequired checks green with no desktop job executed, and no replacement run exists to re-report. A fail-closed merge gate becomes fail-open, and the PR's stated safety property, that only a supersede on a dead SHA passes, does not hold. - Impact: this overlaps the SKILL.md finding on the same recipe, but it is the better-anchored one: it names the workflow where the bypass runs and the four child gates that repeat it.
Issue description
cancelled() is true for every workflow cancellation, not only a cancel-in-progress supersede. A user can cancel the current head's run. This step then sets RUN_CANCELLED, and Check results exits zero before it checks the canceled dependencies. The required Desktop Tests Pass check can therefore turn green although no replacement run exists. The same bypass exists in the four child gates added by this change.
Suggested fix
Only short-circuit after the workflow proves that a newer run superseded this run. For example, read the pull request's live head SHA and compare it with github.event.pull_request.head.sha. Treat a same SHA, a non-PR event, or an API failure as a normal cancellation. In those cases, keep the existing dependency guards fail-closed. Apply the same condition to every desktop gate, update the skill text, and add a WF007 regression test for bare cancelled() short-circuits.
Prompt to fix with AI (copy-paste)
## Context
@.github/workflows/desktop-ci.yml#L101-113
<issue_description>
`cancelled()` is true for every workflow cancellation, not only a `cancel-in-progress` supersede. A user can cancel the current head's run. This step then sets `RUN_CANCELLED`, and `Check results` exits zero before it checks the canceled dependencies. The required `Desktop Tests Pass` check can therefore turn green although no replacement run exists. The same bypass exists in the four child gates added by this change.
</issue_description>
<issue_validation>
- **Checked:** the umbrella gate and all four child gates, the ruleset comment naming the required check, the `merge_group` path, and the WF007 linter that the PR cites as proof of safety.
- **Found:** the short-circuit runs before every allowlist guard. `.github/workflows/desktop-ci.yml:110-113` exits zero ahead of the `needs.build`, `needs.quality`, `needs.typecheck`, `needs.test`, and `needs.backend-coupling` checks at lines 114-133. `cancelled()` reports only that the run was cancelled; it carries no cause, so a Cancel click, a `gh run cancel`, and a `cancel-in-progress` supersede are identical inside the expression.
- **Found:** the gate job's own conclusion is what branch protection reads, so `exit 0` posts green. The PR's own premise proves this: it reports that today a cancelled run posts a *red* `Desktop * Pass`, which can only come from the gate exiting nonzero while the run conclusion is `cancelled`. `.github/workflows/desktop-ci.yml:91-93` calls `Desktop Tests Pass` "the one check name to require in the master ruleset".
- **Found:** the child-gate claim holds. The same three lines appear verbatim at `.github/workflows/desktop-build.yml:139-141`, `.github/workflows/desktop-quality.yml:117-119`, `.github/workflows/desktop-typecheck.yml:136-138`, and `.github/workflows/desktop-test.yml:363-365`, each ahead of its guards. The children are reusable workflows called from `.github/workflows/desktop-ci.yml:32-49`, so their jobs belong to the caller's run and one cancel flips all five gates green together.
- **Found:** nothing re-tests the code afterwards. `.github/workflows/desktop-ci.yml:15-19` and the child conditions (for example `.github/workflows/desktop-build.yml:67`) skip every real job on `merge_group`, and the gates treat `skipped` as success. The PR run is the only place desktop build, lint, typecheck, and tests execute.
- **Found:** WF007 cannot detect this. `tools/hogli-commands/hogli_commands/workflow_lint/checks/required_gates.py:332-345` only asks whether each `needs.<dep>.result` textually reaches a guard inside a step; it models no unconditional earlier `exit 0` that makes those guards unreachable. Its docstring names the outcome it exists to block, "green, with zero tests run", which this recipe reintroduces by a new route. The PR's "WF007 passes" evidence therefore proves the guards exist, not that they run.
- **Impact:** an ordinary human Cancel on the live head SHA leaves all five `Desktop * Pass` required checks green with no desktop job executed, and no replacement run exists to re-report. A fail-closed merge gate becomes fail-open, and the PR's stated safety property, that only a supersede on a dead SHA passes, does not hold.
- **Impact:** this overlaps the SKILL.md finding on the same recipe, but it is the better-anchored one: it names the workflow where the bypass runs and the four child gates that repeat it.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Only short-circuit after the workflow proves that a newer run superseded this run. For example, read the pull request's live head SHA and compare it with `github.event.pull_request.head.sha`. Treat a same SHA, a non-PR event, or an API failure as a normal cancellation. In those cases, keep the existing dependency guards fail-closed. Apply the same condition to every desktop gate, update the skill text, and add a WF007 regression test for bare `cancelled()` short-circuits.
</potential_solution>
There was a problem hiding this comment.
Confirmed, and this is the better-anchored twin of the skill-file finding — same root cause, pointed at the code where the bypass runs. I'm escalating rather than editing, because the fix has to change CI workflow files and pick a design, neither of which this automated fixer does unattended.
What I verified: the exit 0 short-circuit runs ahead of every allowlist guard in the umbrella gate (desktop-ci.yml:110-113), and the identical step is repeated in all four child gates before their guards (desktop-build.yml:136/139, desktop-quality.yml:114/117, desktop-typecheck.yml:133/136, desktop-test.yml:360/363). GitHub's cancelled() is true for any whole-run cancellation and carries no cause, so a plain Cancel click or gh run cancel on the live head SHA trips it exactly like a supersede. The gate job's own success is what the ruleset reads, so all five Desktop * Pass checks go green together with no desktop job run, and merge-queue entries skip the real jobs — nothing re-tests it.
What a human needs to decide: whether to drop the cancelled() short-circuit entirely, or add a real supersede discriminator (for example fetch the PR's current head SHA and only short-circuit when it differs from github.event.pull_request.head.sha, treating a same SHA, a non-PR event, or an API failure as a normal cancellation), likely with a WF007 extension so an early exit can't bypass the guards unnoticed. Proving it holds needs a live cancel-vs-supersede run on GitHub. Because it touches .github/workflows/** and changes the merge gate's safety property, it needs human review — track it together with the skill-file thread, which documents the same recipe.
Problem
Desktop * Passrequired check with no test failure behind it, so people reflex-rerun a gate that never failed.desktop-ci,desktop-build,desktop-quality,desktop-test,desktop-typecheck) run withif: always(), so they run during acancel-in-progresscancel.!= success && != skippedand exits nonzero, turning a superseded run into a hard red.Changes
Desktop * Passchecks green instead of red, so there is nothing false to rerun.Note run cancellationstep guarded byif: ${{ cancelled() }}that writesRUN_CANCELLED=trueto$GITHUB_ENV.Check resultsstep exits 0 when that flag is set, before the per-dependency guards run.cancelled()is true only for a whole-run cancel, so a job-level cancel or timeout leaves the run uncancelled and still fails the gate.if: always()and every guard unchanged, so the WF007 required-gate lint still passes and genuine failures still report red.authoring-ci-workflowsskill documents the short-circuit, since it previously stated a cancelled run should fail the gate.Note
This keeps the team's fail-closed property. A required job that genuinely cancels or times out does not set run-level
cancelled(), so the guards still catch it. Only a supersede — which lands on a dead SHA — passes.How did you test this code?
bin/hogli lint:workflows(WF007 required-gate check): 0 issues on the five desktop workflows.actionlinton the five workflows: clean.test_workflow_lint.py): 118 passed.cancelled()being whole-run only (documented) and on the guards staying statically present for WF007 (verified by the linter).Automatic notifications
Docs update
Updated
.agents/skills/authoring-ci-workflows/SKILL.mdto describe the run-cancellation short-circuit.🤖 Agent context
Autonomy: Fully autonomous
authoring-ci-workflowsskill and read theWF007linter source before choosing an approach.if: !cancelled()on the gate fails WF007, and addingcancelledto the allowlist both fails WF007 and risks passing a genuinely timed-out required job.cancelled()short-circuit because it distinguishes a whole-run supersede from a job-level cancel or timeout, which is the distinction the report asked for./authoring-ci-workflows,/writing-pr-descriptions,/writing-code-comments,/writing-user-facing-copy,/writing-simplified-technical-english.Created with PostHog Desktop from this inbox report.