Skip to content

Get main green, and stop Dependabot merging before CI reports [patch] - #234

Merged
matt-edmondson merged 3 commits into
mainfrom
claude/main-branch-red-status-49d2ti
Sep 15, 2026
Merged

matt-edmondson merged 3 commits into
mainfrom
claude/main-branch-red-status-49d2ti

Conversation

@matt-edmondson

@matt-edmondson matt-edmondson commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Three commits: the fix for what turned main red, the gate that stops it recurring, and the security fix Sonar found in that gate.

1. Assert the Python projection fix the pin caught

SevenTargetProjectionTests.PythonNamesTheClassInsideItsOwnBases asserted that ktsu.Coder writes class Length(IVector0[Length[T], T]) — a base list naming the class being declared, which Python evaluates eagerly and so raises NameError on import. That was never the desired output; the test pinned the defect on purpose, so that fixing it upstream would fail here rather than pass unnoticed.

ktsu-dev/Coder#64 is now fixed. Coder writes the string forward reference, which is the idiom Python has for exactly this:

- class Length(IVector0[Length[T], T])     # NameError on import
+ class Length(IVector0["Length[T]", T])   # valid

So the red on main was the pin doing its job, not a regression. The bump from ktsu.Coder 3.14.0 → 3.14.3 in #233 carried the fix in, and this is the update the pin's own remark said to make when that day came.

The test is renamed to PythonQuotesTheClassInsideItsOwnBases and asserts the quoted form. Keeping it rather than deleting it pins the fix the same way round — a regression upstream fails here instead of shipping a module that cannot be imported.

Go is untouched: GoSpellsAGenericTypeItDidNotDeclare still passes, so ktsu-dev/Coder#63 is still open and still pinned.

CLAUDE.md moves the probe's answer from five of seven to six of seven, drops Python from the table of targets whose toolchain refuses the output, and records what happened — the pin is how the fix was noticed at all, which is the argument for pinning rather than skipping.

2. Merge Dependabot PRs when CI is green, not when nothing is required

dependabot-merge.yml ran on: pull_request and called gh pr merge --auto. That flag reads like "merge when green", but it means "merge when branch protection is satisfied" — it delegates the waiting entirely to required status checks. main is unprotected and declares none, so the condition was vacuously true the instant the PR opened.

Time (UTC) Event
04:18:08 dependabot opens #233
04:18:15 Process Dependabot PR runs gh pr merge --auto
04:18:20 PR merged by github-actions[bot]
04:18:34 the three Test on … jobs start
04:21:35 Test on ubuntu fails

The tests hadn't started when the merge landed. And because github-actions[bot] merged with GITHUB_TOKEN, the push triggered no workflow on main either, so the failure stayed invisible until a manual dispatch 34 minutes later.

Branch protection stays off; the definition of green moves into the workflow. Firing on workflow_run rather than pull_request is what makes that possible: the merge is decided after CI reports. That also avoids the trap in the obvious fix — gh pr checks --watch deadlocks, because it waits on this job's own in-progress check run — and holds no runner for the length of the build.

The triggering run being green is necessary but not sufficient, so the commit is asked what every check says before merging:

  • Anything still running is a reason to wait, not to merge. Whichever watched workflow finishes last fires it again and the merge happens on that pass.
  • A check that is absent isn't counted — Verify Generated Files skips a markdown-only bump by design.
  • skipped and neutral count as green: dotnet.yml skips Analyze & Release and Security Scanning on a PR, and CodeQL reports neutral.

dependabot/fetch-metadata is dropped rather than ported: its gate accepted semver-major, -minor and -patch — every value it can produce — so it filtered nothing. Behaviour is unchanged. Restricting which bumps may auto-merge is a real option but a separate decision.

3. Decide whose PR it is from the API, not the event payload

SonarCloud reported githubactions:S8232 as a BLOCKER on commit 2's if:, and it was right.

github.event.workflow_run.actor.login == 'dependabot[bot]' &&
startsWith(github.event.workflow_run.head_branch, 'dependabot/')

Neither value is an authenticated claim. A re-run re-attributes actor to whoever pressed the button, and a branch is named by whoever pushes it. Those two strings were the entire barrier between an arbitrary pull request and an automatic merge holding contents: write.

The payload now only locates the pull request; GitHub is asked who opened it, and .user.login from the REST representation decides. A PR on a dependabot/-prefixed branch opened by anyone else is left alone.

Matching on the head SHA rather than the branch name also closes a hole this workflow exists to close. The only pull request that can match is one whose head is the exact commit CI just reported on. A commit pushed after CI finished moves the head, nothing matches, and nothing is merged — where the branch lookup would have merged code CI never saw, which is #233's failure by another route.

The SHA is checked to be a hex digest before it reaches a jq filter, so nothing from the payload arrives as syntax. The if: keeps only what the event is — a pull_request run that concluded successfully — as a pre-filter, not a trust boundary.

Testing

The test fix, verified in both directions — so it is a real guard, not a restatement of current behaviour:

ktsu.Coder PythonQuotesTheClassInsideItsOwnBases
3.14.3 passes
3.14.0 (pinned back) fails on the new expectation
  • Semantics.Cpp.Test 33/33, Semantics.Test 1248/1248 (8 Windows-only skips), run locally on net10.0; CI covers net9.0 and all three OSes.
  • Solution builds Release with 0 warnings, 0 errors.
  • Rebuilding leaves no drift in committed generator output or alias props.

The workflow, extracted from the YAML so the shell and jq quoting is exactly what Actions runs, driven by a stub gh:

Fixture Result
Dependabot, every check green Merges
Dependabot, red CI (#233's real check runs) Declines, naming all three failing jobs
Dependabot, one job still in_progress Waits
dependabot/-branch PR opened by someone else Refused
Head moved since CI reported Refused
Malformed head SHA Exits non-zero before any query

The job's own check run is excluded throughout. The last three are cases commit 2 would have got wrong.

Note on ordering

The workflow change takes effect only once merged — workflow_run triggers require the file on the default branch. This is not a dependabot PR, so it needs a manual merge.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GVN7XqiFpbqxgnBBpyCMsx

…patch]

The auto-merge workflow ran `on: pull_request` and called `gh pr merge --auto`.
That flag reads like "merge when green", but it means "merge when branch
protection is satisfied" — it delegates the waiting entirely to required status
checks. `main` is unprotected and declares none, so the condition was vacuously
true the moment the PR opened and the merge landed immediately.

PR #233 opened at 04:18:08 and merged at 04:18:20. Its test jobs did not start
until 04:18:34 and reported failure at 04:21. The bump it carried
(ktsu.Coder 3.14.0 -> 3.14.3) broke PythonNamesTheClassInsideItsOwnBases and
reached main unexamined. Because the merge was made by github-actions[bot] with
GITHUB_TOKEN, the push triggered no workflow on main either, so the failure
stayed invisible until the next manual dispatch.

Rather than turn branch protection on, the definition of green lives in the
workflow. Firing on workflow_run rather than pull_request is what makes that
possible: the merge is decided after CI reports, so nothing waits on a check
that is itself waiting on us -- which is why `gh pr checks --watch` cannot be
used here, as it would block on this job's own in-progress check run -- and no
runner sits idle for the length of the build.

The triggering run being green is necessary but not sufficient, so the commit is
asked what every check says before merging. Anything still running is a reason
to wait, not to merge; whichever run finishes last fires the workflow again and
the merge happens on that pass. A check that is absent is not counted, because
Verify Generated Files skips a markdown-only bump by design. `skipped` and
`neutral` count as green: dotnet.yml skips Analyze & Release and Security
Scanning on a pull request, and CodeQL reports neutral there.

The head branch prefix is checked alongside the actor because a re-run
re-attributes the run to whoever pressed the button; requiring both leaves a
human re-run to be merged by hand, which is the safe direction to fail in.

The dependabot/fetch-metadata step is dropped rather than ported. Its gate
accepted semver-major, -minor and -patch — every value it can produce — so it
filtered nothing. Behaviour is unchanged by its removal, and restricting which
bumps may auto-merge is a separate decision.

Verified by extracting the step from the YAML and running it against three
fixtures built from PR #233's real check runs: as it stood it names the three
failures and declines; with the tests green it merges; with one job still
running it waits. The job's own check run is excluded in all three.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GVN7XqiFpbqxgnBBpyCMsx

Copy link
Copy Markdown
Contributor Author

CI is red, and it is not this PR's failure

Test on macos-latest, ubuntu-latest and windows-latest all fail on SevenTargetProjectionTests.PythonNamesTheClassInsideItsOwnBases — 2 of 1314 (once per TFM). This PR changes one file, .github/workflows/dependabot-merge.yml, which cannot affect a test.

The same failure is on main. Run #808 is red on 040c496 with an identical assertion, and it reproduces locally on a clean checkout of main. Bisected to a single package:

ktsu.Coder SevenTargetProjectionTests
3.14.0 6/6 pass
3.14.3 1 failed, 5 passed

The bump landed in #233.

This is the pin firing as designed, not a regression. The test deliberately asserts the broken upstream output so that an upstream fix surfaces here — as CLAUDE.md puts it, "the tests pin both rather than skipping them, so the day either is fixed upstream the test fails and is updated to assert the fix." ktsu-dev/Coder#64 is now fixed: Coder emits the string forward reference, which is the correct Python idiom.

- class Length(IVector0[Length[T], T])     # NameError on import
+ class Length(IVector0["Length[T]", T])   # valid

No re-run

The failure is a deterministic string assertion, reproduced locally three times and bisected to an exact package version. A re-run would return the same result and tell us nothing, so I have not spent one.

