Review navigation: grouped sections, a reading order, and approvals pinned to content - #21
Open
jbachorik wants to merge 113 commits into
Open
Review navigation: grouped sections, a reading order, and approvals pinned to content#21jbachorik wants to merge 113 commits into
jbachorik wants to merge 113 commits into
Conversation
Adapts an external design (review-helper v2, 2026-08-21) that solves the problem drydock's Review board has left open: the rail is ordered by FallbackIntents' kind heuristic, which is a better order than the diff's own and still not the order the change was made in. The adaptation is mostly subtraction, because most of that pipeline already exists here under other names -- its resolve-source is SessionReviewScopes, its sections are ReviewIntent, its LLM section-namer is review_intents, its served UI is SessionReviewView. What is missing is order-hunks.py, which becomes ChangeGraph + ReadingPath. Four decisions are recorded with the reasoning that produced them, because none is recoverable from the result: - The SQLite KG is cut. It is a file in v2 only because that is a multi-process pipeline; drydock is one process, so the graph is in-memory with scope lifetime and nothing needs invalidating or collecting. - The concept map is cut and same-concept links are kept. An earlier draft bundled them and that was a dependency chain, not an argument. The map loses to the intent rail on redundancy and to RailLayout on width; the links survive with a shared changed symbol as their source, which names its own reason where a community id cannot -- and that removes the last caller of a clustering algorithm. - JGraphT is rejected on value, not quality: 1.27MB plus transitives, a jlink module-list entry RuntimeImageModuleListTest pins, and a POM dependency jbangJar bundles nothing of, for Kahn and Tarjan. - Tree-sitter is taken (io.github.bonede, whose natives are exactly our platform set) with a grammar-absent-means-lexical rule, so the shipped language set stays a packaging decision. The one honest loss is written down rather than engineered around: a diff-scoped graph cannot see a caller in an untouched file, so v2's best entry-point signal is restored by one bounded git grep instead of by the repository-wide index this codebase has twice refused to build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three amendments, each from a question the first draft could not answer. Grouping was the assumption that broke. The draft mapped v2's "sections" onto ReviewIntent and called the row done -- true of the agent path, false of the computed one. Run on a real C++ PR the rail reads "main/cpp · 12 files", "test/cpp · 4 files", "cpp/hotspot · 6 files": FallbackIntents' (kind, directory) key titled by shortDirectory(), with no structural input at all. Ordering that better sequences bad groups and stops. So sections now come from connected components of the file-level reference graph, with the two conventions a C/C++ change is unreadable without -- a .h with its same-basename .cpp, and a header with any changed .cpp that references it even when the referenced symbol is not itself in a changed hunk. Drydock draws the sections; the agent names them, which is v2's own split and what keeps the floor working with no agent: correctly grouped and ordered, only plainly named. review_scope gains a `sections` include so the agent can accept-and-name rather than regroup blind -- reversing this document's own deferral of that include, on the evidence. v2's inconsistent test handling is NOT inherited: drydock's rule that tests get their own card is kept and applied inside a component, so it holds by decision rather than by luck. Assisted review was the second question, and it exposed a hole: §4.3 ran git grep, counted the matches and threw the locations away. A fan-in with nowhere to click is a statistic, not comprehension. It keeps -n now and feeds the occurrence popover the symbol lens already has, so `u` and `a` reach usages and the agent with the question already pointed at the right file. The mechanical layer's job is not to be intelligent; it is to make sure the reviewer knows which question to ask. Stability was the third. Verdicts are keyed (scopeId, intentId), IntentGrouping.set replaces wholesale, and agent ids are agent strings -- so a re-review orphans every approval the human gave. AnnotationStore already carries the scar of the structural version of this bug, with an asymmetric merge that refuses to approve code nobody read; it only knows the `file:` prefix. Intent ids become content-derived, regrouping re-anchors by hunk overlap on that same merge, and determinism stops being a property and becomes a requirement with a test. Order and links now also carry provenance, because a measured edge and a claimed one fail differently and a reviewer has to tell them apart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FallbackIntents makes kind part of the group key so a test never shares a card with its subject. An earlier draft of this design kept that rule and applied it inside each computed component, which was §5.1's own indictment repeated one layer later: §5.2 groups by structure precisely because a path heuristic draws the wrong boundaries, and then splitting the result on /test/, *_ut.cpp and *Test.java is a path heuristic drawing a boundary through a structurally sound group. Dropping the rule needs nothing to replace it. A test references the symbol under test, so a changed declaration in guards.h and its exercise in hotspot_crash_protection_ut.cpp already share a component, and the edge runs test->implementation so the implementation sorts first: here is the change, here is what pins it. A test that references nothing changed forms its own component, which is the honest outcome rather than a special case -- and that is also what the reference implementation's apparently inconsistent output actually was, the graph being right twice about two different kinds of test file. The quoted rationale is obsolete rather than wrong. Separating tests was the only way to stop them burying the change when there was no structure to consult; with a component to place them in, keeping a test beside the code it pins is what lets a reviewer check that the code does what the test claims. Two consequences written down rather than left to be found: a mixed section is Kind.CHANGE because ReviewIntent.Kind holds one value and a section with production code in it cannot honestly be tagged tests, and the entry-point ranking's "not a test" signal is demoted to a tie-break for when the graph is silent -- it now decides only the case it should, a test-only section with no edges into it. The kind key survives untouched in the no-edges fallback, where there is still nothing better to guess with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sections stop partitioning the change. A header groups with its same-basename .cpp AND with every changed .cpp that references it; those are different sections, and disjoint membership made one of them lose -- which is what counters.h looks like in the reference output. Overlap removes the choice: a file appears wherever it is needed to understand what is being read. It also drains most of the over-grouping risk, since a component no longer has to swallow a file merely to keep the connection visible. What overlap breaks is arithmetic. Verdicts are keyed (scopeId, intentId) and the bar reads "n/m intents settled"; both assume a partition. So the verdict moves to the hunk, keeping all three decisions, and a section's state becomes derived by the merge migrateLegacyVerdicts already implements -- any CHANGES makes the section CHANGES, APPROVED needs every hunk. That rule was written for exactly this question and stops being a migration. The anchor is a content digest, not the existing stable line key. The line key is positional, so one pushed commit shifts every key below an insertion and a clean flag comes back covering lines nobody read -- precisely what migrateLegacyVerdicts refuses to produce. A finding landing a few lines off is an annoyance; an approval landing a few lines off is a silent lie about what was reviewed. Digesting the changed lines makes a re-diff correct by construction: a hunk that only moved stays settled, a hunk whose content changed does not, and a hunk in three sections is one flag because it is one digest. This supersedes most of the previous section. Content-derived intent ids and re-anchor-by-overlap existed only because verdicts were keyed to the grouping; nothing durable is keyed to it now, so an agent may regroup twice and differently and the reviewed state does not notice. The worry that agent sections are probabilistic where structural ones are stable is answered at its root instead of compensated for: it stops mattering. Content-derived ids survive only for `reads`, where nothing persists under them. Scope §3 said the verdict model was out of scope. That was not a scoping decision, it was an unexamined assumption that sections stay disjoint, and it is corrected rather than widened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The in-scope list still read "content-derived intent ids", which survived only as an implementation detail of `reads` once verdicts stopped being keyed to the grouping. What is actually in scope is overlapping section membership and reviewed state keyed to hunk content. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two holes in the digest, one of them the same mistake the section was written to avoid. Context is now hashed alongside the changed lines. A changed-lines-only digest leaves an approval standing when the line above the hunk moves: the hunk is byte-identical, its surroundings are not, and a hunk means what it means in place. Review already fetches twelve lines of context, so this costs nothing. The limit is deliberate -- an edit elsewhere in the same file does not unsettle, because a file-wide digest re-reviews code nobody touched, and the context window is drawn where the reviewer could actually see while reading. The base is now recorded and can go stale. A diff means what it means against a base, and a digest over its own text cannot see the base move: a rebase leaves every hunk byte-identical while the code underneath them changed, which reproduces one level up exactly the positional-line-key mistake this section rejects. Verdicts carry the (base, head) they were given against; a base move marks them stale rather than deleting them, renders what moved, and refuses submit through the verdict bar's existing submitRefusalLabel. Deleting was rejected because a rebase is routine and a tool that discards a forty-hunk review on every base advance teaches reviewers not to mark anything. Keeping silently was rejected for the reason the section exists. Stale is a third state because neither of the two simple answers is true. There are no recorded verdicts, so the migration is deleted rather than written -- which removes what this design had called its least recoverable step. It follows that migrateLegacyVerdicts has no remaining caller once this lands: its merge helper is kept and promoted to the live section derivation, and the migration wrapper around it is dead code under AGENTS.md's rule. Flagged rather than assumed, since "no verdicts anywhere" is a check, not a reading of the source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t cannot fix Marking every verdict stale on any base move treats "main advanced by seven commits in an unrelated subsystem" the same as "main advanced by a commit rewriting a function this hunk calls". Only the second can invalidate a reading, and on an active repository the first is nearly all of them. The base delta is now intersected against the scope's own files and the files declaring symbols its hunks reference -- one git diff --name-only, and the same unique-name rule §4.2 already produces. An unresolvable old base marks everything, because failing safe is the only defensible default for a signal about what was read. The intersection is file-level and lexical, so a base change that alters behaviour without touching a file this scope names is missed: §4.3's boundary reappearing. That narrows when the mark fires and fixes nothing about what happens when it fires often anyway, so the risk is now recorded as accepted rather than mitigated. A reviewer in a hot area learns to confirm without reading, and no arrangement of this feature prevents that -- the mark's value comes from being rare, and rarity is a property of the repository, not of the design. The consequence is stated rather than implied: submit-blocking rests on a signal that degrades exactly where it matters most, so in a fast-moving area a blocked submit is theatre. The block is kept because it is right in the ordinary case and because the alternative is silence, but it is not a guarantee, and nothing else in this design may assume a confirmed verdict was re-read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§9.2's relevance filter is file-level and lexical and names its own blind spot: a base change that alters behaviour without touching a file this scope names is invisible to it. An agent has no such boundary, so it can answer the one question neither the digest nor the intersection can. What it may not do is clear an approval. That is the line the MCP surface is drawn around -- the human-side writes were kept off the tool list because exposing them would let an agent approve its own work, propose* is recorded and never applied, and AUTO_APPROVED is labelled the agent's assertion rather than the human's. A recheck gets no exception. But the two directions carry different risk, and treating them alike wastes the capability. "Affected" applies: it can only add staleness, asks for more reading rather than less, and is exactly how the blind spot closes; an agent wrong that way costs a wasted re-read. "Unaffected" stays advisory, because an agent wrong THAT way costs an approval on code nobody re-read, which is the outcome the section exists to refuse. It is migrateLegacyVerdicts' asymmetry -- any CHANGES wins, APPROVED needs everything -- pointed at a different question. A base move that marks anything stale dispatches a bounded recheck through the existing subagent review form, so the assessment is usually already there when the reviewer returns; it reads one base delta and the stale hunks, not the change, which is why it earns a dispatch rather than a full re-review. review_recheck is the only new tool in this design, and it exists because nothing carries a statement ABOUT a verdict: findings are about code, are anchored to line keys, and folding staleness opinions into them would put them in the open-findings count behind the ◨n badge. It does not fix the accepted risk. The reviewer still clicks confirm; what changes is that nine of the twelve are now visibly uninteresting and three are not. It does not make the mark trustworthy, it makes it sorted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-four tasks in three phases, each phase shippable on its own, in an order the spec forces rather than one chosen for convenience: reviewed state has to move to the hunk before sections can overlap, and sections have to overlap before §5.2's header conventions are expressible at all -- a header belongs with its implementation AND with everything referencing it, and disjoint membership makes one of those lose. One dependency is deliberately split across a phase boundary and says so in both halves: §9.2's relevance filter intersects a base delta against the scope's files and against the files declaring symbols its hunks reference. The second half needs the change graph, so Task 5 takes a Collection for exactly that reason and Task 15 widens it without moving a caller. Self-review caught one spec section with no task -- §6.5, provenance -- and Task 24 covers it. It is last because it is the smallest change touching the most rendering paths, and doing it once beats threading it through four tasks as they land. The notes at the end carry the two things that will otherwise be rediscovered the hard way: the full suite takes 14-20 minutes and must not be handed to a subagent under a 10-minute ceiling, and a section order that differs between runs is a hash-ordered collection rather than a bad algorithm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The digest covers the path, the changed lines and the surrounding context. Context is in because a hunk means what it means in place: change the line above it and its changed lines are byte-identical, so a changed-lines-only digest leaves the approval standing over code whose surroundings moved. It stops at the context window because a file-wide digest re-reviews hunks nobody touched. Line numbers are out, so a hunk that only moved stays approved -- which is the reason this is not the positional line key findings are anchored to.
Replace java.util.Locale.ROOT with Locale.ROOT and add import to comply with the constraint 'Never inline fully-qualified class names — use imports'.
…ainst Two changes, one record. The key moves from intentId to a hunk content digest, so a verdict no longer belongs to a grouping and an agent regrouping cannot orphan it. And the (base, head) it was judged against is recorded, because a digest over a hunk's own text cannot see the base move underneath it -- a rebase leaves every hunk byte-identical while the code they sit on changed. Confirm-still-good rewrites the recorded base rather than setting a confirmed flag. A flag would be a second source of truth that the next base move has to remember to clear, and forgetting is a silently-approved-stale- code bug.
Schema 4. A verdict entry carries hunkDigest, base and head instead of intentId; a v3 entry naming an intentId is skipped by the existing lenient decode rather than failing the load. migrateLegacyVerdicts goes with it. It carried verdicts from the old file:<path> intent ids onto directory-clustered intents, and with the key no longer naming a grouping there is nothing for it to carry and no caller left to call it. Its merge helper survives -- it answers how a group's decision follows from its members, which is now a live question rather than a migration one.
…rage key McpToolRouter's review_state forwarded verdict.hunkDigest() straight onto the wire as intents[].id. That field is what an agent correlates against the ids it sent to review_intents, and today hunkDigest happens to equal the intent id (MainWorkspace/FakeReviewHost's placeholder), so nothing looked broken -- but nothing enforced it either, and the join would have silently broken the moment real hunk digests land. review_state now joins the scope's intents (McpSessionContext.intentsOf, mirroring IntentGrouping.intentsFor) against their verdicts by digest internally, and emits the intent's own id on the wire. AnnotationStore.merge is deleted rather than kept: Task 4 turned out to write a new VerdictMerge from scratch rather than move it, so keeping it only parked untested dead code. The setVerdict placeholders in MainWorkspace and FakeReviewHost get grep-able TODO(task-6) markers instead of a prose comment claiming scope.base()/head() are already correct -- they are ref names, not commit shas, so staleness is inert until Task 6 resolves them. A schema-4 entry carrying these decodes cleanly, so nothing else will surface the gap.
Joining intents against verdicts made review_state call context.reviewDiff(scope) unconditionally, so a PR with no local checkout, or a git failure, now failed the whole call -- a live regression, since findings and submission status never depended on a diff before. review_state now reports those two regardless, and omits the intents key entirely (never an empty array) when the diff fails, logging the failure at WARNING with an excerpt. An empty array would read as "nothing is settled"; an absent key correctly says "cannot be known right now", the same absent-vs-zero rule the sidebar's badge follows. FakeMcpSessionContext.reviewDiff never threw, which is why no test caught this. It can now be made to throw, and a new test pins the degraded shape: findings and submission present, intents absent.
Sections overlap, so a section cannot own a verdict -- a hunk shown in three of them would need three. The decision is derived instead, by the asymmetric merge the legacy migration was already written around: any CHANGES makes the section CHANGES because that is true however the group is drawn, and an approval needs every hunk settled because approving a section claims the human read all of it. Extracted from AnnotationStore so it can be tested without a store, and because it is no longer a migration detail but the rule the rail renders.
Marking every verdict stale on any base move treats main advancing in an unrelated subsystem the same as main rewriting a function this hunk calls. On an active repository the first is nearly all of them, and that is how a confirm button becomes reflex. One git diff --name-only, intersected with the scope's files, decides. Failing safe where it cannot decide: an unresolvable old base -- a force-push, a collected commit -- marks everything, because for a signal about what was read there is no defensible alternative. Two more honest limits: the intersection is file-level and lexical, so a base change that alters behaviour without touching a named file marks nothing, and the scope-file set is a Collection so Phase 2 can widen it to the files declaring symbols these hunks reference without moving a caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
git diff --name-only without -z emits C-style-quoted paths for files containing non-ASCII bytes or special characters. A file named "café.txt" would arrive as the literal string with quotes and octal escapes, never matching the scope's unquoted name, so a base move that genuinely touched the file would silently be recorded as not touching it. With -z, git emits raw unquoted paths NUL-separated and avoids quoting entirely. The parsing is extracted to a package-private static method so it can be tested directly without spawning git. A test confirms non-ASCII filenames round-trip intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sections overlap, so the sum of their sizes exceeds the number of hunks and n/m intents settled measures nothing. The bar counts distinct hunks; a section's decision comes from VerdictMerge over its own; and a hunk settled in one section renders as settled in the other, marked with where, so the effect of settling is visible where it lands rather than looking like state changing on its own. The Host seam moves with it. verdict() is keyed by a hunk digest and setVerdict() takes the section's digests, computed by the view: only the view knows which diff the human is looking at, and a host free to re-derive them is free to derive them from a different one. The intent stays on setVerdict because the refusal of an approval over an open blocking finding is stated in terms of the intent. IntentHunks.digestsOf is the one walk from a section to its digests -- the view, review_state and the recording path all needed it, and three copies of that walk would drift. Base and head are resolved from ref names to commits. scope.base() is a branch name, so a verdict recorded against "main" and compared against "main" could never be stale and staleness was an inert no-op. GitStatusService gains commitForRefBlocking, a sibling of headCommitBlocking with the same shape and the same empty-rather-than-throwing rationale, and MainWorkspace resolves both refs off the FX thread. A ref that will not resolve is stored as the literal "unresolved" rather than as the ref name: staleAgainst already asks !baseCommit.equals(currentBase), so no real sha can equal it and the verdict reads as stale until a human confirms -- fail-safe with no second code path. Storing the ref name would have been the silent-approval bug. Whether a base move could actually matter is memoized per (scope, oldBase, newBase) and computed off the FX thread; until the answer lands the delta is unresolvable, which is "could matter". Absent must not read as clean. A failed lookup is recorded as unresolvable rather than left absent, or the next render would spawn the same git again, forever. Considered deriving a section's decision through the full section state when asking whether a sibling settled a shared hunk; that recurses between two sections sharing one. The sibling is asked only for its merged decision. Considered leaving the digests uncached; every card of the rail asks for its section on every rebuild, which is thousands of SHA-256s per keystroke on the FX thread, so they are memoized against the diff instance and keyed by the whole intent -- a reviewer may re-issue an id over different hunks, and an id-keyed memo would answer with a grouping that no longer exists.
…bmit Four things a settled card could say that were not true. "We cannot tell yet" was rendered as "the base moved". Before a scope's baseline lands, currentBase is the literal "unresolved", which is not a revision -- reviewBaseMove handed it to git diff anyway, spawned a command that always fails, logged a warning describing no real problem, and got back an unresolvable delta. couldMatter answers true for unresolvable, so every settled card of a review nobody had touched showed a confirm-me banner until the answer arrived. The spawn is now short-circuited on either side of the comparison being unresolved, and SectionState carries a three-state Staleness rather than a boolean: FRESH, MOVED, and UNKNOWN, which renders as nothing. couldMatter's true-on-unresolvable is right for a DECISION and wrong as evidence of a move, and the card is the place that distinction has to land. A banner clicked reflexively is worth less than no banner. A section whose hunk ids named nothing in the diff was silently unapprovable. Ids are positional (h_<file>_<index>), so a re-diff strands a grouping an agent supplied earlier, and digestsOf then returns nothing: the card looked untouched, pressing a wrote no verdict so the cursor never advanced, n kept landing on it, and Submit refused forever while jumping to the one card that could not be settled. This worked under the old intent-id keying, so it was a regression, not a pre-existing gap. Such a section is now excluded from the counted set that progress and Submit are measured over, skipped by n, and says "hunks are no longer in this diff" on its card. Considered leaving it counted and letting the human dismiss it; there is no gesture that would mean anything -- there is nothing there to settle. refreshBaseline's failure branch left the cache empty, so the next baselineOf -- once per card, per rail rebuild, on every store change -- spawned git again. The identical hazard was already guarded fourteen lines later in reviewBaseMove with the reason written down; the guard was simply missing here. The failure now records UNRESOLVED_BASELINE, and bodyFor still re-reads the scope's baseline whenever the board renders it, so a transient failure is not permanent. settledElsewhere fired only when the sibling section was FULLY settled. A sibling that settles one shared hunk moves this card's count by exactly as much, so marking only the full case solved the easy half of "state changing on its own" and left the other half as mysterious as before. It is populated per shared settled hunk now, and rendered only while this section is itself unsettled -- on a settled card its own verdict already explains the state.
Pure move. digestsOf, filesOf, currentBase, baseDelta, decisionOf, hasResolvableHunks, the sharing-section walk, sectionMark, the staleness reading and sectionState were ~130 lines of SessionReviewView whose only inputs are a Host, a UnifiedDiff and a list of ReviewIntent -- none of it scene graph. They sat in a class that was 1470 lines before this feature and 1742 after, and four more tasks land in the same file. SessionReviewView is 1587 now, and what remains of the derivation in it is four one-line delegations. The board context (scope, diff, sections) is a parameter rather than mutable state on the new class. A setContext seam would let a caller derive one section against the scope now selected and its neighbour against the one before it, which is precisely the class of bug that has no symptom until a chip switch. The view assembles the Board from the same three things the rail is built from, and answers "no board" for itself at each call site rather than letting a default hide the case. SectionState and Staleness move with it, so the rail names them SectionStates.SectionState. UNRESOLVED_BASE deliberately stays on SessionReviewView: MainWorkspace and its tests reference it, and moving a public constant is a separate change from moving package-private derivation. Behaviour is unchanged. Every early return the old code took is reachable in the new shape: no scope and no loaded diff both collapse to an absent Board, which the delegations answer exactly as before; the extra board.isEmpty() guard in renderVerdictBar cannot fire while an intent is current, since intents() is empty without a loaded diff. Eighteen assertions that never needed a rendered board move to a plain JUnit SectionStatesTest -- merge rules, counts, the three-way staleness, adrift groupings, the memo's re-issued-id case, the circled marks. What stays in ReviewHunkProgressTest is what genuinely needs a Stage: the progress label, the settled card class, the rail's labels, and the keyboard and Submit paths. diagSectionState still routes through ReviewDiagFxThread for the three tests that read derived state through the view.
Reading is per hunk; settling usually is not. The unit follows focus rather than adding a parallel key set -- the rule [ and ] already follow -- so a and r and u keep their keys and gain a defined effect on overlapping sections, with SHIFT variants for the file. The bar names the unit, because a key whose target depends on focus has to say what it is about to do. Stale verdicts get their two answers: confirm still good rewrites the recorded base, re-review clears them, and until one of those happens the section does not count as settled and the submit refuses with a reason rather than silently doing nothing.
The acting-unit label never changed after a click into the diff column: digestColumnActedOn was set from a MOUSE_PRESSED filter on the whole rail and column, and nothing re-rendered the bar when it flipped, so a/r silently switched to HUNK while the bar still read "section". settleUnit() now reads the Scene's real focus owner and walks its parent chain instead, and a focusOwnerProperty listener keeps the bar's label live across any focus change, not just the ones this view's own key handling triggers. That real-focus read is also immune to the isFocusWithin() ref-count bug an earlier attempt hit -- ReviewIntentRail rebuilds every card on each render, and JavaFX moves focus off a card about to be discarded via Direction.NEXT traversal, which left isFocusWithin() stuck true long after the real owner moved elsewhere. A fresh getFocusOwner() read every time never accumulates that staleness. The trade-off, on the record: a scrollbar drag or the rail's own collapse toggle can also move real focus and flip the unit, which a hand-tracked flag limited to "the rail" and "the diff" would not have -- accepted because the flag's alternative failure (silently stuck) is worse than this one (visibly, correctly focus-driven). HUNK mode also stopped always settling the section's first hunk: with the diff column acting, a settled hunk one forever, with no way to reach hunks two and up. It now prefers the hunk under an open gutter selection (ReviewDiffColumn#currentLineSelection, resolved to a digest by SectionStates#digestOfLine), falls back to the section's first UNSETTLED hunk so repeated presses walk forward, and only then to the anchor. Same priority for the file behind SHIFT+A/SHIFT+R (SectionStates#currentFileOf). Found and reported rather than worked around: a gutter click's release also opens the comment composer and moves real keyboard focus into its text field, which the existing TextInputControl guard then makes a/r type into rather than trigger -- and closing the composer clears the selection too. There is no way, with the composer unchanged, to hold a gutter selection AND have a/r read as shortcuts immediately afterward from a real completed click; ReviewSettleActionsTest proves the underlying wiring with a bare mouse press (selection painted, composer not yet opened) and says so. A stale verdict also stopped counting toward "everything settled": SectionStates#settledHunkCount now excludes a hunk whose recorded base has moved in a way that could matter, so the progress line and the "all settled" nav hint agree with Submit's own refusal instead of contradicting it one keystroke later. digestsForAction and oldBaseOf moved into SectionStates alongside the rest of the settle-target derivation, out of the view, which had regrown past its post-Task-6 size.
"Approve intent" stayed on the button regardless of what a/r actually hit,
and the acting-unit label this task first added to say so was, by design,
one of the things dropped for room at the code column's floor -- so at
560px the only unit statement left on screen was the wrong one. The unit
is now stated on the Approve/Request-changes buttons themselves ("Approve
(hunk)", "Request changes (section)"), which are never dropped for width,
so removing the separate label costs nothing and removes a whole fit
surface rather than widening anything to keep it.
The stale banner's own fit gap (Phase 1 gate) gets a real assertion too:
assertNothingTruncated only ever looked at .button, so staleLabel -- a
wrapText label with no minWidth -- could reflow silently underneath it.
Measured, not asserted around: at the 560px floor the banner wraps to
exactly two 17px lines rather than one, which is reported here rather than
designed away, since nothing is actually clipped and the row is allowed to
grow a line when a section goes stale.
…on close Ruling reversed: HUNK mode never actually settles the hunk under the gutter's cursor. Tracing a real click confirmed it -- a completed gutter click opens the comment composer and moves keyboard focus into its text field, which the existing TextInputControl guard then makes a/r type into rather than trigger, and closing that composer clears the gutter selection right along with it. Every real reader path leaves HUNK mode acting on the section's first unread hunk, the option the previous ruling rejected in favour of a cursor that turns out not to be reachable this way. The button and its tooltip now say "next unread hunk" rather than "hunk," and so does the shortcuts overlay; SectionStates.digestOfCurrentHunk keeps trying the selection first regardless, since it costs nothing and becomes live for free if the composer coupling is ever loosened. A second bug surfaced once the button named its unit: pressing a focusable Button requests focus on press, so clicking "Approve (next unread hunk)" moved Scene focus off the diff column and onto the button itself before release -- which flipped settleUnit() to SECTION mid-press and settled the whole section on release, silently disagreeing with what the reader just read on the label. ReviewVerdictBar.Host.approve/requestChanges now take the acting unit as a parameter, captured by a MOUSE_PRESSED filter at the moment of the real press (falling back to the live unit for a keyboard activation or a test's Button.fire(), neither of which presses at all). Caught by driving the button through a real press-then-release rather than fire(), which is what let this ship the first time. The focus-owner listener SessionReviewView added to watch for this also turned out to leak: the Scene it attaches to is built once for the whole application, so a listener never removed keeps every session's review board -- diff column included -- reachable for the process's life, and re-renders every one of their verdict bars on every focus change anywhere in the app. Held as a field now so close() (which already detaches the MCP panel and stops the rail's collapse timeline) can remove the exact same instance it was added with.
settledHunkCount already excluded a stale verdict from the verdict bar's
global progress line; SectionStates.stateOf did not exclude the same
verdict from a section's OWN count, so a rail card could read fully
settled ("2/2 hunks") while the bar's line, over the identical hunks, read
one short of it. Same rule now applies at both layers: a hunk whose base
has moved in a way that could matter does not count toward either count.
The section's DECISION is untouched by this -- it still merges the stale
verdict, since staleness puts the base in question, never the decision
itself.
Both were named in the original ruling and neither was actually done: assertNothingTruncated only ever looked at .button, so the stale banner's wrapText label could reflow silently underneath it, and nothing measured the bar's own height at the 560px floor at all. Both are folded into the one helper now, so every existing caller gets them for free. Measured height must be read via prefHeight, not getHeight: the bar is the test Scene's root, and a Scene resizes its root to fill its own fixed dimensions regardless of content, so getHeight() always reported the Scene's own 200px and the assertion would have passed without measuring anything -- the same trap the width check beside it already guards against.
A grammar missing from the classpath resolves to empty and the file takes the lexical path. That single rule is what lets the .app and the jbang jar ship different language sets, and what makes an unsupported language produce a coarser change graph rather than a broken surface. Failures are told apart on purpose. A grammar class that is simply absent is the normal case and logs nothing; a native library that cannot load -- wrong arch, failed extraction, CRC mismatch -- logs once for the process and turns every file lexical, because logging either one per file would bury the one that matters. The core artifact bundles aarch64/x86_64 macOS, x86_64 Windows and both Linux natives, which is exactly the platform set this app supports.
Class.forName succeeding is not enough to prove a grammar loads: a ReflectiveOperationException other than ClassNotFoundException means one grammar's shape changed (constructor visibility, missing no-arg constructor), not that the native library is gone. Folding it into the same catch as UnsatisfiedLinkError latched every language to lexical for one class's problem -- exactly the silent, process-wide failure this registry exists to prevent. Now a per-class reflective-shape failure logs once, naming the class, and only that class's extensions fall back; the native latch is reserved for UnsatisfiedLinkError and RuntimeException, the two shapes an actual failed native load can take. A parameterized test exercises every shipped extension end-to-end, and a companion test asserts nativeAvailable() stays true afterward -- proving a per-class failure cannot take the other languages down with it.
…arsed Two front ends, one shape. With a grammar, declarations come from the parse tree. Without one, every occurrence is a use and the file declares nothing -- a lexical scan cannot tell a declaration from a call without guessing, and a wrong declaration mints wrong edges everywhere that name appears. That asymmetry is the honest reading of what tree-sitter buys: it tells us a token is a declaration and another is a call, not which declaration a call resolves to. It raises the precision of classification, not the correctness of resolution, which is why an ungrammared file degrades to a usable graph rather than to nothing.
…ewer's own grouping too The graph link footers are computed from used to build only where no reviewer grouping existed or PATH mode was showing, so a scope reviewed through review_intents -- the intended path -- rendered zero footers. requestGraph is a no-op for a diff instance already graphed or building, so this costs nothing on a re-diff or a re-selection. The rail's own "refining grouping…" banner is gated separately now, on whether the RAIL's own content depends on this build (PATH mode, or no reviewer grouping) rather than on whether a graph is building at all -- a reviewer's already-final INTENTS grouping must not flash it purely because a background build is running for links. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… unguarded Task 19 round 2: the round-1 fix (requestGraph called unconditionally on Loaded, and the rail's own banner re-gated on hasReviewerGrouping) had no test that could catch its loss -- the full targeted suite passed 534/534 with either half of it reverted. Adds a test with a reviewer grouping installed (host.intents.set), asserting both that link footers still render and that the "refining grouping…" banner never flashes for an already-final grouping while the graph builds purely for those footers. The banner is sampled synchronously inside the same FX-thread task that triggers the diff, since the background build can finish faster than a subsequent poll would ever observe it. The existing ungrouped test gains the same sample as its control, asserting the banner DOES show there -- otherwise the new test's absence assertion would pass just as well against a banner that is simply broken. Mutation-verified: reverting the requestGraph gate back to the narrow `noReviewerGrouping || (pathMode && selected)` times out the new test with "no link footer ever rendered"; reverting the banner gate back to unconditional `graphBuilding.contains(scopeId)` fails only the new test's banner assertion while the ungrouped control still passes. Production code is unchanged from a7ec842. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The out-of-diff fan-in scan had zero production callers: all three sites hardcoded a placeholder saying "unavailable", so rank term 1 was permanently zero and every reason read "outside callers unknown". It runs now. On the board it runs off the FX thread, on the same virtual-thread executor the graph build uses and from that build's completion -- the scan's patterns ARE the graph's changed declarations, so it can neither run before one exists nor run on the thread that draws. A scan that comes back saying exactly what the board already showed records nothing and refreshes nothing: re-rendering the rail to say nothing changed moves the ground under whoever is mid-review. At the MCP boundary it runs synchronously, because that call is already off the FX thread and handing an agent a first-call-always-unavailable answer it will then act on is worse than making it wait. The count then becomes a control. The rail's reason label is the button's graphic rather than the button minting its own text, so ReadingPath stays the one author of that sentence and it keeps the fill that makes it readable. Its ActionEvent is consumed -- un-consumed it bubbles to the row button, selects the row, rebuilds the rail, detaches the node the popover is anchored to, and closes the popover in the same gesture that opened it. The popover is the symbol lens's, on a third source and in the same field, so Escape already unwinds it. It shows every occurrence with its file and line, says "N usages outside this change" in those words, and reports a refused Explorer jump instead of appearing to do nothing. The two occurrence records are not contorted into one: fan-in has no in-diff flag and needs none, since every row is outside the change by construction. This is where the design is honest about its ceiling. A lexical occurrence list cannot tell a reviewer whether a signature change breaks the caller it just found, and nothing mechanical and diff-scoped can. What it can do is put them one click from the party that can answer: a button -- never a key, `a` is the approve gesture -- files the question as a real review comment on the declaring file and hands it to the bound session. An unavailable scan shows no count rather than a zero: absent and none must not look the same. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fan-in is the reading path's first rank term, so the grep landing mid-read re-sorts the rail. pathIndex is a POSITION, and clamping it is not keeping a place: step ① became a different file, the diff column re-narrowed to it, and since settleUnit() is PATH_STEP unconditionally in this mode the reader's next `a` approved a hunk they were never shown. The third time on this branch that a gesture's scope has silently stopped matching what the reader sees. The cursor is re-anchored by hunk id now, the way reanchorCursor already does it for INTENTS and under the same only-when-it-changed guard, so [ and ] still move where they are told. Two more silences, both the shape ruling 1 legislated against. askAgentToFix is a boolean now rather than a void that discarded sendToBoundSession's answer: with no session bound, the popover used to close on a question nobody had been asked, leaving a review comment the reviewer never knowingly wrote. It says so instead, and the button says what it does. And the popover was owned by the rail row that every refresh replaces -- so it closed on the refresh its own ask button caused. It is owned by the column now, which outlives a rebuild; consuming the fan-in event is pinned by the cursor not moving rather than by the popover surviving. An occurrence is attributed to a symbol only when the line names it as a whole word. ZetaSymHelper was two uses of ZetaSym before, and that number is rank term 1 -- an inflated count does not merely read wrong, it reorders what a human reads next. The MCP router scans for real, so an agent and a human see the same ① rather than two orders, and caches per (scope, diff) so an agent polling a review does not spawn one full-worktree grep per poll. The rail's own accessor reads getGraphic explicitly: a Labeled's graphic is not one of its children until its skin exists, and for ~80ms after every render a fan-in row read as having no reason at all -- which makes any assertion over those texts pass or fail on timing. And the screenshot earned its keep, again. The reason was cut to one line and ellipsized in the running app -- "file called from 16 places outside the…" -- because a Button asks its graphic for prefHeight(-1) and a wrapping Label answers that as one line. The test suite could not see it: at full rail width that sentence occupies 189px of a 190px slot and fits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The same defect round 1 fixed on the fan-in popover, one surface over: askAgentToFix was void on the bar's Host and the boolean under it was discarded, so with no session bound -- or no open finding to hand over -- the button did nothing at all and looked exactly as though it had worked. The destination returns the real answer through both of its branches now, the button acts on it, and the button holds :refused while the message is up. Where the refusal goes was measured, not chosen. Beside its own button is where it belongs, and the fit test refused that: at the code column's floor the action row has about 25px of slack once its four actions have taken their widths, and the label was laid out at 25 of the 319px it asked for -- a refusal elided to a sliver is the same defect as the silence it replaces. The footer, one row down, already houses the submit refusal; it did not have room either until the standing shortcuts hint gave way to it, the trade fitActionRow already makes in the row above, and until the message got short enough to fit with the sentence moved into its tooltip. It names both causes because a boolean cannot tell them apart, and naming the wrong one would be worse than naming the pair. assertNothingTruncated now measures refusal labels the way it measures buttons. It looked at .button and the wrapping stale banner only -- and a plain refusal label does not wrap, it elides, which was invisible to both. That gap is why the first placement's defect was findable at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…re away The round-1 re-anchor introduced the defect it was written to prevent, one gesture over. refreshReviewState writes the remembered step list only inside its pathMode branch, so a path that changed while the reader was OUT of PATH mode left that memory stale -- and pressing p then set the cursor to 0 only for the very next refresh to drag it back onto wherever the remembered entry point had gone, leaving them on row 3 with row 1 labelled START HERE. Every deliberate reset now clears the memory through one helper. The rule between the two, written down rather than inferred, because they will keep fighting otherwise: re-anchor when the ground moves under a reader standing still; reset when the reader asks to start again. The cases are told apart by who moved, not by what changed. Two footer refusals could be up at once -- submit refuses, the reader then asks the agent on that same intent, and neither path calls update() -- which left them and Submit sharing one row three ways and the primary action reading "Sub...". They are mutually exclusive now, in both directions, which took two tests to pin because they are two lines. Three measured truncations, all of them findable only because of the elision check the last round added. The submit refusal was already too long for its own footer at the code column's floor. The blocking refusal never fit its action row at all -- 25px of slack for a 146px sentence -- so it shortens to its glyph before it takes the last of the intent title, and keeps the sentence on hover. And fitFooter never consulted the width it had, so a 1400px bar hid "press ? for shortcuts" with hundreds of pixels to spare; no test could see that, because both hints carry the same style class and the one assertion about "the hint" matched the other one's text. Measuring the footer taught me something worth keeping: summing prefWidth under-counted it by a hundred pixels, because the progress bar's 120px floor is a CSS min-width and its preferred width is the fill's seventeen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TestFX hands every class in a JVM the same primary stage, and a stage remembers an explicit size across classes. The tree got away with never setting one -- show() then sized itself to whatever Scene it had just been given -- except that ReviewVerdictBarFitTest has always set one, because the width is what it tests. So a size has been leaking across classes all along, and round 3 did not arm that leak: it moved it onto a value a neighbour could see. ReviewDiffColumnWidthTest asserts a 400-character line wraps, which holds at an inherited 560 and inverts at an inherited 1400, and round 3 handed the stage back at 1400. Nothing in that failure names a width. It surfaces as clicks landing on nothing, or an assertion quietly reading the other way, in whichever class happens to run next -- which is why it has twice been mistaken for flakiness. TestStages.show takes the size from the scene the class already declares, so there is no second copy of the number to drift, and its javadoc carries the rule rather than leaving it to be re-derived. The three hand-backs are gone: choosing a benign value for a leak makes every class depend on a number picked somewhere else. I first applied this to the eleven classes whose assertions read geometry and argued the rest could wait. The full suite refuted that before it could ship -- two narrow-scene classes failed on layout their tests never measure, ten tests between them -- because "asserts on geometry" is a cheap proxy for "depends on layout", and every clickOn depends on layout. So it is all thirty-six. Submit refusals are values now, not four literals, and the floor test loops them: three of the four were elided there and only the fourth was measured. A test that covers one instance of a defect class is how the others ship. INTENT_LABEL_MIN says in its own javadoc that it is a reservation and not a floor -- 96 budgeted, 14 measured, and that is the title yielding as designed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The four-string floor loop was measured in a fixture 35px wider than the bar production gives: ReviewVerdictBarFitTest built the bar at the WINDOW's floor, 560, and the view's chrome takes 35 of those. So "four of four measured" bought six characters of room that do not exist, and two strings were living inside it. The fixture takes the real width now, and a test in the real view fails if production ever gives the bar less than the fixture assumes. Each refusal is also raised through the path that raises it, so nothing rests on substituting one message into another's layout -- which is how the shortening got measured against the wrong Submit label. Both harnesses turned out to be load-bearing: the stale-base string was caught by its own path, and the failed-diff string only by the loop, because that path's fixture happens to show a short "Submit (2 left)" and a twelve-hunk review would not. The ask refusal had the same 35px error one row up, and is a constant now, as the submit refusals already were. A test in ReviewHunkProgressTest was pinned to a phrase rather than the constant and went red when the string shortened -- the drift these constants exist to stop, one file over. TestStages copied the scene's dimensions unconditionally, so a scene built without any pinned the stage at 0x0: a class laying out at nothing and failing while naming no width, which is the exact signature the helper was written to remove. It sizes to content in that case, as the show() it replaced did. The blocking refusal's shortening took three attempts to pin, and the first two are worth recording: at the bar's real width the intent title is squeezed to nothing whether or not the sentence shortens, so the geometric assertion stopped discriminating, and an unshrinkable label does not elide -- it lays the row out past its own right edge, where nothing was looking. There is a check for that now, and the shortening is pinned by what a reader sees: the glyph at the floor, the sentence when there is room. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One optional field, no new tool. drydock renders the assertion and never verifies it, which is the ReviewIntent.Collapse precedent: the agent asserts, the surface shows the assertion, and the evidence stays one click away. Three sources and one rendering path -- reads when it is there, the agent's array order when it is not, the computed path when no agent ran. A reads cycle is kept together and named rather than broken silently, for the same reason a computed one is. And a reads naming an unknown intent rejects the whole batch, because a batch is already all-or-nothing here: half a grouping is worse than none. The rejection happens at DECODE, in intentsFromJson, not where the order is built: Graphs.topologicalOrder does refuse an edge pointing outside its nodes, but it refuses with an IllegalArgumentException on whatever thread IntentGrouping.set was called from, where the agent that sent the payload never hears about it. An MCP error naming the id and the intent that declared it is the report it can act on. The graph's nodes are POSITIONS in the supplied array, not intent ids. Nothing stops an agent sending the same id twice, and a set of ids would collapse those two into one node and lose a card outright. Positions cannot, and they make the tie-break the agent's own array order -- total by construction, and the right answer anyway: where reads says nothing, the order it listed them in is the only other thing it told us. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
stringList answers List.of() for any non-array and drops any non-string
element. reads went through it, so the rejection that was supposed to be
the whole decode-time guarantee could only inspect what survived a lenient
parse -- and what survived from "reads":"the-guard", one dependency written
without the brackets, was nothing.
review_intents then returned {"intents":2} with no error and the rail
rendered the exact REVERSE of the order the agent asserted. The agent could
not discover it either, because reads is echoed on no outbound wire. Absent
and broken looked the same, which is the rule Graphs keeps for an edge
pointing outside its nodes and the reason the decode-time check exists at
all.
readsFromJson decodes reads on its own terms and refuses both shapes: a
present-but-non-array value, and any element that is not a string. stringList
itself is untouched -- its lenience is shared with hunkIds, where a dropped
entry costs at worst one hunk's membership in a group a human can see, and
widening this task to change that is a different decision.
An explicit null stays ABSENT rather than broken: it is how several clients
spell an omitted optional field, and refusing a batch over it would reject a
grouping that declared nothing wrong.
Also: a determinism test over twenty intents whose ids run opposite to their
array order, set twice on the same grouping; getOrDefault where a method
reference could have handed Graphs a null; and a note that the decode-time
number is provisional, since set re-assigns 1..N over the reads order and
discards it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The relevance filter is file-level and lexical and names its own blind spot: a base change that alters behaviour without touching a file this scope names is invisible to it. An agent has no such boundary, so it can answer the one question neither the digest nor the intersection can. The two directions carry different risk and are treated differently. Affected applies -- it only adds reading, and it is how the blind spot closes; an agent wrong that way costs a wasted re-read. Unaffected is advice that never clears a verdict, because an agent wrong that way would cost an approval on code nobody re-read. It is VerdictMerge's asymmetry -- any CHANGES wins, APPROVED needs every hunk -- pointed at a different question, and it is structural here: nothing on this path writes to verdicts at all, so "unaffected" is indistinguishable from never having been asked. SectionStates.stalenessOf consults the recheck SECOND: after the base is known to have moved, and before the filter gets to dismiss the move. So the agent can turn a FRESH or an UNKNOWN into MOVED and nothing else. It cannot invent a move that did not happen either -- a verdict recorded against the current base returns at step one. The wire says hunkId and the store is keyed by hunkDigest, and those are different things: hunkId is positional, HunkDigest is content-addressed and excludes line numbers so a hunk that merely moved keeps its approval. ReviewToolCodec translates, walking the diff the way IntentHunks already does, and refuses -- naming the id -- an id the current diff has no hunk for. A silently skipped entry would be an agent's recheck the human believes happened and did not. The base PAIR is derived rather than taken from the wire: fromBase is the base the hunk's own verdict was recorded against, toBase the scope's base now. The key the tool writes is therefore the key the board reads with. An agent-supplied pair could name commits no verdict was ever judged against, leaving the recheck answering a question nobody asks. Two refusals fall out of that: a hunk with no verdict has no fromBase and nothing decided to undermine, and a scope whose base does not resolve to a commit has no base move to assess. why is agent-authored text that a human will read, so it goes through PromptSafety.checkInboundText like intent.title, finding.body and evidence.code already do. Schema 5: the assessments array is new. loadFromDisk reads each named array on its own terms, so a v4 file needs no migration -- pinned by a test that asserts on submitted, which is read after the assessments and is the first thing a throwing decode would lose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three from review, all on review_recheck's decode. why is required whenever affected is true. A staleness signal asserted with no reason is the reflexive click the asymmetry exists to avoid -- it is worth less than no signal at all -- and it is what a renderer could only draw as a blank warning. Refused before a renderer exists, so the store never accumulates reasonless marks. An unaffected assessment may still omit it: "I looked and it does not matter" asks nothing of a human, so there is nothing for a reason to justify. affected and why are decoded on their own terms, the shape readsFromJson established one task ago. "affected":"true" from a stringifying client -- this codebase already accommodates one in optionalIntArg -- used to decode as false, silently: "the agent looked and found nothing", which is the one answer this tool must never manufacture. Absent and broken must not look the same is the rule this entire surface is drawn around, and it had slipped back in on the tool built to embody it. An explicit null stays ABSENT. The 4->5 bump is pinned by an assertion on the version the store actually writes. Reverting the constant killed nothing before; it kills exactly that test now. Two of the new type tests were vacuous on first write. Sent with affected:true, a lenient decode produced "" and the new needs-a-reason refusal fired instead, so the batch was rejected either way and the test could not tell a type check from a blank check -- it passed with the type check deleted. They send affected:false now, where nothing else can refuse, and assert the message names the type rather than merely that something threw. Descriptor: says why is required when affected is true, and says "already-settled hunks" and "a human's verdict" rather than "approved", since a CHANGES verdict can be marked too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
When the base moved under a review, drydock marked the affected approvals stale and stopped there. The reviewer came back to a row of "base moved" banners with nothing said about which of them mattered, and the only way to find out was to re-read every one or type the request by hand -- so the assessment arrived after a wait, exactly when they wanted to move on. A base move now dispatches its own recheck. It is a small bounded task by construction -- one base delta and the stale hunks, not the change -- which is why it earns a dispatch rather than a full re-review. The instruction says outright that "unaffected" does not clear an approval, because an agent should be told the rule rather than left to infer it from what review_recheck happens to refuse. The plan put this in SessionReviewView keyed on AnnotationStore's assessedAffected. Two constraints ruled that out. SessionReviewView has no AgentRegistry to resolve subagent support through, and its baseMove seam is called on the FX thread and must not block. More seriously, assessedAffected cannot deduplicate the dispatch at all: it returns false both for "the agent said unaffected" and for "the agent was never asked" -- deliberately, since only true may add staleness -- so it cannot see a dispatch still in flight. refreshReviewBoards re-renders every open board whenever a background git answer lands, and every one of those renders falls inside that window, so deduplicating on the store alone would spend a subagent per render. That is worse than the per-base-move flood the gate exists to prevent. So the memory is RecheckDispatch, a claim/release record mirroring the baseMoveInFlight set it sits beside, and the decision is SectionStates.requestRechecks, driven once per render. It gates on a section's staleness already being other than FRESH, which reuses BaseMove.couldMatter's file filter rather than repeating it -- the two cannot drift apart. A hand-off that returns false is released again: the send reached no terminal, and no human is watching an automatic recheck, so a swallowed failure is a scope that silently never gets one. Neither the dispatch nor its result can reach a verdict. requestRechecks only reads verdicts, and the recheck's answer lands through review_recheck as an assessment, which remains the one direction that can add staleness and never remove it. MainWorkspace.supportsSubagents is extracted from reviewInstruction so the review and the recheck resolve subagent support one way instead of two. No behaviour change; it is here rather than in its own commit because the recheck is its only new caller. Verified with ./gradlew :app:test --rerun-tasks: 2043 tests, 204 classes, 0 failures in 6m31s, against a baseline of 2023/202/0 confirmed the same way at 8487310. The +20 accounts exactly as 6 + 8 + 6. Mutation testing found four assertions that could not fail, each a test varying two things at once: forRecheck's bases were checked separately, so a genuine swap read identical; RecheckDispatch's key test moved both bases together, pinning neither component nor the separator; and requestRechecks without its staleAgainst filter dispatched fromBase == toBase, spending a subagent on an empty diff. All four are now pinned by tests that fail on the mutant and pass on the fix. Not covered: the wiring inside MainWorkspace.dispatchRecheck itself, which has no test -- MainWorkspace has one test in the whole class. It is kept thin for that reason, and everything decidable was pushed behind the Host seam where FakeReviewHost drives it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Consistency of rendering is right; consistency of warrant is not. A measured edge fails as a false unique-name match -- two unrelated things sharing a name -- and a reviewer can check it by looking. A claimed edge fails as a plausible fabrication and is checkable only against the code the agent says it read. Someone deciding how hard to squint at "③ depends on ①" has to know which of those they are holding, and until now the rail rendered both identically. Not a new principle, only its consistent application: ReviewIntent.Collapse already renders the agent's assertion as an assertion, precisely because drydock does not verify it. The marker goes on the RAIL, which is where the warrant actually varies. Spec §7.1 names three order sources -- `reads`, the agent's array order, and ReadingPath -- of which the first two are the agent's claim and the third is drydock's measurement; hasReviewerGrouping is exactly that split, so the rail takes a Provenance and the view derives it there. It is deliberately NOT on a path row. Spec §6.4 has ReadingPath order the computed grouping only, and currentPath() bears that out: it calls ReadingPath.of over Sections.of(diff, graph), built fresh from the diff, never from the agent's intents -- `reads` reaches IntentGrouping.orderByReads and nothing else. A path row is measured by construction, so marking one would be the only place on this surface where the marker could lie. The plan's own test asserted the opposite -- a CLAIMED path step after agent-supplied intents declaring reads -- and cannot pass without changing where the path's sections come from, which the plan never proposes. Following the Task 1 precedent, the spec wins and the test is re-shaped to the rail. The plan's stylesheet path was also wrong: app/src/main/resources/app.css does not exist, and the rule belongs in app/drydock/ui/app.css, where both its selectors are really added (ReviewIntentRail:491, ReviewDiffColumn:1755). ReadingPath.Step and Link still gain provenance(), constant MEASURED today, because §6.3 has labels carry their provenance and §14's checklist requires a scope holding an agent grouping AND a computed link set to mark each correctly -- a claimed rail beside measured links. Both records take it through a secondary constructor defaulting to MEASURED, the same shape Task 21 used for `reads`, so the ten test construction sites stay as they are and only the two production sites name the warrant. Dashed rather than coloured: four risk encodings already compete for colour on this surface and a fifth would be unreadable. Only the claimed case is modified, since decorating every row would say nothing. Verified with ./gradlew :app:test --rerun-tasks: 2049 tests, 206 classes, 0 failures in 6m29s, from 2043/204 -- the +6 is 2 + 3 + 1. Mutation testing killed every-card-marked, no-card-marked, and view-always-claims. A fourth mutation, PATH mode not resetting the warrant, SURVIVED, and it was right to: buildPathRow never consulted provenance, so the reset was unreachable. It is deleted rather than kept, and the path test was re-checked against a buildPathRow that DOES apply the modifier, where it fails -- so it guards the real risk and not an accident of the current code. Checked visually, not only modelled, per the phase gate: rail snapshots in both warrants show dashed borders against solid ones, legible without opening a tooltip. Not covered: the diff column's link rows carry the CSS selector but can never match it while every link is measured, so that half of the rule is unexercised until a claimed link exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of the previous commit found the relevance gate did not work in the running app. It skipped only FRESH, and staleness has three values: the production host answers UNRESOLVABLE on the FIRST call for any base pair -- it spawns the git off the FX thread and replies on a later render -- which renders as UNKNOWN, not FRESH. So the very render that discovered a base move dispatched, before couldMatter had said anything at all, and the claim is permanent: when the real answer landed one render later saying the move was irrelevant, nothing could retract it. Every base move spent an agent run, which is the flood the gate was written to prevent. No test caught it because FakeReviewHost.baseMove returns an always-resolved field and cannot express the in-flight state the real host returns every first time. The suite already asserted UNKNOWN for that case forty lines away; it was simply never carried into the dispatch path. The gate now requires MOVED, and UNKNOWN means ask again next render, by which time git has spoken. Three more defects from the same review: "unresolved" is not a revision, and the guard existed only for the CURRENT base. A verdict carries the sentinel as its RECORDED base whenever the baseline was unresolved when the human settled the hunk -- while git was still answering, or permanently when resolveRef failed -- and that reached the agent as "read what changed between unresolved and <sha>", a request nothing can satisfy, one-shot because the claim was already taken. dispatchRecheck now refuses a tab whose agent process has exited, the way requestHandoffRefresh and the Explorer bridge already do. sendToBoundSession answers "a tab object exists", not "the agent received it": TerminalBridge returns void and swallows a closing surface, so typing into a dead terminal returned true, the claim stood, and the recheck was lost with nothing logged. The two senders a human drives get away without the check because a human notices the reply never comes; nobody is watching this one. And the feature could be deleted in silence. Removing the requestRechecks call from the render pass left all 322 review-UI tests green, and gutting MainWorkspace.dispatchRecheck to `return true` left the suite green too -- ship both and the feature is inert with nothing failing. ReviewRecheckDispatch Test drives a real render, so the first of those now fails. Two rulings, both against the plan: Spec §9.7 says "inline harnesses simply do not get one" and the plan said the opposite. The spec wins, per the Task 1 precedent. The tempting middle option -- dispatch inline but only when the agent is idle -- is not available: Codex and Pi both return Optional.empty() from AgentProvider.activity(), and Claude, the only provider with subagents, is the only one that reports activity at all. The harnesses that would need an idle signal are exactly the ones with none. The in-memory claim dies with the view while the stale mark outlives it, so every restart re-asked a question already answered on disk. AnnotationStore gains assessedMove, which is deliberately NOT assessedAffected: that one folds "said unaffected" into "never asked" because only true may add staleness, and dispatch needs the other question. Smaller: only APPROVED and AUTO_APPROVED verdicts trigger it, since the instruction says "for each approved hunk"; counted(board) replaces board.sections() so a collapsed section spends nothing; forRecheck null-checks both bases, which are concatenated into a line typed at a prompt; and the NUL-joined key is a record Move, which deletes the argument about which byte is impossible in a scope handle rather than defending it. Verified with ./gradlew :app:test --rerun-tasks: 2060 tests, 207 classes, 0 failures in 8m53s, from 2049/206. Ten mutations, each with the mutated source printed and confirmed, all killed by the test written for them -- including the two the review found surviving (the gate itself, and collecting only the first distinct base). Not covered: MainWorkspace.dispatchRecheck still has no test, so the liveness guard and the instruction assembly are unexercised -- gutting it to `return true` still leaves the suite green. That class has one test in total. It is the next thing an end-to-end run against a real agent has to exercise. Also worth knowing for anyone writing a multi-scope test: ReviewScopeRegistry .mint does computeIfAbsent on the spec identity, so minting an "identical" scope returns the SAME handle. The first version of the two-scope test was comparing one scope against itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous fix moved the gate to MOVED but left it on the wrong thing. A section's staleness is the strongest claim true of any of its hunks, and the loop that collects bases runs per verdict -- so one hunk whose move was confirmed dragged in every other stale approval in the same section, whatever that approval's own base delta said. Re-review demonstrated both halves: a neighbour whose git was still in flight was asked about before couldMatter had answered (the original defect, intact), and a neighbour whose move was resolved and touched only docs was asked about too, which is exactly what the filter exists to prevent. The comment claiming "UNKNOWN means ask again next render" was false as written. The relevance test now runs inside the digest loop, per verdict. The section check stays as a cheap pre-filter and says so: a section with no MOVED hunk can hold no MOVED verdict, but the converse does not follow, and that gap was the bug. FakeReviewHost.baseMove returned one delta for every recorded base, so no test could express two approvals resolving differently. That is the third defect on this branch to hide behind a fake collapsing a distinction the real host makes -- it memoizes one delta per (scope, oldBase, newBase). The fake now answers per recorded base, and the two tests this makes possible both fail against the section-only gate. Two deletions the review earned. AUTO_APPROVED is gone from the decision filter: nothing stores such a verdict, VerdictMerge derives it for display only, so the clause was unreachable. And forRecheck loses its supportsSubagents parameter -- once dispatch is gated on subagent support the inline form cannot be reached, and keeping it meant tests defending code that can never run. One case is knowingly forfeited and now says so in the code: a base move that is PERMANENTLY unresolvable, the old base force-pushed away, stays UNKNOWN and is never asked about. BaseMove.Delta cannot tell "in flight" from "gone", and an agent handed a base git can no longer resolve could not answer anyway. Separating them needs a third state on Delta, which is a design change rather than a condition to bolt on here. Also documented rather than left to be rediscovered: the release-on-false retry is deliberately unbounded, because the check short-circuits on a dead tab before typing anything and a silently abandoned recheck is the failure this path exists to avoid; and assessedMove proves "some assessment exists for this pair", not "every approval at this pair was answered" -- review_recheck is agent-initiated and may answer partially. Verified with ./gradlew :app:test --rerun-tasks: 2061 tests, 207 classes, 0 failures in 8m44s, from 2060/207. Reverting the per-verdict filter to the section-only one kills both new tests. Still not covered, unchanged from the last commit: MainWorkspace .dispatchRecheck and supportsAutomaticRecheck have no tests -- removing the liveness guard, or making supportsAutomaticRecheck return true so inline harnesses get dispatched, both leave the suite green. Nothing in the test tree constructs MainWorkspace. An end-to-end run against a real agent is the only thing that exercises them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-review approved the per-approval relevance fix and left two smaller things, both about code that asserted more than it enforced. The section-level pre-filter is gone. It was introduced as a cheap short-circuit and was neither: stateOf performs a strict superset of the work the guarded loop does -- the same stalenessOf for every digest, plus collectSharingSections walking every other section and its digests, plus a VerdictMerge and an allocation -- and only digestsByIntent is memoized, so guarding with it could only ever add work to every render. It was dead as well as expensive: deleting it changes no test, because a section is MOVED exactly when one of its hunks is, so "no hunk MOVED" already implies "no verdict MOVED". Calling it cheap was the mistake; the loop now says why there is no guard rather than leaving the next reader to add one back. The decision filter now reads "not CHANGES" instead of naming the approving decisions. The previous commit dropped AUTO_APPROVED on the grounds that nothing stores one, which is true today and enforced nowhere: putVerdict accepts any Decision, and the verdict load path accepts "auto-approved" straight off disk. An enumeration would therefore silently stop asking about an approval the day a new writer, or a hand-edited annotations file, produces one. Inverting it fails toward asking, which is the direction that costs least when wrong: one extra agent run against a human's approval going unexamined with nothing said. Mutating the filter away still fails aRequestedChangesVerdictEarnsNoRecheck, so it has not gone slack. Verified with ./gradlew :app:test --rerun-tasks: 2061 tests, 207 classes, 0 failures in 6m34s -- unchanged, as expected for two changes neither of which was meant to alter behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
reviewkey has been documented in the explorerScript hook since Task 18 -- "deliver one KeyCode to the Review view, so the ⏎/esc page transitions can be driven" -- and never had a case in the switch. It fell through to the default branch, which prints "[diag] mark <arg>". A driver script asking for reviewkey:A therefore printed a plausible beacon and did nothing at all. Found by using it: the first end-to-end run of the automatic recheck showed a fully loaded Review view sitting at "0/1 hunks reviewed" while the log claimed a key had been delivered. Any earlier visual verification that leaned on this verb proved less than it appeared to -- the same shape as the note about Robot input in diag runs, where the verbs report success and nothing reaches the app. A verb that only reports what it did is not a test. SessionReviewView.diagReviewKey fires a real KeyEvent through the same addEventFilter(KEY_PRESSED) path a physical press takes, rather than calling the settle action directly, so a driver exercises the routing and the focus rules too -- which is the part with no unit coverage. Verified by use, not by assertion: the next run logged "[diag] reviewkey A delivered" and the annotation store gained a real approved verdict stamped with the resolved base commit, where the previous run had produced no store at all. Full suite unaffected: 2061 tests, 207 classes, 0 failures in 6m26s (--rerun-tasks, all 207 result files written by that run). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of Task 24 approved the modelling and failed the half the task's own
test javadoc calls for: "the distinction has to be visible, not merely
modelled."
The claimed border was not legible. -drydock-border is rgba(255,255,255,0.08),
about 1.26:1 against the rail, so dashing it removed half the ink of a
hairline nobody can see -- Task 18's 1.13:1 defect in a new place. I had
looked at two snapshots and called it clearly dashed; I was reading enlarged
PNGs rather than judging what a reviewer sees. The claimed border now also
lifts onto -drydock-text-faint. Neutral on purpose: the reason for not using a
hue is that four risk encodings already compete for colour here, and a grey is
not a fifth.
Nothing pinned the visible half either. Deleting the whole CSS rule, and
dropping the warrant from the tooltip, both left every review-UI test green --
they all asserted on style-class STRINGS, and a class nothing renders says
nothing. Two tests now read the resolved Border and the tooltip off the live
scene, so both deletions fail.
Spec §9.7 was unimplemented: "Assessments render as claimed, not measured."
A hunk stale because an AGENT asserted the base move disturbed it was
pixel-identical to one stale because drydock's own file-level filter found the
move -- exactly the confusion §6.5 exists to prevent, and the reviewer's
argument for accepting "affected" at all is that it is the agent's judgement
closing §9.2's blind spot. SectionState now carries the warrant, set only when
an assessment is what made a hunk MOVED, and the chip reads "⚠ agent: base
moved — confirm". That string is LONGER than the measured one, so it gets a
narrow-width card test: this rail has truncated before.
Smaller, all from the same review: the .review-link-row.provenance-claimed
selector could never match anything (nothing adds that class to a link row,
and by §6.4 a link is measured by construction) and is deleted rather than
parked; setIntents' null-coalesce was a dead branch and is a requireNonNull;
the convenience constructors' MEASURED default is pinned, because every
default in this design points at the MORE trusted value and flipping both
survived the suite; and the tooltip naming the warrant on every card is
reconciled with the "do not decorate every row" rule in the comment rather
than left to look like a contradiction.
Four citations pointed at §7.1 for a sentence that is in §8 ("Three sources,
one rendering path -- and the first two are marked claimed while the third is
marked measured"). The substance held; the justification pointed at the wrong
section.
Verified with ./gradlew :app:test --rerun-tasks: 2069 tests, 207 classes, 0
failures in 6m29s, from 2061/207. The run before it failed once on
ReviewSettleActionsTest.withTheDiffColumnFocusedApproveSettlesOneHunk
(expected 1, got 3) -- the focus race characterised at Task 22, verbatim. It
passed twice in isolation and did not reproduce on the full re-run, so it is
that flake and not a regression from touching the rail.
Mutation-verified: deleting the CSS rule, dropping the tooltip label, flipping
the constructor defaults, dropping the CLAIMED assignment, and removing the
chip's "agent:" prefix are each killed by the test written for them -- the
last two at both the model and the screen.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of Task 23 noted this path was silent in both directions: SectionStates
has no logger, sendToBoundSession returns a boolean nobody records, and a
recheck that silently never happened looked exactly like one nobody needed.
It is the only dispatch on this surface with no human watching it land, which
is precisely the one that cannot afford to be quiet.
It now logs which, and names the base pair it was about.
Written for the end-to-end run, and immediately earned by it. With a WORKTREE
scope (probe vs main), a hunk approved against 055157a, and main advanced to
a70cb80 with a commit touching the same src/guards.cpp the scope's diff
carries:
INFO: Dispatched a recheck for scope rs_MHBKSQE2YR0MQD2EJH
(055157aa... -> a70cb800...)
That is the first execution of MainWorkspace.dispatchRecheck outside a fake --
subagent support resolved from the bound Claude session, the liveness guard
passed, the instruction assembled, and the hand-off returning true against a
live terminal. All four were assertions until now; the class has one test in
the whole repo and every mutant of this method survives.
The rail agreed: "⚠ base moved — confirm" with "approved against base 055157a
· base is now a70cb80", and progress fell to 0/1 because a stale hunk stops
counting as settled. The chip read "base moved", not "agent: base moved",
which is correct -- drydock's own filter found this move -- and incidentally
exercised Task 24's new warrant in the same run.
Not covered: the agent did not answer (assessments stayed 0). The instruction
reaches the terminal; whether Claude acts on it needs MCP permissions granted
ahead of time or a human present, the same wall the Task 24 run hit. The
drydock half of §9.7 is verified; the agent round-trip is not.
Verified with ./gradlew :app:test --rerun-tasks: 2069 tests, 207 classes, 0
failures in 6m28s, 0 result files older than the run start.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two of the three script dispatchers ended in
`default -> println("[diag] mark " + arg)`, so any verb without a case became
a marker: a driver script asking for one printed a plausible beacon and did
nothing. That is how `reviewkey` and `comment` sat documented-but-unwired
since Task 18, and how a screenshot run that approved nothing looked exactly
like one that worked. `settingsScript` already ended in "unknown settings
verb"; the other two now match it, and name the verb they did not recognise.
`mark` had to become a real case first. It WAS the default, so replacing the
default outright would have silently broken every synchronisation marker in
every existing driver -- the defect's own mechanism was load-bearing for a
legitimate verb.
`comment` is wired at the same time, and it was not missing an implementation:
ReviewDiffColumn.diagOpenComposer() was already written, routed through
ReviewDiagFxThread, returning the anchor it resolved to, with a "no changed
line to comment on" fallback -- and it had ZERO callers. Only the case label
and two hops stood between it and working, which is a harder gap to notice
than an absent method: anyone grepping found a real, careful, well-documented
implementation and would reasonably conclude the verb worked. It matters
because the composer is opened by a click on a 34px label inside a virtualized
cell that the harness cannot aim at, so without this there is no way to drive
or photograph a gutter comment at all.
Verified by running it, not by reading it -- a fix to an error-reporting path
that was only read would be the very mistake it exists to prevent. One script
carrying a real mark in each dispatcher, a bogus verb, a one-character typo of
a real verb, and a genuine comment:
[diag] UNKNOWN tabScript verb 'definitelyNotAVerb' -- nothing was done.
[diag] mark from-tabscript
[diag] mark a-real-marker
[diag] UNKNOWN explorerScript verb 'reviewkeyy' -- nothing was done.
[diag] comment -> composer on src/guards.cpp n11
[diag] mark done
The typo is the case that matters: under the old default `reviewkeyy` printed
"[diag] mark A". The composer was confirmed on screen -- input field anchored
"guards.cpp line 11" -- not from that log line.
Full suite: 2069 tests, 207 classes, 0 failures in 6m20s, 0 result files older
than the run start.
Not covered: this makes FUTURE unwired verbs loud, it does not audit the
existing ones. Two of the eight verbs I checked were broken; explorerScript
has roughly two dozen cases and tabScript its own set, and no test anywhere
asserts that a documented verb has a case.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot run
The dispatchers now name an unrecognised verb at runtime, which only helps
somebody who runs it. `reviewkey` was documented from Task 18 and unwired for
two months precisely because nobody did. This asserts it at build time: every
verb the DrydockApplication comments advertise has a case that implements it.
One-directional on purpose. The comments document a SUBSET -- the explorer
block lists six of roughly two dozen cases -- so an unlisted case is fine and
a listed verb with no case is not.
The test found a bug in itself before it found anything else. It reported
`tslider` as unwired; `tslider` is wired, through `case "uislider", "tslider"
->`, and a bare `case "x"` match cannot see a multi-label case. Trusting it
would have meant "fixing" working code. It now collects every literal in a
`case ... ->` label.
It was then silently blind twice, which is worth recording because it is the
same shape as the defect it exists to catch: a verb the parser misses is not
reported, it is simply never checked. The prose anchor never matched -- the
sentence ends `:saved". Verbs:`, not `steps. Verbs:` -- so the explorer block
contributed nothing. With that fixed, line-wrapped entries were still skipped:
`type` ends one line and `(insert ...)` begins the next, so a contiguous
"type (" never appears. The block is unwrapped before matching now.
Both fixes are mutation-verified, not assumed: unwiring `reviewkey` fails the
test, and so does renaming BOTH `case "type"` labels -- one alone does not,
since two dispatchers implement it, which is itself a reminder that a mutant
has to be confirmed rather than assumed.
It parses 25 verbs. A guard fails if it ever parses none, so a comment-format
change breaks the build instead of quietly disabling the check.
Not covered: this catches DOCUMENTED-but-unwired. It would not have caught
`comment`, whose implementation ReviewDiffColumn.diagOpenComposer() was fully
written with zero callers and which no source comment ever advertised -- an
unreachable implementation is a different shape, and would need a check that
every diag hook has a caller.
Full suite: 2071 tests, 208 classes, 0 failures in 6m29s, 0 result files older
than the run start.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jbachorik
force-pushed
the
feat/review++
branch
from
August 26, 2026 12:54
0c29e01 to
9527abe
Compare
withTheDiffColumnFocusedApproveSettlesOneHunk has failed the same way on both CI runs -- expected 1, got 3, i.e. the whole section settled instead of one hunk -- but passes every local run, isolated and full suite. That shape means settleUnit() read SECTION instead of HUNK, which only happens if Scene focus never actually landed inside the diff column after focusDiffColumn()'s click. diagFocusSnapshot() reports the live focus owner, whether it's inside diffColumn, and settleUnit()'s reading; the test now captures it right after the click and right after the key press and folds both into the assertion message, so the next CI failure's report shows what focus actually was instead of just the wrong count.
…on CI diagFocusSnapshot() (previous commit) showed the CI failure's real shape: focus was never inside the diff column at all, even immediately after focusDiffColumn()'s click and before any key press. Root cause: showing a diff kicks off a ChangeGraph build on a background executor (SessionReviewView#requestGraph). Its completion -- success or failure -- runs refreshReviewState() from the FX thread, which rebuilds the rail's cards and touches the diff column. The shared ReviewViewFixture's @beforeeach never waited for that build to settle before handing control to a test method, so every test built on it raced an async computation that has nothing to do with what it was testing. On the CI runner the build apparently finishes right as the test's click lands; locally it never has, in any run tried, isolated or full-suite. diagGraphBuildPending(scopeId) exposes the in-flight state; the fixture now waits for it to clear (and drains the resulting FX events) before returning from showBoard(), closing the race for every test built on this fixture -- ReviewSettleActionsTest, ReviewPathModeTest, ReviewProvenanceTest, ReviewRecheckDispatchTest -- not just the one that happened to expose it on CI. Full local suite passes.
…a fixed drain count The previous fix (waiting for the async ChangeGraph build to settle before a test starts) was real but not the cause: the next CI run failed identically -- focus never inside the diff column, even immediately after the click, before any key press -- proving the race isn't with that background build at all. The real mechanism: focusDiffColumn's clickOn is a real TestFX robot press. Monocle turns it into an FX MouseEvent on its own schedule, off the calling thread. A single waitForFxEvents() after the click only waits for whatever was ALREADY queued when it's called -- if the click's translated event hasn't been posted yet, it returns having waited for nothing. That explains why some tests using focusDiffColumn() passed on CI and others didn't: the passing ones happened to have a second, logically redundant waitForFxEvents() later that incidentally covered the gap. diagFocusInDiffColumn() exposes the real postcondition -- Scene focus actually inside diffColumn -- and focusDiffColumn() now polls it with a bounded wait instead of guessing how many drains are enough. Full local suite passes.
…code on CI The postcondition-wait fix proved the click's focus effect never happens on CI within 5s -- not a timing issue, since a real wait now times out instead of racing. That rules out "just needs more time" but not WHERE in the pipeline it breaks: TestFX's Robot-driven click could be failing to reach the FX layer at all, or reaching it and getting diverted before settling on the diff column. Two temporary diagnostics to tell those apart: - ReviewDiffColumn's MOUSE_PRESSED filter on `list` now logs its target and scene coordinates whenever it fires -- confirms whether the click physically reaches production code on CI at all. - ReviewViewFixture.focusDiffColumn() logs every ".review-diff-cell" match's empty/visible/bounds state before calling clickOn, since clickOn(String) picks whichever match lookup() returns first and a virtualized ListView's cell pool holds empty placeholder cells alongside populated ones. Confirmed locally that an empty-cell target is not itself the problem: the filter lives on `list`, not the individual cell, so it fires (and the test passes) even when clickOn happens to land on an empty placeholder row. Both remain temporary -- remove once CI shows what the CI-only case actually does differently.
CI run 32986189657 (workflow_dispatch retry after an isolated GitHub Actions hiccup stuck the normal run queued with no runner for 20+ minutes) ran app:test clean: all 2123+ tests passed, including ReviewSettleActionsTest. The two real fixes -- waiting for the async ChangeGraph build to settle (diagGraphBuildPending) and polling the actual focus postcondition after focusDiffColumn's click (diagFocusInDiffColumn) -- resolved it. The single TimeoutException seen on the previous commit's run most likely landed during that same patch of CI degradation, pushing an already-marginal click delay past the 5s wait. Removing the two prints that were only ever meant to localize where in the pipeline the click was failing (production filter fire, and pre-click cell-pool state) -- their job is done. Keeping diagFocusSnapshot as a lasting diag accessor: cheap, and exactly the kind of thing worth having on hand if a different focus race ever surfaces.
The previous claim that this was fixed was wrong: it was one green run
among many failures, and removing the temporary diagnostics right
before it failed again lost the observability that mattered most.
Restoring both prints, and adding one more: a TimeoutException from
focusDiffColumn now carries diagFocusSnapshot() instead of a bare
message, since a plain timeout says only "it never happened."
Also trying a real, evidence-based change rather than another blind
wait tweak: withAGutterSelectionOpenApproveSettlesTheSelectedHunkNotTheAnchor
is the one test in this class using moveTo() + a separate press() (on
the gutter, not clickOn) and has never once failed on CI across every
run so far, while every test going through the compound
clickOn(".review-diff-cell") has. focusDiffColumn now uses the same
moveTo+press+release shape. Not confirmed as the fix -- needs CI, and
this time won't be declared resolved off a single green run given the
history here.
The previous run's diagnostics (moveTo+press+release, commit d9277bd) cut ReviewSettleActionsTest CI failures from 3 to 1, and proved something important: the MOUSE_PRESSED filter fired and list.requestFocus() ran for all four diff-column clicks in that run -- including the one that still timed out. All four produced a "[diag] review-diff-list MOUSE_PRESSED" line, with the failing one landing on a populated cell, same as the passing ones. So this is not a click-delivery failure; something downstream hands focus to a Button afterward, and "focusOwner=Button" alone never said which one. diagFocusSnapshot now describes the focus owner as SimpleClassName[#id][.styleClass...]("text") via diagDescribe, plus its full ancestor chain via diagAncestorChain -- enough to trace back to the exact code path that grabs focus, on the next CI failure.
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.
Summary
Reworks review navigation around a single question: where should the reviewer start, and what follows? The reviewer no longer walks a diff file-by-file — the change is grouped into sections derived from the code's structure, ordered by a change graph, and each approval is pinned to the content it was actually given against.
What changed
Grouping and order
pwalks the change in reading order.Approvals that stay honest
Verdict bar and PATH mode
a,r,usettle the row on screen; the bar fit-checks its own height and the stale label.MCP surface
review_scopecharges sections against the budget and degrades gracefully; it can hand the agent the grouping it is being asked to name.review_statereports a verdict under the intent's own id and degrades gracefully when a scope's diff cannot be produced.affected, are refused.Diag
mark, and a documented verb without a case fails the build rather than a screenshot run.Test plan
🤖 Generated with Claude Code