Skip to content

Confirm whether a quit with no annotations means finished or stopped #333

Description

@chris-peterson

Context

revdiff is often driven by an AI agent: the agent generates a change, launches revdiff for a human to read, then decides what to do next from the exit code. The bundled launcher sets REVDIFF_EXIT_CODE_ON_ANNOTATIONS, so every reviewer-driven exit that is not a crash is one of two values:

Reviewer action Exit code What the agent concludes
q with annotations 10 feedback to address
q with no annotations 0 nothing to address
Q (discard and quit), even with annotations 0 nothing to address

.claude-plugin/skills/revdiff/SKILL.md states the reading: "Done when user quits without annotations" (line 73).

The case I hit: I was reading an agent-generated diff, spotted something wrong, and decided to go fix it myself rather than write it up. I quit. The agent took the clean exit as a finished review and committed.

A quit with no annotations is ambiguous. "I read this and it is fine" and "I am stopping here to go fix it myself" are the same keystroke, and nothing tells the reviewer that one of them will be reported as a completed review. An exit with annotations carries a clear signal, so the gap is only the no-annotations case.

Proposed approach

When the reviewer quits with no annotations, ask before exiting:

┌────────────────┐┌───────────────────────────────────────────────────────┐
│ app/           ││ app/main.go                                           │
│   main.go      ││  326       // the agent may read this concurrently    │
│   config.go    ││  327 -     err := fsutil.AtomicWriteFile(p, out)      │
│ fsutil/        ││  328 +     err := os.WriteFile(p, out, 0o644)         │
│ README.md      ││  329       if err != nil {                            │
│                ││  330           return fmt.Errorf("write: %w", err)    │
│                ││  331       }                                          │
└────────────────┘└───────────────────────────────────────────────────────┘
 finish the review with no annotations? [y/n]

y exits 0, as today. n exits with a code distinct from 0 and 10 (11 sits next to the existing one), reporting that the review was stopped rather than finished. esc returns to the review, as it does in the discard prompt.

revdiff already has this interaction: Q with a non-empty store renders discard %d annotations? [y/n] in the status bar (app/ui/view.go:171) off an inConfirmDiscard flag, dispatched ahead of normal key handling (app/ui/model.go:973) and handled by handleConfirmDiscardKey (app/ui/handlers.go:202). This would be a sibling on the other quit path.

Opt-in semantics. A bool flag --confirm-no-annotations, env REVDIFF_CONFIRM_NO_ANNOTATIONS, config key confirm-no-annotations, off by default, set by the bundled launchers alongside --exit-code-on-annotations. The two stay independent: that one governs annotations reporting 10, this one governs the no-annotations prompt and its stopped code.

The alternative is doing this agent-side, with SKILL.md telling agents to ask whenever revdiff exits 0 with no annotations. The difference is what the outcome rests on. Resolving it in the TUI turns the reviewer's answer into an exit code: a strong signal, immediately, identical for every consumer. Agent-side it stays an instruction in a prompt, so whether the reviewer is asked at all depends on the model honoring it on that turn.

A smaller alternative

Q already means discard and quit, and already renders its own confirm when the store is non-empty. Making its exit code configurable gives an informed reviewer the same stop signal for about five lines.

finalize collapses two cases into one branch today (app/main.go:311):

if r.discarded || r.annotations == "" {
    return 0, nil
}

Split it so the discarded branch returns a configured code, add --discard-exit-code (int, default 0, the shape --tab-width and --compact-context already use), and Q becomes the stop signal with no new key, no new prompt, and nothing new to name.

What it does not do is close the gap that prompted this issue: q with no annotations still exits 0, so the signal reaches only a reviewer who already knows to press Q. It also ties stopping to discarding, since Q skips the history save as well (finalize returns before saveHistory; README.md:721). That is harmless on an empty store and lossy on a non-empty one, though exit 10 already covers the feedback-and-stop case.

The two compose. The flag can land on its own, and the prompt can follow if silent quits keep reading as finished reviews.

Acceptance criteria (for the prompt)

  • With --confirm-no-annotations, quitting with no annotations prompts rather than exiting.
  • y exits 0. n exits with a code distinct from 0 and 10. esc returns to the review with no state lost.
  • Without it, quit behavior is unchanged.
  • launch-revdiff.sh treats the stopped code as a reviewer outcome, not a launcher failure. Its check allows only 0 and 10 today (.claude-plugin/skills/revdiff/scripts/launch-revdiff.sh:65), documented in the header at line 5.
  • SKILL.md tells agents the stopped code means do not proceed, and is not a failure to report or retry.

Considerations

  • Naming is deliberate. "Finished" and "stopped" describe what revdiff can observe; it never learns whether a change is good, only whether the reviewer left feedback and how they left. "Approve" and "reject" would assert more than the tool knows, and reject already means input validation across the codebase (app/compare_test.go:45, app/annotations_load.go:80, app/reviewinfo.go:70). "No annotations" is the phrase already used for this state in README.md:349, app/ui/output.go:34, and app/ui/overlay/annotlist.go:68.
  • The prompt covers the interactive case only. It has nowhere to render under --no-status-bar, which is why handleDiscardQuit bails out of its own confirm there (app/ui/handlers.go:142), and a signal-driven exit bypasses the TUI and returns 0 through finalize's signaled branch (app/main.go:315). Both should keep exiting 0.
  • Size, anchored on add Q hotkey to discard annotations and quit #4, which added the Q discard confirm this mirrors: 84 lines of production Go and 251 of tests. This should be similar or smaller, since it reuses that confirm pattern and adds no keymap action. Beyond Go it touches both launcher scripts, SKILL.md and its Codex and Pi copies, references/config.md, references/usage.md, README, and site/docs.html, on the order of 30 lines.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions