Review a pull request in kunai - #4
Open
HEGADE wants to merge 17 commits into
Open
Conversation
The design conversation, written down before any of it is built. Records the decisions that delete work rather than add it: a webhook-less GitHub App so reviews are attributed to kunai[bot] without kunai ever exposing a port, a button as the only trigger so there is no polling and no always-on host, and posting as a second click so nothing half-baked reaches a colleague's PR. Also records the three things that decide whether the reviewer is used or muted, none of which are plumbing: prompt discipline about what not to say, a verification pass to kill confident nonsense, and treating a fork's diff as hostile input.
Phase 1 of docs/pr-review.md: identity only, no reviewing yet. The package can list a repository's open pull requests as the App, which is the whole of what this phase claims. The App has no webhook and never will. People assume App means webhooks, but an App registered without one is purely an identity plus a credential, which is what lets kunai post as a bot while still exposing nothing to the internet. Authentication is two hops because GitHub requires it: a short-lived RS256 JWT signs as the App and may only ask which installations exist, and an installation token scoped to one org is what may actually touch a repository. The two are not interchangeable, so the auth mode is named at every call site rather than inferred, because confusing them produces a 403 that reads like a permissions problem. Three details are load-bearing and are what the tests are about. The JWT backdates iat and claims nine minutes rather than ten, because GitHub judges both against ITS clock and a machine running slightly fast otherwise mints a token that is already invalid on arrival. Installation tokens refresh two minutes early, because a token that lapses between the check and the request fails a review that has already spent real quota. And a rejected call's real reason is in GitHub's per-field errors, not the top-level message, which says only "Validation Failed": that is where "line must be part of the diff" lives, and it is the one line that explains what to fix. The private key is never returned, never logged, and never quoted in an error; failures describe the shape of what was found instead. Both PEM encodings are accepted because a key round-tripped through openssl comes back as PKCS#8 and refusing it would be inscrutable to someone who did nothing wrong. FromFork reads the head REPOSITORY rather than the author, since that decides whether the reviewing agent gets Bash: a maintainer may open a PR from their own fork, a stranger cannot push to the base repo, and an unnamed head repo (a deleted fork) is treated as untrusted.
Phases 2 and 3 of docs/pr-review.md: the review runs and can be posted. No UI yet, so this is reachable over the API only. A review is an ORDINARY SESSION, created through the same machinery as any other. That is the whole structure: it appears in the sidebar, speaks over the same socket, and can be interrupted and argued with once the findings are in. Everything specific to reviewing is in what it is handed rather than in a subsystem of its own. What it is handed is a DETACHED worktree at refs/pull/N/head. Detached because a review reads somebody else's commit and will never be merged, so a branch would leave a permanent entry in git branch for something that lasts minutes. That ref is also the only place a fork's commits are reachable without adding a remote, so one fetch covers a colleague's branch and a stranger's fork alike. The agent cannot run git on a fork's pull request, because it has no Bash there, so kunai hands it the diff as text and it reads the surrounding code from the checkout. Diff in, context read out: that split is why the worktree earns its keep, and why Read, Grep and Glob are deliberately left alone. Anchoring is the part that decides whether a review posts at all. GitHub rejects the WHOLE review, not the offending comment, when one line is not part of the diff, so the patch is parsed into the set of positions that are actually commentable and every finding is checked before anything is sent. A finding that cannot anchor is demoted to the summary with a permalink pinned to the reviewed commit, never dropped: the most valuable finding is often about a file the pull request does not change, and that one can never be inline. Posting checks three things between the click and the submission, each because of a way this goes wrong: somebody else may have reviewed this commit already (GitHub is the only state two colleagues' installs share), the head may have moved since the review (comments would land on lines nobody wrote), and every comment must still anchor against the diff as it is now. internal/review is pure and knows nothing about GitHub's JSON or about sessions, so the rules that matter can be exercised against a diff fixture rather than against the network.
Phase 4 of docs/pr-review.md: the surfaces. A Pull requests card on the dashboard, a draft card in the chat, and the App credentials in Settings. The draft card is the reason posting is a second click, so it does not summarise what was found. It shows where each finding GOES: a badge per row saying inline or summary, and the ones GitHub will not accept inline say so with the reason, because that constraint is real and discovering it after posting would be a surprise. The header is the promise, and it updates as you drop findings: two findings, one inline, one in the summary. Nothing else appears on your colleague's pull request. The dashboard card lists only repositories kunai already has open, derived from sessions and history rather than a list somebody maintains. That is the same constraint the whole feature rests on rather than a limitation to work around: reviewing here beats reviewing in CI because the agent reads the real tree at the pull request's head, and that needs a clone to build a worktree from. Nothing new was invented visually. The palette, the mono data voice, cards for context and chrome for actions, and the rule that an action rests visible rather than hiding behind a hover are all the app's own, and t3code was taken for mechanism rather than appearance. Two things are set in Settings and they are deliberately separate. The App is shared, so every install posts as one kunai[bot] and reviews are consistently attributed. The handle is per device, because it names the PERSON who asked: a machine two people use must not credit one's reviews to the other. The private key is write-only from the client and is cleared from the field on save.
Caught before this went anywhere: on a machine with no GitHub App configured, the dashboard fetched pull requests for every repository, failed every fetch, and rendered "kunai has no GitHub App on this machine yet" as an error card. Everybody who does not review pull requests would have got that on their dashboard, twice on a phone. The card now asks whether the App is configured before anything else and renders nothing at all until the answer is yes, which also stops it making one failing request per repository on every dashboard load. The smoke test passed through all of this, because it only asserted that the page still rendered. It now asserts the card is ABSENT, which is the actual claim being made.
Every pull request showed as +0 -0, including a 5,300-line one. GitHub's LIST endpoint does not carry additions and deletions at all; they exist only on the single-pull-request endpoint. The comment in pulls.go said exactly that and the dashboard went on reading them off the list anyway. The size now comes from the per-pull-request lookup, and the enrichment runs concurrently rather than in sequence, which it should have from the start: each row already needed a second call to answer "has this commit been reviewed", so a repository with ten open pull requests made twenty round trips to github.com before the card could paint. Bounded at six in flight, because spending a repository's rate limit on a dashboard nobody has read yet is its own kind of rude. A failed detail lookup leaves the size unknown rather than dropping the row: knowing a pull request exists is worth more than knowing how big it is. ghapp.NewWithBaseURL is exported for this test, which is the seam a GitHub Enterprise host would use too.
Two faults, one of which meant the feature never worked at all. Found by reviewing PR #4 with the feature itself, which is the one good thing here. collectDraft accumulated ev.Text on assistant events. An assistant message carries its content in Blocks; Text is only ever set on delta, thinking and user events. So the collected draft was the empty string, review.Parse found no block, and EVERY review recorded a parse error. It read as the model failing to follow the output format rather than as kunai never having seen the answer, which is the worst shape a bug can take. Text now comes from the text blocks, with thinking and tool calls excluded for the same reason the loop's promise check excludes them. And the brief was sent as an ordinary prompt, so the entire instruction set and the whole diff were printed into the chat as something the user had apparently said. The work started several screens below a wall of JSON schema. It is sent silently now, the way an added project's context already was: the draft card is the conversation's record of it, which is exactly what `silent` means in this codebase.
The dashboard listed every pull request twice: once under "kunai" and once under a heading called "4". The second was the review's own worktree, .../worktrees/kunai/review/4, taken for a repository because nothing said otherwise. A review deliberately runs in a detached worktree that is not registered as work in progress, which is right (it will never be merged, and a branch for a review that lasts minutes would litter git branch), but it left the session with no repository at all. It now reports the checkout it was started from, so it groups under the codebase it is reviewing in the sidebar as well. Tagged from the review record rather than resolved from disk, because this runs for every session on every listing and an extra git call per row is not worth paying to answer a question we already know the answer to.
Three changes, all from the same complaint: the review spent your money badly and put its machinery where the work should be. The diff is written into the worktree and the prompt names the path. Pasting a large pull request inline spends the whole diff in tokens before the model has decided anything is worth looking at, and most of a big diff is not: you saw 146k tokens and $1.47 for a run that had not finished. Given a path it reads what matters, greps for callers, and skips the lockfile. Read reaches the file on a fork's review too, where Bash does not, so this costs nothing in safety. The prompt keeps the changed-file list with per-file sizes, because that is small and is exactly how the model decides what to open first. Reviews get their own account and model, set in Settings. A review is chunky and arrives when somebody else opens a pull request, so spending the window you are working in is the wrong default; pointed at a second account or a provider it can never wall the session you are sitting in. Empty means the machine's default, so nothing changes for a machine that never sets one. And clicking Review no longer throws you into the chat. A review takes minutes and you almost never want to watch one: being dropped into a session reading files is what made this feel wrong. The row reports "Reviewing 2m" in place and offers a way in. The draft card also moves to the TOP of the session, because it is the point of that session: while the review runs it is the progress, and when it ends it is the thing you act on. At the bottom it sat behind minutes of tool calls.
Every review made a full worktree and nothing ever removed one, so they accumulated one per pull request ever reviewed. Tens of megabytes each on a repository of any size: not dangerous, and exactly the kind of thing you find out about in a month. The obvious implementation is the wrong one, which is why this is a sweep and not a callback. A RESPAWN also ends the session (an effort change, an account switch, auto-failover all close it and build a new one in the same checkout), so removing on close would delete the directory out from under a session that is coming straight back into it, and there is no moment at which the difference can be observed: the old session is gone before the new one is registered. So the question asked is "is anyone working here", which the worktree store already answers for this exact reason, plus a grace period long enough that a respawn measured in seconds cannot race a sweep measured in minutes. Asked of the live session list rather than of the review's own id, because a worktree can hold more than one session: you may have opened your own beside the review to read the code, and pulling the directory out from under that would be worse than keeping a few megabytes. Swept on boot as well as on a timer, and the boot pass is the one that matters: it catches the reviews that were running when kunai was killed, restarted or updated, which is precisely when nothing else had the chance.
Two things made watching a long turn worse than reading about it afterwards. A finished turn already collapsed its tool calls behind one summary. A RUNNING turn rendered every block inline, which is the case where it matters most: watching an agent read forty files meant forty cards pushing the work off the screen, and the thing you wanted (what is it doing NOW) was at the bottom of a column you had to chase. While it runs it is now one line naming the current call and counting the finished ones, with the full stream behind a disclosure. That disclosure's state is held by the chat rather than the component, so it does not close itself every time a tool result arrives. And tool output was a grey <pre> whatever came back, which wasted three things already in the app: a syntax highlighter, a diff renderer, and the path the tool was given. A Go file now reads as Go and a unified diff reads as red and green, which matters more now that a review begins by reading one. A diff is sniffed from the content rather than the path, because it arrives from Bash and from git show where there is no path; the sniff needs a hunk header AND a changed line, so a changelog full of bullet points is not mistaken for one. The Read tool's ` 12->` gutter is stripped first, or highlighting colours the line numbers as code.
Three real reviews finished and produced nothing: no draft, no parse error, and not one log line. Not a parse failure, which is what the empty card looked like. The collector was never there at the end. It worked by SUBSCRIBING to the session and waiting for the turn to end. emitLocked drops a subscriber whose buffer fills and closes its channel, which is right for a phone that cannot keep up and fatal for a collector: a review streams for minutes, so the watcher was dropped part-way every time and then saved nothing and logged nothing, because from where it stood the conversation had simply ended. Silence indistinguishable from success, again. Watching from outside was the wrong mechanism. The answer is taken from inside the session now, where lastText is already maintained for the loop's completion promise, through a hook that cannot be dropped for lag and fires exactly once per turn. Deliberately not SetTurnEndHook, which auto-failover owns: a session has one of those and two features needing the end of a turn should not have to fight over it. Pinned by a test that overflows a subscriber, asserts it really was dropped, and requires the hook to have fired anyway. Separately, the brief is now wrapped in a tag. It is sent silently so it never renders live, but the CLI writes every turn to the transcript and reopening a session replays it, so a review looked at afterwards printed its entire instruction set back as a user message. Seeding already skips a user turn that opens with a tag; this is the same mechanism <loop-iteration> relies on, and it costs one line.
The chat was the wrong surface and the tell was that every improvement to it was an attempt to hide it: the brief sent silently, the findings pinned above the conversation, the tool calls collapsed, the prompt wrapped so a reopened session would not replay it. When every change suppresses a surface, the surface is wrong. A chat is for open-ended conversation; a review is a fixed set of judgements, each with evidence, that you accept or drop and then send. So a review session now opens on a view of its own: one column of self-contained cards, each carrying the claim, the diff lines it is about, a suggested change where there is one, and the two decisions you can make. One column rather than a list beside a detail pane because kunai is used from a phone, and a split does not survive a narrow screen; this is one layout to get right instead of two. Each finding travels with its hunk, cut from the patch rather than read from disk: the patch is what the finding's line numbers refer to, and a file read from disk would drift the moment the pull request moved. Lines the finding is actually about are marked, so context can be generous without the point being lost in it. Keyboard on desktop, because a review is a rhythm: j and k to move, d to drop, cmd-enter to post. Untouched on a phone, where scrolling and tapping is the whole interaction. Post is sticky in the header and says how many findings it will send, so it is a promise rather than a button. The conversation is one click away, and a finding can be carried into it: being able to argue with the reviewer is the thing kunai has that a CI reviewer does not. It is just not the room you start in. Two bugs the new view's own test found. Dropping every finding and pressing Post published every finding, because an empty selection was read as "no pruning" -- nil and empty mean different things and JSON distinguishes them for free. And with everything dropped the button went grey, when the summary alone is still a review worth sending; it now says so.
Updating nightly did nothing, twice, and then reported "still on nightly-099509". The release really did carry the new build; the updater downloaded the old one. Assets were fetched from the release's name-based URLs, on the stated assumption that CI overwrites them on every push so the URL is always the latest. GitHub redirects those to a CDN that caches BY URL, so for a window after CI replaces an asset the previous bytes are still served. A nightly published at 23:36 was still handing out the build before it at 23:37. The checksum did not catch it, and that is the part worth understanding rather than patching: checksums.txt was fetched from a name-based URL as well, so the stale binary and the stale checksum came from the same cached generation and agreed with each other. A consistent stale pair passes every check that only compares the pair. Assets are now resolved through the release API, where every re-upload gets a new asset id: there is no shared URL left to cache. Both the binary and its checksum come from ONE read of the release, so they cannot be from different generations even in principle. The old name-based helper is deleted rather than left beside the new one, because it looks perfectly usable and is not. One test was stubbing the removed variable and so had been reaching real GitHub; it now uses the same fake release as the others.
The same stale-CDN bug as the self-updater, in the other place a binary arrives from. install.sh fetched /releases/download/<tag>/<name>, which GitHub serves through a cache keyed on that URL, so re-running it during the window after CI replaced an asset reinstalled the build already on the machine. Handing somebody a curl one-liner to escape a broken updater is not much use if the one-liner has the same fault. Assets are resolved through the release API and fetched by id, which changes on every re-upload, and the binary and checksums.txt come from one read of the release so they cannot disagree about which generation they belong to. The release payload is parsed with awk rather than jq, which this script cannot assume exists. Within an asset GitHub emits "url" before "name", so the last url seen when a matching name appears belongs to that asset; the nested uploader's url comes after the name and is overwritten before the next asset's name is reached; and browser_download_url does not match because the character before `url` there is an underscore, not a quote. Checked against the real payload rather than reasoned about: it resolves both assets and returns nothing for a name that is absent.
The session sat on "Needs you" while the view said "Nothing worth reporting". Both halves were wrong in the same way. A review runs with a permission mode that gets on with safe work and stops to ask about a risky command, which is right for a session you are sitting in front of. A review is the opposite by construction: the dashboard deliberately does not open it, because you almost never want to watch one. So the first unusual Bash command parked the whole review on a question that would never be answered. Bash is therefore withheld from every review, including on your own team's code where it was allowed so tests could run. That was a real gain in quality and it is being given up on purpose: a reviewer that hangs silently is worth less than one that cannot run tests, which is the same trade the loop already makes when it borrows acceptEdits. Reading is untouched, and reading is the job; Read, Grep and Glob never need permission, which is what makes an unwatched review possible at all. One consequence is worth keeping: the toolset no longer depends on trust, so there is no second list to keep in step and no way for a stranger's diff to be handed more than your own branch gets. And the view lied. awaiting_permission is neither running nor finished, so it fell through to the empty case and reported a clean bill of health for a review that had not started looking. It now says it is waiting and offers the way to answer. Reviews can no longer reach that state, but a UI that claims "nothing found" whenever it is confused will find another way to be wrong.
…back A review stopped a minute in, having read the diff and some source, with no error anywhere. The log named the culprit against a session that was never shared: "session 4092bed3 outlived its share; restoring its tools". The share reconciler gives back the toolset of any session whose share has ended, because a link that simply runs out of time is swept by whatever next reads the store and nothing was watching for that. It found those sessions by looking for withheld tools, which was proof of a share only while sharing was the one thing that withheld them. A review withholds Bash too, so that it cannot park on a permission ask nobody is there to answer. So every review looked like an expired share, and about a minute after one started the reconciler respawned its session and killed the running turn. A session now records what withheld its tools, and the reconciler lifts only its own. The owner travels with the tools through spawnSpec, because a restriction nobody claims is indistinguishable from an abandoned share and would be lifted by the next tick; changing the tools re-states the owner, and giving them back drops the claim. The reconciler's judgement is a pure function so it can be tested without spawning a CLI, the same reason discoveryCache.merge is one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You open a pull request in kunai, click Review, and an agent reviews it against a
real checkout of that PR's code. The findings come back as a draft you read and
prune, and a second click posts them as a bot, anchored to the files and lines
they are about.
Nothing polls, nothing listens, nothing runs unless a person clicks. The design
discussion is written up in
docs/pr-review.md.What is here
internal/ghappauthenticates as a webhook-less GitHub App. People assume Appmeans webhooks; an App registered without one is purely an identity plus a
credential, which is what lets kunai post as a bot while still exposing nothing
to the internet.
internal/reviewis the pure part: what a finding is, reading the agent'sanswer, where a finding may be anchored, and what the posted review looks like.
It knows nothing about GitHub's JSON or about sessions.
internal/server/prreview*.goorchestrates. A review is an ORDINARY SESSION,so it appears in the sidebar, speaks over the same socket, and can be argued
with once the findings are in.
The parts worth reviewing carefully
Anchoring decides whether a review posts at all. GitHub rejects the whole
review, not the offending comment, when one line is not part of the diff. The
patch is parsed into the set of commentable positions and every finding is
checked before anything is sent; one that cannot anchor is demoted to the summary
with a permalink rather than dropped, because the most valuable finding is often
about a file the pull request does not change.
Fork pull requests get no Bash. A stranger's diff is untrusted input that the
agent is about to read in full, so it can read and report but not execute. The
trust decision reads the head repository rather than the author, and lives in one
function.
Posting is a second click, and checks three things first: whether somebody has
already reviewed this commit (GitHub is the only state two colleagues' installs
share), whether the head has moved since, and whether every comment still anchors.
What is verified, and what is not
Full Go suite,
go vet,gofmtand the race detector are clean, with aroundforty new tests covering JWT timing, token refresh, diff anchoring, the parser,
and the placement rules. Playwright covers the surfaces.
Not verified: this has never touched real GitHub. Every test runs against a
fake API. The first live review is the real test, particularly the line
anchoring.
Also known: reviews currently run on the machine's default Claude account. The
seam to point them elsewhere exists and is not wired.