SemianiakaVY/openpencil
1
0
Fork 0
Code Issues Pull requests Actions 1 Packages Projects Releases Wiki Activity

feat(agent): itemize every quality repair into an expandable ledger

Browse source 
'41 auto-repair(s) applied' told the user nothing about what changed.
A counting-sink decorator now captures each accepted edit as a record
(pass group, node, field-level before/after — removed attributes read
as '(unset)', not a dash) at the one point every command crosses, so
counts derive from records and cannot drift. The polish step expands
into one row per record on desktop and as sub-rows on the text
streams, capped inline with the remainder in the INFO log, and scope
notes (skipped tiers) always precede the repairs that did run.

Claude-Session: https://claude.ai/code/session_01FqKQqNj8exYwopGDpYUU7x
This commit is contained in:
Fini 2026-08-09 01:18:38 +08:00
parent 6c05d0a7a8
commit f0e3ef48c0
12 changed files with 1815 additions and 60 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
crates/op-editor-ui/src/widgets/ai_chat_transcript_steps.rs
View file

@ -31,6 +31,11 @@ pub(crate) fn activity_step(activity: &ChatActivity) -> ParsedStep {
ParsedStep {
title: activity.title.clone(),
status: Some(status),
// One entry, newlines intact. A row's detail may be an itemized list
// — the quality passes write one line per applied repair — and the
// layout step splits it: `wrap_units` breaks on `\n` before wrapping,
// so each line becomes its own expandable row. Do not "helpfully"
// pre-split here; that would duplicate the split, not enable it.
details: activity.detail.iter().cloned().collect(),
}
}

117
crates/op-editor-ui/src/widgets/ai_chat_transcript_tests/activities.rs
View file

@ -288,3 +288,120 @@ render: captured frame
"collapsed TS accordions hide details until opened"
);
}
#[test]
fn an_itemized_activity_detail_expands_into_one_row_per_line() {
// The quality passes report their repairs as one line each on the
// "Polishing the layout" row. A single-string detail that rendered as one
// unwrappable blob is exactly the "I cannot see what the check changed"
// complaint this exists to answer.
let detail = [
"3 auto-repair(s) applied",
"layout · table-gap · Pricing Row [n42] · gap 0 → 16",
"palette · light-mobile-nav-surface · Tab Bar [n7] · fill #F8FAFC → #FFFFFF",
"hierarchy · text-hierarchy · Title [n3] · fontWeight 800 → 400",
]
.join("\n");
let mut message = ChatMessage::assistant_streaming();
message.activities = vec![op_editor_core::ChatActivity {
id: "__polish".into(),
title: "Polishing the layout".into(),
detail: Some(detail),
status: op_editor_core::ChatActivityStatus::Done,
content_offset: None,
}];
let items = build_transcript(
std::slice::from_ref(&message),
body(),
op_editor_core::Locale::EnUs,
);
let step = &items[0].steps[0];
assert_eq!(step.label, "Polishing the layout");
// Rows are word-wrapped to the bubble width, so a long record may occupy
// more than one row — what must not happen is lines being merged or
// dropped.
assert!(
step.details.len() >= 4,
"each reported repair must reach the row list: {:?}",
step.details
);
// Each reported repair must OWN a row: a row that merely CONTAINS the
// text could be one blob the layout wrapped at an arbitrary column, which
// is the unreadable rendering this test exists to reject.
for record in [
"3 auto-repair(s) applied",
"layout · table-gap · Pricing Row",
"palette · light-mobile-nav-surface",
"hierarchy · text-hierarchy · Title",
] {
assert!(
step.details.iter().any(|row| row.starts_with(record)),
"`{record}` must begin its own row: {:?}",
step.details
);
}
assert!(
step.details.iter().all(|row| !row.contains('\n')),
"no row may carry a raw newline: {:?}",
step.details
);
}
#[test]
fn a_long_repair_list_renders_every_line_it_was_given_plus_the_overflow_notice() {
// The host caps the list at 30 and appends a localized "and N more"
// notice; the transcript's job is to render exactly what it was handed —
// silently dropping rows here would hide repairs the host chose to show.
let mut lines = vec!["45 auto-repair(s) applied".to_string()];
lines.extend(
(0..30).map(|i| format!("layout · container-geometry · Card {i} [n{i}] · gap 24 → 16")),
);
lines.push("… and 15 more (see log)".to_string());
let mut message = ChatMessage::assistant_streaming();
message.activities = vec![op_editor_core::ChatActivity {
id: "__polish".into(),
title: "Polishing the layout".into(),
detail: Some(lines.join("\n")),
status: op_editor_core::ChatActivityStatus::Done,
content_offset: None,
}];
message.action_step_expanded_overrides = vec![Some(true)];
let items = build_transcript(
std::slice::from_ref(&message),
body(),
op_editor_core::Locale::EnUs,
);
let step = &items[0].steps[0];
assert!(
step.details.len() >= 32,
"head line + 30 records + overflow notice must all reach the rows: {}",
step.details.len()
);
assert!(
step.details
.last()
.is_some_and(|row| row.starts_with("… and 15 more")),
"the truncation notice must own the closing row: {:?}",
step.details.last()
);
assert_eq!(
step.details
.iter()
.filter(|row| row.starts_with("layout · container-geometry"))
.count(),
30,
"every one of the 30 shown records must begin its own row"
);
assert!(
step.expanded,
"an explicit expand override must survive the itemized list"
);
assert!(
step.rect.size.y > 32.0 * LINE_H,
"an expanded 32-row list must reserve height for its rows"
);
}

12
crates/op-host-desktop/src/chat_session.rs
View file

@ -348,6 +348,18 @@ fn execute_tool_requests(
serde_json::json!({ "check": check.key(), "count": count })
})
.collect::<Vec<_>>(),
// The itemized half — one rendered line per applied
// edit, so the loop's credential can list WHAT was
// repaired, not only how much.
"records": quality
.records()
.iter()
.map(op_orchestrator::RepairRecord::line)
.collect::<Vec<_>>(),
// Non-edit statements — today, a deliberately skipped
// pass tier. Carried separately from `records` so the
// loop never reports a decision as a repair.
"notes": quality.notes(),
},
})
.to_string(),

80
crates/op-host-desktop/src/design_session.rs
View file

@ -457,17 +457,35 @@ fn apply_progress(msg: &mut ChatMessage, progress: &[Progress], locale: Locale)
// treatment as the two reports above. `remaining` is `None`:
// the promise-delivery check has not run yet at this point, and
// an assumed "no issues left" would be a claim, not a fact.
Progress::QualityChecked { checks, repairs } => {
match op_host_services::quality_credential::quality_credential_line(
&op_ai::chat_provider::QualitySummary {
checks: checks.clone(),
repairs: repairs.clone(),
},
None,
) {
Some(line) => append_narration(msg, line.trim_start()),
None => false,
Progress::QualityChecked {
checks,
repairs,
records,
notes,
} => {
let quality = op_ai::chat_provider::QualitySummary {
checks: checks.clone(),
repairs: repairs.clone(),
records: records.clone(),
notes: notes.clone(),
};
let mut event_changed =
match op_host_services::quality_credential::quality_credential_line(
&quality, None,
) {
Some(line) => append_narration(msg, line.trim_start()),
None => false,
};
// The itemized half rides the "Polishing the layout" row's
// detail rather than the narration: the row is expandable, so
// 41 repairs are one click away instead of 41 lines of prose.
// `_with_records` is deliberately NOT used above — that would
// print the same list twice.
if let Some(detail) = repair_detail_text(&quality, locale) {
event_changed |=
update_activity(msg, "__polish", ChatActivityStatus::Done, Some(detail));
}
event_changed
}
};
}
@ -548,6 +566,44 @@ fn planned_narration(locale: Locale, count: usize) -> String {
op_i18n::translate(locale, key).replace("{{count}}", &count.to_string())
}
/// Build the expandable detail block for the "Polishing the layout" row:
/// a localized "N auto-repair(s) applied" head line, then one line per
/// repair (capped), then a localized notice for whatever the cap left out.
///
/// Newline-separated because [`op_editor_core::ChatActivity::detail`] is one
/// string that the transcript splits per line into the row's expandable
/// details. `None` when the passes reported no itemized repairs at all — an
/// empty block would add a chevron that reveals nothing.
fn repair_detail_text(
quality: &op_ai::chat_provider::QualitySummary,
locale: Locale,
) -> Option<String> {
let lines = op_host_services::quality_credential::quality_repair_detail_lines(quality);
let notes = op_host_services::quality_credential::quality_note_lines(quality);
if lines.is_empty() && notes.is_empty() {
return None;
}
// Notes first: a run whose intent tier was skipped for authored template
// input reads "layout 0" as "not checked", not "nothing wrong", and a
// reader who never expands past the first line must still get that.
let mut out = notes;
if !lines.is_empty() {
out.push(
op_i18n::translate(locale, "ai.designProgress.detail.repairsApplied")
.replace("{{count}}", &quality.records.len().to_string()),
);
out.extend(lines);
let overflow = op_host_services::quality_credential::quality_repair_overflow(quality);
if overflow > 0 {
out.push(
op_i18n::translate(locale, "ai.designProgress.detail.repairsMore")
.replace("{{count}}", &overflow.to_string()),
);
}
}
Some(out.join("\n"))
}
fn append_narration(msg: &mut ChatMessage, text: &str) -> bool {
if text.is_empty() || msg.content.contains(text) {
return false;
@ -590,6 +646,10 @@ fn count_u32(count: usize) -> u32 {
#[path = "design_session_tests.rs"]
mod tests;
#[cfg(test)]
#[path = "design_session_quality_tests.rs"]
mod quality_tests;
#[cfg(test)]
#[path = "design_session_worker_tests.rs"]
mod worker_tests;

210
crates/op-host-desktop/src/design_session_quality_tests.rs Normal file
View file

@ -0,0 +1,210 @@
//! The quality-ledger half of the design-turn progress adapter: what the
//! deterministic passes repaired, and what they deliberately did not run.
//!
//! Split out of `design_session_tests.rs` at the 800-line cap. These share
//! that file's `super::*` surface and drive the same `apply_progress` entry
//! point; they are grouped here because they all assert on ONE thing — the
//! "Polishing the layout" row's expandable detail, which is the only place a
//! user can see which node a pass touched and why.
use super::*;
#[test]
fn quality_records_land_on_the_polish_row_as_expandable_detail() {
// The user's complaint was that the check stage reported "41 auto-repair(s)
// applied" and nothing else. The narration keeps the headline; the itemized
// list rides the "Polishing the layout" row, which the transcript renders
// as expandable detail.
let mut message = op_editor_core::ChatMessage::assistant_streaming();
let events = vec![
Progress::CleanupDone,
Progress::QualityChecked {
checks: vec!["layout".into(), "palette".into()],
repairs: vec![("layout".into(), 2), ("palette".into(), 1)],
records: vec![
"layout · table-gap · Pricing Row [n42] · gap 0 → 16".into(),
"layout · container-geometry · Hero [n7] · padding [32,32] → [16,16]".into(),
"palette · light-mobile-nav-surface · Tab Bar [n9] · fill #F8FAFC → #FFFFFF".into(),
],
notes: Vec::new(),
},
];
assert!(super::apply_progress(&mut message, &events, Locale::EnUs));
let polish = message
.activities
.iter()
.find(|activity| activity.id == "__polish")
.expect("cleanup must have created the polish row");
let detail = polish.detail.as_deref().expect("itemized detail");
assert!(
detail.starts_with("3 auto-repair(s) applied"),
"the head line states the count in the user's locale: {detail}"
);
for record in [
"gap 0 → 16",
"padding [32,32] → [16,16]",
"fill #F8FAFC → #FFFFFF",
] {
assert!(
detail.contains(record),
"`{record}` must be listed on the row: {detail}"
);
}
assert!(
message.content.contains("41 auto-repair(s)")
|| message.content.contains("3 auto-repair(s)"),
"the narration keeps the headline credential: {}",
message.content
);
assert_eq!(
message.content.matches("gap 0 → 16").count(),
0,
"the itemized list must not also be dumped into the narration"
);
}
#[test]
fn a_long_repair_list_is_capped_on_the_row_with_a_localized_remainder_notice() {
let mut message = op_editor_core::ChatMessage::assistant_streaming();
let records: Vec<String> = (0..45)
.map(|i| format!("layout · container-geometry · Card {i} [n{i}] · gap 24 → 16"))
.collect();
let events = vec![
Progress::CleanupDone,
Progress::QualityChecked {
checks: vec!["layout".into()],
repairs: vec![("layout".into(), 45)],
records,
notes: Vec::new(),
},
];
assert!(super::apply_progress(&mut message, &events, Locale::ZhCn));
let detail = message
.activities
.iter()
.find(|activity| activity.id == "__polish")
.and_then(|activity| activity.detail.clone())
.expect("itemized detail");
let lines: Vec<&str> = detail.lines().collect();
assert_eq!(
lines.len(),
32,
"head line + 30 records + remainder notice: {lines:?}"
);
assert!(
lines[0].contains("45"),
"the head line counts every repair, not just the shown ones: {}",
lines[0]
);
assert!(
lines[31].contains("15"),
"the remainder notice states how many were withheld: {}",
lines[31]
);
assert!(
!lines[31].is_ascii(),
"the notice must come from the locale table, not a hardcoded English string: {}",
lines[31]
);
}
const TIER_SKIP_NOTE: &str = "intent-tier passes skipped (template provenance: slide-deck via \
namespaced-variables) — authored spacing, surfaces and palette \
kept as designed; contract-tier checks still ran";
#[test]
fn a_skipped_tier_note_heads_the_polish_row_detail() {
let mut message = op_editor_core::ChatMessage::assistant_streaming();
let events = vec![
Progress::CleanupDone,
Progress::QualityChecked {
checks: vec!["layout".into()],
repairs: vec![("layout".into(), 1)],
records: vec!["layout · table-gap · Pricing Row [n42] · gap 0 → 16".into()],
notes: vec![TIER_SKIP_NOTE.to_string()],
},
];
assert!(super::apply_progress(&mut message, &events, Locale::EnUs));
let detail = message
.activities
.iter()
.find(|activity| activity.id == "__polish")
.and_then(|activity| activity.detail.clone())
.expect("itemized detail");
let lines: Vec<&str> = detail.lines().collect();
assert!(
lines[0].starts_with("intent-tier passes skipped"),
"what was deliberately NOT run must be the first thing read: {lines:?}"
);
assert!(
lines[1].contains("1 auto-repair(s) applied"),
"the count head line follows the note: {lines:?}"
);
assert!(
lines[2].contains("table-gap"),
"then the itemized repairs: {lines:?}"
);
}
#[test]
fn a_template_run_that_repaired_nothing_still_shows_why() {
// The shape tiering actually produces: authored input needs no repairs,
// so there is no record list at all. Before notes reached the row, this
// rendered as a bare "Polishing the layout ✓" with nothing behind it —
// indistinguishable from "we checked everything and it was perfect".
let mut message = op_editor_core::ChatMessage::assistant_streaming();
let events = vec![
Progress::CleanupDone,
Progress::QualityChecked {
checks: vec!["layout".into(), "structure".into()],
repairs: Vec::new(),
records: Vec::new(),
notes: vec![TIER_SKIP_NOTE.to_string()],
},
];
assert!(super::apply_progress(&mut message, &events, Locale::EnUs));
let detail = message
.activities
.iter()
.find(|activity| activity.id == "__polish")
.and_then(|activity| activity.detail.clone())
.expect("a note alone must still open the row");
assert!(detail.starts_with("intent-tier passes skipped"), "{detail}");
assert!(
!detail.contains("auto-repair(s) applied"),
"no repairs ran, so no count line should claim any: {detail}"
);
}
#[test]
fn a_run_with_neither_notes_nor_records_leaves_the_row_bare() {
// The honesty counterpart: nothing to itemize must not manufacture an
// empty expandable row that reveals nothing when clicked.
let mut message = op_editor_core::ChatMessage::assistant_streaming();
let events = vec![
Progress::CleanupDone,
Progress::QualityChecked {
checks: vec!["layout".into()],
repairs: Vec::new(),
records: Vec::new(),
notes: Vec::new(),
},
];
super::apply_progress(&mut message, &events, Locale::EnUs);
let polish = message
.activities
.iter()
.find(|activity| activity.id == "__polish")
.expect("polish row");
assert!(polish.detail.is_none(), "{:?}", polish.detail);
}

78
crates/op-host-services/src/quality_credential.rs
View file

@ -48,9 +48,54 @@ pub fn quality_summary_from_repairs(summary: &op_orchestrator::RepairSummary) ->
.into_iter()
.map(|(check, count)| (check.key().to_string(), count))
.collect(),
records: summary
.records()
.iter()
.map(op_orchestrator::RepairRecord::line)
.collect(),
notes: summary.notes().to_vec(),
}
}
/// How many repair lines a caller shows inline before summarizing the rest.
/// A routine run applies dozens; the ceiling keeps one turn from burying the
/// transcript while the log keeps the complete list.
pub const MAX_INLINE_REPAIR_RECORDS: usize = 30;
/// The itemized repair lines, capped at [`MAX_INLINE_REPAIR_RECORDS`].
///
/// Returns the lines only — no header, no "and N more" notice: the callers
/// that have a locale (the desktop progress panel) localize those around
/// this list, and the ones that do not (the diagnostic transcript lines)
/// render them with their own English framing. Empty when the summary
/// carries no records, so a caller can skip the whole block.
pub fn quality_repair_detail_lines(quality: &QualitySummary) -> Vec<String> {
quality
.records
.iter()
.take(MAX_INLINE_REPAIR_RECORDS)
.cloned()
.collect()
}
/// The non-edit statements about the run, rendered ahead of the repair list.
///
/// Uncapped on purpose: notes are decisions, not edits — the orchestrator
/// de-duplicates them, so a run carries 0 or 1 today and would carry a
/// handful at worst. Truncating a "a whole tier of passes did not run" line
/// would hide exactly the fact it exists to surface.
pub fn quality_note_lines(quality: &QualitySummary) -> Vec<String> {
quality.notes.clone()
}
/// How many records the cap left out — `0` when everything is shown.
pub fn quality_repair_overflow(quality: &QualitySummary) -> usize {
quality
.records
.len()
.saturating_sub(MAX_INLINE_REPAIR_RECORDS)
}
/// Render the credential, or `None` when nothing was checked.
///
/// `remaining` is how many issues are still open after the passes ran:
@ -92,6 +137,39 @@ pub fn quality_credential_line(
Some(line)
}
/// [`quality_credential_line`] plus one `▸` sub-line per applied repair.
///
/// For the callers whose ONLY channel to the user is this text: the agentic
/// loop's transcript stream and the web host's thinking stream. Both render
/// `▸` sub-lines as the owning step's expandable detail
/// (`split_design_progress`), so the itemized list lands under the credential
/// instead of beside it. Hosts that can attach detail to a structured
/// progress row (the desktop panel) use the plain line and pass
/// [`quality_repair_detail_lines`] to that row instead, so nothing is shown
/// twice.
pub fn quality_credential_line_with_records(
quality: &QualitySummary,
remaining: Option<usize>,
) -> Option<String> {
let mut line = quality_credential_line(quality, remaining)?;
// Notes lead: a tier that was deliberately SKIPPED reframes every number
// above it ("layout 0" means "not checked", not "nothing wrong"), so it
// must not sit below a list the reader may never scroll through.
for note in quality_note_lines(quality) {
line.push_str(&format!("\n ▸ {note}"));
}
for record in quality_repair_detail_lines(quality) {
line.push_str(&format!("\n ▸ {record}"));
}
let overflow = quality_repair_overflow(quality);
if overflow > 0 {
line.push_str(&format!(
"\n ▸ … and {overflow} more repair(s) — full list in the log"
));
}
Some(line)
}
#[cfg(test)]
#[path = "quality_credential_tests.rs"]
mod tests;

170
crates/op-host-services/src/quality_credential_tests.rs
View file

@ -1,4 +1,7 @@
use super::{quality_credential_line, quality_summary_from_repairs};
use super::{
quality_credential_line, quality_credential_line_with_records, quality_note_lines,
quality_summary_from_repairs, MAX_INLINE_REPAIR_RECORDS,
};
use op_ai::chat_provider::QualitySummary;
use op_orchestrator::{CheckCategory, RepairSummary};
@ -6,6 +9,8 @@ fn summary(checks: &[&str], repairs: &[(&str, usize)]) -> QualitySummary {
QualitySummary {
checks: checks.iter().map(|c| c.to_string()).collect(),
repairs: repairs.iter().map(|(c, n)| (c.to_string(), *n)).collect(),
records: Vec::new(),
notes: Vec::new(),
}
}
@ -95,3 +100,166 @@ fn repair_summary_converts_preserving_checked_and_repaired_split() {
);
assert_eq!(wire.total_repairs(), 3);
}
#[test]
fn the_itemized_variant_lists_each_repair_under_the_credential() {
// For the callers whose only channel is this text (the agentic loop, the
// web thinking stream): the `▸` sub-lines are what the transcript turns
// into the credential step's expandable detail.
let quality = QualitySummary {
checks: vec!["layout".into()],
repairs: vec![("layout".into(), 2)],
records: vec![
"layout · table-gap · Pricing Row [n42] · gap 0 → 16".into(),
"layout · container-geometry · Hero [n7] · padding [32,32] → [16,16]".into(),
],
notes: Vec::new(),
};
let line = quality_credential_line_with_records(&quality, Some(0)).expect("credential");
assert!(line.contains("2 auto-repair(s) applied"));
assert!(
line.contains("\n ▸ layout · table-gap · Pricing Row [n42] · gap 0 → 16"),
"each record is its own sub-line: {line}"
);
assert!(
!line.contains("more repair(s)"),
"nothing was withheld, so no remainder notice: {line}"
);
}
#[test]
fn the_itemized_variant_caps_the_list_and_says_how_many_it_withheld() {
let records: Vec<String> = (0..MAX_INLINE_REPAIR_RECORDS + 11)
.map(|i| format!("layout · container-geometry · Card {i} [n{i}] · gap 24 → 16"))
.collect();
let total = records.len();
let quality = QualitySummary {
checks: vec!["layout".into()],
repairs: vec![("layout".into(), total)],
records,
notes: Vec::new(),
};
let line = quality_credential_line_with_records(&quality, None).expect("credential");
assert_eq!(
line.matches("· Card ").count(),
MAX_INLINE_REPAIR_RECORDS,
"the inline list stops at the cap"
);
assert!(
line.contains("… and 11 more repair(s) — full list in the log"),
"the withheld count must be stated, never silently dropped: {line}"
);
assert!(
line.contains(&format!("{total} auto-repair(s) applied")),
"the headline still counts every repair: {line}"
);
}
#[test]
fn record_lines_ride_the_wire_conversion_alongside_the_counts() {
let mut repairs = RepairSummary::default();
repairs.record_all(
CheckCategory::Layout,
vec![op_orchestrator::RepairRecord {
pass: "table-gap".into(),
category: CheckCategory::Layout,
node_id: "n42".into(),
node_name: Some("Pricing Row".into()),
detail: "gap 0 → 16".into(),
}],
);
let wire = quality_summary_from_repairs(&repairs);
assert_eq!(wire.repairs, vec![("layout".to_string(), 1)]);
assert_eq!(
wire.records,
vec!["layout · table-gap · Pricing Row [n42] · gap 0 → 16".to_string()],
"the count and the line it stands for must cross the wire together"
);
}
const SKIP_NOTE: &str = "intent-tier passes skipped (template provenance: slide-deck via \
namespaced-variables) — authored spacing, surfaces and palette kept \
as designed; contract-tier checks still ran";
#[test]
fn a_skipped_tier_is_stated_before_the_repairs_that_did_run() {
// Order is the point. "layout 1" under a skipped intent tier means "one
// contract-tier fix", not "the layout was reviewed and needed one fix" —
// a reader who stops after the first detail line must already know that.
let quality = QualitySummary {
checks: vec!["layout".into()],
repairs: vec![("layout".into(), 1)],
records: vec!["layout · table-gap · Pricing Row [n42] · gap 0 → 16".into()],
notes: vec![SKIP_NOTE.to_string()],
};
let line = quality_credential_line_with_records(&quality, Some(0)).expect("credential");
let note_at = line
.find("intent-tier passes skipped")
.expect("note rendered");
let record_at = line.find("table-gap").expect("record rendered");
assert!(
note_at < record_at,
"the skipped-tier note must precede the repair list: {line}"
);
}
#[test]
fn a_note_survives_a_run_that_repaired_nothing() {
// The case the tiering exists to produce: an authored template needs no
// repairs, so the record list is empty. Gating the note on repairs would
// hide the decision exactly where it is the ONLY thing worth reporting.
let quality = QualitySummary {
checks: vec!["layout".into(), "structure".into()],
repairs: Vec::new(),
records: Vec::new(),
notes: vec![SKIP_NOTE.to_string()],
};
let line = quality_credential_line_with_records(&quality, Some(0)).expect("credential");
assert!(line.contains("nothing needed fixing"));
assert!(
line.contains("\n ▸ intent-tier passes skipped"),
"the note must still be reported: {line}"
);
}
#[test]
fn a_note_is_never_counted_as_a_repair() {
let quality = QualitySummary {
checks: vec!["layout".into()],
repairs: Vec::new(),
records: Vec::new(),
notes: vec![SKIP_NOTE.to_string()],
};
assert_eq!(
quality.total_repairs(),
0,
"a statement about the run is not an edit to the document"
);
assert_eq!(quality_note_lines(&quality).len(), 1);
}
#[test]
fn notes_ride_the_wire_conversion_beside_the_records() {
let mut repairs = RepairSummary::default();
repairs.record(CheckCategory::Layout, 0);
repairs.note(SKIP_NOTE);
let wire = quality_summary_from_repairs(&repairs);
assert_eq!(wire.notes, vec![SKIP_NOTE.to_string()]);
assert!(
wire.records.is_empty(),
"notes must not leak into the itemized repair list"
);
}

165
crates/op-orchestrator/src/cleanup_repair_summary_tests.rs
View file

@ -254,3 +254,168 @@ fn the_tally_never_exceeds_the_edits_the_sink_actually_took() {
sink.applied.len()
);
}
#[test]
fn the_counts_are_the_itemized_records_grouped_and_nothing_else() {
// The one invariant that keeps the credential honest end to end: the
// number the user is shown and the list they expand are the SAME data.
// Maintaining them separately is how a "41 repairs" headline ends up over
// 39 lines with nobody able to say which two are missing.
let (_sink, summary) = run_with_summary(json!({
"type": "frame",
"id": "root",
"name": "Mobile Root",
"width": 390,
"height": 844,
"children": [
{ "type": "text", "id": "title", "role": "heading",
"content": "Popular Restaurants", "width": 320, "height": 40,
"fontSize": 30, "fontWeight": 800 },
{ "type": "text", "id": "subtitle", "role": "body-text",
"content": "Fresh Brooklyn favorites, delivered fast.",
"width": 320, "height": 22, "fontSize": 16, "fontWeight": 800 },
{ "type": "text", "id": "placeholder", "name": "Placeholder",
"content": "Search restaurants or dishes", "width": 280,
"height": 24, "fontSize": 17, "fontWeight": 800 },
{ "type": "text", "id": "metadata", "role": "caption",
"content": "20-30 min", "width": 100, "height": 18,
"fontSize": 14, "fontWeight": 800 }
]
}));
assert_eq!(
summary.total_repairs(),
summary.records().len(),
"the headline total is the record list's length, not a second tally"
);
for category in CheckCategory::ALL {
assert_eq!(
summary.repairs_for(category),
summary
.records()
.iter()
.filter(|record| record.category == category)
.count(),
"the per-category count for {category:?} must be its records grouped"
);
}
let repaired_total: usize = summary.repaired().iter().map(|(_, count)| count).sum();
assert_eq!(
repaired_total,
summary.records().len(),
"the breakdown must account for every record, with none double-counted"
);
}
#[test]
fn every_record_names_the_pass_the_node_and_the_change() {
// "41 auto-repair(s) applied" with no way to see WHAT moved was the whole
// complaint. Each record must carry an attributed pass, a real target,
// and a field-level description — an empty one is a repair nobody can
// audit.
let (_sink, summary) = run_with_summary(json!({
"type": "frame",
"id": "root",
"name": "Mobile Root",
"width": 390,
"height": 844,
"children": [
{ "type": "text", "id": "title", "role": "heading",
"content": "Popular Restaurants", "width": 320, "height": 40,
"fontSize": 30, "fontWeight": 800 },
{ "type": "text", "id": "subtitle", "role": "body-text",
"content": "Fresh Brooklyn favorites, delivered fast.",
"width": 320, "height": 22, "fontSize": 16, "fontWeight": 800 },
{ "type": "text", "id": "placeholder", "name": "Placeholder",
"content": "Search restaurants or dishes", "width": 280,
"height": 24, "fontSize": 17, "fontWeight": 800 },
{ "type": "text", "id": "metadata", "role": "caption",
"content": "20-30 min", "width": 100, "height": 18,
"fontSize": 14, "fontWeight": 800 }
]
}));
assert!(
!summary.records().is_empty(),
"precondition: this fixture must provoke repairs"
);
for record in summary.records() {
assert!(!record.pass.is_empty(), "unattributed record: {record:?}");
assert_ne!(
record.pass, "unattributed",
"a real cleanup run must never fall back to the count-only shim: {record:?}"
);
assert!(!record.detail.is_empty(), "undescribed record: {record:?}");
assert!(
record.line().contains(&record.detail),
"the rendered line must carry the change it describes: {record:?}"
);
}
let hierarchy: Vec<String> = summary
.records()
.iter()
.filter(|record| record.category == CheckCategory::Hierarchy)
.map(|record| record.detail.clone())
.collect();
assert!(
hierarchy
.iter()
.any(|detail| detail == "fontWeight 800 → 400"),
"the hierarchy demotion must be itemized with before → after: {hierarchy:?}"
);
}
#[test]
fn the_double_inset_stripper_reports_which_section_lost_which_padding() {
// The incident this whole itemized-record line exists for: a run reported
// "41 auto-repair(s) applied — layout 36" and the user could see their
// section rhythm had been flattened but not by what. 36 of those came from
// `strip_wrapper_double_inset`. Pinned here as the worked example — the
// record must name the SECTION and the padding it lost, because "padding
// dropped on Hero Section" is the sentence that makes the repair
// disputable instead of mysterious.
let (_sink, summary) = run_with_summary(json!({
"type": "frame",
"id": "root",
"name": "Home Screen",
"width": 390,
"height": 844,
"layout": "vertical",
"gap": 20,
"padding": [24, 20, 24, 20],
"children": [{
"type": "frame",
"id": "hero",
"name": "Hero Section",
"layout": "vertical",
"gap": 8,
"padding": [24, 20, 24, 20],
"children": [
{ "type": "text", "id": "t1", "role": "heading", "content": "Good morning",
"width": 300, "height": 32, "fontSize": 24, "fontWeight": 700 },
{ "type": "text", "id": "t2", "role": "body-text", "content": "Three tasks today",
"width": 300, "height": 20, "fontSize": 15, "fontWeight": 400 }
]
}]
}));
let padding_records: Vec<String> = summary
.records()
.iter()
.filter(|record| record.detail.contains("padding"))
.map(|record| record.line())
.collect();
assert!(
padding_records
.iter()
.any(|line| line.contains("Hero Section")
&& line.contains("padding [24,20,24,20] → (unset)")),
"the stripped section and the inset it lost must both be named: {padding_records:?}"
);
assert!(
padding_records
.iter()
.any(|line| line.starts_with("layout · spacing+footer-sink")),
"the repair must be attributed to the pass group that made it: {padding_records:?}"
);
}

599
crates/op-orchestrator/src/repair_record.rs Normal file
View file

@ -0,0 +1,599 @@
//! Field-level record of every document edit the deterministic quality
//! passes applied — the itemized half of `crate::repair_summary`'s tally.
//!
//! **Why this exists.** The credential the user sees used to be a bare
//! count ("41 auto-repair(s) applied"). When a pass repaired something the
//! user did NOT want repaired — a pale showcase band restyled, a section's
//! alignment rewritten — there was no way to find out which pass touched
//! which node, so a mis-repair was indistinguishable from a model mistake.
//! One [`RepairRecord`] per accepted edit makes that answerable.
//!
//! **One source of truth.** Records are produced at exactly the same place
//! the counter counts — `repair_summary::RecordingSink::apply` — so counts
//! are `records.len()` grouped by category, never a parallel tally that can
//! drift from the list.
//!
//! **Granularity, honestly stated.** [`describe_command`] reads the target
//! node BEFORE the edit lands, so single-property commands
//! (`SetNodeLayoutProp` / `SetNodeFillHex` / `PatchNodeData` / …) carry a
//! real `before → after`. Whole-subtree swaps (`ReplaceSubtree`, the shape
//! every `apply_root_transform` pass uses) carry a bounded diff of what
//! changed inside the subtree instead — coarser, but still node-and-field
//! level. Anything else degrades to the command's own name rather than
//! inventing a description.
use jian_ops_schema::node::PenNode;
use op_editor_core::{EditorCommand, EditorState, LayoutPropValue, NodeId, PenNodeExt};
use serde_json::Value;
use crate::repair_summary::CheckCategory;
/// How many changed nodes a subtree-replacement diff names before it
/// summarizes the rest as a count. Keeps one restructure from producing a
/// transcript-length line.
const SUBTREE_DIFF_NODE_LIMIT: usize = 3;
/// How many changed fields are named per node inside that diff.
const SUBTREE_DIFF_FIELD_LIMIT: usize = 3;
/// How an absent value renders on either side of a `before → after`. A word,
/// not a dash: a repair that DROPS a property (`strip_wrapper_double_inset`
/// removes `padding` outright when every side reaches 0) must read as "this
/// property is now unset", which is the disputed fact — a bare dash reads
/// like a placeholder the renderer failed to fill in.
const UNSET: &str = "(unset)";
/// Longest rendered value before it is elided. A fill body or a styled-text
/// run serializes to hundreds of characters; the point is recognizability.
const VALUE_MAX_CHARS: usize = 48;
/// Node fields a subtree diff reports. Deliberately the *visible* ones —
/// the properties a user can see go wrong (surface colour, spacing,
/// alignment, sizing, clipping) — so the record answers "what changed on
/// screen", not "what moved in the JSON".
const DIFFED_FIELDS: [&str; 16] = [
"fills",
"fill",
"backgroundColor",
"stroke",
"padding",
"gap",
"layoutMode",
"justifyContent",
"alignItems",
"clipContent",
"width",
"height",
"opacity",
"cornerRadius",
"fontSize",
"fontWeight",
];
/// One accepted document edit a quality pass applied.
///
/// `pass` is the pass GROUP the edit belongs to, stamped at the checkpoint
/// that closes the group (see `RepairCounter::checkpoint`). Groups that run
/// a single pass name that pass exactly; multi-pass groups name the family.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct RepairRecord {
/// Pass (or pass group) that applied the edit.
pub pass: String,
/// Check family the edit is credited to in the summary.
pub category: CheckCategory,
/// Target node id — empty for document-level edits (variables).
pub node_id: String,
/// Target node's name at edit time, when it had one.
pub node_name: Option<String>,
/// What changed, field level: `gap 24 → 16`.
pub detail: String,
}
impl RepairRecord {
/// One-line rendering for the transcript and the log:
/// `layout · table-repair · Pricing Row [n42] · gap 0 → 16`.
pub fn line(&self) -> String {
let mut out = String::new();
out.push_str(self.category.key());
out.push_str(" · ");
out.push_str(&self.pass);
let who = self.node_label();
if !who.is_empty() {
out.push_str(" · ");
out.push_str(&who);
}
out.push_str(" · ");
out.push_str(&self.detail);
out
}
/// `Name [id]`, `[id]`, or empty — whichever the node actually has.
fn node_label(&self) -> String {
let name = self
.node_name
.as_deref()
.map(str::trim)
.filter(|name| !name.is_empty());
match (name, self.node_id.as_str()) {
(Some(name), "") => name.to_string(),
(Some(name), id) => format!("{name} [{id}]"),
(None, "") => String::new(),
(None, id) => format!("[{id}]"),
}
}
}
/// A repair captured at apply time, before a checkpoint attributes it to a
/// category and a pass group.
#[derive(Debug, Clone, PartialEq, Eq)]
pub(crate) struct PendingRepair {
pub(crate) node_id: String,
pub(crate) node_name: Option<String>,
pub(crate) detail: String,
}
impl PendingRepair {
pub(crate) fn attribute(self, category: CheckCategory, pass: &str) -> RepairRecord {
RepairRecord {
pass: pass.to_string(),
category,
node_id: self.node_id,
node_name: self.node_name,
detail: self.detail,
}
}
}
/// Describe `cmd` against the document as it stands BEFORE the command is
/// applied, so `before → after` reflects the real prior value.
///
/// Never fails: an unrecognized command still yields a record naming the
/// operation, because a silent gap in the list would break the "count ==
/// records" contract the summary depends on.
pub(crate) fn describe_command(state: &EditorState, cmd: &EditorCommand) -> PendingRepair {
match cmd {
EditorCommand::PatchNodeData {
node_id,
patch_json,
..
} => {
let detail = describe_patch(node_value(state, node_id).as_ref(), patch_json);
pending(state, node_id, detail)
}
EditorCommand::SetNodeLayoutProp {
node_id,
property,
value,
} => {
let before = node_field(state, node_id, property);
pending(
state,
node_id,
format!(
"{property} {} → {}",
render_opt(before.as_ref()),
render_layout_value(value)
),
)
}
EditorCommand::SetNodeFillHex { node_id, hex } => {
let before = find(state, node_id).and_then(op_editor_core::first_solid_fill_hex);
pending(
state,
node_id,
format!("fill {} → {hex}", before.unwrap_or(UNSET)),
)
}
EditorCommand::SetNodeStrokeHex { node_id, hex } => {
pending(state, node_id, format!("stroke → {hex}"))
}
EditorCommand::SetNodeStrokeWidth { node_id, width } => {
let before = find(state, node_id).and_then(op_editor_core::fills::node_stroke_width);
pending(
state,
node_id,
format!("strokeWidth {} → {width}", render_num_opt(before)),
)
}
EditorCommand::SetNodeStrokeSideWidth {
node_id,
side,
width,
} => pending(state, node_id, format!("strokeWidth {side:?} → {width}")),
EditorCommand::SetNodeFontSize { node_id, font_size } => {
let before = node_field(state, node_id, "fontSize");
pending(
state,
node_id,
format!("fontSize {} → {font_size}", render_opt(before.as_ref())),
)
}
EditorCommand::SetNodeFontWeight {
node_id,
font_weight,
} => {
let before = node_field(state, node_id, "fontWeight");
pending(
state,
node_id,
format!("fontWeight {} → {font_weight}", render_opt(before.as_ref())),
)
}
EditorCommand::SetNodeCornerRadius { node_id, radius } => {
let before = node_field(state, node_id, "cornerRadius");
pending(
state,
node_id,
format!("cornerRadius {} → {radius}", render_opt(before.as_ref())),
)
}
EditorCommand::SetNodeRotation { node_id, degrees } => {
pending(state, node_id, format!("rotation → {degrees}"))
}
EditorCommand::SetNodeText { node_id, text } => {
pending(state, node_id, format!("text → {}", elide(text)))
}
EditorCommand::SetNodeName { node_id, name } => {
pending(state, node_id, format!("name → {}", elide(name)))
}
EditorCommand::SetNodeFlag {
node_id,
flag,
value,
} => pending(state, node_id, format!("{flag:?} → {value}")),
EditorCommand::UpdateNode {
node_id,
x,
y,
width,
height,
name,
fill_hex,
..
} => {
let before = node_value(state, node_id);
let mut parts = Vec::new();
push_num_change(&mut parts, before.as_ref(), "x", x.map(f64::from));
push_num_change(&mut parts, before.as_ref(), "y", y.map(f64::from));
push_num_change(&mut parts, before.as_ref(), "width", width.map(f64::from));
push_num_change(&mut parts, before.as_ref(), "height", height.map(f64::from));
if let Some(name) = name {
parts.push(format!("name → {}", elide(name)));
}
if let Some(hex) = fill_hex {
let prior = find(state, node_id).and_then(op_editor_core::first_solid_fill_hex);
parts.push(format!("fill {} → {hex}", prior.unwrap_or(UNSET)));
}
let detail = if parts.is_empty() {
"updated".to_string()
} else {
parts.join(", ")
};
pending(state, node_id, detail)
}
EditorCommand::DeleteNode { node_id, .. } => {
let kind = find(state, node_id)
.map(node_kind_label)
.unwrap_or_else(|| "node".to_string());
let descendants = find(state, node_id)
.map(crate::cleanup::count_descendants)
.unwrap_or(0);
pending(
state,
node_id,
format!("removed {kind} (+{descendants} descendant(s))"),
)
}
EditorCommand::MoveNode {
node_id,
target_parent,
index,
..
} => {
let parent = node_label_for(state, target_parent);
let slot = index.map(|i| format!(" at index {i}")).unwrap_or_default();
pending(state, node_id, format!("moved under {parent}{slot}"))
}
EditorCommand::ReplaceSubtree { node_id, node, .. } => {
let detail = match find(state, node_id) {
Some(before) => describe_subtree_replacement(before, node),
None => "restructured".to_string(),
};
pending(state, node_id, detail)
}
EditorCommand::InsertSubtree {
nodes, parent_id, ..
}
| EditorCommand::InsertAuthoredSubtree {
nodes, parent_id, ..
}
| EditorCommand::InsertAuthoredSubtreePreservingRoots {
nodes, parent_id, ..
} => pending(
state,
parent_id,
format!("inserted {} subtree(s)", nodes.len()),
),
EditorCommand::SetVariableColor { name, hex } => {
doc_level(format!("variable {name} → {hex}"))
}
EditorCommand::Batch { commands } => {
doc_level(format!("batch of {} edit(s)", commands.len()))
}
other => doc_level(command_label(other).to_string()),
}
}
/// Fallback label for a command this module does not describe field-by-field.
/// A short static name, never a `Debug` dump — `ReplaceSubtree`'s payload
/// alone would be a whole document.
fn command_label(cmd: &EditorCommand) -> &'static str {
match cmd {
EditorCommand::SetVariableScalar { .. } => "variable value set",
EditorCommand::SetVariables { .. } | EditorCommand::UpsertVariables { .. } => {
"variables merged"
}
EditorCommand::CreateVariable { .. } => "variable created",
EditorCommand::DeleteVariable { .. } => "variable deleted",
EditorCommand::RenameVariable { .. } => "variable renamed",
EditorCommand::SetThemes { .. } | EditorCommand::MergeThemePreset { .. } => "theme merged",
EditorCommand::MergeAppState { .. } => "app state merged",
EditorCommand::ReplaceFontFamily { .. } => "font family replaced",
EditorCommand::ReplaceAllMatchingProperties { .. } => "matching properties replaced",
EditorCommand::SetEllipseArc { .. } => "arc geometry set",
EditorCommand::AddNodeEffect { .. } => "effect added",
EditorCommand::RemoveNodeEffect { .. } => "effect removed",
EditorCommand::SetEffectParam { .. } | EditorCommand::SetEffectColor { .. } => {
"effect updated"
}
_ => "edit applied",
}
}
/// Compare a `PatchNodeData` payload against the node's current fields.
fn describe_patch(before: Option<&Value>, patch_json: &str) -> String {
let Ok(patch) = serde_json::from_str::<Value>(patch_json) else {
return format!("patched {}", elide(patch_json));
};
let Some(fields) = patch.as_object() else {
return format!("patched {}", elide(patch_json));
};
let mut parts = Vec::new();
for (key, after) in fields {
let prior = before.and_then(|value| value.get(key));
if prior == Some(after) {
continue;
}
parts.push(format!(
"{key} {} → {}",
render_opt(prior),
render_value(after)
));
}
if parts.is_empty() {
return "patched (no field change)".to_string();
}
parts.join(", ")
}
/// Bounded node-and-field diff of a whole-subtree swap.
///
/// Matching is by node id: `ReplaceSubtree` rebuilds the root but keeps the
/// ids of the nodes it carries over, so ids present on both sides are the
/// nodes a pass EDITED (as opposed to added or dropped) — exactly the set a
/// user asking "what did the polish stage do to my section" wants named.
fn describe_subtree_replacement(before: &PenNode, after: &PenNode) -> String {
let old_nodes = flatten(before);
let new_nodes = flatten(after);
let mut changed: Vec<String> = Vec::new();
let mut changed_total = 0usize;
for (id, new_value) in &new_nodes {
let Some(old_value) = old_nodes.get(id) else {
continue;
};
let fields = changed_fields(old_value, new_value);
if fields.is_empty() {
continue;
}
changed_total += 1;
if changed.len() < SUBTREE_DIFF_NODE_LIMIT {
let label = new_value
.get("name")
.and_then(Value::as_str)
.map(str::trim)
.filter(|name| !name.is_empty())
.map(|name| format!("{name} [{id}]"))
.unwrap_or_else(|| format!("[{id}]"));
changed.push(format!("{label}: {}", fields.join(", ")));
}
}
let added = new_nodes
.keys()
.filter(|id| !old_nodes.contains_key(*id))
.count();
let removed = old_nodes
.keys()
.filter(|id| !new_nodes.contains_key(*id))
.count();
let mut out = String::from("restructured");
if !changed.is_empty() {
out.push_str(&format!(" — {}", changed.join("; ")));
if changed_total > changed.len() {
out.push_str(&format!(
" (+{} more node(s))",
changed_total - changed.len()
));
}
} else if changed_total > 0 {
out.push_str(&format!(" — {changed_total} node(s) changed"));
}
if added > 0 || removed > 0 {
out.push_str(&format!(" [+{added}/-{removed} node(s)]"));
}
out
}
/// Changed [`DIFFED_FIELDS`] between two serialized nodes, capped.
fn changed_fields(before: &Value, after: &Value) -> Vec<String> {
let mut out = Vec::new();
for key in DIFFED_FIELDS {
let old = before.get(key);
let new = after.get(key);
if old == new {
continue;
}
out.push(format!("{key} {} → {}", render_opt(old), render_opt(new)));
if out.len() == SUBTREE_DIFF_FIELD_LIMIT {
break;
}
}
out
}
/// Serialize a subtree into `id -> node object` with `children` stripped, so
/// a field comparison sees only the node's own properties.
fn flatten(node: &PenNode) -> std::collections::BTreeMap<String, Value> {
let mut out = std::collections::BTreeMap::new();
if let Ok(value) = serde_json::to_value(node) {
flatten_value(&value, &mut out);
}
out
}
fn flatten_value(value: &Value, out: &mut std::collections::BTreeMap<String, Value>) {
let Some(object) = value.as_object() else {
return;
};
if let Some(children) = object.get("children").and_then(Value::as_array) {
for child in children {
flatten_value(child, out);
}
}
let Some(id) = object.get("id").and_then(Value::as_str) else {
return;
};
let mut shallow = object.clone();
shallow.remove("children");
out.insert(id.to_string(), Value::Object(shallow));
}
fn pending(state: &EditorState, node_id: &NodeId, detail: String) -> PendingRepair {
PendingRepair {
node_id: node_id.as_str().to_string(),
node_name: find(state, node_id).and_then(|node| node.base().name.clone()),
detail,
}
}
fn doc_level(detail: String) -> PendingRepair {
PendingRepair {
node_id: String::new(),
node_name: None,
detail,
}
}
fn find<'a>(state: &'a EditorState, node_id: &NodeId) -> Option<&'a PenNode> {
op_editor_core::walkers::find_node(state.active_children(), node_id)
}
fn node_value(state: &EditorState, node_id: &NodeId) -> Option<Value> {
find(state, node_id).and_then(|node| serde_json::to_value(node).ok())
}
fn node_field(state: &EditorState, node_id: &NodeId, key: &str) -> Option<Value> {
node_value(state, node_id).and_then(|value| value.get(key).cloned())
}
fn node_kind_label(node: &PenNode) -> String {
serde_json::to_value(node)
.ok()
.and_then(|value| {
value
.get("type")
.or_else(|| value.get("kind"))
.and_then(Value::as_str)
.map(str::to_string)
})
.unwrap_or_else(|| "node".to_string())
}
fn node_label_for(state: &EditorState, node_id: &NodeId) -> String {
if node_id.as_str().is_empty() {
return "page root".to_string();
}
match find(state, node_id).and_then(|node| node.base().name.clone()) {
Some(name) if !name.trim().is_empty() => format!("{name} [{}]", node_id.as_str()),
_ => format!("[{}]", node_id.as_str()),
}
}
fn push_num_change(parts: &mut Vec<String>, before: Option<&Value>, key: &str, after: Option<f64>) {
let Some(after) = after else {
return;
};
let prior = before.and_then(|value| value.get(key));
parts.push(format!("{key} {} → {}", render_opt(prior), trim_num(after)));
}
fn render_layout_value(value: &LayoutPropValue) -> String {
match value {
LayoutPropValue::Number(n) => trim_num(*n),
LayoutPropValue::Keyword(k) => k.clone(),
LayoutPropValue::Bool(b) => b.to_string(),
LayoutPropValue::NumberArray(values) => format!(
"[{}]",
values
.iter()
.map(|n| trim_num(*n))
.collect::<Vec<_>>()
.join(",")
),
}
}
fn render_opt(value: Option<&Value>) -> String {
value.map(render_value).unwrap_or_else(|| UNSET.to_string())
}
fn render_num_opt(value: Option<f64>) -> String {
value.map(trim_num).unwrap_or_else(|| UNSET.to_string())
}
/// Compact, human-first rendering: strings unquoted, numbers without a
/// trailing `.0`, containers elided once they stop being recognizable.
fn render_value(value: &Value) -> String {
match value {
Value::Null => UNSET.to_string(),
Value::String(s) => elide(s),
Value::Number(n) => n.as_f64().map(trim_num).unwrap_or_else(|| n.to_string()),
Value::Bool(b) => b.to_string(),
// Padding / radii arrive as number arrays; rendering them through
// `to_string` would print `[32.0,32.0,32.0,32.0]` for what the user
// authored as `[32,32,32,32]`.
Value::Array(items) => elide(&format!(
"[{}]",
items.iter().map(render_value).collect::<Vec<_>>().join(",")
)),
other => elide(&other.to_string()),
}
}
fn trim_num(value: f64) -> String {
if value.fract() == 0.0 && value.abs() < 1e15 {
format!("{}", value as i64)
} else {
format!("{value:.2}")
}
}
fn elide(text: &str) -> String {
let cleaned = text.replace('\n', " ");
if cleaned.chars().count() <= VALUE_MAX_CHARS {
return cleaned;
}
let head: String = cleaned.chars().take(VALUE_MAX_CHARS).collect();
format!("{head}…")
}
#[cfg(test)]
#[path = "repair_record_tests.rs"]
mod tests;

230
crates/op-orchestrator/src/repair_record_tests.rs Normal file
View file

@ -0,0 +1,230 @@
//! Coverage for the itemized repair record — specifically the part that is
//! easy to get wrong and impossible to notice: the `before` half of
//! `before → after`. It only exists in the document for the instant before
//! the command is applied, so every assertion here builds a real node, reads
//! the description against the pre-edit state, and checks the OLD value is
//! in the line. A record that only reported the new value would look right
//! in the UI and be useless for the question it exists to answer.
use super::*;
use crate::test_support::VecDocSink;
use op_editor_core::{EditorCommand, LayoutPropValue, NodeId, PenNodeExt};
use serde_json::json;
fn sink_with(tree: serde_json::Value) -> VecDocSink {
let mut sink = VecDocSink::new();
let node: PenNode = serde_json::from_value(tree).expect("fixture json");
sink.state.apply(EditorCommand::InsertSubtree {
nodes: vec![node],
parent_id: NodeId::NONE,
page_id: None,
});
sink
}
fn showcase_tree() -> serde_json::Value {
json!({
"type": "frame",
"id": "root",
"name": "Landing Page",
"width": 1200,
"height": 800,
"layout": "vertical",
"gap": 24,
"children": [{
"type": "frame",
"id": "showcase",
"name": "Showcase Band",
"layout": "horizontal",
"gap": 24,
"padding": [32, 32, 32, 32],
"justifyContent": "center",
"fill": [{ "type": "solid", "color": "#F8FAFC" }],
"children": []
}]
})
}
fn node_id_of(sink: &VecDocSink, name: &str) -> NodeId {
fn walk(nodes: &[PenNode], name: &str) -> Option<String> {
for node in nodes {
if node.base().name.as_deref() == Some(name) {
return Some(node.id_str().to_string());
}
if let Some(children) = node.children() {
if let Some(hit) = walk(children, name) {
return Some(hit);
}
}
}
None
}
NodeId::new(walk(sink.state.active_children(), name).expect("fixture node"))
}
#[test]
fn layout_prop_record_carries_the_previous_value() {
let sink = sink_with(showcase_tree());
let node_id = node_id_of(&sink, "Showcase Band");
let described = describe_command(
&sink.state,
&EditorCommand::SetNodeLayoutProp {
node_id: node_id.clone(),
property: "gap".into(),
value: LayoutPropValue::Number(16.0),
},
);
assert_eq!(described.node_name.as_deref(), Some("Showcase Band"));
assert_eq!(
described.detail, "gap 24 → 16",
"the record must name the value the pass overwrote, not only the new one"
);
}
#[test]
fn fill_record_carries_the_previous_colour() {
// The user-facing question this exists for: "the polish stage changed my
// pale showcase band — to what, from what?"
let sink = sink_with(showcase_tree());
let node_id = node_id_of(&sink, "Showcase Band");
let described = describe_command(
&sink.state,
&EditorCommand::SetNodeFillHex {
node_id,
hex: "#FFFFFF".into(),
},
);
assert_eq!(described.detail, "fill #F8FAFC → #FFFFFF");
}
#[test]
fn patch_record_names_every_changed_key_and_skips_unchanged_ones() {
let sink = sink_with(showcase_tree());
let node_id = node_id_of(&sink, "Showcase Band");
let described = describe_command(
&sink.state,
&EditorCommand::PatchNodeData {
node_id,
// `justifyContent` is already `center`: an unchanged key must not
// be reported as a repair detail, or the list accuses a pass of
// an edit it did not make.
patch_json: r#"{"justifyContent":"center","alignItems":"start"}"#.into(),
page_id: None,
},
);
assert_eq!(described.detail, "alignItems (unset) → start");
}
#[test]
fn subtree_replacement_record_diffs_the_nodes_that_changed() {
let sink = sink_with(showcase_tree());
let root_id = node_id_of(&sink, "Landing Page");
let before = op_editor_core::walkers::find_node(sink.state.active_children(), &root_id)
.expect("root")
.clone();
// What every `apply_root_transform` pass does: hand back a rebuilt root.
let mut after_json = serde_json::to_value(&before).expect("serialize");
after_json["children"][0]["padding"] = json!([16, 16, 16, 16]);
after_json["children"][0]["justifyContent"] = json!("start");
let after: PenNode = serde_json::from_value(after_json).expect("rebuilt root");
let described = describe_command(
&sink.state,
&EditorCommand::ReplaceSubtree {
node_id: root_id,
node: Box::new(after),
drop_children: true,
page_id: None,
},
);
assert!(
described.detail.contains("Showcase Band"),
"the diff must name the node that changed: {}",
described.detail
);
assert!(
described
.detail
.contains("padding [32,32,32,32] → [16,16,16,16]"),
"the diff must carry before → after for the changed field: {}",
described.detail
);
assert!(
described.detail.contains("justifyContent center → start"),
"alignment changes are exactly what a user disputes: {}",
described.detail
);
}
#[test]
fn deletion_record_names_what_was_removed_before_it_is_gone() {
let sink = sink_with(showcase_tree());
let node_id = node_id_of(&sink, "Showcase Band");
let described = describe_command(
&sink.state,
&EditorCommand::DeleteNode {
node_id,
page_id: None,
},
);
assert_eq!(described.node_name.as_deref(), Some("Showcase Band"));
assert!(
described.detail.starts_with("removed"),
"unexpected detail: {}",
described.detail
);
}
#[test]
fn unknown_commands_still_produce_a_record_rather_than_a_gap() {
// The count is `records.len()`, so a command with no bespoke description
// must still yield one — a silent skip would make the credential's number
// disagree with its own list.
let sink = sink_with(showcase_tree());
let described = describe_command(&sink.state, &EditorCommand::PromoteLegacyWidgets);
assert!(!described.detail.is_empty());
}
#[test]
fn record_line_reads_as_pass_node_and_change() {
let record = RepairRecord {
pass: "table-gap".into(),
category: CheckCategory::Layout,
node_id: "n42".into(),
node_name: Some("Pricing Row".into()),
detail: "gap 0 → 16".into(),
};
assert_eq!(
record.line(),
"layout · table-gap · Pricing Row [n42] · gap 0 → 16"
);
}
#[test]
fn record_line_degrades_without_a_node_name() {
let record = RepairRecord {
pass: "theme-variable-polarity".into(),
category: CheckCategory::Palette,
node_id: String::new(),
node_name: None,
detail: "variable surface → #FFFFFF".into(),
};
assert_eq!(
record.line(),
"palette · theme-variable-polarity · variable surface → #FFFFFF"
);
}

