Repository navigation
Conversation
Adds the `Confidential terms` job to dotnet-test.yml, a NEEDS of `Consolidate test results` with an explicit fail step. The denylist is the CONFIDENTIAL_TERMS secret (newline-separated, case-insensitive substrings, `word:<term>` for whole words), never a file in this public repository; `preflight` asserts it is present, fork and Dependabot PRs are exempt by event, and the log prints only path:line. The checker (.github/scripts/check-confidential-terms.py) is repository-agnostic and self-tests against a scratch repository. AGENTS.md documents the rule. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results (shard 3)2 307 tests - 60 2 307 ✅ - 60 6m 33s ⏱️ -10s Results for commit cc4a925. ± Comparison against base commit d1414f8. This pull request removes 282 and adds 220 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results (shard 4) 4 files ± 0 4 suites ±0 17m 1s ⏱️ +54s Results for commit cc4a925. ± Comparison against base commit d1414f8. This pull request removes 226 and adds 322 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
![systemorph-com[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a CI gate that fails the build when a confidential term appears in the tracked tree, plus the scrub the gate exists to enforce, plus the roster split that moves the client estate out of the public file. (1) Guard: new job Confidential terms in dotnet-test.yml running .github/scripts/check-confidential-terms.py — denylist from the CONFIDENTIAL_TERMS secret (never in the repo; newline-separated, # comments, case-insensitive substrings, word:<term> whole-word), scanned over every tracked file's CONTENT and PATH, hits reported as path:line only, secret absent = notice + exit 0, --self-test with synthetic terms; wired as a needs of Consolidate test results with an explicit fail step (same shape as the licence gate), asserted in preflight, fork/Dependabot exempt by EVENT — never by "secret empty". (2) Scrub: ~350 files replace client names, client staff names, client tenants/subscriptions/vaults/registries/hosts and operated-instance hosts with neutral placeholders (globex/fabrikam/umbrella-style ids, <tenant-id>/<namespace> placeholders); AGENTS.md gains a "Confidential terms — this repository is PUBLIC" section. (3) Roster: .github/acr-retention/instances.json drops the client estate's repositories/registries/instances lines; they move to the ACR_RETENTION_PRIVATE_ROSTER secret, merged over the committed file by lock-pinned-digests.py's new read_roster_document (key declared in both = refusal; malformed private JSON = unreadable roster, red); workflows pass MW_ACR_PRIVATE_ROSTER through and notice when it is absent. Checked: the checker line by line (term parsing, both match modes, path hits, ls-files/grep failure handling, exit codes, every self-test arm), the workflow wiring (job-level and fail-step conditions including the precheck-skip and fork/Dependabot arms — skipped jobs do not red consolidation for forks), merge_roster/read_roster_document against each roster reader's error doctrine, and every scrub hunk I could read (comments and templates only in bicep/yaml/sh where seen). -z differs, and under the documented layout the printed hit would carry the matched line (the term itself) into the public log — please answer that one before merge with a runner-verified layout plus a two-hit self-test arm.
Findings: 0 blocking · 2 should-fix · 4 question · 1 nit
Internal review of 2c60d959dc6f6f5876da88a453ccbed1a1c8c548 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| # With -z the fields are NUL-separated: path \0 line \0 content. | ||
| parts = record.split(b"\0", 2) | ||
| if len(parts) >= 2: | ||
| hit = f"{parts[0].decode('utf-8', 'surrogateescape')}:{parts[1].decode()}" |
There was a problem hiding this comment.
question — The parser assumes git grep -n -z emits records shaped path\0line\0content separated by newlines (comment at line 80; stdout.split(b"\n") at 77, split(b"\0", 2) at 81). Git's documented -z behavior ("Use \0 as the delimiter for pathnames in the output, instead of :") describes a single NUL after the pathname — under that layout parts[1] is line:content, so the hit built at line 83 would be path:line:<matched line>, and ::error::confidential term found at {hit} would print the matched line — term included — into the public Actions log: the exact disclosure the docstring (line 18) promises never to make. Under the older all-NUL layout (NUL-terminated records) the same split instead yields only the FIRST hit per grep run. The single-hit self-test arms would catch the first layout (job red — fail-safe, since the self-test step runs before the real check and prints only synthetic terms), but no arm exercises two content hits, so the second layout degrades the hit list silently. Please state the record layout you verified on the runner (with its git version) and add a two-hit self-test arm that pins it. If the runner emits the documented single-NUL layout this is blocking: the job can never go green, and on any path where the self-test were bypassed the error lines would carry the term.
There was a problem hiding this comment.
Verified on git 2.50.1: git grep -n -z emits path\0line\0content\n per record (checked with od on a two-hit file). Hardened anyway: only the leading DIGITS of the line field leave the function — under a path\0line:content layout the content is cut, never printed. Added a two-hit arm (notes.md:2 and notes.md:3 both reported) that pins the layout.
| # -w whole words for the `word:` group. The matched line is DISCARDED below — only | ||
| # path and line number leave this function. | ||
| result = subprocess.run( | ||
| ["git", "-C", root, "grep", "-I", "-n", "-i", "-F", "-z", *extra, |
There was a problem hiding this comment.
question — -I excludes binary-detected blobs entirely — the self-test asserts it ("binary files are skipped") — so a client identifier inside a UTF-16 text file, a PDF, an image or an office document committed to the tree is never checked, while the docstring's first line claims "anywhere in the tracked tree". If text-only is intended, the docstring (and the AGENTS.md rule) should say so; if not, binary content needs a story (raw byte-pattern search over blobs, or at least flagging newly added binary artifacts). Which is it?
There was a problem hiding this comment.
Text-only is intended: -I skips binary content, while every file's PATH is still checked. The docstring and the AGENTS.md paragraph now say so explicitly (binary content is out of the gate's reach and is a review matter).
| nonlocal failures | ||
| print(f"[{'PASS' if condition else 'FAIL'}] {message}") | ||
| if not condition: | ||
| failures += 1 |
There was a problem hiding this comment.
nit — failures = 1 where the final message prints {failures} arm(s) failed — with several failing arms the count is wrong (looks like += 1 was meant). Exit code is unaffected.
There was a problem hiding this comment.
The line reads failures += 1 (line 118 on this head); the count is correct.
| MeshWeaver is an open-source framework for building **data meshes**: documents, data, code, AI agents, and UI live together as addressable nodes on a mesh — versioned, collaboratively editable, vector-searchable, and processed by an actor-model message hub with a reactive Blazor UI. **Memex**, the portal application included in this repository, is a knowledge portal built on it where people and AI agents work on the same content. | ||
|
|
||
| **See it live at [memex.meshweaver.cloud](https://memex.meshweaver.cloud)** — try the portal and read the full documentation at [memex.meshweaver.cloud/Doc](https://memex.meshweaver.cloud/Doc). The same docs are in this repo under [`src/MeshWeaver.Documentation/Data/`](src/MeshWeaver.Documentation/Data/). | ||
| Read the full documentation online at [memex.meshweaver.cloud/Doc](https://memex.meshweaver.cloud/Doc). The same docs are in this repo under [`src/MeshWeaver.Documentation/Data/`](src/MeshWeaver.Documentation/Data/). |
There was a problem hiding this comment.
should-fix — This PR removes "See it live at memex.meshweaver.cloud" yet keeps "Read the full documentation online at memex.meshweaver.cloud/Doc" — the same host, in the same paragraph it edits. The new AGENTS.md section added by this PR says generic docs do not list the instances you operate. Either that host is a term the gate must treat as confidential (then the kept link contradicts the new rule — and if it is in CONFIDENTIAL_TERMS, the gate this PR adds keeps the repository red), or it is not (then dropping the live-demo line cost the README its most useful pointer for no confidentiality gain). Please reconcile — scrub it consistently or restore it consistently, and if there is an exception for the public docs host, write it into the AGENTS.md section (whose tail I could not read — the patch is cut).
There was a problem hiding this comment.
Deliberate: the live-instance pointer is an instance listing, which the maintainer's rule removes from generic docs; the documentation link stays as the product's documentation URL. The host is not a denylist term — the gate covers client identities only — so the kept link does not red the gate.
| # "every foreign host is declared, and none of them fleet-unlockable". The comment | ||
| # here used to assert exactly that. Measured on the 2026-09-17→21 schedules: the | ||
| # lane printed *"pins images ONLY in memexaksacr43rzd6faaix36.azurecr.io, every one | ||
| # lane printed *"pins images ONLY in clientacr2.azurecr.io, every one |
There was a problem hiding this comment.
should-fix — The lane's own output names identifiers that this PR moves into a secret: the comment here quotes a real report line ("pins images ONLY in clientacr2.azurecr.io, every one of them declared third-party") — i.e. pinned registry hosts, repo ids and overlay paths are printed into the run log of a PUBLIC repository, whose Actions logs are public (the new checker's own docstring says exactly this). After this change the client estate's registries/repositories live in ACR_RETENTION_PRIVATE_ROSTER, but the same diagnostics (check_registry_roster's stale-line and undeclared-host blockers, check_repository_roster's undeclared-repository blocker, the axis-2 pin reports) will still print them on any red fleet run. Please state the acceptance, or redact identifiers that came from the private roster (or from repos only the fleet token can read) from public-run output.
There was a problem hiding this comment.
Fixed in #5851 (merged into this branch): mask_private_roster() registers every repository, registry, instance id/repo/host from the private roster as an ::add-mask:: at the start of lock-pinned-digests.py and derive-combo-instances.py --discover runs, so a red fleet run prints them masked. Called from main only, never from the self-test (its fixtures would mask ordinary words).
| env: | ||
| GH_TOKEN: ${{ steps.fleet-token.outputs.token }} | ||
| # A client estate's roster lines are private — see lock-pinned-digests.py PRIVATE_ROSTER_ENV. | ||
| MW_ACR_PRIVATE_ROSTER: ${{ secrets.ACR_RETENTION_PRIVATE_ROSTER }} |
There was a problem hiding this comment.
question — This step now passes MW_ACR_PRIVATE_ROSTER (secrets.ACR_RETENTION_PRIVATE_ROSTER) to derive-combo-instances.py --discover, but no changed line in .github/scripts/derive-combo-instances.py reads that variable — its diff is rename-only. If --discover's roster comparison does not route through lock-pinned-digests.py's new read_roster_document (added in this PR), the variable is inert here and the client instance, now absent from the committed .github/acr-retention/instances.json, reds the combo lane on fleet runs even with the secret set. Please confirm derive-combo-instances.py --discover actually consumes the merged roster (and if it does, that its own "a client estate is not declared in this file" expectations match).
There was a problem hiding this comment.
Confirmed: derive-combo-instances.py loads lock-pinned-digests.py as a module and reads the roster through lock.read_instance_roster (line ~557), which now goes through read_roster_document — so the merged roster is what --discover sees. It now also calls lock.mask_private_roster() first.
|
|
||
| Full reference: [/i18n](.claude/skills/i18n/SKILL.md) · [Localization.md](src/MeshWeaver.Documentation/Data/Architecture/Localization.md). | ||
|
|
||
| ## 🚨 Confidential terms — this repository is PUBLIC |
There was a problem hiding this comment.
question — The tree is scrubbed, but this is a PUBLIC repository: every prior commit remains cloneable, so all removed identifiers (client names, their tenant/subscription/ACR ids, hosts, staff names) stay readable from history indefinitely — the new gate only stops NEW occurrences. Is history exposure accepted (worth saying next to the new rule, since "is ever committed here" reads absolute), or is a history rewrite / GitHub-side purge planned? If a rewrite is planned, note that GitHub retains old commits in PR refs and caches until support purges them.
There was a problem hiding this comment.
History exposure is out of scope for this change: rewriting history, purging refs or changing visibility are the maintainer's decisions and are listed as such. The AGENTS.md sentence now says the gate stops new occurrences and that what earlier commits hold is a history question for the maintainer.
…s binary content is out of reach Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…uard # Conflicts: # .github/workflows/dotnet-test.yml
|
Updated after main moved: main removed the shared |
Test Results 22 files + 1 22 suites +1 49m 22s ⏱️ + 3m 52s Results for commit 0395064. ± Comparison against base commit 3fc485c. This pull request removes 29 and adds 1338 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Replaces the CONFIDENTIAL_TERMS denylist with the CRM-driven check the
maintainer approved on 2026-09-28 ("build system should not have access to
crm"). The build holds no term list, not in the repo and not in a secret.
The reusable workflow client-names.yml sends the added lines and new paths
(file:line) to the instance that owns the CRM as a Governance/NameCheck
request, over its MCP endpoint, as the build's own service user. It reads
back pass/fail with masked positions (client#n at file:line:column plus the
kind that matched) and annotates them without printing the name or the line.
Fails closed: missing NAME_CHECK_URL/NAME_CHECK_TOKEN, an unreachable
endpoint, a refused token, a timeout or a NotChecked answer is a red job.
Private repos call it with report-only. The self-test runs both arms
against a fake instance. Still a NEEDS of Consolidate test results, and
fork and Dependabot PRs are exempt by event.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
rbuergi
changed the title
|
Reworked in place rather than duplicated (8c9d940). The |
…ely name clients A caller passes paths (newline-separated git pathspecs, ':(exclude)x' to exclude) so a private repo can check only its publicly-bound files. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ill fails) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
check-reusable-workflow-runners: a literal ubuntu-latest in a reusable workflow bills a private caller. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Adopting this PR: the author's session is idle. #5851 merged at f4a681b, so I'm merging On head 7cb7b64 the check was red for a reason unrelated to names. Its log says |
|
This needs an owner to act. The check can only run once the build's service user on the CRM-owning instance exists and both secrets are set under Settings → Secrets → Actions. Per AGENTS.md, if any Dependabot |
There was a problem hiding this comment.
What this head does: adds a CI confidentiality gate with no local term list — .github/scripts/check-client-names.py collects a change's added lines and new/renamed paths (whole tree without --base), submits them as Governance/NameCheck nodes to the CRM-owning instance over its MCP endpoint as the build's own service user, polls for Pass/Fail/NotChecked, and fails closed; .github/workflows/client-names.yml is the reusable job (secrets declared optional so a missing secret is a red job, not a workflow error); dotnet-test.yml adds the client-names job (fork/Dependabot exempt by event) as a NEEDS of Consolidate test results with an explicit fail step; AGENTS.md documents the gate. What I checked: the diff collector and its -U0 line numbering and rename/path handling, the whole-tree fallback, chunking, the MCP-over-HTTP client (initialize/session, SSE parsing, the retry matrix), submit-and-poll with its deadline, the fold in report() and the term masking, main's exit codes, the workflow's env plumbing and BASE_SHA selection (zeros-guard, cat-file check), the job's if, the consolidate fail step and its exemption mirroring, and the AGENTS.md paragraph — fail-closed holds on every path I could trace (missing secret, HTTP error, unreachable, timeout, unknown status, refused tool call, no answer in time). NOT VERIFIED: the check-client-names.py patch is TRUNCATED (first 20,000 of 27,597 characters) — the tail of the file, most of --self-test's fake instance and its assertions, was not read and I assert nothing about it, including the '17/17' claim; the remote side (the Governance/NameCheck watcher in MeshWeaver.Plugins#2507, the CRM fields in MeshWeaver.Crm#157), the assumed MCP create/get tool contract ('Created' prefix, node JSON as text), the service user's actual grants, the org secrets' existence, and what needs.precheck.outputs.skip covers are all outside this diff. Design limits the body accepts, not findings: fork and Dependabot PRs are exempt by event (no secret access), and checking is text-only (binary files and files over 2 MB are skipped). Findings: 3 should-fix (the gate runs the PR head's own checker with the token in env; partial hits are discarded when one chunk answers NotChecked; parse_diff misparses '++ '/'-- ' content lines as file headers), 3 questions (the precheck-skip exemption may bypass the gate for docs-only changes; the remote NotChecked reason is printed verbatim into public logs; no retention for the created NameCheck nodes), 2 nits. Nothing blocking in what I read.
Findings: 0 blocking · 3 should-fix · 3 question · 2 nit
Internal review of c01442fe457fb638b45e027ef9d30a991352530d — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| if: ${{ !cancelled() && needs.precheck.outputs.skip != 'true' && (github.event_name != 'pull_request' || (github.event.pull_request.head.repo.fork != true && github.actor != 'dependabot[bot]')) }} | ||
| uses: ./.github/workflows/client-names.yml | ||
| with: | ||
| script-ref: ${{ github.event.pull_request.head.sha || github.sha }} |
There was a problem hiding this comment.
should-fix — script-ref: ${{ github.event.pull_request.head.sha || github.sha }} makes the code under review perform its own gate: the reusable job checks out check-client-names.py from the PR head and runs it with NAME_CHECK_URL/NAME_CHECK_TOKEN in env, and uses: ./.github/workflows/client-names.yml resolves the reusable workflow itself from the head on same-repo pull_request runs. A commit on the branch can silently neuter this required check (always exit 0) or read the mw_ token while the gate still reports green. Same-repo PRs already receive repo secrets in env, so the exposure class is not new — but for a confidentiality gate, pin the checker to a reviewed ref (the input's own default main, or the PR base) and exercise the branch's own checker changes only in the secretless --self-test step, or record the residual risk deliberately.
There was a problem hiding this comment.
Fixed in 41eab72. script-ref is now the pull request's base sha (the queue's base on merge_group, the pushed commit on main), so the checker that runs with NAME_CHECK_URL/NAME_CHECK_TOKEN is the reviewed one. The branch's own copy is proved in a separate --self-test step with no secret in its env. One fallback, limited to this repository: when the reviewed ref carries no checker at all (true only for this PR, which introduces it) the change's own copy runs, with a notice. Residual risk, stated rather than closed: uses: ./.github/workflows/client-names.yml and dotnet-test.yml itself still resolve from the head on a same-repo pull request, so a branch can still edit the workflow around the checker. That is the same exposure every secret-bearing job in this file has; fork and Dependabot runs never receive the secrets.
| def report(answers: list[dict], say=None) -> tuple[str, list[dict]]: | ||
| """Folds the answers: ('NotChecked', []) wins over ('Fail', hits) over ('Pass', []). Hits are | ||
| re-numbered across chunks so one client keeps one number in this run. Pure but for ``say``.""" | ||
| for a in answers: |
There was a problem hiding this comment.
should-fix — report() raises NotChecked on the first non-Pass/Fail answer before a single hit is annotated, and main's except branch then writes summary('NotChecked', ...) with empty hits — so when one chunk answers NotChecked, hits that other chunks did find (status Fail) are never printed as ::error annotations nor tabled in the step summary. In default fail mode the run is still red and only the diagnostics are lost; with --report-only / --unchecked warn (private repositories) a KNOWN client name exits 0 with only a 'not checked' warning. Annotate the answered chunks' hits first, then raise.
There was a problem hiding this comment.
Correct, fixed in 41eab72. report() now annotates the hits of every chunk that answered Fail first and only then raises NotChecked, which carries those hits. main tables them in the summary and exits 1 when any exist, also under --unchecked warn (only --report-only forgives a hit, as before). Two new self-test cases pin it: the pure fold (hit annotated, then the refusal) and an end-to-end run with one chunk Fail and one NotChecked under --unchecked warn, which must exit 1.
| if raw.startswith("--- "): | ||
| source = raw[4:] | ||
| old = None if source == "/dev/null" else (source[2:] if source.startswith("a/") else source) | ||
| continue |
There was a problem hiding this comment.
should-fix — parse_diff treats any line starting '+++ ' as the new-file header and '--- ' as the old-file header. An added line whose content starts with '++ ' (raw '+++ ') is consumed as a header: path becomes the tail, a bogus line-0 path entry is emitted, the remaining lines of that hunk are attributed to that wrong 'file', and their line numbers run one low (the consumed line never increments). A deleted line starting '-- ' similarly overwrites old. No false pass — the text still reaches the CRM — but the verdict's only output, file:line, is wrong for the rest of the hunk, and files that embed unified-diff text (fixtures, docs about this very gate) hit this on real content. Only recognize the headers while no hunk body is open, or take paths from the diff --git line.
There was a problem hiding this comment.
Correct, fixed in 41eab72. parse_diff now reads the old/new counts from each @@ header and treats exactly that many lines as body, so --- /+++ are file headers only between hunks. The self-test has a diff whose added lines start ++ b/…, -- and @@ : the previous parser attributed them to a bogus path with a line-0 entry (I ran the old function on it), the new one keeps file and line. On the real 12,142-entry diff of this branch old and new give identical output.
| # timeout, NotChecked), and it is a NEEDS of `Consolidate test results` with an explicit fail | ||
| # step there, like every other gate. The fork / Dependabot exemption is the EVENT, nowhere else. | ||
| needs: [precheck] | ||
| if: ${{ !cancelled() && needs.precheck.outputs.skip != 'true' && (github.event_name != 'pull_request' || (github.event.pull_request.head.repo.fork != true && github.actor != 'dependabot[bot]')) }} |
There was a problem hiding this comment.
question — The client-names job (and the matching fail step in Consolidate test results) is also exempt when needs.precheck.outputs.skip == 'true'. precheck is not in this diff: if its skip flag marks docs-only changes — the exact place a client name leaks into prose (cf. #5851) — a docs-only PR bypasses the confidentiality gate entirely. What does precheck's skip cover, and is exempting this gate from it intended?
There was a problem hiding this comment.
precheck.outputs.skip is the already-green-tree reuse, not a docs-only flag: it is true only when refs/ci-green/<tree>/… holds a fresh marker for this exact tree, and that marker is written by a run in which Consolidate test results was green, which needs this gate. So a skip means these bytes already passed the name check. Docs-only changes are the other output, tests-mode=none, and this job's if: does not read it: a docs-only pull request runs the gate. Exempting on skip is intended and matches every other gate in the file; no change.
There was a problem hiding this comment.
Correction to my reply above: one sentence in it is wrong, and the evidence is this PR's own previous run. I wrote that the green-tree marker is written by a run in which Consolidate test results was green. It is not: the Record green tree step sits BEFORE every Fail if the … gate failed step and its if: reads only build and test. In run 36480427008 (job 109131799247) step 13 Record green tree succeeded and step 19 Fail if the client-names gate failed then failed. So a tree whose name check was red can hold a marker for 24 h, and a later run on the same tree takes skip == 'true' and exempts this gate. That predates this PR and holds for every gate consolidated here (doc, licence, binary compatibility and the rest), not for docs-only changes specifically, which do run the gate as said. It is NOT fixed in 41eab72. The fix is to record the marker only after the last gate verdict step (or to add the gate results to its if:); I am handing that to the maintainer-side drain as an open item on this PR rather than pushing a second time onto the run that just started.
There was a problem hiding this comment.
Follow-up on the open item from my correction: it is closed on main now, so this PR no longer has to carry it. #5988 moved Record green tree to the end of collect-results, after every gate verdict step, Fail if the client-names gate failed included. check-green-marker-order.py holds that shape. I ran it on this branch after merging main (32bdf98386): it reports every marker step follows every gate verdict (2 marker(s) checked) and its self-test passes. A tree whose name check was red can no longer hold a green marker.
| """Folds the answers: ('NotChecked', []) wins over ('Fail', hits) over ('Pass', []). Hits are | ||
| re-numbered across chunks so one client keeps one number in this run. Pure but for ``say``.""" | ||
| for a in answers: | ||
| if status_of(a) == "NotChecked" or status_of(a) not in ("Pass", "Fail"): |
There was a problem hiding this comment.
question — The NotChecked path embeds the remote answer's reason verbatim (a.get('reason') or 'no reason given') into the ::error/::warning message and the step summary — both public on this public repository. The never-print-a-name discipline lives on the CRM-owning instance (the Plugins#2507 watcher, outside this diff); I cannot verify from this diff that a NotChecked reason can never carry a matched term or a line's text. Confirm the watcher's reason contract, or mask/shorten the reason here.
There was a problem hiding this comment.
Checked the watcher (MeshWeaver.Plugins, Governance/NameCheck/Source/NameCheckWatcher.cs on main): it writes three reasons, all fixed text plus a line count or an exception TYPE name, never a message, and it clears the submitted lines when it answers. That contract lives in another repository though, so this side no longer depends on it: since 41eab72 the reason is not printed at all. The log and the summary say which status came back for how many requests and that the reason is on the request node. A self-test case asserts a reason string appears in neither.
| nid = node_id(repo, sha, run, i) | ||
| node = {"id": nid, "namespace": NAMESPACE, "name": f"{repo}@{sha[:12]} ({i + 1}/{len(parts)})", | ||
| "nodeType": NODE_TYPE, | ||
| "content": {"$type": CONTENT_TYPE, "repo": repo, "sha": sha, "lines": part}} |
There was a problem hiding this comment.
question — submit_and_wait creates one Governance/NameCheck node per chunk per run (id keyed by repo/sha/run-attempt) and the service user's only grant is create + read — no delete. Nothing in this diff ever removes these nodes, so every PR push and every merge permanently accumulates nodes carrying the added lines' text on the CRM-owning instance — fleet-wide once other repositories adopt the reusable workflow. Is there a retention/TTL on the instance side? If not, this grows unbounded.
There was a problem hiding this comment.
Half of this holds. The text does not accumulate: the watcher empties lines when it answers, and the type has keepsHistory: false, so an answered node holds the verdict (status, masked hits, counts) and no submitted text. The nodes themselves do accumulate, one per chunk per run attempt: I found no retention for Governance/NameChecks in the Plugins source. That is not fixable from this repository (the service user has create + read only, by design) and it does not affect the verdict, so it is not changed here; it is owed on the instance side as a pruning rule in the Governance module.
| hits: list[dict] = [] | ||
| renumber: dict[tuple[int, str], str] = {} | ||
| for i, a in enumerate(answers): | ||
| for h in a.get("hits") or []: |
There was a problem hiding this comment.
nit — The renumber map keys on (chunk, term), so the same client gets a different client#n per chunk, while report()'s docstring says 'one client keeps one number in this run' — and the inline comment states the opposite of the docstring. Align the docstring or key by term alone.
There was a problem hiding this comment.
Fixed in 41eab72 by correcting the docstring, not the key. Keying by term alone would be wrong: each chunk's answer numbers its clients independently, so client#1 from two chunks need not be the same client. The docstring now says that, and that one client seen in two chunks gets two numbers. A self-test case pins it.
| for h in hits: | ||
| f, line, col = h.get("file", ""), int(h.get("line") or 0), int(h.get("column") or 1) | ||
| what = f"{h['term']} ({h.get('kind', 'name')})" | ||
| if line <= 0: |
There was a problem hiding this comment.
nit — esc() guards the file property, but what — the remote-provided kind — goes into the ::error message unescaped, and summary() tables remote-provided fields into GITHUB_STEP_SUMMARY markdown. A kind containing a newline could forge workflow commands in the log. The source is our own instance, but escape consistently.
There was a problem hiding this comment.
Fixed in 41eab72. kind is cut to [A-Za-z0-9 _-], 40 characters, before it reaches a message or the table; the message goes through a data escape (%, CR, LF); and every summary cell from a remote field has newlines, pipes, backticks and angle brackets replaced. A self-test case feeds a kind containing a newline and ::error:: and asserts neither a second command nor a broken cell comes out.
…hecked chunk never hides a hit Review findings on the gate itself: - The checker that runs with NAME_CHECK_URL / NAME_CHECK_TOKEN is taken from a REVIEWED ref (the pull request's base, the queue's base, the pushed commit on main), no longer from the change under review. The change's own copy is still proved by its secretless --self-test. One fallback, for the platform repository only: a reviewed ref that carries no checker yet (the change that introduces it) runs its own. - report() annotates the hits of the chunks that answered BEFORE refusing on a chunk that did not, and the refusal carries them: a hit fails under --unchecked warn even when another chunk was not checked. - parse_diff recognises file headers only between hunks (it follows the hunk header's counts), so an added line whose text starts "++ " or "-- " is content on the right file and line. - The instance's free-text reason is no longer printed to the (public) log or the step summary; it stays on the request node. - The remote-provided kind and the summary's table cells are sanitised. - The renumbering docstring says what the code does. Self-test: 27 of 27. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
🚰 PR babysitter (build instance) is merging the base into this branch on head Why: inherited from its base 'main': 3 pull requests of Systemorph/MeshWeaver fail identically — 'Consolidate test results' concluded failure: Process completed with exit code 1. — the base 'main' moved from 86c840b (what the red run tested) to 0ee43f0. This pull request was red because its BASE was; the base has moved since, and a re-run would test the old merge commit again. Validated: update-branch proposal, and the evidence is the base's, not this diff's: three open MeshWeaver pull requests (#5852, #5948, #5967) fail identically with the fingerprint "main :: 'Consolidate test results' concluded failure: process completed with exit code 1." — an inherited base red, class base-red, while the sibling rows carry the same fingerprint untouched by their own diffs. The base 'main' moved from 86c840b (what red run 36989145016 tested) to 0ee43f0, so merging the base into this branch is what drains it. This decision merges the base into the branch only; it merges no pull r… It does not merge the pull request, push anything else or dequeue. A red after this is left for the owner (rbuergi). |
|
🚰 PR babysitter (build instance) is merging the base into this branch on head Why: stale: 'Consolidate test results' red on a head that tested 'main' at 199912a — green on the base's newest run, so a gate the base has fixed since, not the diff ('Consolidate test results' concluded failure: Process completed with exit code 1.) — the base 'main' moved from 199912a (what the red run tested) to 1d1e202. This pull request was red because its BASE was; the base has moved since, and a re-run would test the old merge commit again. Validated: Run 37201225897 is red on 'Consolidate test results' only because its head tested main at 199912a, where main itself was red on that check; main has since moved to 1d1e202 and its newest run is green on it (the same stale-base fingerprint was approved and executed today on #61285c9a928). The red is the base's at the time, not this diff's - merging the current base in gives a new head whose run re-tests against the fixed base. It does not merge the pull request, push anything else or dequeue. A red after this is left for the owner (rbuergi). |
systemorph-com
Bot
added
the
thread:pr-systemorph-meshweaver-5852-e59808
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Adds a CRM-driven confidentiality gate for this public repository: a reusable workflow (.github/workflows/client-names.yml, job 'No client names') and a build-side checker (.github/scripts/check-client-names.py) that collects a change's added lines and new paths, submits them as Governance/NameCheck nodes to the CRM-owning instance over MCP as the build's own service user, reads back Pass/Fail/NotChecked with masked client#n positions, and emits sanitized workflow-command annotations and a summary; dotnet-test.yml calls the reusable job with fork/Dependabot exemptions, adds it to the needs list of the if: always() consolidation job, and adds an explicit fail step for it there; AGENTS.md gains a 'Confidential terms' section describing the policy and the gate — readable as its own file entry on this item (it was not at the previous head) and consistent with the workflows; no finding there. Checked in full: both workflow files and AGENTS.md. Checked as far as visible: the checker's diff collection (parse_diff, collect_diff, collect_tree, chunks), the MCP-over-HTTP client with its retry and fail-closed handling, submit_and_wait, and the report/summary code. NOT VERIFIED: the checker's patch is truncated in this item (first 20,000 of 35,314 characters kept) — main()'s body below the --path argument, the --self-test arm, the missing-secret fail-closed path, the --report-only/--unchecked handling, the exit codes and the run-identifier default used in node ids are unread and nothing is asserted about them; the body's '17/17 locally' self-test claim is likewise unconfirmed, as are the CRM-side matcher and verdicts (MeshWeaver.Plugins#2507, Crm#157 — outside this diff) and that check-confidential-terms.py and the CONFIDENTIAL_TERMS secret 'are gone' (this diff deletes no file). The checker's patch is the same size as the previous head's (35,314 characters, 644 additions) and the flagged code is unchanged in every respect; the findings repeat the previous head's posted review, re-verified against this diff. Findings: 3 should-fix (a pure rename's new path escapes the diff-mode path check; a NotChecked raised out of submit_and_wait drops the hits of chunks that already answered; the event-payload base sha can include base-branch drift, attributing foreign lines to this change) and 2 questions (the paths loop's exit status under set -e with the default empty paths input; the script-ref and OWN-fallback trust boundary around which checker code runs with the secrets). None blocking, so the event is COMMENT.
Findings: 0 blocking · 3 should-fix · 2 question · 0 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
should-fix .github/scripts/check-client-names.py
parse_diff builds the per-path line-0 entry only from the `+++ ` file header (the branch guarded by `if path is not None and path != old:`), while its docstring promises one line-0 entry per `added or renamed path`. collect_diff diffs with --find-renames, and a 100%-similarity rename emits no ---/+++ headers and no hunk: git prints only a similarity index, `rename from <old>` and `rename to <new>`, none of which parse_diff handles. A file renamed to a path that names a client is therefore never submitted in --base mode and the gate passes it. Renames that also change content are covered (their `+++ b/<new>` header appears and path != old adds the entry), and whole-tree mode still sees the renamed path via git ls-files; the miss is exactly the pure rename in diff mode. Collecting the `rename to <path>` lines as line-0 entries, or diffing with --no-renames so a pure rename surfaces as delete plus add, closes the gap.
should-fix .github/scripts/check-client-names.py
submit_and_wait raises NotChecked with no hits both when the answer deadline passes (`no answer within ... — is the name-check watcher installed on the CRM-owning instance?`) and when a transport NotChecked propagates out of mesh.call('get', ...) inside the poll loop — yet at that point the local answers dict can already hold chunks the instance answered Fail, and those contents are local to submit_and_wait and are lost on the raise, NotChecked.hits left empty. That contradicts NotChecked's own documented contract (`hits are the masked hits of the chunks that DID answer: a known client name is reported, and fails, whatever else went unchecked`): a name the CRM actually found appears nowhere in the build log, only the generic not-checked message, though the Fail verdicts remain on the request nodes on the CRM-owning instance. Fail-closed is preserved — the job still goes red — so this is a reporting loss, not an enforcement loss. report() shows the intended pattern, passing the collected hits into the NotChecked it raises; the two raises inside submit_and_wait do not.
should-fix .github/scripts/check-client-names.py
collect_diff diffs BASE..HEAD directly, and the workflow feeds it BASE_SHA = github.event.pull_request.base.sha — the base tip as recorded in the event payload — while the checkout (fetch-depth: 0 of refs/pull/N/merge) is the head merged into the base as it stands at run time. When the base branch advances between the event and the checkout — and on any re-run of a pull_request, where the payload keeps the original base.sha while the merge ref is fetched afresh, reflecting the base as it stands then — the diff includes the base-side commits' added lines, which are submitted and annotated (file, line) as if this change added them: a hit on a foreign line red-fails an innocent PR, and even a clean foreign line inflates the checked-line count. The in-code claim that on a PR's merge-commit checkout the direct diff `is exactly the change` holds only while BASE_SHA equals the checkout's merge parent. With fetch-depth: 0 the merge base is available: diffing $(git merge-base BASE_SHA HEAD) HEAD is exact for push, pull_request and merge_group alike, and the existing cat-file guard still covers a missing or all-zero base.
Internal review of 60598ed03d3b569f2ef5c5bd74684b512fbfe709 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| set -euo pipefail | ||
| args=(--unchecked "${UNCHECKED:-fail}") | ||
| [ "$REPORT_ONLY" = "true" ] && args+=(--report-only) | ||
| while IFS= read -r p; do [ -n "$p" ] && args+=(--path "$p"); done <<< "${PATHS:-}" |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The paths loop `while IFS= read -r p; do [ -n "$p" ] && args+=(--path "$p"); done <<< "${PATHS:-}"`. With the default empty paths input — which is what dotnet-test.yml's own call passes, since it sets no paths — the here-string yields exactly one empty line; `[ -n "$p" ]` fails and short-circuits, the loop's last body command exits 1, and the while loop completes with that status (the exit status of a while loop is that of the last command executed in its body). The step runs under set -euo pipefail; whether errexit then acts on the loop's nonzero completion cannot be determined from the diff alone — the failing command is exempt inside the && list, but bash's treatment of that propagated status at a compound-command boundary is exactly the classic set -e trap. The decisive reproduction is: bash -ec 'while IFS= read -r p; do [ -n "$p" ] && echo x; done <<< ""; echo survived'. If 'survived' does not print, the step exits before `python3 "$CHECKER"` ever runs, the gate is red for every caller with the default empty paths, and it cannot go green even once the CRM side is ready — indistinguishable in the log from the designed missing-secret red. An `if [ -n "$p" ]; then args+=(--path "$p"); fi` body makes the loop's status 0 in every case.
There was a problem hiding this comment.
The loop does not abort the step. A command that fails inside an && list (other than the last one) is exempt from set -e, and so is the nonzero status that list leaves as the while loop's status. I ran the step's exact lines (set -euo pipefail, empty PATHS, REPORT_ONLY=false, the same while … done <<< "${PATHS:-}" and the [ … ] && args+=(…) line). Both shells print survived and exit 0:
- bash 3.2.57:
survived … args=0, exit 0 - bash 5.3.15:
survived … args=0, exit 0
So with the default empty paths, the step reaches python3 "$CHECKER". You were right that this depended on a subtle rule, though. In 7db213b both lines are now plain if … then …; fi, so their status no longer depends on set -e list semantics.
| if [ -f "$PINNED" ]; then | ||
| echo "path=$PINNED" >> "$GITHUB_OUTPUT" | ||
| echo "Checker: Systemorph/MeshWeaver@${REF}" | ||
| elif [ "$GITHUB_REPOSITORY" = "Systemorph/MeshWeaver" ] && [ -f "$OWN" ]; then |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
Two trust-boundary points about which checker code runs with NAME_CHECK_URL and NAME_CHECK_TOKEN in its environment. (1) script-ref is caller-supplied and REVIEWED only by the input's description: a repository that calls this reusable workflow can pass any ref resolvable in Systemorph/MeshWeaver at checkout time — including refs/pull/N/head of an unmerged, even fork-origin, pull request whose .github/scripts/check-client-names.py nobody has reviewed — and that code then runs with the service token. (2) While MeshWeaver's base carries no checker at all (exactly this PR's window), the OWN-copy fallback at line 78 runs the pull request's own copy with the secrets — and not only for the change that introduces the checker: any pull request against that base whose own branch adds a file at .github/scripts/check-client-names.py gets the same fallback, at CI time, before any review. Both are bounded by who may call the workflow and by the token's narrow create+read grant on Governance/NameChecks, which are outside this diff; the comment at the 'Choose the reviewed checker' step shows the boundary is deliberate. Whether a mechanical guard on script-ref (refusing refs/, or limiting it to branches and tags) or a tighter condition on the fallback is planned is not visible in this diff.
There was a problem hiding this comment.
Both points were fair. Fixed in 7db213b: "reviewed" is now checked mechanically rather than taken from the input's description.
script-ref: the platform repository is checked out with full history (fetch-depth: 0,filter: blob:none). The step then requiresgit merge-base --is-ancestor <script-ref commit> origin/main. A ref that is not onmain(arefs/pull/N/head, a feature branch, a stacked PR's base) is not honoured:main's checker runs instead, with a notice. Only merged code ever runs withNAME_CHECK_URL/NAME_CHECK_TOKEN.- Own-copy fallback: now taken only while
mainitself carries no checker (! git cat-file -e origin/main:.github/scripts/check-client-names.py), i.e. the window before this change lands. Once it is onmainthe branch is unreachable. A later PR that adds its owncheck-client-names.pygetsmain's checker, not its own. Inside the window it grants nothing new: secrets are never given to a fork's PR, and a branch pushed to this repository can already edit the workflows that hold the secrets.
Verification: I extracted the step and ran it against throwaway origin/clone repositories in 7 cases (script-ref on/off main × main with/without a checker × platform/other repository). The new step never runs the unreviewed copy. As a negative control, the previous step ran the unreviewed copy in 4 of those 7 cases. The workflow guards pass (check-workflow-timeouts, check-pr-secret-preflight, check-workflow-shell, check-workflow-yaml-keys, 0 violations each) and actionlint is clean.
Not verified: the new checkout settings have not run on a real GitHub runner yet; this PR's own CI run on 7db213b will be the first.
…the input's word Review of #5852: script-ref was trusted by its description, so a caller could point it at refs/pull/N/head and run an unreviewed checker with the CRM token. The checker ref is now honoured only when its commit is an ancestor of the platform repository's main; otherwise main's checker runs, with a notice. The own-copy fallback applies only while main itself carries no checker (the introducing change's window). The two && guards in the check step become plain if-statements so their status never depends on set -e list semantics. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Adds a CRM-driven confidentiality gate for this public repository: a reusable workflow (.github/workflows/client-names.yml, job 'No client names') and a build-side checker (.github/scripts/check-client-names.py) that collect a change's added lines and new paths, submit them as Governance/NameCheck nodes to the CRM-owning instance over MCP as the build's own service user, read back Pass/Fail/NotChecked with masked client#n positions, and emit sanitized workflow-command annotations and a summary; dotnet-test.yml calls the reusable job with fork/Dependabot exemptions, adds it to the needs list of the if: always() consolidation job, and adds an explicit fail step for it there; AGENTS.md gains a 'Confidential terms' section describing the policy and the gate. Checked in full: both workflow files and AGENTS.md. Checked as far as visible: the checker's diff collection (parse_diff, collect_diff, collect_tree, chunks), the MCP-over-HTTP client with its retry and fail-closed handling, submit_and_wait, and the report/summary code, which sanitizes every remote-provided value before it reaches a workflow command or the markdown table. NOT VERIFIED: the checker's patch is truncated in this item (first 20,000 of 35,314 characters kept) — main()'s body below the --path argument, the --self-test arm, the missing-secret fail-closed path, the --report-only/--unchecked handling, the exit codes and the run identifier used in node ids are unread and nothing is asserted about them; the body's '17/17 locally' self-test claim is unconfirmed, as are the CRM-side matcher and verdicts (MeshWeaver.Plugins#2507, Crm#157 — outside this diff) and that check-confidential-terms.py and the CONFIDENTIAL_TERMS secret 'are gone' (this diff deletes no file). Versus the previous head's posted review the two questions are answered at this head: the paths loop body is now an if statement, which returns status 0 in every case, so the set -e doubt is gone; and the Choose-the-reviewed-checker step now mechanically pins the checker to main (merge-base --is-ancestor against origin/main, any other ref checked out from main instead with a notice) and conditions the own-copy fallback on main carrying no checker at that path — so neither question is repeated. The three should-fix findings remain, re-verified against this diff: the checker script is the same size (644 additions, patch cut at the same point) and the flagged code is identical in the readable portion. None blocking, so the event is COMMENT.
Findings: 0 blocking · 3 should-fix · 0 question · 0 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
should-fix .github/scripts/check-client-names.py
parse_diff builds the per-path line-0 entry only from the `+++ ` file header (the branch guarded by `if path is not None and path != old:`), while its docstring promises one line-0 entry per added or renamed path. collect_diff diffs with --find-renames, and a 100%-similarity rename emits no ---/+++ headers and no hunk: git prints only a similarity index, `rename from <old>` and `rename to <new>`, none of which parse_diff handles. A file renamed to a path that names a client is therefore never submitted in --base mode and the gate passes it. Renames that also change content are covered (their `+++ b/<new>` header appears and path != old adds the entry), and whole-tree mode still sees the renamed path via git ls-files; the miss is exactly the pure rename in diff mode. Collecting the `rename to <path>` lines as line-0 entries, or diffing with --no-renames so a pure rename surfaces as delete plus add, closes the gap.
should-fix .github/scripts/check-client-names.py
submit_and_wait raises NotChecked with no hits both when the answer deadline passes (`no answer within ... — is the name-check watcher installed on the CRM-owning instance?`) and when a transport NotChecked propagates out of mesh.call('get', ...) inside the poll loop — yet at that point the local answers dict can already hold chunks the instance answered Fail, and those contents are local to submit_and_wait and are lost on the raise, NotChecked.hits left empty. That contradicts NotChecked's own documented contract (`hits are the masked hits of the chunks that DID answer: a known client name is reported, and fails, whatever else went unchecked`): a name the CRM actually found appears nowhere in the build log, only the generic not-checked message, though the Fail verdicts remain on the request nodes on the CRM-owning instance. Fail-closed is preserved — the job still goes red — so this is a reporting loss, not an enforcement loss. report() shows the intended pattern, passing the collected hits into the NotChecked it raises; the two raises inside submit_and_wait do not.
should-fix .github/scripts/check-client-names.py
collect_diff diffs BASE..HEAD directly, and the workflow feeds it BASE_SHA = github.event.pull_request.base.sha — the base tip as recorded in the event payload — while the checkout (fetch-depth: 0 of refs/pull/N/merge) is the head merged into the base as it stands at run time. When the base branch advances between the event and the checkout — and on any re-run of a pull_request, where the payload keeps the original base.sha while the merge ref is fetched afresh — the diff includes the base-side commits' added lines, which are submitted and annotated (file, line) as if this change added them: a hit on a foreign line red-fails an innocent PR, and even a clean foreign line inflates the checked-line count. The in-code claim that on a PR's merge-commit checkout the direct diff `is exactly the change` holds only while BASE_SHA equals the checkout's merge parent. With fetch-depth: 0 the merge base is available: diffing $(git merge-base BASE_SHA HEAD) HEAD is exact for push, pull_request and merge_group alike, and the existing cat-file guard still covers a missing or all-zero base.
Internal review of 7db213b77c46c4e4fd9797ba02eac3b15a3f17a1 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
|
🚰 PR babysitter (build instance) is merging the base into this branch on head Why: stale: 'Consolidate test results', 'Platform compatibility: deployed plugin set links against this platform (ladder)' red on a head that tested 'main' at 1d1e202 — green on the base's newest run, so a gate the base has fixed since, not the diff ('Consolidate test results' concluded failure: Process completed with exit code 1.) — the base 'main' moved from 1d1e202 (what the red run tested) to d1414f8. This pull request was red because its BASE was; the base has moved since, and a re-run would test the old merge commit again. Validated: Run 37390776878 failed 'Consolidate test results' and 'Platform compatibility: deployed plugin set links against this platform (ladder)' against main at 1d1e202, which this pass's own reading names red on those gates; main has since moved to d1414f8 and its newest run of both is green — a stale base, not this diff. Approving the update-branch so the head merges the fixed base and its gates re-run against it. It does not merge the pull request, push anything else or dequeue. A red after this is left for the owner (rbuergi). |
The PR's own run failed "NOT CHECKED: JSON-RPC error -32602" — which does not say whether initialize, the create or the read-back was refused, so the instance side cannot be diagnosed from the build log. The refusal now reads e.g. "tools/call create: JSON-RPC error -32602"; the endpoint's free-text message is still never printed. Self-test added (fails without the change). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…s it The run on de66ca8 was red with 'the build's service user may lack create on Governance/NameChecks' and nothing about who fixes it. A refused create (a non-Created answer, a refused tool call or a JSON-RPC error on the create) now names the cure: a global admin proposes Governance/Activity with standard namecheck.caller.grant and inputs.service = the token's svc-... identity, another global admin signs, and it writes Governance/NameChecks/_Access/{svc}_Access with role NameCheckCaller. Still red (fail closed), never a skip; an unreachable endpoint or a 401 keeps its own message. Self-test: two new arms (refused create fails and names the standard; --report-only still prints it, as a warning). 32/32. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
namecheck.caller.grant now sets authority.maintainer (Systemorph/MeshWeaver.Plugins#3089), so the maintainer can sign their own proposal and one global admin can make the grant alone. The refused-create message no longer sends the reader looking for a second admin. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Heads-up from Systemorph/MeshWeaver.Plugins#3147, which fixes MeshWeaver.Plugins#2785. The name-check watcher now answers only for text it can prove is committed. A request must carry:
Every submitted line must also be in that commit. A request that falls short gets Before this lands, |
There was a problem hiding this comment.
🟡 Changes recommended
Coverage gaps, verdict handling, and checker isolation can let unchecked or rejected changes pass.
7 open findings
What changed in this PR
Adds a CRM-backed confidentiality gate to protect the public repository without storing client-name lists in CI.
Changes:
- Collects changed text and paths for remote checks with masked results.
- Adds a reusable workflow and required-result enforcement.
- Documents confidentiality rules and provisioning requirements.
| File | Description |
|---|---|
AGENTS.md |
Documents confidentiality rules and gate behavior. |
.github/workflows/dotnet-test.yml |
Integrates the gate into consolidated results. |
.github/workflows/client-names.yml |
Adds reusable checker execution and secret handling. |
.github/scripts/check-client-names.py |
Implements collection, remote checks, reporting, and self-tests. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| new_left -= 1 | ||
| text = raw[1:] | ||
| if path is not None and text.strip(): | ||
| out.append({"file": path, "line": line, "text": text[:MAX_TEXT]}) |
There was a problem hiding this comment.
Real defect — fixed in f3e86c3. Both collectors now send every line WHOLE through one entry() helper; requests are bounded by a text budget (CHUNK_CHARS) as well as a line count, so a long line goes in a request of its own instead of being cut. A line over MAX_LINE_CHARS (1,000,000) raises NotChecked naming file:line (never its text), so it can never count as checked. Self-test arms: a name at column ~4,500 reaches the matcher; an over-limit line is NotChecked. Negative control: the previous code returned a 2,000-char prefix for the same input.
| diff = git(root, "diff", "--no-color", "--no-ext-diff", "-U0", "--find-renames", base, "HEAD", | ||
| "--", *(paths or [])) | ||
| return parse_diff(diff) |
There was a problem hiding this comment.
Real defect — fixed in f3e86c3. collect_diff now also reads git diff --name-only -z --find-renames --diff-filter=ACR BASE HEAD, independently of the textual hunks, and adds a path entry for every new/renamed/copied destination not already seen. New self-test arms: a pure rename, a new empty file and a new binary file are each checked by path, and a change consisting only of those three fails on all three.
| "Replace it with a neutral placeholder (AGENTS.md, 'Confidential terms').") | ||
| if any(status_of(a) not in ("Pass", "Fail") for a in answers): | ||
| raise NotChecked(unchecked_message(answers), hits) | ||
| return ("Fail" if hits else "Pass"), hits |
There was a problem hiding this comment.
Real defect — fixed in f3e86c3. report() now folds the STATUS: any Fail chunk makes the verdict Fail whether or not it carries hits; a hit-less Fail gets its own ::error:: (positions are on the request node), and a hit-less Fail next to a NotChecked chunk is carried on the refusal as failed=True, which main() now enforces even under --unchecked warn. Arms: report([{status: Fail, hits: []}]) and report([{status: 3}]) return Fail; negative control: the old code returned Pass.
| - name: Prove this change's own checker too (no secret in reach) | ||
| if: ${{ hashFiles('.github/scripts/check-client-names.py') != '' }} | ||
| run: python3 .github/scripts/check-client-names.py --self-test |
There was a problem hiding this comment.
Real defect — fixed in f3e86c3. The change's own --self-test no longer runs in the secret-bearing job. It is a separate job, own-checker-self-test, on ubuntu-latest (a fresh GitHub-hosted VM per job, deliberately not MW_RUNNER), with a sparse checkout of just the script, persist-credentials: false, and no secret. The client-names job now runs only reviewed code (the checker from a commit on main) before it receives NAME_CHECK_TOKEN; a failing self-test still fails the reusable workflow, hence the gate.
| # timeout, NotChecked), and it is a NEEDS of `Consolidate test results` with an explicit fail | ||
| # step there, like every other gate. The fork / Dependabot exemption is the EVENT, nowhere else. | ||
| needs: [precheck] | ||
| if: ${{ !cancelled() && needs.precheck.outputs.skip != 'true' && (github.event_name != 'pull_request' || (github.event.pull_request.head.repo.fork != true && github.actor != 'dependabot[bot]')) }} |
There was a problem hiding this comment.
Real defect — fixed in f3e86c3. The client-names job no longer carries needs.precheck.outputs.skip != 'true', and the consolidate step Fail if the client-names gate failed no longer has the reuse exemption. The only exemptions left are the event ones (fork / Dependabot PRs). Reason, in the comment there: a green marker vouches for a TREE, while this verdict depends on the diff's base and on the CRM's current catalogue. check-green-marker-order.py --root . still passes (both markers follow every verdict).
| if not p.is_file() or p.stat().st_size > MAX_FILE_BYTES: | ||
| continue |
There was a problem hiding this comment.
Real defect — fixed in f3e86c3. collect_tree no longer has a size cap: it streams each tracked file line by line, skipping only files with a NUL in the first 8 KiB (binary: their path is still checked). An unreadable file is now NotChecked instead of a silent continue. Arm: a ~2.4 MB text file whose name-bearing line is its last (line 200,001) is collected whole.
| while len(answers) < len(ids): | ||
| for nid in ids: | ||
| if nid in answers: | ||
| continue | ||
| text = mesh.call("get", {"path": f"@{NAMESPACE}/{nid}"}).strip() | ||
| try: | ||
| content = (json.loads(text) or {}).get("content") or {} | ||
| except json.JSONDecodeError: | ||
| content = {} | ||
| if status_of(content) not in ("Requested", ""): | ||
| answers[nid] = content | ||
| if len(answers) < len(ids): | ||
| if time.monotonic() > deadline: | ||
| raise NotChecked(f"no answer within {ANSWER_TIMEOUT_S:g}s — is the name-check watcher installed " | ||
| "on the CRM-owning instance?") | ||
| time.sleep(POLL_S) |
There was a problem hiding this comment.
Real defect — fixed in f3e86c3. The poll loop in submit_and_wait now catches the NotChecked from a read failure or the timeout, runs report() over the chunks that DID answer (annotating their hits first), and re-raises with those hits and the rejection flag carried. Arm: with one request per line, the first chunk answers Fail and the second chunk's read errors (JSON-RPC error -32603); the run exits 1 under --unchecked warn with file=b.md,line=3 annotated, and the error's free text is not printed.
…ath, Fail is Fail, isolated self-test, no reuse skip - Lines are sent whole (chunks bounded by text size too); a line over 1M chars is NotChecked, never cut. - New paths from `--name-only -z --diff-filter=ACR`: pure renames, empty and binary files are checked. - A `Fail` status fails even with no hit details, and is carried on a NotChecked refusal. - Whole-tree reads stream files of any size; an unreadable file is NotChecked. - A read failure / timeout after some chunks answered annotates and carries their hits. - The change's own checker self-tests in a separate secretless job on a fresh hosted runner. - The gate no longer skips on green-tree reuse (its verdict depends on the diff and the CRM). Self-test: 39/39, with new arms for each defect (negative control: old code passes Fail-empty, cuts at 2000). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
# Conflicts: # .github/workflows/dotnet-test.yml
…oves (Plugins#3147) MeshWeaver.Plugins#3147 (#2785) makes the CRM-owning instance answer a name check only after its GitHub App proves the request: a live run of the repository (runId), on the named 40-character commit or the PR head it merges, with every submitted line in that commit. A request without runId is answered NotChecked. - every request now carries runId (GITHUB_RUN_ID) beside the full sha; - no GITHUB_RUN_ID, or a HEAD that is not a 40-character id, is refused locally as not checked, before anything is sent; - the fake instance in --self-test refuses an unproven request exactly as the real one does; negative control: dropping runId reds 7 cases; - own-checker-self-test runs on the MW_RUNNER pool (ephemeral, one job per pod), the fleet runner rule that reddened CI's own shell. Also merges main (dotnet-test.yml needs: keeps client-names beside the new dependent-suites needs). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Closed at the maintainer's request: the CRM-driven client-name check never completed its governance grant (four filings hit successive governance defects, since fixed in MeshWeaver.Plugins #3157, #3186, #3204). The existing confidential-terms check still screens public-bound files. Reopen this PR together with MeshWeaver#5852, Memex#610, MeshWeaver.Crm#158 and MeshWeaver.Education#378 to resume; the grant proposal is |
Adds a CI gate so a client name, a client person or a client identifier cannot land in this public repository. Stacked on #5851, the docs neutralization, which is its first commit.
What it does (reworked 2026-09-28: CRM-driven, no denylist)
The CRM decides who is a client. Builds never read the CRM, and this repo holds no term list, in a file or in a secret.
.github/workflows/client-names.ymlis a reusable job, "No client names", that every repo in the fleet can call..github/scripts/check-client-names.pycollects the change's added lines and new or renamed paths, each with file:line. Push and merge-group runs diff against the base; a new branch checks the whole tree.Governance/NameCheckrequests to the CRM-owning instance over its MCP endpoint, authenticated as the build's own service user.Pass,FailorNotChecked. A hit isclient#natfile:line:columnplus the kind that matched. The log never prints a name or a line.NAME_CHECK_URL,NAME_CHECK_TOKEN)NotCheckedanswer--self-testproves every arm against a fake instance (17/17 locally).Consolidate test results, with an explicit fail step. Fork and Dependabot PRs are exempt by event.check-confidential-terms.pyand theCONFIDENTIAL_TERMSsecret are gone, and the AGENTS.md paragraph now describes the new gate.The matcher, aliases, deployment identifiers and verdicts run on the CRM-owning instance: Systemorph/MeshWeaver.Plugins#2507, which reads the CRM fields from Systemorph/MeshWeaver.Crm#157.
Status (2026-10-07)
Governance/NameChecks, the roleGovernance/NameCheckCaller(Read, Create, Api) and the standardGovernance/Standards/namecheck.caller.grantall exist. The service identityAdmin/_ServiceIdentity/svc-name-checkand its token exist, and the org secretsNAME_CHECK_URL/NAME_CHECK_TOKENreach the job: the run onde66ca8828got past the handshake.Governance/NameChecks/_Accessholds only the two gated Public/Anonymous denials. Nobody hasaccessObject: svc-name-checkanywhere, and no activity fornamecheck.caller.granthas been proposed. So the create is refused andNo client namesis red, which makesConsolidate test resultsred.32bdf98386, the red names its cure in the log: "the check request was not created — … propose a Governance/Activity … standard 'namecheck.caller.grant' and inputs.service = the token's service identity (svc-…) …". It stays a failure, never a skip. Self-test arms pin both the message and the exit code.Why this stays on the control instance (requested retarget to the public instance: not possible today)
This was measured on 2026-10-07 on the public instance (MCP
memex). Each of these reasons blocks the move by itself:nodeType:Crm/Counterparty partitions:allandnodeType:Crm/Contact partitions:allreturn 0 over 145 readable partitions. The watcher fails closed toNotChecked, so the job would be red forever.Governancethere has a one-wayGovernance/_GitSyncfrom PluginsGovernance/, so the partition is system-owned. Core'sSystemOwnedGrantRejectionrefuses any write-conferringAccessAssignment, andNameCheckCallerconfersCreate. Repo sync never carries_Access, so a "declared" grant is not possible.Admin/_ServiceIdentity/*is empty there, and nothing declares one from content.Moving the check would mean publishing the client catalogue to the public instance, which is what this gate prevents. The check and spokes are instance-agnostic, since
NAME_CHECK_URLis a secret, so no code change follows from where it points. Full write-up:Governance/ClientNameGate.mdin Plugins#3089.Maintainer action needed (one admin, alone)
The grant is an access-control change, so no agent makes it.
Governance/NameChecksis system-owned (_Accessis never synced from a repository), so the designed route is the governed standard, not a hand-written AccessAssignment.1. Propose (you, as maintainer, on memex.systemorph.com, MCP server
systemorph,create):{ "id": "namecheck-caller-grant-svc-name-check", "namespace": "Governance/Activities", "nodeType": "Governance/Activity", "name": "Grant svc-name-check the NameCheckCaller role on Governance/NameChecks", "content": { "$type": "ActivityContent", "standard": "namecheck.caller.grant", "inputs": { "service": "svc-name-check" }, "trigger": "Governance/ClientNameGate", "rationale": "Core #5852: the build's client-name check is refused at create because svc-name-check holds no grant on Governance/NameChecks." } }2. Answer and sign, alone. First merge Systemorph/MeshWeaver.Plugins#3089 and let the Governance package sync to the control instance. Check that
get @Governance/Standards/namecheck.caller.grantthere showsauthority.maintainer: rbuergi. Then answer the manual gatemanual:is-service:Admin/_ServiceIdentity/svc-name-checkexists, is a ServiceIdentity, and is not revoked (read on 2026-10-07). Sign asrbuergi. As the standard's maintainer, you may sign your own proposal, so no second admin is needed. Every other signer still needs someone else's signature. The executor runs as System and writes exactly:{ "id": "svc-name-check_Access", "namespace": "Governance/NameChecks/_Access", "nodeType": "AccessAssignment", "name": "svc-name-check — NameCheckCaller (governed)", "content": { "$type": "AccessAssignment", "accessObject": "svc-name-check", "displayName": "svc-name-check", "roles": [ { "$type": "RoleAssignment", "role": "NameCheckCaller" } ] } }3. Re-run this PR's
No client namesjob. That is the first real answer on whether the tree still holds a client name.🤖 Generated with Claude Code