Skip to content

Name a reason instead of crashing, and stop the verdict depending on the host - #2

Open
GiulioDER wants to merge 2 commits into
egnaro9:mainfrom
GiulioDER:pr2/verifier-robustness
Open

Name a reason instead of crashing, and stop the verdict depending on the host#2
GiulioDER wants to merge 2 commits into
egnaro9:mainfrom
GiulioDER:pr2/verifier-robustness

Conversation

@GiulioDER

@GiulioDER GiulioDER commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Rebased onto main now that #1 is in. Single commit, no longer stacked.

Thank you for the revert note. You were right, and the reason is worse than a flaky test: my two deep-nesting tests built their fixture with json.loads('{"a":' * 3000), which raises inside the decoder at a depth that varies by interpreter. Green on my 3.14, red on your 3.11 leg. So the tests for a host-dependent defect were themselves host-dependent, and the window they probed had an interpreter-dependent width.

You offered two ways back. This takes the first one.

Both traversals over issuer data are now iterative

_todo_failures walks the whole manifest and _summary_outruns walks results.summary. Both were recursive; both are now explicit stacks. There is no depth window left to be host-dependent about, so there is nothing host-dependent left to test.

Order is preserved exactly: children are pushed in reverse so popping yields them in the original order, and a popped node's children are handled before its siblings, which is the depth-first order the recursive form gave. Your committed failure lists pin that, and they stay green.

The replacement tests build a 20000-deep structure in Python rather than through the decoder, so they exercise the traversal on every interpreter. The backstop's append-not-replace property is now tested by injecting a RecursionError rather than by inducing one.

The RecursionError backstop stays as defence in depth for traversals that are not iterative yet. It appends rather than replaces, because returning a fresh list would let an issuer delete every other reason, a smuggled unlisted-file included, just by padding a structure with depth.

Conflict resolutions worth naming

Crashkit severity: yours wins. You now refuse out-of-table labels instead of weighing them 0, which closes the hole properly rather than at the arithmetic. I kept your version and added one thing: x in {...} and a set comprehension both raise on a list, so an unhashable severity escaped as a TypeError rather than as your named refusal. Hashability is tested first, and your message shape, ordering key and repr of the original value are unchanged, so both of your pinning tests stay green.

verify_bundle: yours wins. The draft short-circuit stays ahead of the backstop, byte-identical in behaviour across four draft shapes including a real 21-reason draft.

Crashes that escaped verify_bundle

Every input is issuer-controlled JSON, so any assumption about a field's type was an assumption about the issuer's goodwill.

  • non-dict elements in certlab verdicts, fleet rows, or a raw jsonl line
  • a scalar evidence or results.checks, since 5 or [] is 5
  • an unhashable value reaching a dict key or set element. dict.get() raises on an unhashable key rather than returning its default. The guard is hashability, not a string requirement: SPEC 3.2 and 3.3 type none of suite, member or operator_id, so a board keyed by integer ids is legal evidence, and two control tests hold that door open.
  • a non-string modeldrift t, previously coerced to "" without a reason
  • a non-list or mixed-type fails
  • a deeply nested manifest, and a top-level JSON array artifact

The host-dependent verdict, which you called the most serious thing

read_text and write_text used the platform codec, so a manifest carrying a valid UTF-8 byte 0x81 failed as invalid-json on Windows and passed under PYTHONUTF8=1. Same bytes, opposite answers.

