Skip to content

tools: the PI manifest declared contract evidence it never stat'd - #1099

Open
mdheller wants to merge 1 commit into
mainfrom
fix/pi-manifest-evidence-paths-stat
Open

tools: the PI manifest declared contract evidence it never stat'd#1099
mdheller wants to merge 1 commit into
mainfrom
fix/pi-manifest-evidence-paths-stat

Conversation

@mdheller

Copy link
Copy Markdown
Member

Closes the declared-but-never-stat'd gap in
tools/validate_professional_intelligence_manifest.py, reported but not fixed
in #1096.

require_set_contains() proves the manifest lists a contract path. Nothing
ever checked the path was there. Deleting all five in-repo evidence files
and re-running the tool on unmodified origin/main, in a temp copy of the repo:

OK: professional-intelligence.manifest.yaml structure valid
OK: workspaceOS contract-aligned evidence present
OK: workroom update runtime boundary refs present
OK: workroom update invalid fixture is registered
OK: claim boundaries present
rc=0

A check that verifies a declaration about the world rather than the world.

What changed

require_declared_paths_exist() stats the in-repo members of
REQUIRED_WORKSPACE_OS_CONTRACTS / REQUIRED_WORKSPACE_OS_CONTROLS. It is
driven off the same constants the listing checks use, so the two cannot
drift, and the manifest cannot weaken the check by editing itself.

Cross-repo refs are skipped out loud

SocioProphet/<repo>:<path> refs resolve into peer repos absent from this
checkout and genuinely cannot be stat'd here. They are skipped deliberately and
the skip is printed, on every run, including green ones:

SKIP: workspaceOS.contractPaths: 3 cross-repo ref(s) not stat-able from this checkout, not verified here: [...]
OK: workspaceOS.contractPaths: 3 in-repo evidence path(s) exist on disk
SKIP: workspaceOS.controlRefs: 4 cross-repo ref(s) not stat-able from this checkout, not verified here: [...]
OK: workspaceOS.controlRefs: 2 in-repo evidence path(s) exist on disk

A silently-skipped ref is how this class comes back. 7 skipped, 5 verified,
both counts visible.

Classification matches <org>/<repo>: against a known-org set, not any
<a>/<b>: shape, so an unrecognised prefix falls through to the LOCAL side and
fails loudly. Skipping is the costly direction of a misclassification. This is
not theoretical: the first draft of the regex sent docs/notes:draft.md to the
skipped column and the new test caught it before commit.

Teeth — the mandatory proof

Each of the five deleted in turn in a temp copy of the repo:

tool rc behaviour
origin/main 0 prints OK: workspaceOS contract-aligned evidence present
this branch 2 ERR: workspaceOS.controlRefs declares 'docs/WORKROOM_UPDATE_RUNTIME_BOUNDARY.md' but that path does not exist in this repo

Restored → rc=0 again. All five caught, identical under python3 and
python3 -O
. Every one of the 21 new tests runs under both interpreters,
extending the -O discipline #1096 introduced rather than regressing it.

The baseline test asserts the evidence line is present, not merely rc=0 —
rc=0 alone would still pass if the check were deleted.

2. demoAcceptance named the wrong field

A non-mapping demoAcceptance collapsed to None through an inline ternary and
was then reported as "demoAcceptance.required must be a non-empty list". It
fails closed either way (rc=2), so this is diagnostics only. It now routes
through expect_mapping() like every other field in the file:

manifest before after
demoAcceptance: [...] ERR: demoAcceptance.required must be a non-empty list ERR: demoAcceptance must be a mapping/object
demoAcceptance: {required: []} ERR: demoAcceptance.required ... unchanged

Verification

  • tools/tests: 310 passed (289 on this base + 21 new). No regressions.
  • Real manifest still validates rc=0 under python3, python3 -O, and
    make validate-professional-intelligence-manifest.

⚠️ Merge-order note — interacts with #1096

#1096 is not merged, so this branch is cut from main and its 289-test
baseline excludes #1096's 9 tests. The two merge cleanly textually, but the
combination is not green, and that is worth catching before it lands:

#1096's run_against() builds a fixture root holding only the tool and the
manifest. Under this branch's tool that root is no longer a valid repo — the
five declared evidence paths are absent — so
test_real_manifest_still_validates fails (2 tests; the malformed-shape tests
still pass). The tool is behaving correctly; the fixture is under-specified.

Verified fix, 7 lines in #1096's file — whichever PR merges second applies it:

