Skip to content

The two package enumerators disagree on dot-directories, and check-skip-sets.py cannot see it #4774

Closed

Description

@rbuergi

Copilot review finding, raised on two satellite PRs and unanswerable inside either of them: MeshWeaver.Reinsurance#225 (4050708495) and MeshWeaver.SocialMedia#208 (4050491291). Verified 2026-09-19; the fix is cross-repo, which is why it is an issue rather than part of #4775.

The divergence

Core's canonical plugin_dirs() grew an implicit dot-directory rule:

return sorted((d for d in root.iterdir()
               if d.is_dir() and d.name not in skip and not d.name.startswith(".")),
              key=lambda d: d.name)

The rule itself is right and its comment says why — tooling writes .example-check/ and .platform-scripts/ at the top level, and hashing one as a module is how a scratch build ends up demanding a manifest.lock.

But this repository enumerates its top-level packages twice, and the other enumerator did not get the rule. Measured on origin/main, 2026-09-19:

repo validate-repos.py's enumerator dot rule?
Reinsurance :288 repos = [d for d in root.iterdir() if d.is_dir() and d.name not in SKIP] no
SocialMedia :228 same no
Manufacturing :228 same no
Crm :799 same (plus three bare root.iterdir() sites at :668, :726, :765) no
Plugins :1060 same (plus three bare sites at :779, :948, :987) no
Education :441 repos = [d for d in root.iterdir() if is_course_dir(d)] n/a — different shape

So a top-level .foo that is in neither declared skip set is skipped by gen-manifests.py and walked by validate-repos.py — validated as nodes it does not contain, which is the first half of the exact failure check-skip-sets.py's own docstring names.

Why the guard is blind to it

check-skip-sets.py compares validate-repos.SKIP == gen-manifests.skip_set(root) — the two declarations. The dot rule is not in either declaration; it is an extra filter applied after. Equal declared sets, different effective enumerations, and the guard passes.

That is the shape check-skip-sets.py was written against, one level up: its docstring says "a guard whose subject moved out from under it and that answers green has checked nothing, which is worse than the drift it exists to catch." The subject moved when the dot rule landed in the canonical.

What would close it

Either half of the finding's own remedy, applied consistently:

  • (a) give validate-repos.py's enumerator the same dot rule, and make check-skip-sets.py compare the effective enumerations (run both against a fixture tree containing a .foo directory) rather than the declared sets — so the next implicit rule cannot hide from it either; or
  • (b) take the implicit filter out of plugin_dirs() and require every dot-directory to be declared in each repo's skip — which is a change to five repos' config files, and regresses the scratch-directory case the comment documents.

(a) is the one that matches the canonical's stated intent. Not done in #4775 because validate-repos.py and check-skip-sets.py are satellite-local with no canonical, their enumerators differ per repo (four call sites in Crm and Plugins, a different shape in Education), and it therefore needs a per-repo change in six repositories rather than one edit here.

Not currently firing

No top-level dot-directory is undeclared on any satellite's main today, so nothing is red. This is latent: the next tool that writes one, or the next repo that hits .example-check/, meets it.

