Skip to content

test(audit): coverage for schedule CLI, perf-vis, VLM extraction, PDF gen/export; fix silent table-row loss#2259

Merged
kovtcharov-amd merged 6 commits into
mainfrom
audit/coverage-tests
Jul 18, 2026
Merged

test(audit): coverage for schedule CLI, perf-vis, VLM extraction, PDF gen/export; fix silent table-row loss#2259
kovtcharov-amd merged 6 commits into
mainfrom
audit/coverage-tests

Conversation

@kovtcharov

Copy link
Copy Markdown
Collaborator

Closes #1995, #1997, #1998, #2002, #2003 (weekly-audit zero-coverage findings).

Five modules where a parsing or formatting bug would ship silently — the gaia schedule CLI dispatch, the perf-log parser behind gaia perf-vis, the VLM structured-extraction parsers, the synthetic-PDF eval corpus generator, and the summarizer PDF export — now have 102 unit tests. Writing them immediately caught a real production bug: when a VLM wrapped a table's JSON array in any chatter ("Sure! Here's the table: [...]"), extract_json_from_text parsed only the first row object and extract_table silently returned [] — every row dropped, exactly the corruption #1997 warned about. Fixed in gaia.utils.parsing (now extracts top-level arrays too, with regression tests; all existing callers type-check the result, and the EMR suite still passes).

