Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/rfc/workon-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ The remaining roadmap is resequenced around the tool being **the author's own ev

- **Conflict resolution** *(stretch)*. Resolve merge/rebase conflicts in the SBS view. Large surface; may not make v1.

- **Agent loop** *(the eventual north star)*. **Design locked 2026-09-02 (ADR-039); the three open forks below are resolved, not just re-flagged.** One substrate serves both review comments and an integrated `/explain-diff`-style walkthrough: `AnnotationKind::{Comment, TourStop, Chapter}` in one sqlite table at `<commondir>/workon-review/annotations.db`, anchored by content-hash context (target line + 3 lines each way, re-resolved per load: exact match, then a scored windowed scan, then whitespace-tolerant, else `Orphaned` — never silently wrong) rather than the originally-proposed `(changeset_id, path, side, lnum)` key. **Comment-store home:** its own crate, `git-workon-annotations` (`publish = false`, serde-free, no git2) — `git workon mcp` is the second consumer the "no separate core crate" rule was waiting on, so that condition is now met and the rule no longer applies to it. **MCP crate/transport:** `rmcp` 3.2 (minimal features, current-thread tokio), not hand-rolled JSON-RPC, served from its own `git-workon-mcp` crate (ADR-040) and reached from `git-workon` via the existing PATH-dispatch mechanism (not a built-in `Cmd::Mcp`, and not a `git-workon-review mcp` subcommand) — this is what lets the published `git-workon` binary depend on the feature without depending on the unpublished annotations crate. Landing as five stacked slices (crate scaffold → TUI read → TUI authoring → prose/walkthrough polish → MCP crate); each lands alone. Still deferred behind the daily-driver work, but no longer blocked on open design questions.
- **Agent loop** *(the eventual north star)*. **Design locked 2026-09-02 (ADR-039); the three open forks below are resolved, not just re-flagged.** One substrate serves both review comments and an integrated `/explain-diff`-style walkthrough: `AnnotationKind::{Comment, TourStop, Chapter}` in one sqlite table at `<commondir>/workon-review/annotations.db`, anchored by content-hash context (target line + 3 lines each way, re-resolved per load: exact match, then a scored windowed scan, then whitespace-tolerant, else `Orphaned` — never silently wrong) rather than the originally-proposed `(changeset_id, path, side, lnum)` key. **Comment-store home:** its own crate, `git-workon-annotations` (`publish = false`, serde-free, no git2) — `git workon mcp` is the second consumer the "no separate core crate" rule was waiting on, so that condition is now met and the rule no longer applies to it. **MCP crate/transport:** `rmcp` 3.2 (minimal features, current-thread tokio), not hand-rolled JSON-RPC, served from its own `git-workon-mcp` crate (ADR-040) and reached from `git-workon` via the existing PATH-dispatch mechanism (not a built-in `Cmd::Mcp`, and not a `git-workon-review mcp` subcommand) — this is what lets the published `git-workon` binary depend on the feature without depending on the unpublished annotations crate. Landing as five stacked slices (crate scaffold → TUI read → TUI authoring → prose/walkthrough polish → MCP crate); each lands alone. — DONE (2026-09-02): four of five shipped — `annot-crate` (the store + resolver crate), `annot-read` (gutter markers, view/reply overlay, tour stepping), `annot-write` (the multi-line annotation editor, create/reply/resolve), `annot-prose` (walkthrough chapters wrapped into the summary panel, a `stop i/n` tour-progress indicator in the diff header, and `--tour <name>` to open straight into a walkthrough). `mcp-crate` lands next.

## Orchestration notes

Expand Down
46 changes: 39 additions & 7 deletions git-workon-review/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,14 @@ impl ChangesetIdentity {
}
}

/// This session's [`ChangesetKey`] for an ARBITRARY changeset — the shared core
/// [`App::current_changeset_key`] (the active one) and [`App::summary_for`] (an outline
/// selection, which need not be the active changeset) both key their ADR-039 lookups off. See
/// [`ChangesetIdentity`]'s doc comment for why name alone is ambiguous.
fn changeset_key_for(cs: &Changeset) -> ChangesetKey {
ChangesetKey::new(cs.name.clone(), cs.span == ChangesetSpan::Uncommitted)
}

/// The outline side pane's own state (locked fork 3): whether it's showing, whether IT (rather
/// than the diff) currently has keyboard focus, its own cursor (an index into
/// [`App::outline_items`]'s row list — a wholly separate coordinate space from [`App::cursor`]),
Expand Down Expand Up @@ -1921,8 +1929,7 @@ impl App {
/// `ChangesetIdentity` itself, since that type's fields are private to this module's own
/// nav-identity use and carry no annotations-crate conversion.
fn current_changeset_key(&self) -> ChangesetKey {
let cs = &self.cur().cs;
ChangesetKey::new(cs.name.clone(), cs.span == ChangesetSpan::Uncommitted)
changeset_key_for(&self.cur().cs)
}

/// The active file `idx`'s `role` view's annotation markers, one [`MarkerKind`] per
Expand Down Expand Up @@ -2338,11 +2345,13 @@ impl App {
}
}

/// Set the active walkthrough by name and reload its stops (`main.rs`'s future `--tour`
/// flag, and tests). Nothing infers a tour automatically today — the store has no "list
/// tours" query (a tour's identity is just whatever name its stops share), so a tour must
/// be named explicitly by the caller, matching how [`Self::set_review_source`] is a setter
/// rather than a constructor parameter.
/// Set the active walkthrough by name and reload its stops (`main.rs`'s `--tour` flag, and
/// tests). Nothing infers a tour automatically today — the store has no "list tours" query
/// (a tour's identity is just whatever name its stops share), so a tour must be named
/// explicitly by the caller, matching how [`Self::set_review_source`] is a setter rather
/// than a constructor parameter. Fail-soft on an unknown name: [`Self::reload_tour_stops`]
/// yields an empty stop list, so [`Self::tour_next`]/[`Self::tour_prev`] fall straight into
/// their existing "no active tour" notice rather than erroring.
pub fn set_tour(&mut self, tour: impl Into<String>) {
self.tour_name = Some(tour.into());
self.tour_idx = None;
Expand All @@ -2356,6 +2365,20 @@ impl App {
};
}

/// The active tour's step position for the diff header's progress indicator
/// (`diff_header_line`'s "stop i/n") — `Some((1-based index, total))` once
/// [`Self::tour_next`]/[`Self::tour_prev`] has parked on a stop, `None` before the first
/// step or when no tour is active/has stops. Not derived from [`Self::tour_name`] alone
/// (a tour can be SET but not yet STEPPED into), matching the "stop" wording — showing
/// "stop -/n" for a not-yet-entered tour would claim a position that doesn't exist yet.
pub fn tour_progress(&self) -> Option<(usize, usize)> {
let idx = self.tour_idx?;
if self.tour_stops.is_empty() {
return None;
}
Some((idx + 1, self.tour_stops.len()))
}

/// `]t`: step to the next stop of the active tour (see [`Self::set_tour`]). Clamps at the
/// last stop — does not wrap. A no-op with a footer notice when no tour is active or it has
/// no stops.
Expand Down Expand Up @@ -3676,13 +3699,22 @@ impl App {
let view = &self.changesets[cs_idx];
let label = display_label(&view.cs);
let failure_message = view.failure_message().map(|s| s.to_string());
// ADR-039's per-changeset walkthrough prose: `None` on a store-open failure or
// when no `Chapter` annotation has been authored for this changeset — degrades
// silently, matching every other `annotations`-optional read in this module.
let chapter = self
.annotations
.as_ref()
.and_then(|store| store.chapter(&changeset_key_for(&view.cs)).ok().flatten())
.map(|annotation| annotation.body);
Summary::Changeset(summary::changeset_summary(
label,
view.cs.current,
view.cs.needs_restack,
view.is_pending(),
view.is_failed(),
failure_message,
chapter,
view.files(),
))
}
Expand Down
28 changes: 28 additions & 0 deletions git-workon-review/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ struct Cli {
/// a PR reference
#[arg(value_name = "SOURCE", add = ArgValueCompleter::new(complete_source))]
source: Option<String>,

/// Open into an ADR-039 walkthrough by name — `]t` then steps to its first stop. Fail-soft
/// on an unknown name: `App::set_tour` degrades to an empty tour, which reads exactly like
/// no `--tour` flag at all plus the usual "no active tour" notice on `]t` — there's no
/// "list tours" query yet for this flag to validate against up front.
#[arg(long, value_name = "NAME")]
tour: Option<String>,
}

fn main() -> Result<()> {
Expand Down Expand Up @@ -222,6 +229,7 @@ fn main() -> Result<()> {
repo,
views,
source,
cli.tour.as_deref(),
&view_config,
&keymap,
&theme_override_warnings,
Expand All @@ -248,6 +256,7 @@ fn main() -> Result<()> {
repo,
views,
source,
cli.tour.as_deref(),
&view_config,
&keymap,
&theme_override_warnings,
Expand All @@ -272,6 +281,7 @@ fn seat_app(
repo: Repository,
views: Vec<ChangesetView>,
source: Option<Source>,
tour: Option<&str>,
view_config: &config::RawViewConfig,
keymap: &Keymap,
theme_override_warnings: &[String],
Expand All @@ -280,6 +290,12 @@ fn seat_app(
if let Some(source) = source {
app.set_review_source(source);
}
// `--tour`: fail-soft per `Cli::tour`'s doc comment — an unknown name just yields an empty
// stop list, so the reviewer's first `]t` gets the existing "no active tour" notice rather
// than a startup error.
if let Some(tour) = tour {
app.set_tour(tour.to_string());
}
// Idle-deferred file loads: defer file loads to the event loop's input-idle window rather than
// blocking here (or on any later selection change) — `app.open_current()` below marks the
// initial open pending
Expand Down Expand Up @@ -325,6 +341,18 @@ fn surface_warnings(app: &mut App, keymap: &Keymap, extra_warnings: Vec<String>)
mod tests {
use super::*;

#[test]
fn tour_flag_parses_into_cli() {
let cli = Cli::try_parse_from(["git-workon-review", "--tour", "explain-stack"]).unwrap();
assert_eq!(cli.tour.as_deref(), Some("explain-stack"));
}

#[test]
fn tour_flag_defaults_to_none() {
let cli = Cli::try_parse_from(["git-workon-review"]).unwrap();
assert_eq!(cli.tour, None);
}

#[test]
fn no_color_truth_table() {
assert!(!no_color(None), "unset must not trigger mono");
Expand Down
Loading