Fix the LOW severity audit findings, and reject the four that were never QUIZZ's - #37
Merged
Conversation
Twenty one low severity findings. Sixteen fixed, five rejected: one because the earlier pass already fixed it, three because the file they describe belongs to QUARANTINEZ rather than here, one because the file is generated. 2162 of ABMI's rows land in a November vintage, but ten of those are each observation's first published value, not a change. The README now says 2152 and the test excludes an observation's min(vintage) the way golden.py already does. The same paragraph's rebasing sentence called six of ABMI's nine rebasings "almost all", so it now says six, and a new test in test_corpus.py recomputes both the count and the November split straight from SOURCE.json. The cassette cost was 1.14 euros on the README and 1.16 EUR in ADR 0001 for the same 224-exchange recording. Computed from Cassettes.cost_tokens and the prices in scripts/record_cassettes.py, it is 1.16. Both documents are now checked against that one number. "Six records under docs/adr/" had no producer either; it now counts docs/adr/*.md and the table rows beneath it. IHYQ's "45 previous versions and 43 distinct dates" is stated in four places: asof.py, corpus.py, capture_vintages_ons.py and ADR 0001. asof.py and the capture script had both dropped the word "previous", making 45 wrong by one against the corpus's 46 versions. One test now reads all four and recomputes both numbers from the observations table. asof.period_key was a third implementation of the ordering barrier.period_rank and notes.period_rank already do, called from nowhere in src, tests, examples, scripts or docs. Deleted rather than kept as a landmine for whichever import gets typed next. corpus.py's docstring and its insert-loop comment both called IHYQ 2018-Q4's published blank "the only" one; ABMI carries the identical gap at the identical version and asof.py already said so correctly. Both now name both series, and a test counts the two null rows directly. Two of the given fixes were wrong for the tree as it stands today, not just as worded. test_recorded.py's docstring described two colliding renderings from before the golden set dropped four questions; today's 52 questions times 4 variants render to 208 requests with zero collisions against a 224-exchange cassette holding sixteen leftovers. Asserting exchanges == pairs, as suggested, would have failed on that leftover count, so the test now asserts the 208 keys are distinct instead, which I proved catches a real collision by forcing two variants to render identically and watching 52 of 208 pairs collapse. Plan.has_filter was asserted True on both of search_sql's own plans and never asserted False anywhere, so a hardcoded True passes the whole services suite. The suggested negative case, running search_sql on the owner connection, does not work: the owner's plan still shows a residual Filter line because the covering index is built per knowing-time and the bound parameter is checked against it regardless of role. The real contrast is row level security itself, so the new test runs a query with no predicate of its own on the owner connection, where nothing is left to filter, against the same query under the retriever role, where the policy still injects one. agent.py's two docstrings called the LangGraph two nodes, choose and act, three. durable.connect's docstring credited WAL specifically with surviving SIGKILL, which the suite cannot actually show: a committed transaction under the default rollback journal survives the same SIGKILL the tests send. Softened to describe what the pragmas are for and pinned the two settings directly instead. capture_vintages_ons.py's canonical_period returned a bare four digit year unchanged, which sorts before its own month and quarter forms and crashes both period_rank parsers; no SERIES entry reads the ONS "years" block today, so nothing has hit it yet. The branch is gone and a new test file proves a bare year is now refused like anything else unrecognised. CHANGELOG.md's only release entry still describes the Philadelphia Fed extract ADR 0001 says was never licensed for redistribution, and the ONS swap that replaced it three days later was never written down. Added an entry recording why, and flagged that the v0.1.0 GitHub release still points at the withdrawn tree and needs a person to replace or delete it; that is not something a documentation commit can do on its own. Rejected: AS_OF_CONTRACT.md's pointer to a HOLDOUT_FROM constant was already fixed, pointing at barrier.WINDOWS, by the prior pass. Three findings named files this repository does not have: examples/venue_session.py, an ADR called "a fixture not a generator", and src/quarantinez/breaker.py all belong to QUARANTINEZ. test_doc_contract.py's missing negative control for _resolves is real but the file is generated from the shared toolset manifest and a hand edit to it is erased on the next run, so it is recorded rather than touched. 203 tests pass, eighteen of them needing the compose services. The Pages card was regenerated from the new evidence with pagesgen rather than hand edited.
ruff's 100 column limit and the checklist's ruff/mypy gates were not run before the previous commit. Three assertion messages in the new tests ran past 100 columns and one comprehension iterated source()["series"] without first narrowing it from object to list, which is the same isinstance guard test_every_series_holds_the_number_of_rows_its_provenance_records already uses for the same call.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
The adversarial four lens audit's LOW severity pass on QUIZZ. Sixteen of twenty one findings were real and are fixed here, each with a new or extended test that fails against the old code (proved by mutating the fix away and watching the suite go red, or in two cases by proving the finding's own suggested assertion was itself wrong against this tree and writing a correct one instead). Five are rejected: one was already fixed by the prior MEDIUM/HIGH pass, three describe files that exist in QUARANTINEZ rather than QUIZZ, and one names a real gap in a file this repository does not hand edit because an external generator owns it.
Checklist
uv sync --devuv run ruff check .uv run ruff format --check .uv run mypyuv run pytest -quv run pytest -m services -qCHANGELOG.mdunder the unreleased heading.What I checked that a reviewer cannot see from the diff
Verified every finding against the current tree rather than trusting its text, since the prior MEDIUM/HIGH pass had already moved several numbers and the golden set size (56 to 52 questions).
Two of the given fixes were wrong for the tree as it stands, not just as worded, and I deviated from them deliberately:
test_recorded.py's docstring described two colliding renderings from before the golden set dropped four questions. The suggested fix'sassert len(store.exchanges) == len(golden.build(db)) * len(golden.VARIANTS)would assert224 == 208and fail immediately, because the cassette still holds sixteen recordings for the four removed questions. I wrote the test that is actually true today (208 distinct keys, zero collisions) and proved it catches a real collision by monkeypatchinggolden.renderso variant 1 renders like variant 0, watching 52 of 208 pairs collapse to the same key with zero replay misses.Plan.has_filter's suggested negative case, runningsearch_sqlon the owner connection, does not producehas_filter == False: I ran it against the livedocker composedatabase and the owner's plan still shows a residualFilter:line, because the covering index is built per knowing-time and the bound parameter is checked against it regardless of role. I found the real negative case empirically (a query with no predicate of its own, on the owner connection, which bypasses row level security entirely) and proved the new test catches a hardcodedreturn Truewhere the old suite did not.Ran the full suite against real Redis and PostgreSQL containers (
docker compose up -d,uv run pytest -m services), not just the default 185, for every retrieval and durability change: 203 tests pass, 18 of them needing the containers. Reproduced thecanonical_periodbare year crash againstbarrier.period_rankandnotes.period_rankdirectly in a REPL before fixing it. Confirmedexamples/venue_session.py,docs/adr/0005-a-fixture-not-a-generator.mdandsrc/quarantinez/breaker.pyexist in~/Developer/qfolioz/QUARANTINEZand nowhere in this repository before rejecting those three findings. Regenerateddocs/evidence/*viascripts/capture_evidence.pyand the Pages card viapagesgenrather than hand editing either.check_repo.sh QUIZZpasses.