Tests assert the outgoing call shape at mocked boundaries (#1655 rule) — e.g. the schedule tests drive the real argparse parser and assert the constructed Schedule/store arguments, and the PDF tests validate output by reopening the files with pypdf. No LLM or Lemonade server needed anywhere.

Test plan

  • python -m pytest tests/unit/cli/test_cli_schedule.py tests/unit/test_perf_analysis.py tests/unit/test_structured_vlm_extraction.py tests/unit/eval/test_pdf_document_generator.py tests/unit/test_pdf_formatter.py tests/unit/test_file_watcher.py → 162 passed
  • test_extract_table_strips_surrounding_chatter fails without the parsing fix (reverts red)
  • python util/lint.py --all passes

…jects (#1997)

extract_json_from_text only scanned for a balanced {...} block, so when a
VLM wrapped a JSON array in any prose ("Sure! Here's the table: [...]"),
the first row object was parsed instead of the array —
StructuredVLMExtractor.extract_table then saw a dict, logged a warning,
and silently returned [] (every row dropped). Surfaced by the new
structured-extraction unit tests. The scanner now tries whichever of
{ / [ opens first and falls back to the other, with regression tests
for chatter-wrapped arrays and stray prose brackets.
…enerator/formatter (#1995, #1997, #1998, #2002, #2003)

Five zero-coverage modules flagged by the weekly audit now have 102 unit
tests:

- gaia schedule CLI dispatch (#1995): drives the real argparse parser and
  asserts the constructed Schedule / store-call arguments at the schedule-
  package boundary (the #1655 rule), including sink_args from --to and
  mark_run with next_fire_time.
- perf_analysis.py (#1998): regex parsing of Lemonade perf logs, TTFT/TPS
  aggregation, empty/malformed-log behavior, headless (Agg) plot rendering.
- StructuredVLMExtractor (#1997): table/key-value/schema/chart parsers and
  the pure helpers (_parse_page_range, _parse_time_to_hours) with the VLM
  boundary mocked; malformed-output branches exercised explicitly.
- PDFDocumentGenerator (#2002): generates corpora into tmp_path and
  validates the PDFs by reopening them with pypdf.
- PDFFormatter (#2003): summarizer PDF export branches (headings, bullets,
  performance tables, edge inputs) validated by reopening the output.
@github-actions github-actions Bot added the tests Test changes label Jul 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve with suggestions

Strong test-coverage PR (102 new unit tests across five zero-coverage modules) that also fixes a real bug: a VLM wrapping a table's JSON array in chatter used to have every row after the first silently dropped. The parsing fix is sound, and the tests are genuinely good — they assert outgoing call shape at mocked boundaries (the #1655 rule) and reopen generated PDFs with pypdf rather than trusting a mock.

One thing worth addressing: the PR says "all existing callers type-check the result," but the EMR patient-extraction caller does not. extract_json_from_text can now return a list, and that caller calls .get(...) on the result unconditionally — so a VLM that wraps the patient object in an array (e.g. [{...}]) would crash with AttributeError instead of the old behavior of returning the first object. It's an edge path and the EMR suite passes because no test feeds a top-level array, but it's exactly the "green tests, still breaks" gap. A one-line isinstance guard closes it. Not a merge blocker for this test PR, but worth a quick fix here or a fast follow-up.

🔍 Technical details

🟡 Important

EMR caller doesn't guard against the new list return type (hub/agents/python/emr/gaia_agent_emr/agent.py:1098)

The VLM callers in src/gaia/vlm/structured_extraction.py (297/360/438/598) all isinstance-check before use, so they're safe. But _parse_extraction only checks is None and then does result.get("phone"):

result = extract_json_from_text(raw_text)
if result is None:
    logger.warning("No valid JSON found in extraction")
    return None
if not result.get("phone"):   # AttributeError if result is a list

Before this PR, an embedded array returned the first row object (a dict), so this never fired on chatter-wrapped output; now it returns the whole list. A form-extraction VLM that emits [{...}] (a patient object wrapped in an array) would reach this line and crash. Suggested guard:

        result = extract_json_from_text(raw_text)
        if not isinstance(result, dict):
            logger.warning("No valid JSON object found in extraction")
            return None

The Optional[Dict[str, Any]] return annotation on _parse_extraction (line 1095) already promises this. (Strictly, the top-level json.loads(text) path at parsing.py:88 could already return a list for a bare-array input, so this is a pre-existing latent gap — but the PR widens the surface enough that it's worth closing now.)

🟢 Minor

Behavior change when a valid array and valid object both appear (src/gaia/utils/parsing.py:95) — candidates are tried in open-position order, so if a valid JSON array opens before a valid object, the array now wins where the object used to. The dict-expecting callers all isinstance-check and degrade gracefully, so no crash — just worth being aware the "first bracket wins" tie-break is intentional. No change needed.

Strengths

  • The _extract_balanced_json refactor cleanly generalizes the balanced-bracket scan to both {} and [] without duplicating the string/escape handling, and the "scan only { drops array rows" WHY comment names the exact invariant.
  • Tests exercise cold/edge states the mocks would otherwise mask: empty content, missing metadata, malformed perf-log lines, zero-total pie slices, Agg-backend show=True non-hang. test_extract_object_after_prose_bracket specifically covers the stray-[-in-prose regression the new ordering could have introduced.
  • Boundary assertions check the constructed Schedule/store-call arguments and reopen PDFs with pypdf rather than asserting "a mock was called" — the gaia init --profile npu fails with status 400 (missing user. prefix for model name) #1655 discipline applied correctly.

@kovtcharov
kovtcharov enabled auto-merge July 18, 2026 22:55
tests/unit/test_perf_analysis.py imports matplotlib to exercise the
perf-vis plotting functions headlessly, but the unit-test jobs install
".[api]" plus a hand-picked test-dep list that did not include it, so the
module failed to import on all four runners.

matplotlib stays out of the runtime extras deliberately: gaia.perf_analysis
imports it lazily and _require_matplotlib() raises an actionable error when
it is absent, so perf-vis is opt-in rather than a dependency every GAIA user
pays for. Installing it in CI keeps that property while letting the new
coverage execute — guarding the module with importorskip would have silently
skipped it forever, which is indistinguishable from green.

Mirrors the existing pattern in this workflow for pyfakefs/keyring/httpx/respx.
extract_json_from_text can now return a list — that is the point of the
table-row-loss fix — but _parse_extraction called .get() on the result
unconditionally. A VLM that wrapped the patient object in an array (`[{...}]`)
raised AttributeError: 'list' object has no attribute 'get' instead of
returning the object, and a bare JSON scalar failed the same way.

A single-element array is unwrapped, restoring the prior behaviour for this
path. A multi-element array or a scalar is ambiguous for a single patient
record, so it is rejected with an error naming what came back and what to do
about it rather than guessing at a row.

The four structured_extraction.py callers already isinstance-check their
result; this was the only caller that did not.
@github-actions github-actions Bot added devops DevOps/infrastructure changes agent::emr Medical-intake (EMR) agent changes labels Jul 18, 2026
tests/unit/test_structured_vlm_extraction.py exercises the PDF branch of
StructuredExtractor.extract(), which does `import fitz`. The test mocks
fitz.open, but mocker.patch("fitz.open") can only resolve that target when
the module is importable, so the test failed with ModuleNotFoundError.

pymupdf ships in the [ui] and [rag] extras; this job installs ".[api]".
Installing it here keeps it out of the runtime path for users who do not
need PDF support, while letting the new coverage execute. Stubbing
sys.modules instead would have faked the very import under test.

Surfaced once the matplotlib collection error stopped aborting the run.
main rejects `gaia schedule add --skill` up front: such a schedule would
register but never fire while skill-format resolution is blocked on #888.
This test still asserted the older accept-and-store behaviour, so it passed
on the branch in isolation and failed against the PR merge commit that CI
actually builds.

Assert the guard instead — exit code 1, nothing persisted (a stored skill
schedule is precisely the silent-failure case), and an error naming both the
--prompt workaround and the tracking issue.
@kovtcharov-amd
kovtcharov-amd added this pull request to the merge queue Jul 18, 2026
Merged via the queue into main with commit 6c6f8a3 Jul 18, 2026
43 checks passed
@kovtcharov-amd
kovtcharov-amd deleted the audit/coverage-tests branch July 18, 2026 23:44
kovtcharov added a commit that referenced this pull request Jul 18, 2026
@itomek itomek mentioned this pull request Jul 22, 2026
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent::emr Medical-intake (EMR) agent changes devops DevOps/infrastructure changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gaia schedule CLI dispatch (_handle_schedule, shipped 2026-07-07 for #1371) has zero tests, though the underlying schedule package is well covered

2 participants