docs+fix(audit): seven instruments that reported confidently and were wrong — plus the four silent handlers they led to - #2572
Conversation
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0793b80bdf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 2. **Healthchecks and status targets assert payload, not status code.** | ||
| `pmoves-mesh-preflight` and `health:check-all` currently trust endpoint verdicts; | ||
| either would have passed the 0-byte exporter every day it was broken. |
There was a problem hiding this comment.
Wire exporter validation into the named health flows
pmoves-mesh-preflight cannot have passed the broken exporter because its script only probes ports parsed from .claude/CATALOG.md, where port 9835 is absent, and health:check-all delegates to verify-all, which never invokes rocm-exporter-status. In the recurrence scenario described here, adding payload assertions to those existing flows still will not inspect this exporter; recommend wiring the exporter/Prometheus amd-gpu target into a health flow or naming rocm-exporter-status as the required check.
Useful? React with 👍 / 👎.
| | `%` and `${}` in a systemd `ExecStart` are expanded **by systemd** | `%d` → credentials dir, `%s` → user shell, `${#body}` → empty. Put shell code in a real script; escaping works but reintroduces itself on the next edit. | | ||
| | Submodule branch **name** ≠ branch **membership** | detached HEAD is normal; the real test is `merge-base --is-ancestor <sha> origin/<tracked>`. And the *recorded gitlink* is a different layer from the *working tree* — only the former affects clones. | | ||
| | Fleet `NATS_URL` is container-scoped (`nats:4222`) | any host-run tool hangs on it. Use the published address from the host. | | ||
| | A tool run in a bare `uv run` env has no venv packages | a soft-import fallback then renders a full report from nothing. Declare inline deps (`# /// script`). | |
There was a problem hiding this comment.
Correct the bare
uv run environment claim
A bare uv run does not inherently omit virtual-environment packages: uv help run states that inside a project it creates/updates and uses the project environment, and outside a project it uses a virtual environment found in the current or a parent directory. Isolation occurs for a script with inline dependency metadata or when options such as --isolated/--no-project request it, so this node-agnostic rule currently gives operators an incorrect diagnosis and remedy; qualify it to the isolated PEP 723 script case that was actually observed.
Useful? React with 👍 / 👎.
Joins the existing audit lane (#2522 ruleset exposure, #2525 CI enforcement, #2527 reproducibility) rather than opening a new one. Six checks on B850 reported a confident result that did not match reality — a 0-byte exporter behind 200 OK, a launcher that WARNed and exec'd anyway, a submodule audit measuring branch NAME instead of membership, a dmesg evicted by a failing USB keyboard, a health checker that printed 'Health: 0.0%' for a bus it never contacted, and this auditor reading an AttributeError as an ImportError and writing it into a Makefile comment as fact. Two shapes needing different remedies: three are mechanizable (a surface returning success while the payload is absent/stale/malformed — assert content, not status), three are not (a wrong question, unrelated noise destroying evidence, a misread). Deliberately does NOT restate the verification discipline. .claude/agents/verifier.md already specifies it — 'evidence before assertions ... capture verbatim ... state UNVERIFIED (environment) rather than approximating' — and predates this session. The documented gap is INVOCATION: that agent was invoked zero times during a session in which it would have caught finding #6 immediately. Records the mechanical traps, which are the genuinely new material: $() strips trailing newlines (bit four times in one evening), nats-py connect_timeout does not bound DNS, % and ${} in a systemd ExecStart are expanded by systemd, submodule branch name != membership and recorded gitlink != working tree. Notes that #2525's merge-gate finding has already been repaired (pytest_ratchet runs all 264 test files; the gate exit 1s) — verified before relying on it for merges. The audit lane is driving fixes ahead of its own PRs merging, which argues for landing it. Proposes the #2527 package as a calibration fixture: #2525 had to hand-roll a deliberately failing test to prove a check COULD fail; a frozen, hash-manifested, network-isolated package with six deterministic checks of known outcome is the standing form of that — a target whose answer is known, which every instrument in the table lacked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0793b80 to
4cc3a6e
Compare
An AST sweep for finding #7's cause (soft-import + broad except returning a plausible default) hit 157 sites. That number is itself the finding: torch, faiss, sentence_transformers, numpy, tqdm, rich and psutil guards are all CORRECT — the feature degrades and the caller is told. Narrowing to handlers that are silent (no log, no raise, no warn) gave 60; to those in a path that reports outward, 4. The antipattern is not "a broad except on an import". It is a silent handler in a path that reports outward. Fixed (logging only — no behaviour or contract change, best-effort delivery stays best-effort): sign_trail.py:77 substituted the whole agent identity in silence; now warns to stderr naming the reason. It already warned about a missing ALTER twenty lines below — it could report a missing persona but not a missing person. geometry.py:166,583 dropped every live subscriber and returned {"ok": true}. Eight lines up, the persist logs and raises HTTPException(500). Two disciplines, one function. hf-mcp-server:853 hf.model.gguf.converted.v1 never published while the caller was told everything worked. Left alone deliberately, as counterexamples of correct degradation: hf-mcp-server:542 stamps "source":"catalog" vs "registry" chit_security.py:13 sets an explicit _CRYPTO_OK = False common/__init__.py:41 optional exports fail loudly at the call site Verified: pmoves/tests/test_sign_trail.py 2 passed; unregistered agent-id now warns, registered b850-claude still resolves to glyph U+232C / #DC2626 with no warning; detector re-run shows only the two correct sites remaining. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🔏 CHIT-aware change — control-body routingThis PR touches a CHIT-aware service (ports 8086/8087/8103/8106/8113/9224 surface). Before a Control-Body ACK:
Advisory routing only — the blocking contract check is CHIT Contract. |
…n JSON
Running the merged checker on B850 for the first time exposed two gaps in the
NOT MEASURED work itself:
1. The JSON branch still emitted "health_pct": 0.0 when the bus was never
contacted. The human-readable branch had been fixed to refuse an unmeasured
percentage; any dashboard consuming --json kept receiving the exact false
negative. Now null, with an explicit "measured" flag. Fixed the instance,
not the class — the same error this audit documents.
2. The failure report offered a list of GUESSES and no facts. The real cause
was 'Authorization Violation' (the server requires credentials; this tool
deliberately ships no credential-bearing default), but nats-py surfaces that
through error_cb and keeps retrying, so the only exception reaching the
caller was TimeoutError. The report therefore said "timed out" — reading as
a network fault and sending the operator to check host and port, which were
both already correct. An error_cb now captures what the server actually
said, and the report leads with it before any guesses.
Verified:
no creds -> measured=False, health_pct=null,
error="timed out after 5s connecting to nats://localhost:4222
— last server error: Error: nats: 'Authorization Violation'"
connected -> health_pct=4.2, error=null, no NOT MEASURED banner (synthetic
BusHealth; success path formatting unchanged)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…st contact Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ne number was unreproducible
The audit argued that confident reports need their underlying values read, then
published a 157/60/6/4 sweep funnel produced by a script that was thrown away, and
a table of seven findings with almost no citations. Every claim was true and none
of them was checkable by a reader.
Grounding pass:
- Add pmoves/tools/silent_handler_sweep.py (+ `make silent-handler-sweep`), the
durable replacement for the discarded sweep. Stages 1-3 are mechanical; stage 4
("reports outward") is deliberately NOT automated and emits null in --json —
automating that judgement is the error the audit documents.
- The original script was recovered from scratch and its predicate preserved as
`--legacy`, so the historical column stays regenerable. It reconciles: 157 -> 158
stage-1 across two days, and 6 -> 2 pass-only is exactly the four sites fixed in
#2572. The headline was never wrong, only unverifiable.
- Both predicates were wrong about silence, in opposite directions. The original
substring-scanned ast.dump(handler) for "log"/"print"/... — a text grep wearing an
AST costume, inside a sweep whose headline is that text greps cannot see handler
shape. The replacement's first draft counted Return as audible, which excludes
`except Exception: return _FALLBACK` — finding #7 itself — and reported stage 2 as
7 instead of 69. Both pinned as named regression tests.
- Fix a citation that had rotted into pointing at its own opposite: geometry.py:583
-> :587; the stale line landed on the correct-discipline counterexample four lines
above the defect it claimed to cite.
- Correct the merge-gate paragraph. The job literally named `merge-gate`
(merge-gate.yml:16-29) is still vacuous today; `python-tests` (:34) was the repair
and `merge-decision` (:68) is the actual enforcer — which fails only on "failure",
so a cancelled or skipped required job passes.
- Ground recommendation #2 in the mechanism instead of asserting it: preflight.sh:64
is `curl -o /dev/null`, structurally incapable of noticing an empty 200.
- Add per-row "verify against" citations for all seven findings and the traps table.
Rows 4 and 6 have no in-tree artifact and now say so.
Re-derived rather than copied: 263 CI-visible test files against pytest_ratchet.py's
264. That took three attempts (5058, then 4, then 263) — two confidently wrong
measurements inside a grounding pass about confidently wrong measurements, recorded
in the doc because the third number is only trustworthy in the company of the first
two.
22 new tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…beat a populated alias — and the audit that missed it, grounded (#2605) * docs(audit): ground the instrument-trust audit in source — its headline number was unreproducible The audit argued that confident reports need their underlying values read, then published a 157/60/6/4 sweep funnel produced by a script that was thrown away, and a table of seven findings with almost no citations. Every claim was true and none of them was checkable by a reader. Grounding pass: - Add pmoves/tools/silent_handler_sweep.py (+ `make silent-handler-sweep`), the durable replacement for the discarded sweep. Stages 1-3 are mechanical; stage 4 ("reports outward") is deliberately NOT automated and emits null in --json — automating that judgement is the error the audit documents. - The original script was recovered from scratch and its predicate preserved as `--legacy`, so the historical column stays regenerable. It reconciles: 157 -> 158 stage-1 across two days, and 6 -> 2 pass-only is exactly the four sites fixed in #2572. The headline was never wrong, only unverifiable. - Both predicates were wrong about silence, in opposite directions. The original substring-scanned ast.dump(handler) for "log"/"print"/... — a text grep wearing an AST costume, inside a sweep whose headline is that text greps cannot see handler shape. The replacement's first draft counted Return as audible, which excludes `except Exception: return _FALLBACK` — finding #7 itself — and reported stage 2 as 7 instead of 69. Both pinned as named regression tests. - Fix a citation that had rotted into pointing at its own opposite: geometry.py:583 -> :587; the stale line landed on the correct-discipline counterexample four lines above the defect it claimed to cite. - Correct the merge-gate paragraph. The job literally named `merge-gate` (merge-gate.yml:16-29) is still vacuous today; `python-tests` (:34) was the repair and `merge-decision` (:68) is the actual enforcer — which fails only on "failure", so a cancelled or skipped required job passes. - Ground recommendation #2 in the mechanism instead of asserting it: preflight.sh:64 is `curl -o /dev/null`, structurally incapable of noticing an empty 200. - Add per-row "verify against" citations for all seven findings and the traps table. Rows 4 and 6 have no in-tree artifact and now say so. Re-derived rather than copied: 263 CI-visible test files against pytest_ratchet.py's 264. That took three attempts (5058, then 4, then 263) — two confidently wrong measurements inside a grounding pass about confidently wrong measurements, recorded in the doc because the third number is only trustworthy in the company of the first two. 22 new tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(secrets): deliver the host-side CHIT passphrase, and stop telling operators to destroy their bundle Two defects that both fail by silently succeeding. 1. The allowlist gap. Every compose file writes `CHIT_PASSPHRASE=${CHIT_PROD_PASSPHRASE:?...}` — the container-side name is CHIT_PASSPHRASE, the HOST-side name is CHIT_PROD_PASSPHRASE, and only the container-side one was ever delivered or registered. 26 refs across 5 compose files / 12 services need the host name, and because compose interpolates the whole file before running anything, the absence gated every `up-*` target on the node rather than only the services that use it. Meanwhile the funnel reported zero errors. Verified 2026-08-17: no CHIT_PROD_* secret exists in either the repo or the prod environment scope — only CHIT_PASSPHRASE. So rather than asking the operator to mint a duplicate of the same value, this maps the one real secret to the name the runtime reads: - sync-secrets-local.yml: add the host-side name to the env: map (per its own line 100, "absent from this map = never delivered"). - chit_manifest_register.py: register it with the container-side name as a source alias, so bundles predating the workflow change still resolve. secrets_sync.py:120-131 tries label first, then aliases, and emits the canonical target key either way — the same shape as KIMI_CODING_API / MOONSHOT_API_KEY. required=True is deliberate and is not the free choice: SECRETS_SYNC_FLAGS defaults to `--merge` (strict), so a node genuinely lacking the secret now fails the funnel instead of emitting tier files. required=False is not "safer", it is silent — build_outputs() only records a missing key when the entry is required, so the funnel would keep reporting 0 errors for a node whose every container is ungated. Escape hatch: SECRETS_ALLOW_MISSING=1. 2. The Pattern-B antipattern. SECRETS_DISTRIBUTION_PATTERNS.md told operators to run `make secrets-funnel` right after pulling a CI bundle. That is the Pattern-A funnel: its secrets-funnel-sync step depends on chit-export (mk/codex.mk:111), which re-encodes the node's LOCAL env.shared over CHIT_EXPORT_PATH — the exact file the pull just installed. The CI credentials were destroyed before anything read them, and the node then materialized tier files from its own pre-existing state while appearing to succeed. The doc already contradicted itself: its status line (updated 2026-07-24) names secrets-pull and secrets-funnel-from-prod, while the copy-pasteable recipe below still carried the harmful command. The wrong half was the half operators use. Also corrected in the same block: the artifact name omitted its target segment (real name is chit-bundle-<target>-<run_id>, sync-secrets-local.yml:402), and the "adds a second workflow" trade-off was stale — the producer is the upload step already in sync-secrets-local.yml. 6 new tests pin the rename: the entry emits the host-side key only, an old bundle carrying just the alias still yields it, the canonical name wins when both are present, and absence raises rather than passing quietly. Operator step remains: `make -C pmoves chit-manifest-register && make -C pmoves chit-manifest-sync && make -C pmoves secrets-funnel`. The manifest YAML is machine-emitted and hook-protected; per the tool's own docstring agents edit the code-level registry, never the YAML. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(secrets): a blank canonical label silently beat a populated alias build_outputs() resolved a source by key PRESENCE alone. When the canonical label existed in the bundle with a zero-length value, it won over an alias carrying the real one: the blank was written as `KEY=` into every target file and kept out of `missing` even for required entries. Measured on B850 2026-08-18, and it is not hypothetical — env.shared carries CHIT_PROD_PASSPHRASE with a zero-length value while the GH-delivered CHIT_PASSPHRASE alias carries the real 64-char one. chit-export encodes env.shared into the bundle, so the blank canonical shadowed the good alias at exactly this line. Blank is worse than absent, which is why this was invisible: * compose `${KEY:?}` rejects empty, but `${KEY?}` accepts it, so half the obvious checks pass; * anything that SOURCES an env file and exports it re-exports the blank — and shell environment beats every --env-file. That is how a tool shell ends up shadowing a correct tier-file value with an empty string. _first_usable() now walks label-then-aliases and takes the first non-blank value, so "delivered as empty" and "never delivered" are treated identically — which is already how every consumer of a line-based env file treats them. Provenance note, since this lane is about instruments: the first version of this comment claimed the funnel had emitted an empty value and gated 8 services. That was wrong. The 8 services failed because THIS session's shell exported an empty CHIT_PROD_PASSPHRASE, shadowing correct tier files; with the export removed, `docker compose config` is clean across the whole stack. The underlying defect is real and the fix stands, but the observed symptom belonged to a different cause, and the wrong version was briefly committed to a code comment as fact — finding #6 of the instrument-trust audit, committed by its own author, again. Remaining and operator-owned: env.shared still holds the blank key (lane L5 — "env.shared residue; the pipeline has no clear-a-key operation"). Tier files override it for compose, so the stack is healthy; only sourced-and-exported environments are affected. 3 new tests: blank counts as missing, whitespace-only counts as missing, and a blank canonical falls through to a populated alias. 69 passed in tests/tools/ (2 pre-existing failures unrelated: one baselined at _known_failures.yaml:166, one caused by this same shell-export class via NATS_URL). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(audit): re-derive every citation against main after 49 commits Rebased onto a main that moved 49 commits (4090 landing PRs). Every line reference in the audit re-checked rather than assumed — which is the discipline the document itself argues for, and it caught four drifts: * merge-decision :68 -> :80, and its if-condition :79-84 -> :91-96 * secrets-funnel-sync-from-bundle codex.mk:121 -> :130 * secrets_sync.py:120-131 -> :112 (_first_usable), shifted by my own edit in the preceding commit * the sweep funnel, re-run rather than restated: 176/69/4 -> 178/71/4 current, 158/63/2 -> 160/65/2 legacy Stage 4 is still 0: the same four stage-3 sites, individually re-classified. And the claim that mattered most held — the job literally named `merge-gate` is STILL vacuous on today's main (PASSED=true, three echoes, never read), even though #2592 landed "the last required check that could not fail". #2592 fixed hardening-validation, not this. 13 other line citations verified unchanged. 34 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(agnote): B850-CLAUDE CLAIM secrets lane L4/L5 — signed, append-only Claims L4 + L5 from the L1-L7 board opened in PR #2594. Disjoint from Z890's Append-only per the Restore Safety rule: 3 insertions, 0 deletions. Records, with measurements rather than assertions: * the host-side CHIT key was never in the workflow env: map, gating every up-* target on the node because compose interpolates whole-file; * build_outputs() let a present-but-EMPTY canonical label beat a populated alias, writing KEY= into every target while staying out of `missing`; * the Pattern-B recipe told operators to run the funnel that overwrites the bundle they just pulled - and the same doc's status line already named the correct targets, so the wrong half was the copy-pasteable half. Two corrections to the board itself: * L4's "secrets-audit exits non-zero -> funnel fails at step 6" does NOT reproduce on B850: 12s, exit 0, 0 errors / 5 warnings. It is conditional on a node having ERROR-level findings, not universal. * a new defect found while verifying targets: the documented gate `make chit-manifest-register ARGS='--check'` cannot work, because ARGS is exported to sub-makes and env-bootstrap-lite forwards it to a tool that has no such flag. 7 targets share the prerequisite. Left unclaimed for the Make lane. And three corrections to my own earlier claims in this lane, including one that was briefly committed to a source comment as fact. The real cause was an empty export in this session's shell shadowing correct tier files - shell environment beats --env-file. With it removed, `docker compose config` is clean across the whole stack. CHIT trail signed (HMAC-SHA256, kid chit-signing-v01), identity resolved to the registered glyph rather than the fallback - finding #7's fix verified in production on a real signing operation, not a test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(silent-sweep): emit POSIX paths so the sweep is platform-stable test_counterexamples_are_not_flagged_as_pass_only failed on Windows: the sweep emitted "pmoves\tools\chit_security.py" while the test compares the POSIX literal, so the lookup found nothing and the assertion read as "the guard was not detected" when it had been. CI is Linux, so this passed there — the test could only fail on the platform a fleet operator actually runs it on. Fixed in the tool rather than the test: Site.path now uses .as_posix() instead of str(). str() yields backslashes on Windows, so every emitted path — and anything derived from it, including a baseline or a cross-referenced report — differed by platform for the same file. Forward slashes are what the rest of this repo's tooling records. pmoves/tests/tools: 31 passed (was 30 passed, 1 failed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Joins the existing audit lane (#2522 ruleset exposure, #2525 CI enforcement, #2527 reproducibility) rather than opening a new one.
The six
:9835200 OK%d/${#body}in an inlineExecStartclaude-pmoveslauncherWARN-then-execon a root it failed to resolvedmesggeometry_bus_health.pyParseResult→re.subraised, caught and relabelled "connection failed"AttributeErroras anImportError, then wrote it into a Makefile comment as factTwo shapes, different remedies. 1/2/5 are mechanizable — each is a surface returning success while the payload is absent, stale, or malformed; a check asserting content rather than status catches all three generically. 3/4/6 are not — a wrong question, unrelated noise destroying evidence, and a misread. No linter catches those.
What this deliberately does NOT do
It does not restate the verification discipline.
.claude/agents/verifier.mdalready specifies it, and predates this session:That covers the general case better than a restatement would. The gap documented here is invocation, not documentation — that agent was invoked zero times during a session in which it would have caught finding #6 immediately.
Writing a new PATTERNS section would have been the fifth instance of this session's recurring pattern: authoring something that already exists, unwired.
The genuinely new material
Mechanical traps, node-agnostic and in none of the existing surfaces:
$(...)strips all trailing newlines — bit four separate times in one evening: the responder, two tests of the responder, and the installer verifying itconnect_timeoutdoes not bound DNS resolution — 60s hang whileconnect_timeout=5looked authoritative; wrap inasyncio.wait_for%and${}in a systemdExecStartare expanded by systemd —%d→ credentials dir,%s→ user shellNATS_URLis container-scoped; a bareuv runenv has no venv packagesTwo notes for the lane
merge-gatehas been repaired. #2525 found it vacuous;pytest_ratchet.pynow runs all 264 test files (washead -20by filesystem order), the dep install lost its|| fallback, and the gateexit 1s. Verified before relying on it for the merges this session. The audit lane is driving fixes ahead of its own PRs merging — an argument for landing #2522/#2525/#2527.#2527's package may be undervalued as a calibration fixture. #2525 had to hand-roll "a deliberately failing test run through an equivalent path" to prove a check could fail. A frozen, network-isolated, hash-manifested package with six deterministic checks of known outcome is the standing form of that — a target whose answer is already known, which is exactly what every instrument in the table lacked.
Provenance
Every row verified by reading the underlying value, not the verdict:
Content-Lengthvs actual bytes,LnkSta: Width x16vs expectedx8,4.2%vs0.0%,merge-base --is-ancestorvs branch name,importlib.metadata.version()vs a traceback's first line. Claims that could not be checked on this node are marked as such rather than approximated.🤖 Generated with Claude Code