Share one path gate between the two Darling CI jobs and report its decision - #3122
Conversation
The Darling PostgreSQL job and the Darling Linux job now read .github/darling-paths-filter.yml instead of each carrying its own list of paths, and that gate covers every shared PerformanceMonitor.* library in Darling.Tests' ProjectReference closure. Both gate-decision notices report the decision and the changed files it was made from rather than a hard-coded cause.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed this as a CI/CD infra change (no T-SQL, so the T-SQL style section of CONTRIBUTING.md doesn't apply here) — two GitHub Actions jobs, a new shared path-filter YAML, and a new derived-coverage test. What I checked:
Overall this is unusually well self-verified (mutation-tested assertions, both acceptance arms demonstrated). Nothing blocking. |
RepoFileAdoptionTests requires RepoFile to be the only repo-file reader in Darling.Tests, and requires the LF-reading pins to be exactly the ones it declares. The new pin's anchors span line breaks, so it takes the LF reader and is declared in that set.
list-files: shell escapes for substitution into script text; the lists reach these steps through env:, and a runtime parameter expansion does not honour those escapes, so word splitting turned a path containing a space into several entries naming no file. Eight tracked paths under Screenshots/ carry spaces and that directory is on the documentation allowlist. A JSON array is self-delimiting, so each notice prints it verbatim.
|
Reviewed this against CONTRIBUTING.md's conventions (no T-SQL here, so the style section doesn't apply) and the Lite/Darling parity requirement. Summary: no correctness, security, or parity issues found. Independently verified the closure claim. I walked the Self-validation is real, not just claimed. Since the gate file lists itself and Lite/Darling parity. No Lite files touched, and the deliberate absence of Shell notice loops ( One thing worth double-checking outside this review's reach: the PR body's "Arm 2" (a No blocking findings. |
Review summaryWent through the full diff (the new Correctness
Lite/Darling parity
No correctness, security, or parity issues found. Nice self-validating design — the mutation-style tests ( |
|
Reviewed the diff (CI workflow + Darling.Tests only — no T-SQL, no Lite changes, so the T-SQL style rules and Lite/Darling app-parity concerns don't apply here; this is CI-gate parity between two jobs in the same workflow file, and that's what the PR fixes). What I checked:
No correctness bugs, no parity drift, and no security concerns found — if anything this closes a latent injection vector. Nice work on the mutation-tested facts and the two throwaway-PR arms (#3125/#3124) demonstrating both directions of the fix. |
|
Reviewed the diff ( Things I specifically checked and found correct:
No correctness, security, or parity issues found. This is unusually well self-validated (four new/adjusted xunit facts pin the gate's shape, with an explicit mutation test), so I don't have anything to flag. |
|
Reviewed this as a CI/tooling-only change (no T-SQL, no Lite/Darling app code touched), so the T-SQL style/parity checklist mostly doesn't apply — checked instead for correctness of the new shared gate, the two rewritten Verified independently (not just taking the PR body's word for it):
Not verified: couldn't run No correctness, security, or Lite/Darling parity issues found. The |
1 participant
Darling PostgreSQL testsskipped every change to a sharedPerformanceMonitor.*library Darling consumes, reported success in 17s having run no PostgreSQL at all, and printed a notice asserting the change was documentation. Its siblingDarling Linux build, in the same file and on the same commit, ran — because that job's filter listed shared libraries and the PostgreSQL job's listed none.What changed
.github/darling-paths-filter.ymlis new and holds the Darling path gate. Both Darling jobs read it through dorny/paths-filter's documentedfilters: <relative path>form, so the gate is one file rather than one list per job. It lives outside.github/workflows/because anything inside that directory is parsed as a workflow.The gate's
darlingfilter isDarling/**/!(*.md), the seven shared libraries in extglob form,build.yml,nightly.yml, and the gate file itself. It also carries anall: ['**']counter, which is what the notices report against — never a gate, since every changed file matches'**'.Both gate-decision notices report the decision and the changed files it was made from: which files are in the gate, or how many changed and what they were. The PG job's skip notice hard-coded
documentation-onlyas the sole explanation for a false gate; on #3114 (a PostgreSQL collector and its test) that was false in both clauses, and it read as authoritative because it was the only log there was.Darling/Darling.Tests/DarlingPathFilterGateTests.csis new: four facts holding the gate to the dependency graph rather than to a remembered list.All three filter steps in the file move from
list-files: shelltolist-files: json, including thebuildjob's docs fast path. The lists reach these steps throughenv:rather than a${{ }}interpolation into script text — deliberately, since that interpolation is thepull_requestscript-injection footgun — and a runtime parameter expansion does not honour the escapesshellformat emits, so word splitting turned one path containing a space into several entries naming no file. Eight tracked paths underScreenshots/carry spaces and that directory is on the documentation allowlist, so the docs notice is the one that meets a spaced path first. A JSON array is self-delimiting, so each notice prints it verbatim. Nothing in the file consumes*_filesother than these three notices, and*_countis emitted regardless of the format.CommentFilterAdoptionTests' class summary described the PG job's filter as naming "onlyDarling/**and the two workflow files". Corrected; the claim it supports — that no entry added to the build job's filter can stand up the throwaway cluster — is unchanged and still true.RepoFileAdoptionTestsgains one entry, because the new pin's anchors span line breaks so it takes the shared LF reader.How a third copy of the library list was avoided
Sharing between the two Darling jobs, not adding to them. The gate file replaces
Darling Linux build's inline list; it does not sit alongside it. So the repository holds the same number of copies of this dependency fact as before — thebuildjob'scorefilter, which decides what gets compiled and published, and this one, which decides what reaches Darling — and the job that had none now reads the one the other job reads.YAML anchors were not the mechanism: GitHub Actions' workflow parser rejects anchors and aliases, and the filter body is a nested YAML document inside a string input anyway, so an anchor could not span the two jobs. A
build-job output consumed vianeeds:was the other candidate and was rejected: it would serialise the PostgreSQL job behind the whole Windows build, and a failedbuildwould then skip the PG job entirely rather than run it — losing coverage exactly when a commit is in trouble.darling-tree-guardsalready pays that price deliberately, for a job whose gate is by design the complement of the build job's.Requirement derived, not enumerated.
TheSharedGate_CoversEverySharedLibraryDarlingCompileswalks the ProjectReference closure ofDarling.Testsand requires aNAME/**/!(*.md)entry for everyPerformanceMonitor.*project at the repository root. All seven are in that closure:.Vieweraccounts for.Ui,.Analysisand.Notifications,.Serviceadds.Alertingand.PlanAnalysis,.StorageandDarling.Analysisadd.Collectors, and.Commonis reached by nearly all of them. So "how far does the dependency graph reach" is not a judgement any more — add a shared library to a Darling project and the gate entry becomes mandatory.EachDarlingJob_ReadsTheSharedGate_AndCarriesNoListOfItsOwnasserts both jobs name the gate file and that neither containsfilters: |. That second half is the one that matters: re-inlining a list is how a private copy comes back, and a private copy is what nothing compares.Deleting the gate file is a hard error rather than a silent pass — dorny throws
Configuration file not found, so both Darling jobs red.Both acceptance arms, each on its own PR
Neither arm can be demonstrated on this PR. Its change touches
build.yml, which every filter in that file names, so it fires all of them — that self-validation is why the entry exists, and it is also why this PR cannot show a skip. And its run arm fires onbuild.ymlrather than on the.Collectorsentry, which is the entry that was missing. So both arms were exercised on throwaway PRs branched off this one, where the diff against the base is the single file each arm is about and the base already carries this gate.Arm 1 — a
PerformanceMonitor.Collectors-only PR runsRun Darling PG teststo completion. #3125, one changed file,PerformanceMonitor.Collectors/PgIndexBloatCollector.cs— the same file #3114 changed, whose PG job reported success in 17s having skipped every step.Filter darling = true; the notice reads1 of 1 changed files are in the Darling gateand names the file.Darling Linux buildalso ran the publish and the image build on the same commit, so the two jobs now agree.Arm 2 — a
Darling/**/*.md-only PR still skips it. #3124, one added markdown file underDarling/docs/.Darling PostgreSQL testssucceeded in 16s with steps 5–14skipped,Run Darling PG testsamong them;Filter darling = false,Matching files: none; the notice readsNone of the 1 changed files are in the Darling gate (.github/darling-paths-filter.yml), so the TimescaleDB suite did not run on this commitand then names the file.Darling Linux buildskipped its steps 5–14 likewise.Both probe branches carry this PR's gate file byte-for-byte and its
build.ymlwith exactly one line different —on.pull_request.brancheswidened to include the throwaway base, without whichbuild.ymlnever dispatches for a pull request into a feature branch. That widening exists only on the throwaway branches. The first attempt at arm 2 registered onlycheck-branchesfor precisely that reason and was closed unrun.Ahead of CI, the pattern set was evaluated through dorny's own matcher —
picomatch(pattern, {dot: true})under thesomequantifier, which is what dorny/paths-filter v4 does — over 23 file-set cases. Each of the seven libraries istrueindividually. The negative cases all stayfalse: a root.gitignoreedit,CHANGELOG.mdalone, the rootREADME.md,Lite/source and XAML, a deprecated product, the installer tree, and markdown inside a shared library. The first two are the #1714 / run-30218459544 regressions the extglob form exists to prevent, which is why no bare negation appears in the new file. The same harness ran the same cases against the gate as it stands ondev, as a control that the check discriminates rather than agreeing with whatever it is shown: there, arm 1 isfalse— the defect — and arm 2 isfalse, correctly.Each of the four new facts was mutation-tested: dropping the
.Collectorsentry, rewriting one entry as a baredir/**include, re-inlining afilters: |list in the PG job, restoring thedocumentation-onlyskip notice, revertinglist-filestoshell, and removing it altogether. Each mutation reds the fact it should and no other.TheCoverageCheck_ReportsAnInjectedGapcarries two of those mutations in-suite, so the coverage check cannot pass by parsing nothing.Deliberately left out
Lite/**is absent from the gate, though thebuildjob's owndarlingfilter carries it. That entry exists becauseDarling.Testsreads Lite source in its cross-app parity guards — and those guards run in thebuildjob, whose "Run Darling tests" step fires on that filter, withdarling-tree-guardsas the backstop when it does not. What these two jobs add over that is a live TimescaleDB cluster and a linux-x64 publish, neither of which a Lite XAML edit can affect. ALite/**entry here would buy a four-minute cluster and no signal.The
buildjob'scorefilter is untouched. It is a description of what to compile and publish, not of what reaches Darling, and three tests read pattern strings out of it by position.Two costs are accepted rather than engineered around, both from the two jobs sharing one answer.
Darling Linux buildgainsPerformanceMonitor.Ui, which it does not compile — it publishes the service, not the WPF viewer — so a UI-only change now costs it one publish and one image build. It also gainsnightly.yml, which it has no tests to run against. Both err toward building. In exchange it gainsPerformanceMonitor.AlertingandPerformanceMonitor.PlanAnalysis, which the service it publishes references directly and its own list had been missing.Proposed CHANGELOG entry
Not applied here, to avoid conflicting in the shared
[Unreleased]block:Darling PostgreSQL testsgated onDarling/**and the two workflow files, so a change to anyPerformanceMonitor.*library Darling compiles against skipped every step includingRun Darling PG testsand reported success in seconds - whileDarling Linux build, on the same commit in the same file, ran, because its filter listed three of those libraries. Both jobs now read one gate at.github/darling-paths-filter.ymlcovering all seven libraries inDarling.Tests' ProjectReference closure, andDarlingPathFilterGateTestsderives that requirement from the closure rather than restating it. The skip notice named "documentation-only" as the reason for every skip; it now reports what the gate matched, or how many changed files it classified and what they were. The three changed-file listings also move tolist-files: json, which survives theenv:indirection thatshellformat's escaping does not - eight tracked paths underScreenshots/carry spaces, and each would have printed as several entries naming no file.