From 8f629934bb3d2a2d056a17ca9a00a7648a0bc23e Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Wed, 28 Oct 2026 15:19:43 -0400 Subject: [PATCH] feat(gate): explicit approve and a machine-readable decision - `plannotator-tui --gate` adds Approve (`A`, and an `Approve (A)` header button) beside Send and quit. The review ends with one decision: approved (unsent notes ride along as approval notes), annotated (a successful send) or dismissed (quit). Without `--gate` nothing changes. - output matches `plannotator annotate --gate`: plaintext `The user approved.` or the approved-with-notes message; `--json` prints `{"decision","feedback"}` plus `annotationCount` and `target`; `--require-approval` exits 0 only on approval; `--result-file` publishes the strict record atomically and never overwrites. Strict flags need `--gate --json`, and the gate's own failures exit 2. - `herdr open --gate [--result-file PATH]` delivers the approval message or the numbered feedback to the agent pane and publishes the result. - the feedback archive records approved, approved-with-notes and dismissed in the browser's shape. refs #121 --- README.md | 41 +++ crates/plannotator-tui/README.md | 1 + crates/plannotator-tui/src/app/approve.rs | 92 ++++++ .../plannotator-tui/src/app/approve/tests.rs | 172 +++++++++++ crates/plannotator-tui/src/app/draw.rs | 4 + crates/plannotator-tui/src/app/header.rs | 20 +- crates/plannotator-tui/src/app/input.rs | 5 + crates/plannotator-tui/src/app/mod.rs | 11 + crates/plannotator-tui/src/app/send.rs | 16 +- crates/plannotator-tui/src/archive.rs | 43 ++- crates/plannotator-tui/src/cli.rs | 147 +++++++++- crates/plannotator-tui/src/delivery.rs | 20 ++ crates/plannotator-tui/src/gate.rs | 267 ++++++++++++++++++ crates/plannotator-tui/src/gate/tests.rs | 183 ++++++++++++ crates/plannotator-tui/src/herdr/context.rs | 15 + crates/plannotator-tui/src/herdr/launch.rs | 15 + .../plannotator-tui/src/herdr/launch/tests.rs | 24 ++ crates/plannotator-tui/src/keys.rs | 18 +- crates/plannotator-tui/src/keys/tests.rs | 20 ++ crates/plannotator-tui/src/main.rs | 10 +- crates/plannotator-tui/tests/gate.rs | 90 ++++++ docs/decisions.md | 47 +++ docs/spec-herdr-integration.md | 13 +- skills/plannotator-tui/SKILL.md | 10 + 24 files changed, 1255 insertions(+), 29 deletions(-) create mode 100644 crates/plannotator-tui/src/app/approve.rs create mode 100644 crates/plannotator-tui/src/app/approve/tests.rs create mode 100644 crates/plannotator-tui/src/gate.rs create mode 100644 crates/plannotator-tui/src/gate/tests.rs create mode 100644 crates/plannotator-tui/tests/gate.rs diff --git a/README.md b/README.md index 8976e44..2f8280e 100644 --- a/README.md +++ b/README.md @@ -97,6 +97,7 @@ dimmed. The keys also work without opening the menu. |---|---| | anywhere | `Tab` cycle tree · document · notes; `E` send; `t` tree; `r` reload; `o` open the file in `$EDITOR`; `q` quit | | reply review | `S` send and close | +| gated review | `A` approve; `E` send and finish; `q` dismiss | | document | `j`/`k` block; `c` comment on the block; `x` clear its annotations; `X` clear every annotation on the document, after a `y`; `v` select with `hjkl` `w` `b` `0` `$`, `Enter` to finish; `i` move the cursor with those keys first, then `v` to select from there; `I`/`D` attach or remove an image on the block's note | | toolbar | `a` looks good · `c` comment · `d` delete · `Esc` | | notes | `j`/`k`; `e` edit; `x` remove; `I` attach an image, `D` remove the last; click a bubble | @@ -106,6 +107,39 @@ dimmed. The keys also work without opening the menu. These are the defaults; every key can be rebound under `[keys]` (see [Keys](#keys)). +### Approval gate + +When a script or an agent must know whether you signed off, add `--gate`. The review gains +an **Approve** action (`A`, and an `Approve (A)` button on the header's edge) beside Send and +quit, and ends with one decision: Approve is `approved`, a successful Send is `annotated`, and +quitting is `dismissed`. Notes you have not sent yet ride along with an approval +(`Approve with 2 notes (A)`). Without `--gate` nothing changes. + +```sh +plannotator-tui docs/plan.md --gate # prints the outcome when you finish +plannotator-tui docs/plan.md --gate --json --require-approval --result-file /tmp/plan-decision.json +``` + +The output is the one `plannotator annotate --gate` prints. Plain `--gate` prints `The user +approved.`, the approved-with-notes message (the notes plus the file they are about), the +feedback, or nothing for a dismissal. `--json` prints one line instead, +`{"decision":"approved|annotated|dismissed","feedback":"…"}`; `feedback` is absent from a +bare approval and a dismissal, and the line also carries `annotationCount` and the reviewed +`target`. `--json` without `--gate` decides between `annotated` and `dismissed` only. + +For a gate a script can trust, add either or both strict flags (they need `--gate --json`): + +- `--require-approval` exits 0 only on approval and 1 on `annotated` or `dismissed`. +- `--result-file ` also writes `{"decision":…,"feedback":…}` to ``, atomically, + and never over an existing file: the folder must exist and the file must not. A relative + path starts at the current directory. + +Under a strict flag a failure of the gate itself (a bad flag, a missing file, a result file +that cannot be written) exits 2, never 1. The review draws on stdout, so a gated review +refuses to start when stdout is not a terminal: read the decision from `--result-file` +rather than capturing stdout. Each decision is also added to the feedback archive as +`approved`, `approved-with-notes`, `feedback` or `dismissed`. + `o` hands the terminal to `$VISUAL` (or `$EDITOR`, which may carry flags such as `code -w`) on the open file, at the current line for vim, nvim, nano, helix, emacs, micro and kak. When the editor exits the viewer comes back and, if the file changed, reloads it as `r` does so @@ -139,6 +173,12 @@ if you install the add-on: `herdr plugin install plannotator/herdr-annotate/link placement = "overlay" # overlay (full tab, default) | split | popup ``` +`plannotator-tui herdr open plan.md --gate [--result-file ]` opens a gated review. Approve +sends the agent `The user approved.` (or the approved-with-notes message), Send sends the +numbered feedback as usual, and quitting sends nothing; the window closes on any of the three. +With `--result-file` the pane also writes the decision JSON there. The command returns as soon +as the pane opens, so a script waits for the file. + `plannotator-tui herdr last --newest` opens the agent's newest reply without asking which one; without the flag the picker comes first, as it always has. @@ -237,6 +277,7 @@ screen show your keys. `plannotator-tui config` prints every action with its key | `quit` `interrupt` | `q` `ctrl+c` | quit | | `cycle-focus` | `Tab` | tree · document · notes | | `send` `send-and-close` | `E` `S` | send; send and close a reply review | +| `approve` | `A` | approve a gated review (`--gate`) | | `toggle-tree` `reload` `pick-message` | `t` `r` `p` | show/hide the tree; reload; choose another reply | | `open-in-editor` | `o` | edit the open file in `$VISUAL` or `$EDITOR`, then reload it | | `quick-open` | `ctrl+p` | find a file by name in a folder review | diff --git a/crates/plannotator-tui/README.md b/crates/plannotator-tui/README.md index 05c1a54..d028bdb 100644 --- a/crates/plannotator-tui/README.md +++ b/crates/plannotator-tui/README.md @@ -17,6 +17,7 @@ cargo build --release | selection toolbar | `a` 👍 looks good · `c` 💬 comment (opens a box at the selection) · `d` ✗ delete · `Esc` clears | | rail | `j`/`k` move · `e` / `Enter` edit body · `x` remove · `I` attach a local image by path · `D` remove the last image · click a bubble to focus it | | reply review | `S` send feedback and quit; stays open if the agent is at a dialog | +| gated review (`--gate`) | `A` approve, with any unsent notes · `E` send and finish · `q` dismiss; the decision is printed (`--json` for JSON) and, with `--result-file`, written there | | file/folder review | `E` send new · `m` review menu (`R` resend all · `F` finish review · `U` undo · `H` archive) | | tree | `j`/`k` move · `Enter` open · `.` show/hide hidden (dot-prefixed) entries · `E` send new feedback across files, including collapsed folders · counts show active notes per file | | quick open (folder) | `Ctrl+P` find a Markdown file by name anywhere in the folder · type to filter · ↑/↓ or wheel select · `Enter` or click open · `Esc` cancel | diff --git a/crates/plannotator-tui/src/app/approve.rs b/crates/plannotator-tui/src/app/approve.rs new file mode 100644 index 0000000..6277e9b --- /dev/null +++ b/crates/plannotator-tui/src/app/approve.rs @@ -0,0 +1,92 @@ +//! Gate mode in the app: the Approve action, and the decision a gated review ends with. +//! Send and quit keep their meaning; in a gated review each also ends it (decision 17). + +use anyhow::Result; +use plannotator_tui_schema::Provenance; + +use super::App; +use super::feedback::{Feedback, SendScope}; +use super::send::SendState; +use crate::delivery::DeliveryError; +use crate::gate::{self, Decision, Gate}; +use crate::keys::Action; + +#[cfg(test)] +mod tests; + +impl App { + /// Approve and end the review. Notes not yet sent ride along as approval notes, which + /// Plannotator calls approving with notes. An agent receives the approval message; a + /// refused delivery keeps the review open, as a refused send does. + pub(super) fn approve(&mut self) -> Result<()> { + if self.gate != Gate::Approve { + return Ok(()); + } + let scope = if self.is_file_review() { SendScope::Pending } else { SendScope::All }; + let mut feedback = self.prepare_feedback(scope)?; + if self.tree.is_some() { + self.folder_counts = std::mem::take(&mut feedback.counts); + } + let target = self.delivery.describe(); + let retry = + self.keys.labelled(Action::Approve, "retry").unwrap_or_else(|| "click to retry".to_owned()); + match self.delivery.deliver(&gate::approval_message(&feedback.text, &self.context_line())) { + Ok(()) => { + let decision = Decision::Approved { feedback: feedback.text.clone(), count: feedback.count }; + self.archive_submission(&mut feedback, decision.archive_decision()); + // The notes reached the agent with the approval, so they count as sent. The + // review is ending; a record that cannot say so only re-offers them next time. + let _ = self.remember_delivery(&mut feedback, &target); + self.derive_send_state(); + self.decision = Some(decision); + self.quit = true; + } + Err(DeliveryError::Blocked(msg)) => { + self.status = Some(format!("{target} is at a dialog · {retry}")); + self.send_state = SendState::Blocked(msg); + } + Err(DeliveryError::Unavailable(msg)) => { + self.status = Some(format!("no agent to send the approval to ({msg}) · {retry}")); + } + Err(DeliveryError::Failed(err)) => { + self.status = Some(format!("approve failed: {err:#} · {retry}")); + } + } + Ok(()) + } + + /// `Approve (A)`, or `Approve with 2 notes (A)` when unsent notes would ride along. + pub(super) fn approve_label(&self) -> String { + let notes = match self.send_count() { + 0 => String::new(), + 1 => " with 1 note".to_owned(), + n => format!(" with {n} notes"), + }; + let key = match self.keys.hint(Action::Approve) { + key if key.is_empty() => String::new(), + key => format!(" ({key})"), + }; + format!("Approve{notes}{key}") + } + + /// The `File:` or `Folder:` line an approval with notes names its target by. + fn context_line(&self) -> String { + match (&self.tree, &self.open.source.provenance) { + (Some(tree), _) => gate::context_line(tree.root(), true), + (None, Provenance::File { path }) => gate::context_line(path, false), + _ => String::new(), + } + } + + /// How a gated review ended, for whoever ran it; `None` for an ordinary review. + /// Quitting without approving or sending is a dismissal, archived as one. + pub(crate) fn take_decision(&mut self) -> Option { + if self.gate == Gate::Off { + return None; + } + Some(self.decision.take().unwrap_or_else(|| { + self.archive_submission(&mut Feedback::default(), Decision::Dismissed.archive_decision()); + Decision::Dismissed + })) + } +} diff --git a/crates/plannotator-tui/src/app/approve/tests.rs b/crates/plannotator-tui/src/app/approve/tests.rs new file mode 100644 index 0000000..f93a83e --- /dev/null +++ b/crates/plannotator-tui/src/app/approve/tests.rs @@ -0,0 +1,172 @@ +#![allow(clippy::expect_used, clippy::indexing_slicing, reason = "tests assert by panicking")] + +use plannotator_tui_schema::Kind; + +use crate::app::Mode; +use crate::app::review_test_support::{Outcome, click, draw, file_app, folder_app, press}; +use crate::gate::{Decision, Gate}; + +fn archived(app: &crate::app::App) -> Vec { + let index = app.data_dir.join("feedback").join(&app.project).join("index.jsonl"); + std::fs::read_to_string(index) + .unwrap_or_default() + .lines() + .map(|line| serde_json::from_str(line).expect("json line")) + .collect() +} + +#[test] +fn without_a_gate_nothing_changes() { + let (root, mut app, delivery) = file_app("ungated"); + app.add_quote_annotation("one", Kind::Comment, "note".into()).expect("note"); + let screen = draw(&mut app, 100, 12); + assert!(!screen.contains("Approve"), "{screen}"); + assert!(app.geometry.approve_button.is_none()); + assert!(!screen.lines().last().unwrap_or_default().contains("approve")); + + press(&mut app, 'A'); + assert!(delivery.calls.borrow().is_empty(), "A means nothing outside a gate"); + assert!(!app.quit); + press(&mut app, 'E'); + assert_eq!(delivery.calls.borrow().len(), 1); + assert!(!app.quit, "an ordinary send leaves the review open"); + assert_eq!(app.take_decision(), None); + assert!(archived(&app).iter().all(|record| record["decision"] == "feedback")); + let _ = std::fs::remove_dir_all(root); +} + +#[test] +fn a_bare_approval_tells_the_agent_and_ends_the_review() { + let (root, mut app, delivery) = file_app("bare"); + app.gate = Gate::Approve; + press(&mut app, 'A'); + assert_eq!(*delivery.calls.borrow(), ["The user approved."]); + assert!(app.quit); + assert_eq!(app.take_decision(), Some(Decision::Approved { feedback: String::new(), count: 0 })); + let records = archived(&app); + assert_eq!(records.len(), 1); + assert_eq!(records[0]["decision"], "approved"); + assert!(records[0].get("recordFile").is_none(), "a bare approval is a decision-only line"); + let _ = std::fs::remove_dir_all(root); +} + +#[test] +fn unsent_notes_ride_along_as_an_approval_with_notes() { + let (root, mut app, delivery) = file_app("notes"); + app.gate = Gate::Approve; + app.add_quote_annotation("two", Kind::Comment, "keep it short".into()).expect("note"); + let screen = draw(&mut app, 100, 12); + assert!(screen.lines().next().unwrap_or_default().contains("Approve with 1 note (A)"), "{screen}"); + + press(&mut app, 'A'); + let message = delivery.calls.borrow()[0].clone(); + let path = root.join("docs/a.md"); + assert!(message.starts_with("# Approved with Notes\n\nThe artifact is approved."), "{message}"); + assert!(message.contains(&format!("File: {}\n\n# Annotations on a.md", path.display())), "{message}"); + assert!(message.contains("> keep it short"), "{message}"); + assert_eq!(app.send_count(), 0, "the notes went with the approval"); + let decision = app.take_decision(); + assert!( + matches!(&decision, Some(Decision::Approved { feedback, count: 1 }) + if feedback.starts_with("# Annotations on a.md")), + "{decision:?}" + ); + assert_eq!(archived(&app)[0]["decision"], "approved-with-notes"); + let _ = std::fs::remove_dir_all(root); +} + +#[test] +fn a_gated_send_ends_the_review_as_annotated() { + let (root, mut app, delivery) = file_app("send"); + app.gate = Gate::Approve; + app.add_quote_annotation("one", Kind::Comment, "fix".into()).expect("note"); + press(&mut app, 'E'); + assert!(app.quit); + let sent = delivery.calls.borrow()[0].clone(); + assert!(sent.starts_with("# Annotations on a.md"), "the numbered feedback, as an ordinary send"); + assert_eq!(app.take_decision(), Some(Decision::Annotated { feedback: sent, count: 1 })); + assert_eq!(archived(&app)[0]["decision"], "feedback"); + let _ = std::fs::remove_dir_all(root); +} + +#[test] +fn quitting_a_gated_review_dismisses_it() { + let (root, mut app, delivery) = file_app("quit"); + app.gate = Gate::Approve; + press(&mut app, 'q'); + assert!(app.quit); + assert_eq!(app.take_decision(), Some(Decision::Dismissed)); + assert!(delivery.calls.borrow().is_empty(), "a dismissal tells the agent nothing"); + assert_eq!(archived(&app)[0]["decision"], "dismissed"); + + // Unsent notes ask first; declining the send still dismisses. + let (other, mut app, delivery) = file_app("quit-notes"); + app.gate = Gate::Approve; + app.add_quote_annotation("one", Kind::Comment, "fix".into()).expect("note"); + press(&mut app, 'q'); + assert_eq!(app.mode, Mode::ConfirmQuit); + press(&mut app, 'n'); + assert!(app.quit); + assert_eq!(app.take_decision(), Some(Decision::Dismissed)); + assert!(delivery.calls.borrow().is_empty()); + let _ = std::fs::remove_dir_all(root); + let _ = std::fs::remove_dir_all(other); +} + +#[test] +fn a_refused_approval_keeps_the_review_open() { + let (root, mut app, delivery) = file_app("blocked"); + app.gate = Gate::Approve; + delivery.outcome.set(Outcome::Blocked); + press(&mut app, 'A'); + assert!(!app.quit); + assert!(app.status.as_deref().is_some_and(|s| s.contains("at a dialog · A retry")), "{:?}", app.status); + assert!(archived(&app).is_empty(), "nothing was decided"); + delivery.outcome.set(Outcome::Success); + press(&mut app, 'A'); + assert!(app.quit); + let _ = std::fs::remove_dir_all(root); +} + +#[test] +fn the_approve_button_sits_on_the_edge_and_approves_when_clicked() { + let (root, mut app, delivery) = file_app("button"); + app.gate = Gate::Approve; + let screen = draw(&mut app, 100, 12); + let header = screen.lines().next().unwrap_or_default(); + assert!(header.trim_end().ends_with("Approve (A)"), "{header}"); + let footer = screen.lines().last().unwrap_or_default(); + assert!(footer.contains("A approve"), "{footer}"); + let rect = app.geometry.approve_button.expect("drawn"); + assert_eq!(rect.right(), 100); + click(&mut app, rect); + assert_eq!(*delivery.calls.borrow(), ["The user approved."]); + assert!(app.quit); + let _ = std::fs::remove_dir_all(root); +} + +#[test] +fn json_alone_decides_without_an_approve() { + let (root, mut app, delivery) = file_app("decision"); + app.gate = Gate::Decision; + let screen = draw(&mut app, 100, 12); + assert!(!screen.contains("Approve"), "{screen}"); + press(&mut app, 'A'); + assert!(delivery.calls.borrow().is_empty() && !app.quit); + press(&mut app, 'q'); + assert_eq!(app.take_decision(), Some(Decision::Dismissed)); + let _ = std::fs::remove_dir_all(root); +} + +#[test] +fn a_folder_approval_names_the_folder() { + let (root, mut app, delivery) = folder_app("folder"); + app.gate = Gate::Approve; + app.add_quote_annotation("one", Kind::Comment, "fix".into()).expect("note"); + app.refresh_review_counts(); + press(&mut app, 'A'); + let message = delivery.calls.borrow()[0].clone(); + assert!(message.contains(&format!("Folder: {}\n\n", root.join("docs").display())), "{message}"); + assert!(matches!(app.take_decision(), Some(Decision::Approved { count: 1, .. }))); + let _ = std::fs::remove_dir_all(root); +} diff --git a/crates/plannotator-tui/src/app/draw.rs b/crates/plannotator-tui/src/app/draw.rs index 7441f4c..c9c8639 100644 --- a/crates/plannotator-tui/src/app/draw.rs +++ b/crates/plannotator-tui/src/app/draw.rs @@ -11,6 +11,7 @@ use ratatui::widgets::{Block, BorderType, Borders, Clear, Paragraph}; use unicode_width::UnicodeWidthStr; use super::{App, Focus, GUTTER, Geometry, Mode, TOOLBAR, glyph, label}; +use crate::gate::Gate; use crate::keys::Action; use crate::theme::palette; use crate::wrap::wrap_line; @@ -487,6 +488,7 @@ impl App { let send_and_close = if self.is_file_review() { None } else { key(Action::SendAndClose, "send+quit") }; let quit = [send_and_close, key(Action::Quit, "quit")]; + let approve = (self.gate == Gate::Approve).then(|| key(Action::Approve, "approve")).flatten(); // The image keys act on the current note; removing is offered only when it has one. let note = self.current_note(); let has_images = note.as_deref().is_some_and(|id| self.open.store.has_images(id)); @@ -503,6 +505,7 @@ impl App { key(Action::Confirm, "open"), key(Action::ToggleHidden, "hidden"), key(Action::Send, "send"), + approve, key(Action::ToggleTree, "hide"), key(Action::Quit, "quit"), ], @@ -533,6 +536,7 @@ impl App { note.is_some().then(|| key(Action::AttachImage, "image")).flatten(), remove_image, key(Action::Send, "send"), + approve, tab, ] .into_iter() diff --git a/crates/plannotator-tui/src/app/header.rs b/crates/plannotator-tui/src/app/header.rs index 8f074b4..6670584 100644 --- a/crates/plannotator-tui/src/app/header.rs +++ b/crates/plannotator-tui/src/app/header.rs @@ -1,5 +1,6 @@ -//! The header: the send button and, for file and folder reviews, the Review menu button. -//! The two wrap onto another row only when a pane is too narrow for both. +//! The header: the send button and, for file and folder reviews, the Review menu button; a +//! gated review puts its Approve button on the edge. They wrap onto another row only when +//! a pane is too narrow for them. use ratatui::Frame; use ratatui::layout::Rect; @@ -9,6 +10,7 @@ use unicode_width::UnicodeWidthStr; use super::App; use super::send::SendState; +use crate::gate::Gate; use crate::keys::Action; use crate::theme::palette; @@ -17,23 +19,29 @@ use crate::theme::palette; const SEND_BG: Color = Color::Indexed(30); const SENT_BG: Color = Color::Indexed(22); const BLOCKED_BG: Color = Color::Indexed(58); +const APPROVE_BG: Color = Color::Indexed(28); const REVIEW_LABEL: &str = "Review \u{25be}"; #[derive(Debug, Clone, Copy)] enum Button { + Approve, Send, Review, } impl App { /// Buttons with their rects relative to the header, right to left: send on the edge, - /// the Review menu to its left. + /// the Review menu to its left. A gated review puts Approve on the edge, before send. fn header_buttons(&self, width: u16) -> Vec<(Button, String, Rect)> { if width == 0 { return Vec::new(); } - let mut labels = vec![(Button::Send, self.send_label())]; + let mut labels = Vec::new(); + if self.gate == Gate::Approve { + labels.push((Button::Approve, self.approve_label())); + } + labels.push((Button::Send, self.send_label())); if self.is_file_review() { labels.push((Button::Review, self.review_label())); } @@ -75,6 +83,10 @@ impl App { rect.x += area.x; rect.y += area.y; let style = match button { + Button::Approve => { + self.geometry.approve_button = Some(rect); + Style::new().fg(Color::Black).bg(APPROVE_BG).bold() + } Button::Send => { self.geometry.send_button = Some(rect); self.button_style() diff --git a/crates/plannotator-tui/src/app/input.rs b/crates/plannotator-tui/src/app/input.rs index 287dd12..5088e1f 100644 --- a/crates/plannotator-tui/src/app/input.rs +++ b/crates/plannotator-tui/src/app/input.rs @@ -10,6 +10,7 @@ use super::selection::Selection; use super::send::SendState; use super::{App, Focus, GUTTER, Mode, Pending, TOOLBAR}; use crate::delivery::Delivery as _; +use crate::gate::Gate; use crate::keys::{self, Action}; impl App { @@ -86,6 +87,7 @@ impl App { return Ok(()); } Some(Action::Send) => return self.send_feedback(), + Some(Action::Approve) if self.gate == Gate::Approve => return self.approve(), // A reply review sends every note, so one the agent already has closes like `q`. Some(Action::SendAndClose) if !self.is_file_review() => { if self.send_state == SendState::Sent { @@ -463,6 +465,9 @@ impl App { let hit = |rect: Option| { rect.is_some_and(|r| mouse.row == r.y && mouse.column >= r.x && mouse.column < r.right()) }; + if hit(self.geometry.approve_button) { + return self.approve(); + } if hit(self.geometry.review_button) { self.open_review_menu(); return Ok(()); diff --git a/crates/plannotator-tui/src/app/mod.rs b/crates/plannotator-tui/src/app/mod.rs index 1a7cc56..4ad0f30 100644 --- a/crates/plannotator-tui/src/app/mod.rs +++ b/crates/plannotator-tui/src/app/mod.rs @@ -1,6 +1,7 @@ //! Application state. Input handling lives in `input`, drawing in `draw`; this module owns //! the data they share and the operations that change it. +mod approve; mod archive_view; mod compose; mod disk; @@ -34,6 +35,7 @@ use ratatui::layout::Rect; use crate::config::UiConfig; use crate::delivery::Delivery; use crate::doc::Document; +use crate::gate::{Decision, Gate}; use crate::keys::{Action, Keymap}; use crate::layout::DocLayout; use crate::store::{Location, Store}; @@ -94,6 +96,8 @@ struct Geometry { bubbles: Vec<(Rect, String)>, /// The header's Send button; `None` when the header was too narrow for it. send_button: Option, + /// The header's Approve button; only a gated review draws it. + approve_button: Option, /// The header's Review button; only file and folder reviews draw it. review_button: Option, undo_button: Option, @@ -162,6 +166,11 @@ pub(crate) struct App { tree_visible: Option, delivery: Box, send_state: SendState, + /// What ending the review means; `Gate::Off` unless `--gate` or `--json` asked for a + /// decision. + pub(crate) gate: Gate, + /// How a gated review ended, once it has; quitting without one is a dismissal. + decision: Option, folder_counts: HashMap, /// Annotated files the folder counts could not read, in `review_files` order. unreadable_files: Vec, @@ -257,6 +266,8 @@ impl App { tree_visible: None, delivery, send_state, + gate: Gate::Off, + decision: None, folder_counts: HashMap::new(), unreadable_files: Vec::new(), undo_archive: Vec::new(), diff --git a/crates/plannotator-tui/src/app/send.rs b/crates/plannotator-tui/src/app/send.rs index 4a8537d..84fd8a8 100644 --- a/crates/plannotator-tui/src/app/send.rs +++ b/crates/plannotator-tui/src/app/send.rs @@ -8,6 +8,7 @@ use plannotator_tui_schema::Provenance; use super::feedback::{Feedback, FeedbackPart, ReviewCounts, SendScope}; use super::{App, Mode}; use crate::delivery::{Clipboard, Delivery as _, DeliveryError}; +use crate::gate::{Decision, Gate}; use crate::keys::Action; use crate::store::{Location, Store}; @@ -50,7 +51,8 @@ impl App { match self.delivery.deliver(&feedback.text) { Ok(()) => { let files = feedback.parts.len(); - self.archive_submission(&mut feedback); + let sent = (feedback.text.clone(), feedback.count); + self.archive_submission(&mut feedback, "feedback"); let errors = self.remember_delivery(&mut feedback, &target); self.derive_send_state(); let verb = if self.delivery.is_agent() { "sent" } else { "copied" }; @@ -70,6 +72,12 @@ impl App { ); } self.status = Some(status); + // A gated review ends with the first send: one review, one decision. + if self.gate != Gate::Off { + let (feedback, count) = sent; + self.decision = Some(Decision::Annotated { feedback, count }); + self.quit = true; + } } Err(DeliveryError::Blocked(msg)) => { let copied = if self.copy_fallback(&feedback.text) { " · copied to clipboard" } else { "" }; @@ -95,7 +103,7 @@ impl App { /// Keep the ids from the body we delivered. Attempt every file even when one record /// cannot be saved, and refresh counts from any intervening changes. - fn remember_delivery(&mut self, feedback: &mut Feedback, target: &str) -> Vec { + pub(super) fn remember_delivery(&mut self, feedback: &mut Feedback, target: &str) -> Vec { let mut errors = Vec::new(); for mut part in feedback.parts.drain(..) { if let Err(err) = self.record_feedback_delivery(&mut part, target) { @@ -142,7 +150,8 @@ impl App { /// The shared submission history records only the selected feedback. Finishing a /// review has its own complete copies in the annotation record and does not rely on it. - fn archive_submission(&self, feedback: &mut Feedback) { + /// `decision` is the archive's word for it: `feedback` for a send, or a gate's decision. + pub(super) fn archive_submission(&self, feedback: &mut Feedback, decision: &str) { use crate::archive::{self, Submission, Target}; if !archive::enabled(|key| std::env::var(key).ok(), &self.data_dir) { return; @@ -170,6 +179,7 @@ impl App { data_dir: &self.data_dir, project: &self.project, surface, + decision, origin, target, feedback: &feedback.text, diff --git a/crates/plannotator-tui/src/archive.rs b/crates/plannotator-tui/src/archive.rs index 94319d9..c1e6524 100644 --- a/crates/plannotator-tui/src/archive.rs +++ b/crates/plannotator-tui/src/archive.rs @@ -133,7 +133,7 @@ struct Record<'a> { #[serde(skip_serializing_if = "Option::is_none")] origin: Option<&'a str>, surface: &'a str, - decision: &'static str, + decision: &'a str, #[serde(skip_serializing_if = "Option::is_none")] target: Option<&'a Target>, #[serde(skip_serializing_if = "str::is_empty")] @@ -150,6 +150,9 @@ pub(crate) struct Submission<'a> { pub(crate) project: &'a str, /// "annotate" | "annotate-folder" | "annotate-last". pub(crate) surface: &'a str, + /// "feedback" for a send; a gated review also writes "approved", "approved-with-notes" + /// and "dismissed". + pub(crate) decision: &'a str, /// Receiving agent host in Plannotator's spelling; absent for clipboard delivery. pub(crate) origin: Option, pub(crate) target: Target, @@ -186,7 +189,7 @@ fn try_append(submission: &Submission<'_>) -> std::io::Result { project: submission.project, origin: submission.origin.as_deref(), surface: submission.surface, - decision: "feedback", + decision: submission.decision, target: (!submission.target.is_empty()).then_some(&submission.target), feedback: submission.feedback, annotations: &submission.annotations, @@ -212,7 +215,8 @@ fn try_append(submission: &Submission<'_>) -> std::io::Result { fn write_sidecar(project_dir: &Path, submission: &Submission<'_>, ts: &str) -> std::io::Result { let records_dir = project_dir.join("records"); std::fs::create_dir_all(&records_dir)?; - let base = format!("{}-{}-feedback-{CLIENT}", ts.replace([':', '.'], "-"), submission.surface); + let base = + format!("{}-{}-{}-{CLIENT}", ts.replace([':', '.'], "-"), submission.surface, submission.decision); let body = render_markdown(submission, ts); for n in 1..=100u32 { let name = if n == 1 { format!("{base}.md") } else { format!("{base}-{n}.md") }; @@ -259,7 +263,7 @@ fn render_markdown(submission: &Submission<'_>, ts: &str) -> String { let mut lines = vec![format!("# {title}"), String::new()]; lines.push(format!("- Submitted: {ts}")); lines.push(format!("- Surface: {}", submission.surface)); - lines.push("- Decision: feedback".to_owned()); + lines.push(format!("- Decision: {}", submission.decision)); lines.push(format!("- Project: {}", submission.project)); if let Some(origin) = &submission.origin { lines.push(format!("- Origin: {origin}")); @@ -325,6 +329,7 @@ mod tests { data_dir, project: "demo", surface: "annotate", + decision: "feedback", origin: Some("claude-code".to_owned()), target: Target::file(Path::new("/work/plan.md")), feedback, @@ -470,4 +475,34 @@ mod tests { assert!(value["target"].get("filePath").is_none()); let _ = std::fs::remove_dir_all(d); } + + #[test] + fn an_approval_with_notes_is_named_by_its_decision() { + let d = dir("approved-notes"); + let sub = Submission { decision: "approved-with-notes", ..submission(&d, "1. tighten this") }; + let index = try_append(&sub).unwrap(); + let value: serde_json::Value = + serde_json::from_str(std::fs::read_to_string(index).unwrap().trim()).unwrap(); + assert_eq!(value["decision"], "approved-with-notes"); + let record = "records/2026-09-01T06-00-00-123Z-annotate-approved-with-notes-plannotator-tui.md"; + assert_eq!(value["recordFile"], record); + let body = std::fs::read_to_string(d.join("feedback/demo").join(record)).unwrap(); + assert!(body.contains("- Decision: approved-with-notes\n"), "{body}"); + let _ = std::fs::remove_dir_all(d); + } + + #[test] + fn a_bare_approval_or_dismissal_is_a_decision_only_line() { + for decision in ["approved", "dismissed"] { + let d = dir(decision); + let sub = Submission { decision, annotations: Vec::new(), count: 0, ..submission(&d, "") }; + let index = try_append(&sub).unwrap(); + let value: serde_json::Value = + serde_json::from_str(std::fs::read_to_string(index).unwrap().trim()).unwrap(); + assert_eq!(value["decision"], decision); + assert!(value.get("recordFile").is_none()); + assert!(!d.join("feedback/demo/records").exists()); + let _ = std::fs::remove_dir_all(d); + } + } } diff --git a/crates/plannotator-tui/src/cli.rs b/crates/plannotator-tui/src/cli.rs index 232fbf9..6f2e73f 100644 --- a/crates/plannotator-tui/src/cli.rs +++ b/crates/plannotator-tui/src/cli.rs @@ -4,8 +4,9 @@ #![allow(clippy::print_stdout, reason = "the CLI's job is to print")] -use std::io::stdout; +use std::io::{IsTerminal as _, Write as _, stdout}; use std::path::{Path, PathBuf}; +use std::process::ExitCode; use std::time::{Duration, Instant}; use anyhow::{Context, Result}; @@ -18,13 +19,14 @@ use ratatui::crossterm::execute; use crate::app::App; use crate::config::Config; -use crate::delivery::{Clipboard, Delivery, Discard, HerdrAgent}; +use crate::delivery::{Caller, Clipboard, Delivery, Discard, HerdrAgent}; use crate::doc::Document; +use crate::gate::{self, Decision, GateError, ReviewArgs}; use crate::herdr::context::HerdrEnv; use crate::layout::DocLayout; const USAGE: &str = "usage: - plannotator-tui + plannotator-tui [--gate] [--json] [--require-approval] [--result-file ] plannotator-tui --export plannotator-tui --bench plannotator-tui --blocks @@ -34,6 +36,7 @@ const USAGE: &str = "usage: plannotator-tui config plannotator-tui --version plannotator-tui herdr open [file.md | folder] [--placement overlay|split|popup] [--deliver-to ] + [--gate] [--result-file ] plannotator-tui herdr last [--placement P] [--deliver-to ] [--newest] plannotator-tui herdr terminal [--lines N] [--placement P] [--deliver-to ] [--print] plannotator-tui herdr pane @@ -57,18 +60,30 @@ pub(crate) fn delivery(interactive: bool) -> Box { if !interactive { return Box::new(Discard); } + herdr_agent().unwrap_or_else(|| Box::new(Clipboard)) +} + +/// A gated review sends to the same agent pane; without one its decision goes to the caller. +fn gate_delivery() -> Box { + herdr_agent().unwrap_or_else(|| Box::new(Caller)) +} + +fn herdr_agent() -> Option> { let env = HerdrEnv::from_env(); match env.delivery_target() { Some(target) if env.in_herdr => { let agent = target.agent.or_else(|| env.agent_in_pane(&target.pane)); - Box::new(HerdrAgent::new(env.bin, target.pane, agent)) + Some(Box::new(HerdrAgent::new(env.bin, target.pane, agent))) } - _ => Box::new(Clipboard), + _ => None, } } fn open_app(path: &PathBuf, width: usize, interactive: bool) -> Result { - let delivery = delivery(interactive); + open_review(path, width, delivery(interactive)) +} + +fn open_review(path: &PathBuf, width: usize, delivery: Box) -> Result { if path.is_dir() { App::open_folder(path, width, delivery) } else { @@ -107,7 +122,28 @@ fn annotate_options(rest: &[String]) -> Result<(Kind, usize)> { Ok((kind.unwrap_or(Kind::Comment), occurrence)) } -pub(crate) fn run(args: &[String]) -> Result<()> { +pub(crate) fn run(args: &[String]) -> Result { + if wants_decision(args) { + return review_command(args); + } + tool(args).map(|()| ExitCode::SUCCESS) +} + +/// A review with a decision flag, which may come before the path. Without one the +/// invocation means what it always has. +fn wants_decision(args: &[String]) -> bool { + let decision_flag = |arg: &String| gate::FLAGS.contains(&arg.as_str()); + match args.first() { + Some(first) if decision_flag(first) => true, + Some(first) if first.starts_with('-') || matches!(first.as_str(), "config" | "herdr" | "last") => { + false + } + Some(_) => args.iter().skip(1).any(decision_flag), + None => false, + } +} + +fn tool(args: &[String]) -> Result<()> { let arg = |i: usize| args.get(i).map(String::as_str); let path = |i: usize| arg(i).map(|p| crate::workspace_paths::absolute(Path::new(p))).context(USAGE); match arg(0) { @@ -173,9 +209,60 @@ fn show_config() -> Result<()> { Ok(()) } -/// `plannotator-tui herdr open [PATH] [--placement P] [--deliver-to PANE]`; -/// `herdr last` takes `--newest` on top, which `open` has nothing to skip. +/// `plannotator-tui [--gate] [--json] [--require-approval] [--result-file PATH]`: +/// the review ends with one decision, printed on stdout and published to the result file +/// (decision 17). Under a strict flag every failure is a [`GateError`]. +fn review_command(args: &[String]) -> Result { + let strict = gate::is_strict(args); + gated_review(args).map_err(|err| if strict { GateError(err).into() } else { err }) +} + +fn gated_review(args: &[String]) -> Result { + let options = ReviewArgs::parse(args).map_err(|err| anyhow::anyhow!("{err:#}\n{USAGE}"))?; + let path = crate::workspace_paths::absolute(&options.path); + anyhow::ensure!(path.exists(), "{}: no such file or folder", path.display()); + let result_file = options.result_file.as_deref().map(crate::workspace_paths::absolute); + if let Some(file) = &result_file { + gate::check_result_file(file)?; + } + // The review draws on stdout. Redirected, nobody could see it and the gate would wait + // forever; the result file is how a script captures the decision. + anyhow::ensure!( + stdout().is_terminal(), + "stdout is not a terminal: the review is drawn there; read the decision from --result-file instead" + ); + let mode = options.mode(); + let decision = run_review(|width| { + let mut app = open_review(&path, width, gate_delivery())?; + app.gate = mode; + Ok(app) + })? + .unwrap_or(Decision::Dismissed); + // Stdout first, as Plannotator does: if the file cannot be published, the decision has + // still reached the caller once. + let mut out = stdout().lock(); + if options.json { + let line = if options.strict() { decision.strict_json() } else { decision.json(&path) }; + writeln!(out, "{line}")?; + } else if let Some(text) = decision.plain(&gate::context_line(&path, path.is_dir())) { + writeln!(out, "{text}")?; + } + out.flush()?; + if let Some(file) = &result_file { + gate::publish(file, &decision.strict_json())?; + } + Ok(ExitCode::from(decision.exit_code(options.require_approval))) +} + +/// `plannotator-tui herdr open [PATH] [--placement P] [--deliver-to PANE] [--gate] +/// [--result-file PATH]`; `herdr last` takes `--newest` on top, which `open` has nothing to +/// skip. A gated open with a result file fails as a [`GateError`], like the direct gate. fn herdr_command(args: &[String]) -> Result<()> { + let strict = args.first().is_some_and(|sub| sub == "open") && gate::is_strict(args); + herdr_launch(args).map_err(|err| if strict { GateError(err).into() } else { err }) +} + +fn herdr_launch(args: &[String]) -> Result<()> { use crate::herdr::launch::{OpenArgs, agent_get, agent_identity, plan, plan_last, process_info, run}; let sub = args.first().map(String::as_str); if sub == Some("pane") { @@ -202,11 +289,24 @@ fn herdr_command(args: &[String]) -> Result<()> { "--newest" => { anyhow::bail!("--newest is only for `herdr last`; `herdr open` has no picker to skip") } + "--gate" if sub == Some("open") => open.gate = true, + "--result-file" if sub == Some("open") => { + let value = rest.next().filter(|v| !v.starts_with("--")); + let value = value.context("missing value for --result-file")?; + open.result_file = Some(crate::workspace_paths::absolute(Path::new(value))); + } + "--gate" | "--result-file" => anyhow::bail!("{arg} is only for `herdr open`"), flag if flag.starts_with("--") => anyhow::bail!("unknown flag {flag}\n{USAGE}"), path if open.path.is_none() => open.path = Some(PathBuf::from(path)), extra => anyhow::bail!("unexpected argument {extra:?}\n{USAGE}"), } } + if let Some(file) = &open.result_file { + anyhow::ensure!(open.gate, "--result-file requires --gate"); + // The pane publishes when the review ends; a name that is already taken would only + // fail then, after the review. + gate::check_result_file(file)?; + } let env = HerdrEnv::from_env(); let config = Config::load()?; let cwd = std::env::current_dir().context("current directory")?; @@ -286,7 +386,7 @@ fn herdr_pane() -> Result<()> { }) } else { let path = env.file.clone().unwrap_or(std::env::current_dir().context("current directory")?); - interactive(&path) + if env.gate { gated_pane(&path, env.result_file.as_deref()) } else { interactive(&path) } }; // The pane closes when we exit; an error that flashes by is an error nobody can read. if let Err(err) = &result { @@ -326,12 +426,35 @@ fn last_command(args: &[String]) -> Result<()> { crate::last::run(&options) } +/// A gated review in a Herdr pane: the agent hears the decision through delivery, and the +/// result file, when asked for, records it. The pane has no caller reading its stdout. +fn gated_pane(path: &PathBuf, result_file: Option<&Path>) -> Result<()> { + if let Some(file) = result_file { + gate::check_result_file(file)?; + } + let decision = run_review(|width| { + let mut app = open_review(path, width, gate_delivery())?; + app.gate = gate::Gate::Approve; + Ok(app) + })? + .unwrap_or(Decision::Dismissed); + match result_file { + Some(file) => gate::publish(file, &decision.strict_json()), + None => Ok(()), + } +} + fn interactive(path: &PathBuf) -> Result<()> { run_ui(|width| open_app(path, width, true)) } /// Own the terminal for one app: `build` gets the document width the screen allows. pub(crate) fn run_ui(build: impl FnOnce(usize) -> Result) -> Result<()> { + run_review(build).map(drop) +} + +/// `run_ui`, returning how a gated review ended; `None` for an ordinary one. +fn run_review(build: impl FnOnce(usize) -> Result) -> Result> { // Settle the palette before the screen is ours: the background-colour query talks to // the terminal directly, and it must not race the alternate screen or the event loop. // A config that fails to parse never kept the plain TUI from starting; it still does @@ -407,7 +530,7 @@ fn run_editor( Ok(outcome) } -fn event_loop(terminal: &mut ratatui::DefaultTerminal, mut app: App) -> Result<()> { +fn event_loop(terminal: &mut ratatui::DefaultTerminal, mut app: App) -> Result> { app.clipboard = true; let mut dirty = true; while !app.quit { @@ -435,7 +558,7 @@ fn event_loop(terminal: &mut ratatui::DefaultTerminal, mut app: App) -> Result<( } } } - Ok(()) + Ok(app.take_decision()) } /// Headless timing of the expensive paths: parse, per-block render + align, and reflow. diff --git a/crates/plannotator-tui/src/delivery.rs b/crates/plannotator-tui/src/delivery.rs index 3ea5694..b43674b 100644 --- a/crates/plannotator-tui/src/delivery.rs +++ b/crates/plannotator-tui/src/delivery.rs @@ -91,6 +91,26 @@ impl Delivery for Discard { } } +/// A gated review with no agent pane: the decision reaches whoever ran the review, on +/// stdout and in the result file, when the review ends. Sending only ends it, so like an +/// agent it is a target that acts on the feedback, and quitting with notes unsent asks first. +#[derive(Debug, Default)] +pub(crate) struct Caller; + +impl Delivery for Caller { + fn describe(&self) -> String { + "caller".to_owned() + } + + fn is_agent(&self) -> bool { + true + } + + fn deliver(&self, _feedback: &str) -> Result<(), DeliveryError> { + Ok(()) + } +} + /// An agent running in a Herdr pane. `herdr agent prompt` pastes the feedback as the /// agent's next message and presses Enter. #[derive(Debug)] diff --git a/crates/plannotator-tui/src/gate.rs b/crates/plannotator-tui/src/gate.rs new file mode 100644 index 0000000..d0e608b --- /dev/null +++ b/crates/plannotator-tui/src/gate.rs @@ -0,0 +1,267 @@ +//! Gate mode: an explicit Approve beside Send and quit, and the machine-readable decision a +//! script reads when the review ends (decision 17). +//! +//! The contract is Plannotator's `annotate --gate` (`apps/hook/server/strict-annotate-result.ts`, +//! `annotate-output.ts`, `packages/shared/prompts.ts`): knowledge copied, not code. One review, +//! one decision: `approved` (with or without notes), `annotated`, or `dismissed`. + +use std::fmt; +use std::io::Write as _; +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result}; +use serde::Serialize; + +#[cfg(test)] +mod tests; + +/// What ending the review means. `Off` is the ordinary review: sending leaves it open and +/// quitting decides nothing. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub(crate) enum Gate { + #[default] + Off, + /// `--json` alone: Send or quit ends the review with a decision; there is no Approve. + Decision, + /// `--gate`: Approve as well. + Approve, +} + +/// How a gated review ended. `feedback` is the numbered feedback the notes render to, the +/// same text Send delivers; `count` is how many notes it carries. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum Decision { + Approved { feedback: String, count: usize }, + Annotated { feedback: String, count: usize }, + Dismissed, +} + +/// Plannotator's plaintext approval, byte for byte. +pub(crate) const APPROVED: &str = "The user approved."; + +/// Plannotator's `approvedWithNotes` annotate prompt; `{context}` is a `File:`/`Folder:` line +/// followed by a blank line. +const APPROVED_WITH_NOTES: &str = "# Approved with Notes\n\nThe artifact is approved. The notes below are \ +non-blocking guidance, not a request for another revision.\n\n{context}{feedback}\n\nDo not revise or \ +reopen the artifact solely because of these notes unless the user explicitly requests it. Carry the \ +notes into subsequent work where applicable."; + +/// The exit code for a failure of the gate itself under a strict flag: bad flags, a document +/// that cannot be opened, a result file that cannot be published. Never a reviewer outcome. +pub(crate) const GATE_ERROR_EXIT: u8 = 2; + +/// An error the strict gate reports with [`GATE_ERROR_EXIT`]. +#[derive(Debug)] +pub(crate) struct GateError(pub(crate) anyhow::Error); + +impl fmt::Display for GateError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{:#}", self.0) + } +} + +impl std::error::Error for GateError {} + +/// One decision record. Field order is the browser's. +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct Record<'a> { + decision: &'static str, + #[serde(skip_serializing_if = "Option::is_none")] + feedback: Option<&'a str>, + #[serde(skip_serializing_if = "Option::is_none")] + annotation_count: Option, + #[serde(skip_serializing_if = "Option::is_none")] + target: Option<&'a str>, +} + +impl Record<'_> { + fn line(&self) -> String { + // A struct of strings and numbers always serializes. + serde_json::to_string(self).unwrap_or_default() + } +} + +impl Decision { + pub(crate) fn is_approved(&self) -> bool { + matches!(self, Self::Approved { .. }) + } + + /// The record `--require-approval` and `--result-file` publish: the decision and its + /// feedback, nothing else. A bare approval carries no `feedback`; a dismissal never does. + pub(crate) fn strict_json(&self) -> String { + self.record().line() + } + + /// `--json` without a strict flag: the strict record plus `annotationCount` and the + /// reviewed `target` (an absolute path), which the browser adds there too. + pub(crate) fn json(&self, target: &Path) -> String { + let target = target.display().to_string(); + let annotation_count = match self { + Self::Approved { count, .. } | Self::Annotated { count, .. } => Some(*count), + Self::Dismissed => None, + }; + Record { annotation_count, target: Some(&target), ..self.record() }.line() + } + + fn record(&self) -> Record<'_> { + let (decision, feedback) = match self { + Self::Approved { feedback, .. } => ("approved", non_empty(feedback)), + Self::Annotated { feedback, .. } => ("annotated", Some(feedback.as_str())), + Self::Dismissed => ("dismissed", None), + }; + Record { decision, feedback, annotation_count: None, target: None } + } + + /// Plaintext stdout under `--gate`: the approval message, the feedback, or nothing for a + /// dismissal or an empty send. + pub(crate) fn plain(&self, context: &str) -> Option { + match self { + Self::Approved { feedback, .. } => Some(approval_message(feedback, context)), + Self::Annotated { feedback, .. } => non_empty(feedback.trim_end()).map(str::to_owned), + Self::Dismissed => None, + } + } + + /// The feedback archive's `decision` for this outcome (Plannotator's `FeedbackDecision`). + pub(crate) fn archive_decision(&self) -> &'static str { + match self { + Self::Approved { feedback, .. } if feedback.trim().is_empty() => "approved", + Self::Approved { .. } => "approved-with-notes", + Self::Annotated { .. } => "feedback", + Self::Dismissed => "dismissed", + } + } + + /// `--require-approval` exits 0 only on approval; otherwise every decision exits 0. + pub(crate) fn exit_code(&self, require_approval: bool) -> u8 { + u8::from(require_approval && !self.is_approved()) + } +} + +fn non_empty(text: &str) -> Option<&str> { + (!text.is_empty()).then_some(text) +} + +/// What an approval tells the agent: Plannotator's bare marker, or its approved-with-notes +/// message naming the reviewed file or folder when notes ride along. +pub(crate) fn approval_message(feedback: &str, context: &str) -> String { + let feedback = feedback.trim_end(); + if feedback.trim().is_empty() { + return APPROVED.to_owned(); + } + let context = if context.is_empty() { String::new() } else { format!("{context}\n\n") }; + APPROVED_WITH_NOTES.replace("{context}", &context).replace("{feedback}", feedback) +} + +/// The `File: ` or `Folder: ` line an approval with notes names its target by. +pub(crate) fn context_line(path: &Path, folder: bool) -> String { + format!("{}: {}", if folder { "Folder" } else { "File" }, path.display()) +} + +/// `plannotator-tui [--gate] [--json] [--require-approval] [--result-file PATH]`. +#[derive(Debug, Default, PartialEq, Eq)] +pub(crate) struct ReviewArgs { + pub(crate) path: PathBuf, + pub(crate) gate: bool, + pub(crate) json: bool, + pub(crate) require_approval: bool, + /// As given; the caller resolves it from the invocation's directory. + pub(crate) result_file: Option, +} + +/// The flags that turn a review into a decision. +pub(crate) const FLAGS: [&str; 4] = ["--gate", "--json", "--require-approval", "--result-file"]; + +/// Whether the invocation carries a strict flag, which owns the exit-code contract: every +/// failure, even a mistyped flag, exits [`GATE_ERROR_EXIT`] so it never reads as a rejection. +pub(crate) fn is_strict(args: &[String]) -> bool { + args.iter().any(|arg| arg == "--require-approval" || arg == "--result-file") +} + +impl ReviewArgs { + pub(crate) fn parse(args: &[String]) -> Result { + let mut out = Self::default(); + let mut path = None; + let mut rest = args.iter(); + let once = |flag: &mut bool, name: &str| { + anyhow::ensure!(!*flag, "{name} may only be specified once"); + *flag = true; + Ok(()) + }; + while let Some(arg) = rest.next() { + match arg.as_str() { + "--gate" => once(&mut out.gate, "--gate")?, + "--json" => once(&mut out.json, "--json")?, + "--require-approval" => once(&mut out.require_approval, "--require-approval")?, + "--result-file" => { + anyhow::ensure!(out.result_file.is_none(), "--result-file may only be specified once"); + let value = rest.next().filter(|v| !v.starts_with("--")); + out.result_file = Some(PathBuf::from(value.context("missing value for --result-file")?)); + } + flag if flag.starts_with("--") => anyhow::bail!("unknown flag {flag}"), + other if path.is_none() => path = Some(PathBuf::from(other)), + other => anyhow::bail!("unexpected argument {other:?}"), + } + } + out.path = path.context("missing the file or folder to review")?; + if out.strict() { + anyhow::ensure!( + out.gate && out.json, + "--require-approval and --result-file require --gate --json" + ); + } + Ok(out) + } + + pub(crate) fn strict(&self) -> bool { + self.require_approval || self.result_file.is_some() + } + + pub(crate) fn mode(&self) -> Gate { + if self.gate { + Gate::Approve + } else if self.json { + Gate::Decision + } else { + Gate::Off + } + } +} + +/// Fail before the review opens when the result could never be published: the file must not +/// exist (not even as a dangling link) and its folder must. +pub(crate) fn check_result_file(path: &Path) -> Result<()> { + match path.symlink_metadata() { + Ok(_) => anyhow::bail!("result file already exists: {}", path.display()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => return Err(err).with_context(|| format!("checking result file {}", path.display())), + } + let parent = path.parent().unwrap_or(Path::new("")); + anyhow::ensure!(parent.is_dir(), "result file folder does not exist: {}", parent.display()); + Ok(()) +} + +/// Publish `record` and a newline to `path` atomically, never over an existing file: a +/// private temporary file beside it, flushed and closed, then hard-linked into place. A +/// link fails when the name is taken, so a reader sees no file or the whole record. +pub(crate) fn publish(path: &Path, record: &str) -> Result<()> { + let parent = path.parent().unwrap_or(Path::new("")); + let name = path.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default(); + let nanos = + std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map_or(0, |d| d.as_nanos()); + let temporary = parent.join(format!(".{name}.{}.{nanos}.tmp", std::process::id())); + let result = write_new(&temporary, record).and_then(|()| std::fs::hard_link(&temporary, path)); + let _ = std::fs::remove_file(&temporary); + result.with_context(|| format!("publishing result file {}", path.display())) +} + +fn write_new(path: &Path, record: &str) -> std::io::Result<()> { + let mut options = std::fs::OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + std::os::unix::fs::OpenOptionsExt::mode(&mut options, 0o600); + let mut file = options.open(path)?; + file.write_all(format!("{record}\n").as_bytes())?; + file.sync_all() +} diff --git a/crates/plannotator-tui/src/gate/tests.rs b/crates/plannotator-tui/src/gate/tests.rs new file mode 100644 index 0000000..f848979 --- /dev/null +++ b/crates/plannotator-tui/src/gate/tests.rs @@ -0,0 +1,183 @@ +#![allow(clippy::expect_used, reason = "tests assert by panicking")] + +use super::*; + +fn args(text: &str) -> Vec { + text.split_whitespace().map(str::to_owned).collect() +} + +fn approved(feedback: &str, count: usize) -> Decision { + Decision::Approved { feedback: feedback.to_owned(), count } +} + +fn annotated(feedback: &str, count: usize) -> Decision { + Decision::Annotated { feedback: feedback.to_owned(), count } +} + +fn dir(tag: &str) -> PathBuf { + let d = std::env::temp_dir().join(format!("plannotator-gate-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(&d).expect("temp dir"); + d +} + +#[test] +fn the_strict_record_is_the_browsers_byte_for_byte() { + assert_eq!(approved("", 0).strict_json(), r#"{"decision":"approved"}"#); + assert_eq!( + approved("# Annotations on plan.md\n", 1).strict_json(), + r##"{"decision":"approved","feedback":"# Annotations on plan.md\n"}"## + ); + assert_eq!(annotated("notes", 2).strict_json(), r#"{"decision":"annotated","feedback":"notes"}"#); + assert_eq!(annotated("", 0).strict_json(), r#"{"decision":"annotated","feedback":""}"#); + assert_eq!(Decision::Dismissed.strict_json(), r#"{"decision":"dismissed"}"#); +} + +#[test] +fn plain_json_adds_the_count_and_target_the_browser_adds() { + let target = Path::new("/work/plan.md"); + assert_eq!( + approved("", 0).json(target), + r#"{"decision":"approved","annotationCount":0,"target":"/work/plan.md"}"# + ); + assert_eq!( + approved("n", 1).json(target), + r#"{"decision":"approved","feedback":"n","annotationCount":1,"target":"/work/plan.md"}"# + ); + assert_eq!( + annotated("n", 3).json(target), + r#"{"decision":"annotated","feedback":"n","annotationCount":3,"target":"/work/plan.md"}"# + ); + assert_eq!(Decision::Dismissed.json(target), r#"{"decision":"dismissed","target":"/work/plan.md"}"#); +} + +#[test] +fn plaintext_is_the_approval_marker_the_feedback_or_nothing() { + let context = "File: /work/plan.md"; + assert_eq!(approved("", 0).plain(context).as_deref(), Some("The user approved.")); + assert_eq!(annotated("# Annotations\n\n", 1).plain(context).as_deref(), Some("# Annotations")); + assert_eq!(annotated("", 0).plain(context), None); + assert_eq!(Decision::Dismissed.plain(context), None); +} + +#[test] +fn an_approval_with_notes_uses_the_browsers_framing_and_names_the_target() { + let message = approval_message("# Annotations on plan.md\n\n## Annotation 1\n\n", "File: /work/plan.md"); + assert_eq!( + message, + "# Approved with Notes\n\nThe artifact is approved. The notes below are non-blocking guidance, \ + not a request for another revision.\n\nFile: /work/plan.md\n\n# Annotations on plan.md\n\n\ + ## Annotation 1\n\nDo not revise or reopen the artifact solely because of these notes unless \ + the user explicitly requests it. Carry the notes into subsequent work where applicable." + ); + assert!(approval_message("n", "").contains("revision.\n\nn\n\nDo not"), "no context, no blank block"); + assert_eq!(approval_message(" \n", "File: x"), APPROVED); + assert_eq!(context_line(Path::new("/work/docs"), true), "Folder: /work/docs"); +} + +#[test] +fn only_an_approval_succeeds_under_require_approval() { + assert_eq!(approved("", 0).exit_code(true), 0); + assert_eq!(approved("n", 1).exit_code(true), 0); + assert_eq!(annotated("n", 1).exit_code(true), 1); + assert_eq!(Decision::Dismissed.exit_code(true), 1); + for decision in [approved("", 0), annotated("n", 1), Decision::Dismissed] { + assert_eq!(decision.exit_code(false), 0, "{decision:?}"); + } +} + +#[test] +fn the_archive_records_the_browsers_decision_words() { + assert_eq!(approved("", 0).archive_decision(), "approved"); + assert_eq!(approved("n", 1).archive_decision(), "approved-with-notes"); + assert_eq!(annotated("n", 1).archive_decision(), "feedback"); + assert_eq!(Decision::Dismissed.archive_decision(), "dismissed"); +} + +#[test] +fn flags_parse_in_any_order_around_the_path() { + let parsed = ReviewArgs::parse(&args("--gate plan.md --json --require-approval --result-file out.json")) + .expect("parses"); + assert_eq!( + parsed, + ReviewArgs { + path: PathBuf::from("plan.md"), + gate: true, + json: true, + require_approval: true, + result_file: Some(PathBuf::from("out.json")), + } + ); + assert!(parsed.strict()); + assert_eq!(parsed.mode(), Gate::Approve); + assert_eq!(ReviewArgs::parse(&args("plan.md --json")).expect("json alone").mode(), Gate::Decision); +} + +#[test] +fn strict_flags_need_gate_and_json() { + for bad in ["p.md --require-approval", "p.md --gate --require-approval", "p.md --json --result-file r"] { + let err = ReviewArgs::parse(&args(bad)).expect_err(bad); + assert!(err.to_string().contains("require --gate --json"), "{bad}: {err}"); + } +} + +#[test] +fn malformed_invocations_are_rejected_with_a_reason() { + let cases = [ + ("p.md --gate --gate", "only be specified once"), + ("p.md --gate --json --result-file", "missing value"), + ("p.md --gate --json --result-file --require-approval", "missing value"), + ("p.md --gate --json --result-file a --result-file b", "only be specified once"), + ("p.md --gat", "unknown flag --gat"), + ("p.md q.md --gate", "unexpected argument"), + ("--gate --json", "missing the file or folder"), + ]; + for (input, expected) in cases { + let err = ReviewArgs::parse(&args(input)).expect_err(input); + assert!(err.to_string().contains(expected), "{input}: {err}"); + } +} + +#[test] +fn a_strict_invocation_is_recognised_before_parsing() { + assert!(is_strict(&args("p.md --gat --require-approval"))); + assert!(is_strict(&args("--result-file r p.md"))); + assert!(!is_strict(&args("p.md --gate --json"))); +} + +#[test] +fn a_result_file_is_published_once_and_never_overwritten() { + let d = dir("publish"); + let path = d.join("decision.json"); + check_result_file(&path).expect("free name"); + publish(&path, r#"{"decision":"approved"}"#).expect("published"); + assert_eq!(std::fs::read_to_string(&path).expect("read"), "{\"decision\":\"approved\"}\n"); + + let err = check_result_file(&path).expect_err("taken"); + assert!(err.to_string().contains("already exists"), "{err}"); + publish(&path, r#"{"decision":"dismissed"}"#).expect_err("no clobber"); + assert_eq!(std::fs::read_to_string(&path).expect("read"), "{\"decision\":\"approved\"}\n"); + + let names: Vec<_> = std::fs::read_dir(&d).expect("list").map(|e| e.expect("entry").file_name()).collect(); + assert_eq!(names, vec![std::ffi::OsString::from("decision.json")], "no temporary file left behind"); + let _ = std::fs::remove_dir_all(d); +} + +#[test] +fn a_result_file_needs_an_existing_folder() { + let d = dir("parent"); + let err = check_result_file(&d.join("missing/decision.json")).expect_err("no folder"); + assert!(err.to_string().contains("folder does not exist"), "{err}"); + publish(&d.join("missing/decision.json"), "{}").expect_err("cannot publish"); + let _ = std::fs::remove_dir_all(d); +} + +#[cfg(unix)] +#[test] +fn a_dangling_link_counts_as_taken() { + let d = dir("dangling"); + let path = d.join("decision.json"); + std::os::unix::fs::symlink(d.join("nowhere"), &path).expect("symlink"); + check_result_file(&path).expect_err("a link occupies the name"); + let _ = std::fs::remove_dir_all(d); +} diff --git a/crates/plannotator-tui/src/herdr/context.rs b/crates/plannotator-tui/src/herdr/context.rs index cfd70a5..9ca7da7 100644 --- a/crates/plannotator-tui/src/herdr/context.rs +++ b/crates/plannotator-tui/src/herdr/context.rs @@ -61,6 +61,10 @@ pub(crate) struct HerdrEnv { pub(crate) terminal_pane: Option, /// `PLANNOTATOR_TUI_TERMINAL_LINES`: how many recent lines to read from that pane. pub(crate) terminal_lines: Option, + /// `PLANNOTATOR_TUI_GATE=1`: a gated review, from `herdr open --gate`. + pub(crate) gate: bool, + /// `PLANNOTATOR_TUI_RESULT_FILE`: where a gated review publishes its decision. + pub(crate) result_file: Option, } impl HerdrEnv { @@ -89,6 +93,8 @@ impl HerdrEnv { newest: env("PLANNOTATOR_TUI_NEWEST").as_deref() == Some("1"), terminal_pane: non_empty("PLANNOTATOR_TUI_TERMINAL_PANE"), terminal_lines: non_empty("PLANNOTATOR_TUI_TERMINAL_LINES").and_then(|v| v.parse().ok()), + gate: env("PLANNOTATOR_TUI_GATE").as_deref() == Some("1"), + result_file: non_empty("PLANNOTATOR_TUI_RESULT_FILE").map(PathBuf::from), } } @@ -252,4 +258,13 @@ mod tests { assert_eq!(env.terminal_pane.as_deref(), Some("w1:p2")); assert_eq!(env.terminal_lines, Some(80)); } + + #[test] + fn only_an_exact_gate_flag_gates_the_pane() { + let gated = env(&[("PLANNOTATOR_TUI_GATE", "1"), ("PLANNOTATOR_TUI_RESULT_FILE", "/tmp/d.json")]); + assert!(gated.gate); + assert_eq!(gated.result_file, Some(PathBuf::from("/tmp/d.json"))); + assert!(!env(&[("PLANNOTATOR_TUI_GATE", "true")]).gate); + assert!(!env(&[]).gate); + } } diff --git a/crates/plannotator-tui/src/herdr/launch.rs b/crates/plannotator-tui/src/herdr/launch.rs index be3975c..d2642be 100644 --- a/crates/plannotator-tui/src/herdr/launch.rs +++ b/crates/plannotator-tui/src/herdr/launch.rs @@ -19,6 +19,10 @@ pub(crate) struct OpenArgs { pub(crate) deliver_to: Option, /// `last` only: open the newest reply without offering the picker first. pub(crate) newest: bool, + /// `open` only: a gated review, with Approve beside Send (decision 17). + pub(crate) gate: bool, + /// `open --gate` only: where the pane publishes the decision; absolute. + pub(crate) result_file: Option, } /// A fully resolved launch. @@ -45,6 +49,9 @@ pub(crate) struct Launch { pub(crate) newest: bool, /// Open a pane's recent terminal output instead of `file`. pub(crate) terminal: Option, + /// A gated review of `file`, and where its decision is published. + pub(crate) gate: bool, + pub(crate) result_file: Option, } /// Which pane's output the opened pane reads, and how much of it. @@ -290,6 +297,8 @@ pub(crate) fn plan(env: &HerdrEnv, config: &Config, args: OpenArgs, cwd: &Path) session: None, newest: false, terminal: None, + gate: args.gate, + result_file: args.result_file, }) } @@ -341,6 +350,12 @@ pub(crate) fn argv(launch: &Launch) -> Vec { } (None, None) => { out.extend(["--env".to_owned(), format!("PLANNOTATOR_TUI_FILE={}", launch.file.display())]); + if launch.gate { + out.extend(["--env".to_owned(), "PLANNOTATOR_TUI_GATE=1".to_owned()]); + } + if let Some(file) = launch.result_file.as_ref().filter(|_| launch.gate) { + out.extend(["--env".to_owned(), format!("PLANNOTATOR_TUI_RESULT_FILE={}", file.display())]); + } } } if let Some(target) = &launch.deliver { diff --git a/crates/plannotator-tui/src/herdr/launch/tests.rs b/crates/plannotator-tui/src/herdr/launch/tests.rs index e862d93..6cee030 100644 --- a/crates/plannotator-tui/src/herdr/launch/tests.rs +++ b/crates/plannotator-tui/src/herdr/launch/tests.rs @@ -446,3 +446,27 @@ fn terminal_review_of_a_shell_splits_beside_it_and_delivers_nowhere() { assert_eq!(target.map(String::as_str), Some("w1:p4")); assert!(args.contains(&"PLANNOTATOR_TUI_TERMINAL_PANE=w1:p4".to_owned()), "{args:?}"); } + +#[test] +fn a_gated_open_tells_the_pane_to_gate_and_where_to_publish() { + let root = temp_folder("gate"); + let args = OpenArgs { + path: Some(PathBuf::from("docs/plan.md")), + gate: true, + result_file: Some(root.join("decision.json")), + ..OpenArgs::default() + }; + let launch = plan(&env(Some("w2:p4"), None), &Config::default(), args, &root).expect("plans"); + let gated = argv(&launch); + assert!(gated.windows(2).any(|pair| pair == ["--env", "PLANNOTATOR_TUI_GATE=1"])); + let published = format!("PLANNOTATOR_TUI_RESULT_FILE={}", root.join("decision.json").display()); + assert!(gated.windows(2).any(|pair| pair == ["--env", published.as_str()])); + + let args = OpenArgs { path: Some(PathBuf::from("docs/plan.md")), ..OpenArgs::default() }; + let launch = plan(&env(Some("w2:p4"), None), &Config::default(), args, &root).expect("plans"); + assert!( + !argv(&launch).iter().any(|a| a.contains("_GATE") || a.contains("RESULT_FILE")), + "ungated: unchanged" + ); + std::fs::remove_dir_all(&root).expect("cleanup"); +} diff --git a/crates/plannotator-tui/src/keys.rs b/crates/plannotator-tui/src/keys.rs index bb72bbe..a1bce00 100644 --- a/crates/plannotator-tui/src/keys.rs +++ b/crates/plannotator-tui/src/keys.rs @@ -34,6 +34,7 @@ pub(crate) enum Action { CycleFocus, Send, SendAndClose, + Approve, ToggleTree, Reload, OpenInEditor, @@ -61,8 +62,8 @@ pub(crate) enum Action { } use Action::{ - Archive, AttachImage, Back, Bottom, ClearAll, ClearBlock, Comment, Confirm, CycleFocus, Delete, Down, - Edit, FinishReview, Interrupt, Left, LineEnd, LineStart, LooksGood, MoveCursor, No, OpenInEditor, + Approve, Archive, AttachImage, Back, Bottom, ClearAll, ClearBlock, Comment, Confirm, CycleFocus, Delete, + Down, Edit, FinishReview, Interrupt, Left, LineEnd, LineStart, LooksGood, MoveCursor, No, OpenInEditor, PageDown, PageUp, PickMessage, QuickOpen, Quit, Reload, Remove, RemoveImage, ResendAll, ReviewMenu, Right, Send, SendAndClose, ToggleHidden, ToggleTree, Top, UndoFinish, Up, Visual, WordBack, WordForward, Yes, @@ -75,6 +76,7 @@ pub(crate) const GLOBAL: &[Action] = &[ CycleFocus, Send, SendAndClose, + Approve, ReviewMenu, ResendAll, FinishReview, @@ -257,6 +259,7 @@ const DEFAULTS: &[(Action, &[Key])] = &[ (CycleFocus, &[named(KeyCode::Tab)]), (Send, &[ch('E')]), (SendAndClose, &[ch('S')]), + (Approve, &[ch('A')]), (ToggleTree, &[ch('t')]), (Reload, &[ch('r')]), (OpenInEditor, &[ch('o')]), @@ -297,7 +300,15 @@ impl Default for Keymap { impl<'de> Deserialize<'de> for Keymap { fn deserialize>(deserializer: D) -> Result { let mut keymap = Self::default(); - keymap.0.extend(BTreeMap::>::deserialize(deserializer)?); + let entries = BTreeMap::>::deserialize(deserializer)?; + let approve_set = entries.contains_key(&Approve); + keymap.0.extend(entries); + // `approve` arrived after configs were written and is live only in a gated review. + // Its default key yields to a config that gives that key to another action, so a + // config that loaded before still loads. + if !approve_set && keymap.check().is_err() { + keymap.0.insert(Approve, Vec::new()); + } keymap.check().map_err(serde::de::Error::custom)?; Ok(keymap) } @@ -389,6 +400,7 @@ impl Action { Self::CycleFocus => "cycle-focus", Self::Send => "send", Self::SendAndClose => "send-and-close", + Self::Approve => "approve", Self::ToggleTree => "toggle-tree", Self::Reload => "reload", Self::OpenInEditor => "open-in-editor", diff --git a/crates/plannotator-tui/src/keys/tests.rs b/crates/plannotator-tui/src/keys/tests.rs index 195c6e2..8655541 100644 --- a/crates/plannotator-tui/src/keys/tests.rs +++ b/crates/plannotator-tui/src/keys/tests.rs @@ -114,3 +114,23 @@ fn an_unbound_action_drops_out_of_the_hints() { assert_eq!(keymap.labelled(Quit, "quit"), None); assert_eq!(keymap.labelled(Send, "send").as_deref(), Some("E send")); } + +#[test] +fn approve_is_shifted_a_and_distinct_from_looks_good() { + let keymap = Keymap::default(); + assert_eq!(keymap.action(&press(KeyCode::Char('A'), KeyModifiers::SHIFT), GLOBAL), Some(Approve)); + assert_eq!(keymap.action(&press(KeyCode::Char('a'), KeyModifiers::NONE), TOOLBAR), Some(LooksGood)); + assert_eq!(keymap.action(&press(KeyCode::Char('a'), KeyModifiers::NONE), GLOBAL), None); +} + +#[test] +fn a_config_that_gave_approves_key_away_still_loads() { + // Written before `approve` existed: `A` is the user's, and approve is left unbound. + let keymap = parse("comment = [\"A\"]\n").expect("an older config keeps loading"); + assert_eq!(keymap.action(&press(KeyCode::Char('A'), KeyModifiers::SHIFT), DOCUMENT), Some(Comment)); + assert_eq!(keymap.hint(Approve), ""); + // Naming approve opts into the check like any other action. + let err = parse("comment = [\"A\"]\napprove = [\"A\"]\n").expect_err("both live while browsing"); + assert!(err.message().contains("approve"), "{}", err.message()); + assert_eq!(parse("approve = [\"Y\"]\n").expect("rebinds").hint(Approve), "Y"); +} diff --git a/crates/plannotator-tui/src/main.rs b/crates/plannotator-tui/src/main.rs index 3584808..40a5f2b 100644 --- a/crates/plannotator-tui/src/main.rs +++ b/crates/plannotator-tui/src/main.rs @@ -10,6 +10,7 @@ mod doc; mod editor; mod export; mod file_search; +mod gate; mod herdr; mod keys; mod last; @@ -24,13 +25,18 @@ mod wrap; fn main() -> std::process::ExitCode { let args: Vec = std::env::args().skip(1).collect(); match cli::run(&args) { - Ok(()) => std::process::ExitCode::SUCCESS, + Ok(code) => code, Err(err) => { #[allow(clippy::print_stderr, reason = "the one place errors reach the user")] { eprintln!("plannotator_tui: {err:#}"); } - std::process::ExitCode::FAILURE + // A strict gate's own failure must not read as the reviewer's rejection. + if err.is::() { + std::process::ExitCode::from(gate::GATE_ERROR_EXIT) + } else { + std::process::ExitCode::FAILURE + } } } } diff --git a/crates/plannotator-tui/tests/gate.rs b/crates/plannotator-tui/tests/gate.rs new file mode 100644 index 0000000..34b6ecb --- /dev/null +++ b/crates/plannotator-tui/tests/gate.rs @@ -0,0 +1,90 @@ +//! Gate flags through the real binary: a strict gate's own failures exit 2, never 1, and +//! leave an existing result file alone. Every case fails before the review would draw, so +//! none needs a terminal; stdout is a pipe here, which a gated review refuses. + +#![allow(clippy::expect_used, reason = "tests assert by panicking")] + +use std::path::{Path, PathBuf}; +use std::process::Output; + +fn run(root: &Path, args: &[&str]) -> Output { + std::process::Command::new(env!("CARGO_BIN_EXE_plannotator-tui")) + .current_dir(root) + .env("PLANNOTATOR_DATA_DIR", root.join("data")) + .env_remove("HERDR_ENV") + .args(args) + .output() + .expect("runs") +} + +fn scratch(name: &str) -> PathBuf { + let root = std::env::temp_dir().join(format!("plannotator-tui-gate-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).expect("dirs"); + std::fs::write(root.join("plan.md"), "# Plan\n\nShip it.\n").expect("doc"); + root +} + +fn stderr(out: &Output) -> String { + String::from_utf8_lossy(&out.stderr).into_owned() +} + +#[test] +fn strict_failures_exit_2_and_ordinary_ones_exit_1() { + let root = scratch("codes"); + let cases: [(&[&str], i32, &str); 8] = [ + (&["plan.md", "--require-approval"], 2, "require --gate --json"), + (&["plan.md", "--gate", "--result-file", "r.json"], 2, "require --gate --json"), + (&["plan.md", "--gat", "--require-approval"], 2, "unknown flag --gat"), + (&["missing.md", "--gate", "--json", "--require-approval"], 2, "no such file or folder"), + (&["missing.md", "--gate", "--json"], 1, "no such file or folder"), + (&["plan.md", "--gate", "--json", "--require-approval"], 2, "stdout is not a terminal"), + (&["plan.md", "--gate"], 1, "stdout is not a terminal"), + (&["plan.md", "--gate", "--json", "--result-file", "nowhere/r.json"], 2, "folder does not exist"), + ]; + for (args, code, message) in cases { + let out = run(&root, args); + assert_eq!(out.status.code(), Some(code), "{args:?}: {}", stderr(&out)); + assert!(stderr(&out).contains(message), "{args:?}: {}", stderr(&out)); + assert!(out.stdout.is_empty(), "{args:?}: no decision was made"); + } + std::fs::remove_dir_all(&root).expect("cleanup"); +} + +#[test] +fn an_existing_result_file_is_refused_and_left_as_it_was() { + let root = scratch("existing"); + std::fs::write(root.join("r.json"), "keep me\n").expect("existing"); + let out = run(&root, &["plan.md", "--gate", "--json", "--result-file", "r.json"]); + assert_eq!(out.status.code(), Some(2), "{}", stderr(&out)); + assert!(stderr(&out).contains("already exists"), "{}", stderr(&out)); + assert_eq!(std::fs::read_to_string(root.join("r.json")).expect("read"), "keep me\n"); + std::fs::remove_dir_all(&root).expect("cleanup"); +} + +#[test] +fn herdr_gate_flags_belong_to_open() { + let root = scratch("herdr"); + let out = run(&root, &["herdr", "open", "plan.md", "--result-file", "r.json"]); + assert_eq!(out.status.code(), Some(2), "{}", stderr(&out)); + assert!(stderr(&out).contains("--result-file requires --gate"), "{}", stderr(&out)); + let out = run(&root, &["herdr", "last", "--gate"]); + assert_eq!(out.status.code(), Some(1), "{}", stderr(&out)); + assert!(stderr(&out).contains("only for `herdr open`"), "{}", stderr(&out)); + std::fs::write(root.join("r.json"), "").expect("existing"); + let out = run(&root, &["herdr", "open", "plan.md", "--gate", "--result-file", "r.json"]); + assert_eq!(out.status.code(), Some(2), "{}", stderr(&out)); + assert!(stderr(&out).contains("already exists"), "{}", stderr(&out)); + std::fs::remove_dir_all(&root).expect("cleanup"); +} + +#[test] +fn headless_commands_ignore_what_looks_like_a_gate_flag() { + // `--json` as an annotation body is text, not a flag: tools keep their own arguments. + let root = scratch("headless"); + let out = run(&root, &["--annotate", "plan.md", "Ship it.", "--json"]); + assert!(out.status.success(), "{}", stderr(&out)); + let out = run(&root, &["--export", "plan.md"]); + assert!(String::from_utf8_lossy(&out.stdout).contains("> --json"), "{}", stderr(&out)); + std::fs::remove_dir_all(&root).expect("cleanup"); +} diff --git a/docs/decisions.md b/docs/decisions.md index b52732c..43bc638 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -426,3 +426,50 @@ omitted when empty, so a note without images keeps the server shape and older re unchanged. Feedback writes one `Image: ` line per image: the whole line is the path, so spaces need no escaping, and agents open a path more readily than a `file://` URL. Nothing is copied or uploaded; uploading belongs to a future Workspaces sync. + +## 17. Gate mode: an explicit Approve and one decision per review (2026-10-28) + +Rebuilt from plannotator-tui#121. Quitting with no notes was ambiguous: a wrapper could not +tell "approved" from "closed", and had to guess. `--gate` adds an Approve action (`A`, and a +header button on the edge), separate from Send and quit, and makes the review end with one +decision: `approved` (Approve; unsent notes ride along as approval notes), `annotated` (a +successful Send) or `dismissed` (quit). Decision 2 still holds: approval is not a kind of +annotation, it is this per-review object. + +The output contract is Plannotator's `annotate --gate`, read from +`apps/hook/server/strict-annotate-result.ts`, `annotate-output.ts`, `host-result.ts` and +`packages/shared/prompts.ts`, `feedback-archive.ts`, `packages/server/annotate.ts`: + +- Plaintext (`--gate`): `The user approved.`, or the `approvedWithNotes` prompt with a + `File:`/`Folder:` line when notes ride along; the feedback on a send; nothing on quit. +- `--json`: one line, `{"decision":…,"feedback":…}`, plus `annotationCount` and `target` + as the browser adds them. `--json` alone decides without an Approve, as in the browser. +- `--require-approval` exits 0 only on approval and 1 otherwise. `--result-file` publishes + the strict record (decision and feedback only) atomically: a `0600` temporary file beside + it, then a hard link, which fails rather than overwrite. Both need `--gate --json`, and + under either one every failure of the gate itself exits 2, so a typo never reads as a + rejection. Stdout is written before the file, as in the browser. +- The feedback archive gets the browser's decision words: `approved`, + `approved-with-notes`, `feedback`, `dismissed`. A dismissal is archived only in a gated + review; an ordinary quit still records nothing. + +Where the TUI differs, and why: + +- A send ends a gated review. An ordinary review stays open across sends; a gate cannot, + because a script waits for exactly one answer. +- The review draws on stdout, so a gated review refuses to start when stdout is not a + terminal (`$(…)` capture would hide it and wait forever). Scripts read `--result-file`. +- With no agent pane the target is the caller (`Send 2 new ▸ caller`): sending copies + nothing to the clipboard, because the decision is the output. +- Plannotator's prompts can be overridden in its `config.json`; this client always uses the + default wording. There is no free-text "Approve with a note…": a note on a block is the + same thing here. The content digest in #121's last point is not built. + +Inside Herdr, `herdr open --gate [--result-file PATH]` passes `PLANNOTATOR_TUI_GATE=1` and +`PLANNOTATOR_TUI_RESULT_FILE` to the pane, which delivers the approval message (or the +numbered feedback, as an ordinary send would) to the agent and publishes the result file. +The launcher returns when the pane opens, so it has no exit-code contract; it checks the +result path first so a taken name fails before anyone reviews. + +`approve` is a new rebindable action. Its default key yields when a config already gives +`A` to an action that is live at the same time, so a config that loaded before still loads. diff --git a/docs/spec-herdr-integration.md b/docs/spec-herdr-integration.md index 64087ee..adb0913 100644 --- a/docs/spec-herdr-integration.md +++ b/docs/spec-herdr-integration.md @@ -243,6 +243,8 @@ PLANNOTATOR_TUI_FILE file or folder to open (launcher → pane) PLANNOTATOR_TUI_DELIVER_TO pane id feedback goes to (launcher → pane) PLANNOTATOR_TUI_DELIVER_AGENT agent name for the label, e.g. "claude" (launcher → pane) PLANNOTATOR_TUI_PLACEMENT overlay | split | popup; beats the config file +PLANNOTATOR_TUI_GATE 1: a gated review with Approve (launcher → pane; decision 17) +PLANNOTATOR_TUI_RESULT_FILE where a gated review publishes its decision (launcher → pane) PLANNOTATOR_TUI_CONFIG path of the config file; beats XDG HERDR_* as above; HERDR_PANE_ID inside the pane is plannotator-tui's OWN pane ``` @@ -265,7 +267,7 @@ popup_width = "90%" # popup only; cells or percent popup_height = "85%" ``` -### The launcher: `plannotator-tui herdr open [PATH] [--placement P] [--deliver-to PANE]` +### The launcher: `plannotator-tui herdr open [PATH] [--placement P] [--deliver-to PANE] [--gate] [--result-file PATH]` One command for humans (via the manifest actions) and agents (via the skill). It reads the env above, resolves, and execs `herdr plugin pane open`. Resolution, in order: @@ -279,6 +281,15 @@ the env above, resolves, and execs `herdr plugin pane open`. Resolution, in orde It passes `PLANNOTATOR_TUI_FILE`, `PLANNOTATOR_TUI_DELIVER_TO`, `PLANNOTATOR_TUI_DELIVER_AGENT` explicitly so the pane never has to guess. `argv` construction is a pure, tested function. +`--gate` adds `PLANNOTATOR_TUI_GATE=1`: the pane shows Approve beside Send, and the review ends +with one decision (decision 17). Approve delivers `The user approved.`, or Plannotator's +approved-with-notes message when unsent notes ride along; Send delivers the numbered feedback +as it always does; quitting delivers nothing. `--result-file PATH` (needs `--gate`) is resolved +against the launcher's directory, checked before the pane opens (the folder must exist, the +file must not), and passed as `PLANNOTATOR_TUI_RESULT_FILE`; the pane publishes +`{"decision":…,"feedback":…}` there atomically when the review ends. The launcher returns +once the pane is open, so a script waits for the file, not for the exit code. + ### `plannotator-tui herdr terminal [--lines N] [--placement P] [--deliver-to PANE] [--print]` Reviews a pane's recent output. The pane is the context's `focused_pane_id` (from an action), diff --git a/skills/plannotator-tui/SKILL.md b/skills/plannotator-tui/SKILL.md index 4d52ae4..917f359 100644 --- a/skills/plannotator-tui/SKILL.md +++ b/skills/plannotator-tui/SKILL.md @@ -30,6 +30,16 @@ file is and ask them to review it. Address every item, then continue. + When you need an explicit sign-off before you act, open it with `--gate`: + + ```bash + plannotator-tui herdr open docs/plans/auth.md --gate + ``` + + The human can then approve. An approval arrives as `The user approved.`, or as a message + headed `# Approved with Notes` whose notes are guidance, not a request to revise. Numbered + feedback still means revise; no message means they closed it without deciding. + When you list files you want the human to open, print them as `file://` hyperlinks (OSC 8) so Ctrl-click in Herdr opens them in plannotator-tui: