Skip to content

feat(doc-ingest): --csv compute-exact table view (stacked on #2377) - #2434

Merged
john-the-dev merged 8 commits into
mainfrom
feat/doc-ingest-compute-over-tables
Aug 5, 2026
Merged

john-the-dev merged 8 commits into
mainfrom
feat/doc-ingest-compute-over-tables

Conversation

@john-the-dev

Copy link
Copy Markdown
Collaborator

Stacked PR — parent: #2377

Base is feat/doc-ingest-skill (#2377), NOT main. Merge order: #2377 first, then this (I'll rebase onto main and rerun full checks after the parent lands, per CONTRIBUTING's stacked-PR rule). The diff shown is the child layer only.

What

The GAIA file-attached experiment (2026-07-30) proved a general rule: quantitative answers over tabular files must come from computing over the exact table, never from reading extracted text. Re-running the failed file tasks blind with only that change flipped 3/3 computable misses (multi-row Whyte-notation sums, filtered counts, address-parity logic): 84.2% → 92.1% on the official scorer (artifacts: workspace data/agent-eval/files_ans/SCORE_20260730_retry2.md).

This PR encodes the capability in the skill itself:

  • --csv — the compute-exact twin of the row-capped markdown reading view: csv/tsv verbatim (tsv normalized), xlsx/xlsm per-sheet === sheet: NAME === CSV via openpyxl.
  • Exactness guarantees: no default row/char caps in --csv (a silently truncated table computes a silently wrong aggregate; explicit --max-chars still wins), and xlsx without openpyxl is refused with a clear error rather than served by the approximate zip-XML fallback — approximate cells are fine to read, not to compute with.
  • SKILL.md: new "Quantitative questions: compute, don't read" section with the evidence, so every agent consuming the skill applies the rule.

Evidence (at head e20c2b9)

$ python3 tests/doc-ingest-csv-mode.test.py
All doc-ingest --csv checks passed.        # 13 checks:
  exact 601-row csv (beyond the 500-row reading cap) · tsv normalization ·
  default cap lifted / explicit --max-chars wins · per-sheet xlsx markers ·
  exact aggregate recomputed from emitted CSV (4+14+8=26) ·
  no-openpyxl refusal · non-tabular --csv errors per-file (exit 1, --json)

$ python3 tests/doc-ingest.test.py         # parent suite untouched
OK — 45 checks passed

$ git diff origin/feat/doc-ingest-skill...HEAD | bash scripts/review-checks.sh
review-checks: PASS (hardcoded-paths clean)

🤖 Generated with Claude Code

https://claude.ai/code/session_019vXW4jZyEySfbo1UGUZmm7

@john-the-dev john-the-dev added the ag2product AG2 product related (packages, PyPI, task-relay) label Jul 30, 2026
@CLAassistant

CLAassistant commented Jul 30, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issue on head e20c2b9f7e032417cb90a097ea375180ee7769fe:

  • skills/doc-ingest/scripts/ingest.py:276-290 materializes the entire CSV/TSV/XLSX table into Python lists, while main() disables the default character cap for --csv at skills/doc-ingest/scripts/ingest.py:450-454. That makes the new compute-exact path unbounded by default for user-supplied attachments: a large CSV/XLSX can consume memory and then emit arbitrarily large stdout. Compute-exact mode should not silently truncate, agreed, but it still needs a fail-closed resource budget (for example file size/rows/cells/output bytes, with a loud error and an explicit override) rather than removing the only output guard and reading the full table into memory. The existing archive extractor already has a 64 MiB cumulative budget because attachments are untrusted; --csv needs an equivalent bound instead of codifying unbounded output in the tests.

Please keep the exactness contract by failing loudly when the default compute budget is exceeded, and add regressions for oversized CSV/XLSX inputs so the skill cannot OOM or stream unbounded output by default.

Focused checks run:

  • PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2434-pycache-2330 python3 tests/doc-ingest-csv-mode.test.py (passes; xlsx positive cases skipped because openpyxl is not installed here)
  • PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2434-pycache-2330 python3 tests/doc-ingest.test.py (passes)
  • PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2434-pycache-2330 python3 -m py_compile skills/doc-ingest/scripts/ingest.py tests/doc-ingest-csv-mode.test.py tests/doc-ingest.test.py (passes)
  • git diff --check a552908fa680002546da8938ffdaa06138c3b860..HEAD (passes)

Current GitHub status is only the pending CLA check; this PR is also still draft.

Reviewed by Qingyun's Personal Codex.

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

@qingyun-wu addressed in 67fdeb7 (plus merge commit 838bd6e syncing the base's extract() signature — conflict was one dispatch line).

What changed (fail-closed budget, exactness kept):

  • --csv no longer materializes tables into lists: csv/tsv and xlsx rows stream through _stream_rows_to_csv() under one shared budget dict (multi-sheet workbooks share a single budget).
  • Three bounds, all refuse LOUDLY (never truncate) and name the override: source file > 32 MiB (CSV_MAX_SOURCE_BYTES, checked before reading), rendered output > 64 MiB (CSV_MAX_OUTPUT_BYTES), cells > the shared 1M MAX_TABLE_CELLS.
  • Explicit --csv-no-budget flag restores unbounded behavior when the caller genuinely wants it; SKILL.md documents the contract.

Evidence at HEAD (67fdeb7):

$ python3 tests/doc-ingest-csv-mode.test.py
ok   oversized source: refused, exit 1
ok   oversized source: nothing on stdout (no partial table)
ok   oversized source + --csv-no-budget: exact full output
ok   cell budget: refused loudly
ok   output-bytes budget: refused loudly
ok   budgets restored: normal-sized table still exact
All doc-ingest --csv checks passed.   (19 checks)
$ python3 tests/doc-ingest.test.py
OK — 50 checks passed
$ diff-cover (vs origin/feat/doc-ingest-skill): 96% (gate 95%)

At the parent commit e20c2b9 the same oversized inputs stream unbounded output (the behavior you flagged); at HEAD they exit 1 with the budget error. Conflict with the base is resolved (PR shows MERGEABLE); license/cla remains PENDING due to the repo-wide cla-assistant integration outage (owner-admin, tracked separately).

@john-the-dev
john-the-dev marked this pull request as ready for review July 31, 2026 13:52

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issue on head 67fdeb7b2df92ae3a1ede819b8258ea09b423ce3:

  • skills/doc-ingest/scripts/ingest.py:444-448 enforces CSV_MAX_OUTPUT_BYTES by subtracting StringIO.tell() deltas, which are character positions, not encoded bytes. That means the advertised 64 MiB rendered-output budget can be bypassed by non-ASCII or replacement characters while --csv still exits successfully and writes more than the byte budget to stdout. I reproduced this directly by setting CSV_MAX_OUTPUT_BYTES = 40 and feeding a 30-byte invalid UTF-8 CSV row: extract_table_csv() returned normally with 31 characters whose UTF-8 rendering is 91 bytes. This keeps the important part of the previous blocker alive: untrusted attachments can exceed the fail-closed stdout byte budget without using --csv-no-budget.

Please count the actual encoded output bytes (for example, the bytes that will be emitted as UTF-8) before accepting each row, and add a regression with multibyte/replacement-character data so the CSV_MAX_OUTPUT_BYTES name and docs remain true. The existing tests cover ASCII only, so they do not catch this unit mismatch.

Focused checks run:

  • python3 -X pycache_prefix=/private/tmp/sutando-pr2434-67fdeb7-pycache -m py_compile skills/doc-ingest/scripts/ingest.py tests/doc-ingest-csv-mode.test.py tests/doc-ingest.test.py
  • python3 tests/doc-ingest-csv-mode.test.py (passes; xlsx positive cases skipped because openpyxl is not installed here)
  • python3 tests/doc-ingest.test.py
  • git diff --check ba6816551ce9b9919649a375ff845119acd325e3...HEAD
  • bash scripts/review-checks.sh --diff /private/tmp/pr2434-67fdeb7.diff
  • Custom budget probe: CSV_MAX_OUTPUT_BYTES = 40, invalid UTF-8 CSV row, returned 91 UTF-8 bytes without raising

Reviewed by Qingyun's Personal Codex.

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Byte-budget unit mismatch fixed at head b0e0c85a.

Root cause confirmed as diagnosed: _stream_rows_to_csv charged the budget with StringIO.tell() deltas — character positions — so any content whose UTF-8 rendering exceeds its character count (U+FFFD replacement chars are 3 bytes each) slid under CSV_MAX_OUTPUT_BYTES. Rows now render into a per-row buffer and charge len(chunk.encode("utf-8")) before acceptance, so the budget's contract is literally bytes-on-stdout.

Your probe, re-run at the fixed head (CSV_MAX_OUTPUT_BYTES = 40, ~30-byte invalid-UTF-8 CSV row — the case that previously returned 91 emitted bytes with exit 0):

PROBE OK: raised: --csv output exceeds the 0 MiB compute budget (fail-closed: attachments are untr…

Regressions added (tests/doc-ingest-csv-mode.test.py §7b), all passing:

ok   byte budget counts encoded bytes (replacement chars): refused
ok   byte budget (replacement chars): nothing on stdout (no partial table)
ok   byte budget counts encoded bytes (multibyte): refused
ok   multibyte under the byte budget: exact output, exit 0

The first case is your exact shape (chars fit the budget, encoded bytes don't); the euro-row pair pins both directions — refused at 40 bytes where a character count would pass, and exact emission at 128 bytes where the encoded size genuinely fits.

Full suites green: doc-ingest-csv-mode (all checks incl. §7b) and doc-ingest.test.py (50 checks). Child-layer diff-cover vs feat/doc-ingest-skill: 92.3% — the 4 uncovered lines (654, 675–677: unknown-suffix fallback + argv int-parse guard) predate this commit; every line touched by this fix is covered in-process.

Requesting re-review of b0e0c85a.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XaocEvWwhBokFUmK5ECsfs

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head b0e0c85a4549b2ef9e00ff6d0d4b8001cae45602 as the child layer on #2377 and cumulatively.

The remaining output-budget blocker is fixed: rows are rendered independently and charged by their encoded UTF-8 byte length before emission, so replacement and multibyte characters cannot bypass CSV_MAX_OUTPUT_BYTES or leak a partial table. Reviewer-run tests/doc-ingest-csv-mode.test.py passes, full diff/path gates pass, and the current stack CI state is green/pending only where reported.

Approved on this head. Because this is stacked, refresh after #2377 lands and rerun the child-only plus cumulative checks before merge.

Base automatically changed from feat/doc-ingest-skill to main August 3, 2026 04:53

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1 blocker] Rebase the child onto merged parent #2377 and preserve the parent hardening.

#2377 merged as 42e4cde9 at 2026-08-03T04:53:32Z, but this head b0e0c85a still carries the pre-final parent history. The current GitHub three-dot diff therefore reports all four files as 1,548 additions, and the branch is not a valid post-parent child layer. More importantly, a direct tree comparison against current main is 418 additions / 278 deletions: skills/doc-ingest/scripts/ingest.py:253-269 restores unbounded subprocess.run / whole-document joins and drops the parent versions of _run_text_command_bounded, _read_ooxml_member_bounded, and _validate_ooxml_archive_bounded; tests/doc-ingest.test.py:398 jumps directly to table-summary tests, omitting the corresponding converter timeout/byte-budget and OOXML inflation regressions. Resolving the stack from this stale tree could regress resource-exhaustion protections for untrusted attachments.

Please rebase the child onto current main, resolve ingest.py and its tests by retaining the parent converter/OOXML bounds while applying the compute-exact CSV layer, then rerun child-only and cumulative suites plus the complete hosted gates and CLA. The old-stack checks still pass here (doc-ingest-csv-mode, 20 checks; doc-ingest.test.py, 50 checks; Python compile; diff/path gate), but they do not validate the required post-parent composition.

Changes requested; this PR is not merge-ready until the stacked-child refresh and cumulative verification are complete.

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreeing with the existing CHANGES_REQUESTED — adding a new, concrete blocker on top of it.

Now that parent #2377 has landed on main (mergedAt 2026-08-03T04:53), mergeable on this PR is CONFLICTING, and it's not cosmetic: git merge-tree $(git merge-base main pr2434) main pr2434 shows real 3-way conflicts in skills/doc-ingest/SKILL.md, skills/doc-ingest/scripts/ingest.py, and tests/doc-ingest.test.py. ingest.py specifically has diverged in substance since this branch was cut — main now carries extract_pdf(path, max_chars=DEFAULT_MAX_CHARS) (this branch still has the old extract_pdf(path) signature), a new _ResourceBudgetError class, and MAX_DOCUMENT_TEXT_CHARS/MAX_DOCUMENT_BYTES/TEXT_COMMAND_TIMEOUT_SECONDS constants that don't exist on this branch at all.

So gh pr diff here is showing a diff against a stale merge-base, not against current main — the --csv streaming/budget work in this PR hasn't actually been reconciled against the newer streaming/budget refactor that landed with the parent. Not ready to merge until the author does the rebase they already committed to in the PR description and re-verifies the child suite against the real current ingest.py.

Rebuilt as a clean single-layer child of merged parent #2377 (qingyun-wu +
bassilkhilo-ag2 CR): the old branch was cut from the pre-hardening parent, so
its tree had drifted to REMOVE main's resource bounds (_run_text_command_bounded,
OOXML member/archive budgets, bounded extractor signatures). This layer is now
built on current main and preserves every one of those bounds — it only ADDS the
compute-exact CSV path.

--csv renders tabular files (.xlsx/.xlsm/.csv/.tsv) to exact per-sheet CSV for
COMPUTING over (the machine twin of the row-capped markdown reading view):
  * no default row/char caps — a silently truncated table computes a silently
    wrong aggregate (--csv sets max_chars=0 unless --max-chars is explicit);
  * xlsx without openpyxl is REFUSED, not served by the approximate zip-XML
    fallback (approximate cells are fine to read, not to compute with);
  * not uncapped: untrusted attachments over CSV_MAX_SOURCE_BYTES (32 MiB),
    renders over CSV_MAX_OUTPUT_BYTES (64 MiB), or tables over MAX_TABLE_CELLS
    (1M) fail LOUDLY (never truncate); --csv-no-budget is the explicit override.
  * byte budget counts ENCODED UTF-8 bytes, not char deltas (qingyun CR 2026-07-31).

Motivation: GAIA file-attached subset 84.2%→92.1% (3/3 computable numeric misses
flipped) by computing over the loaded table instead of reading extracted text.

Verified on current main: tests/doc-ingest-csv-mode.test.py green, and the parent's
tests/doc-ingest.test.py (59 checks incl. all converter-timeout / byte / OOXML
resource-exhaustion bounds) still green — no hardening regressed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XaocEvWwhBokFUmK5ECsfs
@john-the-dev
john-the-dev force-pushed the feat/doc-ingest-compute-over-tables branch from b0e0c85 to ac2bd38 Compare August 3, 2026 12:09
@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Rebased onto merged parent #2377 at ac2bd380 — addresses both @qingyun-wu's and @bassilkhilo-ag2's CRs. mergeable is now clean and the three-dot diff is a true single-layer child: +380/-4, three files (ingest.py, SKILL.md, tests/doc-ingest-csv-mode.test.py).

What was wrong (both of you flagged it): the old branch was cut from the pre-hardening parent, so its tree had drifted to remove main's resource bounds — _run_text_command_bounded, _read_ooxml_member_bounded, _validate_ooxml_archive_bounded, _join_bounded, and the bounded max_chars extractor signatures. Merging it as-is would have regressed resource-exhaustion protection on untrusted attachments.

How I fixed it: rather than a 14-block 3-way merge of a tangled history, I rebuilt the branch as a clean layer on current main — kept every one of main's bounds untouched, and added only the compute-exact CSV path:

  • CSV_MAX_SOURCE_BYTES / CSV_MAX_OUTPUT_BYTES, TABULAR_SUFFIXES, _stream_rows_to_csv(), extract_table_csv(), and the --csv / --csv-no-budget CLI handling (incl. --csv ⇒ max_chars=0 unless --max-chars is explicit).
  • The byte budget counts encoded UTF-8 bytes, not char deltas (qingyun CR 2026-07-31).

Verification on current main (not the stale tree):

  • tests/doc-ingest-csv-mode.test.py — green (the feature, incl. multibyte/replacement-char byte-budget cases).
  • tests/doc-ingest.test.py — 59 checks green, including every hardening regression: converter-timeout, converter-stopped-at-budget, pdf-text-budget, docx-member-byte-budget, pptx-member-byte-budget, xlsx-uncompressed-byte-budget, xlsx-sparse-reference-budget. No hardening regressed.
  • py_compile clean, gen-src-map.py --check up to date.

The old-stack checks you each ran no longer apply — this is the post-parent composition you asked for. --csv xlsx still refuses without openpyxl (approximate cells are for reading, not computing).

@github-actions

github-actions Bot commented Aug 3, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 77%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • skills/doc-ingest/scripts/ingest.py (100%)

Summary

  • Total: 66 lines
  • Missing: 0 lines
  • Coverage: 100%

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested on exact rebased head ac2bd380.

[P1 resource exhaustion] skills/doc-ingest/scripts/ingest.py:764-773 opens untrusted XLSX/XLSM with openpyxl.load_workbook() before checking the archive's uncompressed size. The 32 MiB CSV_MAX_SOURCE_BYTES gate measures only the compressed file, and the cell/output budgets start only after openpyxl has parsed the workbook and row iteration begins. A small OOXML zip bomb (especially an oversized sharedStrings.xml or worksheet XML) can therefore allocate/inflate far beyond the advertised default compute budget before either limit can fire.

This bypasses the parent hardening preserved elsewhere on the rebased head: extract_xlsx() preflights compressed and uncompressed table XML before openpyxl, and _validate_ooxml_archive_bounded() exists specifically to reject archive inflation before a third-party OOXML parser opens it. Apply an equivalent uncompressed preflight to the --csv XLSX path (unless the caller explicitly chose --csv-no-budget) and add a regression whose compressed file is small but declared/uncompressed OOXML payload is over budget, asserting load_workbook is never reached.

Reviewer-run tests/doc-ingest-csv-mode.test.py passes (its positive XLSX cases skip here because openpyxl is unavailable), the parent tests/doc-ingest.test.py passes 59/59, and compilation/diff/path gates pass. The current tests bound rendered cells/bytes but do not cover parser-time OOXML inflation on the new path. Worst-case disruption is an untrusted attachment exhausting memory before the fail-closed budget runs, so this is not merge-ready.

Reviewed by Qingyun's Personal Codex.

… --csv path (qingyun CR #2434)

The --csv XLSX path called openpyxl.load_workbook() after only a compressed-size
gate, so a small OOXML zip bomb (oversized sharedStrings/worksheet XML) could
inflate past the compute budget before any cell/byte budget fired. Add an
uncompressed-archive preflight (sum of zip member file_size vs CSV_MAX_SOURCE_BYTES)
BEFORE importing/handing the file to openpyxl, so the bomb is rejected even where
openpyxl is absent. --csv-no-budget opts out, matching the other budgets. Mirrors
extract_xlsx's _validate_ooxml_archive_bounded.

Regression: a small compressed .xlsx whose uncompressed payload exceeds the
budget is refused with the inflation error while the openpyxl import is blocked —
proving load_workbook is never reached; and --csv-no-budget skips the preflight.
csv-mode suite green (+3 checks), parent doc-ingest.test.py 59/59, py_compile,
gen-src-map --check, git diff --check all pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XaocEvWwhBokFUmK5ECsfs
john-the-dev pushed a commit that referenced this pull request Aug 3, 2026
… --csv path (qingyun CR #2434)

The --csv XLSX path called openpyxl.load_workbook() after only a compressed-size
gate, so a small OOXML zip bomb (oversized sharedStrings/worksheet XML) could
inflate past the compute budget before any cell/byte budget fired. Add an
uncompressed-archive preflight (sum of zip member file_size vs CSV_MAX_SOURCE_BYTES)
BEFORE importing/handing the file to openpyxl, so the bomb is rejected even where
openpyxl is absent. --csv-no-budget opts out, matching the other budgets. Mirrors
extract_xlsx's _validate_ooxml_archive_bounded.

Regression: a small compressed .xlsx whose uncompressed payload exceeds the
budget is refused with the inflation error while the openpyxl import is blocked —
proving load_workbook is never reached; and --csv-no-budget skips the preflight.
csv-mode suite green (+3 checks), parent doc-ingest.test.py 59/59, py_compile,
gen-src-map --check, git diff --check all pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XaocEvWwhBokFUmK5ECsfs
@john-the-dev

Copy link
Copy Markdown
Collaborator Author

@qingyun-wu fixed at 091ff7db — good catch on the parser-time inflation gap.

The --csv XLSX path now preflights the uncompressed archive size (sum of zip member file_size vs CSV_MAX_SOURCE_BYTES) before importing/handing the file to openpyxl — so a small-compressed / large-inflated OOXML bomb is rejected before load_workbook allocates anything. Placing it ahead of the import openpyxl also makes the guard fire on hosts without openpyxl. --csv-no-budget opts out, consistent with the other budgets. This mirrors extract_xlsx's _validate_ooxml_archive_bounded.

Regression added (your exact requested shape — small compressed, over-budget uncompressed, load_workbook never reached), proven host-independently by blocking the openpyxl import and asserting the inflation error fires anyway:

zip-bomb: compressed file is under the source budget          ok
zip-bomb: refused on UNCOMPRESSED size before openpyxl        ok   ("uncompressed" in err, "openpyxl" not in err)
zip-bomb + --csv-no-budget: skips inflation preflight         ok   (reaches openpyxl path)

Checks: doc-ingest-csv-mode.test.py all pass (+3), parent doc-ingest.test.py 59/59, py_compile, gen-src-map.py --check, git diff --check origin/main...HEAD clean. Please re-review when you can.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I re-reviewed exact head 091ff7db8a4b964cdb7f0e39b03308a9182163ee in an isolated checkout and the CSV/TSV and XLSX zip-bomb preflight shape looks good to me. Focused local checks passed:

  • PYTHONDONTWRITEBYTECODE=1 python3 tests/doc-ingest-csv-mode.test.py
  • PYTHONDONTWRITEBYTECODE=1 python3 tests/doc-ingest.test.py
  • python3 -m py_compile for the touched ingest/test files
  • git diff --check origin/main...HEAD

Blocking on CI/test coverage only: the hosted diff coverage gate is still failing at 92.4%, missing skills/doc-ingest/scripts/ingest.py lines 788-793, which are the actual openpyxl.load_workbook(...) / worksheet streaming path. Since local and CI environments can skip real XLSX extraction when openpyxl is absent, please add coverage that exercises this branch (either with a tiny XLSX under an environment that has openpyxl, or a focused fake/stub that drives the workbook + worksheet iteration path). Once that path is covered, I do not see another code-level blocker in this head.

Reviewed by Qingyun's Personal Codex.

…tly (#2434)

The hosted diff-coverage gate was failing at 92.4%, missing ingest.py
788-793 (the openpyxl load_workbook -> per-sheet iteration path in
extract_table_csv). The existing xlsx happy-path test skips when openpyxl
is not importable, which is exactly the coverage env, so those lines were
never exercised there.

Add a stub-openpyxl case: inject a fake openpyxl module into sys.modules
so the load_workbook path runs regardless of whether the real dependency
is installed. Verified with openpyxl blocked (simulating CI): lines
788-793 are covered; all 20 --csv checks pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XaocEvWwhBokFUmK5ECsfs
john-the-dev added a commit that referenced this pull request Aug 4, 2026
…tly (#2434)

The hosted diff-coverage gate was failing at 92.4%, missing ingest.py
788-793 (the openpyxl load_workbook -> per-sheet iteration path in
extract_table_csv). The existing xlsx happy-path test skips when openpyxl
is not importable, which is exactly the coverage env, so those lines were
never exercised there.

Add a stub-openpyxl case: inject a fake openpyxl module into sys.modules
so the load_workbook path runs regardless of whether the real dependency
is installed. Verified with openpyxl blocked (simulating CI): lines
788-793 are covered; all 20 --csv checks pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XaocEvWwhBokFUmK5ECsfs
@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Pushed 9d059614 addressing the diff-coverage blocker (the only item left after your P1 was resolved).

What was uncovered: ingest.py 788-793 — the openpyxl.load_workbook → per-sheet iteration path in extract_table_csv. The existing xlsx happy-path test skips when openpyxl isn't importable, which is exactly the hosted coverage env, so those lines were never exercised there → 92.4%.

Fix: a host-independent stub-openpyxl case that injects a fake openpyxl into sys.modules, so the load_workbook path runs whether or not the real dep is installed (same ethos as the zip-bomb test that blocks the import to prove the preflight fires).

Evidence — coverage with openpyxl BLOCKED (simulating the CI env):

$ python3 -m coverage run --include="*ingest.py" block_and_run.py   # meta_path blocks openpyxl
ok   xlsx (stub openpyxl): per-sheet markers — covers load_workbook path
ok   xlsx (stub openpyxl): exact aggregate over emitted CSV (4+14+8=26)
All doc-ingest --csv checks passed.

ingest.py missing among 788-793: NONE (all covered)

All 20 --csv checks pass locally (tests/doc-ingest-csv-mode.test.py). Awaiting the hosted diff-cover re-run on 9d059614.

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

✅ Follow-up: the hosted diff-cover re-run on 9d059614 is now SUCCESS, and all other required checks (tsc + tests (clean install), smoke, etc.) are green — nothing failing or pending. Since your last CR was "Blocking on CI/test coverage only," this should be ready for re-review. Thanks @qingyun-wu!

@john-the-dev
john-the-dev requested a review from qingyun-wu August 4, 2026 03:44

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved for this head. The new stubbed openpyxl coverage exercises the load_workbook(...).worksheets -> iter_rows(values_only=True) path even on hosts where the real dependency is absent, which closes the diff-coverage hole without weakening the no-openpyxl refusal contract. I also rechecked the source/output/cell budget behavior and the explicit --csv-no-budget override path.

Local checks I ran:

  • python3 tests/doc-ingest-csv-mode.test.py
  • PYTHONPYCACHEPREFIX=/private/tmp/pycache-pr2434 python3 -m py_compile skills/doc-ingest/scripts/ingest.py tests/doc-ingest-csv-mode.test.py
  • python3 -m ruff check skills/doc-ingest/scripts/ingest.py tests/doc-ingest-csv-mode.test.py
  • git diff --check origin/main...HEAD

Hosted tsc + tests and diff coverage were also green when I checked this head.

Reviewed by Qingyun's Personal Codex.

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

@bassilkhilo-ag2 your CHANGES_REQUESTED (Aug 3) was on the branch being CONFLICTING after parent #2377 landed — specifically the stale ingest.py (old extract_pdf(path), no _ResourceBudgetError, missing the MAX_DOCUMENT_*/TEXT_COMMAND_TIMEOUT_SECONDS constants). That's all reconciled now on head 9d059614:

  • extract_pdf(path: Path, max_chars: int = DEFAULT_MAX_CHARS) — the new signature is present (ingest.py:378)
  • _ResourceBudgetError class present (ingest.py:61); MAX_DOCUMENT_TEXT_CHARS/TEXT_COMMAND_TIMEOUT_SECONDS present (ingest.py:36/38)
  • mergeable=MERGEABLE — no 3-way conflicts remain (the --csv streaming/budget work now sits on top of the parent's refactor, not against a stale merge-base)

@qingyun-wu independently APPROVED this exact head (9d059614) on Aug 4 03:47Z, and all required checks are green (diff-cover re-run SUCCESS, tsc + tests, smoke). State shows BEHIND only in the routine sense — a pre-merge update-branch, no conflicts.

Could you re-review or dismiss the stale CHANGES_REQUESTED when you have a moment? It's the only thing left holding merge.

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent verification at head 9d059614. The coverage blocker is cleared, and CI already agrees — blocked 2026-08-03, fix pushed 2026-08-04T03:34, nothing appears to have re-triggered a look since. Not a formal approval; note at the end.

Blocker — "diff coverage gate still failing at 92.4%, missing ingest.py 788-793 (the openpyxl.load_workbook / worksheet streaming path)".

The gate itself now reports:

diff coverage >= 95% (python): SUCCESS
failing checks at this head: 0

And the fix is the shape you asked for — a stub rather than a real XLSX, chosen for the right reason. tests/doc-ingest-csv-mode.test.py:119:

# 3b. Host-independent coverage of the openpyxl load_workbook path (ingest.py
# 788-793). The happy-path test above SKIPS when openpyxl is not importable —
# which is exactly the hosted diff-coverage env — leaving 788-793 uncovered
# (qingyun CR #2434). Inject a stub openpyxl so the load_workbook -> per-sheet
# iteration path executes regardless of whether the real dep is installed.

That comment names the actual failure mode rather than just satisfying the percentage: the pre-existing xlsx test skips when openpyxl is absent, and the hosted coverage environment is precisely such a host — so the branch was uncovered because the test that would cover it silently opted out. A stub is the correct answer to that, not a workaround for it.

Suite passes locally at this head:

All doc-ingest --csv checks passed.

Still outstanding from the review: the branch is BEHIND. Also worth noting for whoever merges — the title says "stacked on #2377", so merge order matters if that parent is still open.

On the approval: this repo's gh credential is the owner's identity, so an APPROVE from me spends his rather than adding an independent one. Recording as a comment; the re-review is @qingyun-wu's to clear.

Stand: Echo Act IV Mini

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at exact head 9d059614ba6e6c92fa3e2eecc07e2dc18034c052, superseding my 09:11:31Z CHANGES_REQUESTED.

My blocking claim was specific and falsifiable: ingest.py on this branch was missing symbols that had landed on main via the parent (extract_pdf(path, max_chars=...), _ResourceBudgetError, MAX_DOCUMENT_TEXT_CHARS, MAX_DOCUMENT_BYTES, TEXT_COMMAND_TIMEOUT_SECONDS). Checked directly at this head — every one of those is now present with the reconciled signature. mergeable is MERGEABLE (was CONFLICTING). The divergence is genuinely resolved, not just conflict-marker-free.

Ran the full test suites from a fresh clone (pull/2434/head), not trusting CI status alone:

  • tests/doc-ingest.test.py: 59/59 checks pass.
  • tests/doc-ingest-csv-mode.test.py: all 13 checks pass, including the budget/zip-bomb/multibyte-byte-budget cases.

qingyun-wu holds a formal APPROVED at this head already; my own block was the only other formal state and it's now resolved.

Approving.

@john-the-dev
john-the-dev enabled auto-merge (squash) August 5, 2026 13:29

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 24f71057b8bdd414e0735055c5dbc90c71dad273.

No PR-specific blocking findings. This head is merge-only relative to the approved doc-ingest CSV/XLSX budget repair; the topic files are unchanged from 9d059614.

Focused verification:

  • git diff --check origin/main...HEAD
  • confirmed no changed-file delta from 9d059614 to this head in skills/doc-ingest/SKILL.md, skills/doc-ingest/scripts/ingest.py, or tests/doc-ingest-csv-mode.test.py
  • python3 tests/doc-ingest-csv-mode.test.py — PASS

I am leaving this as a review comment rather than a formal fresh approval because hosted checks are not green yet: the inherited refuse bridge tests that read host config guard is red, and the broad test/coverage jobs are still running.

Reviewed by Qingyun's Personal Codex.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head be96f903b3eb2455d94e9044cb06f0522a5695fa.

No PR-specific blocking findings. This is another merge-only refresh after my 24f71057 comment; the doc-ingest topic files are unchanged from that head, and the effective PR diff is still the approved CSV/XLSX budget repair.

Focused verification:

  • git diff --check origin/main...HEAD
  • confirmed no changed-file delta from 24f71057 to this head in skills/doc-ingest/SKILL.md, skills/doc-ingest/scripts/ingest.py, or tests/doc-ingest-csv-mode.test.py
  • python3 tests/doc-ingest-csv-mode.test.py — PASS

I am leaving this as a review comment rather than a formal fresh approval because hosted checks are not green yet: the inherited refuse bridge tests that read host config guard is red, and the broad test/coverage jobs are still running.

Reviewed by Qingyun's Personal Codex.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head a7bea1abbe36090ff31182668d3916548cebc4d7.

No blocking findings. This is a merge-only refresh incorporating #2674; the complete doc-ingest CSV/XLSX topic patch is byte-for-byte identical to the previously approved refresh. The focused CSV-mode suite passed on that unchanged topic, including the budgeted XLSX path and explicit override behavior. The bounded defaults remain intact, so the worst-case resource exposure is still opt-in through the documented no-budget override.

APPROVED for code. Merge-ready only after hosted checks are green and the repository's second current-head maintainer approval requirement is satisfied.

Reviewed by Qingyun's Personal Codex.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 99e2e3fafe9e9d0bf5442d55312ca70f4444c650.

No blocking findings. This is a merge-only refresh of the previously approved doc-ingest CSV/XLSX topic through #2640; the three effective topic files have no changed-file delta from reviewed head a7bea1abbe36090ff31182668d3916548cebc4d7. The bounded defaults remain intact, and the no-budget behavior remains explicit opt-in.

Focused verification:

  • git diff --check origin/main...HEAD
  • python3 tests/doc-ingest-csv-mode.test.py - PASS
  • python3 tests/doc-ingest.test.py - PASS

APPROVED for code. Merge-ready only after hosted checks finish green and the repository's current-head maintainer approval requirements are satisfied.

Reviewed by Qingyun's Personal Codex.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 1bf63f1da1377feeb90d8f351b070babbd8bfb35.

No blocking findings. This is another merge-only refresh of the doc-ingest CSV/XLSX topic; the three effective topic files have no changed-file delta from the just-reviewed 99e2e3fafe9e9d0bf5442d55312ca70f4444c650 head. The bounded defaults and explicit --csv-no-budget opt-out remain unchanged.

Focused verification:

  • git diff --check origin/main...HEAD
  • python3 tests/doc-ingest-csv-mode.test.py - PASS
  • python3 tests/doc-ingest.test.py - PASS

APPROVED for code. Merge-ready only after hosted checks finish green and the repository's current-head maintainer approval requirements are satisfied.

Reviewed by Qingyun's Personal Codex.

@john-the-dev
john-the-dev force-pushed the feat/doc-ingest-compute-over-tables branch from 1bf63f1 to 48d79b8 Compare August 5, 2026 14:44
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

@john-the-dev
john-the-dev merged commit 9f0c17f into main Aug 5, 2026
16 checks passed
@john-the-dev
john-the-dev deleted the feat/doc-ingest-compute-over-tables branch August 5, 2026 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ag2product AG2 product related (packages, PyPI, task-relay)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants