Skip to content

review-commit: don't panic in the blast step when there is no graph DB - #216

Open
jschatz1 wants to merge 1 commit into
mainfrom
fix/review-blast-nil-graph
Open

jschatz1 wants to merge 1 commit into
mainfrom
fix/review-blast-nil-graph

Conversation

@jschatz1

@jschatz1 jschatz1 commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

What broke

kai review-commit <sha> (the fast pass) printed its review, then panicked with a nil pointer dereference in the blast-radius step: runReviewCommit → reviewanalyze.BlastFor(ctx, nil, …) → safetygate.Reachback → (*graph.DB).GetEdgesToByPath. Reproduced 2026-10-24 with release v0.36.34 in ~/kai/kai-server-wt-prices (a kai-server git worktree with no .kai) on 7323f714 and af83f1a4.

Why

The guard was set != nil. But projects.Discover returns OutcomeRootsFound whenever an ancestor kai.projects.yaml exists (here ~/kai/kai.projects.yaml). For a cwd the yaml doesn't list, Primary() returns a synthetic project for the cwd. Set.Open never opens a DB for it, and it also leaves a pinned-but-never-captured project's DB nil. So set was non-nil while set.Primary().DB was nil.

Fix

The blast step moves into rcBlast, which returns an empty panel when there is no set, no primary, or no DB. This is the same "no graph, no panel" behaviour the uncaptured fast pass already had. The guard lives in kai-cli, the one caller that can pass a nil DB. Hardening BlastFor against nil in kai-engine is optional and not done here.

Tests

  • TestRCBlast_RootsFoundButNoGraphDoesNotPanic builds the real layout: a temp workspace kai.projects.yaml plus an unlisted subdirectory, then calls Discover and Open on it and asserts an empty blast. With the DB == nil check removed it fails with the original nil-pointer panic.
  • go test ./cmd/kai passes.
  • End to end, a binary built from this branch ran review-commit 7323f714 in the same kai-server worktree that crashed and exited 0.

Could CI review pods hit this?

Normally no. The CI fast pass (review_default_workflow.go) runs in a fresh mktemp -d clone before kai init. There is no ancestor kai.projects.yaml, so Discover finds nothing, set is nil, and the old guard already held. The pod could hit it only if the reviewed repo commits its own kai.projects.yaml: its pinned entries count as roots with no DB. Even then the step is non-fatal in CI. The fast pass would panic before printing its JSON, the workflow logs "fast pass failed", and the grounded --deep pass still runs. This was mainly a local-run crash, for worktrees and for checkouts under a workspace yaml.

Not merging and not releasing. That's Jacob's call.

🤖 Generated with Claude Code


Kai context

Written by Kai from the change itself. Kai only edits this section; everything above it is the author's.

Fixes a nil-pointer panic in kai review-commit's blast-radius step that occurred when projects.Discover reported roots but the primary project had no db.sqlite, causing BlastFor to dereference a nil DB after the review printed.

Changes

  • Extracted the blast-radius step from runReviewCommit into a new rcBlast function in cmd/kai/review_commit.go that returns an empty finding.Blast when set is nil, set.Primary() is nil, or the primary's DB is nil, preventing the dereference in reviewanalyze.BlastFor.

Testing

  • Added cmd/kai/review_commit_blast_test.go with TestRCBlast_RootsFoundButNoGraphDoesNotPanic, which builds a temp workspace with a kai.projects.yaml listing a different directory, runs Discover and Open from an unlisted subdirectory, and asserts rcBlast returns an empty panel; also added TestRCBlast_NoSet covering the nil-set case.

The fast pass guarded the blast-radius step on set != nil only. Discover
can report roots while the primary project has no db.sqlite (a checkout
under a kai.projects.yaml that does not list it, e.g. a git worktree next
to its repo, or a pinned project never captured); Set.Open leaves that
DB nil and BlastFor dereferenced it, so the review printed and then
panicked. rcBlast now returns an empty panel when there is no DB.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kaicontext kaicontext Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewView in KaiKai review · Ready to merge (10/10)

Read the whole thing — it does what it says, and nothing jumped out at me.

Next: merge when your checks are green — I found nothing to fix.

Important files changed
File Change
cmd/kai/review_commit.go modified · +28 −15
cmd/kai/review_commit_blast_test.go modified · +53 −0
How I reviewed it — 8 files, 21 turns, 1m19s

0 confirmed findings, 1 refuted. Intent verified.

What I opened

  • api/projects/projects.go
  • cmd/kai/agentsvc.go
  • cmd/kai/gate.go
  • cmd/kai/review_analyze.go
  • cmd/kai/review_commit.go
  • cmd/kai/review_commit_blast_test.go
  • cmd/kai/tui.go
  • go.mod
Full read-through

Scope

  • cmd/kai/review_commit.go rcBlast and its call site
  • cmd/kai/review_commit_blast_test.go
  • Open/Close usage in sibling callers (tui.go, gate.go, agentsvc.go)

No proposed defect was confirmed by this check within the reviewed scope.

Size S · Risk M · +81 −15 · 2 files · reaches 63 · 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".

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant