Repository navigation
fix(ci): the validate lane compares what the two package enumerators RETURN, not what they declare (#4774) - #5455
Conversation
…RETURN, not what they declare (#4774) A node repo enumerates its top-level packages twice: the platform's canonical gen-manifests.py (`plugin_dirs`, the verdict) and its own scripts/validate-repos.py. Each satellite's check-skip-sets.py compared the two DECLARED skip sets, and plugin_dirs applies a rule in no declaration — a top-level dot-directory is never a package — so equal declarations over different enumerations were green. check-package-enumeration.py, fetched and run by node-repo-validate.yml beside the canonical, calls validate-repos.py's `package_dirs(root)` and the canonical's `plugin_dirs` on the checkout AND on a fixture holding every declared name, every top-level dir of the checkout, a probe dot-directory and a probe package — so a disagreement that needs a directory the tree does not contain today is red now. First run against MeshWeaver.Plugins found a live one: `dist` in both of its SKIP sets and not in the gen-manifests.config.json the canonical actually reads. A caller with no package_dirs yet is NAMED (`NOT compared`), not failed: adoption is one PR per satellite, and reddening first would red every satellite's required validate context at once. The flip is a change to the guard once all seven callers have adopted. Everything else is red. Self-test (11 cases, both directions, on tree and fixture) runs in the lane and in core's CI. Refs #4774, refs #4777. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate correctness and contract-enforcement issues remain in the enumeration guard, with documentation updates also outstanding.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (4)
What changed in this PR
This PR adds a centralized CI guard comparing satellite and canonical package enumeration, with workflow and documentation updates.
Changes:
- Adds checkout and fixture-based enumeration checks.
- Integrates the guard and self-tests into CI workflows.
- Updates the enumeration contract and adoption documentation.
| File | Review findings |
|---|---|
src/MeshWeaver.Documentation/Data/Architecture/ModuleVersioning.md |
Two nit findings remain (2 votes; 1 vote): update the outdated config table and correct adoption instructions. |
.github/workflows/node-repo-validate.yml |
No final findings. |
.github/workflows/dotnet-test.yml |
No final findings. |
.github/scripts/gen-manifests.py |
One nit finding remains (2 votes): update the obsolete missing-config diagnostic. |
.github/scripts/check-package-enumeration.py |
Five moderate findings remain (3, 1, 2, 1, and 1 votes): enforce list results, handle duplicates, verify main() delegation, avoid probe-name collisions, and register dynamically loaded modules. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| package_dirs = getattr(validator, "package_dirs", None) | ||
| if not callable(package_dirs): | ||
| say("::warning::package enumeration NOT compared: scripts/validate-repos.py exposes no " | ||
| "package_dirs(root), so this repo's second enumerator cannot be called " | ||
| "(MeshWeaver#4774). Adopt: define package_dirs(root) — SKIP plus the dot-directory rule " | ||
| "— and enumerate through it in main().") |
There was a problem hiding this comment.
Fixed in 3bdbb9d: new main_delegates() parses validate-repos.py statically and reds unless there is exactly one top-level main() that calls package_dirs(...) and contains no iterdir() call of its own. Two self-test cases cover it (main never calls it; main calls it AND walks the tree itself). Measured on all seven satellite adoptions: each main() makes 1 package_dirs call and 0 iterdir calls.
| package in another — and `validate-repos.py`'s `package_dirs(root)` must | ||
| return what `plugin_dirs` returns, which the lane's | ||
| `check-package-enumeration.py` asserts on the checkout and on a fixture. |
There was a problem hiding this comment.
Fixed in 3bdbb9d: the missing-config refusal now says validate-repos.py's package_dirs(root) must return what plugin_dirs does and names check-package-enumeration.py. The canonical self-test stays green.
| **The guard is the platform's, and it calls both enumerators.** `node-repo-validate.yml` fetches | ||
| `.github/scripts/check-package-enumeration.py` beside the canonical and runs it on every caller. The | ||
| contract is one function: the caller's `scripts/validate-repos.py` exposes | ||
| `package_dirs(root) -> list[Path]` — its skip set **and** the dot-directory rule, a pure function of | ||
| the top-level names — and its own `main()` enumerates through it. The guard then compares what the two | ||
| enumerators *return*, twice: |
There was a problem hiding this comment.
Fixed in 3bdbb9d: the skip row of the config table now states the package_dirs / effective-enumeration contract and points to the guard section below; no remaining check-skip-sets.py contract text in that page except the historical paragraph explaining why it was blind.
… a list; the config refusal and the doc table name the new contract Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
What
node-repo-validate.ymlnow fetches and runs.github/scripts/check-package-enumeration.pybeside the canonicalgen-manifests.py. It calls the caller'sscripts/validate-repos.py→package_dirs(root)and the canonical'splugin_dirs(root)and compares what they return:Why (Refs #4774)
Each satellite's own
check-skip-sets.pycompared the two declared skip sets;plugin_dirsalso drops every top-level dot-directory, a rule in no declaration — so equal declarations over different enumerations were green. The guard's first run againstMeshWeaver.Pluginsfound a live one the old guard could not see:distin both of Plugins' SKIP sets and NOT in thegen-manifests.config.jsonthe canonical actually reads (the verdict enumerator reads neither SKIP).Ordering (no fleet-wide red)
A caller whose
validate-repos.pyhas nopackage_dirsyet is named (::warning::package enumeration NOT compared), not failed. Everything else is red: missing/unloadablevalidate-repos.py, apackage_dirsthat raises or returns a non-list, an unloadable canonical, a missing config, any disagreement. The satellite adoptions are ready (all seven callers: Plugins, Education, Reinsurance, SocialMedia, Manufacturing, Crm, FundReporting) and are opened after this lands so their CI exercises the guard; the flip of the absent case to red is a follow-up change to this guard once all seven have merged — never a date.Measured locally against each satellite with the adoption applied: guard green on checkout + fixture in all seven; canonical
--check/--check-versionsgreen on every committed tree (no lock or version moves).Also
dotnet-test.ymlruns the guard's--self-test(14 cases: both directions on tree and fixture, main() must walk THROUGH package_dirs, every unreadable input red) so a core PR that breaks it is red here.check-skip-sets.pyas the contract now name the platform guard (docstring-only; the copy guard reports code-identical).ModuleVersioning.md→ "The declaredskipis NOT the whole rule" describes the guard, the contract and the ordering.Local:
check-package-enumeration.py --self-test14/14;gen-manifests.py --self-testgreen;check-guard-step-masking(107 steps, 0 violations),check-workflow-timeouts,check-workflow-yaml-keys,check-workflow-shell,check-reusable-workflow-runners,check-workflow-permission-pairinggreen;MeshWeaver.DocumentationRelease-warnaserrorbuild clean.Refs #4774 · Refs #4777
Pairs-with: none — removes no public type or member (Python and workflow only).
🤖 Generated with Claude Code