Skip to content

Run Darling's whole-tree guards where the area filters do not reach - #3026

Merged
erikdarlingdata merged 2 commits into
devfrom
fix/md-ci-guard-coverage
Sep 5, 2026
Merged

erikdarlingdata merged 2 commits into
devfrom
fix/md-ci-guard-coverage

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 5, 2026

Copy link
Copy Markdown
Owner

What changed

Darling.Tests is not only Darling's test suite. Some of its guards take the repository itself as input rather than the Darling tree, and build.yml gated all of them on filters that describe Darling product code.

Two instances, both derived from the guards' own source. MigrationUpgradeLadderLiveTests.TheMostRecentRelease_HasALadderFixture derives the ladder fixture it requires from CHANGELOG.md's newest released heading — its own doc comment says "The release cut moves the heading, and this fails until the fixture beside it exists" — so a release cut committed as a CHANGELOG-only change is precisely the input it exists to fail on, and it is the input no check would run it against. FleetIdentifierScrubTests enumerates every tracked file carrying one of nine extensions, .md among them, so markdown is in its own declared scope.

The filters are not wrong; they answer a different question. darling, core and root decide what to compile and publish, which is what they are for. They are not a description of what a whole-tree guard reads, so the two sets differ. Measured against the last 100 pull requests, replaying each one's file list through this workflow's actual filter patterns: 92 lit one of the three and 8 lit none — 7 of the 8 changed markdown, the eighth changed .github/.

So the new job's gate is the complement of the build job's, read off that job's own step rather than restated. The build job now publishes steps.darling-tests.outcome as an output; darling-tree-guards runs the suite when it reads skipped. Nothing is enumerated, so nothing can go stale: an enumerated area list stops covering the next tree added, which is the shape #2114, #2830 and #2839 each hit in turn, and a second copy of the gate would drift from the first the same way. It also runs whenever this workflow changes, so a change to the gate is exercised by the gate — the same self-validation darling-pg takes.

The cost, and the two shapes I did not take

The complement gate never double-runs the suite: 92 of those 100 PRs land on the already-ran side and report here without spinning up .NET at all. On the other 8 it costs one Windows runner for about three minutes, priced off run 33969850863's real step times — 35s of .NET setup, 9s restore, 93s to build Darling.Tests, 32s for the suite.

Not '**/*.md' on the darling filter. It would retire the #1712 docs fast path for the only change class it serves (the fast path engages only when no area is lit, and running the guards needs the restore the fast path skips), and it would make darling mean "Darling code, or any markdown anywhere" for the four steps that consume it — including two publishes. Same runner cost, worse signal.

Not '**/*.md' on a new job's own filter. 52 of the last 100 PRs touch markdown and 45 of those already run the suite in build, so a markdown filter would double-run it on 45% of PRs — about 2.4 hours of extra Windows runner per 100 PRs on the pool this workflow's own concurrency comment identifies as the thing that serializes everyone's CI.

No PostgreSQL in the new job, deliberately. The whole-tree guards need none, and standing up a throwaway TimescaleDB cluster would make a documentation change depend on the flakiest part of CI — #1862's compression flake failed twice on CI in two unrecognizably different ways and could not be reproduced locally at all.

