feat(quiz): comprehension quiz for the current diff (#191) - #192
Merged
Conversation
… phase 1)
Phase 1 of the comprehension-quiz feature: the agent-facing half, headless.
A reviewer will ask for a quiz about the current diff; the agent authors it
and submits it here. No UI yet — Phase 2 wires the server and the view.
The shape clones two existing patterns rather than inventing machinery:
quiz.jsonl is agent-owned and append-only like suggestions.jsonl (so it never
races the server's comments.csv), quiz-answers.jsonl is server-owned, and the
two are merged at load — the one-writer-per-file rule from thread.go.
The load-bearing decision is WHERE the quality contract lives. Quiz prompts
are a user-extensible library (built-ins plus ~/.config/prereview/quizzes),
so no guarantee may depend on the prompt's text. Structure is enforced by
ValidateQuiz in the review package — known probe, >=2 options, answer index in
range, non-empty explanation — which binds whatever prompt produced the quiz;
grounding (does the cited line range exist?) is left to the server, the side
that holds the diff. A sloppy custom prompt can make a quiz boring, but not
structurally invalid or hallucinated.
Two subtleties worth flagging for later phases:
- `decision` questions may be about an ABSENCE ("chose not to add a test"),
which has no lines to point at, so from_line 0 is legal for that probe only.
That state must stay distinct from "claimed an anchor that doesn't resolve":
one is deliberate, the other is a hallucination, and collapsing them would
let hallucinated anchors hide behind a legitimate label.
- Side is defaulted on BOTH the write path (NormalizeQuiz) and the read path
(loadQuizzes), because the file is append-only and a line can arrive without
going through the CLI. A question with no side jumps to the wrong row of a
modified line.
Audit findings that changed the plan: Comment.Hidden turned out to be
"dismiss a RESOLVED comment", meaningless on unresolved ones, so a quiz
request cannot be silently hidden — Phase 4 will clone the Save path and the
request comment will be visible, matching how a saved Prompt comment already
behaves.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
Wires the server half and the UI. An agent-written quiz.jsonl now appears
live in a new full-screen view (peer of the all-comments pane), and the
reviewer can answer it, see why each answer is right, and jump to the code a
question cites.
The load-bearing addition is GROUNDING — the anti-slop lever the CLI verb
could not enforce because only the server holds the diff. Each question
claims a line range; applyQuiz checks that line actually exists in the file
under review and stamps one of three states:
ok → renders a jump link to the cited code
ungrounded → the question cited a line that is NOT in this diff, i.e. a
hallucinated anchor. Rendered as a warning, and deliberately
offered NO jump link: the link would land on unrelated code.
absent → a `decision` question about an OMISSION ("chose not to add a
test"), which has nothing to point at. Legitimate, not a
warning.
The last two must never collapse into one state. Both render without a jump,
but one is deliberate and the other is a defect; giving them the same label
would let a hallucinated anchor hide behind a legitimate one. They are
distinct in Go, in the template, in CSS, and are pinned by both a render
golden and a browser test.
Answers are the reviewer's, so they go in a server-owned quiz-answers.jsonl
rewritten atomically — never into the agent's append-only quiz.jsonl. One
writer per file, per the thread.go rule.
Scope note: phase 3's core (answering, scoring, explanation reveal, retake,
jump-to-cited-line) landed here rather than separately, because a quiz you
cannot answer is not reviewable. Phase 3 retains staleness detection.
Verified beyond the unit suite, since three defects here were invisible to it:
- `prereview quiz --file -` did not read stdin, though done/reply/applied all
document "-" for it. Only the e2e drove the verb the way an agent does.
- A stray list marker painted left of every option (Pico re-asserts a marker
on the nested <ul>). The DOM tests pass either way; only a screenshot shows
it. Fixed by resetting list-style on the items, not just the lists.
- Dark mode was unguarded — the contrast walker never opens this view — so it
was checked by screenshot too. No Pico fall-through; all tokens flip.
All 151 browser tests pass. Render goldens gained quiz + quiz-empty fixtures;
the only pre-existing golden movement is the new `z` keybinding, traced in
both the markup and the wire-format payloads before regenerating.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
#191 phases 4-5) Closes the loop. Until now a quiz could only arrive by hand-writing JSON; now the reviewer taps "Quiz me" in the file header and the agent answers. REQUEST. RequestQuiz saves a kind=file comment whose body is the quiz prompt, riding the ordinary comment queue — no new comment kind, no second channel, exactly how a saved Prompt comment (#147) already reaches the agent. It sends on one tap rather than pre-filling the composer, because this tool is mostly driven from a phone and there is rarely anything to tweak. The request is therefore VISIBLE in the reviewer's own queue: an earlier design hoped to hide it behind Comment.Hidden, but that flag only applies to RESOLVED comments, so there is no way to hide it and no reason to invent one. The visible row doubles as a record of what was asked, with the agent's reply threaded under it. The control degrades by library size: one prompt renders a plain button, two or more render a picker. Both shapes are pinned by render goldens. ADVISORY RECORD. The snapshot gains a `quizzes` array — file, score, total, and `taken`. It never gates anything; it lets the agent tell "accepted after a comprehension check" from "accepted without one", which is the entire point, since an accept otherwise records only that the reviewer clicked. `taken` is a separate field rather than inferred from a non-zero score because "never opened it" and "answered everything wrong" both serialize as 0/N and mean opposite things. Nil when no quiz exists, so `omitempty` leaves the wire format byte-identical for every review that does not use the feature. DEFERRED, deliberately: staleness detection. It needed a fingerprint of the quizzed content, but the server cannot compute one retroactively (it cannot distinguish a first load from a reload-after-edit) and the agent cannot compute prereview's hash — making it work means asking the agent to shell out a sha256 and trusting it hashed the same bytes. Grounding already covers the visible symptom: edit the file and the cited lines stop resolving, so those questions render ungrounded. Reasoning recorded in the plan. One regression caught by the full sweep and worth naming, because the failure mode is nasty: "Quiz me" initially reused the `.prompts-trigger` class for visual consistency. That made the selector ambiguous and silently re-pointed TestE2E_PromptPicker's click at the wrong button — which did not fail, it HUNG, waiting for a dropdown that never opened, and took the whole suite past its timeout. The quiz control now carries its own class with the styling shared via grouped selectors, and a guard asserts `.prompts-trigger` matches exactly one element so a recurrence fails in seconds instead of hanging. All 152 browser tests pass (440s, 0 skips); full unit suite and both golden guards green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
adnaan
changed the title
…191) Reported from a real phone: tapping "Quiz me" appeared to do nothing, so the reviewer tapped again and queued two identical requests. They were right to. The tap saved a comment and changed NOTHING visible — on a narrow viewport the queue sits behind a menu, so the only feedback was silence, and silence reads as "it didn't work". Two changes: - The tap is now acknowledged, with a toast through the existing Flash machinery ("Quiz requested — the agent will answer shortly"). - While the request is unanswered the button is replaced by a non-tappable "Quiz requested…", so a second tap CANNOT queue a duplicate. It returns once the quiz arrives, so another can still be asked for. QuizRequestedFile is a session-scoped UI hint, deliberately not persisted: worst case a reconnect re-offers the button, which is merely the old behaviour. Worth recording why the test suite missed this. All 152 browser tests passed with the bug present, because every one asserted the STATE TRANSITION — a comment was saved — and none asserted that a human could TELL it had happened. "The action worked" and "the user can see that it worked" are different properties, and only the first was covered anywhere. The new test is written from the report and runs at phone width, asserting all three parts: the tap is confirmed, no second request is possible while one is pending, and the control recovers afterwards. It fails without the fix. 153 browser tests pass (0 skips); unit suite and both golden guards green. Only the signature golden moved — QuizPending is false in every fixture, so rendered markup is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
#191) Found on a real phone. The quiz request carried the ENTIRE built-in prompt as its comment body — ~40 lines — and a kind=file comment renders in full in the file view. Opening the file showed a wall of instructions where the code should be. On a narrow viewport it filled the screen. This is the visible-request trade-off the previous commit accepted, and it was worse in practice than judged: "the request is visible" is fine, "the request is the entire page" is not. The fix is that the long contract never needed to be in the queue. SKILL.md and clients/body.md already carry the probe taxonomy, the schema and the anchoring rules to every agent out of band, so the comment only needs the essentials: do not edit, do not suggest, answer with `prereview quiz`, 3-5 questions grounded in the diff, at least one `decision`. 40 lines to 10. A second-order property makes the short body safe: if an agent still gets the schema wrong, ValidateQuiz rejects the payload naming the offending question, so the validator teaches the contract on failure and the queue doesn't have to. Guarded by a test that caps the built-in body's length and asserts the three instructions that must survive any future shortening (`prereview quiz`, the NOT-`prereview suggest` rule, and `decision`) — without those a quiz request reads like a suggestions prompt and the agent proposes edits instead. Also worth recording, though it needed no code: the reviewer could not see the quiz at all, and that was a DEMO defect, not a product one. The demo repo had the 26 MB prereview binary built into the reviewed tree; it sorted ahead of the real file, so the session opened on "file too large to review" and — since the quiz entry is correctly scoped to the selected file — showed no quiz. Diagnosed by pointing a phone-sized headless browser at the live server rather than a synthetic fixture, which is the check that should have run after the first report instead of a reassurance. 153 browser tests pass (0 skips); unit suite and both golden guards green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
Reported from real use on a phone: "it's difficult to recall from the code what
we are talking about — I need to switch to the code, read it once more, and then
come back to the quiz."
This overturns a call I made in the Phase 2 audit and wrote down as deliberate:
"the jump deliberately ejects the reviewer from the quiz — that may be right for
v1, go read the code and toggle back." It is not right, for a specific reason: a
comprehension quiz you cannot see the code for stops testing comprehension and
starts testing short-term memory. The round trip also costs you the question,
since by the time you are back you have to re-read that too — two context
switches per question, on a phone.
So bring the code to the question instead of sending the reader to the code.
Each question now renders its cited lines inline: real syntax highlighting
(reusing DiffLine.HighlightedContent), the cited lines solid, two lines of
context either side dimmed for orientation, capped at 14 lines so a large range
cannot push the options off screen, and horizontally scrollable rather than
wrapped — a wrapped line misrepresents the code being asked about.
The jump link stays: the excerpt handles recall, the jump handles depth.
Two behaviours fall out of the anchoring data the schema already required, and
both reinforce the existing markers rather than contradicting them:
- an UNGROUNDED question gets no excerpt, because its cited range does not
resolve — there is nothing to show, which is the point of the warning;
- an anchorless `decision` question gets none either, because it is about
something that is not in the diff at all.
Both are asserted in the browser test, alongside a check that cited lines stay
visually distinct from context lines — an excerpt that shows code without
showing which part is being asked about would defeat the purpose.
153 browser tests pass (0 skips); unit suite and both golden guards green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
… screen (#191) Reviewer's proposal, and it is the right one: "wouldn't it be better to have quiz questions behave like comments and suggestions as an annotation to a line or range of lines? this way everything remains consistent." It is, and the previous commit was evidence for it. That one added an inline code EXCERPT to each question because the quiz replaced the diff and you could not see what you were being asked about. But "show the code next to the annotation" is something prereview already does natively by anchoring annotations to lines — the excerpt was a worse, read-only re-implementation of the diff view, nested inside a screen that should not have existed. It is deleted here. Questions now render exactly where comments and suggestions do: - line-anchored ones inline in the diff row they ask about (QuizByEndLine, the same shape as CommentsByEndLine / SuggestionsByEndLine); - kind=file ones at the file head, the slot a kind=file COMMENT occupies. The schema gains the comment anchor vocabulary — line / text / file / area / region. Only line and file render for now, but storing the full vocabulary means adding image-region or live-page questions later is pure rendering work rather than a migration of quizzes already on disk. The old "from_line": 0 sentinel for "about something absent" is gone: that is kind=file, stated in the same words a whole-file comment uses instead of a parallel invention. Two bugs surfaced by the rework, both worth recording: 1. UNGROUNDED QUESTIONS WOULD HAVE VANISHED. A question citing a line the diff does not contain has no row to render in, so it simply would not appear — HIDING a hallucinated question instead of surfacing it, which is precisely what the grounding check exists to prevent. They now render at the file head with their warning, alongside the kind=file ones. 2. A LATENT ORDERING BUG FROM PHASE 2. Mount loaded the quizzes ~100 lines before it loaded CurrentDiff, so grounding compared every cited line against a nil diff and condemned the entire quiz. It stayed invisible while the quiz was a separate screen, because ToggleQuiz re-grounded on open by which point the diff was in hand — the extra screen was accidentally masking a broken Mount path. Rendering inline removes that second chance and it failed immediately, with every question flagged "not in this diff". Grounding now runs beside the other re-anchor calls, after the diff is loaded. The quiz screen survives as the OVERVIEW — score, retake, and the questions in one list — exactly as inline comments coexist with the all-comments view. The browser test asserts the inline surface first and answers a question in place; its two placement assertions double as the regression guard for (2), since a nil-diff grounding sends every card to the file head. 153 browser tests pass (0 skips); unit suite and both golden guards green. A new quiz-inline render fixture pins the annotation rendering, which the overview fixture could not reach. Still to come: threads on a question, so the reviewer can ask the agent to clarify or revise one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
…191) Second half of the reviewer's proposal: "we should also be able to reply to a quiz question to have a conversation with the LLM to clarify or update things". A quiz question is a claim the agent made about your code, and sometimes the claim is wrong — an ambiguous option, a mis-read of the diff, a question whose "correct" answer isn't. Until now the only response was to get it wrong and move on. Now each question carries a thread: the reviewer pushes back in place, the agent answers or revises. This needed almost no new machinery, which is itself an argument for modelling questions as annotations. ThreadEntry.TargetID is a bare string and PostReply/OpenReply never cared what KIND of thing an id names, so the whole reviewer side worked unchanged the moment questions had ids to point at. The one genuinely new piece is IDENTITY. A question id ("q1") is unique only within its quiz, while a thread target is global — so the target is the composite "<quizID>:<questionID>". Colon-separated rather than NUL-joined like the internal answer key, because unlike that key this one is typed by a human or an agent on a command line. `prereview reply` validates it against the loaded quizzes and fails loudly on an unknown question, matching how it already treats comment and suggestion ids. The skill now also says what to do when the reviewer is RIGHT: re-submit the quiz with the same id (last write wins) with that question corrected, then reply saying what changed. "This question is wrong" should be actionable, not merely acknowledged. The browser test drives the full round trip — reviewer replies in place, agent answers out of process via the CLI, the answer appears live — plus two properties worth pinning: a reply attaches to its OWN question rather than leaking onto the next card, and an unknown question id is rejected instead of recording a dangling thread. One note on that test: it first waited on a `.thread` element appearing, which passed instantly because the reviewer's own reply had already created one — it proved nothing about the agent's message arriving. It now polls for the content. Same vacuous-green shape as a -run pattern that matches no tests. 154 browser tests pass (0 skips); unit suite and both golden guards green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
…tion (#191) Two asks from real use, both consequences of moving questions into the diff. NAVIGATOR. Anchoring questions to lines gave back the code as context but cost the "take the quiz" shape: there was no way from question 2 to question 3 except scrolling and hoping. A sticky strip now carries one small badge per question — tap to jump, colour says unanswered / right / wrong — plus a link to the overview and an × to put it away. It appears on its own whenever the file has a quiz, rather than behind a menu. Not being able to FIND things has been this feature's repeated failure, so the navigator itself should not need discovering. The scroll target is the card's HEAD, not the card. The client centres whatever it scrolls to, and a question with four options is taller than a phone screen, so centring the card put the question text off the top and landed the reader on the options. COLLAPSE. Questions now count toward the row's annotation badge and fold away with the comments and suggestions on that line — same badge, same toggle, no new mechanism. A row carrying only a question still gets a badge, so there is always a way to fold it and a marker that the line holds something. Answered questions stay visible rather than auto-collapsing the way a resolved comment does: the explanation is the payload, and hiding it the instant you answer takes away the thing you just earned. Two defects worth recording, both mine, both about how I checked: 1. A STRAY LIST MARKER over the badge numbers — the reviewer saw "a square dot which hides the quiz numbers". Same bug as the answer options, in a list I added afterwards without applying the fix I had already written a comment about. Every quiz list now lives in ONE css rule so a new one cannot skip it. The second-order failure matters more: a screenshot HAD shown it, and I explained it away as a downscaling artifact after checking textContent, which returned "1".."5". A ::marker is not text, so that check could only ever confirm the text existed — it could not distinguish "renders fine" from "renders with a square on top". The guard is now computed list-style-type, which is paint-adjacent and machine-checkable, and I verified it is discriminating by reverting the css: it fails naming all five offending elements, then passes once restored. 2. TWO COMPETING FRACTIONS. The strip read "1/5 answered" beside a "0/5" chip — progress and score, side by side, looking like they should agree. One phrase now ("1/5 answered · 0 right"), and the chip says what it does: "Overview". 157 browser tests pass (0 skips); unit suite and both golden guards green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
…croll (#191) "selection highlight should move with the scroll when a question is in view." The strip could jump you somewhere but never said where you were: tapping a badge scrolled the card and left no marker, and scrolling past a question lit nothing at all. It showed click history, not position. TWO FIELDS, NOT ONE. Jumping to a question and marking where you are look like one feature but have opposite lifetimes: the scroll nudge must fire once and stop, or every re-render yanks the page, while the highlight has to survive every render or the strip forgets. ScrollToQuizID stays transient; SelectedQuizID persists. The badge ring is an outline rather than a colour change, because colour already carries unanswered / right / wrong and overloading it would destroy that. FOLLOWING THE SCROLL WITHOUT TOUCHING READ PROGRESS. The viewport reporter that drives read progress already fires on every scroll with the visible key range, so the quiz DERIVES the current question from it. Nothing new is reported to the server and ReadThrough keeps its exact meaning — it is a high-water mark (furthest ever read) while a spy needs the current bottom, so that value is recorded in a new field beside it rather than by redefining the existing one. Changing what read progress means to serve the quiz would have silently moved the read rail. PRECEDENCE, which the full sweep caught and the targeted run did not. Adding the spy broke tapping: viewport position overrode explicit selection, so on a short file — where every question is on screen at once — tapping badge 3 highlighted badge 1. Two inputs now feed one piece of state and I had not decided how they compose. The rule is: an explicit tap wins while its question is still reachable, then scrolling hands control back to position. The first attempt at that rule was still wrong: it checked the explicit selection only against line-ordered questions, which excludes file-level and ungrounded ones. Those carry no line, so the viewport can neither confirm nor contradict them, and tapping their badge could never have highlighted anything. They are honoured whenever explicitly selected. On the test. The first scroll assertion failed, and it would have been easy to read that as a broken feature. It was not: the standard fixture's edited.go is five lines, so every question is visible at once and scrolling cannot change what is on screen — the test was structurally incapable of passing. It now runs against setupLongFileRepo, the 150-line fixture the read-progress suite already had, reused rather than duplicated. Verified discriminating: with the spy disabled it reports "none" for both scroll positions. 158 browser tests pass (0 skips); unit suite and both golden guards green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
Two reports from real use, and the second exposed a modelling error rather than a bug in either patch. CANNOT COLLAPSE A QUESTION. Collapse was built on the row badge, which folds a whole line's annotations together — consistent with comments and suggestions, and it works. But a question with no line renders at the FILE HEAD, outside any row, so it had no control at all. That is the card most likely to be in the way, since it sits at the top of the file. Every card now carries its own fold, which composes with the row badge. A folded card keeps its probe and prompt so it still says what it is about rather than becoming an anonymous stub. "SCROLL FOLLOWING DOESN'T WORK". It did — until you tapped a badge. The previous commit honoured an explicit selection unconditionally when the question had no line, reasoning the viewport "could not contradict it". That is a permanent pin: tap the file-level badge once and the highlight never moved again, however far the reviewer scrolled, which is indistinguishable from scroll-tracking being broken. My test only ever tapped a line-anchored badge, so it never entered that branch. The two fixes for this were in DIRECT CONFLICT, and I shipped each in turn: "tapping badge 3 highlights badge 3" (badge 3 is the ungrounded question) "tapping the file-level badge must not pin the highlight" Both concern questions with no line. Honour them unconditionally and the highlight sticks; ignore them and tapping their badge does nothing. Each patch traded one bug for the other. The error was in the framing: I had modelled those questions as POSITIONLESS. They are not — they render at the file head, which is a position. Once "in view" means "the viewport is at the top of the file", one uniform rule satisfies both: a tap wins while its question is on screen, and scrolling hands control back to whatever is on screen. The special case disappears, and with it the precedence tuning between two rules that only ever needed to be one. New tests cover the branches the earlier ones missed: tapping an UNANCHORED badge and then scrolling, and folding a card that belongs to no row. 160 browser tests pass (0 skips); unit suite and both golden guards green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
#191) Reviewer: "why did we build a new collapse solution while we already have the comment and suggestion way of collapsing and expanding?" Because I mis-scoped the problem. The report was "not able to collapse a quiz question", and I heard "quiz questions need collapsing" — so I built a quiz-only fold control with its own state, handler, markup and css. That gave quiz cards TWO collapse mechanisms (the row badge and the new fold) while leaving the actual gap open. The gap was general. `toggleRow` appears zero times in page.tmpl: the file head has never had an annotation badge, so a file-level COMMENT could not be collapsed either, and neither could an image-area comment. Only diff rows and markdown blocks ever could. So the file head now gets a row key of its own ("file") and renders the SAME .line-marks badge a diff row renders. toggleRow, ToggledRows, RowToggled and the collapse css are all reused unchanged. Deleted: CollapsedQuiz state, the ToggleQuizCard handler, the fold button, its css, and its folded-render branch. Quiz cards need nothing of their own to collapse. This also fixes something that predates the feature: file-level and image-area comments are collapsible for the first time, with the badge counting and colouring across all three annotation kinds at the head. One bug caught by the sweep, and it is a good illustration of why the wide blast radius was worth taking seriously. I gated the whole file-head block on "FileHeadAnnotationCount > 0" — but that block also contains the AREA COMPOSER, and when you are starting an area comment there are no annotations yet. Count zero, composer never rendered, TestE2E_ImageAreaComment hung forever waiting for it. Gating a container on "does it have content" silently gated the control that CREATES the content. The wrapper now always renders; only the badge is conditional. The test asserts the shared mechanism AND that no quiz-only fold control exists, so the parallel path cannot be quietly reintroduced. All 160 browser tests pass across four shards (40 each, 0 skipped); unit suite and both golden guards green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
Reported: "the badge is on the left instead of on the right. only 1 question has the badge." Both trace to how I attached the file-head badge. I reused the diff row's .line-marks CLASS but not its PLACEMENT. A diff row pins its badge into an absolutely-positioned right-gutter container (.line-margin, position:absolute top:0 right); I had made the file-head badge a plain flex child, so it landed on the LEFT of the cards. It now sits in the same .line-margin container, top-right, matching every other annotation badge. "only 1 question has the badge" is the same root cause seen from the other side. The line-anchored questions DO each have a badge — it lives in the gutter at their code line, shared per line exactly like a comment's or suggestion's (two questions on one line share one badge, so 4 line questions on 3 lines show 3 badges). But the badge pins to the top of the row, at the code, while a quiz card — four options plus an explanation — extends far below it. So scrolled to a card, its gutter badge is off the top of the screen, and only the file-head badge, which sits on its own card, was visibly attached. Moving that one to the right removes the odd-one-out. Left deliberately unchanged: the gutter badge stays at the code line, consistent with comments and suggestions. Making it stick to the viewport as you scroll a tall card would be a real improvement, but it is a change to the SHARED badge mechanism that affects every annotation, not a quiz fix — not something to do speculatively on this report. The test now asserts the badge is in the .line-margin gutter and to the right of its cards, so the left-placement regression cannot return. All 160 browser tests pass across four shards; unit suite and both golden guards green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
Reported together: "collapsed badges are not visible. clicking breadcrumbs on top doesn't expand the collapsed question." Both are one flow, and the screenshot showed why. Collapse a question and its card hides behind a badge in the gutter. That badge is small, and for the FILE-HEAD question it is worse than small — it sits at the top of the article, directly under the sticky quiz-navigator strip (nav z-index 6, badge z-index 2), so it is painted but occluded. The reliable way back is the one the reviewer reached for: the breadcrumb, which lists every question and is always on screen. But JumpToQuestion only set the scroll + selection; it never cleared the collapse. So tapping a collapsed question's breadcrumb scrolled to a hidden card and nothing appeared to happen. It now expands the question's row first — a quiz card is hidden only while its row carries `row-toggled` (ToggledRows[key] matches the row's default), so dropping that entry restores the card. The row key is the file head for an unanchored question, "<line>-<side>" for a line one. One correctness detail: AnchorStatus is derived (json:"-") and can be zero on a fresh handler state, so the row-key logic re-grounds first — otherwise a hallucinated (ungrounded) question, which renders at the file head, would be mistaken for a line-anchored one and the wrong row cleared. This makes the navigator the dependable recovery for any collapsed question, which is the right primary path regardless of how findable the gutter badge is. The badge still works where it is visible; the breadcrumb is the answer when it is not. 161 browser tests pass across four shards; unit suite and both golden guards green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
Follow-on to the breadcrumb fix: surface the badge itself, not just give an alternative recovery path. The file-head badge was the diff row's absolute gutter badge (.line-margin, position:absolute top:0). A diff row aligns its badge to a code line, which has height; the file head has no line, so when the group collapsed (cards display:none) the wrapper went to zero height and the absolute badge pinned to the top of the article — directly under the sticky navigator (nav z-index 6 over the badge's z-index 2). Painted, but occluded: exactly "collapsed badges are not visible". It is now an IN-FLOW header row above the cards, right-aligned. In-flow means it keeps real height and real position regardless of whether the cards below it are shown, so the toggle stays visible and tappable when collapsed. Confirmed by screenshot: the badge sits just below the nav strip, top-right, above the diff. The placement stays "on the right" (the earlier fix's requirement) — a right-aligned row reads the same as a top-right gutter badge, without the zero-height collapse failure. The .line-margin absolute container and its content padding are gone. The collapse test now also asserts the badge remains laid out (non-zero box, offsetParent set) while the group is collapsed, so the occlusion regression cannot return. 161 browser tests pass across four shards; unit suite and both golden guards green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
#191) A cleanup pass over the quiz surface, which grew across ~15 commits of UI feedback with helpers rewritten in place. All behavior-preserving — goldens byte-identical, 161 e2e green. - Question.LineGrounded() replaces the "is this a head vs a line question" predicate (LineAnchored() && !Ungrounded()) that was spelled out at five sites. Deliberately NOT folded into the existing Jumpable() (== AnchorStatus "ok"), which diverges when AnchorStatus is unset — expandQuestionRow re-grounds precisely because that state is reachable. - Question.EndLine() replaces the ToLine-vs-FromLine clamp duplicated in QuizByEndLine and expandQuestionRow. - QuizCountLines / QuizOpenLines now use the shared row-key helpers that CommentCountLines / CommentOpenLines use, matching what their comments already claimed. Safe because quiz Side is always defaulted on both load paths. - FileQuizItems is now the exact complement of the inline set (!LineGrounded()) instead of a separately-spelled kind=file-or-ungrounded predicate. Identical for v1 (the only non-line-grounded kinds emitted are file and ungrounded), but it closes a latent gap the simplifier flagged: a future area/region question matched neither the inline nor the head predicate and would have rendered nowhere. Stating head as "not inline" makes that impossible by construction. 161 browser tests pass across four shards; unit suite and both golden guards green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
Reported: "on scroll breadcrumbs highlight doesn't change." Diagnosed as a one-render lag — the highlight was computed server-side from the read-progress viewport reporter, so every scroll was a debounce + WebSocket round-trip + re-render. Invisible on localhost, laggy on a phone over the tailnet; it settled ~300ms after you stopped but trailed during a continuous scroll. The fix is the client-side primitive the reviewer asked for, and it turned out to already exist and already be vendored: livetemplate's lvt-spy scroll-spy (the same one the TOC uses). Each navigator badge is now <a href="#qc-<id>" lvt-spy-link>, and lvt-spy toggles lvt-active on whichever badge's question card is on screen — a rAF-throttled client scroll listener, zero server round-trip, so it tracks without lag. Two things fell out, both replacing server round-trips with platform features: - SCROLL to a question is now the native anchor (href="#qc-<id>"), instant, with scroll-margin-top reserving the sticky nav's height. - EXPAND a collapsed question on breadcrumb tap is now CSS :target — the hash the anchor sets force-shows the addressed card even inside a collapsed row. No JumpToQuestion handler, no expandQuestionRow. This DELETES the entire server-side scroll-spy: currentQuizID and its tap-vs-scroll precedence, viewportLines, headQuestions, firstDiffLine, SelectedQuizID, ScrollToQuizID, the LastViewBottomKey field and its ReportViewport write, JumpToQuestion, expandQuestionRow, and the QuizItem.Current/ScrollTo fields. The read-progress reporter is left exactly as it was — this feature no longer piggybacks on it. All 160 browser tests pass across four shards (the obsolete tap-precedence-doesn't-stick test is removed — there is no precedence to test now, the spy simply reflects what's on screen); unit suite and both golden guards green. The nav e2e now assert lvt-active (client) and the :target reveal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
#191) Reported: "Are the questions not sequential from top to bottom? I'm at the bottom and the highlighted question number is three." The badges were numbered by the agent's JSON order (Num = QuizItems index), which has no relation to where the questions sit in the diff. So at the bottom of the page, the last question by position could be badge 3 — the badges did not read 1..N top to bottom. QuizItems now orders itself by DOCUMENT position — head questions (which render above the diff) first, then line-anchored ones by line — and numbers 1..N in that order. The navigator strip, the overview list, and the file-head group all follow it. So badge N is the bottom question, and the strip reads left-to-right the way the page reads top-to-bottom. Two follow-ons needed to make "at the bottom, badge N is active" actually hold: - The spy trigger line moved from 7rem to 40vh, so the active question is the one you are READING (mid-screen), not one that has scrolled nearly out of view. - "Scroll beyond last line" padding (the pattern VS Code and docs sites use) below a quizzed diff, so the LAST question can scroll up to the 40vh trigger and light its badge — without it the last card tops out mid-screen at max scroll and its badge never activates. Scoped to `main.viewer > article[lvt-spy]` (higher specificity than the `main.viewer > article { padding: 0 }` reset it has to beat). New e2e asserts badge numbers increase with the card's vertical position and that the bottom of the page activates the last badge. 161 browser tests pass across four shards; unit suite and both golden guards green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp
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.
Implements #191 — a button that asks the agent to quiz you on the diff you're about to accept, rendered in a new quiz view.
Why
An
acceptverdict in prereview records that the reviewer clicked, not that they understood.CodeReviewQA (Findings of ACL 2025) is the motivating result: models that produced a correct refinement still failed the intermediate comprehension probes for 49.0–99.6% of those same successes. Producing the right output is not evidence of understanding it — exactly the gap between "the agent's diff passes" and "the human reviewing it understood it." Three of its reasoning probes became three of the five probe kinds here.
Answering is retrieval practice, which is also where the "every question ships an explanation, revealed after you answer" requirement comes from.
What makes it not slop
A generic "make me a quiz" button produces generic questions. The difference: prereview owns the diff, so it can verify every cited line range exists. Three states, and the last two must not collapse:
okungroundedabsentdecisionabout an omission, which has nothing to point atCollapsing
ungroundedintoabsentwould let a hallucinated anchor hide behind a legitimate label. Distinct in Go, template, CSS, a render golden, and a browser test.The
decisionprobeFour probes ask whether you understood what is in the diff. The fifth asks what the agent decided that you never asked for — an unrequested dependency, a changed default, a widened interface, a skipped edge case. It's diff-versus-intent rather than diff-internal, so only the agent that did the work can author it; a reviewer skimming can't see that gap precisely because the code looks fine.
Its honest limitation is recorded in the plan and the prompt: it's self-reported by the party being reported on, so under-reporting is the expected failure. It raises the floor; it is not a safety net.
Architecture — existing patterns, cloned
kind=filecomment carrying the prompt. No new comment kind, no second channel. Visible in the reviewer's queue —Comment.Hiddenonly applies to resolved comments, so hiding it isn't possible and isn't worth inventing.quiz.jsonlagent-owned and append-only,quiz-answers.jsonlserver-owned and atomically rewritten. Two files, one writer each — thethread.gorule.~/.config/prereview/quizzes/*.md,PREREVIEW_QUIZZES_DIR), reusingLoadPrompts' loader. Safe only because the prompt is guidance and never the guarantee:ValidateQuiz(structure) andapplyQuiz(grounding) bind whatever prompt produced the quiz.quizzes(file, score, total,taken). Never gates.takenis separate from the score because "never opened it" and "answered everything wrong" both serialize as 0/N and mean opposite things. Nil ⇒omitempty⇒ wire format unchanged for reviews that don't use the feature.Verification
Full unit suite, both golden guards, and all 152 browser e2e green (440s, 0 skips).
Four defects the layer below couldn't see:
--file -didn't read stdin, thoughdone/reply/appliedall document it — only the e2e drove the verb the way an agent does..prompts-trigger, which silently re-pointedTestE2E_PromptPicker's click at the wrong button — it didn't fail, it hung, taking the suite past its timeout. Fixed with its own class + grouped styles, plus a guard asserting that selector matches exactly one element so a recurrence fails in seconds.Deferred
Staleness detection. It needed a fingerprint of the quizzed content, but the server can't compute one retroactively and the agent can't compute prereview's hash — it would mean asking the agent to shell out a
sha256and trusting it hashed the same bytes. Grounding already covers the visible symptom: edit the file and the cited lines stop resolving. Reasoning recorded rather than the checkbox silently dropped.Also out of scope, per the plan: folder-wide quizzes and free-recall questions (M2).
🤖 Generated with Claude Code
https://claude.ai/code/session_01NHkKEgLvjwvK9EEprdtAjp