tools: four ways a check reported success without checking - #1111
Open
mdheller wants to merge 2 commits into
Open
tools: four ways a check reported success without checking#1111mdheller wants to merge 2 commits into
mdheller wants to merge 2 commits into
Conversation
Every fix below is paired with an observed failure: break the guarded thing, show red, restore, show green. Evidence is local — Actions is spend-capped. 1. An absent validator was a silent pass `run_optional_validator` ran a validator only `if validator.exists()`. All 11 call sites sit inside `tools/validate_repo.py`, which is `make validate-repo`, a leg of the REQUIRED diagnostics-gate. Deleting or renaming any of the 11 disarmed that check while the gate stayed green. Renamed to `run_validator`; absence is now a failure. The only way to tolerate it is an explicit `OPTIONAL_VALIDATORS` entry naming the reason, and even that prints a SKIP line. The dict ships empty — all 11 validators exist today. Red/green: with `validate_cell_gateway_api.py` renamed away, before rc=0 "OK: validate passed" -> after rc=2 naming the missing file -> restored rc=0. 2. `|| true` inside a required-gate leg Makefile:100, last recipe line of `lattice-studio-smoke`, which feeds diagnostics-gate via smoke-target-diagnostics. Stripping `|| true` showed what it hid: `--catalog-asset .../services_demo-inference-service.json` is missing its `/catalog-asset.json` segment, so the step exited 2 and neither studio-platform-records.json nor its enrichments were ever written. Same commit (847d4c3, #633) also severed the target's 14 `test -s` output assertions into a bogus `test -s:` target — a make target named `test` with prerequisite `-s`. `make -n lattice-studio-smoke` confirmed 0 assertions ran. Fixed the path, dropped `|| true`, re-attached the 14 assertions, deleted the bogus target and the two dead emit lines it carried. Red/green: re-break the path with no `|| true` -> `make lattice-studio-smoke` rc=2; restored -> rc=0 with all 14 assertions executing. 3. Empty globs read as pass Seven validators drive every check from a glob and print success when it matches nothing. Six say so literally ("0 <name> checks passed", rc=0); the seventh, adr-035, kept one non-glob check and reported 1 of 6. Each now materialises the glob, declares a MIN_FIXTURES floor set to what ships today, and fails before the loop if the match is short. Red/green: with each fixture directory emptied, all seven go rc=1 with a named diagnostic; restored, all seven pass with their original check counts. The audit called this four validators; measured against emptied directories it is seven. All seven are fixed. 4. Verification that evaporates under `python -O` Of the 44 bare asserts across 6 files, 41 are already claimed by open PRs: 38 by #1084 (the pattern followed here) and 3 by #1096/#1099, which owns validate_professional_intelligence_manifest.py. Only tools/test_liberty_stack_runtime_demo.py was unclaimed. Its 3 asserts are the whole check, so under -O it printed {"ok": true} regardless. None was vestigial and none sat behind an isinstance guard, so all three were converted rather than deleted. It also could not pass at all: the demo emits two JSON documents and the tool called json.loads on the stream, raising JSONDecodeError before any check. Now parses the readout document and raises DemoCheckFailure. Red/green under `python3 -O`: original asserts against a wrong payload -> rc=0 {"ok": true}; converted check -> DemoCheckFailure. End to end with the readout status sabotaged -> rc=1; restored -> rc=0 under both python3 and python3 -O. Guard tests: tools/tests/test_tools_silent_pass_guards.py (27 tests) locks in all four. Verified to have teeth both ways — reverting any one of validate_repo.py, the Makefile, a glob validator, or the liberty demo turns the relevant tests red. pytest tools/tests: 289 -> 316 passed. Untouched: validate-target-diagnostics.yml, gitops-promote.yml, images.yml, apps/health-twin/**, compute_gateway/**, infra/tofu/**, tools/validate_professional_intelligence_manifest.py.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR hardens the tooling/diagnostics gate by eliminating multiple “silent pass” paths where validations could report success without actually checking anything.
Changes:
- Make missing validators fail closed in
tools/validate_repo.py, with an explicit allowlist for truly optional checks. - Add “minimum fixture count” guards to multiple glob-driven validators so empty fixture sets become a hard failure.
- Fix
lattice-studio-smokein the Makefile by removing error suppression, correcting inputs, and re-attaching output assertions; add regression tests covering all silent-pass classes.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/validate_repo.py | Fail-closed validator dispatch with explicit optional allowlist and clearer missing-validator failure. |
| Makefile | Removes ` |
| tools/validate_workroom_schemas.py | Fails when fixture glob is empty/thinned below a minimum threshold. |
| tools/validate_semantic_governance.py | Same fixture-floor guard for semantic governance fixtures. |
| tools/validate_proof_artifacts.py | Same fixture-floor guard for proof artifact examples. |
| tools/validate_mutation_evidence.py | Same fixture-floor guard for mutation-evidence fixtures. |
| tools/validate_helper_causal_receipts.py | Same fixture-floor guard for causal receipts fixtures. |
| tools/validate_device_orchestration.py | Same fixture-floor guard for device orchestration fixtures. |
| tools/validate_adr_035_contracts.py | Adds fixture-floor guard for ADR-035 examples (but currently introduces a syntax issue). |
| tools/test_liberty_stack_runtime_demo.py | Replaces assert-based checks with explicit failures and parses concatenated JSON output correctly. |
| tools/tests/test_tools_silent_pass_guards.py | Adds regression tests to ensure these silent-pass shapes cannot return. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+89
to
+92
| print( | ||
| f"ERR: expected at least {MIN_FIXTURES} adr-035-*.json fixtures under " | ||
| f"{(CONTRACTS / "examples").relative_to(ROOT)}, found {len(fixture_paths)} — an empty or thinned fixture set " | ||
| f"silently disables this validator", |
Member
Author
There was a problem hiding this comment.
Valid and fixed in a4b1aaa. Confirmed: nested same-quotes in an f-string are Python 3.12+ (PEP 701), so on 3.11 this is a hard SyntaxError: f-string: unmatched '(' — and CI runs 3.11, so the file didn't parse and this fixture-floor guard couldn't run at all. Single-quoted the inner string (CONTRACTS / 'examples'); test_tools_silent_pass_guards.py went 1 failed/26 -> 27 passed on 3.11 (and stays 27 on 3.12). Good catch.
Comment on lines
+62
to
+75
| # Minimum-count assertion. An empty glob is a FAILURE, not a pass: every workroom | ||
| # check below is driven by these files, so "zero matched" means this validator ran and | ||
| # verified nothing while still printing success. The floor is the number that ships | ||
| # today — deleting or relocating fixtures has to trip this, not slip past it. | ||
| MIN_FIXTURES = 2 | ||
| fixture_paths = sorted(FIXTURES.glob("*.json")) | ||
| if len(fixture_paths) < MIN_FIXTURES: | ||
| print( | ||
| f"ERR: expected at least {MIN_FIXTURES} *.json fixtures under " | ||
| f"{FIXTURES.relative_to(ROOT)}, found {len(fixture_paths)} — an empty or thinned fixture set " | ||
| f"silently disables this validator", | ||
| file=sys.stderr, | ||
| ) | ||
| sys.exit(1) |
| # ── 1. an absent validator is a failure, not a skip ────────────────────────────── | ||
|
|
||
|
|
||
| def test_validate_repo_fails_when_a_validator_is_missing(tmp_path: Path) -> None: |
Comment on lines
+75
to
+83
| allowlist = re.search(r"OPTIONAL_VALIDATORS: dict\[str, str\] = \{(.*?)\}", source, re.S) | ||
| assert allowlist is not None, "OPTIONAL_VALIDATORS declaration not found" | ||
| for rel in sorted(dispatched): | ||
| if rel in allowlist.group(1): | ||
| continue | ||
| assert (ROOT / rel).exists(), ( | ||
| f"{rel} is dispatched by validate_repo.py but is not on disk, and has no " | ||
| f"OPTIONAL_VALIDATORS entry explaining why" | ||
| ) |
Comment on lines
+26
to
+36
| def iter_json_documents(text: str) -> Iterator[Any]: | ||
| """Yield each top-level JSON document from a concatenated stream.""" | ||
| decoder = json.JSONDecoder() | ||
| index = 0 | ||
| while True: | ||
| while index < len(text) and text[index].isspace(): | ||
| index += 1 | ||
| if index >= len(text): | ||
| return | ||
| document, index = decoder.raw_decode(text, index) | ||
| yield document |
The nested double-quotes in f"{(CONTRACTS / "examples").relative_to(ROOT)}"
are a SyntaxError on 3.11 (PEP 701 nested same-quotes are 3.12+). CI runs 3.11,
so validate_adr_035_contracts.py failed to parse — a fixture-floor guard that
cannot run. Single-quote the inner string. Guard suite: 1 failed/26 -> 27 passed
on 3.11. Addresses Copilot inline on #1111.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tools/tests/test_tools_silent_pass_guards.py:83
- This allowlist detection is brittle because it relies on regex + substring matching against the source text (e.g., formatting changes, comments, or one path being a substring of another can make the check behave incorrectly). Consider parsing
OPTIONAL_VALIDATORSviaast(find the annotated assignment andast.literal_evalits value) so the test is robust to formatting and only matches exact keys.
allowlist = re.search(r"OPTIONAL_VALIDATORS: dict\[str, str\] = \{(.*?)\}", source, re.S)
assert allowlist is not None, "OPTIONAL_VALIDATORS declaration not found"
for rel in sorted(dispatched):
if rel in allowlist.group(1):
continue
assert (ROOT / rel).exists(), (
f"{rel} is dispatched by validate_repo.py but is not on disk, and has no "
f"OPTIONAL_VALIDATORS entry explaining why"
)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four silent-pass classes in
tools/and the Makefile. Every fix is paired with an observed failure: break the guarded thing, show red, restore, show green.1 — an absent validator was a silent pass
run_optional_validatorran a validator onlyif validator.exists(). All 11 call sites live intools/validate_repo.py, which ismake validate-repo, a leg of the required diagnostics-gate. Deleting or renaming any of the 11 disarmed that check while the gate stayed green.Renamed to
run_validator. Absence is now a failure. The only way to tolerate it is an explicitOPTIONAL_VALIDATORSentry naming the reason, and even that prints aSKIPline rather than passing silently. The dict ships empty — all 11 validators exist on disk today, so this is fail-closed with no exemptions.Red → green,
tools/validate_cell_gateway_api.pyrenamed away:rc=0·OK: validate passedrc=2·ERR: validator missing: tools/validate_cell_gateway_api.py — …rc=0·OK: validate passed2 —
|| trueinside a required-gate legMakefile:100— the last recipe line oflattice-studio-smoke, which feeds diagnostics-gate throughsmoke-target-diagnostics.Stripping
|| trueshowed what it was hiding:The path is missing its
/catalog-asset.jsonsegment, so the step had been exiting 2 since it landed — neitherstudio-platform-records.jsonnor its enrichments were ever written.The same commit (
847d4c35, #633) also severed the target's 14test -soutput assertions into a bogustest -s:target — a make target namedtestwith prerequisite-s. They never ran:test -slines inlattice-studio-smoke|| trueFixed the path, removed
|| true, re-attached the 14 assertions to the target they belong to, and deleted the bogus target plus the two dead emit lines it carried. (Those dead lines carried a richer arg set that has never executed; kept the version that actually runs, so this is not a behaviour change.)Red → green: re-break the path with no
|| true→make lattice-studio-smokerc=2. Restored → rc=0, full target green with all 14 assertions executing.3 — empty globs read as pass
Seven validators drive every check from a glob and print success when it matches nothing. Six say so literally; the seventh kept one non-glob check and reported 1 of 6.
Each now materialises the glob, declares a
MIN_FIXTURESfloor set to what ships today, and fails before the loop if the match is short.Proof — each fixture directory emptied:
validate_device_orchestrationrc=0· 0 checks passedrc=1validate_helper_causal_receiptsrc=0· 0 checks passedrc=1validate_mutation_evidencerc=0· 0 checks passedrc=1validate_proof_artifactsrc=0· 0 checks passedrc=1validate_semantic_governancerc=0· 0 checks passedrc=1validate_workroom_schemasrc=0· 0 checks passedrc=1validate_adr_035_contractsrc=0· 1 of 6rc=1Restored, all seven pass with their original check counts, and all seven
maketargets are green.4 — verification that evaporates under
python -OOf the 44 bare asserts across 6 files, 41 are already claimed by open PRs:
smoke_regis_acr_service.pyvalidate_workspace_context_record.pysmoke_sourceos_m2_lifecycle_proof.pysmoke_sourceos_m2_filesystem_registry.pyvalidate_professional_intelligence_manifest.pytools/test_liberty_stack_runtime_demo.py#1084 is open and unmerged, so
mainstill shows all 44. Redoing its 38 would have guaranteed a conflict, so this PR follows its pattern on the one file no open PR touches.Converted, not deleted — all 3. None was vestigial and none sat behind a real
isinstanceguard (the #1096 false-positive shape). They are the tool's entire check: under-Oit printed{"ok": true}whatever the demo returned.It also could not pass at all: the demo emits two JSON documents and the tool called
json.loadson the whole stream, raisingJSONDecodeErrorbefore any check was reached. Now parses the readout document and raisesDemoCheckFailure.Red → green under
python3 -O:rc=0,{"ok": true, "subject_ref": "manifest://bogus"}DemoCheckFailure: readout action: expected 'validate_manifest', got 'WRONG'statussabotaged →rc=1; restored →rc=0under bothpython3andpython3 -OGuard tests
tools/tests/test_tools_silent_pass_guards.py— 27 tests locking in all four. A silent-pass fix can regress as silently as the original defect, so these assert the absence of the shape.Verified to have teeth both ways — reverting each fix turns the relevant tests red:
tools/validate_repo.pyMakefiletools/validate_workroom_schemas.pytools/test_liberty_stack_runtime_demo.pypytest tools/tests: 289 → 316 passed.Also green locally:
make validate-repo,make lattice-studio-smoke, and all seven patched validator targets.Lane
Untouched:
validate-target-diagnostics.yml(#1080/#1082),gitops-promote.yml(#1076),images.yml,apps/health-twin/**,compute_gateway/**,infra/tofu/**,tools/validate_professional_intelligence_manifest.py(#1096/#1099).🤖 Generated with Claude Code