Deliberately unchanged

  • The lite / Lite.Tests filter asymmetry. darling reaches Lite/**/*.cs but not Lite.Tests/**, while lite reaches Darling/Darling.Tests/**. Untouched: it is a separate question about cross-app reads, EveryCrossAppSourceRead_IsReachableByTheFilterThatGatesItsSuite already governs it, and nothing here depends on it.
  • The docs fast path. A markdown-only PR still fast-paths build in ~30s; the guards run in a job of their own.
  • Workflow triggers. No on: block is touched in any workflow, so nothing that can register twice registers differently.
  • CHANGELOG.md. Entry text is below rather than committed.

The required-check count goes from six to seven.

Verified

  • The pin is red six ways, five of them on distinct assertions, executed by compiling the actual CrossAppGuardCiGateTests.cs — unmodified, so the assertions are the real ones — into a net10.0 console harness with xunit.v3 3.2.2, the version this repo pins. build.yml as it stands on dev fails on the missing step id; removing only the id fails the same way; removing only the outputs: block fails on the published outcome; deleting the consuming job fails on "nothing reads the build job's Darling.Tests outcome"; pointing the consuming job at a different suite fails on the dotnet run assertion; and changing the gate to key on success instead of skipped fails on that comparison. Green with the change in place.
  • The two pre-existing facts in that file still pass — 3 of 3 in the class, Failed: 0, Not Run: 0.
  • FleetIdentifierScrubTests passes on this tree and is not vacuous here: run through the same harness it reports Failed: 0, and planting a non-allowlisted slug with a padded ordinal into a markdown file under docs/ turns both of its facts red, naming the file and line; removing it returns both to green. So the guard does reach markdown, which is what makes the gate matter.
  • The workflow parses, and jobs resolves to build, darling-pg, darling-linux, darling-tree-guards with the new job's needs, if, runs-on, timeout-minutes and permissions as written, and id: darling-tests landing on the Run Darling tests step and no other.
  • The three existing assertions that read this file by offset still find the build job's step first. ViewerSidebarDotRendersTheCardStatusTests.TheGuard_RunsOnEveryTreeItScans and CrossAppGuardCiGateTests.Check both locate - name: Run Darling tests by IndexOf and read forward a bounded window; the new job is appended after the build job and its step is named Run the whole-tree guards, so neither can latch onto it. CiClusterWorkerSizingTests parses only Add-Content ... -Value "setting = value" lines, and the diff adds none.
  • Line endings and BOM: build.yml and CrossAppGuardCiGateTests.cs are both CRLF with no BOM on dev and stay that way in the working tree; both blobs are LF after staging, 0 CRLF, which is what * text=auto eol=crlf should produce.
  • The mutation loop restored by content, not by git checkout, with the fixed file's md5 re-asserted after every one of the six cases; the working tree matched the commit afterwards.

Review findings

Taken, in 3529495. The else arm printed "the suite has already run on this commit" for an EMPTY needs.build.outputs.darling-tests as well as for success / failure. A job's declared outputs do not reach a downstream consumer when the producing job's conclusion is failure, so a build that fails — at Lite.Tests, before reaching the Darling step, or at the Darling step itself — can leave that read empty whatever the step actually did, and the notice would then assert something it cannot know. That is the dishonest-log failure this job exists to close, arriving in the job's own log. The empty outcome now has its own arm: still skip, because a red build blocks the merge on its own and re-running the suite on a commit nothing can merge buys no decision while costing three minutes on every failing build — but it says the status is not knowable from here and names the log that holds it.

Declined, in the second pass: passing github.event_name through env: for consistency. The env: indirection in this step carries the two filter-derived values, and that is the pattern the workflow already uses — ALL_COUNT, DOCS_COUNT and AREAS in the docs fast path, DARLING_TESTS and WORKFLOW here. github.event_name is interpolated directly into bash in every gate-decision step in this file: lines 255 and 261 in the fast-path classifier, 690 in the Darling PG gate, 865 in the Linux gate, and 1035 here. So this step already matches the house pattern on both halves; routing event_name through env: would make it the only one that does.

Raised as a checklist item, not a code change: the required-status-checks list. See "Action needed outside this diff" above — it is real, it is confirmed against the API, and it cannot be expressed in this diff.

Fixing the wording also retires the suggestion to confirm the propagation behaviour with a deliberately broken build: both branches are now correct under either behaviour, so the question stops being load-bearing.

All four arms exercised directly, by extracting the gate script from the parsed workflow and running it under each input: release → skip; skipped → run; workflow filter true → run; empty → the new not-knowable arm; success and failure → skip with the outcome named. bash -n clean on the extracted script.

After, observed on 3529495

Darling whole-tree guards ran and did the work: 2m39s wall, of which .NET setup 29s, restore 8s, Build Darling.Tests 77s, and Run the whole-tree guards 30s reporting Darling.Tests Total: 7546, Errors: 0, Failed: 0, Skipped: 313, Not Run: 0. A no-op report on this job costs ~15s, so the duration alone separates the two, and the test count settles it.

Which arm fired is in the log, and it is the one predicted: DARLING_TESTS: success, WORKFLOW: true, ##[notice]build.yml changed, so this job's own gate is exercised by the gate. The complement arm could not fire here — build ran the suite on this PR because build.yml is in the root filter — which is the limit recorded below.

needs: build sequenced as intended: build completed 14:41:22, this job started 14:41:30.

Incidental confirmation of if: ${{ !cancelled() }}: on the superseded first run this job reported cancelled with started_at == completed_at, so it did not take a runner for a run that no longer mattered.

Action needed outside this diff

dev's branch protection currently requires exactly two contexts — build and Darling PostgreSQL tests. Darling whole-tree guards therefore reports a result but does not yet block a merge, and neither do Darling Linux build, check-branches or verify. That is a repo Settings change, not something a workflow file can express (there is no .github/settings.yml or rulesets-as-code here), so it will not happen as a side effect of merging this:

gh api -X PATCH repos/erikdarlingdata/PerformanceMonitor/branches/dev/protection/required_status_checks \
  -f 'contexts[]=build' -f 'contexts[]=Darling PostgreSQL tests' -f 'contexts[]=Darling whole-tree guards'

Worth noting alongside it: on a markdown-only change both currently-required contexts go green having executed nothing, so until this job is added the required set is satisfied by two vacuous passes.

Before, measured

Markdown-only PRs on this workflow today, by duration and by step conclusion rather than by the check mark:

check #2984 (CHANGELOG.md only, 4344fcf) #2980 (CHANGELOG.md only, fa7fd5f) a Darling code PR (run 33969850863)
build 31s 20s 545s
Darling PostgreSQL tests 14s 21s 262s
Darling Linux build 8s 11s 108s

On #2984's build job, all 35 build / test / publish steps report skipped; four steps ran — job setup, checkout, Detect changed paths, and Classify change for the docs fast path. Run Darling tests is one of the 35. That skipped is the exact value the new gate keys on, so the arm is reading a value whose real-world setting on a markdown-only diff is measured rather than assumed.

Not verified

  • The complement arm did not fire pre-merge, and no design could have made it. Confirmed rather than predicted: this run logged DARLING_TESTS: success. A PR that introduces this job necessarily changes build.yml, which is in the root filter, so build runs Darling.Tests on the very commit that adds the job and publishes success, never skipped. What is observed instead is the job's mechanics end to end via the self-validation arm, plus the arm's input in two measured halves: build publishes that step's outcome (seen here as success), and Record #2975's entries, and correct a claim its measurements disproved #2984's run shows the same step's conclusion is skipped on a markdown-only diff. One observation short of end-to-end, and I am not going to call it end-to-end.
  • The Windows suites cannot run on macOS. The harness compiles the real test files, so the assertions and the regexes are the real ones, but xunit's own discovery and the rest of both suites are CI's word.
  • needs: build adds the new job's own startup to the workflow's wall clock on the 92% of PRs where it reports without working. This run measured the pre-gate steps at 9s (job setup 1s, checkout 7s, filter 0s, gate 1s), so ~10s plus queue time — but a run where the gate says skip has not been observed, because this PR's gate says run.
  • Whether GitHub's expression parser accepts a hyphenated job-output name. Settled by the run: needs.build.outputs.darling-tests resolved to success in the job's env: block.

CHANGELOG entry, not committed — for the coordinator to place

- Darling's whole-tree guards now run on changes the product-area path filters do not reach. `Darling.Tests` takes the repository itself as input in places — the ladder-fixture guard derives what it expects from `CHANGELOG.md`, and the identifier sweep enumerates every tracked file carrying one of nine extensions — so gating the suite on `darling` / `core` / `root` left it unrun on markdown-only and `.github`-only changes. A new `Darling whole-tree guards` job runs the suite when the `build` job's filters did not, keyed on that job's own step outcome rather than on a second list of paths.

Darling.Tests takes the repository itself as input in places: the ladder
fixture guard derives what it expects from CHANGELOG.md's newest released
heading, and the identifier sweep enumerates every tracked file carrying one
of nine extensions, .md among them. The darling/core/root filters describe
Darling product code, correctly - that is what decides the compile and the
publish - so a change confined to markdown or to .github leaves all three
false and the suite unrun. Measured over the last 100 pull requests: 92 lit
one of the three, 8 lit none, 7 of those 8 changed markdown.

Add a job whose gate is the complement of the build job's, read off that
job's own step outcome rather than written out again as a second set of
paths. A second set is free to fall out of agreement with the first, and
enumerated area lists going stale is the shape #2114, #2830 and #2839 each
hit in turn. Being the complement it never double-runs the suite, and it
needs no upkeep as the filters change.

Pin the three pieces of wiring in CrossAppGuardCiGateTests: the step must
carry the id, the build job must publish its outcome, and the consuming job
must actually invoke the suite. Losing any one leaves a job reporting
success having run nothing.
Comment thread .github/workflows/build.yml
@claude

claude Bot commented Sep 5, 2026

Copy link
Copy Markdown

Reviewed. This is CI-only (.github/workflows/build.yml + Lite.Tests/CrossAppGuardCiGateTests.cs), no T-SQL/collector code and no Lite/Darling app-parity surface touched, so those two review angles don't really apply here.

Design checks out. Walked the complement logic end to end:

  • steps.darling-tests.outcome on the id: darling-tests step, published as build.outputs.darling-tests — correctly placed, matches the step it's meant to expose.
  • The three-way decision in darling-tree-guards (release → build already ran it unconditionally; skipped → run; workflow filter true → run for self-validation; else → skip) covers the states I could enumerate, including the docs fast-path (which still leaves the darling filter false, so Run Darling tests reports skipped and the new job correctly picks it up).
  • contents: read-only permissions, --locked-mode restore, cache-dependency-path, timeout-minutes, and the checkout/paths-filter base: expression all match the conventions already established by the neighboring darling-pg job — good consistency.
  • Confirmed FleetIdentifierScrubTests and MigrationUpgradeLadderLiveTests exist as described and do take whole-tree/CHANGELOG input, so the premise holds.
  • The new CrossAppGuardCiGateTests pin cross-checks the right three wiring pieces (step id, job output, consuming job's gate + suite invocation) and reuses the existing ReadBuildYaml/RepoRoot helpers correctly.

Left one inline note on a GitHub Actions edge case worth confirming (job outputs potentially not propagating when the producing build job fails outright) — I believe it's non-blocking since a failed build already prevents merge regardless of what darling-tree-guards reports, but flagging so it can be verified against a real failing run rather than assumed.

No security, correctness, or parity issues found beyond that note.

…d no outcome

A job's declared outputs do not reach a downstream consumer when the
producing job's conclusion is failure, so a failing build job can leave
needs.build.outputs.darling-tests empty whatever its Darling step actually
did. That fell through to the arm that prints "the suite has already run on
this commit", which is the dishonest-log failure the job exists to close.

Give the empty outcome its own arm: still skip - a red build blocks the merge
on its own, and re-running the suite on a commit nothing can merge buys no
decision and would cost three minutes on every failing build - but say that
the suite's status is not knowable from here and name the log that holds it.
@claude

claude Bot commented Sep 5, 2026

Copy link
Copy Markdown

Reviewed the build.yml change and the new Lite.Tests/CrossAppGuardCiGateTests.TheDarlingSuite_RunsWhereTheAreaFiltersDoNotReach pin. Traced the gate logic (build job's outputs.darling-testsdarling-tree-guards's five-armed elif) against the actual step/job structure — the Run Darling tests step and Build Darling step share the same if:, so they can't disagree on skip vs. run; the new job's own .github/workflows/build.yml self-validation filter mirrors darling-pg's existing pattern; and the new xunit test's string anchors (id: darling-tests, the outputs: line, needs.build.outputs.darling-tests, the "skipped" branch, the dotnet run invocation) all match what's actually in the YAML. No correctness bugs found in the gate logic itself, and no Lite/Darling parity drift — the new test lives in Lite.Tests alongside the existing cross-app guard tests, consistent with where EveryCrossAppSourceRead_IsReachableByTheFilterThatGatesItsSuite already lives, and the doc comment explains why it isn't duplicated into Darling.Tests (a pin on "the suite didn't run" can't live inside the suite it's pinning).

One thing worth confirming outside this diff: darling-tree-guards only closes the gap this PR describes if it's added to the required status checks list in branch protection for main/dev. That's a repo Settings change, not something this diff can express (no .github/settings.yml or rulesets-as-code in this repo), so it's easy for it to quietly not happen. Worth a checklist item on this PR or a follow-up to confirm it's been done — otherwise darling-tree-guards reports its result but doesn't actually block a merge that skipped it.

Minor/non-blocking nit: in the "Decide whether the whole-tree guards run here" step, ${{ github.event_name }} is interpolated directly into the bash if condition, while the two other decision inputs (DARLING_TESTS, WORKFLOW) are passed through env: first. event_name is a fixed GitHub-controlled enum so there's no injection risk here, but it's an inconsistency with the pattern the step otherwise follows.

@erikdarlingdata
erikdarlingdata merged commit 9bb6dfc into dev Sep 5, 2026
12 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/md-ci-guard-coverage branch September 5, 2026 15:11
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