Skip to content

fix(audit): kb.audit with tail=0 returns the entire log instead of nothing - #644

Merged
plind-junior merged 2 commits into
vouchdev:testfrom
kai392:fix/critical-issue-zero-tail-returns-all
Jul 30, 2026
Merged

fix(audit): kb.audit with tail=0 returns the entire log instead of nothing#644
plind-junior merged 2 commits into
vouchdev:testfrom
kai392:fix/critical-issue-zero-tail-returns-all

Conversation

@kai392

@kai392 kai392 commented Jul 30, 2026

Copy link
Copy Markdown

What changed

the kb.audit tail window moves into audit.tail_events, which returns
nothing for a non-positive tail, and the three surfaces call it instead of
each slicing for themselves. retrieval_events.read_events gets the same
guard at its own [-limit:].

Why

the window was events[-tail:]. that reads as "the last n" and is wrong at
the boundary — -0 is 0, so a zero tail slices from the start and returns
the entire visible log, the opposite of the bound the caller asked for. a
negative tail is worse: it drops that many events off the front and returns
everything after.

on test with five events in the log:

  tail=  2 -> returned  2 events
  tail=  1 -> returned  1 events
  tail=  0 -> returned  5 events   <-- WRONG
  tail= -1 -> returned  4 events   <-- WRONG

tail is caller-supplied on every surface — kb_audit(tail: int = 50) in the
mcp tool, int(p.get("tail", 50)) in the jsonl handler, --tail on the cli —
so this is reachable input, not an internal invariant. asking for a bounded
window and getting an unbounded dump of the audit log is the wrong direction
for a read the scoping layer exists to keep narrow.

the expression was copy-pasted into all three handlers, which is the surface
drift CLAUDE.md calls the most common contributor mistake, and the same shape
kb.search was consolidated for in #476. so the clamp lands once and the
three call it rather than being patched three times.

retrieval_events.read_events carried the identical [-limit:] with a
limit >= 0 guard that lets zero through. it is the same defect rather than a
second concern, so it is fixed here; say the word if you would rather it were
split out.

What might break

nothing on disk, no method signature or response-shape change. a positive
tail behaves exactly as before — tail_events(events, 2) is still the last
two. the only behaviour change is at the boundary: tail<=0 now yields an
empty list where it previously yielded the whole log (or all-but-n). anyone
who was passing 0 to mean "everything" was relying on a bug, and kb.audit
has no documented spelling for that.

VEP

not a surface change — no vep.

Tests

  • make check passes locally (lint + mypy + pytest)

  • New / changed behaviour has a test

  • CHANGELOG.md updated under ## [Unreleased]

  • tests/test_audit.py::test_tail_events_never_widens_the_window
    parametrised over 3, 1, 99, 0, -1, so the positive cases pin the existing
    behaviour and the boundary cases pin the fix.

  • tests/test_audit.py::test_tail_events_keeps_the_newest — the window is the
    newest end, not the oldest.

  • tests/test_jsonl_server.py::test_jsonl_audit_zero_tail_returns_no_events
    the same assertion through handle_request, so it fails on the previous
    code at the surface rather than only against the new helper.

validation run:

python -m ruff check src tests
python -m mypy src
python -m pytest tests/test_audit.py tests/test_audit_scoping.py \
                 tests/test_jsonl_server.py tests/test_cli.py -q

no ui files touched, so no screenshots.

@kai392
kai392 requested a review from plind-junior as a code owner July 30, 2026 15:20
@github-actions github-actions Bot added docs documentation, specs, examples, and repo guidance cli command line interface mcp mcp, jsonl, and http surfaces storage kb storage, migrations, schemas, and proposals tests tests and fixtures size: S 50-199 changed non-doc lines labels Jul 30, 2026
@kai392
kai392 force-pushed the fix/critical-issue-zero-tail-returns-all branch from e0b326b to de2b59d Compare July 30, 2026 15:21
`kb.audit` windowed with `events[-tail:]`. that reads as "the last n" and is
wrong at the boundary: `-0` is `0`, so `tail=0` slices from the start and
returns every event the viewer can see — the opposite of the bound asked
for. a negative tail is worse, dropping that many off the front and
returning the rest.

`tail` is caller-supplied on all three surfaces — the mcp tool signature,
the jsonl `params.tail`, and the cli `--tail` — and the expression was
copy-pasted into each, which is the surface drift CLAUDE.md warns about. so
the clamp lands once in `audit.tail_events` and all three call it.

`retrieval_events.read_events` had the same `[-limit:]` boundary, guarded by
`limit >= 0` which lets zero through. fixed alongside, since it is the same
defect rather than a second concern.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes a boundary-condition bug where kb.audit with tail=0 (or negative) could widen the result window and return the entire visible audit log, by centralizing the tail-window logic and applying the same guard to retrieval event log reads.

Changes:

  • Introduce audit.tail_events() to ensure non-positive tail returns an empty list (never widens the window).
  • Update MCP (server.py), JSONL (jsonl_server.py), and CLI (cli.py) kb.audit surfaces to delegate tail-windowing to audit.tail_events().
  • Add regression tests for tail<=0 behavior and update retrieval_events.read_events(limit=...) to avoid the same [-0:] widening; document the fix in CHANGELOG.md.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/test_jsonl_server.py Adds a JSONL-surface regression test asserting kb.audit with tail=0 returns no events.
tests/test_audit.py Adds unit tests pinning audit.tail_events behavior for positive and non-positive tails and ensuring “newest N” selection.
src/vouch/server.py Switches MCP kb_audit to use audit.tail_events instead of events[-tail:].
src/vouch/retrieval_events.py Prevents limit=0 (and other non-positive limits) from returning the whole retrieval-events log via [-0:].
src/vouch/jsonl_server.py Switches JSONL kb.audit handler to use audit.tail_events instead of events[-tail:].
src/vouch/cli.py Switches CLI vouch audit to use audit_mod.tail_events instead of events[-tail:].
src/vouch/audit.py Adds tail_events(events, tail) helper implementing the non-widening tail window semantics.
CHANGELOG.md Records the user-visible behavior change for kb.audit tail=0 and the related retrieval-events limit fix.

@plind-junior
plind-junior enabled auto-merge July 30, 2026 18:56

@plind-junior plind-junior left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Nice catch!!!

@github-actions
github-actions Bot disabled auto-merge July 30, 2026 19:11
@plind-junior
plind-junior enabled auto-merge July 30, 2026 19:14
@plind-junior
plind-junior merged commit 63eea98 into vouchdev:test Jul 30, 2026
15 checks passed
@github-actions github-actions Bot added the ci: passing ci is green label Jul 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

diff coverage: 100% — every python line this PR changes under src/vouch/ is executed by a test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci: passing ci is green cli command line interface docs documentation, specs, examples, and repo guidance mcp mcp, jsonl, and http surfaces size: S 50-199 changed non-doc lines storage kb storage, migrations, schemas, and proposals tests tests and fixtures

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants