Repository navigation
ci(node-repo-validate): the layout-area data-bake ratchet for every satellite - #5972
Conversation
…d for every satellite check-layout-area-data-bake.py ports LayoutAreaDataBakeRatchetGuard's scanner line for line (exact parity on core: 73 units / 37 files against test/LayoutAreaDataBakeSites.allow at #5940's head) and holds each node repo to its own root layout-area-data-bake.allow: NEW, MORE and a MISSING allow-file are red; on a pull request the base tree is scanned too, so ADDED units (even with their line), a GROWN/RAISED allow-file and a file the PR converted while keeping its line (STALE) are red. A stale line the PR did not cause is a warning, so a concurrent merge never reds unrelated PRs. The lane fetches it at the scripts ref, self-tests, then gates; core's workflow-shell lane runs the self-test on the PR that changes it. Documented in Doc/GUI/DataBinding and the new-repo skill. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results (shard 0) 1 files 1 suites 3m 12s ⏱️ Results for commit 43cc312. |
Test Results (shard 2)1 032 tests 1 032 ✅ 3m 34s ⏱️ Results for commit 43cc312. |
Test Results (shard 1) 1 files 1 suites 4m 3s ⏱️ Results for commit 43cc312. |
Test Results (shard 3)2 849 tests 2 847 ✅ 8m 42s ⏱️ Results for commit 43cc312. |
Test Results (shard 5) 4 files 4 suites 11m 9s ⏱️ Results for commit 43cc312. |
Test Results (shard 4)835 tests 644 ✅ 7m 52s ⏱️ Results for commit 43cc312. |
Test Results 19 files 19 suites 38m 35s ⏱️ Results for commit 43cc312. |
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
The satellite half of the layout-area data-bake ratchet. A new .github/scripts/check-layout-area-data-bake.py counts layout areas that load data on the hub and build controls out of the values; the shared node-repo-validate.yml lane fetches that script at its scripts ref, runs its self-test, and holds each caller's root layout-area-data-bake.allow to a shrink-only rule (on pull requests the base commit's tree is exported and scanned with the same scanner, feeding the ADDED rule and the base-side allow-file comparison); dotnet-test.yml runs the same self-test in this repository so a broken scanner reds core before it reds the fleet; Doc/GUI/DataBinding gains the verdict table and the new-repo skill the matching allow-file row.
The diff is incomplete: the new script's patch is truncated at 20,000 of 33,059 characters, so the tail of check() (the STALE/GREW/RAISED enforcement and the inherited-STALE warning), --report, the 27-case self-test with its negative controls, and main() were not read, and nothing here asserts anything about them. Clean in the readable portion: the scanner masks comments and string/char literal contents (raw, verbatim and interpolated forms) so literals are never scanned as code; the unit, untyped-lambda, outermost-unit and same-file loader-helper logic is coherent; and the ratchet is fail-closed — a missing or malformed allow-file, a base commit absent from the checkout, or a failed git archive is red, never skipped — with the base tree extracted via tarfile's filter="data". The lane's fetch step fails closed too (an empty scripts-ref, a failed fetch, or a fetched body whose first 400 bytes do not name the script each exit 1), and the docs and skill row match the script's visible verdicts.
Not verifiable from this diff: the claimed line-for-line parity with #5940's LayoutAreaDataBakeRatchetGuard and the measured reproduction of test/LayoutAreaDataBakeSites.allow (that guard's source is not part of this change); the seven satellite allow-file seeds, their required contexts, and the stated merge ordering — MISSING is red by design, so a satellite without its seed goes red on its first run once this lane reaches it; and the validate job's checkout, on which the base-sha question turns.
Findings: 0 blocking · 0 should-fix · 1 question · 1 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
nit .github/scripts/check-layout-area-data-bake.py
In parse_allow, `raw.split("#", 1)[0].strip()` executes only when the line's first non-space character is `#` — where the split's prefix is always empty — so the expression reads like inline-comment stripping but only ever discards full-line comments: a trailing note after an entry (`Src/A.cs<TAB>3 # converted in #123`) is a red parse error, not a stripped comment. That strictness matches the two-token grammar the error message states, but a plain `if raw.lstrip().startswith("#"): continue` would say the same without implying behavior the code does not have.
Internal review of 43cc3122179d25eac5101d0808c69367592689b1 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| run: | | ||
| set -euo pipefail | ||
| if [ "$IS_PR" = "true" ]; then | ||
| python3 "$RUNNER_TEMP/check-layout-area-data-bake.py" --root . --base "${BASE_SHA}" |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The gate hands github.event.pull_request.base.sha to --base, and the script's base_state answers a base commit missing from the checkout with a red refusal ('refusing rather than passing on no evidence') — the right polarity, but it makes the validate job's checkout the load-bearing precondition, and the checkout step is not part of this diff. No step visible in this lane passes a base sha (check-covers.py runs with --root . only), and the PR body's seed verifications ran locally against origin/main, not in the lane: if the lane's checkout is a shallow fetch of the pull-request merge ref, the base commit is absent and every satellite pull request reds on the BASE refusal from the gate's first run after merge. Whether that checkout fetches github.event.pull_request.base.sha (fetch-depth 0, or an explicit fetch of the base branch) cannot be determined from the diff. A smaller claim on the same step: the comment above BASE_SHA says 'an empty base on a PR is red', but the readable portion of the script enters the base comparison under `if base:` truthiness, so an empty --base would silently skip the PR-only rules unless main() — beyond the truncation — rejects it; real pull_request events always carry a base.sha, so the step never sends an empty one.
There was a problem hiding this comment.
Both halves checked against the lane, no change needed.
The base commit is in the checkout. The validate job's checkout is shallow (fetch-depth: 1) only as a log-volume measure; the very next step, Full history and tags, quietly (already on main, lines ~122–131 of this file), runs git fetch --tags --prune --unshallow origin '+refs/heads/*:refs/remotes/origin/*' "$HEAD_SHA" and then REFUSES to continue if git rev-parse --is-shallow-repository is still true. github.event.pull_request.base.sha is a commit on the base branch, so it is reachable from refs/remotes/origin/* after that step, which precedes the ratchet steps in the same job. If that ever stopped holding, the script's BASE refusal is red and names it, which is the polarity you noted.
An empty --base is red, not a silent skip. main() rejects it before check() is reached (if args.base is not None and not args.base.strip(): … return 1, line ~620, beyond the truncation), so the if base: truthiness in check() only ever separates 'no --base passed' (push/schedule) from a real sha. The step chooses between the two invocations on the EVENT, so the comment 'an empty base on a PR is red' is what the code does.
Ordering (PR body: satellites land first) — verified today: all seven repositories whose ci.yml calls node-repo-validate.yml@main carry layout-area-data-bake.allow at the root of their default branch, so the first run after this merges is not MISSING anywhere.
What
The satellite half of the data-binding ratchet. Core's
LayoutAreaDataBakeRatchetGuard(#5940) stops core from regrowing views that load data on the hub and bake it into controls. This PR gives every node repository the same guard, through the shared lane:.github/scripts/check-layout-area-data-bake.pyis a line-for-line port of the guard's scanner:LoadPattern,ControlPattern, the unit header, the untyped(host, ctx) =>lambda, same-file loader helpers, and the outermost-unit rule. Parity measured: run on feat(layout): templates first, data later — Markdown Edit reference conversion + shrink-only ratchet #5940's tree oversrc/,memex/andsamples/, it reproducestest/LayoutAreaDataBakeSites.allowexactly (37 files, 73 units,diffempty). The self-test plants the guard's own cases. It runs 27 cases, and negative controls (disabling the STALE rule or the ADDED rule) turn them red.node-repo-validate.ymlfetches the script at the scripts ref, runs the self-test, and then runs the gate. The gate checks the caller's ownlayout-area-data-bake.allow, which sits at the repository root. It runs as steps inside the existingvalidate / Validate node reposjob, so no new required context is needed. All seven callers require that context today (measured: classic protection on six repos, ruleset19153714on Education).dotnet-test.ymlruns the script's self-test in the workflow-shell lane, so a broken scanner turns this repository red before it turns every satellite red.The rule
NEW,MOREMISSINGADDEDGREW/RAISEDSTALESTALE(inherited)main::warningnaming the one-line tidyThe inherited STALE case is deliberately a warning. Core's guard makes every STALE line a warning, because two converting PRs that merge concurrently would otherwise turn
mainred. Here the converting PR itself is held to the rule, and only a race it did not cause is a warning.ADDEDstops anyone from re-using the spare allowance, so a warning leaves nothing open.Ordering: the satellites land FIRST
All seven callers use
node-repo-validate.yml@mainwithscripts-ref: main(measured from each repo'sci.yml). Merging this PR therefore reaches every satellite on its next run, and with no allow-file that run isMISSING: red. So the seven allow-file PRs merge first, then this one. A root allow-file is inert until this lane reads it. Each seed was verified locally with this exact script against its PR head, using--base origin/main.Docs
Doc/GUI/DataBindinghas a new section, "The 'load, then bake' shape is ratcheted in every repository". The new-repo skill's allow-file table has a new row.No public surface changes, no catalog keys, no in-mesh source, and nothing to recycle.
Pairs-with: none — CI-only change (a script, two workflow steps, docs); no public C# surface is added or removed.