203
crates/op-orchestrator/src/repair_summary.rs
View file

@ -12,13 +12,20 @@
//! meaning — see `op_host_services::quality_credential`.
//!
//! **How the count is taken.** [`RepairCounter::wrap`] returns a
//! [`CountingSink`] that delegates every method to the real sink and bumps a
//! shared atomic on each accepted apply. The cleanup driver shadows its own
//! `sink` binding with that wrapper once, at the top, then calls
//! [`CountingSink`] that delegates every method to the real sink and records
//! each accepted apply. The cleanup driver shadows its own `sink` binding
//! with that wrapper once, at the top, then calls
//! [`RepairCounter::checkpoint`] between contiguous groups of passes to
//! attribute the edits since the previous checkpoint to a [`CheckCategory`].
//! Pass bodies are untouched — this is deliberately a measurement layer, not
//! a refactor of ~40 repair passes.
//! attribute the edits since the previous checkpoint to a [`CheckCategory`]
//! and a pass-group name. Pass bodies are untouched — this is deliberately a
//! measurement layer, not a refactor of ~40 repair passes.
//!
//! **One list, not two accounts.** Every accepted apply produces exactly one
//! [`RepairRecord`] (`crate::repair_record::describe_command` reads the
//! target node BEFORE the edit so the record carries `before → after`), and
//! the per-category counts this summary reports are that list grouped by
//! category. There is no separately-maintained tally that could drift from
//! the itemized detail the user is shown.
//!
//! **Honesty rules baked in.** A category is recorded as *checked* by the
//! checkpoint itself, whether or not it repaired anything, because reaching
@ -34,13 +41,13 @@
//! (`repair_overbold_text_hierarchy` for hierarchy, the geometry loop for
//! overflow, and so on).
use std::collections::BTreeMap;
use std::sync::atomic::{AtomicUsize, Ordering};
use std::sync::Arc;
use std::collections::{BTreeMap, BTreeSet};
use std::sync::{Arc, Mutex};
use jian_ops_schema::node::PenNode;
use op_editor_core::{EditorCommand, EditorState, NodeId};
use crate::repair_record::{describe_command, PendingRepair, RepairRecord};
use crate::types::DocSink;
/// A family of quality checks, as named to the user. Declaration order is
@ -95,68 +102,124 @@ impl CheckCategory {
}
}
/// What the quality passes checked and how much they repaired, per category.
/// What the quality passes checked and every edit they applied.
///
/// The itemized [`records`](Self::records) list is the single source: the
/// per-category counts every accessor below reports are derived from it by
/// grouping, so the number in the credential and the lines under it can
/// never disagree.
#[derive(Debug, Clone, Default, PartialEq, Eq)]
pub struct RepairSummary {
/// Category -> repairs applied. A key present with value 0 means the
/// category was checked and found nothing to fix.
counts: BTreeMap<CheckCategory, usize>,
/// Categories whose passes reached a checkpoint — including those that
/// found nothing to fix, which is what lets the credential say "checked"
/// truthfully about a clean family.
checked: BTreeSet<CheckCategory>,
/// Every applied edit, in the order the passes applied it.
records: Vec<RepairRecord>,
/// Statements about the run that are NOT edits — today, the one line
/// saying a whole tier of passes was deliberately not run (see
/// `crate::repair_tier`). Kept apart from `records` on purpose: a record
/// means "one accepted apply", and a note that counted as one would make
/// the credential claim a repair that never happened.
notes: Vec<String>,
}
impl RepairSummary {
/// Record that `category`'s passes ran and applied `repairs` edits.
/// Calling with `repairs == 0` still marks the category as checked.
/// Mark `category` as checked and file `records` under it.
pub fn record_all(&mut self, category: CheckCategory, records: Vec<RepairRecord>) {
self.checked.insert(category);
self.records.extend(records);
}
/// Count-only ingestion for callers that have a number but no itemized
/// detail — a wire round-trip from an older host, or a test building a
/// summary by hand. Synthesizes placeholder records so counts stay
/// derived from the one list; the placeholders say plainly that no
/// detail was reported rather than pretending to describe an edit.
pub fn record(&mut self, category: CheckCategory, repairs: usize) {
*self.counts.entry(category).or_insert(0) += repairs;
let records = (0..repairs)
.map(|_| RepairRecord {
pass: "unattributed".to_string(),
category,
node_id: String::new(),
node_name: None,
detail: "no detail reported".to_string(),
})
.collect();
self.record_all(category, records);
}
/// Add a statement about the run that is not an edit. Repeats are dropped
/// so a driver that reaches the same gate on several roots still reports
/// the decision once.
pub fn note(&mut self, note: impl Into<String>) {
let note = note.into();
if !self.notes.contains(&note) {
self.notes.push(note);
}
}
/// Statements about the run that are not edits, in the order recorded.
pub fn notes(&self) -> &[String] {
&self.notes
}
/// Fold another summary in — used when one run drives cleanup over
/// several root batches (the orchestrator's append path).
pub fn merge(&mut self, other: &RepairSummary) {
for (category, count) in &other.counts {
self.record(*category, *count);
self.checked.extend(other.checked.iter().copied());
self.records.extend(other.records.iter().cloned());
for note in &other.notes {
self.note(note.clone());
}
}
/// True when no category was ever checked: render NO credential rather
/// than claiming a clean bill of health nobody verified.
pub fn is_empty(&self) -> bool {
self.counts.is_empty()
self.checked.is_empty()
}
/// Categories whose passes actually ran, in display order.
pub fn checked(&self) -> Vec<CheckCategory> {
self.counts.keys().copied().collect()
self.checked.iter().copied().collect()
}
/// Every applied edit, in application order.
pub fn records(&self) -> &[RepairRecord] {
&self.records
}
/// Repairs applied under `category` (0 when checked-and-clean or absent).
pub fn repairs_for(&self, category: CheckCategory) -> usize {
self.counts.get(&category).copied().unwrap_or(0)
self.records
.iter()
.filter(|record| record.category == category)
.count()
}
/// Categories that repaired something, paired with their counts, in
/// display order. Clean categories are omitted.
pub fn repaired(&self) -> Vec<(CheckCategory, usize)> {
self.counts
.iter()
.filter(|(_, count)| **count > 0)
.map(|(category, count)| (*category, *count))
.collect()
let mut counts: BTreeMap<CheckCategory, usize> = BTreeMap::new();
for record in &self.records {
*counts.entry(record.category).or_insert(0) += 1;
}
counts.into_iter().collect()
}
/// Total edits applied across every category.
pub fn total_repairs(&self) -> usize {
self.counts.values().sum()
self.records.len()
}
}
/// Shared edit counter behind a [`CountingSink`]. Held by the cleanup driver
/// alongside — never inside — the wrapped sink, so a checkpoint can read the
/// tally while the sink is still mutably borrowed.
/// Shared edit buffer behind a [`CountingSink`]. Held by the cleanup driver
/// alongside — never inside — the wrapped sink, so a checkpoint can drain the
/// buffer while the sink is still mutably borrowed.
#[derive(Debug)]
pub struct RepairCounter {
applied: Arc<AtomicUsize>,
checkpointed: usize,
pending: Arc<Mutex<Vec<PendingRepair>>>,
}
impl Default for RepairCounter {
@ -168,8 +231,7 @@ impl Default for RepairCounter {
impl RepairCounter {
pub fn new() -> Self {
Self {
applied: Arc::new(AtomicUsize::new(0)),
checkpointed: 0,
pending: Arc::new(Mutex::new(Vec::new())),
}
}
@ -177,27 +239,63 @@ impl RepairCounter {
pub fn wrap<'a>(&self, inner: &'a mut dyn DocSink) -> CountingSink<'a> {
CountingSink {
inner,
applied: self.applied.clone(),
pending: self.pending.clone(),
}
}
/// Attribute every edit since the previous checkpoint to `category`, and
/// mark `category` as checked even when the delta is 0.
pub fn checkpoint(&mut self, summary: &mut RepairSummary, category: CheckCategory) {
let total = self.applied.load(Ordering::SeqCst);
let delta = total.saturating_sub(self.checkpointed);
self.checkpointed = total;
summary.record(category, delta);
/// Attribute every edit since the previous checkpoint to `category` and
/// the pass group named by `pass`, and mark `category` as checked even
/// when nothing was applied.
///
/// `pass` names the group of passes that ran since the last checkpoint —
/// exact for a single-pass group, a family name for a multi-pass one.
/// Each attributed record is also logged at INFO so a user who pastes
/// their log can be told which pass touched which node.
pub fn checkpoint(&mut self, summary: &mut RepairSummary, category: CheckCategory, pass: &str) {
let drained: Vec<PendingRepair> = match self.pending.lock() {
Ok(mut pending) => std::mem::take(&mut *pending),
// A poisoned buffer means a pass panicked mid-apply; the tally is
// then unknowable, so record a checked-but-empty category rather
// than a number nobody can stand behind.
Err(_) => Vec::new(),
};
let records: Vec<RepairRecord> = drained
.into_iter()
.map(|repair| repair.attribute(category, pass))
.collect();
for record in &records {
tracing::info!(
pass = %record.pass,
category = %record.category.key(),
node = %record.node_id,
node_name = %record.node_name.as_deref().unwrap_or(""),
detail = %record.detail,
"quality repair applied"
);
}
summary.record_all(category, records);
}
}
/// [`DocSink`] decorator that counts accepted applies. Every method
/// [`DocSink`] decorator that records accepted applies. Every method
/// delegates verbatim — including `insert_subtree_returning_root_ids`, which
/// MUST forward rather than fall through to the trait default, or an
/// immediate-apply sink's real remapped ids would be swallowed.
///
/// The description is computed BEFORE delegating, against the pre-edit
/// document — that is the only moment the `before` half of `before → after`
/// still exists — and is kept only when the sink accepts the command.
pub struct CountingSink<'a> {
inner: &'a mut dyn DocSink,
applied: Arc<AtomicUsize>,
pending: Arc<Mutex<Vec<PendingRepair>>>,
}
impl CountingSink<'_> {
fn push(&self, repair: PendingRepair) {
if let Ok(mut pending) = self.pending.lock() {
pending.push(repair);
}
}
}
impl DocSink for CountingSink<'_> {
@ -206,9 +304,10 @@ impl DocSink for CountingSink<'_> {
}
fn apply(&mut self, cmd: EditorCommand) -> bool {
let described = describe_command(self.inner.state(), &cmd);
let accepted = self.inner.apply(cmd);
if accepted {
self.applied.fetch_add(1, Ordering::SeqCst);
self.push(described);
}
accepted
}
@ -218,11 +317,23 @@ impl DocSink for CountingSink<'_> {
nodes: Vec<PenNode>,
parent_id: &NodeId,
) -> Option<Vec<String>> {
let described = describe_command(
self.inner.state(),
&EditorCommand::InsertSubtree {
nodes: Vec::new(),
parent_id: parent_id.clone(),
page_id: None,
},
);
let count = nodes.len();
let out = self
.inner
.insert_subtree_returning_root_ids(nodes, parent_id);
if out.is_some() {
self.applied.fetch_add(1, Ordering::SeqCst);
self.push(PendingRepair {
detail: format!("inserted {count} subtree(s)"),
..described
});
}
out
}

6
crates/op-orchestrator/src/repair_summary_tests.rs
View file

@ -113,7 +113,7 @@ fn counting_sink_counts_only_accepted_applies() {
page_id: None,
}));
}
counter.checkpoint(&mut summary, CheckCategory::Structure);
counter.checkpoint(&mut summary, CheckCategory::Structure, "test-structure");
assert_eq!(
summary.repairs_for(CheckCategory::Structure),
@ -135,7 +135,7 @@ fn checkpoints_attribute_only_the_edits_since_the_previous_one() {
page_id: None,
});
}
counter.checkpoint(&mut summary, CheckCategory::Structure);
counter.checkpoint(&mut summary, CheckCategory::Structure, "test-structure");
// `InsertSubtree` remaps ids, so target the root by the id it actually
// landed under rather than the fixture's authored one.
let root_id = base.state.active_children()[0].id_str().to_string();
@ -147,7 +147,7 @@ fn checkpoints_attribute_only_the_edits_since_the_previous_one() {
page_id: None,
});
}
counter.checkpoint(&mut summary, CheckCategory::Layout);
counter.checkpoint(&mut summary, CheckCategory::Layout, "test-layout");
assert_eq!(summary.repairs_for(CheckCategory::Structure), 1);
assert_eq!(