Activity

  1. rbuergi commented on Sep 20, 2026

    @rbuergi
    ContributorAuthor

    The reason this needs six edits is itself the deeper defect

    The issue explains why (a) cannot be one change here:

    "validate-repos.py and check-skip-sets.py are satellite-local with no canonical, their enumerators differ per repo (four call sites in Crm and Plugins, a different shape in Education), and it therefore needs a per-repo change in six repositories rather than one edit here."

    That is an accurate description of the cost — and it is also a statement that these two scripts sit outside the architecture the platform otherwise holds itself to. Doc/Architecture/ModuleBuildArchitecture is explicit that scripts are centralized: the lane fetches the platform's copy at the pin, and repos keep only allow-files. AGENTS.md says the same in the imperative — "never hand-roll (or copy-paste) a node repo's CI" — and the whole node-repo-validate lane exists to make that true, fetching check-resolver-copy.py, check-workflow-timeouts.py, check-pr-secret-preflight.py and the rest from core at scripts-ref.

    validate-repos.py is the one that did not move. Everything the issue measures follows from that:

    • The divergence exists at all. A canonical plugin_dirs() grew a rule; six private copies of a second enumerator did not. With one fetched copy there is no second enumerator to fall behind.
    • It differs per repo — four call sites in Crm and Plugins, a different shape in Education. That is drift that only private copies can accumulate, and it is the same class as resolve-platform.py's "70 re-copy commits in 30 days" (The vendored resolve-platform.py is 70 re-copy commits in 30 days — the guard's own docstring names 'no copy' as the end state #4787).
    • The guard is blind by construction. check-skip-sets.py compares two declarations because it is the only thing a per-repo guard can compare without running the canonical. Comparing effective enumerations — remedy (a) — means running core's plugin_dirs() against a fixture, which is exactly the fetched-canonical shape.

    So (a) done properly is not six independent edits: it is "give validate-repos.py a canonical in core and have the lane fetch it", after which the dot rule is fixed once and check-skip-sets.py can compare what the two enumerators actually return rather than what they declare.

    🚨 The ordering matters, and it is the reverse of the obvious one. Tightening check-skip-sets.py first would red all five satellites' required validate context at once for a condition none of them can fix without their own PR — the fleet-wide-red shape this repo has been bitten by before (a dated guard that reddened every repository at midnight). The safe sequence is: canonical lands in core → the lane fetches it → repos adopt → the guard tightens last, when it can only pass.

    Nothing here is urgent, and the issue says so — no top-level dot-directory is undeclared on any satellite's main today. I am recording the shape rather than starting a six-repo change, because the version worth doing is the centralisation, and that is a scope call rather than a mechanical sweep.

    Related: #4787 (the same class, measured on resolve-platform.py), #4777 (vendored --resolve defects with no guard comparing a vendored copy).

    2026-09-20, Claude Opus 5 — read-only.

  2. added
    bugSomething isn't working
    area:buildCore subsystem: the build - manifests, bundles, bakes
    sev:LLow - cosmetic, rare edge case, or developer-only
    on Sep 20, 2026
  3. rbuergi commented on Sep 21, 2026

    @rbuergi
    ContributorAuthor

    Triage 2026-09-21: still latent, sev:L stands

    Blocked on: a per-repo change in six repositories (four call sites in Crm and Plugins, a different enumerator shape in Education), none of which core can commit. Core's half — the canonical's dot rule — is already there and is the behaviour the issue calls correct.

    sev:L is right and worth defending, because the reasoning in this issue argues higher than the label: nothing is red on any satellite's main today, no top-level dot-directory is undeclared anywhere, and the failure needs a tool to write one first. That is the rubric's "rare edge case" — with the qualification that when it fires it is not cosmetic, it validates nodes a package does not contain. If a .foo appears on a satellite, this becomes sev:M immediately.

    One thing to carry into the fix, because it is the issue's own lesson applied to itself

    Option (a) — give validate-repos.py the dot rule and make check-skip-sets.py compare the effective enumerations against a fixture tree containing a .foo — is the one to take, and not only because it matches the canonical's stated intent. Option (b) regresses the scratch-directory case the canonical's comment documents, and more importantly it leaves the guard still comparing declarations, which is the defect: equal declared sets, different effective enumerations, green.

    🚨 The same shape is live one level along in this backlog. #4777 asks for a guard comparing a vendored gen-manifests.py to the canonical; a guard written there as "does the copy declare these three features" would pass having checked nothing, exactly as check-skip-sets.py does here. Both guards want to compare behaviour against a fixture, not a list of properties — and the fixture is the cheap part: one tree with a .foo in it answers both enumerators at once.

    check-skip-sets.py's own docstring already states the rule it is violating — "a guard whose subject moved out from under it and that answers green has checked nothing, which is worse than the drift it exists to catch" — so the finding is not that the guard is wrong in principle, it is that its subject moved when the dot rule landed in the canonical and nothing re-derived it.

  4. rbuergi commented on Sep 22, 2026

    @rbuergi
    ContributorAuthor

    Core's half is merged: gen-manifests.py --list-packages exports the EFFECTIVE enumeration (PR #5180)

    Remedy (a) needs a guard that compares effective enumerations, and a satellite guard cannot run core's plugin_dirs() on its own — so the canonical now answers the question directly:

    MW_REPO_ROOT=$PWD python3 <platform>/.github/scripts/gen-manifests.py --list-packages

    stdout is the package names only, one per line, sorted — a check-skip-sets.py diffs its own enumerator's output against it and needs no copy of either rule; the count goes to stderr so an empty answer is still distinguishable from a broken one. Reach is at the pin every satellite lane already fetches (centralized-gen-manifests), no per-repo change needed for the export itself. --self-test case 8 pins both directions over a fixture with .example-check/, .platform-scripts/, a declared skip and two real packages; controls measured on both sides (dot rule neutered → exit 1 naming the dot-directories; a banner on stdout → exit 1 naming the disagreement).

    What is still owed is per-satellite, and the ordering matters — each repo's check-skip-sets.py compares against --list-packages, and its validate-repos.py enumerator gains the dot rule (four call sites in Crm and Plugins, a different shape in Education). 🚨 The guard tightens last: tightening first reds five satellites' required validate context at once for a condition none of them can fix without their own PR. Canonical (done) → repos adopt → guard tightens when it can only pass.

    Doc: Doc/Architecture/ModuleVersioning → "The declared skip is NOT the whole rule". Leaving this open for the satellite half.

  5. rbuergi commented on Sep 24, 2026

    @rbuergi
    ContributorAuthor

    The satellite half had been prepared and never opened — now opened.

    Core's half is merged: #5180 (the canonical exports its effective enumeration) and #5455 (2026-09-23T05:52Z: node-repo-validate.yml runs check-package-enumeration.py, comparing what validate-repos.py's package_dirs(root) and the canonical's plugin_dirs(root) RETURN, on the checkout and on a fixture with a probe dot-directory). Until a caller has package_dirs, the guard only warns for it.

    The adoptions sat as pushed branches (all committed 2026-09-23 ~04:55Z) with no pull request for ~21 h. Opened from those branches, unchanged, auto-merge armed so each lands only on its own green CI:

    repo PR branch
    MeshWeaver.Reinsurance Systemorph/MeshWeaver.Reinsurance#237 chore/4774-package-dirs
    MeshWeaver.SocialMedia Systemorph/MeshWeaver.SocialMedia#221 chore/4774-package-dirs
    MeshWeaver.Manufacturing Systemorph/MeshWeaver.Manufacturing#117 chore/4774-package-dirs
    MeshWeaver.Crm Systemorph/MeshWeaver.Crm#153 chore/4774-package-dirs
    MeshWeaver.FundReporting Systemorph/MeshWeaver.FundReporting#5 chore/4774-package-dirs
    MeshWeaver.Plugins Systemorph/MeshWeaver.Plugins#2339 chore/4774-4777-canonical-gen-manifests (258 commits behind main — may need a merge of main)

    Still owed after these merge:

    1. MeshWeaver.Education — no branch exists; its enumerator has a different shape (is_course_dir), so its package_dirs is a separate small change.
    2. The guard's flip in core: a caller without package_dirs goes from warning to red — only once all seven have merged (fix(ci): the validate lane compares what the two package enumerators RETURN, not what they declare (#4774) #5455's stated ordering), never before.

    This issue stays open until both are done. Not established: whether each branch is still green against today's main of its repo — each PR's CI is that measurement.

  6. rbuergi commented on Oct 8, 2026

    @rbuergi
    ContributorAuthor

    Status: six of the seven satellite adoptions opened on 2026-09-24 have merged (Reinsurance#237, SocialMedia#221, Manufacturing#117, Crm#153, FundReporting#5, Plugins#2339). Still owed:

    1. MeshWeaver.Education. Its scripts/validate-repos.py on main still has no package_dirs and enumerates through is_course_dir (line 441), so check-package-enumeration.py names it NOT compared.
    2. The guard flip in core (a caller without package_dirs goes from warning to red), only after Education has adopted.

    sev:L sweep, Claude Opus 5.5 (agent session).

  7. rbuergi commented on Oct 9, 2026

    @rbuergi
    ContributorAuthor

    Education is still not adopted, and the blocker is one step earlier than the 10-08 comment said. Measured today:

    • MeshWeaver.Education main scripts/validate-repos.py still enumerates through root.iterdir() + is_course_dir (line 441). It has no package_dirs, and no open PR adds one.
    • I added package_dirs(root) locally and routed main() through it, then ran core's .github/scripts/check-package-enumeration.py --root <edu> --canonical .github/scripts/gen-manifests.py. Result: ::error:: … gen-manifests.config.json not found. Education has no scripts/gen-manifests.config.json: its validate call does not set centralized-gen-manifests, so it still runs its vendored gen-manifests.py.
    • So adopting package_dirs alone turns Education's validate lane red, because the guard only says NOT compared while the function is absent. Education first has to centralize its manifest generator (add scripts/gen-manifests.config.json with its skip list + hashModuleSources, set centralized-gen-manifests: true), and then adopt package_dirs. Both can go in one PR.

    Nothing was pushed. Step 2, the core guard flip, still waits on that PR.

    sev:L sweep, Claude Opus 5.5 (agent session).

  8. rbuergi commented on Oct 9, 2026

    @rbuergi
    ContributorAuthor

    Correction to my comment above: the Education step is done in one PR, Systemorph/MeshWeaver.Education#391 (open). It adds scripts/gen-manifests.config.json, sets centralized-gen-manifests: true, and adds package_dirs(root) routed through main(). Locally against core main, the canonical --check and --check-versions are both green (11 modules), and check-package-enumeration.py agrees on 11 packages on the checkout and 12 on the fixture. After it merges, only the core guard flip (step 2) remains.

    sev:L sweep, Claude Opus 5.5 (agent session).

  9. rbuergi commented on Oct 9, 2026

    @rbuergi
    ContributorAuthor

    Closing as done. Both remaining steps have merged:

    1. Education adopted: Systemorph/MeshWeaver.Education#391 (merged). Its validate job logged ✓ both package enumerators return the same 11 package(s) on this checkout and the same 12 on a 22-name fixture.
    2. Guard flip: ci(validate): a node repo without package_dirs is red, not merely named #6358 (merged). A caller with no package_dirs(root) is now ::error::, and the self-test passes 14/14. The satellites call the lane @main, so this is in force for all of them now.

    Denominator: every caller of node-repo-validate.yml found by code search (Crm, Education, FundReporting, Manufacturing, Reinsurance, SocialMedia, plus Plugins' resolve lane) has def package_dirs in scripts/validate-repos.py on main, checked today.

    sev:L sweep, Claude Opus 5.5 (agent session).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:buildCore subsystem: the build - manifests, bundles, bakesbugSomething isn't workingfeature:package-enumerationFeature: package enumerationsev:LLow - cosmetic, rare edge case, or developer-only

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions