fix(tables): stop grading output against a native layer that is not a grid - #444
fix(tables): stop grading output against a native layer that is not a grid#444r-uben wants to merge 1 commit into
Conversation
… grid (GH-249) The value guard built its baseline from any numeric rows the native parser returned. On a chart page those are axis tick labels, so a transcript that correctly reports "no table here" is the one flagged for dropped rows while a dump of every tick matches the phantom exactly. Apply rows_establish_grid -- already used by the GH-96 exactness metric and the GH-113 escalation trigger -- and abstain when it fails. No chart detection: the reporter measured that drawing density does not separate broken from clean across years. Narrows the guard: a one-data-row native table is no longer verifiable. Pinned in test_single_row_native_now_abstains.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 6 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/tables/native_verifier.py">
<violation number="1" location="src/socr/tables/native_verifier.py:1078">
P2: On page scope, `_rows_by_y_from_words(words)` includes chart and prose numerals outside the table. If one-token chart rows outnumber the table's two-token rows, this gate abstains and real table corruption no longer reaches the value guard; scope the grid check to the matched table band or region.</violation>
<violation number="2" location="src/socr/tables/native_verifier.py:1078">
P2: When a table has a numeric header and only one data row, this gate counts the header as the second grid row and still enables `_value_guard`. Exclude leading header-like rows before applying `rows_establish_grid` so the stated two-data-row narrowing is enforced.</violation>
<violation number="3" location="src/socr/tables/native_verifier.py:1080">
P2: The PR documents this as narrowing only "a native table with only one data row," but the gate silently disables the value guard for a broader and very common shape: any table with a single numeric column (label + one numeric value column, e.g. "Country | GDP"). `_rows_by_y_from_words` groups numeric tokens per y-line, so every row of such a table has width 1, and `rows_establish_grid` returns False (it requires modal row width >= 2) no matter how many rows exist. The verifier then returns the bypass result without running the TR-4 value guard, so dropped/invented values in single-column tables now pass silently. This is the exact "silent content loss" failure mode the codebase treats as worst-case (CLAUDE.md), and no test pins the single-column case — only the one-row case is recorded. If single-numeric-column tables are intended to be unverifiable, the documented impact and risk should be widened to cover that shape.</violation>
</file>
<file name="tests/test_native_table_verifier.py">
<violation number="1" location="tests/test_native_table_verifier.py:133">
P2: The GH-249 gate makes every single-native-row fixture abstain (rows_establish_grid requires two rows at the modal width), so several single-row 'no hard-fail' tests now pass via the early return before the value-guard ever runs. test_no_hard_fail_year_paired_cells_all_values_present — whose docstring calls it 'the key fix for the real-CE false-positive' — now only proves abstention and no longer protects the year-paired/collapse regression it was written for. Add a genuine second native row (and matching output row) to the single-row fixtures that assert on value-guard behavior, so the guard they document still actually runs.</violation>
</file>
<file name="tests/test_gh259_flagged_model_table_wins.py">
<violation number="1" location="tests/test_gh259_flagged_model_table_wins.py:430">
P3: This test's docstring frames the case as 'an ungridded native is not a better home for this page', but the new fixture lays out two numeric rows at the modal width, so the native layer is now a valid grid under rows_establish_grid. The test now exercises the case where native IS a grid and the ragged model table is still kept as a flagged deferral, which contradicts the documented premise and will mislead the next reader about what the test guards. Update the docstring to describe the gridded-native scenario (or keep a single-row native fixture if the ungridded premise is what you intend to cover).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| # output either way. | ||
| native_grid_rows = [ | ||
| _GridRow(tuple(tok for _x, tok in row_tokens)) | ||
| for row_tokens in _rows_by_y_from_words(words).values() |
There was a problem hiding this comment.
P2: On page scope, _rows_by_y_from_words(words) includes chart and prose numerals outside the table. If one-token chart rows outnumber the table's two-token rows, this gate abstains and real table corruption no longer reaches the value guard; scope the grid check to the matched table band or region.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/socr/tables/native_verifier.py, line 1078:
<comment>On page scope, `_rows_by_y_from_words(words)` includes chart and prose numerals outside the table. If one-token chart rows outnumber the table's two-token rows, this gate abstains and real table corruption no longer reaches the value guard; scope the grid check to the matched table band or region.</comment>
<file context>
@@ -1037,6 +1050,42 @@ def _verify_from_words(
+ # output either way.
+ native_grid_rows = [
+ _GridRow(tuple(tok for _x, tok in row_tokens))
+ for row_tokens in _rows_by_y_from_words(words).values()
+ ]
+ if not rows_establish_grid(native_grid_rows):
</file context>
There was a problem hiding this comment.
Agree. Gate sits upstream of the TR-6 y-band _value_guard already uses for this contamination; when width-1 ticks dominate most_common, a real table band never reaches the hard-fail predicates. Blocking in the review — needs a mixed-page pin (ticks + two-row grid with a dropped value → still hard-fail).
| # output either way. | ||
| native_grid_rows = [ | ||
| _GridRow(tuple(tok for _x, tok in row_tokens)) | ||
| for row_tokens in _rows_by_y_from_words(words).values() |
There was a problem hiding this comment.
P2: When a table has a numeric header and only one data row, this gate counts the header as the second grid row and still enables _value_guard. Exclude leading header-like rows before applying rows_establish_grid so the stated two-data-row narrowing is enforced.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/socr/tables/native_verifier.py, line 1078:
<comment>When a table has a numeric header and only one data row, this gate counts the header as the second grid row and still enables `_value_guard`. Exclude leading header-like rows before applying `rows_establish_grid` so the stated two-data-row narrowing is enforced.</comment>
<file context>
@@ -1037,6 +1050,42 @@ def _verify_from_words(
+ # output either way.
+ native_grid_rows = [
+ _GridRow(tuple(tok for _x, tok in row_tokens))
+ for row_tokens in _rows_by_y_from_words(words).values()
+ ]
+ if not rows_establish_grid(native_grid_rows):
</file context>
There was a problem hiding this comment.
Weaker than the other P2s. Numeric header + one data row enabling the guard is closer to a real grid than the documented one-row narrowing; leave as leftover, not a merge blocker.
| @@ -130,14 +130,17 @@ def test_no_hard_fail_year_paired_cells_all_values_present(self): | |||
|
|
|||
There was a problem hiding this comment.
P2: The GH-249 gate makes every single-native-row fixture abstain (rows_establish_grid requires two rows at the modal width), so several single-row 'no hard-fail' tests now pass via the early return before the value-guard ever runs. test_no_hard_fail_year_paired_cells_all_values_present — whose docstring calls it 'the key fix for the real-CE false-positive' — now only proves abstention and no longer protects the year-paired/collapse regression it was written for. Add a genuine second native row (and matching output row) to the single-row fixtures that assert on value-guard behavior, so the guard they document still actually runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_native_table_verifier.py, line 133:
<comment>The GH-249 gate makes every single-native-row fixture abstain (rows_establish_grid requires two rows at the modal width), so several single-row 'no hard-fail' tests now pass via the early return before the value-guard ever runs. test_no_hard_fail_year_paired_cells_all_values_present — whose docstring calls it 'the key fix for the real-CE false-positive' — now only proves abstention and no longer protects the year-paired/collapse regression it was written for. Add a genuine second native row (and matching output row) to the single-row fixtures that assert on value-guard behavior, so the guard they document still actually runs.</comment>
<file context>
@@ -130,14 +130,17 @@ def test_no_hard_fail_year_paired_cells_all_values_present(self):
def test_hard_fail_two_lane_collapse_to_one_cell(self):
"""Two well-separated native lanes, output data row has only 1 populated cell."""
+ # Two rows: the GH-249 grid gate needs two rows at the modal width
+ # before the native layer can serve as ground truth at all.
native_rows = [
</file context>
There was a problem hiding this comment.
Agree, and it's broader than the line this thread landed on. test_no_hard_fail_year_paired_cells_all_values_present, test_no_hard_fail_when_cell_count_matches_native_lanes, test_no_hard_fail_paired_year_columns, and test_no_hard_fail_spanning_header are still single-row and now pass via abstention. Same upgrade as the ten fixtures already touched — blocking in the review.
| _GridRow(tuple(tok for _x, tok in row_tokens)) | ||
| for row_tokens in _rows_by_y_from_words(words).values() | ||
| ] | ||
| if not rows_establish_grid(native_grid_rows): |
There was a problem hiding this comment.
P2: The PR documents this as narrowing only "a native table with only one data row," but the gate silently disables the value guard for a broader and very common shape: any table with a single numeric column (label + one numeric value column, e.g. "Country | GDP"). _rows_by_y_from_words groups numeric tokens per y-line, so every row of such a table has width 1, and rows_establish_grid returns False (it requires modal row width >= 2) no matter how many rows exist. The verifier then returns the bypass result without running the TR-4 value guard, so dropped/invented values in single-column tables now pass silently. This is the exact "silent content loss" failure mode the codebase treats as worst-case (CLAUDE.md), and no test pins the single-column case — only the one-row case is recorded. If single-numeric-column tables are intended to be unverifiable, the documented impact and risk should be widened to cover that shape.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/socr/tables/native_verifier.py, line 1080:
<comment>The PR documents this as narrowing only "a native table with only one data row," but the gate silently disables the value guard for a broader and very common shape: any table with a single numeric column (label + one numeric value column, e.g. "Country | GDP"). `_rows_by_y_from_words` groups numeric tokens per y-line, so every row of such a table has width 1, and `rows_establish_grid` returns False (it requires modal row width >= 2) no matter how many rows exist. The verifier then returns the bypass result without running the TR-4 value guard, so dropped/invented values in single-column tables now pass silently. This is the exact "silent content loss" failure mode the codebase treats as worst-case (CLAUDE.md), and no test pins the single-column case — only the one-row case is recorded. If single-numeric-column tables are intended to be unverifiable, the documented impact and risk should be widened to cover that shape.</comment>
<file context>
@@ -1037,6 +1050,42 @@ def _verify_from_words(
+ _GridRow(tuple(tok for _x, tok in row_tokens))
+ for row_tokens in _rows_by_y_from_words(words).values()
+ ]
+ if not rows_establish_grid(native_grid_rows):
+ logger.debug(
+ "native_verifier [%s]: native layer parsed %d numeric row(s) that do "
</file context>
There was a problem hiding this comment.
Agree. Shared-predicate cost may be intentional, but the PR only documents/pins the one-row narrowing. Dropped values on Country | GDP shapes used to be reachable by TR-4 and now bypass with no pin. Blocking — widen the documented cost and pin the single-column abstention (or an explicit keep-unverifiable decision).
| must NOT be kept — shipping it would replace native with a table socr has | ||
| proved wrong. | ||
| """ | ||
| # Two rows: the GH-249 grid gate needs two rows at the modal width before |
There was a problem hiding this comment.
P3: This test's docstring frames the case as 'an ungridded native is not a better home for this page', but the new fixture lays out two numeric rows at the modal width, so the native layer is now a valid grid under rows_establish_grid. The test now exercises the case where native IS a grid and the ragged model table is still kept as a flagged deferral, which contradicts the documented premise and will mislead the next reader about what the test guards. Update the docstring to describe the gridded-native scenario (or keep a single-row native fixture if the ungridded premise is what you intend to cover).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_gh259_flagged_model_table_wins.py, line 430:
<comment>This test's docstring frames the case as 'an ungridded native is not a better home for this page', but the new fixture lays out two numeric rows at the modal width, so the native layer is now a valid grid under rows_establish_grid. The test now exercises the case where native IS a grid and the ragged model table is still kept as a flagged deferral, which contradicts the documented premise and will mislead the next reader about what the test guards. Update the docstring to describe the gridded-native scenario (or keep a single-row native fixture if the ungridded premise is what you intend to cover).</comment>
<file context>
@@ -427,18 +427,27 @@ def test_verifier_hard_fail_still_falls_back_to_native(tmp_path: Path) -> None:
must NOT be kept — shipping it would replace native with a table socr has
proved wrong.
"""
+ # Two rows: the GH-249 grid gate needs two rows at the modal width before
+ # the native layer can serve as ground truth at all.
fitz_page = _fitz_page_with_numeric_rows(
</file context>
There was a problem hiding this comment.
Agree, leftover not blocker. Fixture is now a valid grid under rows_establish_grid; update the docstring so it stops claiming an ungridded-native premise.
|
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.
REQUEST_CHANGES
Direction is right: native_verifier was the one consumer of the native layer that never asked rows_establish_grid, which is exactly why a page could be table_not_scorable upstairs and still get graded against tick labels here. Putting the gate in _verify_from_words covers page + region, abstain = untouched bypass matches the no-words / no-lanes exits, and TestGH249ChartPageAbstains pins the inversion as a DIFFERENCE the way CLAUDE.md wants. Not-Closes #249 is correct — emit-path residue stays open.
Three holes before this ships.
1. Gate runs before TR-6 y-band — mixed pages can lose the value guard
rows_establish_grid is applied to the raw scoped word list, then _value_guard (which already does header exclusion + pairing-derived y-band for exactly the CE "chart ticks share the page with a real grid" case). On a page whose width-1 tick rows outnumber the table's width-≥2 rows, most_common picks width 1 → abstain → real multiset / label-binding hard-fails never run.
Before this PR the soft row-count warn was inverted on pure chart pages (the #249 bug). On mixed pages the y-band still let the guard fire on the table band. Early total abstain on unscoped words trades the chart bug for silent loss of the guard on the mixed shape the value-guard comments call out.
Fix shape (not prescribing the diff): run the grid predicate on the same effective / y-band-narrowed rows the value guard trusts, or otherwise keep a secondary width-≥2 cluster with ≥2 rows visible when width-1 dominates. Pin a fixture: many width-1 ticks + a two-row two-lane table with a dropped value → still hard-fail.
2. Single-column tables lose the guard with no pin
rows_establish_grid needs modal_width >= 2. A Country | GDP native table is every row width 1 forever, so the verifier now bypasses regardless of how many rows exist. That may be the intended shared-predicate cost (GH-96 / GH-113 already treat it as not-scorable), but the PR only documents and pins the one-data-row narrowing. Dropped/invented values on single-numeric-column tables used to be reachable by TR-4; now they are not, and nothing fails if that regresses further.
Widen the documented cost and pin test_single_numeric_column_native_now_abstains (or an explicit decision that this shape stays unverifiable).
3. Hollowed "no hard-fail" pins
These still ship single-row native fixtures and now pass via the early return, not via the value-guard acceptance they claim:
test_no_hard_fail_year_paired_cells_all_values_present("the key fix for the real-CE false-positive")test_no_hard_fail_when_cell_count_matches_native_lanestest_no_hard_fail_paired_year_columnstest_no_hard_fail_spanning_header
Same treatment as the ten fixtures you already upgraded: add a genuine second row (and matching output) so the guard they document still runs. test_single_row_native_now_abstains already owns the narrowing.
cubic triage
Agree on the page-scope / single-column / hollowed-fixture P2s (above). Header-as-second-row is weaker — numeric header + one data row enabling the guard is closer to a real grid than the one-row narrowing claim; leave as leftover, not a blocker. P3 on test_ragged_kept_grid_… docstring: real leftover, update the "ungridded native" premise now that the fixture is a valid grid.
CI: test (3.11) + typecheck green on b755a98.
1 participant
Part of #249 — deliberately NOT
Closes. This fixes one half; see "What this does not fix".Summary
The value guard built its ground truth from whatever numeric rows the native
parser returned. On a chart page those rows are axis tick labels, one number
per line. Grading a transcript against that manufactures a phantom table, and
the failure is inverted: the reading that correctly reports "there is no table
on this page" sits furthest from the tick count and is the one flagged for
dropped rows, while a dump of every tick matches the phantom exactly.
rows_establish_grid(core/table_grid.py) already exists for precisely this— it is the GH-113 gate, used by the GH-96 exactness metric and by the
orchestrator, where failing it emits the
table_not_scorableevent quoted inthe issue.
native_verifierwas the one consumer of the native layer thatnever asked, which is why a page could be declared not-scorable by the
orchestrator and still be graded against that same layer by the verifier.
This applies the predicate in
_verify_from_words— the single funnel for bothverify_native_tableandverify_native_table_region, so page and regionscope are both covered — and abstains when it fails. Abstaining returns the
untouched bypass result, exactly as the existing no-words and no-lanes cases do.
No chart detection is involved. That is deliberate: the reporter measured that
drawing-op density does not separate broken from clean pages across years
(2010 broken pages sit at 176 ops; clean pages on the same document at 172 and
143), so any fix that depends on recognising a chart page would not generalise.
Behaviour change, stated rather than buried
rows_establish_gridrequires two rows at the modal width, so a one-data-rownative table is no longer verifiable. That is consistent with the predicate's
existing meaning — a single numeric line is exactly what it cannot tell apart
from prose or a stray axis label — but it does narrow the value guard, and it
is the reason ten existing tests needed real second rows rather than weakened
assertions. Pinned in
test_single_row_native_now_abstainsso the cost isvisible and any future change to it is a decision.
What this does not fix
Per #249 (comment), a page
can route to a model rung, be accepted, and still ship the residue unfenced
(FOMC Minutes 2018-03-21 p13,
qwen, 35% bare-number lines). Thechart_assetlane fences its leftovers; the model lane does not. That belongs in the emit
path and is not touched here.
Test plan
~/venvs/socr/bin/pytest tests -q→ 2711 passed, 6 xfailed.uvx ruff@0.16.0 format --check .→ clean (the CI gate, run as CI runs it).TestGH249ChartPageAbstains: the predicate on tick labels; the verifierabstaining on a tick-label page; the inversion pinned as a DIFFERENCE (the
tick dump and the correct reading must be indistinguishable to the verifier,
rather than pinning either verdict); a reverse regression that a dropped
value on a genuine two-row grid still hard-fails; and the one-row narrowing.
given a genuine second row with the asserted behaviour preserved — the
dropped value, the collapse, the lane gap, the exact pass — not relaxed.
Summary by cubic
Stops the native table verifier from grading output against a native layer that is not a grid, so axis tick labels on chart pages no longer produce phantom-table failures. Side effects: one-data-row native tables are now abstained instead of verified, and the model-rung residue path stays unfixed.
Behavior change
rows_establish_gridand abstains when it fails, matching the predicate used by the Hierarchical / spanning-header tables: nested rows scramble while headline totals look right #96 exactness metric and bug(96): escalation trigger fires on non-table pages (degenerate 2-cell pseudo-tables) #113 escalation trigger.Test updates
TestGH249ChartPageAbstainspins the tick-label abstention and the inverted failure as an indistinguishable pair.Written for commit b755a98. Summary will update on new commits.