DECLARED_EVIDENCE = [
    "contracts/workspace/workroom-update-request.example.json",
    "contracts/workspace/workroom-update-response.accepted.example.json",
    "contracts/workspace/workroom-update-response.invalid-runtime-mutation.example.json",
    "docs/WORKROOM_UPDATE_RUNTIME_BOUNDARY.md",
    "tools/validate_workroom_update_contract.py",
]
# ...in run_against(), after copying the tool:
for rel in DECLARED_EVIDENCE:
    dest = root / rel
    dest.parent.mkdir(parents=True, exist_ok=True)
    shutil.copy2(REPO_ROOT / rel, dest)

Measured on the actual merge tree: 30 passed across both files with that
patch applied. Not applied here — the file does not exist on this branch's base.

Lane

tools/ only — two files. Does not touch
.github/workflows/validate-target-diagnostics.yml.

`require_set_contains()` proves the manifest LISTS a contract path. Nothing
checked the path was there. Deleting all five in-repo evidence files and
re-running the tool on origin/main:

    OK: professional-intelligence.manifest.yaml structure valid
    OK: workspaceOS contract-aligned evidence present
    OK: workroom update runtime boundary refs present
    OK: workroom update invalid fixture is registered
    OK: claim boundaries present
    rc=0

A check that verifies a declaration about the world rather than the world.

## What changed

`require_declared_paths_exist()` stats the in-repo members of
REQUIRED_WORKSPACE_OS_CONTRACTS / REQUIRED_WORKSPACE_OS_CONTROLS -- driven off
the same constants the listing checks use, so the two cannot drift, and the
manifest cannot weaken the check by editing itself.

Cross-repo refs (`SocioProphet/<repo>:<path>`) resolve into peer repos absent
from this checkout and genuinely cannot be stat'd here. They are skipped
deliberately and the skip is PRINTED, not silent -- an unreported skip is how
this class returns:

    SKIP: workspaceOS.controlRefs: 4 cross-repo ref(s) not stat-able from this
          checkout, not verified here: [...]
    OK: workspaceOS.controlRefs: 2 in-repo evidence path(s) exist on disk

Classification matches `<org>/<repo>:` against a known-org set rather than any
`<a>/<b>:` shape, so an unrecognised prefix falls through to the LOCAL side and
fails loudly. Skipping is the costly direction of a misclassification; the
first draft of this regex sent `docs/notes:draft.md` to the skipped column and
the new test caught it.

## Teeth, measured

Each of the five deleted in turn, in a temp copy of the repo:

| tool | rc | output |
|---|---|---|
| origin/main | 0 | prints the OK banner |
| this branch | 2 | `ERR: ... declares '<path>' but that path does not exist in this repo` |

Restored: rc=0 again. Identical results under `python3` and `python3 -O`.

## demoAcceptance named the wrong field

A non-mapping `demoAcceptance` collapsed to None through an inline ternary and
was then reported as *"demoAcceptance.required must be a non-empty list"*.
Fails closed either way (rc=2), so this is diagnostics only -- it now routes
through `expect_mapping()` like every other field in the file:

    demoAcceptance: [ ... ]   before: ERR: demoAcceptance.required must be a non-empty list
                              after:  ERR: demoAcceptance must be a mapping/object

`{"required": []}` still reports `.required`, unchanged.

## Verification

`tools/tests`: 310 passed (289 + 21 new). The real manifest still validates
rc=0 under `python3`, `python3 -O`, and `make
validate-professional-intelligence-manifest`.

Every new test runs under both interpreters, extending the `-O` discipline
#1096 introduced. The baseline test asserts the evidence line is present, not
just rc=0 -- rc=0 alone would still pass if the check were deleted.

## Lane

`tools/` only. Does not touch validate-target-diagnostics.yml.
Copilot AI review requested due to automatic review settings July 30, 2026 04:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Tightens the professional-intelligence manifest validator so it verifies that declared in-repo evidence paths actually exist, and improves diagnostics for malformed demoAcceptance.

Changes:

  • Add an existence check that stats declared in-repo evidence paths while explicitly reporting skipped cross-repo refs.
  • Fix demoAcceptance shape validation to report the correct field when it’s not a mapping.
  • Add tests that prove the new existence check runs (including under python -O) and fails loudly per missing path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
tools/validate_professional_intelligence_manifest.py Adds cross-repo ref classification + declared-path existence verification; improves demoAcceptance diagnostics.
tools/tests/test_professional_intelligence_manifest_evidence_paths.py Adds regression tests ensuring declared evidence is verified on disk and cross-repo skips are reported.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +126 to +127
local_refs = sorted(r for r in required if not is_cross_repo_ref(r))
skipped = sorted(r for r in required if is_cross_repo_ref(r))
Comment on lines 4 to 6
from pathlib import Path
import re
import sys
mdheller added a commit that referenced this pull request Aug 2, 2026
* tools: four ways a check reported success without checking

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.

* fix(tools): adr-035 fixture-floor f-string parses on Python 3.11

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants