feat(plans): VI-A1 replay-binding harness — fresh bind() vs frozen binding_adjudication - #593
feat(plans): VI-A1 replay-binding harness — fresh bind() vs frozen binding_adjudication#593r-uben wants to merge 3 commits into
Conversation
…nding_adjudication multiset
…) score The D3 fail-closed-marker fallback previously chose the winning cache candidate by how well it reproduced the recorded contradiction -- letting bind() choose its own input. Select by provenance only (the table_binding_adjudicated audit event's engine, matched against each cache entry's own engine field); an ambiguous or missing match marks the row unreplayable and never calls bind() for it.
_provenance_engine_by_table took the first table_binding_adjudicated event when a table had more than one, silently choosing among conflicting engines. Replaced with _provenance_engines_by_table: the distinct engine values across every such event for a table_id are collected, and two or more distinct engines makes the row unreplayable (bind() never called) before the cache side is even consulted -- one level above the cache-candidate collision this module already guards against.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
r-uben
left a comment
r-uben
left a comment
There was a problem hiding this comment.
Verdict: MERGE
VI-A1 harness does what the ticket needs for A2's regression oracle. Measured at real tables/binding.py:bind() via current-tree prepare_table_witnesses + native words on the frozen PDF. Multiset compare is (kind, native_token, model_token) with counts — not status echo. Provenance-only D3 fallback (commits 2–3) closes the bind()-picks-its-input hole at both cache collision and conflicting table_binding_adjudicated engines; unreplayable rows never call bind() (spies on p3/p4). Sidecar bytes unchanged. Hermetic PATH strip. orchestrator.py untouched. Corpus 7/7 exact match on unchanged tree is in the log as required.
to_record really omits native_bbox (adjudication.py:83-90) — crop/label scoring correctly stays n/a until A1b's --labels.
Soft (non-blocking)
- Done-when wording vs test: ticket asks a hermetic fixture that perturbs the native words and asserts the exact delta.
test_perturbed_recorded_items_report_exact_deltaperturbs the recorded items instead (dual of a binder change). Still pins delta reporting; not a silent-pass hole. Prefer also mutating native words (or say so in TICKETS.md) before A2 leans on this line. - Ticket Files: read-only import of
_render_adjudication_cropnever lands here — crop path is deferred with A1b. Fine if intentional; don't let A1b assume the import already exists. test_fresh_bind_matches_frozen_record_on_unchanged_treeonly assertsp1-t0.p2-t0is also replayable (marker → provenance cache). Assert all non-unreplayablefixture rows so a p2 regression can't hide behind p1.mainalways exits 0 even onNO/UNREPLAYABLE. Fine for A1 reporting; A2 may want a non-zero gate before treating the CLI as a CI check.- STATUS.md still leads with "Wave 1 dispatch: A1 … ∥ B1 …" under Next action; A1-done note is appended. Same stale-next-action pattern as #590.
- CI: typecheck green on
aac4c2b;test (3.11)still in progress at review time — wait for that head before merge (conflict-no-CI hole still real).
Bot noise (Qodo paused, CodeRabbit trigger) ignored.
Did not merge.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/socr/benchmark/replay_binding.py">
<violation number="1" location="src/socr/benchmark/replay_binding.py:335">
P2: replay_table() documents 'never raises', but open_pdf() can raise on a missing/corrupt in/<slug>.pdf, which then propagates up through replay_page()/replay_corpus() and aborts the entire corpus replay. discover_pages() has the same gap: json.loads(sidecar_path.read_text()) has no try/except, even though _cache_candidate_texts() does guard against OSError/JSONDecodeError. The module's documented contract is per-row failure ('a missing PDF fails per-row, not here'), but nothing actually implements that for replay — one bad artifact silently kills the whole run.</violation>
<violation number="2" location="src/socr/benchmark/replay_binding.py:399">
P3: replay_page() calls replay_table() once per table_id on a page, and replay_table() re-enters prepare_table_witnesses() each time, re-running _locate_boxes and rendering every page crop for the whole page even though only one witness (one block) is selected per call. A multi-table page pays the full locate+witness build once per table. For a per-table benchmark this is avoidable duplicate work across rows on the same page.</violation>
<violation number="3" location="src/socr/benchmark/replay_binding.py:399">
P1: When witness preparation cannot produce a fresh binding, this path still reports a normal comparison. Mark the row unreplayable and suppress added/removed deltas whenever `bind_note` is non-empty.</violation>
</file>
<file name="docs/plans/verifier-independence/STATUS.md">
<violation number="1" location="docs/plans/verifier-independence/STATUS.md:63">
P3: This change marks A1 as DONE but leaves the "## Next action" line above it reading "Wave 1 dispatch: A1 on `feat/vi-A1-replay-binding` ∥ B1 on `feat/vi-B1-stage-timings`.", which now contradicts the status and the appended note saying A1 is done and A1b can dispatch next. A reader acting on Next action would re-dispatch (or get confused about) already-finished A1. Update the Next action line to reflect A1 complete (e.g. dispatch B1 and A1b) instead of re-dispatching A1.</violation>
</file>
<file name="docs/log/2026-09-05_vi-a1-replay-binding.md">
<violation number="1" location="docs/log/2026-09-05_vi-a1-replay-binding.md:71">
P3: The "What changed" bullet says `tests/test_replay_binding.py` (new, 10 tests), but the same document's test plan and Tests sections (and the actual file) record 12 tests. Update the count to 12 so the summary, the rest of the doc, and the shipped test file agree.</violation>
<violation number="2" location="docs/log/2026-09-05_vi-a1-replay-binding.md:72">
P3: The "What changed" bullet describes the fixture as "two sidecars, one cache entry", but the fixture that ships with this PR has four page sidecars (`pages/00001..00004.json`) and five cache entries — the third and fourth pages were added by the revisions documented in the same file. The end-state summary contradicts the shipped fixture; either correct the counts or describe the initial scaffold as the starting state.</violation>
</file>
<file name="tests/fixtures/replay_binding/generate_fixture.py">
<violation number="1" location="tests/fixtures/replay_binding/generate_fixture.py:140">
P3: `_binding_adjudication_for` is annotated `-> dict` but returns a two-tuple `(dict, witness.table_id)`. Callers in `main()` unpack it as `ba1, table_id1 = ...`, so the annotation lies about the actual return shape and would break static type checkers and mislead future edits. Change the return annotation to `tuple[dict, str]`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ) | ||
| continue | ||
|
|
||
| fresh_items, bind_note = replay_table( |
There was a problem hiding this comment.
P1: When witness preparation cannot produce a fresh binding, this path still reports a normal comparison. Mark the row unreplayable and suppress added/removed deltas whenever bind_note is non-empty.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/socr/benchmark/replay_binding.py, line 399:
<comment>When witness preparation cannot produce a fresh binding, this path still reports a normal comparison. Mark the row unreplayable and suppress added/removed deltas whenever `bind_note` is non-empty.</comment>
<file context>
@@ -0,0 +1,490 @@
+ )
+ continue
+
+ fresh_items, bind_note = replay_table(
+ record.pdf_path, record.page_num, candidate_markdown, table_id
+ )
</file context>
| otherwise names why nothing could be bound (no such table_id, table not | ||
| LOCATED this tree, or the page has no native words) — never raises. | ||
| """ | ||
| with open_pdf(pdf_path) as doc: |
There was a problem hiding this comment.
P2: replay_table() documents 'never raises', but open_pdf() can raise on a missing/corrupt in/.pdf, which then propagates up through replay_page()/replay_corpus() and aborts the entire corpus replay. discover_pages() has the same gap: json.loads(sidecar_path.read_text()) has no try/except, even though _cache_candidate_texts() does guard against OSError/JSONDecodeError. The module's documented contract is per-row failure ('a missing PDF fails per-row, not here'), but nothing actually implements that for replay — one bad artifact silently kills the whole run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/socr/benchmark/replay_binding.py, line 335:
<comment>replay_table() documents 'never raises', but open_pdf() can raise on a missing/corrupt in/<slug>.pdf, which then propagates up through replay_page()/replay_corpus() and aborts the entire corpus replay. discover_pages() has the same gap: json.loads(sidecar_path.read_text()) has no try/except, even though _cache_candidate_texts() does guard against OSError/JSONDecodeError. The module's documented contract is per-row failure ('a missing PDF fails per-row, not here'), but nothing actually implements that for replay — one bad artifact silently kills the whole run.</comment>
<file context>
@@ -0,0 +1,490 @@
+ otherwise names why nothing could be bound (no such table_id, table not
+ LOCATED this tree, or the page has no native words) — never raises.
+ """
+ with open_pdf(pdf_path) as doc:
+ words = doc[page_num - 1].get_text("words")
+
</file context>
| ) | ||
| continue | ||
|
|
||
| fresh_items, bind_note = replay_table( |
There was a problem hiding this comment.
P3: replay_page() calls replay_table() once per table_id on a page, and replay_table() re-enters prepare_table_witnesses() each time, re-running _locate_boxes and rendering every page crop for the whole page even though only one witness (one block) is selected per call. A multi-table page pays the full locate+witness build once per table. For a per-table benchmark this is avoidable duplicate work across rows on the same page.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/socr/benchmark/replay_binding.py, line 399:
<comment>replay_page() calls replay_table() once per table_id on a page, and replay_table() re-enters prepare_table_witnesses() each time, re-running _locate_boxes and rendering every page crop for the whole page even though only one witness (one block) is selected per call. A multi-table page pays the full locate+witness build once per table. For a per-table benchmark this is avoidable duplicate work across rows on the same page.</comment>
<file context>
@@ -0,0 +1,490 @@
+ )
+ continue
+
+ fresh_items, bind_note = replay_table(
+ record.pdf_path, record.page_num, candidate_markdown, table_id
+ )
</file context>
| `docs/log/2026-09-05_vi-a1-replay-binding.md`. Corpus replay: 7/7 rows, | ||
| exact multiset match on the unchanged tree (one row needed a D3 | ||
| fail-closed-marker cache fallback, folded into the harness). A1b can | ||
| dispatch once this branch merges. |
There was a problem hiding this comment.
P3: This change marks A1 as DONE but leaves the "## Next action" line above it reading "Wave 1 dispatch: A1 on feat/vi-A1-replay-binding ∥ B1 on feat/vi-B1-stage-timings.", which now contradicts the status and the appended note saying A1 is done and A1b can dispatch next. A reader acting on Next action would re-dispatch (or get confused about) already-finished A1. Update the Next action line to reflect A1 complete (e.g. dispatch B1 and A1b) instead of re-dispatching A1.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/plans/verifier-independence/STATUS.md, line 63:
<comment>This change marks A1 as DONE but leaves the "## Next action" line above it reading "Wave 1 dispatch: A1 on `feat/vi-A1-replay-binding` ∥ B1 on `feat/vi-B1-stage-timings`.", which now contradicts the status and the appended note saying A1 is done and A1b can dispatch next. A reader acting on Next action would re-dispatch (or get confused about) already-finished A1. Update the Next action line to reflect A1 complete (e.g. dispatch B1 and A1b) instead of re-dispatching A1.</comment>
<file context>
@@ -54,3 +54,10 @@ gate; `tests/` is flat. Nothing dispatched.
+`docs/log/2026-09-05_vi-a1-replay-binding.md`. Corpus replay: 7/7 rows,
+exact multiset match on the unchanged tree (one row needed a D3
+fail-closed-marker cache fallback, folded into the harness). A1b can
+dispatch once this branch merges.
</file context>
| (new: `generate_fixture.py`, `corpus/in/doc00.pdf`, two sidecars, one | ||
| cache entry). |
There was a problem hiding this comment.
P3: The "What changed" bullet describes the fixture as "two sidecars, one cache entry", but the fixture that ships with this PR has four page sidecars (pages/00001..00004.json) and five cache entries — the third and fourth pages were added by the revisions documented in the same file. The end-state summary contradicts the shipped fixture; either correct the counts or describe the initial scaffold as the starting state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/log/2026-09-05_vi-a1-replay-binding.md, line 72:
<comment>The "What changed" bullet describes the fixture as "two sidecars, one cache entry", but the fixture that ships with this PR has four page sidecars (`pages/00001..00004.json`) and five cache entries — the third and fourth pages were added by the revisions documented in the same file. The end-state summary contradicts the shipped fixture; either correct the counts or describe the initial scaffold as the starting state.</comment>
<file context>
@@ -0,0 +1,165 @@
+ <corpus-dir>` entry point.
+- `pyproject.toml`: added the `[project.scripts]` entry.
+- `tests/test_replay_binding.py` (new, 10 tests) + `tests/fixtures/replay_binding/`
+ (new: `generate_fixture.py`, `corpus/in/doc00.pdf`, two sidecars, one
+ cache entry).
+
</file context>
| (new: `generate_fixture.py`, `corpus/in/doc00.pdf`, two sidecars, one | |
| cache entry). | |
| (new: `generate_fixture.py`, `corpus/in/doc00.pdf`, four page sidecars, | |
| five cache entries). |
| - `tests/test_replay_binding.py` (new, 10 tests) + `tests/fixtures/replay_binding/` | ||
| (new: `generate_fixture.py`, `corpus/in/doc00.pdf`, two sidecars, one | ||
| cache entry). |
There was a problem hiding this comment.
P3: The "What changed" bullet says tests/test_replay_binding.py (new, 10 tests), but the same document's test plan and Tests sections (and the actual file) record 12 tests. Update the count to 12 so the summary, the rest of the doc, and the shipped test file agree.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/log/2026-09-05_vi-a1-replay-binding.md, line 71:
<comment>The "What changed" bullet says `tests/test_replay_binding.py` (new, 10 tests), but the same document's test plan and Tests sections (and the actual file) record 12 tests. Update the count to 12 so the summary, the rest of the doc, and the shipped test file agree.</comment>
<file context>
@@ -0,0 +1,165 @@
+ preserved) against what the sidecar recorded. `socr-replay-binding
+ <corpus-dir>` entry point.
+- `pyproject.toml`: added the `[project.scripts]` entry.
+- `tests/test_replay_binding.py` (new, 10 tests) + `tests/fixtures/replay_binding/`
+ (new: `generate_fixture.py`, `corpus/in/doc00.pdf`, two sidecars, one
+ cache entry).
</file context>
| - `tests/test_replay_binding.py` (new, 10 tests) + `tests/fixtures/replay_binding/` | |
| (new: `generate_fixture.py`, `corpus/in/doc00.pdf`, two sidecars, one | |
| cache entry). | |
| - `tests/test_replay_binding.py` (new, 12 tests) + `tests/fixtures/replay_binding/` | |
| (new: `generate_fixture.py`, `corpus/in/doc00.pdf`, four page sidecars, | |
| five cache entries). |
| doc.close() | ||
|
|
||
|
|
||
| def _binding_adjudication_for(pdf_path: Path, page_num: int, markdown: str) -> dict: |
There was a problem hiding this comment.
P3: _binding_adjudication_for is annotated -> dict but returns a two-tuple (dict, witness.table_id). Callers in main() unpack it as ba1, table_id1 = ..., so the annotation lies about the actual return shape and would break static type checkers and mislead future edits. Change the return annotation to tuple[dict, str].
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/fixtures/replay_binding/generate_fixture.py, line 140:
<comment>`_binding_adjudication_for` is annotated `-> dict` but returns a two-tuple `(dict, witness.table_id)`. Callers in `main()` unpack it as `ba1, table_id1 = ...`, so the annotation lies about the actual return shape and would break static type checkers and mislead future edits. Change the return annotation to `tuple[dict, str]`.</comment>
<file context>
@@ -0,0 +1,297 @@
+ doc.close()
+
+
+def _binding_adjudication_for(pdf_path: Path, page_num: int, markdown: str) -> dict:
+ """Compute the REAL ``bind()`` contradiction for one page, so the fixture
+ sidecar cannot drift from ``bind()``'s own behaviour."""
</file context>
| def _binding_adjudication_for(pdf_path: Path, page_num: int, markdown: str) -> dict: | |
| def _binding_adjudication_for(pdf_path: Path, page_num: int, markdown: str) -> tuple[dict, str]: |
1 participant
Summary
TICKET-A1 of
docs/plans/verifier-independence/.socr.benchmark.replay_bindingre-runsbind()on a frozen PDF against the model candidate the pipeline actually bound, with the current tree, and compares the fresh contradiction multiset (duplicate counts preserved) to the sidecar's recordedbinding_adjudication[*].items[]. This is A2's regression oracle.socr-replay-binding <corpus-dir>(pyproject.toml [project.scripts]).table_binding_adjudicatedevent's engine matched to cache entries — never by agreement with the fresh bind (two review rounds from the Codex brain seat closed that hole at both levels: ambiguous cache side, and conflicting provenance events). Ambiguity → the row isUNREPLAYABLEandbind()is never called (tests spy on it).tests/fixtures/replay_binding/, no corpus, no provider (autouse fixture stripsollama/qwen-ocrfrom PATH)._render_adjudication_cropis import-only;orchestrator.pyuntouched (B1 owns it in parallel).Findings for the plan, recorded in
docs/log/2026-09-05_vi-a1-replay-binding.md:native_bbox(adjudication.py:82-90); crop coverage can only come from a fresh re-bind. A1b/C2b wording to follow.winning_output.textis the D3 fail-closed marker; the bound candidate lives only incache/.Frozen corpus (
~/Data/socr/ladder-run2-2026-09-04, unchanged tree): 7/7 rows, exact multiset match, reproducing run-2's 1 lifted / 6 held from fresh binds.Test plan
PYTHONPATH=src ~/venvs/socr/bin/pytest tests/test_replay_binding.py -q→ 12 passed, ollama absent.uvx ruff@0.16.0 format --check .clean.Summary by cubic
Adds a replay-binding harness that re-runs
bind()on frozen PDFs against the candidate the pipeline actually bound, comparing the fresh contradiction multiset to the recorded sidecar to catch binder regressions.New Features
socr-replay-bindingCLI that replays each recorded binding and reports exact multiset matches or deltas.bind()output, so the function under test cannot choose its own input.bind(), and handles pages whose shipped text is the fail-closed marker by falling back to cache via provenance.Label accuracy and crop coverage are reported as unavailable until TICKET-A1b provides hand-read labels.
Written for commit aac4c2b. Summary will update on new commits.