Every grounded review ships the record of what it opened

[jschatz1]

Stacked on #96 (same bundle struct; that one should land first). Base retargets to main automatically when #96 merges.

The facts were already there. rcRunReviewAgent gathers them on every run — its own comment says so:

Facts about how the run ENDED, gathered whether or not it wrote itself down. […] they are collected unconditionally and cost nothing.

and then throws them away unless the review died.

So a review that finished said nothing about its own coverage, and the only account of scope on a healthy review was the model's Scope: paragraph. That is prose, and prose can be wrong about itself — reviews on kai-desktop#288 and kai-desktop#300 named the repository they were reading as kai-engine and kai-server respectively.

rcCoverage is the half that cannot hallucinate: files opened, turns, seconds. It now rides in the bundle on every grounded run.

Absent is not empty

Nil in, nil out. The fast pass makes one call over the diff and opens nothing, so omitempty drops the field entirely rather than publishing a record that claims zero files were read. An absent manifest is not a claim of no coverage, and the server-side companion treats it that way.

Companion

kaicontext/kai-server#213 renders it. That side degrades gracefully on bundles from a kai predating this change, so the two are not release-coupled.

Verified

TestCoverageShipsOnEveryGroundedRun covers the conversion and the nil case. go test ./cmd/... green (exit 0).

🤖 Generated with Claude Code

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: aad554e9-64f6-4365-9618-25fd252ee10e

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

jschatz1 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

[kaicontext]

Kai review

Kai Summary

Read through this one. 3 things worth your eyes before it merges. 👇

Review: cmd/kai/review_commit.go — coverage record ships on every grounded run

Scope: cmd/kai/review_commit.go and cmd/kai/review_commit_incomplete_test.go in this repo. I could not read github.com/kaicontext/kai-engine/finding's Finding struct — it is an engine dependency pinned at v0.6.56, not vendored or checked into this workspace tree — so anything depending on its field set is outside what I can confirm here.

The rest of the read-through

What this does: It adds a Coverage *rcCoverage field to the JSON bundle that review-commit emits on every grounded run, populated from the rcIncomplete record that rcRunReviewAgent already builds unconditionally. A small rcCoverageOf converter turns *rcIncomplete into *rcCoverage, with nil-in/nil-out semantics so the fast pass (which opens nothing) omits the field via omitempty rather than falsely claiming zero files were read. The intent is sound, the code is tiny, and the nil contract is correct and tested.

The wiring is not tested end-to-end. TestCoverageShipsOnEveryGroundedRun exercises rcCoverageOf in isolation — it asserts the converter produces the right Seconds, Turns, and FilesRead and that nil stays nil. That is a unit test of the helper. Nothing in this diff marshals the production bundle struct (the anonymous struct with embedded finding.Finding at review_commit.go:454) and asserts that a "coverage" key actually appears in the emitted JSON. The sibling TestIncompleteBundleCarriesTheFlag defines its own local bundle type that does not include Coverage, so it would not catch the wiring being dropped either. Concretely: if the rcCoverageOf(inc) argument were removed from the marshal literal at line 460, every test in this diff still passes. That is the shape of a test that passes on the unfixed code — the change's claim is "coverage now ships in the bundle," and no test asserts the bundle ships it. A test that builds the real production struct with a non-nil inc and asserts strings.Contains(out, "coverage") would close the gap. This is local and quick to add.

Possible JSON field collision I could not rule out. The bundle is an anonymous struct embedding finding.Finding plus four sibling fields tagged review, depth, incomplete, coverage. Go's encoding/json silently drops same-named tagged fields at the same nesting level. The three existing tags work, which is evidence finding.Finding has no conflicting tag for those names — but coverage is a new name, and the Finding type lives in the engine module I cannot read from this workspace. If finding.Finding ever defines a json:"coverage" tag or an untagged Coverage field that a future engine change promotes, the new field would silently vanish from every bundle rather than erroring. Low probability, but the failure mode is exactly the silent absence the change is meant to prevent. A one-line check against the engine's Finding definition would settle it.

The fast-pass nil contract is sound. inc is only assigned on the grounded path (line 296); the fast path leaves it nil (line 274), rcCoverageOf(nil) returns nil, and omitempty drops the field. The author's framing — absent manifest is not a claim of no coverage — is correct. The server-side companion handling is described in the commit message but lives outside this repo, so I take that on the author's word.

Seconds rounding is correct. int(inc.Elapsed.Round(time.Second).Seconds()) is tested at 4m19s → 259, and matches the existing time.Round(time.Second) idiom used elsewhere in the file. No off-by-one concern.

This is a small, well-scoped change. The only real gap is that the test proves the converter works but not that the bundle carries its output. Add one marshal-level assertion and confirm finding.Finding has no coverage tag, and it is ready.

Important files changed

File	Change
cmd/kai/review_commit.go	modified · +38 −4
cmd/kai/review_commit_incomplete_test.go	modified · +28 −0

Decisions

Correct as written, but somebody should say yes to these:

