fix(146): never promote a data row to the table header - #149
Merged
Conversation
_grid_to_markdown took grid[0] as the header unconditionally. When the column-header band is absent from the grid, the first data row became the schema: its values were presented as column names and the observation vanished from the body. Every number survives, so a numeric-multiset check passes and nothing detects it. Gate the promotion on a new _is_data_row: a row is data when it names an entity in column 0 and holds at least one value-shaped token. Data rows get an empty header row and stay in the body -- lossless and schema-free. Covers cause 1 of #146. The rowizer's region excluding the header band (cause 2) is untouched.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe table reconstruction logic detects data-first rows, preserves them in Markdown output, and emits an empty header when no valid header exists. Header repair explicitly preserves reconstructed headers. Regression tests cover numeric formats, metadata rows, value retention, escaping, and PDF repair. ChangesHeaderless table reconstruction
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
PR Summary by QodoPrevent data rows from being promoted to Markdown table headers
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/socr/tables/reconstruct.py`:
- Around line 236-238: Update _is_data_row to return False immediately when row
is empty, before calling _is_header_row(row); preserve the existing header and
numeric-token checks for non-empty rows.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b8717d9-b7de-4e40-b5e9-5ea8c9a1c1ca
📒 Files selected for processing (2)
src/socr/tables/reconstruct.pytests/test_table_header_gh146.py
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 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/reconstruct.py">
<violation number="1" location="src/socr/tables/reconstruct.py:238">
P2: This new header-demotion path is shared by the header_repair path too: `header_repair.py:465` calls the same `_grid_to_markdown(repaired)` on the *repaired* header. Because `_is_data_row` returns True any time a non-empty col-0 row has at least one numeric-shaped data cell (e.g. a repaired/merged header like `['Firm', '2024', '2025']` or a header whose cells are percentage/stat tokens), the freshly repaired header would now be demoted to a body row and replaced with an empty header — silently discarding the repair that `repair_table_headers_in_text` just performed. The PR states header_repair remains compatible, but only its existing tests (which use word headers) exercise this; a numeric-shaped repaired header is not covered. Consider scoping the data-row demotion to the primary rowize path (e.g. a flag/separate renderer) or restricting it so a header produced by `_rebuild_header` is never demoted.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| """ | ||
| if _is_header_row(row): | ||
| return False | ||
| return any(_NUM_TOKEN_RE.match(c.strip()) for c in row[1:] if c.strip()) |
There was a problem hiding this comment.
P2: This new header-demotion path is shared by the header_repair path too: header_repair.py:465 calls the same _grid_to_markdown(repaired) on the repaired header. Because _is_data_row returns True any time a non-empty col-0 row has at least one numeric-shaped data cell (e.g. a repaired/merged header like ['Firm', '2024', '2025'] or a header whose cells are percentage/stat tokens), the freshly repaired header would now be demoted to a body row and replaced with an empty header — silently discarding the repair that repair_table_headers_in_text just performed. The PR states header_repair remains compatible, but only its existing tests (which use word headers) exercise this; a numeric-shaped repaired header is not covered. Consider scoping the data-row demotion to the primary rowize path (e.g. a flag/separate renderer) or restricting it so a header produced by _rebuild_header is never demoted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/socr/tables/reconstruct.py, line 238:
<comment>This new header-demotion path is shared by the header_repair path too: `header_repair.py:465` calls the same `_grid_to_markdown(repaired)` on the *repaired* header. Because `_is_data_row` returns True any time a non-empty col-0 row has at least one numeric-shaped data cell (e.g. a repaired/merged header like `['Firm', '2024', '2025']` or a header whose cells are percentage/stat tokens), the freshly repaired header would now be demoted to a body row and replaced with an empty header — silently discarding the repair that `repair_table_headers_in_text` just performed. The PR states header_repair remains compatible, but only its existing tests (which use word headers) exercise this; a numeric-shaped repaired header is not covered. Consider scoping the data-row demotion to the primary rowize path (e.g. a flag/separate renderer) or restricting it so a header produced by `_rebuild_header` is never demoted.</comment>
<file context>
@@ -216,13 +216,46 @@ def _looks_tabular(grid: list[list[str]]) -> bool:
+ """
+ if _is_header_row(row):
+ return False
+ return any(_NUM_TOKEN_RE.match(c.strip()) for c in row[1:] if c.strip())
+
+
</file context>
Review finding on #149: `_NUM_TOKEN_RE` is anchored, so `0.67***` does not match it. A starred coefficient row -- the common shape in an econometrics table -- was therefore still classified as a header and promoted, which is the exact defect this branch exists to close. Delegate value-shapedness to `native_verifier.is_numeric_token`, which already strips presentation before matching (GH-103): significance markers, markdown emphasis, unicode minus and currency prefixes. Imported inside the function because native_verifier imports `_NUM_TOKEN_RE` from this module. Also pins the empty-row case, which `_is_header_row` already guards.
Second review finding on #149: `header_repair.py` calls the same `_grid_to_markdown` on a header it has just reconstructed from native word geometry and gated on `_header_is_faithful`. Header inference then ran on top of that evidence and demoted any numeric-shaped band, discarding the repair and leaving the table with no schema at all. Reproduced end-to-end on synthetic CE-style geometry: a percentage-bin header that also prints its label-column name (`Currency`) yields `['Currency', '-23%', ...]`, which is `_is_data_row` by shape. Add a keyword-only `assume_header` to `_grid_to_markdown` and pass it from the repair path. Evidence beats inference, but only where the evidence actually exists -- the reconstruct path has none and keeps inferring.
r-uben
added a commit
that referenced
this pull request
Aug 12, 2026
Review finding on #149: `_NUM_TOKEN_RE` is anchored, so `0.67***` does not match it. A starred coefficient row -- the common shape in an econometrics table -- was therefore still classified as a header and promoted, which is the exact defect this branch exists to close. Delegate value-shapedness to `native_verifier.is_numeric_token`, which already strips presentation before matching (GH-103): significance markers, markdown emphasis, unicode minus and currency prefixes. Imported inside the function because native_verifier imports `_NUM_TOKEN_RE` from this module. Also pins the empty-row case, which `_is_header_row` already guards.
r-uben
added a commit
that referenced
this pull request
Aug 12, 2026
PR #149 fixed only cause 1 of #146 — a data row promoted to header — and deliberately left the issue open for cause 2: the rowizer's region excludes the column-header band above the data rows (NS p13, region y 129..374 vs a header at y 112..120). No plan covered that residual, so the table now ships an empty header instead of a wrong one: lossless, still schema-less. Add TICKET-A2b in the GH-144 folder rather than a new one — it is the same class of reconstruct.py boundary error and must serialize on the same file, which only one folder can express. A2 and A2b are one lane held by one agent in sequence; waves bound concurrency, not how much work an agent does on a file it owns. A2b does not name its own cause. A ~9 pt gap is below _SPLIT_GAP_MIN_PT (10.0), so the split threshold is unlikely to be it; GH-144 A1 now also measures region y-extent against the header band and must name the code path before A2b fixes it. Critical path grows to six: A1 - A2 - A2b - 152 A1 - 152 A2 - 152 B1. Wave 0 recorded DONE: PR #148 and PR #149 merged 2026-08-12.
This was referenced Aug 13, 2026
This was referenced Aug 13, 2026
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.
Summary
Fixes cause 1 of #146:
_grid_to_markdowntookgrid[0]as the header unconditionally, so when the column-header band was absent from the grid the first data row became the schema — its values presented as column names, and the observation gone from the body.Reference case is Nakamura & Steinsson (WP) p.13, Table I:
Every number survives, so a numeric-multiset check passes and nothing detects it. That is the dangerous part: a wrong schema plus one fewer observation, silently.
Promotion is now gated on a new
_is_data_row. A row is data when it names an entity in column 0 and holds at least one value-shaped token. Data rows get an empty header row (markdown permits one) and stay in the body — lossless, and no invented schema.Scope / non-goals
Cause 2 is not touched — the rowizer's region still excludes the header band above the data rows (
y 129..374vs. a header aty 112..120). That is the same class of boundary error as GH-145 and wants its own design pass. #146 stays open for it; this PR is deliberately notCloses.Commits
02c0ed5— gate header promotion on_is_data_row.7aaf992— treat decorated values as values (review finding).a6a2b9d— letheader_repairassert the header it just rebuilt (review finding).Implementation notes
Not
_is_header_row. The issue proposed calling the existing_is_header_row(grid[0]). That is not safe: it returnsFalsefor['Firm', 'Nominal', 'Real'], a perfectly ordinary header, so wiring it in directly would demote real headers and breakstest_grid_to_markdown_shape. Its docstring scopes it to "safe to merge into the multi-line header prefix" — a conservative merge test, not a header test.Decorated values (2).
_NUM_TOKEN_REis anchored, so0.67***does not match it — a starred coefficient row, the common shape in an econometrics table, was still being promoted. Value-shapedness now delegates tonative_verifier.is_numeric_token, which already strips presentation before matching (GH-103): significance markers, markdown emphasis**23,126**, unicode minus−0.253, currency prefixes£43.2. Reuse, not a new regex. Imported inside the function becausenative_verifierimports_NUM_TOKEN_REfromreconstruct— module level would cycle.The
header_repairseam (3). An earlier revision of this description claimed header_repair was unaffected. That was wrong, and a reviewer caught it.header_repair.py:465feeds the same_grid_to_markdowna header it has just reconstructed from native word geometry and gated on_header_is_faithful; inference then ran on top of that evidence and demoted any numeric-shaped band, discarding the repair and leaving the table with no schema at all — strictly worse than the original bug.Reproduced end-to-end on synthetic CE-style geometry. A bare year band cannot reach that code (
_is_table_header_rowrequires a range marker or connector word); the reachable case is a percentage-bin header that also prints its label-column name on the header line:Column 0 non-empty, every data cell a bare numeric token — exactly
_is_data_row. Fixed with a keyword-onlyassume_headeron_grid_to_markdown, passed only from the repair path. Evidence beats inference, but only where the evidence exists; the reconstruct path has none and keeps inferring.Known residual ambiguity, documented in the docstring. On the inference path, a single-line header whose cells are themselves numeric (
['Firm', '2024', '2025'],['Variable', '(1)', '(2)']) is indistinguishable from data by shape alone and is treated as data — trading a lost column name for a guaranteed-present observation, the right direction for a citation corpus. Multi-line headers, where the band sits on its own row with an empty column 0, are unaffected:_is_header_rowaccepts those and_collapse_header_prefixmerges them before this check runs.Test plan — observed results
New
tests/test_table_header_gh146.py, 17 tests:_is_data_row— label+value, parenthesised SEs,%/thousands separators, significance markers, bold cells, U+2212, currency prefixes, word headers (incl. bold ones), column-metadata rows, label-only rows, single-column and empty rows._grid_to_markdown— empty-header arity, no value lost, real headers still promoted, pipe escaping in a demoted row.assume_header— the flag at the unit seam, plus an end-to-end test driving the realrepair_collapsed_header/repair_table_headers_on_pagepath on a syntheticfitzpage. It assertscount == 1and the exact repaired header, so it fails loudly if the fixture ever stops exercising the repair.Observed:
~/venvs/socr/bin/pytest -q→ 1528 passed, 1 xfailed~/venvs/socr/bin/pytest tests/test_table_header_gh146.py tests/test_header_repair.py tests/test_reconstruct.py -q→ 39 passeduvx ruff@0.16.0 format --check .→ 272 files already formattedReviewer notes
src/socr/tables/reconstruct.pyand the one call site insrc/socr/tables/header_repair.py.assume_headerboundary: callers with geometric evidence assert; callers without it infer. If a third caller appears, it must make that choice explicitly.fix/145) — different files, and the tests live in their own file rather than extendingtest_region_overlap_gh145.py, so the two branches do not conflict.