mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(transforms/cross-turn-dedup): don't renumber-fold zero-padded line prefixes (#2369)
## Description
On an HTTP tool-output re-read, `cross_turn_dedup` folds a contiguous
span that
already appeared in an earlier block into a compact pointer, and when
the line
numbers shifted by a constant it carries the offset as a `delta` so the
original
bytes recover as `int(number) + delta`. The module states this renumber
path is
"strictly lossless" for UNPADDED numbers only.
`_LINENO_RE = ^(\d+)(:|\t)(.*)$` does not enforce the "unpadded"
restriction: `\d+`
also matches a LEADING-ZERO prefix. A timestamped log row such as
`08:00:01 ...`
is read as line number `8`, not as data, so a re-read shifted by a
constant (a
later window of the same hourly log) folds under a uniform delta.
Recovery then
renders `str(int("08") + 1)` = `"9"`, not `"09"`: the round-trip is not
byte-exact. This is a lossy (false-positive) fold in a module whose
stated
posture is to prefer false negatives (`CONTRIBUTING.md:129`,
`cross_turn_dedup.py:45-50`).
## Type of Change
- [x] Bug fix (non-breaking change that fixes an issue)
## Changes Made
- `headroom/transforms/cross_turn_dedup.py`: restrict `_LINENO_RE` to
`[1-9]\d*`
so a leading-zero run stays non-numbered and can fold only on an EXACT
match
(delta 0), never under a lossy renumber. Real `grep -n` / `sed -n` / `rg
-n`
numbers never carry a leading zero, so the intended renumber-fold
feature is
unchanged. Added a comment stating why the character class is
load-bearing.
- `tests/test_cross_turn_dedup.py`: added a delta-aware reconstruction
helper and
three regression tests (the existing `_reconstruct` asserts delta is
absent, so
it never exercised the numbered path this bug lives on).
## Testing
- [x] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [x] Type checking passes (`mypy headroom`)
- [x] New tests added for new functionality
- [x] Manual testing performed
Three named scenarios, one test each:
1. `test_zero_padded_prefix_not_folded_lossily`: a padded shifted
re-read is left
verbatim (`spans_folded == 0`). This fails on `main` (it folds under a
delta).
2. `test_unpadded_renumber_still_folds_and_recovers_exactly`: an
unpadded `grep -n`
read renumbered by `+5` still folds and reconstructs byte-exact (feature
guard).
3. `test_padded_content_exact_redisplay_still_folds`: the same padded
rows
re-displayed verbatim still fold with delta 0 (surgical-scope guard).
### Test Output
```text
--- ruff check ---
All checks passed!
--- ruff format --check ---
2 files already formatted
--- mypy ---
Success: no issues found in 1 source file
--- pytest: 3 new tests on the branch (fixed) ---
3 passed, 14 deselected
--- pytest: revert regex to \d+ (simulate main): the regression test must FAIL ---
1 failed
```
## Real Behavior Proof
- Environment: clean `python:3.12-slim` Docker, `PYTHONPATH` at the
source tree,
core deps installed by name (tiktoken, pydantic, litellm, click, rich,
opentelemetry-api, pyyaml, tomlkit), `ruff==0.15.17`, `mypy==1.20.2`.
- Exact command / steps: import the module and print provenance, then
run ruff,
ruff format, mypy on the two changed files, then `pytest` the three new
tests
on the branch, then revert only the regex to `\d+` and re-run the
regression
test.
- Observed result: module `cross_turn_dedup.py` (sha256
7ff204b65f40617d505895841aa1cfc1ee54bf63ffe6520198f0aa05a850aa8d), regex
now `^([1-9]\d*)(:|\t)(.*)$`; ruff, ruff format, mypy all green; branch
`3 passed`, reverted-regex main `1 failed`. Breakdown:
- `module: /src/headroom/transforms/cross_turn_dedup.py`
- `sha256:
7ff204b65f40617d505895841aa1cfc1ee54bf63ffe6520198f0aa05a850aa8d`
- `regex : ^([1-9]\d*)(:|\t)(.*)$`
- ruff, ruff format, mypy: all green (output above).
- Branch: `3 passed`. Reverted-regex main: the regression test `1
failed`.
- Not tested: the router-level and Rust-backed integration tests in this
file
(`test_apply_*`, `test_dedup_*`) need the compiled `headroom._core`
extension,
which is not built in this lightweight container; they are
`ModuleNotFoundError`
on both `main` and this branch here, so they were not exercised. The
change is a
pure-stdlib regex in a pure-stdlib function; the unit-level
`dedup_blocks` tests
above cover it directly. I also did not measure how often real-world
tool output
hits the leading-zero shifted shape; the argument is the module's own
strictly-lossless contract, not observed field frequency.
## Review Readiness
- [x] I have performed a self-review
- [x] This PR is ready for human review
## Checklist
- [x] My code follows the project's style guidelines
- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation (N/A: no
doc surface)
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective
- [x] New and existing unit tests pass locally with my changes
- [x] I did **not** edit `CHANGELOG.md`
## Additional Notes
Per `CONTRIBUTING.md` ("Bug or small fix -> Open a PR with repro +
test"), this
goes straight to a PR rather than an issue. One concern only; no
dependency or
generated-file changes.
This commit is contained in:
parent
c811007f81
commit
f4070c44cb
2 changed files with 105 additions and 1 deletions
|
|
@ -48,7 +48,17 @@ MAX_ANCHOR_CANDIDATES = 16
|
|||
# NOT match a padded/right-aligned prefix (`` 123<TAB>`` from ``cat -n``): the
|
||||
# line must start with the digit, so ``number + delta`` re-numbers byte-exactly
|
||||
# without touching alignment padding, keeping renumbered folds strictly lossless.
|
||||
_LINENO_RE = re.compile(r"^(\d+)(:|\t)(.*)$", re.DOTALL)
|
||||
#
|
||||
# ``[1-9]\d*`` (not ``\d+``) is load-bearing: a LEADING-ZERO run (``08:00:01`` in
|
||||
# a timestamped log, ``007:...``) is not an unpadded grep/sed line number, and
|
||||
# recovery is ``str(int(number) + delta)``, which drops the zero pad — ``int("08")
|
||||
# + 1`` renders ``"9"``, not ``"09"``. Matching those would fold them under a
|
||||
# uniform delta and the round-trip would NOT be byte-exact, breaking the strictly
|
||||
# lossless promise above. Excluding them keeps such lines non-numbered, so they
|
||||
# fold only on an EXACT match (delta 0) — the "prefer false negatives" posture.
|
||||
# Real grep -n / sed -n / rg -n numbers never carry a leading zero, so the
|
||||
# intended renumber-fold feature is unaffected.
|
||||
_LINENO_RE = re.compile(r"^([1-9]\d*)(:|\t)(.*)$", re.DOTALL)
|
||||
|
||||
|
||||
def _num_and_key(line: str) -> tuple[int | None, str, str]:
|
||||
|
|
|
|||
|
|
@ -129,6 +129,100 @@ def test_info_preserving_reconstruction_multiref():
|
|||
_reconstruct(blocks, out)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------
|
||||
# Numbered (renumber-fold) path: a leading line-number lets the same content
|
||||
# fold across a uniform shift, with the offset carried in the pointer so the
|
||||
# original bytes recover as ``str(int(number) + delta)``. That recovery is
|
||||
# byte-exact ONLY for UNPADDED numbers: a leading-zero prefix (a timestamped
|
||||
# log row, ``08:00:01``) loses its pad on renumber (``int("08") + 1`` -> ``"9"``,
|
||||
# not ``"09"``), so it must not fold under a delta. The helper below renumbers
|
||||
# exactly as the module documents, so the round-trip assertion is faithful.
|
||||
# --------------------------------------------------------------------------
|
||||
def _reconstruct_numbered(orig_blocks, out_blocks):
|
||||
"""Like ``_reconstruct`` but honours a non-zero delta: recover each folded
|
||||
span from the referenced msg, renumbering leading line numbers by the stated
|
||||
offset (``str(int(number) + delta) + key``), then assert byte-exact bytes."""
|
||||
by_turn = {b.turn: b.text.split("\n") for b in orig_blocks}
|
||||
|
||||
def _content(line):
|
||||
return _num_and_key(line)[2].strip()
|
||||
|
||||
for orig, out in zip(orig_blocks, out_blocks):
|
||||
if orig.protected:
|
||||
assert out.text == orig.text
|
||||
continue
|
||||
rebuilt = []
|
||||
for line in out.text.split("\n"):
|
||||
m = _FOLD_RE.search(line)
|
||||
if m and line.lstrip().startswith("[↑"):
|
||||
n, ref = int(m.group(1)), int(m.group(2))
|
||||
delta = int(m.group(3)) if m.group(3) else 0
|
||||
anchor = m.group(4)
|
||||
assert ref < orig.turn, "reference must point to an EARLIER msg"
|
||||
core = anchor[:-3] if anchor.endswith("...") else anchor
|
||||
ref_lines = by_turn[ref]
|
||||
idx = next(i for i, rl in enumerate(ref_lines) if _content(rl).startswith(core))
|
||||
for rl in ref_lines[idx : idx + n]:
|
||||
num, key, _c = _num_and_key(rl) # key keeps the separator
|
||||
rebuilt.append(f"{num + delta}{key}" if (num is not None and delta) else rl)
|
||||
else:
|
||||
rebuilt.append(line)
|
||||
assert "\n".join(rebuilt) == orig.text, f"turn {orig.turn} not faithfully reconstructable"
|
||||
|
||||
|
||||
def _log(prefixes):
|
||||
# Timestamped probe rows: identical content, distinct LEADING-ZERO hour.
|
||||
return "\n".join(f"{p}:00:01 probe ok latency=12ms region=us-east-1" for p in prefixes)
|
||||
|
||||
|
||||
def test_zero_padded_prefix_not_folded_lossily():
|
||||
# A leading-zero numeric prefix shifted by a uniform +1 looks like a renumber
|
||||
# (keys match, delta is uniform), but recovery via int(number)+delta drops the
|
||||
# pad, so the fold would NOT round-trip. It must be left verbatim instead.
|
||||
blocks = [
|
||||
_blk("probe window A\n" + _log(["06", "07", "08"]) + "\ndone A", 1),
|
||||
_blk("probe window B\n" + _log(["07", "08", "09"]) + "\ndone B", 5),
|
||||
]
|
||||
out, stats = dedup_blocks(blocks)
|
||||
assert stats["spans_folded"] == 0
|
||||
assert out[1].text == blocks[1].text and "[↑" not in out[1].text
|
||||
_reconstruct_numbered(blocks, out) # trivially exact: nothing folded
|
||||
|
||||
|
||||
def test_unpadded_renumber_still_folds_and_recovers_exactly():
|
||||
# The intended feature: an UNPADDED grep -n / sed -n read re-displayed after an
|
||||
# edit shifted every line number by a constant still folds, and the pointer's
|
||||
# delta recovers the exact numbered bytes. The fix must not regress this.
|
||||
span = [
|
||||
" result_0 = compute_overdraft(business_id=0, amount=0)",
|
||||
" result_1 = compute_overdraft(business_id=1, amount=100)",
|
||||
" result_2 = compute_overdraft(business_id=2, amount=200)",
|
||||
]
|
||||
b1 = "read A\n" + "\n".join(f"{10 + i}:{s}" for i, s in enumerate(span)) + "\nend A"
|
||||
b2 = "read B\n" + "\n".join(f"{15 + i}:{s}" for i, s in enumerate(span)) + "\nend B"
|
||||
blocks = [_blk(b1, 1), _blk(b2, 3)]
|
||||
out, stats = dedup_blocks(blocks)
|
||||
assert stats["spans_folded"] == 1
|
||||
assert "+5L" in out[1].text # uniform +5 shift carried in the pointer
|
||||
_reconstruct_numbered(blocks, out)
|
||||
|
||||
|
||||
def test_padded_content_exact_redisplay_still_folds():
|
||||
# Surgical-scope guard: the fix only blocks the LOSSY renumbered fold. The same
|
||||
# zero-padded rows re-displayed VERBATIM (delta 0) are still a byte-identical
|
||||
# fold and must continue to compress.
|
||||
log = _log(["06", "07", "08"])
|
||||
blocks = [
|
||||
_blk("probe window A\n" + log + "\ndone A", 1),
|
||||
_blk("re-check\n" + log + "\ntail", 4),
|
||||
]
|
||||
out, stats = dedup_blocks(blocks)
|
||||
assert stats["spans_folded"] == 1
|
||||
m = _FOLD_RE.search(out[1].text)
|
||||
assert m is not None and m.group(3) is None # folded, delta-free (byte-identical)
|
||||
_reconstruct_numbered(blocks, out)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------
|
||||
# Integration: full router.apply() path (content-block tool_result format)
|
||||
# --------------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue