From c7665ca08863da12dc9c656bd8bdf1f55c95bda7 Mon Sep 17 00:00:00 2001 From: Parideboy Date: Tue, 7 Jul 2026 19:45:46 +0200 Subject: [PATCH] fix(transforms): pass through ragged tables instead of misaligning columns (#1713) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Issue #1652 reports the proxy's compression layer surfacing an "impossible mixed" status line — a row combining fields from two different rows of a version-status table (Docker row `0.42.4 → 0.43.0 update available` blended with WSL row `0.42.4 → 0.42.4 up-to-date`). The reporter's follow-up refined the claim: the stored canonical content was intact, but the compression path presents a lossier view that invites exactly this misattribution. There is a concrete mechanism for that in the tabular bridge: `parse_tabular` (`headroom/transforms/tabular_ingest.py`) hands parsed rows to `to_records`, which **silently pads/truncates every row to the header width**. For ragged tables — rows whose cell count differs from the header row, exactly what mixed-shape status tables like the reporter's produce (`✓` and `-` placeholder cells change the token count per row) — this shifts values under the wrong column before SmartCrusher compaction. The compressed output can then state column/value pairings the original never contained. Fix: `parse_tabular` now rejects ragged tables (any row width ≠ header width) and returns `None`, so the content passes through verbatim, per the issue's requirement that a lossy summary "must not create impossible mixed facts". Aligned tables compress exactly as before. The Rust `log_template` Drain miner was also examined; its template rendering only emits tokens that are constant across all rows of a run, so no defect was found there and it is left untouched. Fixes #1652 ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Performance improvement - [ ] Code refactoring (no functional changes) ## Changes Made - `headroom/transforms/tabular_ingest.py`: `parse_tabular` returns `None` when any parsed row's cell count differs from the header count, instead of letting `to_records` pad/truncate rows into the wrong columns. `TabularCompressor.compress` then takes its existing pass-through branch (`was_modified=False`). - `tests/test_transforms_tabular.py`: three new tests — ragged fixed-width table rejected (reproducing the issue's rtk version-status shape), ragged markdown table rejected, and end-to-end `TabularCompressor.compress` pass-through of a ragged table. ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [x] Type checking passes (`mypy headroom`) - [x] New tests added for new functionality - [ ] Manual testing performed ### Test Output ```text $ python -m pytest tests/test_transforms_tabular.py -q 39 passed $ ruff check headroom/transforms/tabular_ingest.py tests/test_transforms_tabular.py All checks passed! $ ruff format --check headroom/transforms/tabular_ingest.py tests/test_transforms_tabular.py 2 files already formatted $ mypy headroom --ignore-missing-imports Success: no issues found (note-level messages only) ``` ## Real Behavior Proof - Environment: Windows 11, Python 3.13, local checkout branched from `upstream/main` (9fbd47ba), Rust core built locally. - Exact command / steps: constructed the issue's table shape (Docker row with 4 cells, WSL row with 6 cells under 4 headers) and ran it through `TabularCompressor().compress()` before and after the change; ran the full `tests/test_transforms_tabular.py` suite. - Observed result: before — `to_records` turned the WSL row into `{'tool': 'rtk', 'installed': '✓', 'latest': '0.42.4', 'status': '0.42.4'}`: the `up-to-date` status is dropped and a version number lands under `status` — precisely the misattributed-fact class from the issue. After — `parse_tabular` returns `None`, `compress` returns the original text unmodified (`was_modified=False`, byte-identical pass-through), and all 39 tests pass (36 pre-existing + 3 new). - Not tested: the reporter's exact end-to-end session (OMP → headroom proxy on 8787 → sticky-router on 4140); the Rust BuildOutput `log_template` path, which was reviewed and found to only emit run-constant tokens. ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review Co-authored-by: Claude Fable 5 --- headroom/transforms/tabular_ingest.py | 7 ++++ tests/test_transforms_tabular.py | 48 +++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/headroom/transforms/tabular_ingest.py b/headroom/transforms/tabular_ingest.py index 07b889b13..373b9c771 100644 --- a/headroom/transforms/tabular_ingest.py +++ b/headroom/transforms/tabular_ingest.py @@ -134,6 +134,13 @@ def parse_tabular( if not headers or not rows: return None + # Ragged tables (rows whose cell count differs from the header count) + # can't be zipped into records without shifting values under the wrong + # column — a compressed table must never state facts the original + # didn't (#1652). Treat them as non-tabular and pass through verbatim. + width = len(headers) + if any(len(row) != width for row in rows): + return None return headers, rows, fmt diff --git a/tests/test_transforms_tabular.py b/tests/test_transforms_tabular.py index 858708892..9316ad22f 100644 --- a/tests/test_transforms_tabular.py +++ b/tests/test_transforms_tabular.py @@ -153,6 +153,54 @@ def test_parse_markdown_table_drops_separator() -> None: assert all("---" not in cell for row in rows for cell in row) +def test_parse_tabular_rejects_ragged_fixed_width(monkeypatch) -> None: + # Rows with differing cell counts can't be zipped under the headers + # without misattributing columns (#1652) — must pass through. + import headroom.transforms.tabular_ingest as ti + + monkeypatch.setattr( + ti, + "detect_content_type", + lambda _c: DetectionResult(ContentType.TABULAR, 0.9, {"format": "fixed_width"}), + ) + ragged = ( + "tool installed latest status\n" + "rtk 0.42.4 0.43.0 update available\n" + "rtk ✓ 0.42.4 0.42.4 - up-to-date" + ) + assert ti.parse_tabular(ragged) is None + + +def test_parse_tabular_rejects_ragged_markdown(monkeypatch) -> None: + import headroom.transforms.tabular_ingest as ti + + monkeypatch.setattr( + ti, + "detect_content_type", + lambda _c: DetectionResult(ContentType.TABULAR, 0.9, {"format": "markdown"}), + ) + ragged = "| a | b | c |\n| --- | --- | --- |\n| 1 | 2 | 3 |\n| 4 | 5 |" + assert ti.parse_tabular(ragged) is None + + +def test_compress_passes_through_ragged_table(monkeypatch) -> None: + import headroom.transforms.tabular_ingest as ti + + monkeypatch.setattr( + ti, + "detect_content_type", + lambda _c: DetectionResult(ContentType.TABULAR, 0.9, {"format": "fixed_width"}), + ) + ragged = ( + "tool installed latest status\n" + "rtk 0.42.4 0.43.0 update available\n" + "rtk ✓ 0.42.4 0.42.4 - up-to-date" + ) + result = TabularCompressor().compress(ragged) + assert not result.was_modified + assert result.compressed == ragged + + def test_parse_tabular_returns_none_for_non_tabular() -> None: assert parse_tabular("just a normal paragraph here") is None