Skip to content

fix(desktop): stop cancelled desktop CI runs reporting as failures #91373

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
posthog wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

fix(desktop): stop cancelled desktop CI runs reporting as failures #91373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .agents/skills/authoring-ci-workflows/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,12 @@ Adding another predicate can skip the required check, so `always() && <condition
`!cancelled()` is identical to `always()` on any run that is not cancelled, so failure-path reporting still works; only cancelled runs skip.
Measured on a live superseded run ([evidence](https://github.com/PostHog/posthog/actions/runs/29765284128)): an `always()` worker dispatched and ran to completion _after_ the cancel, while the `!cancelled()` worker never started and reported `cancelled` (not `skipped`), so the gate still fails closed.

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.
Comment on lines +108 to +112

@posthog posthog Bot Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guidance publishes an unsafe required-gate recipe

must_fix bug

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-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 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>

@posthog posthog Bot Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Four rules for the gate body:

1. **Allowlist every dependency, never denylist.** Assert `success`/`skipped` and fail everything else.
Expand Down
11 changes: 11 additions & 0 deletions .github/workflows/desktop-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,19 @@ jobs:
timeout-minutes: 5
if: always()
steps:
# 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
if [[ "${{ needs.changes.result }}" != "success" && "${{ needs.changes.result }}" != "skipped" ]]; then
echo "Change detection did not succeed (result: ${{ needs.changes.result }})."
exit 1
Expand Down
11 changes: 11 additions & 0 deletions .github/workflows/desktop-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,19 @@ jobs:
runs-on: depot-ubuntu-24.04
timeout-minutes: 5
steps:
# 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
Comment on lines +101 to +113

@posthog posthog Bot Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manual cancellation can make required gates pass

must_fix bug

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_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 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>

@posthog posthog Bot Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if [[ "${{ needs.build.result }}" != "success" && "${{ needs.build.result }}" != "skipped" ]]; then
echo "Desktop build did not succeed (result: ${{ needs.build.result }})."
exit 1
Expand Down
11 changes: 11 additions & 0 deletions .github/workflows/desktop-quality.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,19 @@ jobs:
timeout-minutes: 5
if: always()
steps:
# 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
if [[ "${{ needs.changes.result }}" != "success" && "${{ needs.changes.result }}" != "skipped" ]]; then
echo "Change detection did not succeed (result: ${{ needs.changes.result }})."
exit 1
Expand Down
11 changes: 11 additions & 0 deletions .github/workflows/desktop-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,19 @@ jobs:
timeout-minutes: 5
if: always()
steps:
# 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
if [[ "${{ needs.changes.result }}" != "success" && "${{ needs.changes.result }}" != "skipped" ]]; then
echo "Change detection did not succeed (result: ${{ needs.changes.result }})."
exit 1
Expand Down
11 changes: 11 additions & 0 deletions .github/workflows/desktop-typecheck.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,19 @@ jobs:
timeout-minutes: 5
if: always()
steps:
# 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
if [[ "${{ needs.changes.result }}" != "success" && "${{ needs.changes.result }}" != "skipped" ]]; then
echo "Change detection did not succeed (result: ${{ needs.changes.result }})."
exit 1
Expand Down
Loading