Three things that turned up while chasing it, none of which I expected:

  1. The verifier's own verdict line carries an em dash and was printed raw, so on an ascii or cp437 console the CLI died after the FAIL reasons and before the verdict, which is the one line a reader needs. cp1252 masks it because cp1252 contains U+2014.
  2. vac/registry.py re-reads the same vac.json with the locale codec. Once the verifier stopped refusing those bundles, a bundle that now PASSES reached registry.py:156 and crashed there, where before this branch it was refused with a named reason. That is a regression this branch would have introduced into the adjacent module, so all five sites there are pinned too. The structural guard now scans vac/*.py rather than verify.py alone.
  3. tests/test_refusals_modeldrift_b.py reads RESULTS.md the same way, so it fails on a cp1252 host on main, before this branch touches anything. Pinned, with newline="" on the writes because those artifacts are sha256-pinned. That is the only file of yours this PR edits, and it is six I/O calls, encoding only, no assertion changed.

There are roughly 120 more bare text I/O sites across tests/. They are latent rather than red, so I left them alone rather than swamp this diff. Happy to send that list separately.

Also here

An unbounded scan on an issuer-controlled field. re.sub(r"<[^>]+>", "", narr["html"]) rescans to end-of-string from every start position when no > follows. A bundle that verifies completely clean cost 22.4s for 128k characters. The replacement is linear and reproduces the regex exactly, verified over 200,000 random strings from a <>-dense alphabet plus the edge cases, because SPEC 3.5 pins the committed narrative against this transform and redefining "tag-stripped" would flip verdicts on honest bundles. Same input now takes 0.004s.

Issuer text reaching the terminal. The replay block is echoed after the verdict line and was printed raw, so ANSI escapes could paint a forged PASS over a failing run. Exit codes were always honest, so a CI gate keyed on exit status was never fooled. Bidi overrides and the Unicode separators are escaped too, since U+202E reverses the displayed order of a replay command a reader is invited to copy. The test builds those with chr() rather than as literals, so the test file does not itself carry invisible reordering marks.

Verification

On this host, imports pinned to each tree so neither borrows the other's vac:

main this branch
pytest 2 failed, 236 passed 0 failed, 266 passed, 16 skipped, 4 xfailed

python -m vac.verify fixtures/valid exits 0, every tamper-* fixture still exits 1, the fixtures regenerate byte-identically, and ruff output is unchanged.

Every test was verified to fail against origin/main rather than merely to pass after it. The one exception is the _printable unit test, whose subject does not exist upstream, so it has no upstream red state; the CLI-level test covering the same defect does.

For refusal-coverage: the four refusals this branch adds are each pinned by their exact reason text, and I confirmed each one by mutating the f.append away and watching a test catch it. An assertion that only checks the failure list is non-empty survives that mutation through collateral damage, which is why they assert the full string.

One thing to pre-empt: test_the_narrative_strip_is_not_quadratic asserts wall-clock under 4.0s and runs in 0.05s, an 80x margin. Given the revert reason I would rather flag it than have you find it. If you would prefer no timing assertion at all, say so and I will swap it for an equivalence check plus an operation-count bound.

Known residuals, deliberately out of scope

  • The per-point continue in the modeldrift coherence loop suppresses the remaining reasons for that one point. It cannot widen acceptance, since any appended failure aborts the check, but the report is less complete than the docstring promises.
  • A RecursionError raised inside _coherence still loses that function's own partial reasons, because the result is only added on return. The phases before it survive.
  • no verdicts[] array and no rows[] array now also fire when the array exists but holds a non-dict element, so the reason slightly misdescribes that input. I kept your wording verbatim rather than break the tests that pin it. Say the word and I will split it into two reasons.

@GiulioDER
GiulioDER force-pushed the pr2/verifier-robustness branch 2 times, most recently from b810874 to 858121c Compare August 15, 2026 09:33
@egnaro9

egnaro9 commented Aug 15, 2026

Copy link
Copy Markdown
Owner

#1 is merged, so this one is now conflicting exactly as you called it — it carries that commit too.

I want this half as much as the first. The three that matter most to me, in order:

  • The verdict that depended on your host. Same bytes, same code, opposite verdicts depending on the platform codec. For a protocol whose entire premise is that a stranger gets the same answer offline, that is the most serious thing in either PR — a false PASS is a bug, but a verdict that varies by locale means the artifact was never the thing being checked. The structural test that reads the source and asserts no text read leaves its encoding to the host is the right shape, precisely because the behavioural one is green on this repo's own CI and would have protected nothing.
  • The recursion backstop appending rather than replacing. An earlier version handing the issuer a switch that deletes every other named reason — including a smuggled unlisted-file — while still exiting 1 is the subtlest finding here, and it is a bug in a fix. That is the failure mode I most need tests for.
  • ANSI and bidi in the replay block. The exit code stayed honest, so CI was never fooled, but the block exists to be read and copy-pasted by a human. U+202E reversing a command someone is invited to paste is a real attack on the one part of the output that asks for trust.

Rebase when you have time and I'll merge it. If you would rather hand it off, say the word and I'll rebase it myself with your authorship intact — it is your work either way.

On the residuals you named: vac/registry.py has the same two defect classes and is the CLI that consumes this verifier, so it should get the same treatment; I'll open an issue rather than let it sit in a PR comment.

…depending on the host

Rebased onto main after egnaro9#1, and reworked to address the revert: the two
tests that induced a RecursionError built their fixture with
json.loads('{"a":' * 3000), which raises inside the DECODER at a depth that
varies by interpreter. They were green on 3.14 and red on the 3.11 CI leg,
so the tests for a host-dependent defect were themselves host-dependent.

Rather than move the fixture, both issuer-data traversals are now
ITERATIVE, so there is no depth window left to be host-dependent about:

- _todo_failures walks the whole manifest
- _summary_outruns walks results.summary

Order is preserved exactly (children pushed in reverse, popped in original
order, depth-first as before), which the committed failure lists pin. The
replacement tests build a 20000-deep structure in Python rather than through
the decoder, so they exercise the traversal on every interpreter, and the
backstop's append-not-replace property is now tested by INJECTING a
RecursionError rather than by inducing one.

The RecursionError backstop stays as defence in depth for traversals that
are not iterative yet. It APPENDS: returning a fresh list would let an
issuer delete every other reason, a smuggled unlisted-file included, just by
padding a structure with depth.

Crashes that escaped verify_bundle, each now a named reason:

- non-dict elements in certlab verdicts, fleet rows, or a raw jsonl line
- a scalar evidence / results.checks, since `5 or []` is 5
- an unhashable value reaching a dict key or set element. dict.get() RAISES
  on an unhashable key rather than returning its default. The guard is
  hashability, NOT a string requirement: SPEC 3.2 and 3.3 type none of
  suite/member/operator_id, so integer ids are legal evidence and two
  control tests hold that door open.
- a non-string modeldrift `t`, previously coerced to "" without a reason
- a non-list or mixed-type `fails`
- a deeply nested manifest, and a top-level JSON array artifact

Conflict resolutions worth naming:

- crashkit severity: main's refusal of out-of-table labels supersedes the
  weigh-as-zero approach this branch carried, so main's version is kept.
  One addition: `x in {...}` and a set comprehension both RAISE on a list,
  so an unhashable severity escaped as a TypeError rather than as that
  named refusal. Hashability is now tested first.
- verify_bundle: main's draft short-circuit is kept ahead of the backstop.

An unbounded scan on an issuer-controlled field:

- `re.sub(r"<[^>]+>", "", narr["html"])` rescans to end-of-string from every
  start position when no '>' follows. A CLEAN-verifying bundle cost 22.4s
  for 128k characters. The replacement is linear and reproduces the regex
  EXACTLY, verified over 200000 random strings, because SPEC 3.5 pins the
  committed narrative against this transform. Same input now takes 0.004s.

Issuer text reaching the terminal, and the host-dependent verdict:

- the replay block is echoed after the verdict line and was printed raw, so
  ANSI escapes could paint a forged PASS over a failing run. Escaping the
  control ranges is not sufficient on its own: it does not make text
  ENCODABLE, and the verifier's own verdict line carries an em dash, so on
  an ascii stdout the CLI died after the FAIL reasons and before the
  verdict. Bidi overrides and the Unicode separators are escaped too.
- read_text/write_text used the platform codec, so a manifest carrying a
  valid UTF-8 byte 0x81 failed as invalid-json on Windows and passed under
  PYTHONUTF8=1. Same bytes, opposite verdicts.
- tests/test_refusals_modeldrift_b.py read RESULTS.md the same way, so it
  failed on a cp1252 host before this branch touched anything. Pinned, with
  newline="" on the writes because those artifacts are sha256-pinned.

On this host main is 2 failed / 236 passed; this branch is 0 failed /
262 passed. Every test was verified to fail against origin/main rather than
merely to pass after it, except the bidi unit test, whose subject does not
exist upstream.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: giulio d'erme <giulio@derme.dev>
@GiulioDER
GiulioDER force-pushed the pr2/verifier-robustness branch from 858121c to bb4dda3 Compare August 16, 2026 11:14
@egnaro9

egnaro9 commented Aug 16, 2026

Copy link
Copy Markdown
Owner

Two updates you should probably know about, and one question.

First, your encoding finding sent me looking for the same class everywhere
else. I found four more, all in code your PR had already hardened. The cheapest
one is a single capital letter: re-casing critical to Critical on the
failing rows only drops their weight out of the numerator, and a security score
divides to a clean, false 0.0 through ordinary arithmetic. My own spec had
written that hole down as a decision.

Second, I stopped hand-hunting and measured. I disabled each of the verifier's
named refusals one at a time and asked whether anything noticed. 75 of 112 could
be deleted with the whole suite and all 16 tamper fixtures still green. The CI
job I wrote specifically to prove the verifier can refuse caught zero of them.
Fixing the four defects we found, each with its own regression fixture, moved
that number by -0.002. Testing every refusal moved it to 1.000.

Your PR #2 is rebased and verified locally but not landed: two of its new tests
build fixtures with json.loads('{"a":' * 3000), which RecursionErrors inside
the decoder on 3.11 before the verifier is called. Green on 3.14, red on the CI
leg. The tests need the manifest to parse while a later traversal blows the
stack, and that gap's width is interpreter-dependent, which is the same
host-dependence you were pointing at. I did not want to quietly rewrite your
tests, so it is sitting there.

The question. The registry has 11 entries and all 11 are mine. That makes it one
fixture, not a protocol, and it is the weakest thing about the whole project.
Would you be interested in issuing a bundle from cca-audit? Not as a favor to
make the registry look bigger. As the first real test of whether the spec is
writable-against by someone who did not write it. I would expect you to hit
something ambiguous, and that would be the useful part.

One thing changed since I last wrote, and it is the reason the ask is
reasonable now rather than presumptuous. Every profile in the spec used to
hard-code one of my own artifact shapes, so an outsider literally could not
issue: I walked the path with a fake repo and a plausible results file, filled
in every field honestly, and it died on "no per_kind object". There is now a
generic profile where you supply the rows and declare how each number is
recomputed from them, and a walkthrough with a worked example from a fictional
outside issuer. So cca-audit would not have to look like my repos.

Entirely optional and on no timeline. If you would rather just rebase #2 when
you are up to it, that is worth more than a bundle.

test_the_narrative_strip_is_not_quadratic asserted `time.monotonic()` under
4.0s. It ran in 0.05s, an 80x margin, but the margin was never the point: a
verdict about this patch depended on how loaded the host was, which is the
same class of defect the patch exists to remove. I flagged it in the PR body
and offered to swap it. This is the swap, made before your review rather than
after it, so nothing host-dependent is left in the branch to object to.

Two deterministic tests replace it.

EQUIVALENCE. _strip_tags is asserted equal to `re.sub(r"<[^>]+>", "", s)`
over 20,022 strings from a `<>`-dense alphabet under a fixed seed, plus the
edge cases that decide the semantics: "<>" is not a match, and a "<" with no
later ">" stays literal. SPEC 3.5 pins the committed narrative against this
transform, so a divergence would flip verdicts on honest bundles rather than
merely change speed. The 200,000-string check quoted in the PR body was run
ad hoc and never shipped; this ships a repeatable version of it. It has no
upstream red state, since its subject does not exist on main.

MECHANISM. The pathological bundle is unchanged (narrative.html of 120,000
"<"), but instead of timing the run it records every pattern handed to the
engine and asserts that none contains `[^>]`. Both routes are recorded,
re.sub(pattern, ...) and re.compile(pattern).sub(...), because recording only
the first would let the quadratic form come back through the second unseen.
A vacuity guard asserts the recorder saw something, since a recorder that
observes nothing passes any such assertion.

That is the operation-count bound I offered, in the only honest form
available: the quadratic work happens inside the C engine, where no
Python-level counter can observe it, so counting Python operations would
assert nothing about the path that was slow. I would rather ship a bound that
states what it proves than one that looks stronger and proves less.

Verified red upstream, which the timing test could only claim: ported onto
egnaro9/vac-protocol main, this test fails with `assert ['<[^>]+>'] == []`,
and takes 23.6s there against 1.4s here.

Suite on this branch: 267 passed, 16 skipped, 4 xfailed, 0 failed. ruff
unchanged.

Signed-off-by: giulio d'erme <giulio@derme.dev>
egnaro9 pushed a commit that referenced this pull request Aug 18, 2026
Not a verifier defect. A distribution one, and it lands on exactly the
person this project is aimed at: the stranger who clones and replays.

REPRODUCTION, on a clean machine with the Git for Windows installer defaults:

    git clone https://github.com/egnaro9/vac-protocol.git
    cd vac-protocol
    python -m vac.verify fixtures/valid    # exit 1

Twenty reasons, all of this shape:

    FAIL sha256-mismatch: evidence/bundle.json: manifest 64931fe8..., file
    a2433da2...

core.autocrlf is ON by default in that installer, the repository carries no
.gitattributes, so checkout rewrites every text artifact to CRLF and every
sha256 in the manifest stops matching its file. The bundle is honest, the
manifest is honest, the verifier is right to refuse what it was handed: the
bytes really did change between your commit and my disk. CI is ubuntu-only,
so nothing upstream can see this, and I hit it myself as the first thing that
happened when I re-cloned to work on #2.

It is the same class as the encoding finding in #2, one layer out. There the
same bytes produced opposite verdicts depending on the host codec; here the
same commit produces different BYTES depending on the host's git config.

Two parts, because a fresh clone and an existing one need different things.

`* -text` in .gitattributes turns the conversion off for every future clone.
Verified end to end: cloning this branch with core.autocrlf=true yields LF
artifacts and `python -m vac.verify fixtures/valid` exits 0, where the same
clone of main exits 1.

tests/test_checkout_integrity.py is the named reason for the clones that
already happened. A developer whose checkout is already converted currently
gets twenty hash mismatches and no cause; this gives one failure that says
what happened and the two commands that repair it. It also pins the
attribute, so dropping it later fails loudly rather than silently, on the
same principle as the rest of the suite: a gate that cannot name why it fired
cannot be audited.

The test file is new and touches nothing else, so this stands alone: it is
independent of #2 and #3 and can land in any order.

Verified: the three new tests pass here, the first fails against main (no
.gitattributes), ruff clean.

Signed-off-by: giulio d'erme <giulio@derme.dev>
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