From da9d971d67fc8ec620ea28d5ef5e63e2f641d4b4 Mon Sep 17 00:00:00 2001 From: lucasmaan <305449091+lucasmaan@users.noreply.github.com> Date: Wed, 26 Aug 2026 17:35:02 +0800 Subject: [PATCH] feat(check): cap every finding list in the JSON payload Pointed at a 1024-document knowledge base, `check` printed 482KB of JSON, of which 1024 rows were `{"document": ..., "reason": "missing"}` -- the same reason every time, for a condition check_extractions itself documents as not a fault. The one thing the run had to say (0 mismatched, and grounding unable to check any of the 698 articles it found) was buried in it. Every finding list is now cut to 20 items by default. The cap is a display limit and never a measurement: `count` stays the true total whatever --limit does, and `truncated` says outright that rows were dropped, so a capped list cannot be misread as a complete one. `--limit 0` restores the full payload. The cap is uniform, so the actionable lists (`mismatched`, `unsourced`) are cut at the same 20 as the benign `missing` that motivated it -- the right default, since a KB with thousands of genuine faults has a bigger problem than its report length. Co-Authored-By: Claude Opus 5 (1M context) --- docs/agent-quickstart.md | 8 ++ py/src/kb_ai/commands/check.py | 91 ++++++++++++++--- py/tests/test_commands_check.py | 172 +++++++++++++++++++++++++++----- 3 files changed, 230 insertions(+), 41 deletions(-) diff --git a/docs/agent-quickstart.md b/docs/agent-quickstart.md index f95a4b0..e6a994e 100644 --- a/docs/agent-quickstart.md +++ b/docs/agent-quickstart.md @@ -109,6 +109,14 @@ was built (`unknown` for a KB that was not derived), how far the wiki is behind the prompts that produced it, and whether any article names something that appears in none of its sources. Safe to point at a read-only KB or someone else's. +Each list in the JSON is cut to 20 items by default, because on a 1024-document +knowledge base the full payload ran to 482KB and buried the four lines that +mattered. Nothing is lost to the cut: every list reports the `count` it was cut +from and sets `truncated`, and the stderr summaries always speak for the whole +set. Pass `--limit 0` for every row, or `--limit N` for a different slice — worth +doing when the lists you care about are the actionable ones (`mismatched`, +`unsourced`), since they are capped at the same 20 as the benign ones. + The lag half is what a `compile` cannot tell you: editing a write prompt changes no document, so the next compile finds nothing to do and reports nothing. The counts are report-only — re-composing an article adds to it rather than replacing diff --git a/py/src/kb_ai/commands/check.py b/py/src/kb_ai/commands/check.py index f866937..3c8ca3f 100644 --- a/py/src/kb_ai/commands/check.py +++ b/py/src/kb_ai/commands/check.py @@ -33,6 +33,27 @@ Spends nothing and rewrites nothing, so it is safe to point at a read-only KB or at someone else's. + +One more thing the output has to survive: scale. Pointed at a 1024-document +knowledge base this printed 482KB of JSON, of which 1024 rows were +`{"document": ..., "reason": "missing"}` -- the same reason every time, for a +condition check_extractions itself documents as not a fault. A diagnostic that +takes a screenful to say "nothing is wrong" does not get read, and the one thing +it did have to say (0 mismatched, and grounding able to check none of the 698 +articles it found) was buried in it. + +So every finding list is capped and reports the total it was cut from. The cap +is a display limit, never a measurement: `count` is the real number whatever +--limit does, and `truncated` says outright that rows were dropped. Truncating +into a bare list would be worse than not truncating -- twenty rows that look like +the whole set is a wrong answer, where twenty rows labelled "of 1024" is a short +one. + +The cap is uniform across every list, which means the actionable ones +(`mismatched`, `unsourced`) are cut at the same twenty as the benign `missing` +that motivated it. That is the right default -- a KB with thousands of genuine +faults has a bigger problem than its report length -- but it is why --limit 0 +exists. """ from __future__ import annotations @@ -67,10 +88,43 @@ def _prompt_version(compute) -> str: return "" +DEFAULT_LIMIT = 20 + + +def _capped(items: list, limit: int) -> dict: + """Render one finding list as its total, a shown slice, and whether it was cut. + + limit 0 means no limit. A negative limit never reaches here -- the parser + rejects it, because reading -1 as "unlimited" would hand back the whole + payload to someone who was asking for less of it. + + The slice is copied rather than aliased: the caller's list is a check result, + and handing a live reference to it into the response payload is a + same-object coupling nobody would expect from a function named for cutting. + """ + shown = items[:] if limit == 0 else items[:limit] + return {"count": len(items), "items": shown, + "truncated": len(shown) < len(items)} + + +def nonnegative_int(value: str) -> int: + """argparse renders type.__name__ in its error text, so this name is + operator-facing: `invalid _limit value: 'abc'` leaked a private helper.""" + n = int(value) + if n < 0: + raise argparse.ArgumentTypeError( + f"--limit must not be negative (got {n}); use 0 for no limit") + return n + + def build_parser() -> argparse.ArgumentParser: parser = argparse.ArgumentParser(prog="kb-ai check") parser.add_argument("--kb", default="./.kaas", help="knowledge-base directory to check (default: ./.kaas)") + parser.add_argument("--limit", type=nonnegative_int, default=DEFAULT_LIMIT, + help="how many items to show per finding list; the " + "reported count is always the full total " + f"(default: {DEFAULT_LIMIT}, 0 for no limit)") return parser @@ -109,32 +163,39 @@ def run_check(argv: list[str]) -> None: print(f"[check] wiki: {lag.summary()}", file=sys.stderr) print(f"[check] grounding: {grounding.summary()}", file=sys.stderr) + # Every list below goes through capped(): the summaries above and the count + # inside each wrapper carry the totals, so shrinking the payload cannot shrink + # the answer. + def cap(items: list) -> dict: + return _capped(items, args.limit) + respond_ok(data={ "kb": args.kb, + "limit": args.limit, "extractions": { - "matches": extractions.matches, + "matches": cap(extractions.matches), # Reasons are carried per document rather than summarised: "missing" # and "invalid: counts disagree with body" call for different actions. - "missing": [{"document": rel, "reason": why} - for rel, why in extractions.missing], - "mismatched": [{"document": rel, "reason": why} - for rel, why in extractions.mismatched], + "missing": cap([{"document": rel, "reason": why} + for rel, why in extractions.missing]), + "mismatched": cap([{"document": rel, "reason": why} + for rel, why in extractions.mismatched]), "summary": extractions.summary(), }, "parent": { "source_kb": parent.source_kb, "verdict": parent.verdict, - "in_sync": parent.in_sync, - "changed_in_parent": parent.changed_in_parent, - "gone_from_parent": parent.gone_from_parent, + "in_sync": cap(parent.in_sync), + "changed_in_parent": cap(parent.changed_in_parent), + "gone_from_parent": cap(parent.gone_from_parent), "reason": parent.reason, "summary": parent.summary(), }, # Named rather than counted: the count is what compile already reports, # and what an operator needs here is which articles to re-read. "wiki": { - "behind_extract_prompt": lag.behind_extract, - "behind_write_prompt": lag.behind_write, + "behind_extract_prompt": cap(lag.behind_extract), + "behind_write_prompt": cap(lag.behind_write), "extract_first_run": lag.extract_first_run, "write_first_run": lag.write_first_run, "summary": lag.summary(), @@ -143,14 +204,14 @@ def run_check(argv: list[str]) -> None: # operator's next move is deciding whether the article really claims the # thing, and a bare name sends them opening files to find out. "grounding": { - "checked": grounding.checked, - "unsourced": [{"article": f.article, "name": f.name, "line": f.line} - for f in grounding.unsourced], + "checked": cap(grounding.checked), + "unsourced": cap([{"article": f.article, "name": f.name, + "line": f.line} for f in grounding.unsourced]), # An article that could not be checked is neither clean nor flagged. # Its reason usually points at the extraction layer, which is the # check above. - "skipped": [{"article": rel, "reason": why} - for rel, why in grounding.skipped], + "skipped": cap([{"article": rel, "reason": why} + for rel, why in grounding.skipped]), "summary": grounding.summary(), }, }) diff --git a/py/tests/test_commands_check.py b/py/tests/test_commands_check.py index c69be5a..1456de5 100644 --- a/py/tests/test_commands_check.py +++ b/py/tests/test_commands_check.py @@ -64,8 +64,9 @@ def test_a_kb_whose_extractions_all_match(monkeypatch, tmp_path): assert resp["ok"] is True ext = resp["data"]["extractions"] - assert ext["matches"] == ["raw/a.md", "raw/nested/b.md"] - assert ext["missing"] == [] and ext["mismatched"] == [] + assert ext["matches"]["items"] == ["raw/a.md", "raw/nested/b.md"] + assert ext["matches"]["count"] == 2 + assert ext["missing"]["items"] == [] and ext["mismatched"]["items"] == [] assert "2 match" in ext["summary"] @@ -77,9 +78,9 @@ def test_a_mismatched_extraction_carries_the_document_and_the_reason(monkeypatch resp = _run(monkeypatch, ["--kb", str(tmp_path)]) mismatched = resp["data"]["extractions"]["mismatched"] - assert len(mismatched) == 1 - assert mismatched[0]["document"] == "raw/a.md" - assert "document hashes to" in mismatched[0]["reason"] + assert mismatched["count"] == 1 and len(mismatched["items"]) == 1 + assert mismatched["items"][0]["document"] == "raw/a.md" + assert "document hashes to" in mismatched["items"][0]["reason"] def test_a_missing_extraction_is_reported_without_being_called_a_fault(monkeypatch, @@ -90,7 +91,7 @@ def test_a_missing_extraction_is_reported_without_being_called_a_fault(monkeypat resp = _run(monkeypatch, ["--kb", str(tmp_path)]) assert resp["ok"] is True - assert resp["data"]["extractions"]["missing"] == [ + assert resp["data"]["extractions"]["missing"]["items"] == [ {"document": "raw/a.md", "reason": "missing"}] @@ -118,9 +119,9 @@ def test_a_derived_kb_reports_both_checks(monkeypatch, tmp_path): resp = _run(monkeypatch, ["--kb", str(derived_dir)]) - assert resp["data"]["extractions"]["matches"] == ["raw/a.md", "raw/b.md"] + assert resp["data"]["extractions"]["matches"]["items"] == ["raw/a.md", "raw/b.md"] assert resp["data"]["parent"]["verdict"] == "in_sync" - assert resp["data"]["parent"]["in_sync"] == ["raw/a.md", "raw/b.md"] + assert resp["data"]["parent"]["in_sync"]["items"] == ["raw/a.md", "raw/b.md"] assert resp["data"]["parent"]["source_kb"] == str(parent.base_dir) @@ -132,7 +133,7 @@ def test_a_document_changed_in_the_parent_is_named(monkeypatch, tmp_path): resp = _run(monkeypatch, ["--kb", str(derived_dir)]) assert resp["data"]["parent"]["verdict"] == "changed_in_parent" - assert resp["data"]["parent"]["changed_in_parent"] == ["raw/a.md"] + assert resp["data"]["parent"]["changed_in_parent"]["items"] == ["raw/a.md"] def test_both_summaries_are_printed_for_an_operator_to_read(tmp_path, capsys): @@ -171,8 +172,8 @@ def test_check_names_the_documents_behind_the_write_prompt(monkeypatch, tmp_path resp = _run(monkeypatch, ["--kb", str(tmp_path)]) wiki = resp["data"]["wiki"] - assert wiki["behind_write_prompt"] == ["raw/a.md"] - assert wiki["behind_extract_prompt"] == [] + assert wiki["behind_write_prompt"]["items"] == ["raw/a.md"] + assert wiki["behind_extract_prompt"]["items"] == [] assert "1 behind the write prompt" in wiki["summary"] @@ -183,10 +184,11 @@ def test_check_reports_an_empty_lag_for_a_kb_that_was_never_compiled(monkeypatch resp = _run(monkeypatch, ["--kb", str(tmp_path)]) + empty = {"count": 0, "items": [], "truncated": False} assert resp["data"]["wiki"] == { - "behind_extract_prompt": [], "behind_write_prompt": [], + "behind_extract_prompt": empty, "behind_write_prompt": empty, "extract_first_run": False, "write_first_run": False, - "summary": resp["data"]["wiki"]["summary"]} + "summary": "0 behind the extract prompt, 0 behind the write prompt"} def test_an_unreadable_prompt_set_still_reports_the_checks_that_do_not_need_it( @@ -207,8 +209,8 @@ def boom(): captured = capsys.readouterr() resp = json.loads(captured.out) assert resp["ok"] is True - assert resp["data"]["extractions"]["matches"] == ["raw/a.md"] - assert resp["data"]["wiki"]["behind_write_prompt"] == [] + assert resp["data"]["extractions"]["matches"]["items"] == ["raw/a.md"] + assert resp["data"]["wiki"]["behind_write_prompt"]["items"] == [] assert "write prompt version unavailable" in resp["data"]["wiki"]["summary"] assert "merge-diff" in captured.err @@ -233,8 +235,8 @@ def test_check_names_the_unsourced_items_and_the_line_they_are_on(monkeypatch, resp = _run(monkeypatch, ["--kb", str(tmp_path)]) found = resp["data"]["grounding"] - assert found["checked"] == ["wiki/c.md"] - assert found["unsourced"] == [ + assert found["checked"]["items"] == ["wiki/c.md"] + assert found["unsourced"]["items"] == [ {"article": "wiki/c.md", "name": "Auth", "line": "| `Auth` | bool |"}] assert "1 unsourced" in found["summary"] @@ -247,8 +249,8 @@ def test_check_reports_a_clean_wiki_as_clean(monkeypatch, tmp_path): resp = _run(monkeypatch, ["--kb", str(tmp_path)]) - assert resp["data"]["grounding"]["unsourced"] == [] - assert resp["data"]["grounding"]["skipped"] == [] + assert resp["data"]["grounding"]["unsourced"]["items"] == [] + assert resp["data"]["grounding"]["skipped"]["items"] == [] def test_check_carries_the_reason_an_article_could_not_be_checked(monkeypatch, @@ -259,12 +261,12 @@ def test_check_carries_the_reason_an_article_could_not_be_checked(monkeypatch, resp = _run(monkeypatch, ["--kb", str(tmp_path)]) skipped = resp["data"]["grounding"]["skipped"] - assert len(skipped) == 1 - assert skipped[0]["article"] == "wiki/c.md" - assert "missing" in skipped[0]["reason"] + assert skipped["count"] == 1 and len(skipped["items"]) == 1 + assert skipped["items"][0]["article"] == "wiki/c.md" + assert "missing" in skipped["items"][0]["reason"] # Not counted as a clean article, and not counted as a finding either. - assert resp["data"]["grounding"]["checked"] == [] - assert resp["data"]["grounding"]["unsourced"] == [] + assert resp["data"]["grounding"]["checked"]["items"] == [] + assert resp["data"]["grounding"]["unsourced"]["items"] == [] def test_a_kb_with_no_wiki_yet_reports_an_empty_grounding_check(monkeypatch, @@ -274,9 +276,10 @@ def test_a_kb_with_no_wiki_yet_reports_an_empty_grounding_check(monkeypatch, resp = _run(monkeypatch, ["--kb", str(tmp_path)]) + empty = {"count": 0, "items": [], "truncated": False} assert resp["data"]["grounding"] == { - "checked": [], "unsourced": [], "skipped": [], - "summary": resp["data"]["grounding"]["summary"]} + "checked": empty, "unsourced": empty, "skipped": empty, + "summary": "no articles"} def test_the_grounding_summary_is_printed_for_an_operator_to_read(tmp_path, capsys): @@ -301,6 +304,123 @@ def test_the_command_neither_spends_nor_rewrites(monkeypatch, tmp_path): assert after == before +def test_capped_reports_the_true_total_alongside_the_items_it_shows(): + """A truncated list must not be readable as a complete one. + + The whole point of the cap is that the payload stays small on a large KB, and + the whole risk of a cap is that the reader takes 20 rows for the whole story. + count is the real total whatever the limit does. + """ + got = check_cmd._capped(list(range(100)), 20) + + assert got["count"] == 100 + assert got["items"] == list(range(20)) + assert got["truncated"] is True + + +def test_capped_does_not_claim_truncation_it_did_not_perform(): + under = check_cmd._capped(["a", "b"], 20) + assert under == {"count": 2, "items": ["a", "b"], "truncated": False} + + # Exactly at the limit: nothing was dropped, so nothing may say otherwise. + exact = check_cmd._capped(["a", "b"], 2) + assert exact["truncated"] is False and exact["items"] == ["a", "b"] + + +def test_capped_treats_a_zero_limit_as_no_limit(): + """The escape hatch for anyone who needs every row.""" + got = check_cmd._capped(list(range(50)), 0) + + assert got["count"] == 50 and len(got["items"]) == 50 + assert got["truncated"] is False + + +def test_the_limit_defaults_to_a_value_that_keeps_the_payload_readable(): + assert check_cmd.build_parser().parse_args([]).limit == 20 + + +def test_a_negative_limit_is_rejected_at_the_boundary(capsys): + """Silently reinterpreting -1 as "no limit" would hand back the 482KB the cap + exists to prevent, to someone who thought they were shrinking the output.""" + try: + check_cmd.build_parser().parse_args(["--limit", "-1"]) + except SystemExit: + # Pinned to the validator's own wording. "limit" alone would also match + # argparse's "unrecognized arguments: --limit -1", so this assertion + # would pass on a build where the option does not exist at all. + assert "negative" in capsys.readouterr().err + else: + raise AssertionError("a negative --limit was accepted") + + +def test_a_large_finding_list_is_capped_in_the_payload(monkeypatch, tmp_path): + """On a real KB this list ran to 1024 rows of identical reason, which is how + one diagnostic command came to print 482KB of JSON.""" + # Documents written with no extractions: every one lands in missing. + _kb(tmp_path, {f"raw/d{i:03}.md": f"body {i}" for i in range(25)}) + + resp = _run(monkeypatch, ["--kb", str(tmp_path)]) + + missing = resp["data"]["extractions"]["missing"] + assert missing["count"] == 25 + assert len(missing["items"]) == 20 + assert missing["truncated"] is True + # The summary still speaks for the whole set, not for the shown slice. + assert "25 missing" in resp["data"]["extractions"]["summary"] + + +def test_limit_zero_restores_every_row(monkeypatch, tmp_path): + _kb(tmp_path, {f"raw/d{i:03}.md": f"body {i}" for i in range(25)}) + + resp = _run(monkeypatch, ["--kb", str(tmp_path), "--limit", "0"]) + + missing = resp["data"]["extractions"]["missing"] + assert missing["count"] == 25 and len(missing["items"]) == 25 + assert missing["truncated"] is False + + +def test_no_bare_list_survives_anywhere_in_the_payload(monkeypatch, tmp_path): + """Uniform shape: a reader should not have to remember which lists can be + truncated and which cannot. + + Stated structurally rather than as a whitelist of today's eleven paths. A + whitelist cannot fail when a twelfth check adds an unbounded list, which is + the one regression this test exists to catch -- it would have passed on the + very payload it is supposed to reject. + """ + store = _kb(tmp_path, {"raw/a.md": "a body"}) + _extract(store, "raw/a.md") + _article(store, "wiki/c.md", "| `Auth` | bool |\n", ["raw/a.md"]) + + data = _run(monkeypatch, ["--kb", str(tmp_path)])["data"] + + def bare_lists(node, path="data"): + if isinstance(node, dict): + # A wrapper's own "items" is the shown slice and is meant to be a list. + inner = {k: v for k, v in node.items() + if not (k == "items" and set(node) == {"count", "items", + "truncated"})} + for k, v in inner.items(): + yield from bare_lists(v, f"{path}.{k}") + elif isinstance(node, list): + yield path + + offenders = list(bare_lists(data)) + assert offenders == [], f"unwrapped list(s) in the payload: {offenders}" + + +def test_the_payload_records_the_limit_it_was_produced_under(monkeypatch, tmp_path): + """Without it a reader holding a saved payload cannot tell whether a 20-item + list was capped at 20 or merely happened to have 20 rows.""" + _kb(tmp_path, {"raw/a.md": "a body"}) + + default = _run(monkeypatch, ["--kb", str(tmp_path)]) + assert default["data"]["limit"] == 20 + + unlimited = _run(monkeypatch, ["--kb", str(tmp_path), "--limit", "0"]) + assert unlimited["data"]["limit"] == 0 + + def test_a_path_that_is_not_a_knowledge_base_is_an_error_not_a_clean_bill( monkeypatch, tmp_path): """In the one command built for diagnosis, a typo'd --kb was indistinguishable