• Publishing a machine-written coverage manifest (files opened, turns, elapsed seconds) on every grounded review changes what kai findings get and the inbox expose to readers of a review; it affects every author whose review is ingested, by making a review's actual reading-scope inspectable where previously only the model's prose described it.

_{+66 −4 · 2 files · reaches 9 · the full analysis
💬 Reply to any of my comments and I'll answer, or say @kaicontext anywhere on this PR — a question, or "take another look at the retry logic".}

[kaicontext]

the new test asserts rcCoverageOf's output but never marshals the production bundle struct, so removing the rcCoverageOf(inc) call from the marshal literal at review_commit.go:460 would leave every test green; add an end-to-end assertion that the emitted JSON contains "coverage".

[kaicontext]

the new coverage JSON tag could silently collide with a same-named field on the embedded finding.Finding (engine module v0.6.56, not readable from this workspace); confirm the engine's Finding has no coverage tag or untagged Coverage field, since encoding/json would drop both rather than error.

[greptile-apps]

jschatz1 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

[kaicontext]

Kai review

Kai Summary

Read through this one. One thing worth your eyes before it merges, plus 1 decision to say yes to. 👇

Where I'd land: 3/5 — small fixes first.

Scope: cmd/kai/review_commit.go and cmd/kai/review_commit_incomplete_test.go at the working tree (last commit c4f0547). I did not read the server-side companion that ingests the bundle; I take the author's word that it treats an absent coverage field as "no claim," which the code itself cannot confirm.

The rest of the read-through

What this does

rcCoverageOf(inc) projects the run's own facts (files opened, turn count, elapsed seconds) — already gathered unconditionally into rcIncomplete on every grounded review — into a new *rcCoverage field on the JSON bundle. Nil-in/nil-out, plus omitempty, means the fast pass (which never builds an inc) emits no coverage field at all rather than a record claiming zero files. The production wiring is correct:

• inc is declared var inc *rcIncomplete at line 276 and only assigned on the grounded path (via rcRunReviewAgent). The fast branch leaves it nil.
• rcCoverageOf(nil) returns nil; *rcCoverage with json:"coverage,omitempty" drops a nil pointer entirely. So a fast review's bundle is byte-identical to before, and a grounded review always carries coverage (since inc is non-nil whenever the grounded path runs, line 759).
• The Seconds rounding (int(inc.Elapsed.Round(time.Second).Seconds())) is correct for the test's 4m19s → 259. Note Round rounds to nearest rather than truncating, so 1.4s reports 1 and 1.6s reports 2 — fine for a courtesy field.

Concern

The test does not test what its name claims. TestCoverageShipsOnEveryGroundedRun only exercises rcCoverageOf in isolation — it never marshals the bundle struct at line 456-462. Remove the Coverage *rcCoverage field and the rcCoverageOf(inc) argument from that struct literal and this test still passes green, because nothing it touches is the marshal path. This is the "test that passes on the unfixed code" shape: the behavior the change is named for — coverage shipping in the bundle on a healthy grounded run — is asserted nowhere. The existing TestIncompleteBundleCarriesTheFlag is the right template; it marshals the real struct shape. The coverage test should do the same rather than stopping at the converter. To catch a regression it would need to build a completed grounded finding, marshal it through the same struct literal, and assert "coverage" and "filesRead" appear in the JSON — and that a fast-pass marshal omits them.

That's the only real issue, and it's local. The production wiring is sound, the nil/omitempty reasoning holds, and the slice aliasing (rcCoverageOf assigns inc.FilesRead by reference rather than copying) is harmless because rcIncomplete is built fresh per run and discarded after the single marshal.

Decision

The coverage record now ships on every grounded review bundle that kai-server ingests and surfaces in the inbox/kai findings get. That's a new persistent field on every finding the pipeline stores, read by a server-side companion I could not inspect. If that consumer does not in fact treat an absent coverage field as "no claim" (the author asserts it does), a fast-pass finding would be indistinguishable from a grounded one — which is exactly what this change exists to prevent. Worth a one-line confirmation against the server before this lands, but that is outside this repo, not a defect in this diff.

Ready to merge once the test is extended to cover the marshal path; the code itself is sound.

Important files changed

File	Change
cmd/kai/review_commit.go	modified · +38 −4
cmd/kai/review_commit_incomplete_test.go	modified · +28 −0

Decisions

Correct as written, but somebody should say yes to these:

• Every grounded review's bundle now carries a new persistent coverage field (files read, turns, seconds) ingested and surfaced by kai-server; a fast pass omits it by design. The correctness of the fast/grounded distinction rests on the server-side consumer treating an absent field as "no claim," which the author asserts but this repo cannot confirm.

_{+66 −4 · 2 files · reaches 10 · the full analysis
💬 Reply to any of my comments and I'll answer, or say @kaicontext anywhere on this PR — a question, or "take another look at the retry logic".}

[kaicontext]

TestCoverageShipsOnEveryGroundedRun only unit-tests rcCoverageOf and never marshals the bundle struct literal at review_commit.go:456, so it would pass with the Coverage field removed entirely; the "ships on every grounded run" behavior is not actually tested.