Skip to content

ci: guard the edit-accuracy gate against a failed changes job - #5182

Merged
jrusso1020 merged 1 commit into
heygen-com:mainfrom
egnaro9:guard-edit-accuracy-gate
Oct 26, 2026
Merged

jrusso1020 merged 1 commit into
heygen-com:mainfrom
egnaro9:guard-edit-accuracy-gate

Conversation

@egnaro9

@egnaro9 egnaro9 commented Oct 7, 2026

Copy link
Copy Markdown

Studio: edit accuracy gate is a required status check on main, and it is the only gated required check without a needs.changes.result guard.

If the changes job fails, its outputs are unset, needs.changes.outputs.edit_accuracy == 'true' is false, the job skips, and GitHub reports a skipped job as Success. The required check is satisfied with nothing run.

changes can fail. It runs node scripts/check-no-main-deletions.mjs --base origin/main, which exists to fail.

Six sibling required checks already handle exactly this:

Build                   !cancelled() && (needs.changes.result != 'success' || ...)
Typecheck               !cancelled() && (needs.changes.result != 'success' || ...)
Test                    always()     && (needs.changes.result != 'success' || ...)
Test: runtime contract  !cancelled() && (needs.changes.result != 'success' || ...)
Render on windows-latest   same shape
Tests on windows-latest    same shape

This applies the same guard to the edit-accuracy gate. When changes succeeds the condition is unchanged, so there is no difference on the happy path; the only new behaviour is that a failed changes no longer silently satisfies the gate.

Found with deadgate, which reads workflow files for checks that structurally cannot fail and then resolves each one against the branch's required checks. It flags other jobs here on the same shape, but per the public rulesets API those are not required checks, so they are hygiene rather than a merge-gate hole and I have left them out of this PR.

@egnaro9

egnaro9 commented Oct 9, 2026

Copy link
Copy Markdown
Author

Re-verified the premise today against the live rulesets API: Studio: edit accuracy gate is still a required status check on main, and ci.yml:1113 still carries the unguarded condition.

Worth stating the mechanism without reference to any tool, because GitHub documents all of it. From Handling skipped but required checks, three cases with three different outcomes:

how the job skips what the required check does
workflow bypassed by a path or branch filter checks stay Pending and block merging
job skipped by its own if: conditional the job reports Success
job skipped because a job it needs failed the dependent is skipped and may not block merging

The !cancelled() already in this job's condition is what decides which row applies, and it is the part that makes this a gate hole rather than a non-issue.

Without it, a failed changes would skip this job through the third row. With it, the conditional is evaluated instead: needs.changes.outputs.edit_accuracy on a failed job is the empty string, == 'true' is false, and the job skips by its own if:. That is the second row, where GitHub's documented behaviour is not "may not block" but reports Success. A required check reporting Success with nothing run.

So the failure needs no unusual conditions. changes runs scripts/check-no-main-deletions.mjs, a script whose purpose is to fail, and on the run where it fails the accuracy gate goes green.

The six sibling required checks already encode exactly this, each pairing !cancelled() or always() with a needs.changes.result test. This PR applies the same pairing to the one that is missing it. On the happy path the condition is unchanged.

One thing I should correct from the description above: it says the finding came from deadgate, and the detector that produced it has since been removed from that tool. I measured its output across 275 repositories and about 80% of what it reported was redundant with GitHub's needs-skip rule, the third row in the table, where the dependent skips anyway and the claim adds nothing. Deleting it was the right call on volume.

This repository is in the remainder, and !cancelled() is the reason. I am flagging that rather than leaving a citation to a tool that would no longer reproduce its own finding. The argument above does not depend on it either way: it is GitHub's documented behaviour, this job's existing condition, and the pattern already used by its six siblings.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the careful write-up. The hole is still on main: "Studio: edit accuracy gate" is still a required check, and its condition is still !cancelled() && needs.changes.outputs.edit_accuracy == 'true' with no needs.changes.result guard. A failed changes job (for example the accidental-deletion check) makes the job skip on its own if:, and that counts as a pass for a required check. Build and the other required jobs already use the needs.changes.result != 'success' || guard.

I also checked what the gate does once your guard lets it run after a failed changes. The shard jobs still skip, so no results are downloaded, ratchet.mjs gate reports "0 of N shards reported results", and the final step fails the job. So it fails closed, which is what you want.

This needs a rebase. ci.yml conflicts because main has since reworked this job: it now also needs the retry job, and the github.event_name == 'pull_request' clause is gone (the gate runs in the merge queue too). On current main the condition would become:

if: ${{ !cancelled() && (needs.changes.result != 'success' || needs.changes.outputs.edit_accuracy == 'true') }}

The commit also needs signing, since main requires signed commits. CI has not run on this fork branch yet, so there is no CI result to go on. Happy to see this approved once it is rebased.

Verdict: COMMENT
Reasoning: The fix is correct and still needed, but the branch conflicts with the reworked job on main and has no CI run yet.

@egnaro9 egnaro9 closed this Oct 25, 2026
@egnaro9
egnaro9 force-pushed the guard-edit-accuracy-gate branch from 6f2fc62 to f180ffc Compare October 25, 2026 00:37
@egnaro9 egnaro9 reopened this Oct 25, 2026
@egnaro9

egnaro9 commented Oct 25, 2026

Copy link
Copy Markdown
Author

Thanks, Miguel. Rebased onto current main with your condition exactly as written, and the commit is now signed. Sorry for the brief close and reopen while I reset the branch. CI on the fork needs a maintainer's approval to run; ready whenever you are.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the rebase. This head is exactly the condition from my last review, on top of current main, and the commit is signed. The diff against main is now just the one if: line on "Studio: edit accuracy gate", and the gate still fails closed when changes fails (no shard results, so the final step fails the job).

CI hasn't run on this fork branch yet; it needs a maintainer to approve the workflow run before the required checks can report.

Reviewed head 3c98c1c8.

Verdict: COMMENT
Reasoning: The rebased one-line guard matches the pattern the other required jobs use and closes the skipped-equals-success hole; I'll approve once the workflow run is approved and green.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved. The edit-accuracy gate now runs when change detection fails, so a failed changes job can't silently satisfy the required check. Unrelated changes and cancelled runs behave as before, and a zero-report run exits 2 and is rejected by the final step.

@jrusso1020
jrusso1020 added this pull request to the merge queue Oct 25, 2026
Merged via the queue into heygen-com:main with commit c887643 Oct 26, 2026
65 checks passed
egnaro9 added a commit to egnaro9/egnaro9.github.io that referenced this pull request Oct 26, 2026
jrusso1020 merged it through the merge queue on 2026-10-26 00:58 UTC, after approving it. The résumé's deadgate line now ends ", fixed by my merged PR #5182", and the PDF is rebuilt from the same print template as 0aa0337: a word diff against the previous PDF shows only that clause added, still 2 pages, US Letter, no phone. The findings entry's status reads MERGED, the same markup findings-ledger's generate.py emits for it now.

findings/ was not regenerated from findings-ledger: the deployed page carries corrections the ledger source does not have yet (the ruff outcome, corrected test counts, the light-palette scoping, the Orchestration link), and a full regeneration would undo them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants