From 60ce9fa47a7f394b507e2d1529b5789a1f4761ce Mon Sep 17 00:00:00 2001 From: matt-edmondson Date: Tue, 15 Sep 2026 05:13:17 +0000 Subject: [PATCH 1/3] Merge Dependabot PRs when CI is green, not when nothing is required [patch] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01GVN7XqiFpbqxgnBBpyCMsx --- .github/workflows/dependabot-merge.yml | 93 +++++++++++++++++++++----- 1 file changed, 75 insertions(+), 18 deletions(-) diff --git a/.github/workflows/dependabot-merge.yml b/.github/workflows/dependabot-merge.yml index 6e383a59..147cbfcb 100644 --- a/.github/workflows/dependabot-merge.yml +++ b/.github/workflows/dependabot-merge.yml @@ -1,31 +1,88 @@ name: Dependabot auto-merge -on: pull_request + +# Merges a Dependabot PR only once CI has actually reported green. +# +# This used to run `on: pull_request` and call `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 and the merge landed immediately. +# PR #233 was opened at 04:18:08 and merged at 04:18:20; its test jobs did not start until +# 04:18:34 and went red at 04:21. That is how a failing dependency bump reached main. +# +# Rather than turn on branch protection, the list of checks that must be green lives here. +# Firing on workflow_run instead of pull_request is what makes that possible: the merge is +# decided after CI reports, so there is no check to wait on that is waiting on us, and no +# runner sits idle for the duration of the build. + +on: + workflow_run: + workflows: + - ".NET Workflow" + - "Verify Generated Files" + types: [completed] permissions: contents: write pull-requests: write - packages: read jobs: - dependabot: - name: Process Dependabot PR + merge: + name: Merge green Dependabot PR runs-on: ubuntu-latest - if: github.actor == 'dependabot[bot]' timeout-minutes: 5 + # Only a successful CI run on a Dependabot pull request is a reason to look at all. A + # failed run is not merely "not yet" — it must never reach the merge step below. + # + # The branch prefix is checked as well as the actor because a re-run re-attributes the + # run to whoever pressed the button. Requiring both means a human re-run leaves the PR + # to be merged by hand, which is the safe direction to fail in. + if: > + github.event.workflow_run.event == 'pull_request' && + github.event.workflow_run.conclusion == 'success' && + github.event.workflow_run.actor.login == 'dependabot[bot]' && + startsWith(github.event.workflow_run.head_branch, 'dependabot/') + steps: - - name: Fetch Dependabot metadata - id: metadata - uses: dependabot/fetch-metadata@25dd0e34f4fe68f24cc83900b1fe3fe149efef98 # v3.1.0 - with: - github-token: "${{ secrets.GITHUB_TOKEN }}" - - - name: Enable auto-merge for Dependabot PRs - if: steps.metadata.outputs.update-type == 'version-update:semver-major' || steps.metadata.outputs.update-type == 'version-update:semver-minor' || steps.metadata.outputs.update-type == 'version-update:semver-patch' + - name: Merge if every check on the commit is green env: - PR_URL: ${{github.event.pull_request.html_url}} - GH_TOKEN: ${{secrets.GITHUB_TOKEN}} + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + BRANCH: ${{ github.event.workflow_run.head_branch }} + SHA: ${{ github.event.workflow_run.head_sha }} + SELF: Merge green Dependabot PR run: | - set -e - echo "Processing PR: $PR_URL" - gh pr merge --auto --merge "$PR_URL" + set -euo pipefail + + PR=$(gh pr list --repo "$GITHUB_REPOSITORY" --head "$BRANCH" \ + --state open --json number --jq '.[0].number // empty') + if [ -z "$PR" ]; then + echo "No open pull request on $BRANCH; nothing to merge." + exit 0 + fi + + # The workflow that triggered us is green, but it is not the only one. Ask the + # commit what every check says, and treat anything still running as a reason to + # wait rather than a reason to merge. A check that is absent entirely is not + # 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 pull request, and CodeQL reports neutral there. + gh api --paginate "repos/$GITHUB_REPOSITORY/commits/$SHA/check-runs" \ + --jq ".check_runs[] + | select(.name != \"$SELF\") + | select(.status != \"completed\" + or (.conclusion != \"success\" + and .conclusion != \"skipped\" + and .conclusion != \"neutral\")) + | \" \(.name): \(.status)/\(.conclusion // \"pending\")\"" > "$RUNNER_TEMP/not-green.txt" + + if [ -s "$RUNNER_TEMP/not-green.txt" ]; then + echo "Not merging PR #$PR. These checks are not green:" + cat "$RUNNER_TEMP/not-green.txt" + # Not a failure of this workflow: whichever run finishes last fires it again, + # and the merge happens on that pass. + exit 0 + fi + + echo "Every check on $SHA is green. Merging PR #$PR." + gh pr merge --merge "$PR" From e1353a41dfeeed43c9bf8787e360c06a4c72ae70 Mon Sep 17 00:00:00 2001 From: matt-edmondson Date: Tue, 15 Sep 2026 05:39:45 +0000 Subject: [PATCH 2/3] Assert the Python projection fix the pin caught [patch] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01GVN7XqiFpbqxgnBBpyCMsx --- CLAUDE.md | 17 +++++++---- .../SevenTargetProjectionTests.cs | 28 +++++++++++-------- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 7a12dd74..0d53df7a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -200,7 +200,7 @@ forms as well, and matching it is a change to every form at once rather than par ### Can the quantities be generated for languages other than C# and C++? -`SevenTargetProjectionTests` is the probe that answers it, and the answer so far is **five of +`SevenTargetProjectionTests` is the probe that answers it, and the answer so far is **six of seven**. It builds one quantity — the magnitude form of `Length`, read from the real `dimensions.json` — as a language-agnostic `ktsu.Coder` AST and writes it in all seven of that library's targets. The neutral shape is the whole of what a generated magnitude is: a record struct @@ -210,16 +210,21 @@ C# comes out as exactly what `QuantitiesGenerator` writes today, which is the re the AST is expressive enough for the quantities, so what the other six do is a question about those languages rather than about the model. -**Two targets write source their own toolchain refuses**, both recorded upstream and neither fixable -here: +**One target writes source its own toolchain refuses**, recorded upstream and not fixable here: | Target | What comes out | Why | |---|---|---| | Go | `type Length struct` with no parameters, then `func LengthFromMeter(value T) Length[T]` | ktsu-dev/Coder#63 — a generic type is deliberately written down rather than emitted, and the constructor was not given the same treatment. `go vet` says `undefined: T`. | -| Python | `class Length(IVector0[Length[T], T])` | ktsu-dev/Coder#64 — Python evaluates a base list eagerly, so the self-type idiom every quantity is declared with raises `NameError` on import. | -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. +The test **pins** it rather than skipping it, so the day it is fixed upstream the test fails and is +updated to assert the fix. + +**Python was the second, and the pin is what caught the fix.** It wrote +`class Length(IVector0[Length[T], T])`, which raises `NameError` on import because Python evaluates +a base list eagerly. ktsu-dev/Coder#64 fixed that to the string forward reference +`IVector0["Length[T]", T]`, and the first build after the bump from ktsu.Coder 3.14.0 to 3.14.3 +failed here — which is the whole point of pinning rather than skipping, and is how the fix was +noticed at all. The test asserts the fix now, so a regression upstream fails the same way round. The probe lives in `Semantics.Cpp.Test` because that is where the reader of `dimensions.json` is, and that is itself the finding about this repository: `QuantityMetadata` and `MetadataProjection` diff --git a/Semantics.Cpp.Test/SevenTargetProjectionTests.cs b/Semantics.Cpp.Test/SevenTargetProjectionTests.cs index 09048a14..d4c8a71f 100644 --- a/Semantics.Cpp.Test/SevenTargetProjectionTests.cs +++ b/Semantics.Cpp.Test/SevenTargetProjectionTests.cs @@ -32,10 +32,11 @@ namespace ktsu.Semantics.Cpp.Test; /// before a neutral projection could be a project of its own. /// /// -/// Two targets are known to produce source their own toolchain refuses, and the tests below -/// pin that rather than skipping it, so the day either is fixed upstream the test fails and says -/// so. Both are recorded against ktsu.Coder — ktsu-dev/Coder#63 and ktsu-dev/Coder#64 — and -/// neither is anything this repository can fix. +/// One target is still known to produce source its own toolchain refuses, and the test +/// below pins that rather than skipping it, so the day it is fixed upstream the test fails and +/// says so. It is recorded against ktsu.Coder as ktsu-dev/Coder#63 and is not anything this +/// repository can fix. Python was the second such target until ktsu-dev/Coder#64 was fixed; its +/// pin failed exactly as intended, and it now asserts the fix. /// /// [TestClass] @@ -142,20 +143,25 @@ public void GoSpellsAGenericTypeItDidNotDeclare() } /// - /// Python writes a base that names the class being declared, which Python evaluates before the - /// class exists. + /// Python quotes the self-type in its own base list, which is what makes the class importable. /// /// /// `IVector0<Length<T>, T>` is the self-type idiom every quantity here is declared /// with, and it is what C# needs to give an interface a method returning the implementing type. - /// Python evaluates a base list eagerly, so `class Length(IVector0[Length[T], T])` raises - /// `NameError: name 'Length' is not defined` on import. A string in the base list is the fix - /// Python has for this, and it is not something the AST currently says. ktsu-dev/Coder#64. + /// Python evaluates a base list eagerly, so naming the class inside its own bases — + /// `IVector0[Length[T], T]` — raises `NameError: name 'Length' is not defined` on import. A + /// string is the forward reference Python has for exactly this, and ktsu.Coder now writes one. + /// + /// This was pinned as a defect until ktsu-dev/Coder#64 was fixed, which the bump from + /// ktsu.Coder 3.14.0 to 3.14.3 brought in. The pin failed, as it was written to, and asserts + /// the fix instead. Keeping it pins the fix the same way round, so a regression upstream fails + /// here rather than shipping a module that cannot be imported. + /// /// [TestMethod] - public void PythonNamesTheClassInsideItsOwnBases() + public void PythonQuotesTheClassInsideItsOwnBases() { - Assert.Contains("class Length(IVector0[Length[T], T])", Written["python"]); + Assert.Contains("class Length(IVector0[\"Length[T]\", T])", Written["python"]); } /// From 283bb834cf0d43beee0a659ebfebd6a880328a87 Mon Sep 17 00:00:00 2001 From: matt-edmondson Date: Tue, 15 Sep 2026 05:59:57 +0000 Subject: [PATCH 3/3] Decide whose PR it is from the API, not the event payload [patch] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01GVN7XqiFpbqxgnBBpyCMsx --- .github/workflows/dependabot-merge.yml | 52 ++++++++++++++++++-------- 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/.github/workflows/dependabot-merge.yml b/.github/workflows/dependabot-merge.yml index 147cbfcb..f6c36d21 100644 --- a/.github/workflows/dependabot-merge.yml +++ b/.github/workflows/dependabot-merge.yml @@ -31,32 +31,52 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 5 - # Only a successful CI run on a Dependabot pull request is a reason to look at all. A - # failed run is not merely "not yet" — it must never reach the merge step below. - # - # The branch prefix is checked as well as the actor because a re-run re-attributes the - # run to whoever pressed the button. Requiring both means a human re-run leaves the PR - # to be merged by hand, which is the safe direction to fail in. + # Only what the event itself is, never who it claims to be. A successful CI run on a pull + # request is the cheap pre-filter; whose pull request it is gets decided in the step below, + # from the API. A failed run is not merely "not yet" — it must never reach the merge step. if: > github.event.workflow_run.event == 'pull_request' && - github.event.workflow_run.conclusion == 'success' && - github.event.workflow_run.actor.login == 'dependabot[bot]' && - startsWith(github.event.workflow_run.head_branch, 'dependabot/') + github.event.workflow_run.conclusion == 'success' steps: - - name: Merge if every check on the commit is green + - name: Merge if Dependabot opened it and every check on the commit is green env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - BRANCH: ${{ github.event.workflow_run.head_branch }} SHA: ${{ github.event.workflow_run.head_sha }} SELF: Merge green Dependabot PR run: | set -euo pipefail - PR=$(gh pr list --repo "$GITHUB_REPOSITORY" --head "$BRANCH" \ - --state open --json number --jq '.[0].number // empty') - if [ -z "$PR" ]; then - echo "No open pull request on $BRANCH; nothing to merge." + # The workflow_run payload says who triggered the run, and that is not an + # authenticated statement about who opened the pull request: a re-run re-attributes + # it to whoever pressed the button, and nothing in the payload is signed. Trusting + # `actor.login` or the `dependabot/` branch prefix to decide what may be merged is + # githubactions:S8232. So the payload is used only to locate the pull request, and + # GitHub is asked who actually opened it. + # + # Matching on the head SHA rather than the branch name also buys the property this + # workflow exists for: 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 — instead of merging code CI never saw. + case "$SHA" in + "" | *[!0-9a-f]*) echo "Head SHA is not a hex digest; refusing to continue." >&2; exit 1 ;; + esac + + FOUND=$(gh api --paginate "repos/$GITHUB_REPOSITORY/pulls?state=open&per_page=100" \ + --jq "[.[] | select(.head.sha == \"$SHA\")][0] + | select(. != null) + | [.number, .user.login] | @tsv") + + if [ -z "$FOUND" ]; then + echo "No open pull request whose head is $SHA; nothing to merge." + exit 0 + fi + + PR=$(printf '%s' "$FOUND" | cut -f1) + AUTHOR=$(printf '%s' "$FOUND" | cut -f2) + + if [ "$AUTHOR" != "dependabot[bot]" ]; then + echo "PR #$PR was opened by $AUTHOR, not Dependabot. Leaving it alone." exit 0 fi @@ -84,5 +104,5 @@ jobs: exit 0 fi - echo "Every check on $SHA is green. Merging PR #$PR." + echo "PR #$PR is Dependabot's and every check on $SHA is green. Merging." gh pr merge --merge "$PR"