tools: close the last Shape-C1 item — the PI manifest asserts were not the check

[mdheller]

Closes the last deferred Shape-C1 item, registered in #1084:
tools/validate_professional_intelligence_manifest.py L97/L119/L125 used bare
assert for shape checks, which python -O strips.

Probing it first changed the verdict. These three asserts are not
load-bearing, so #1084's remedy (require() / raise TypeError) does not apply
here — and applying it would have been a regression. Reporting that rather than
shipping a fix whose teeth I could not demonstrate.

Why they were never the check

Each assert sits immediately after an expect_mapping() guard that already
rejects a non-mapping. expect_mapping() is a plain isinstance test that
prints ERR: and returns False — not an assert, so -O leaves it intact.

line	guard that actually enforces it
L97	if not expect_mapping(data, "manifest"): return 2
L119	expect_mapping(...) → bad += 1; capabilities = {}
L125	expect_mapping(...) → bad += 1; workspace_os = {}

Each assert was therefore unreachable-with-a-false-condition: a narrowing hint
for a type checker this repo does not run (no mypy/pyright config, no CI
type-check step anywhere).

Proof, measured on origin/main before any edit

Malformed manifests (top level a list; capabilities a list;
capabilities.workspaceOS a list), run under python3 -O against the
unmodified main tool:

rc=2  -O  A: top-level is a LIST      ERR: manifest must be a mapping/object
rc=2  -O  B: capabilities is a LIST   ERR: capabilities must be a mapping/object
rc=2  -O  C: workspaceOS is a LIST    ERR: capabilities.workspaceOS must be a mapping/object

All three already fail closed under -O. There was no evaporating check.

Converting them to raise would additionally have broken deliberate behaviour:
L119/L125 normalise to {} specifically so a run keeps accumulating every
downstream error instead of stopping at the first.

What this PR does instead

1. Removes the three vestigial asserts, recording at each site which guard
   establishes the invariant — so the next Shape-C sweep does not re-flag this
   file for a fourth time.
2. Adds tools/tests/test_professional_intelligence_manifest_shape.py,
   turning "the guard happens to survive -O" into an enforced property. It
   runs the validator under python3 and python3 -O against each
   malformed shape, plus an AST check that fails if any bare assert returns.
   No other test in tools/tests exercises -O.

Behaviour is unchanged on every input. Pre- vs post-change output across
4 manifests x {python3, python3 -O} is byte-identical (diff: empty). The
real manifest still validates rc=0 under both interpreters and via
make validate-professional-intelligence-manifest.

The new test has teeth — verified both ways

mutation	result
restore the three asserts	AST test fails, naming [97, 119, 125]
make expect_mapping assert-based (the real fail-open)	7 of 9 fail, incl. all six -O/plain shape tests
restored	9 passed, file byte-identical to pre-mutation

tools/tests: 298 passed (289 + 9 new).

Also found in the file — reported, not changed

• demoAcceptance misdiagnoses a bad shape (L164). If demoAcceptance is
  present but not a mapping, the ternary yields None and the tool reports
  "demoAcceptance.required must be a non-empty list" — pointing at the wrong
  field. It still fails closed (rc=2); this is message quality, not teeth.
  Every other field in the file uses expect_mapping for this.
• metadata.name check is skipped when metadata is not a mapping (L117
  elif) — bad is already incremented so it fails closed, but the specific
  diagnostic is lost.
• Declared-but-never-stat'd evidence. The validator enforces that
  contractPaths / controlRefs list certain paths, but never checks they
  exist. The manifest could claim contract alignment against deleted files and
  still print "OK: workspaceOS contract-aligned evidence present". Cross-repo
  refs can't be checked locally, but five in-repo ones could be. I verified all
  five currently exist, so this is latent, not currently broken.

No other bare asserts remain in the file (AST-verified, and now CI-enforced).

Lane

tools/ only — two files. Does not touch validate-target-diagnostics.yml
(#1080 and #1082 were both still open when this was written, which is why this
item was deferred in the first place; the tool change is independent of them).

[Copilot]

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.

Removes vestigial assert isinstance(...) shape “checks” from the manifest validator (which would be stripped under python -O), and adds regression tests to ensure malformed shapes fail consistently under both normal and -O interpreters.

Changes:

• Replaced three bare shape asserts with comments documenting the existing expect_mapping() enforcement.
• Added subprocess-based tests that run the validator under python3 and python3 -O for malformed shapes.
• Added an AST-based test to detect reintroduction of bare assert in the validator.

Reviewed changes

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

File	Description
tools/validate_professional_intelligence_manifest.py	Removes bare asserts and documents why guards remain effective under -O.
tools/tests/test_professional_intelligence_manifest_shape.py	Adds -O parity tests and an AST guard against bare asserts in the validator.

---

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

[mdheller]

Disposition (merge-gate review)

Blocker fixed in 84a92be. On the materialized merged tree (main + #1099 + this PR), test_real_manifest_still_validates[python3] and [python3-O] were failing: once #1099 teaches the validator to stat the manifest's in-repo control-ref/contract paths, run_against() — which copied only the validator into its synthetic root — made the tool reject the real manifest for paths absent from the fixture, not from the repo. Mirrored build_root() from the evidence-paths suite: run_against now copies the manifest's in-repo evidence into the synthetic root; cross-repo refs stay skipped.

Verified on the merged tree: both cases pass, the two PI suites are 30 passed, and full tools/tests is 307 passed / 12 skipped / 0 failed. (Testing this PR alone — 9 passed — is exactly how the merged-tree failure was missed; please merge #1099 first, then this.)

The three inline notes (assert-scope vs docstring, cwd=/timeout= on the subprocess, stderr in the assertion message) are valid minor test-quality improvements — reasonable as a follow-up; left out here to keep this change scoped to the merged-tree failure.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (3)

tools/tests/test_professional_intelligence_manifest_shape.py:136

• The docstring says “no shape check … may be a bare assert”, but the implementation rejects any assert anywhere in the tool (even if it were unrelated to shape checks). Either (a) update the docstring/message to match the stricter invariant (“the tool must contain no assert at all”), or (b) narrow the AST check to only the specific assert patterns/locations you intend to forbid (e.g., assert isinstance(...) in main()).

def test_validator_carries_no_bare_asserts() -> None:
    """Guard the guard: no shape check in this tool may be a bare `assert`.

    Catches a regression at the source rather than only through behaviour --
    if someone reintroduces `assert isinstance(...)` here, -O would strip it.
    """
    import ast

    tree = ast.parse(TOOL.read_text(encoding="utf-8"))
    asserts = [n.lineno for n in ast.walk(tree) if isinstance(n, ast.Assert)]
    assert not asserts, f"{TOOL.name} regained bare assert(s) at line(s) {asserts}"

tools/tests/test_professional_intelligence_manifest_shape.py:85

• Consider adding a timeout= to this subprocess.run() call. If the validator ever regresses into a hang (e.g., unexpected blocking I/O), the test suite can stall indefinitely; a small timeout makes such failures deterministic and easier to debug.

    return subprocess.run(
        argv_prefix + [str(root / "tools" / TOOL.name)],
        capture_output=True,
        text=True,
    )

tools/tests/test_professional_intelligence_manifest_shape.py:99

• When returncode mismatches, this assertion only reports stdout, but the validator’s diagnostics are written to stderr (ERR: ...). For easier triage on CI failures, pass a message that includes stderr (or both stdout and stderr). This pattern repeats in several tests in this file.

    assert proc.returncode == 2, proc.stdout