feat: replace the [ok] summary line with a ✓/✗ echo marker

An audit of the command surface found the `[ok] <verb> <subject>`
summary line duplicated the echo line above it everywhere; its only
unique signal was success-vs-error. Retire it: a command's echo line
now resolves from `running: <input>` to `<input> ✓` / `<input> ✗`
on completion, and the symmetric `"<verb> <subject>" failed:` prefix
is dropped (only the reason remains). Content lines (row counts,
structure, plan tree, teaching echo) are unchanged.

Echo lines carry an EchoStatus; executed commands push Pending and
resolve the oldest-pending echo on their result event (FIFO worker —
correct under interleaving). Parse-time and pre-flight rejections are
not executed and keep their running: + caret rendering. App-command
[ok] lines (rebuild/export/replay) are payload-bearing and untouched.
ADR-0040.
This commit is contained in:
claude@clouddev1
2026-05-30 21:38:48 +00:00
parent f62cccec55
commit 8311de44a8
9 changed files with 546 additions and 124 deletions
+235 -77
View File
@@ -9,7 +9,7 @@
use std::collections::VecDeque;
use crossterm::event::{KeyCode, KeyEvent, KeyEventKind, KeyModifiers};
use tracing::{trace, warn};
use tracing::{debug, trace, warn};
use crate::action::Action;
use crate::db::{
@@ -37,6 +37,21 @@ pub enum OutputKind {
TeachingEcho,
}
/// Completion state of an `OutputKind::Echo` line (ADR-0040).
///
/// An echo for an *executed* command is pushed `Pending` (rendered
/// `running: <input>`) and resolves to `Ok`/`Err` when the result
/// arrives — rendered `<input> ✓` / `<input> ✗`, replacing the old
/// `[ok]`/`failed:` summary line. Parse-time and pre-flight
/// rejections are not executed and carry `None` (they keep the
/// `running:` + caret rendering); non-echo lines also carry `None`.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum EchoStatus {
Pending,
Ok,
Err,
}
/// The semantic style class of an [`OutputSpan`] (ADR-0028 §5).
///
/// A general output-styling vocabulary, resolved to a concrete
@@ -82,6 +97,10 @@ pub struct OutputLine {
/// these runs; when `None` it falls back to whole-line
/// styling by `kind`.
pub styled_runs: Option<Vec<OutputSpan>>,
/// Echo completion state (ADR-0040). `Some(_)` only on
/// `OutputKind::Echo` lines for executed commands; `None`
/// everywhere else (non-echo lines, parse/pre-flight echoes).
pub status: Option<EchoStatus>,
}
impl OutputLine {
@@ -99,6 +118,20 @@ impl OutputLine {
kind,
mode_at_submission,
styled_runs: Some(runs),
status: None,
}
}
/// A `running: <input>` echo for an executed command, pushed
/// `Pending` and resolved to `Ok`/`Err` on completion (ADR-0040).
#[must_use]
pub fn echo(input: &str, mode: Mode) -> Self {
Self {
text: crate::t!("dsl.running", input = input),
kind: OutputKind::Echo,
mode_at_submission: mode,
styled_runs: None,
status: Some(EchoStatus::Pending),
}
}
}
@@ -470,8 +503,9 @@ impl App {
description,
} => {
// No-op (CREATE TABLE IF NOT EXISTS on an existing
// table, ADR-0035 §4): the skip note, then the existing
// structure — no misleading "[ok] create table" line.
// table, ADR-0035 §4): a successful no-op — mark the
// echo ✓ (ADR-0040), then the skip note + structure.
self.mark_oldest_pending_echo(EchoStatus::Ok);
self.note_system(crate::t!(
"ddl.create_skipped_exists",
name = command.target_table()
@@ -484,8 +518,8 @@ impl App {
}
AppEvent::DslDropSkipped { command } => {
// No-op (DROP TABLE IF EXISTS on an absent table,
// ADR-0035 §4, 4c): just the skip note — no structure,
// no misleading "[ok] drop table" line.
// ADR-0035 §4, 4c): successful no-op — echo ✓ + skip note.
self.mark_oldest_pending_echo(EchoStatus::Ok);
self.note_system(crate::t!(
"ddl.drop_skipped_absent",
name = command.target_table()
@@ -494,8 +528,9 @@ impl App {
}
AppEvent::DslDropIndexSkipped { command } => {
// No-op (DROP INDEX IF EXISTS on an absent index,
// ADR-0035 §4d): just the skip note. `target_table()`
// returns the index name for `SqlDropIndex`.
// ADR-0035 §4d): successful no-op — echo ✓ + skip note.
// `target_table()` returns the index name for `SqlDropIndex`.
self.mark_oldest_pending_echo(EchoStatus::Ok);
self.note_system(crate::t!(
"ddl.drop_index_skipped_absent",
name = command.target_table()
@@ -504,9 +539,10 @@ impl App {
}
AppEvent::DslCreateIndexSkipped { command: _, name } => {
// No-op (CREATE INDEX IF NOT EXISTS on an existing index
// name, ADR-0035 §4d): the skip note carries the resolved
// index name (the unnamed form's auto-name isn't on the
// command). No structure shown.
// name, ADR-0035 §4d): successful no-op — echo ✓ + skip
// note (the resolved index name; unnamed form's auto-name
// isn't on the command).
self.mark_oldest_pending_echo(EchoStatus::Ok);
self.note_system(crate::t!("ddl.create_index_skipped_exists", name = name));
Vec::new()
}
@@ -608,6 +644,10 @@ impl App {
path,
message,
} => {
// ADR-0040: if a command's persistence failed fatally,
// resolve its (pending) echo to ✗ before the quit banner,
// so the dying session doesn't leave a `running:` line.
self.mark_oldest_pending_echo(EchoStatus::Err);
let banner = crate::t!(
"fatal.persistence",
operation = operation,
@@ -728,6 +768,10 @@ impl App {
count,
warnings,
} => {
// ADR-0040: the `replay` echo resolves ✓; the
// `[ok] replay — N command(s)` summary is payload-bearing
// (the count) and stays.
self.mark_oldest_pending_echo(EchoStatus::Ok);
self.note_system(crate::t!(
"replay.completed",
path = path,
@@ -747,6 +791,8 @@ impl App {
command,
error,
} => {
// ADR-0040: the `replay` echo resolves ✗.
self.mark_oldest_pending_echo(EchoStatus::Err);
// line_number == 0 is the runtime's signal that
// file-open itself failed (no per-line context to
// surface). Otherwise we lead with the line-number
@@ -1331,12 +1377,7 @@ impl App {
// hand it off as a dedicated `Action::Replay`,
// keeping the worker out of the loop and the
// history.log clean.
self.push_output(OutputLine {
text: crate::t!("dsl.running", input = input),
kind: OutputKind::Echo,
mode_at_submission: mode,
styled_runs: None,
});
self.push_output(OutputLine::echo(input, mode));
vec![Action::Replay { path }]
}
Ok(cmd) => {
@@ -1350,11 +1391,14 @@ impl App {
&cmd,
&self.schema_cache,
) {
// Pre-flight rejection (not executed): plain
// `running:` echo, `status: None` (ADR-0040 scope).
self.push_output(OutputLine {
text: crate::t!("dsl.running", input = input),
kind: OutputKind::Echo,
mode_at_submission: mode,
styled_runs: None,
status: None,
});
self.note_error(note);
return vec![Action::JournalFailure {
@@ -1371,11 +1415,14 @@ impl App {
&cmd,
&self.schema_cache,
) {
// Pre-flight rejection (not executed): plain
// `running:` echo, `status: None` (ADR-0040 scope).
self.push_output(OutputLine {
text: crate::t!("dsl.running", input = input),
kind: OutputKind::Echo,
mode_at_submission: mode,
styled_runs: None,
status: None,
});
for note in notes {
self.note_error(note);
@@ -1385,12 +1432,7 @@ impl App {
source: input.to_string(),
}];
}
self.push_output(OutputLine {
text: crate::t!("dsl.running", input = input),
kind: OutputKind::Echo,
mode_at_submission: mode,
styled_runs: None,
});
self.push_output(OutputLine::echo(input, mode));
vec![Action::ExecuteDsl {
command: cmd,
source: input.to_string(),
@@ -1401,11 +1443,15 @@ impl App {
Err(err) => {
// Echo the source line so the user can see what
// got submitted (and copy-paste it back to fix).
// Parse error (not executed): plain `running:` echo,
// `status: None` — the caret aligns to `running: `
// (ADR-0040 scope).
self.push_output(OutputLine {
text: crate::t!("dsl.running", input = input),
kind: OutputKind::Echo,
mode_at_submission: mode,
styled_runs: None,
status: None,
});
// Caret pointer at the failure position, when we
// have one. Aligned to the "running: " prefix so
@@ -1478,19 +1524,32 @@ impl App {
/// Emit the standard `[ok] <verb> <subject>` header used by
/// every successful DSL command. Routes through the i18n
/// catalog (ADR-0019 §9 sweep).
/// Resolve the oldest still-`Pending` echo to `status`
/// (ADR-0040). Results arrive in submission order (the db worker
/// is FIFO), so the oldest pending echo is this command's; a
/// finished command can never leave an earlier one stuck on
/// `running:`. No-op if there is no pending echo (e.g. a result
/// for a command whose echo path didn't mark one).
fn mark_oldest_pending_echo(&mut self, status: EchoStatus) {
if let Some(line) = self
.output
.iter_mut()
.find(|l| l.kind == OutputKind::Echo && l.status == Some(EchoStatus::Pending))
{
line.status = Some(status);
}
}
/// Mark a command's echo successful (ADR-0040 — replaces the old
/// `[ok] <verb> <subject>` summary line) and emit the ADR-0038
/// DSL → SQL teaching echo that was stashed on the success event.
fn note_ok_summary(&mut self, command: &Command) {
self.note_system(crate::t!(
"ok.summary",
verb = command.verb(),
subject = command.display_subject()
));
// ADR-0038 §4: the DSL → SQL teaching echo, beneath `[ok]`.
// Set on the success event when a DSL-form command ran in an
// advanced effective mode (ADR-0037); `None` otherwise. One
// `OutputKind::TeachingEcho` line per statement (§6 category
// 2): the dimmed `Executing SQL:` prefix + the SQL portion
// re-lexed in advanced mode for syntax highlighting — see
// `ui::render_output_line`'s `TeachingEcho` branch.
debug!(verb = command.verb(), "dsl command succeeded");
self.mark_oldest_pending_echo(EchoStatus::Ok);
// ADR-0038 §4: one `OutputKind::TeachingEcho` line per
// statement — the dimmed `Executing SQL:` prefix + the SQL
// re-lexed in advanced mode for highlighting (see
// `ui::render_output_line`'s `TeachingEcho` branch).
if let Some(lines) = self.pending_echo.take() {
for line in lines {
self.push_teaching_echo(&line);
@@ -1688,18 +1747,13 @@ impl App {
error = %rendered,
"dsl command failed"
);
// Wrap the command portion in quotes so the message
// reads cleanly: "...failed: <reason>" rather than the
// command running into "failed: ..." with no break.
// `note_error` splits on newlines internally — refusal
// diagnostics from `change column …` (ADR-0017 §7) flow
// through as a multi-line bordered table.
self.note_error(crate::t!(
"dsl.failed",
verb = command.verb(),
subject = command.display_subject(),
rendered = rendered
));
// ADR-0040: the echo line carries the ✗; the redundant
// `"<verb> <subject>" failed:` prefix is dropped — only the
// rendered reason is shown. `note_error` splits on newlines
// internally — refusal diagnostics from `change column …`
// (ADR-0017 §7) flow through as a multi-line bordered table.
self.mark_oldest_pending_echo(EchoStatus::Err);
self.note_error(crate::t!("dsl.failed", rendered = rendered));
}
/// Construct a [`TranslateContext`] from a [`Command`] + schema-
@@ -2305,6 +2359,7 @@ impl App {
kind: OutputKind::TeachingEcho,
mode_at_submission: self.mode,
styled_runs: None,
status: None,
});
}
@@ -2348,6 +2403,7 @@ impl App {
kind,
mode_at_submission: self.mode,
styled_runs: None,
status: None,
});
return;
}
@@ -2357,6 +2413,7 @@ impl App {
kind,
mode_at_submission: self.mode,
styled_runs: None,
status: None,
});
}
}
@@ -3344,12 +3401,17 @@ mod tests {
echo: Some(vec!["CREATE TABLE Other (id serial PRIMARY KEY)".to_string()]),
});
let texts: Vec<&str> = app.output.iter().map(|l| l.text.as_str()).collect();
let ok_idx = texts.iter().position(|t| t.starts_with("[ok]")).expect("an [ok] line");
// ADR-0040: no `[ok]` summary; with no preceding `running:` echo
// (event fired directly), the teaching echo leads.
assert!(
!texts.iter().any(|t| t.starts_with("[ok]")),
"no [ok] summary line (ADR-0040): {texts:?}",
);
let echo_idx = texts
.iter()
.position(|t| t.contains("Executing SQL:"))
.expect("an echo line");
assert_eq!(echo_idx, ok_idx + 1, "echo sits immediately beneath [ok]: {texts:?}");
assert_eq!(echo_idx, 0, "teaching echo leads the output: {texts:?}");
assert!(texts[echo_idx].contains("CREATE TABLE Other (id serial PRIMARY KEY)"));
// No echo line when the event carries none (simple mode etc.).
@@ -3382,15 +3444,19 @@ mod tests {
fn assert_echo_beneath_ok(app: &App, expected: &str) {
let texts: Vec<&str> = app.output.iter().map(|l| l.text.as_str()).collect();
let ok_idx = texts
.iter()
.position(|t| t.starts_with("[ok]"))
.expect("an [ok] line");
// ADR-0040: no `[ok]` summary line. These events are fired
// without a preceding `running:` echo (they bypass
// `dispatch_dsl`), so the teaching echo — pushed first by
// `note_ok_summary` — leads the output.
assert!(
!texts.iter().any(|t| t.starts_with("[ok]")),
"no [ok] summary line (ADR-0040): {texts:?}",
);
let echo_idx = texts
.iter()
.position(|t| t.contains("Executing SQL:"))
.expect("an echo line");
assert_eq!(echo_idx, ok_idx + 1, "echo sits immediately beneath [ok]: {texts:?}");
assert_eq!(echo_idx, 0, "teaching echo leads the output: {texts:?}");
assert!(texts[echo_idx].contains(expected), "echo carries the SQL: {texts:?}");
// ADR-0038 §4 polish: every success arm now wires the echo as
// `OutputKind::TeachingEcho` so `ui::render_output_line` fires
@@ -3764,34 +3830,35 @@ mod tests {
]),
});
let texts: Vec<&str> = app.output.iter().map(|l| l.text.as_str()).collect();
let ok_idx = texts
.iter()
.position(|t| t.starts_with("[ok]"))
.expect("an [ok] line");
// The three echo lines sit immediately beneath [ok], in order.
// ADR-0040: no `[ok]` summary; the event is fired without a
// preceding `running:` echo, so the three teaching echoes lead
// the output at indices 0/1/2, in order.
assert!(
texts[ok_idx + 1].contains("Executing SQL: DROP INDEX Customers_Email_idx"),
!texts.iter().any(|t| t.starts_with("[ok]")),
"no [ok] summary line (ADR-0040): {texts:?}",
);
assert!(
texts[0].contains("Executing SQL: DROP INDEX Customers_Email_idx"),
"first echo line: {texts:?}",
);
assert!(
texts[ok_idx + 2].contains("Executing SQL: DROP INDEX Customers_Email_Day_idx"),
texts[1].contains("Executing SQL: DROP INDEX Customers_Email_Day_idx"),
"second echo line: {texts:?}",
);
// ADR-0038 §4 polish: every one of the multi-statement echo lines
// carries TeachingEcho — the polish styling fires per line. A
// regression that pushed the multi-line case as System would
// leave the text intact but break the per-line styling.
for (offset, label) in [(1, "first"), (2, "second"), (3, "third")] {
for (idx, label) in [(0, "first"), (1, "second"), (2, "third")] {
assert_eq!(
app.output[ok_idx + offset].kind,
app.output[idx].kind,
OutputKind::TeachingEcho,
"{label} echo line carries TeachingEcho: {:?}",
app.output[ok_idx + offset],
app.output[idx],
);
}
assert!(
texts[ok_idx + 3]
.contains("Executing SQL: ALTER TABLE Customers DROP COLUMN Email"),
texts[2].contains("Executing SQL: ALTER TABLE Customers DROP COLUMN Email"),
"third echo line: {texts:?}",
);
// Pin the `Executing SQL:` prefix repeats once per statement
@@ -3981,8 +4048,13 @@ mod tests {
app.output.iter().any(|l| l.text.contains("id")),
"expected `id` somewhere in structure output",
);
// Earlier line is the [ok] header.
assert!(app.output.iter().any(|l| l.text.starts_with("[ok]")));
// ADR-0040: no `[ok]` summary line — success is signalled by
// the echo's ✓ marker (no echo pushed in this direct-event
// test) and the structure render itself.
assert!(
!app.output.iter().any(|l| l.text.starts_with("[ok]")),
"no [ok] summary line (ADR-0040)",
);
}
#[test]
@@ -4007,11 +4079,12 @@ mod tests {
command: cmd,
plan,
});
// `[ok] explain Customers` header.
// ADR-0040: no `[ok] explain …` header — the (no-echo here)
// command's success shows via the marker; the plan output
// itself carries the content.
assert!(
app.output.iter().any(|l| l.text.starts_with("[ok]")
&& l.text.contains("explain")),
"expected an [ok] explain header",
!app.output.iter().any(|l| l.text.starts_with("[ok]")),
"no [ok] summary line (ADR-0040)",
);
// The display SQL and the plan node both reach output.
assert!(
@@ -4026,6 +4099,79 @@ mod tests {
);
}
// ---- ADR-0040: completion marker on the echo line ----------
#[test]
fn mark_oldest_pending_echo_resolves_in_submission_order() {
// Two echoes in flight; results arrive in submission order, so
// the oldest pending echo is the correct target each time — a
// finished command never leaves an earlier one stuck Pending.
let mut app = App::new();
app.output
.push_back(OutputLine::echo("first", crate::mode::Mode::Advanced));
app.output
.push_back(OutputLine::echo("second", crate::mode::Mode::Advanced));
app.mark_oldest_pending_echo(EchoStatus::Ok);
app.mark_oldest_pending_echo(EchoStatus::Err);
let echoes: Vec<_> = app
.output
.iter()
.filter(|l| l.kind == OutputKind::Echo)
.collect();
assert_eq!(echoes[0].status, Some(EchoStatus::Ok), "first → Ok");
assert_eq!(echoes[1].status, Some(EchoStatus::Err), "second → Err");
}
#[test]
fn successful_command_resolves_its_echo_to_ok_with_no_summary_line() {
// Full flow: dispatch pushes a Pending echo; the success event
// resolves it to ✓ and emits no `[ok]` summary line (ADR-0040).
let mut app = App::new();
type_str(&mut app, "create table T with pk");
submit(&mut app);
let echo = app
.output
.iter()
.find(|l| l.kind == OutputKind::Echo)
.expect("dispatch pushed an echo");
assert_eq!(echo.status, Some(EchoStatus::Pending), "pending before result");
app.update(AppEvent::DslSucceeded {
command: Command::CreateTable {
name: "T".to_string(),
columns: vec![crate::dsl::ColumnSpec::new("id", Type::Serial)],
primary_key: vec!["id".to_string()],
},
description: None,
echo: None,
});
let echo = app
.output
.iter()
.find(|l| l.kind == OutputKind::Echo)
.expect("echo still present");
assert_eq!(echo.status, Some(EchoStatus::Ok), "resolved to Ok");
assert!(
!app.output.iter().any(|l| l.text.starts_with("[ok]")),
"no [ok] summary line",
);
}
#[test]
fn parse_error_echo_stays_pending_and_keeps_running_prefix() {
// ADR-0040 scope: a parse error never reaches the worker, so
// its echo is not marker-tracked (status None) and keeps the
// `running:` rendering + caret.
let mut app = App::new();
type_str(&mut app, "frobnicate widgets");
submit(&mut app);
let echo = app
.output
.iter()
.find(|l| l.kind == OutputKind::Echo)
.expect("parse error still echoes the input");
assert_eq!(echo.status, None, "parse-error echo is not marker-tracked");
}
#[test]
fn replay_command_dispatches_replay_action_not_execute_dsl() {
// Submitting `replay <path>` must NOT produce an
@@ -4951,12 +5097,17 @@ mod tests {
// ADR-0033 sub-phase 3f: a SQL DELETE reuses the DSL delete
// renderer (CommandOutcome::Delete -> handle_dsl_delete_
// success). This pins that the SHARED renderer produces the
// right user-facing strings for the SQL path — the ok-summary
// (verb + subject, where SqlDelete's subject is its target
// table) and the per-relationship cascade line. The integration
// tests check the DeleteResult struct; this checks the render.
// right user-facing strings for the SQL path — the row count
// and the per-relationship cascade line. ADR-0040: success is
// signalled by the echo's ✓ marker (no more `[ok]` summary), so
// we push the `running:` echo first (as `dispatch_dsl` does)
// and assert it resolves to `Ok`.
use crate::dsl::ReferentialAction;
let mut app = App::new();
app.output.push_back(OutputLine::echo(
"delete from Customers where id = 1",
crate::mode::Mode::Advanced,
));
app.update(AppEvent::DslDeleteSucceeded {
command: Command::SqlDelete {
sql: "delete from Customers where id = 1".to_string(),
@@ -4981,9 +5132,16 @@ mod tests {
echo: None,
});
let texts: Vec<String> = app.output.iter().map(|l| l.text.clone()).collect();
// ADR-0040: the echo resolves to ✓ (Ok); no `[ok]` summary line.
assert!(
texts.iter().any(|t| t.contains("delete from") && t.contains("Customers")),
"ok summary names the verb + target table: {texts:?}",
app.output
.iter()
.any(|l| l.kind == OutputKind::Echo && l.status == Some(EchoStatus::Ok)),
"the echo line resolves to Ok: {texts:?}",
);
assert!(
!texts.iter().any(|t| t.starts_with("[ok]")),
"no [ok] summary line (ADR-0040): {texts:?}",
);
assert!(
texts.iter().any(|t| t.contains("1 row(s) deleted")),