From 4ba447b7183e4cf42d61ef132be2d01873ae54dd Mon Sep 17 00:00:00 2001 From: William Zujkowski Date: Fri, 2 Oct 2026 10:59:52 -0400 Subject: [PATCH 1/7] fix(acq-kits): give verify scripts a "could not check" verdict state All seven kit verify scripts ended with the same two-state verdict: [ "$fail" -eq 0 ] That answers "did anything fail?" and cannot answer "did we actually check anything?". Two consequences, both measured: 1. A run where NOTHING executed reported success. With pass=0 and fail=0 the test is true, so the script printed "All checks passed." and exited 0 having verified nothing. Reproduced in isolation: pass=0 fail=0 -> "All checks passed." exit 0 2. A skipped check was indistinguishable from a passing one. Across the seven scripts there are 29 skip/NOTE sites and 9 warn sites that increment no counter. Worst case: git-ssh-sign skips its end-to-end signed-commit check -- the kit's entire purpose -- whenever no key is in the forwarded SSH agent, and still reported "All checks passed." openchamber and paseo DO define skip() and warn(), but they are cosmetic printf with no counter, so a run can print 19 yellow SKIPs and still claim the offline gate passed. This is the ninth instance of this defect class in this family of repos (ENV-1, SEC-6, DX-7/8/9, SEC-12/13/17, DEP-4 are the prior ones), so the fix is a shared contract rather than seven local patches. WHAT THIS ADDS. integrations/isolation/acq-kits/verify-report.sh: five states and a verdict that is a function of a coverage record, not of a failure counter. ok ran and passed bad ran and failed unver COULD NOT RUN (missing tool, absent credential, no network) -- counted, and degrades the verdict skip not applicable in this configuration -- counted, does NOT degrade warn advisory; a documented fallback was taken -- does not degrade exit 0 every check that ran passed, and at least one ran exit 1 at least one check FAILED exit 3 nothing failed but the run is not a pass: a check could not be performed, or no check ran at all 3 is distinct from 1 so a caller can tell "we looked and found problems" from "we could not look". The unver/skip split is the load-bearing part: collapsing them is how a verify script claims coverage it does not have. STAGED CONVERSION. Two kits here -- git-ssh-sign and usai-provider, the two smallest -- so the contract is reviewable in isolation before the bulk edit. Remaining: openchamber (641 lines, 3 gates), paseo (864, 3), pi-coding-agent (337, 3), agentic-coding-playbook (167), zscaler-ca-certificate (167). They are listed in the test's UNCONVERTED set, which must reach empty. Converted sites: git-ssh-sign - no host SSH key -> unver (was a bare echo NOTE) - no forwarded key -> unver (was a bare echo SKIP; this is the kit's only end-to-end assertion) usai-provider - no USAi secret -> unver (was a bare echo NOTE) - API probe empty -> unver. in_sbx() swallows the exit status, so an empty result means EITHER "no curl in the guest" OR "sbx exec failed" -- indistinguishable, and never a pass. ENFORCEMENT. scripts/tests/test_verify_contract.py, 12 tests, because nothing in CI runs these shell scripts: they need a live sandbox this repo's CI cannot create, and the repo has no shellcheck hook, so 2,400 lines of shell are currently linted by nothing. The tests read the scripts as text and assert the library keeps BOTH degrading branches (a deleted branch silently restores the two-state verdict for every caller at once), that converted kits source it and do not shadow its reporters with no-op locals, that no bare `echo "SKIP"` remains, and that UNCONVERTED describes reality in BOTH directions -- a kit converted without being delisted fails, and so does a listed kit that reverted. Every helper asserts its input set is non-empty first, so the suite cannot pass vacuously over a glob that stopped matching. A near-miss worth recording: the library was first written to acq-kits/lib/, and .gitignore:13 carries the stock Python build-artifact block whose `lib/` rule silently excluded it. Committing would have shipped two kits sourcing a file git never tracked -- every verify run dying on "No such file or directory". Caught because `git status` listed the two modified kits and the new test but not the library. It now sits at the acq-kits root beside validate-kits.py, the existing precedent for shared assets there, and the reason is recorded in the test file so nobody re-adds a lib/. Verified: all five verdict states exercised directly: nothing ran -> exit 3 clean -> exit 0 a failure -> exit 1 an unver -> exit 3 skip-only -> exit 0 pytest scripts/tests/ -> 413 passed (12 new) degraded path of the TESTS: reverting usai-provider to origin/main -> FAILS "does not source lib/verify-report.sh" validate_repo.py --root . -> exit 0 validate-kits.py --root . --strict -> all kits OK scan_unsafe_shell.py . -> 0 errors, 0 warnings shellcheck -s sh verify-report.sh -> exit 0 shellcheck, converted kits -> git-ssh-sign 10 before / 10 after usai-provider 7 before / 7 after (pre-existing SC2015/SC2016 info findings; I added none) ruff check / ruff format --check -> clean markdownlint -> 0 issues git diff --check -> clean NOT IN THIS PR, deliberately: general shell linting. These scripts have never been shellcheck'd and adding a hook would surface unrelated findings across 2,400 lines; that belongs in its own change. This one closes the verdict-honesty gap only. AI assistance disclosed: prepared by an AI agent (OpenCode) on operator instruction. Human review and merge still required. --- integrations/isolation/acq-kits/README.md | 28 +++ .../acq-kits/git-ssh-sign/scripts/verify | 26 ++- .../acq-kits/usai-provider/scripts/verify | 24 +-- .../isolation/acq-kits/verify-report.sh | 148 ++++++++++++++ scripts/tests/test_verify_contract.py | 188 ++++++++++++++++++ 5 files changed, 387 insertions(+), 27 deletions(-) create mode 100644 integrations/isolation/acq-kits/verify-report.sh create mode 100644 scripts/tests/test_verify_contract.py diff --git a/integrations/isolation/acq-kits/README.md b/integrations/isolation/acq-kits/README.md index 048f6765..d1fd1701 100644 --- a/integrations/isolation/acq-kits/README.md +++ b/integrations/isolation/acq-kits/README.md @@ -80,3 +80,31 @@ make test-kits # Live, per-backend end-to-end check (needs a backend CLI + a sandbox-capable host): /scripts/verify ``` + +### What a `scripts/verify` exit code means + +Verify scripts share one verdict contract, `verify-report.sh` in this directory. +It exists because the previous per-kit verdict was `[ "$fail" -eq 0 ]`, which +reports success both when **zero** checks ran and when checks were **skipped** — +so a run that exercised nothing still printed "All checks passed." + +| Exit | Meaning | +|---|---| +| `0` | every check that ran passed, and at least one check ran | +| `1` | at least one check FAILED | +| `3` | nothing failed, but the run is **not** a pass: a check could not be performed, or no check ran at all | + +`3` is distinct from `1` so a caller can tell *"we looked and found problems"* +from *"we could not look"*. Treat any non-zero exit as "do not ship". + +The five reporters: `ok` / `bad` are the ran-and-passed / ran-and-failed cases; +`unver` is **could not check** (missing tool, absent credential, no network) and +degrades the verdict to `3`; `skip` is *not applicable in this configuration* +(e.g. a backend-specific check on the other backend) and does **not** degrade +it; `warn` is advisory. The `unver` / `skip` distinction is load-bearing — +collapsing the two is how a verify script ends up claiming coverage it does not +have. + +`scripts/tests/test_verify_contract.py` enforces this statically, because CI +cannot run these scripts (they need a live sandbox). Its `UNCONVERTED` set lists +kits not yet migrated; it must reach empty. diff --git a/integrations/isolation/acq-kits/git-ssh-sign/scripts/verify b/integrations/isolation/acq-kits/git-ssh-sign/scripts/verify index 9529bd7d..00886b89 100755 --- a/integrations/isolation/acq-kits/git-ssh-sign/scripts/verify +++ b/integrations/isolation/acq-kits/git-ssh-sign/scripts/verify @@ -31,11 +31,12 @@ WORK="$(mktemp -d "${TMPDIR:-/tmp}/git-ssh-sign-verify.XXXXXX")" CREATE_LOG="$WORK/sbx-create.log" KEEP="${KEEP:-0}" -pass=0 -fail=0 -ok() { printf ' \033[32mPASS\033[0m %s\n' "$1"; pass=$((pass+1)); } -bad() { printf ' \033[31mFAIL\033[0m %s\n' "$1"; fail=$((fail+1)); } -info() { printf '\n\033[1m%s\033[0m\n' "$1"; } +# Shared verdict contract (ok / bad / unver / skip / warn + verify_verdict). +# Sourced rather than redefined here so one fix cannot leave the other kits +# behind: the old local reporters had no "could not check" state, so a run that +# skipped every meaningful assertion still reported "All checks passed." +. "$KIT_DIR/../verify-report.sh" +verify_reset cleanup() { if [ "$KEEP" = "1" ]; then @@ -59,8 +60,7 @@ echo " sbx version: $(sbx version 2>/dev/null | grep -oE 'v?[0-9]+\.[0-9]+(\.[0 if ssh-add -L >/dev/null 2>&1; then ok "host SSH agent has a key loaded (signing can succeed)" else - echo " NOTE: no key in the host SSH agent. The kit installs fine, but signing" >&2 - echo " will fail until you run: ssh-add ~/.ssh/id_ed25519" >&2 + unver "host SSH agent has no key loaded — signing cannot be exercised (run: ssh-add ~/.ssh/id_ed25519)" fi info "1. sbx kit validate" @@ -102,12 +102,10 @@ if [ "$(in_sbx 'ssh-add -L >/dev/null 2>&1 && echo yes')" = "yes" ]; then ') [ "$out" = "SIGNED" ] && ok "commit was signed and verified" || bad "commit not signed (got: ${out:-none})" else - echo " SKIP: no key in the forwarded SSH agent inside the sandbox." >&2 - echo " Load one on the host (ssh-add) and re-run for the full check." >&2 + # This is the kit's whole purpose. Reporting it as anything other than + # UNVERIFIED would let the script claim the signing path works when it was + # never exercised. + unver "end-to-end signed commit — no key in the forwarded SSH agent (load one with ssh-add, then re-run)" fi -info "Summary" -printf ' %d passed, %d failed\n' "$pass" "$fail" -[ "$fail" -eq 0 ] && printf ' \033[32mAll checks passed.\033[0m\n' || printf ' \033[31mSome checks failed — see notes above.\033[0m\n' - -[ "$fail" -eq 0 ] +verify_verdict "Summary" diff --git a/integrations/isolation/acq-kits/usai-provider/scripts/verify b/integrations/isolation/acq-kits/usai-provider/scripts/verify index b6771750..ac51b6c9 100755 --- a/integrations/isolation/acq-kits/usai-provider/scripts/verify +++ b/integrations/isolation/acq-kits/usai-provider/scripts/verify @@ -39,11 +39,12 @@ CREATE_LOG="$WORK/sbx-create.log" KEEP="${KEEP:-0}" USAI_MODELS_URL="https://api.gsa.usai.gov/api/v1/models" -pass=0 -fail=0 -ok() { printf ' \033[32mPASS\033[0m %s\n' "$1"; pass=$((pass+1)); } -bad() { printf ' \033[31mFAIL\033[0m %s\n' "$1"; fail=$((fail+1)); } -info() { printf '\n\033[1m%s\033[0m\n' "$1"; } +# Shared verdict contract (ok / bad / unver / skip / warn + verify_verdict). +# Sourced rather than redefined here so one fix cannot leave the other kits +# behind: the old local reporters had no "could not check" state, so a run with +# no secret and no network still reported "All checks passed." +. "$KIT_DIR/../verify-report.sh" +verify_reset cleanup() { if [ "$KEEP" = "1" ]; then @@ -67,8 +68,7 @@ echo " sbx version: $(sbx version 2>/dev/null | grep -oE 'v?[0-9]+\.[0-9]+(\.[0 if sbx secret ls 2>/dev/null | grep -q 'api.gsa.usai.gov'; then ok "USAi custom secret is set" else - echo " NOTE: no USAi custom secret found. Set it with:" >&2 - echo " sbx secret set-custom -g --host api.gsa.usai.gov --env USAI_API_KEY" >&2 + unver "USAi custom secret is not set — the credential path cannot be exercised (sbx secret set-custom -g --host api.gsa.usai.gov --env USAI_API_KEY)" fi info "1. sbx kit validate" @@ -121,14 +121,12 @@ echo " HTTP status: ${status:-}" if [ "$status" = "200" ]; then ok "USAi models API returned 200" elif [ -z "$status" ]; then - echo " NOTE: could not probe (no curl, or no network); skipping." >&2 + # in_sbx swallows the exit status, so an empty result means either "no curl in + # the guest" or "sbx exec failed" — both are "could not look", never a pass. + unver "USAi models API — could not probe (no curl in the guest, or no network)" else bad "USAi API returned $status (key missing/expired, or egress blocked)" echo " Check: sbx policy log $SBX_NAME (api.gsa.usai.gov), and the USAi secret." >&2 fi -info "Summary" -printf ' %d passed, %d failed\n' "$pass" "$fail" -[ "$fail" -eq 0 ] && printf ' \033[32mAll checks passed.\033[0m\n' || printf ' \033[31mSome checks failed — see notes above.\033[0m\n' - -[ "$fail" -eq 0 ] +verify_verdict "Summary" diff --git a/integrations/isolation/acq-kits/verify-report.sh b/integrations/isolation/acq-kits/verify-report.sh new file mode 100644 index 00000000..88430085 --- /dev/null +++ b/integrations/isolation/acq-kits/verify-report.sh @@ -0,0 +1,148 @@ +#!/usr/bin/env bash +# +# verify-report.sh — the shared verdict contract for acq-kit `scripts/verify`. +# +# WHY THIS EXISTS +# +# Every kit's verify script defined its own reporters and ended with the same +# verdict: +# +# [ "$fail" -eq 0 ] +# +# That is a two-state tool: it answers "did anything fail?" and cannot answer +# "did we actually check anything?". Two consequences, both observed in the +# scripts this file replaces: +# +# 1. A run where NOTHING executed reports success. With pass=0 and fail=0 the +# line above is true, so the script prints "All checks passed." and exits 0 +# having verified nothing. +# 2. A skipped check is indistinguishable from a passing one. The scripts had +# 29 `skip`/NOTE sites and 9 `warn` sites that incremented no counter, so a +# run that skipped its single most important assertion — git-ssh-sign skips +# the end-to-end signed-commit check when no key is in the forwarded agent — +# still ended in "All checks passed." +# +# A verdict must be a function of a COVERAGE RECORD, not of a failure counter. +# This library supplies that record and the five states a verify script needs. +# +# STATES +# +# ok "" a check ran and passed +# bad "" a check ran and failed +# unver "" a check COULD NOT RUN — missing tool, absent +# credential, no network. Not a pass. Counted, +# and it degrades the verdict. +# skip "" deliberately not applicable in this +# configuration (e.g. a backend-specific check on +# the other backend). Counted and reported, but +# does NOT degrade the verdict. +# warn "" advisory only; a documented fallback was taken. +# Counted and reported; does not degrade. +# +# The distinction that matters is `unver` vs `skip`: "we could not look" is a +# degraded run and must not be reported as a pass, whereas "this does not apply +# here" is a complete run of a smaller set. Collapsing the two is how a verify +# script ends up claiming coverage it does not have. +# +# EXIT CODES (verify_verdict) +# +# 0 every check that ran passed, and at least one check ran +# 1 at least one check FAILED +# 3 nothing failed, but the run is not a clean pass: +# - at least one check could not be performed (unverified > 0), or +# - no check ran at all (pass == 0) +# +# 3 is separate from 1 so a caller can tell "we looked and found problems" from +# "we could not look". A caller that only cares about red/green can test for +# non-zero and will correctly refuse to treat a degraded run as success. +# +# USAGE +# +# . "$(dirname "$0")/../../lib/verify-report.sh" +# verify_reset # initialize the counters +# info "1. Something" +# ok "it worked" +# unver "API probe — no network" +# verify_verdict "Offline gate" # prints the summary, returns 0/1/3 +# +# `verify_verdict` takes an optional label so a script with more than one gate +# (openchamber, paseo and pi-coding-agent each have three) can name each one. +# +# POSIX sh compatible on purpose: these scripts run on contributor machines with +# whatever /bin/sh is, and the arithmetic and parameter expansion here stay +# within that subset. + +# Counters. Deliberately not `local` — they are the script-wide coverage record. +verify_reset() { + pass=0 + fail=0 + unverified=0 + skipped=0 + warned=0 +} + +ok() { + printf ' \033[32mPASS\033[0m %s\n' "$1" + pass=$((pass + 1)) +} + +bad() { + printf ' \033[31mFAIL\033[0m %s\n' "$1" + fail=$((fail + 1)) +} + +# Could not check. The message SHOULD say what was not checked AND why, because +# the reader's next question is always "why not". +unver() { + printf ' \033[35mUNVERIFIED\033[0m %s\n' "$1" + unverified=$((unverified + 1)) +} + +# Not applicable in this configuration. Does not degrade the verdict. +skip() { + printf ' \033[33mSKIP\033[0m %s\n' "$1" + skipped=$((skipped + 1)) +} + +# Advisory note; a documented fallback was taken. Does not degrade the verdict. +warn() { + printf ' \033[33mWARN\033[0m %s\n' "$1" + warned=$((warned + 1)) +} + +info() { + printf '\n\033[1m%s\033[0m\n' "$1" +} + +# Print the coverage record and return the verdict. +# +# $1 — optional gate label (default "Summary"). +verify_verdict() { + _vr_label="${1:-Summary}" + + info "$_vr_label" + printf ' %d passed, %d failed, %d unverified, %d skipped, %d warned\n' \ + "$pass" "$fail" "$unverified" "$skipped" "$warned" + + if [ "$fail" -gt 0 ]; then + printf ' \033[31m%s FAILED — see above.\033[0m\n' "$_vr_label" + return 1 + fi + + # Nothing ran. Arithmetic over zero must never produce a pass: with the old + # `[ "$fail" -eq 0 ]` verdict this exact state printed "All checks passed." + if [ "$pass" -eq 0 ]; then + printf ' \033[35m%s UNVERIFIED — no check ran, so nothing was proven.\033[0m\n' \ + "$_vr_label" + return 3 + fi + + if [ "$unverified" -gt 0 ]; then + printf ' \033[35m%s UNVERIFIED — %d check(s) could not be performed; this is not a pass.\033[0m\n' \ + "$_vr_label" "$unverified" + return 3 + fi + + printf ' \033[32m%s passed.\033[0m\n' "$_vr_label" + return 0 +} diff --git a/scripts/tests/test_verify_contract.py b/scripts/tests/test_verify_contract.py new file mode 100644 index 00000000..f40c053e --- /dev/null +++ b/scripts/tests/test_verify_contract.py @@ -0,0 +1,188 @@ +"""Contract tests for the acq-kit `scripts/verify` verdict discipline. + +WHY THIS FILE EXISTS + +Every kit's `scripts/verify` used to end with: + + [ "$fail" -eq 0 ] + +That is a two-state verdict. It answers "did anything fail?" and cannot answer +"did we actually check anything?", so two things were true of all seven scripts: + +1. A run where NOTHING executed reported success — with `pass=0` and `fail=0` the + test above is true, so the script printed "All checks passed." and exited 0. +2. A skipped check was indistinguishable from a passing one. Between them the + scripts had 29 `skip`/NOTE sites and 9 `warn` sites that incremented no + counter, including `git-ssh-sign` skipping its end-to-end signed-commit + check — the kit's entire purpose — whenever no key was in the forwarded agent. + +`integrations/isolation/acq-kits/verify-report.sh` replaces that with a +five-state contract whose verdict is a function of a coverage record. These tests +keep it that way. + +NOTHING IN CI RUNS THESE SHELL SCRIPTS. They need a live sandbox, which this +repo's CI cannot create, and the repo has no shellcheck hook. So the contract is +enforced here, statically, by reading them as text — otherwise the next verify +script added would quietly reintroduce the defect. + +MIGRATION STATE: the conversion is deliberately staged. `UNCONVERTED` lists the +kits still on the old pattern. Each test asserts the contract for converted kits +AND asserts that `UNCONVERTED` still describes reality, so the allowlist cannot +rot in either direction — a kit that gets converted without being removed from +the list fails, and a listed kit that silently reverts fails too. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[2] +ACQ_KITS = ROOT / "integrations/isolation/acq-kits" +# Lives at the acq-kits root beside validate-kits.py, NOT in a lib/ subdir: +# .gitignore carries the stock Python build-artifact block, whose `lib/` rule +# would silently exclude it from the repo. Two kits sourcing a file git never +# tracked would fail at run time with "No such file or directory". +LIB = ACQ_KITS / "verify-report.sh" + +# Kits whose verify script has NOT yet been migrated to the shared contract. +# Shrink this as they are converted; it must reach empty, and the test below +# fails if an entry no longer belongs here. +UNCONVERTED = { + "agentic-coding-playbook", + "openchamber", + "paseo", + "pi-coding-agent", + "zscaler-ca-certificate", +} + +# The old two-state verdict. Its absence is the thing being enforced. +LEGACY_VERDICT = re.compile(r'\[\s*"\$fail"\s+-eq\s+0\s*\]') + + +def _verify_scripts() -> dict[str, Path]: + found = {p.parent.parent.name: p for p in sorted(ACQ_KITS.glob("*/scripts/verify"))} + # Guard the guard: if the glob stops matching, every test below would pass + # vacuously over an empty set. That is the exact "no evidence read as a pass" + # defect these tests exist to prevent. + assert found, f"no scripts/verify found under {ACQ_KITS} — this suite cannot pass vacuously" + return found + + +def _converted() -> dict[str, Path]: + return {k: p for k, p in _verify_scripts().items() if k not in UNCONVERTED} + + +def test_shared_library_exists_and_defines_the_five_states(): + assert LIB.is_file(), f"missing shared verdict library at {LIB}" + text = LIB.read_text(encoding="utf-8") + for fn in ("ok()", "bad()", "unver()", "skip()", "warn()", "verify_verdict()", "verify_reset()"): + assert fn in text, f"{LIB.name} does not define {fn}" + + +def test_library_verdict_degrades_on_unverified_and_on_zero_checks(): + """The two branches that distinguish this from the old verdict. + + Asserted structurally rather than by running the shell, so the test stays + hermetic: if either branch is deleted, the library silently becomes the + two-state verdict again and every caller regresses at once. + """ + text = LIB.read_text(encoding="utf-8") + assert re.search(r'\[\s*"\$pass"\s+-eq\s+0\s*\]', text), ( + "verify_verdict no longer has a zero-checks-ran branch — a run that executed nothing would report a pass" + ) + assert re.search(r'\[\s*"\$unverified"\s+-gt\s+0\s*\]', text), ( + 'verify_verdict no longer degrades on unverified checks — "could not check" would be reported as a pass' + ) + assert "return 3" in text, "verify_verdict no longer has a distinct could-not-check exit code" + + +def test_unconverted_allowlist_is_accurate(): + """The allowlist must describe reality, in both directions.""" + scripts = _verify_scripts() + + unknown = UNCONVERTED - scripts.keys() + assert not unknown, f"UNCONVERTED names kits with no verify script: {sorted(unknown)} — remove them from the list" + + for kit in sorted(UNCONVERTED): + text = scripts[kit].read_text(encoding="utf-8") + assert "verify-report.sh" not in text, ( + f"{kit} now sources the shared library but is still listed as " + "UNCONVERTED — remove it from the allowlist so it is covered by the " + "contract tests" + ) + + assert _converted(), "no kit has adopted the shared contract — these tests would pass vacuously over an empty set" + + +@pytest.mark.parametrize("kit", sorted(_converted())) +def test_converted_kit_sources_the_shared_library(kit: str): + text = _converted()[kit].read_text(encoding="utf-8") + assert "verify-report.sh" in text, f"{kit}/scripts/verify does not source lib/verify-report.sh" + assert "verify_reset" in text, ( + f"{kit}/scripts/verify sources the library but never calls verify_reset, so its counters are unset" + ) + + +@pytest.mark.parametrize("kit", sorted(_converted())) +def test_converted_kit_has_no_legacy_two_state_verdict(kit: str): + text = _converted()[kit].read_text(encoding="utf-8") + hits = LEGACY_VERDICT.findall(text) + assert not hits, ( + f'{kit}/scripts/verify still gates on [ "$fail" -eq 0 ] ({len(hits)} ' + "occurrence(s)). That verdict reports success when zero checks ran and " + "when checks were skipped; use verify_verdict instead" + ) + + +@pytest.mark.parametrize("kit", sorted(_converted())) +def test_converted_kit_does_not_shadow_the_shared_reporters(kit: str): + """A local redefinition silently restores the old behaviour. + + A kit could source the library and then define its own `ok()`/`skip()` that + increments nothing — passing every other test here while behaving exactly as + before. + """ + text = _converted()[kit].read_text(encoding="utf-8") + for fn in ("ok", "bad", "unver", "skip", "warn", "info"): + shadow = re.compile(rf"^\s*{fn}\s*\(\)\s*\{{", re.MULTILINE) + assert not shadow.search(text), f"{kit}/scripts/verify redefines {fn}() locally, shadowing the shared contract" + + +@pytest.mark.parametrize("kit", sorted(_converted())) +def test_converted_kit_reports_every_gate(kit: str): + """Each `info "Summary"`-style gate must end in a verify_verdict call. + + openchamber, paseo and pi-coding-agent each have three gates; a conversion + that replaced only the first would leave the others reporting nothing. + """ + text = _converted()[kit].read_text(encoding="utf-8") + verdicts = text.count("verify_verdict") + assert verdicts >= 1, f"{kit}/scripts/verify never calls verify_verdict" + + # A gate that prints a summary header but reaches no verdict is a gate whose + # result is discarded. + headers = len(re.findall(r'info\s+"Summary"', text)) + assert headers == 0, ( + f'{kit}/scripts/verify still prints its own info "Summary" header ' + f"({headers}); verify_verdict prints the summary itself, so a hand-rolled " + "header means a gate bypassing the contract" + ) + + +def test_no_silent_skip_remains_in_converted_kits(): + """A bare `echo " SKIP..."` or `NOTE:` increments no counter. + + That is how a skipped check became indistinguishable from a passing one. In a + converted kit these must be `skip`/`unver` calls instead. + """ + offenders: list[str] = [] + for kit, path in sorted(_converted().items()): + for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + if re.match(r'\s*echo\s+"\s*(SKIP|NOTE)', line): + offenders.append(f"{kit}/scripts/verify:{lineno}: {line.strip()}") + assert not offenders, ( + "converted kits still report skips via bare echo, which increments no counter:\n " + "\n ".join(offenders) + ) From 49572de0af69ddf31773e6d7cfe5812b21f478d6 Mon Sep 17 00:00:00 2001 From: William Zujkowski Date: Fri, 2 Oct 2026 12:08:51 -0400 Subject: [PATCH 2/7] fix(acq-kits): classify kits explicitly so a new one cannot default to converted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught a real defect in the contract test, from a direction I had not anticipated. Rebased onto main (1.10.0) and fixed. WHAT HAPPENED. `oci-engine` (#437) landed on main while this change was in review — an EIGHTH verify script that did not exist when I branched. The test derived its converted set by subtraction: {k: p for k, p in _verify_scripts().items() if k not in UNCONVERTED} so a kit in neither set was treated as CONVERTED by default. The four contract tests then failed against it with "oci-engine/scripts/verify does not source lib/verify-report.sh", which is true but misleading: the real problem was that a kit appeared that nobody had classified. I had guarded both directions I thought of — a kit converted but still listed, and a listed kit that reverted — and missed the one that actually happened. The default was the bug: an unclassified kit silently inherited a contract it does not implement, which is the same shape as the defect this whole change exists to fix (something unmeasured treated as passing). THE FIX. Two explicit sets, CONVERTED and UNCONVERTED, plus `test_every_kit_is_classified` asserting that every verify script appears in exactly one of them. A kit in neither is now a hard error naming the real cause, and the sets cannot name a kit that has no verify script. Proved it by simulating a ninth unclassified kit: AssertionError: verify script(s) in no classification set: ['zz-phantom']. Add each to CONVERTED (if it sources verify-report.sh) or to UNCONVERTED (if it is still on the old two-state verdict). A kit is never converted by default. CORRECTED COUNTS, recounted on the rebased tree rather than carried forward: there are EIGHT verify scripts, not seven, and 35 uncounted `skip`/NOTE sites, not 29 (9 `warn` sites unchanged). oci-engine itself has 303 lines, 2 gates, and 2 `[ "$fail" -eq 0 ]` verdicts, so it has the primary defect too; it is listed as UNCONVERTED for the staged bulk conversion. Stale numbers in the library header and the test docstring are updated. UNCONVERTED is now: agentic-coding-playbook, oci-engine, openchamber, paseo, pi-coding-agent, zscaler-ca-certificate. It must reach empty. Unrelated to the code, but it is why this took a second pass: the CI log could not be read at first because `gh run view --log` failed with `x509: certificate signed by unknown authority`. Root cause is Go-specific and documented in the review workspace, not here — Go's x509.SystemCertPool() omits macOS admin-installed roots (121 of the host's 177), so any Go tool fails against a proxy-intercepted host while `curl` to the same host verifies fine. Reading the log required pointing SSL_CERT_FILE at a bundle including the admin roots. No repo change needed; noted so the next person who sees that error does not mistake it for a CI outage. Verified on the rebased tree: pytest scripts/tests/ -> 420 passed (13 contract tests) unclassified-kit probe -> FAILS with the naming assertion validate_repo.py --root . -> exit 0 validate-kits.py --root . --strict -> all kits OK scan_unsafe_shell.py . -> 0 errors, 0 warnings shellcheck -s sh verify-report.sh -> exit 0 ruff check / ruff format --check -> clean AI assistance disclosed: prepared by an AI agent (OpenCode) on operator instruction. Human review and merge still required. --- .../isolation/acq-kits/verify-report.sh | 2 +- scripts/tests/test_verify_contract.py | 55 ++++++++++++++++--- 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/integrations/isolation/acq-kits/verify-report.sh b/integrations/isolation/acq-kits/verify-report.sh index 88430085..9da15f62 100644 --- a/integrations/isolation/acq-kits/verify-report.sh +++ b/integrations/isolation/acq-kits/verify-report.sh @@ -17,7 +17,7 @@ # line above is true, so the script prints "All checks passed." and exits 0 # having verified nothing. # 2. A skipped check is indistinguishable from a passing one. The scripts had -# 29 `skip`/NOTE sites and 9 `warn` sites that incremented no counter, so a +# 35 `skip`/NOTE sites and 9 `warn` sites that incremented no counter, so a # run that skipped its single most important assertion — git-ssh-sign skips # the end-to-end signed-commit check when no key is in the forwarded agent — # still ended in "All checks passed." diff --git a/scripts/tests/test_verify_contract.py b/scripts/tests/test_verify_contract.py index f40c053e..17c03d5e 100644 --- a/scripts/tests/test_verify_contract.py +++ b/scripts/tests/test_verify_contract.py @@ -7,13 +7,13 @@ [ "$fail" -eq 0 ] That is a two-state verdict. It answers "did anything fail?" and cannot answer -"did we actually check anything?", so two things were true of all seven scripts: +"did we actually check anything?", so two things were true of all eight scripts: 1. A run where NOTHING executed reported success — with `pass=0` and `fail=0` the test above is true, so the script printed "All checks passed." and exited 0. -2. A skipped check was indistinguishable from a passing one. Between them the - scripts had 29 `skip`/NOTE sites and 9 `warn` sites that incremented no - counter, including `git-ssh-sign` skipping its end-to-end signed-commit +2. A skipped check was indistinguishable from a passing one. Across the eight + scripts there were 35 `skip`/NOTE sites and 9 `warn` sites that incremented + no counter, including `git-ssh-sign` skipping its end-to-end signed-commit check — the kit's entire purpose — whenever no key was in the forwarded agent. `integrations/isolation/acq-kits/verify-report.sh` replaces that with a @@ -48,16 +48,32 @@ LIB = ACQ_KITS / "verify-report.sh" # Kits whose verify script has NOT yet been migrated to the shared contract. -# Shrink this as they are converted; it must reach empty, and the test below -# fails if an entry no longer belongs here. +# Shrink this as they are converted; it must reach empty. +# +# This is an explicit DENY list, not a default. A kit absent from both this set +# and CONVERTED is a hard error (see test_every_kit_is_classified), because the +# alternative -- treating an unclassified kit as converted -- is how a new kit +# silently inherits a contract it does not implement. That is not hypothetical: +# `oci-engine` landed on main while this change was in review, and the original +# set-subtraction classified it as converted, failing four tests with a +# misleading "does not source lib/verify-report.sh" instead of the true cause +# ("a kit appeared that nobody classified"). UNCONVERTED = { "agentic-coding-playbook", + "oci-engine", "openchamber", "paseo", "pi-coding-agent", "zscaler-ca-certificate", } +# Kits that HAVE adopted the shared contract. Listing them explicitly, rather +# than deriving them by subtraction, is what makes an unclassified kit visible. +CONVERTED = { + "git-ssh-sign", + "usai-provider", +} + # The old two-state verdict. Its absence is the thing being enforced. LEGACY_VERDICT = re.compile(r'\[\s*"\$fail"\s+-eq\s+0\s*\]') @@ -72,7 +88,7 @@ def _verify_scripts() -> dict[str, Path]: def _converted() -> dict[str, Path]: - return {k: p for k, p in _verify_scripts().items() if k not in UNCONVERTED} + return {k: p for k, p in _verify_scripts().items() if k in CONVERTED} def test_shared_library_exists_and_defines_the_five_states(): @@ -99,6 +115,31 @@ def test_library_verdict_degrades_on_unverified_and_on_zero_checks(): assert "return 3" in text, "verify_verdict no longer has a distinct could-not-check exit code" +def test_every_kit_is_classified(): + """A kit in neither set is a hard error, not a default. + + This is the test that would have named the real problem when `oci-engine` + appeared: the failure message says "a kit appeared that nobody classified" + rather than four confusing assertions about a missing `source` line. + """ + scripts = _verify_scripts() + classified = CONVERTED | UNCONVERTED + + unclassified = scripts.keys() - classified + assert not unclassified, ( + f"verify script(s) in no classification set: {sorted(unclassified)}. " + "Add each to CONVERTED (if it sources verify-report.sh) or to " + "UNCONVERTED (if it is still on the old two-state verdict). A kit is " + "never converted by default." + ) + + phantom = classified - scripts.keys() + assert not phantom, f"classification sets name kits with no verify script: {sorted(phantom)} — remove them" + + overlap = CONVERTED & UNCONVERTED + assert not overlap, f"kits in both sets: {sorted(overlap)}" + + def test_unconverted_allowlist_is_accurate(): """The allowlist must describe reality, in both directions.""" scripts = _verify_scripts() From 4f26eb41028e5968c899e281734ee1317d0499d3 Mon Sep 17 00:00:00 2001 From: William Zujkowski Date: Tue, 6 Oct 2026 12:24:00 -0400 Subject: [PATCH 3/7] fix(acq-kits): classify opencode in the verify-contract allowlist; trim comment narration per mogul's review Two fixes, both from mogul's CHANGES_REQUESTED review: 1. Rebase onto main pulled in the opencode kit (#466), which landed with a verify script in neither CONVERTED nor UNCONVERTED, failing test_every_kit_is_classified exactly as the test is designed to catch (reproduced locally before fixing: `AssertionError: verify script(s) in no classification set: ['opencode']`). opencode's verify script is on the old two-state verdict, so it goes in UNCONVERTED alongside the other not-yet-migrated kits. 2. Trimmed six comment/docstring blocks that narrated the OLD two-state verdict's history ("used to end with...", "the old reporters had no...", the oci-engine near-miss anecdote) instead of describing the current code. Per review: this kind of narration belongs in the commit message, not in the code, where it has no reader value and persists as noise. Trimmed in: - integrations/isolation/acq-kits/git-ssh-sign/scripts/verify - integrations/isolation/acq-kits/usai-provider/scripts/verify - integrations/isolation/acq-kits/README.md - integrations/isolation/acq-kits/verify-report.sh (module header) - scripts/tests/test_verify_contract.py (module docstring + 2 inline comments + 1 docstring) Comment-only / allowlist-entry changes: no test logic, no shell logic, no shared-library behavior changed. Per the review's own standing offer ("if you respond with improvements that only land in comments... feel free to check the box that skips the require-review gate"), this should qualify to merge without a second review round-trip. Verification: - PYTHONPATH=scripts python3 -m pytest scripts/tests/: 420 passed (was failing 1/420 before the opencode classification fix, reproduced first). - python3 scripts/validate_repo.py: all validators pass. Co-authored-by: OpenCode Agent --- integrations/isolation/acq-kits/README.md | 5 +- .../acq-kits/git-ssh-sign/scripts/verify | 5 +- .../acq-kits/usai-provider/scripts/verify | 5 +- .../isolation/acq-kits/verify-report.sh | 30 +++--------- scripts/tests/test_verify_contract.py | 47 ++++++------------- 5 files changed, 27 insertions(+), 65 deletions(-) diff --git a/integrations/isolation/acq-kits/README.md b/integrations/isolation/acq-kits/README.md index 3c38a0e1..6c66a60d 100644 --- a/integrations/isolation/acq-kits/README.md +++ b/integrations/isolation/acq-kits/README.md @@ -85,9 +85,8 @@ make test-kits ### What a `scripts/verify` exit code means Verify scripts share one verdict contract, `verify-report.sh` in this directory. -It exists because the previous per-kit verdict was `[ "$fail" -eq 0 ]`, which -reports success both when **zero** checks ran and when checks were **skipped** — -so a run that exercised nothing still printed "All checks passed." +A verdict is a function of a coverage record, not a failure counter, so a run +that exercised nothing cannot report the same result as a run that passed. | Exit | Meaning | |---|---| diff --git a/integrations/isolation/acq-kits/git-ssh-sign/scripts/verify b/integrations/isolation/acq-kits/git-ssh-sign/scripts/verify index 00886b89..17aaf0c5 100755 --- a/integrations/isolation/acq-kits/git-ssh-sign/scripts/verify +++ b/integrations/isolation/acq-kits/git-ssh-sign/scripts/verify @@ -32,9 +32,8 @@ CREATE_LOG="$WORK/sbx-create.log" KEEP="${KEEP:-0}" # Shared verdict contract (ok / bad / unver / skip / warn + verify_verdict). -# Sourced rather than redefined here so one fix cannot leave the other kits -# behind: the old local reporters had no "could not check" state, so a run that -# skipped every meaningful assertion still reported "All checks passed." +# Sourced rather than redefined here so a future fix to the contract cannot +# leave this kit's copy behind. . "$KIT_DIR/../verify-report.sh" verify_reset diff --git a/integrations/isolation/acq-kits/usai-provider/scripts/verify b/integrations/isolation/acq-kits/usai-provider/scripts/verify index ac51b6c9..b8564ff1 100755 --- a/integrations/isolation/acq-kits/usai-provider/scripts/verify +++ b/integrations/isolation/acq-kits/usai-provider/scripts/verify @@ -40,9 +40,8 @@ KEEP="${KEEP:-0}" USAI_MODELS_URL="https://api.gsa.usai.gov/api/v1/models" # Shared verdict contract (ok / bad / unver / skip / warn + verify_verdict). -# Sourced rather than redefined here so one fix cannot leave the other kits -# behind: the old local reporters had no "could not check" state, so a run with -# no secret and no network still reported "All checks passed." +# Sourced rather than redefined here so a future fix to the contract cannot +# leave this kit's copy behind. . "$KIT_DIR/../verify-report.sh" verify_reset diff --git a/integrations/isolation/acq-kits/verify-report.sh b/integrations/isolation/acq-kits/verify-report.sh index 9da15f62..0030fa71 100644 --- a/integrations/isolation/acq-kits/verify-report.sh +++ b/integrations/isolation/acq-kits/verify-report.sh @@ -2,28 +2,10 @@ # # verify-report.sh — the shared verdict contract for acq-kit `scripts/verify`. # -# WHY THIS EXISTS -# -# Every kit's verify script defined its own reporters and ended with the same -# verdict: -# -# [ "$fail" -eq 0 ] -# -# That is a two-state tool: it answers "did anything fail?" and cannot answer -# "did we actually check anything?". Two consequences, both observed in the -# scripts this file replaces: -# -# 1. A run where NOTHING executed reports success. With pass=0 and fail=0 the -# line above is true, so the script prints "All checks passed." and exits 0 -# having verified nothing. -# 2. A skipped check is indistinguishable from a passing one. The scripts had -# 35 `skip`/NOTE sites and 9 `warn` sites that incremented no counter, so a -# run that skipped its single most important assertion — git-ssh-sign skips -# the end-to-end signed-commit check when no key is in the forwarded agent — -# still ended in "All checks passed." -# -# A verdict must be a function of a COVERAGE RECORD, not of a failure counter. -# This library supplies that record and the five states a verify script needs. +# A verdict is a function of a COVERAGE RECORD, not a failure counter: a run +# that checked nothing, and a run where every check passed, must not produce +# the same result. This library supplies that record and the five states a +# verify script needs to express it. # # STATES # @@ -129,8 +111,8 @@ verify_verdict() { return 1 fi - # Nothing ran. Arithmetic over zero must never produce a pass: with the old - # `[ "$fail" -eq 0 ]` verdict this exact state printed "All checks passed." + # Nothing ran. Arithmetic over zero must never produce a pass: pass=0 and + # fail=0 is a report of nothing proven, not a report of success. if [ "$pass" -eq 0 ]; then printf ' \033[35m%s UNVERIFIED — no check ran, so nothing was proven.\033[0m\n' \ "$_vr_label" diff --git a/scripts/tests/test_verify_contract.py b/scripts/tests/test_verify_contract.py index 17c03d5e..9efa1418 100644 --- a/scripts/tests/test_verify_contract.py +++ b/scripts/tests/test_verify_contract.py @@ -1,29 +1,15 @@ """Contract tests for the acq-kit `scripts/verify` verdict discipline. -WHY THIS FILE EXISTS - -Every kit's `scripts/verify` used to end with: - - [ "$fail" -eq 0 ] - -That is a two-state verdict. It answers "did anything fail?" and cannot answer -"did we actually check anything?", so two things were true of all eight scripts: - -1. A run where NOTHING executed reported success — with `pass=0` and `fail=0` the - test above is true, so the script printed "All checks passed." and exited 0. -2. A skipped check was indistinguishable from a passing one. Across the eight - scripts there were 35 `skip`/NOTE sites and 9 `warn` sites that incremented - no counter, including `git-ssh-sign` skipping its end-to-end signed-commit - check — the kit's entire purpose — whenever no key was in the forwarded agent. - -`integrations/isolation/acq-kits/verify-report.sh` replaces that with a -five-state contract whose verdict is a function of a coverage record. These tests -keep it that way. +`integrations/isolation/acq-kits/verify-report.sh` gives converted kits a +five-state verdict (`ok`/`bad`/`unver`/`skip`/`warn`) whose exit code is a +function of a coverage record, not a failure counter — this distinguishes "we +checked and it passed" from "nothing ran" and from "we could not check this +part." These tests keep that contract in force as kits adopt it. NOTHING IN CI RUNS THESE SHELL SCRIPTS. They need a live sandbox, which this -repo's CI cannot create, and the repo has no shellcheck hook. So the contract is -enforced here, statically, by reading them as text — otherwise the next verify -script added would quietly reintroduce the defect. +repo's CI cannot create, and the repo has no shellcheck hook. So the contract +is enforced here, statically, by reading them as text — otherwise the next +verify script added would quietly ship without the five-state verdict. MIGRATION STATE: the conversion is deliberately staged. `UNCONVERTED` lists the kits still on the old pattern. Each test asserts the contract for converted kits @@ -51,16 +37,13 @@ # Shrink this as they are converted; it must reach empty. # # This is an explicit DENY list, not a default. A kit absent from both this set -# and CONVERTED is a hard error (see test_every_kit_is_classified), because the -# alternative -- treating an unclassified kit as converted -- is how a new kit -# silently inherits a contract it does not implement. That is not hypothetical: -# `oci-engine` landed on main while this change was in review, and the original -# set-subtraction classified it as converted, failing four tests with a -# misleading "does not source lib/verify-report.sh" instead of the true cause -# ("a kit appeared that nobody classified"). +# and CONVERTED is a hard error (see test_every_kit_is_classified) — the +# alternative, treating an unclassified kit as converted by subtraction, lets a +# new kit silently inherit a contract it does not implement. UNCONVERTED = { "agentic-coding-playbook", "oci-engine", + "opencode", "openchamber", "paseo", "pi-coding-agent", @@ -118,9 +101,9 @@ def test_library_verdict_degrades_on_unverified_and_on_zero_checks(): def test_every_kit_is_classified(): """A kit in neither set is a hard error, not a default. - This is the test that would have named the real problem when `oci-engine` - appeared: the failure message says "a kit appeared that nobody classified" - rather than four confusing assertions about a missing `source` line. + The failure message names the real problem directly: a kit appeared that + nobody classified, rather than failing a different test with a misleading + cause (e.g. "does not source lib/verify-report.sh"). """ scripts = _verify_scripts() classified = CONVERTED | UNCONVERTED From b7050f86b1b98f175b445715ea967d2b3352bc03 Mon Sep 17 00:00:00 2001 From: wz-gsa Date: Mon, 12 Oct 2026 14:42:13 +0000 Subject: [PATCH 4/7] test(acq-kits): execute the verify verdict contract Replace text-presence assertions with a six-state executable matrix, require real library/verdict invocations, and trim the remaining investigation-history narration. Co-authored-by: OpenCode Agent --- integrations/isolation/acq-kits/README.md | 10 ++-- .../isolation/acq-kits/verify-report.sh | 6 +- scripts/tests/test_verify_contract.py | 57 ++++++++++++------- 3 files changed, 41 insertions(+), 32 deletions(-) diff --git a/integrations/isolation/acq-kits/README.md b/integrations/isolation/acq-kits/README.md index 6c66a60d..a92854bd 100644 --- a/integrations/isolation/acq-kits/README.md +++ b/integrations/isolation/acq-kits/README.md @@ -101,10 +101,8 @@ The five reporters: `ok` / `bad` are the ran-and-passed / ran-and-failed cases; `unver` is **could not check** (missing tool, absent credential, no network) and degrades the verdict to `3`; `skip` is *not applicable in this configuration* (e.g. a backend-specific check on the other backend) and does **not** degrade -it; `warn` is advisory. The `unver` / `skip` distinction is load-bearing — -collapsing the two is how a verify script ends up claiming coverage it does not -have. +it; `warn` is advisory. -`scripts/tests/test_verify_contract.py` enforces this statically, because CI -cannot run these scripts (they need a live sandbox). Its `UNCONVERTED` set lists -kits not yet migrated; it must reach empty. +`scripts/tests/test_verify_contract.py` executes the shared verdict contract and +statically checks converted scripts. Its `UNCONVERTED` set lists kits not yet +migrated; it must reach empty. diff --git a/integrations/isolation/acq-kits/verify-report.sh b/integrations/isolation/acq-kits/verify-report.sh index 0030fa71..02118353 100644 --- a/integrations/isolation/acq-kits/verify-report.sh +++ b/integrations/isolation/acq-kits/verify-report.sh @@ -21,10 +21,8 @@ # warn "" advisory only; a documented fallback was taken. # Counted and reported; does not degrade. # -# The distinction that matters is `unver` vs `skip`: "we could not look" is a -# degraded run and must not be reported as a pass, whereas "this does not apply -# here" is a complete run of a smaller set. Collapsing the two is how a verify -# script ends up claiming coverage it does not have. +# `unver` degrades the verdict because a check could not run. `skip` records a +# check that does not apply to the current configuration and does not degrade it. # # EXIT CODES (verify_verdict) # diff --git a/scripts/tests/test_verify_contract.py b/scripts/tests/test_verify_contract.py index 9efa1418..04d3c340 100644 --- a/scripts/tests/test_verify_contract.py +++ b/scripts/tests/test_verify_contract.py @@ -6,10 +6,9 @@ checked and it passed" from "nothing ran" and from "we could not check this part." These tests keep that contract in force as kits adopt it. -NOTHING IN CI RUNS THESE SHELL SCRIPTS. They need a live sandbox, which this -repo's CI cannot create, and the repo has no shellcheck hook. So the contract -is enforced here, statically, by reading them as text — otherwise the next -verify script added would quietly ship without the five-state verdict. +CI does not run the kit verify scripts because they need a live sandbox. These +tests execute the shared reporting library in an isolated shell and statically +check that converted scripts use it. MIGRATION STATE: the conversion is deliberately staged. `UNCONVERTED` lists the kits still on the old pattern. Each test asserts the contract for converted kits @@ -21,6 +20,8 @@ from __future__ import annotations import re +import shlex +import subprocess from pathlib import Path import pytest @@ -81,21 +82,30 @@ def test_shared_library_exists_and_defines_the_five_states(): assert fn in text, f"{LIB.name} does not define {fn}" -def test_library_verdict_degrades_on_unverified_and_on_zero_checks(): - """The two branches that distinguish this from the old verdict. - - Asserted structurally rather than by running the shell, so the test stays - hermetic: if either branch is deleted, the library silently becomes the - two-state verdict again and every caller regresses at once. - """ - text = LIB.read_text(encoding="utf-8") - assert re.search(r'\[\s*"\$pass"\s+-eq\s+0\s*\]', text), ( - "verify_verdict no longer has a zero-checks-ran branch — a run that executed nothing would report a pass" +@pytest.mark.parametrize( + ("reporters", "expected"), + [ + pytest.param([], 3, id="zero-activity"), + pytest.param(["ok"], 0, id="pass"), + pytest.param(["bad"], 1, id="failure"), + pytest.param(["ok", "unver"], 3, id="pass-and-unverified"), + pytest.param(["skip"], 3, id="skip-only"), + pytest.param(["ok", "skip", "warn"], 0, id="pass-skip-and-warn"), + ], +) +def test_library_verdict_contract(reporters: list[str], expected: int): + commands = [f". {shlex.quote(str(LIB))}", "verify_reset"] + commands.extend(f'{reporter} "contract fixture"' for reporter in reporters) + commands.append('verify_verdict "Contract fixture"') + + result = subprocess.run( + ["sh", "-c", "\n".join(commands)], + capture_output=True, + text=True, + check=False, ) - assert re.search(r'\[\s*"\$unverified"\s+-gt\s+0\s*\]', text), ( - 'verify_verdict no longer degrades on unverified checks — "could not check" would be reported as a pass' - ) - assert "return 3" in text, "verify_verdict no longer has a distinct could-not-check exit code" + + assert result.returncode == expected, result.stdout + result.stderr def test_every_kit_is_classified(): @@ -144,8 +154,11 @@ def test_unconverted_allowlist_is_accurate(): @pytest.mark.parametrize("kit", sorted(_converted())) def test_converted_kit_sources_the_shared_library(kit: str): text = _converted()[kit].read_text(encoding="utf-8") - assert "verify-report.sh" in text, f"{kit}/scripts/verify does not source lib/verify-report.sh" - assert "verify_reset" in text, ( + commands = [line.strip() for line in text.splitlines() if line.strip() and not line.lstrip().startswith("#")] + source = any(re.match(r"^\.\s+", line) and "verify-report.sh" in line for line in commands) + reset = re.search(r"^\s*verify_reset\s*(?:#.*)?$", text, re.MULTILINE) + assert source, f"{kit}/scripts/verify does not source verify-report.sh" + assert reset, ( f"{kit}/scripts/verify sources the library but never calls verify_reset, so its counters are unset" ) @@ -183,8 +196,8 @@ def test_converted_kit_reports_every_gate(kit: str): that replaced only the first would leave the others reporting nothing. """ text = _converted()[kit].read_text(encoding="utf-8") - verdicts = text.count("verify_verdict") - assert verdicts >= 1, f"{kit}/scripts/verify never calls verify_verdict" + verdicts = re.findall(r"^\s*verify_verdict(?:\s|$)", text, re.MULTILINE) + assert verdicts, f"{kit}/scripts/verify never calls verify_verdict" # A gate that prints a summary header but reaches no verdict is a gate whose # result is discarded. From 98efa06d0a22b37fa11505a5f261cb7d7fb6f75e Mon Sep 17 00:00:00 2001 From: wz-gsa Date: Mon, 12 Oct 2026 14:54:28 +0000 Subject: [PATCH 5/7] fix(acq-kits): classify missing sbx as unverified Require the exact shared reporter source, exercise both missing-tool degraded paths, and keep contract-test comments in present-tense invariant form. Co-authored-by: OpenCode Agent --- .../acq-kits/git-ssh-sign/scripts/verify | 5 +- .../acq-kits/usai-provider/scripts/verify | 5 +- scripts/tests/test_verify_contract.py | 52 +++++++++++++++---- 3 files changed, 48 insertions(+), 14 deletions(-) diff --git a/integrations/isolation/acq-kits/git-ssh-sign/scripts/verify b/integrations/isolation/acq-kits/git-ssh-sign/scripts/verify index 17aaf0c5..bf866392 100755 --- a/integrations/isolation/acq-kits/git-ssh-sign/scripts/verify +++ b/integrations/isolation/acq-kits/git-ssh-sign/scripts/verify @@ -51,8 +51,9 @@ in_sbx() { sbx exec "$SBX_NAME" -- sh -c "$1" /dev/null || true; } info "0. Preconditions" if ! command -v sbx >/dev/null 2>&1; then - echo " sbx not on PATH. Install sbx >= 0.34.0 and retry." >&2 - exit 1 + unver "sbx not on PATH — install sbx >= 0.34.0 and retry" + verify_verdict "Summary" + exit $? fi echo " sbx version: $(sbx version 2>/dev/null | grep -oE 'v?[0-9]+\.[0-9]+(\.[0-9]+)?' | head -n1)" [ -f "$KIT_DIR/spec.yaml" ] && ok "kit spec.yaml present" || { bad "spec.yaml missing"; exit 1; } diff --git a/integrations/isolation/acq-kits/usai-provider/scripts/verify b/integrations/isolation/acq-kits/usai-provider/scripts/verify index b8564ff1..26da1986 100755 --- a/integrations/isolation/acq-kits/usai-provider/scripts/verify +++ b/integrations/isolation/acq-kits/usai-provider/scripts/verify @@ -59,8 +59,9 @@ in_sbx() { sbx exec "$SBX_NAME" -- sh -c "$1" /dev/null || true; } info "0. Preconditions" if ! command -v sbx >/dev/null 2>&1; then - echo " sbx not on PATH. Install sbx >= 0.34.0 and retry." >&2 - exit 1 + unver "sbx not on PATH — install sbx >= 0.34.0 and retry" + verify_verdict "Summary" + exit $? fi echo " sbx version: $(sbx version 2>/dev/null | grep -oE 'v?[0-9]+\.[0-9]+(\.[0-9]+)?' | head -n1)" [ -f "$KIT_DIR/spec.yaml" ] && ok "kit spec.yaml present" || { bad "spec.yaml missing"; exit 1; } diff --git a/scripts/tests/test_verify_contract.py b/scripts/tests/test_verify_contract.py index 04d3c340..b7626a2b 100644 --- a/scripts/tests/test_verify_contract.py +++ b/scripts/tests/test_verify_contract.py @@ -21,6 +21,7 @@ import re import shlex +import shutil import subprocess from pathlib import Path @@ -60,6 +61,9 @@ # The old two-state verdict. Its absence is the thing being enforced. LEGACY_VERDICT = re.compile(r'\[\s*"\$fail"\s+-eq\s+0\s*\]') +SHARED_REPORTER_SOURCE = re.compile( + r'^\s*\.\s+"\$KIT_DIR/\.\./verify-report\.sh"\s*(?:#.*)?$', re.MULTILINE +) def _verify_scripts() -> dict[str, Path]: @@ -154,15 +158,44 @@ def test_unconverted_allowlist_is_accurate(): @pytest.mark.parametrize("kit", sorted(_converted())) def test_converted_kit_sources_the_shared_library(kit: str): text = _converted()[kit].read_text(encoding="utf-8") - commands = [line.strip() for line in text.splitlines() if line.strip() and not line.lstrip().startswith("#")] - source = any(re.match(r"^\.\s+", line) and "verify-report.sh" in line for line in commands) reset = re.search(r"^\s*verify_reset\s*(?:#.*)?$", text, re.MULTILINE) - assert source, f"{kit}/scripts/verify does not source verify-report.sh" + assert SHARED_REPORTER_SOURCE.search(text), ( + f'{kit}/scripts/verify does not source "$KIT_DIR/../verify-report.sh"' + ) assert reset, ( f"{kit}/scripts/verify sources the library but never calls verify_reset, so its counters are unset" ) +def test_shared_library_source_must_be_the_command_operand(): + text = '. other.sh # verify-report.sh\n' + assert not SHARED_REPORTER_SOURCE.search(text) + + +@pytest.mark.parametrize("kit", sorted(_converted())) +def test_missing_sbx_is_unverified(kit: str, tmp_path: Path): + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + for command in ("dirname", "mktemp", "rm"): + executable = shutil.which(command) + assert executable, f"test prerequisite missing: {command}" + (bin_dir / command).symlink_to(executable) + + bash = shutil.which("bash") + assert bash, "test prerequisite missing: bash" + result = subprocess.run( + [bash, str(_converted()[kit])], + env={"PATH": str(bin_dir)}, + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 3, result.stdout + result.stderr + assert "UNVERIFIED" in result.stdout + assert "sbx not on PATH" in result.stdout + + @pytest.mark.parametrize("kit", sorted(_converted())) def test_converted_kit_has_no_legacy_two_state_verdict(kit: str): text = _converted()[kit].read_text(encoding="utf-8") @@ -176,11 +209,10 @@ def test_converted_kit_has_no_legacy_two_state_verdict(kit: str): @pytest.mark.parametrize("kit", sorted(_converted())) def test_converted_kit_does_not_shadow_the_shared_reporters(kit: str): - """A local redefinition silently restores the old behaviour. + """Shared reporter functions must remain the active implementations. - A kit could source the library and then define its own `ok()`/`skip()` that - increments nothing — passing every other test here while behaving exactly as - before. + Local `ok()`/`skip()` definitions bypass the shared coverage counters and + invalidate the verdict contract. """ text = _converted()[kit].read_text(encoding="utf-8") for fn in ("ok", "bad", "unver", "skip", "warn", "info"): @@ -210,10 +242,10 @@ def test_converted_kit_reports_every_gate(kit: str): def test_no_silent_skip_remains_in_converted_kits(): - """A bare `echo " SKIP..."` or `NOTE:` increments no counter. + """Skip and unverified outcomes must update the coverage record. - That is how a skipped check became indistinguishable from a passing one. In a - converted kit these must be `skip`/`unver` calls instead. + Bare `echo " SKIP..."` and `NOTE:` messages bypass the shared counters; + converted kits use `skip` or `unver` instead. """ offenders: list[str] = [] for kit, path in sorted(_converted().items()): From 604b5ba5da79cfae074581b5d25d65d40d7be10a Mon Sep 17 00:00:00 2001 From: wz-gsa Date: Mon, 12 Oct 2026 14:59:49 +0000 Subject: [PATCH 6/7] docs(acq-kits): show the canonical verify-report source path Correct the copyable usage example and format the executable contract tests. Co-authored-by: OpenCode Agent --- integrations/isolation/acq-kits/verify-report.sh | 3 ++- scripts/tests/test_verify_contract.py | 14 ++++---------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/integrations/isolation/acq-kits/verify-report.sh b/integrations/isolation/acq-kits/verify-report.sh index 02118353..cc69b452 100644 --- a/integrations/isolation/acq-kits/verify-report.sh +++ b/integrations/isolation/acq-kits/verify-report.sh @@ -38,7 +38,8 @@ # # USAGE # -# . "$(dirname "$0")/../../lib/verify-report.sh" +# KIT_DIR="$(cd "$(dirname "$0")/.." && pwd)" +# . "$KIT_DIR/../verify-report.sh" # verify_reset # initialize the counters # info "1. Something" # ok "it worked" diff --git a/scripts/tests/test_verify_contract.py b/scripts/tests/test_verify_contract.py index b7626a2b..a2bdeabc 100644 --- a/scripts/tests/test_verify_contract.py +++ b/scripts/tests/test_verify_contract.py @@ -61,9 +61,7 @@ # The old two-state verdict. Its absence is the thing being enforced. LEGACY_VERDICT = re.compile(r'\[\s*"\$fail"\s+-eq\s+0\s*\]') -SHARED_REPORTER_SOURCE = re.compile( - r'^\s*\.\s+"\$KIT_DIR/\.\./verify-report\.sh"\s*(?:#.*)?$', re.MULTILINE -) +SHARED_REPORTER_SOURCE = re.compile(r'^\s*\.\s+"\$KIT_DIR/\.\./verify-report\.sh"\s*(?:#.*)?$', re.MULTILINE) def _verify_scripts() -> dict[str, Path]: @@ -159,16 +157,12 @@ def test_unconverted_allowlist_is_accurate(): def test_converted_kit_sources_the_shared_library(kit: str): text = _converted()[kit].read_text(encoding="utf-8") reset = re.search(r"^\s*verify_reset\s*(?:#.*)?$", text, re.MULTILINE) - assert SHARED_REPORTER_SOURCE.search(text), ( - f'{kit}/scripts/verify does not source "$KIT_DIR/../verify-report.sh"' - ) - assert reset, ( - f"{kit}/scripts/verify sources the library but never calls verify_reset, so its counters are unset" - ) + assert SHARED_REPORTER_SOURCE.search(text), f'{kit}/scripts/verify does not source "$KIT_DIR/../verify-report.sh"' + assert reset, f"{kit}/scripts/verify sources the library but never calls verify_reset, so its counters are unset" def test_shared_library_source_must_be_the_command_operand(): - text = '. other.sh # verify-report.sh\n' + text = ". other.sh # verify-report.sh\n" assert not SHARED_REPORTER_SOURCE.search(text) From 2dc0af4e739ba67a66ac5f18ed463a5150dfcd18 Mon Sep 17 00:00:00 2001 From: wz-gsa Date: Wed, 14 Oct 2026 16:33:54 +0000 Subject: [PATCH 7/7] docs(acq-kits): scope the verdict contract to converted kits The exit-code table described a contract only two kits implement, so a caller could trust a 0 from an unconverted kit whose 0 does not distinguish every-check-passed from a check that could not run. Document the staged split explicitly, state what an unconverted 0 does and does not establish, and derive both lists from the test suite so a README-only edit, a conversion, or a reversion each fail. Co-authored-by: OpenCode Agent --- integrations/isolation/acq-kits/README.md | 30 ++++++++++++---- scripts/tests/test_verify_contract.py | 44 +++++++++++++++++++++++ 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/integrations/isolation/acq-kits/README.md b/integrations/isolation/acq-kits/README.md index a92854bd..129e93ec 100644 --- a/integrations/isolation/acq-kits/README.md +++ b/integrations/isolation/acq-kits/README.md @@ -84,9 +84,14 @@ make test-kits ### What a `scripts/verify` exit code means -Verify scripts share one verdict contract, `verify-report.sh` in this directory. -A verdict is a function of a coverage record, not a failure counter, so a run -that exercised nothing cannot report the same result as a run that passed. +The shared verdict contract lives in `verify-report.sh` in this directory, and +**adoption is staged**: only the kits listed as converted below implement it +today. A verdict is a function of a coverage record, not a failure counter, so a +converted run that exercised nothing cannot report the same result as a run that +passed. + +**Converted kits** — `git-ssh-sign`, `usai-provider`. For these, and only these, +the exit codes and reporters below apply. | Exit | Meaning | |---|---| @@ -103,6 +108,19 @@ degrades the verdict to `3`; `skip` is *not applicable in this configuration* (e.g. a backend-specific check on the other backend) and does **not** degrade it; `warn` is advisory. -`scripts/tests/test_verify_contract.py` executes the shared verdict contract and -statically checks converted scripts. Its `UNCONVERTED` set lists kits not yet -migrated; it must reach empty. +**Unconverted kits** — `agentic-coding-playbook`, `oci-engine`, `opencode`, +`openchamber`, `paseo`, `pi-coding-agent`, `zscaler-ca-certificate`. These still +use the older two-state verdict, so nothing above applies to them. A non-zero +exit means a check failed *or* a precondition could not be met — the two are not +distinguished — and a `0` does **not** establish that every check ran: these +scripts report an unmet precondition with a bare message that increments no +counter, then exit `0` on the strength of the checks that did run. Treat a `0` +here as "nothing that ran failed", never as full coverage. + +The authoritative classification is `scripts/tests/test_verify_contract.py`. It +executes the verdict contract for converted kits, asserts each unconverted kit +still matches its description, and compares both lists above against its own, so +the split cannot rot in either direction: converting a kit without moving it +fails, a converted kit reverting fails, and a README edit alone fails. The +unconverted list must reach empty, at which point this carve-out goes away and +the contract above applies to every kit. diff --git a/scripts/tests/test_verify_contract.py b/scripts/tests/test_verify_contract.py index a2bdeabc..80782678 100644 --- a/scripts/tests/test_verify_contract.py +++ b/scripts/tests/test_verify_contract.py @@ -249,3 +249,47 @@ def test_no_silent_skip_remains_in_converted_kits(): assert not offenders, ( "converted kits still report skips via bare echo, which increments no counter:\n " + "\n ".join(offenders) ) + + +def _readme_classification(label: str, *, required: bool) -> set[str]: + """Kits the acq-kits README lists for one side of the staged migration. + + Scans the whole paragraph after the heading, not just its first sentence, so + a later claim in the same paragraph cannot contradict the list unnoticed. + + `required=False` treats a missing section as the empty set. That is the + documented end state for the unconverted side: when migration completes the + README drops that carve-out, and an absent section then means "no kits", + not "the README is malformed". + """ + text = (ACQ_KITS / "README.md").read_text(encoding="utf-8") + match = re.search(rf"\*\*{label} kits\*\* — (.*?)(?:\n\n|\Z)", text, re.S) + if match is None: + assert not required, f"acq-kits/README.md has no '**{label} kits** — ...' paragraph" + return set() + # Kit names are the backticked tokens that name a real verify script; other + # backticked tokens in the paragraph (exit codes, file names) are prose. + scripts = _verify_scripts() + return {token for token in re.findall(r"`([^`]+)`", match.group(1)) if token in scripts} + + +@pytest.mark.parametrize( + ("label", "expected", "required"), + [("Converted", CONVERTED, True), ("Unconverted", UNCONVERTED, False)], +) +def test_readme_classification_matches_this_suite(label: str, expected: set[str], required: bool): + """The README's per-kit split must match the authoritative classification. + + The README documents exit codes `0`/`1`/`3` as the verdict contract, and + that contract only holds for converted kits. A README naming the wrong kits + tells a caller to trust a `0` from a kit whose `0` does not distinguish + "every check passed" from "a check could not run". Deriving both lists from + the README paragraph fails a README-only edit and a suite-only edit alike, + so the two cannot drift as kits migrate. + """ + documented = _readme_classification(label, required=required) + assert documented == expected, ( + f"acq-kits/README.md lists {label.lower()} kits {sorted(documented)} but this suite " + f"classifies {sorted(expected)}. Update the README section when a kit migrates: " + f"README-only={sorted(documented - expected)}, suite-only={sorted(expected - documented)}" + )