Proposed patch — deliberately not in this PR

Fixing it here would widen a CI-configuration change into a test-and-docs change. It wants its own PR:

Semantics.Cpp.Test/SevenTargetProjectionTests.cs — rename the test (it no longer names the class inside its own bases), rewrite the remark, and assert the fix:

[TestMethod]
public void PythonQuotesTheClassInsideItsOwnBases()
{
    Assert.Contains("class Length(IVector0[\"Length[T]\", T])", Written["python"]);
}

CLAUDE.md — drop the Python row from the two-target table, and move the count from five of seven to six of seven. The Go row stays: GoSpellsAGenericTypeItDidNotDeclare still passes, so ktsu-dev/Coder#63 is still open.

Worth noting the pin did exactly its job — it is how the Coder fix was noticed at all.

Consequence for this PR

main is red, so this PR cannot go green until that fix lands. The change here is reviewable on its own and the red is fully accounted for; it just cannot show green first.


Generated by Claude Code

PythonNamesTheClassInsideItsOwnBases asserted that ktsu.Coder writes
`class Length(IVector0[Length[T], T])` — a base list naming the class being
declared, which Python evaluates eagerly and so raises `NameError` on import.
That was never the desired output. The test pinned the defect deliberately, so
that fixing it upstream would fail here rather than pass unnoticed, and
ktsu-dev/Coder#64 is now fixed: Coder writes the string forward reference
`IVector0["Length[T]", T]`, which is the idiom Python has for exactly this.

So the failure on main is the pin doing its job, not a regression. The bump from
ktsu.Coder 3.14.0 to 3.14.3 in #233 carried the fix in, and this is the update
the pin's own remark said to make when that day came.

The test is renamed to PythonQuotesTheClassInsideItsOwnBases and asserts the
quoted form. Keeping it rather than deleting it pins the fix the same way round:
a regression upstream fails here instead of shipping a module that cannot be
imported. Verified in both directions — against 3.14.3 it passes, and pinning
Directory.Packages.props back to 3.14.0 fails it on the new expectation, so it
is a real guard rather than a restatement of current behaviour.

Go is untouched. GoSpellsAGenericTypeItDidNotDeclare still passes, so
ktsu-dev/Coder#63 is still open and still pinned.

CLAUDE.md moves the probe's answer from five of seven to six of seven, drops
Python from the table of targets whose toolchain refuses the output, and records
what happened: the pin is how the fix was noticed at all, which is the argument
for pinning rather than skipping.

Testing: Semantics.Cpp.Test 33/33 and Semantics.Test 1248/1248 pass, the
solution builds Release with 0 warnings, and rebuilding leaves no drift in the
committed generator output or alias props.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GVN7XqiFpbqxgnBBpyCMsx
@matt-edmondson matt-edmondson changed the title Merge Dependabot PRs when CI is green, not when nothing is required [patch] Get main green, and stop Dependabot merging before CI reports [patch] Sep 15, 2026
SonarCloud reports githubactions:S8232 as a BLOCKER on the job's `if:`, and it
is right. `github.event.workflow_run.actor.login` is not an authenticated
statement about who opened the pull request: a re-run re-attributes the run to
whoever pressed the button, and nothing in the payload is signed. The
`dependabot/` branch prefix beside it was no better — a branch is named by
whoever pushes it. Together they were the whole of what stood between an
arbitrary pull request and an automatic merge with `contents: write`.

The payload now only locates the pull request; GitHub is asked who opened it,
and the answer decides. `.user.login` from the REST representation is the
identity check, so a PR on a `dependabot/`-prefixed branch opened by anyone else
is left alone rather than merged.

Matching on the head SHA rather than the branch name also closes a hole this
workflow existed to close. The only pull request that can match is one whose
head is the exact commit CI just reported on, so a commit pushed after CI
finished moves the head, nothing matches, and nothing is merged. Before this the
lookup was by branch, and a push landing between the green report and the merge
would have been merged without CI ever seeing it — the same failure as #233,
reached by a different route.

The SHA is checked to be a hex digest before it is interpolated into a jq
filter, so nothing from the payload reaches the query as syntax.

The `if:` keeps only what the event is rather than who it claims to be: a
pull_request run that concluded successfully. That is a pre-filter, not a trust
boundary, and the trust decision is the API lookup.

Testing: the step was extracted from the YAML, so the shell and jq quoting is
exactly what Actions runs, and driven against six fixtures with a stub gh —
Dependabot with every check green merges; red CI and a still-running job each
decline and name what is not green; a PR on a dependabot/-prefixed branch opened
by someone else is refused; a head that moved since CI matches nothing; and a
malformed SHA exits non-zero before any query is made.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GVN7XqiFpbqxgnBBpyCMsx
@sonarqubecloud

Copy link
Copy Markdown

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.

1 participant