fix(panel): close Stage 2A review round 3 (2 P1)
**P1-1 — layout invalidation could suppress the authoritative clear.** Real bug. Both render paths resolved the document identity AFTER the evaluator ran callbacks, but BOTH outcome arms carry PHASE-1 contexts. A provider that closes the primary document split changes `primary_document_window` mid-evaluation, so the filter compared phase-1 contexts against a replacement identity, matched nothing, and emitted no clear — leaving stale statusline text on the wire forever. The identity is now captured BEFORE `evaluate_statusline` runs and threaded through both paths (the terminal path via `terminal_chrome`). Pinning it took three attempts, and the two failures are the useful part: - `pmacs.window.close()` takes no argument — it closes the ACTIVE window. The first version passed a window id that was silently ignored, so it closed the panel instead of the document. - The Lua window API acts on the ACTIVE FRONTEND, so driving it against a synthetic semantic view changed nothing at all. - Closing the only document window is structurally REFUSED (Q#BP6 forbids a lone side window as a resting state), so the fixture needs TWO document windows for the close to be legal. The test now asserts its own precondition — that the callback really changed the identity — before asserting the clear, and reproduces the reported symptom (no `StatuslineSegments` at all) when the fix is reverted. **P1-2 — #21 was pinned at the helper, not the producer.** Confirmed: reverting only the call site inside `publish_buffer_snapshot_to_replicas` left both the helper test and the existing socket-pair test green. The helper assertions are removed (with a note saying why) and replaced by `snapshot_publication_follows_the_document_under_a_focused_panel`, which drives the real producer over socket pairs and asserts BOTH directions: the document buffer's snapshot is delivered while a panel holds focus, and a panel-only buffer's is not. Biting that test exposed a defect in the test itself: the delivery read had no timeout, so a regression made it HANG rather than fail. A hanging test is strictly worse than a red one — every read now has a timeout. Gates: fmt clean; workspace clippy clean; 1,832 default + 2,015 CRDT library; Stage 2A 17; Stage 1 46; statusline 8; m11_5 2; GPU initial target 14; terminal config 12; folding Stage 2 48; vterm 1/2 10 / 6; M4 121; required GPU 202; `git diff --check` clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
ccdf352258
commit
842417200a
|
|
@ -494,14 +494,14 @@ implemented and in review.**
|
|||
#173 also changes `src/editor.rs`, so gates were rerun on the merge
|
||||
result, not the old combination). Five commits: the classified census
|
||||
routing, the painter extraction + acceptance, the lane record, then
|
||||
the round-1 and round-2 review fixes. **No protocol change; no behavior
|
||||
the round-1, round-2 and round-3 review fixes. **No protocol change; no behavior
|
||||
change for any frontend today** — with `panel_capable = false` for
|
||||
semantic sessions, `primary_document_window` returns `view.active`
|
||||
in every existing configuration, so this is seam adoption that
|
||||
becomes load-bearing in 2B.
|
||||
- Verification on the merge result: `cargo fmt --check` clean; strict
|
||||
workspace Clippy clean; **1,832 default + 2,014 CRDT** library tests;
|
||||
`bottom_panel_stage2a_acceptance` **16**; bottom-panel Stage 1 46;
|
||||
workspace Clippy clean; **1,832 default + 2,015 CRDT** library tests;
|
||||
`bottom_panel_stage2a_acceptance` **17**; bottom-panel Stage 1 46;
|
||||
statusline segments 8 CRDT; m11_5 semantic 2 CRDT; GPU initial target
|
||||
14 CRDT; terminal config 12 CRDT; vterm Stage 1/2 10 / 6; folding
|
||||
Stage 2 48; M4 121; required GPU 202; `git diff --check` clean.
|
||||
|
|
@ -521,7 +521,12 @@ implemented and in review.**
|
|||
under test; (c) a discriminating fixture must make the two routings
|
||||
DISAGREE — comparing two non-terminal buffers, or two windows with no
|
||||
selection, yields the same answer either way and proves nothing.
|
||||
Round 2 found four of my own pins vacuous by exactly these shapes.
|
||||
Round 2 found four of my own pins vacuous by exactly these shapes, and
|
||||
round 3 found two more problems of the same family: a pin placed at a
|
||||
HELPER while production called it from a producer (reverting only the
|
||||
producer's call site left every test green), and a socket-pair
|
||||
assertion whose blocking read made a regression HANG instead of fail.
|
||||
Both now assert at the producer, with read timeouts on every read.
|
||||
- **Review round 1 closed: 4 P1 + 2 P2, all real.** The P1s were a
|
||||
stale-`Pointer` focus steal (the failed-alignment arm returned the
|
||||
window, so #8's activation focused it before `dispatch_pointer`
|
||||
|
|
|
|||
106
src/daemon.rs
106
src/daemon.rs
|
|
@ -3483,6 +3483,96 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
/// Bottom-panel §1.3 #21 through the REAL producer (round 3).
|
||||
///
|
||||
/// A semantic peer with a FOCUSED PANEL must still receive the
|
||||
/// snapshot for the buffer on its DOCUMENT surface, and must NOT
|
||||
/// receive one for a buffer visible only in its panel. Asserting the
|
||||
/// helper alone was insufficient: reverting the producer's call site
|
||||
/// to focused-window routing left every helper-level test green.
|
||||
#[cfg(feature = "crdt")]
|
||||
#[test]
|
||||
fn snapshot_publication_follows_the_document_under_a_focused_panel() {
|
||||
let (editor, fid, document, panel) = panel_focused_semantic_fixture();
|
||||
let (doc_buf, panel_buf) = {
|
||||
let core = editor.core.borrow();
|
||||
(
|
||||
core.windows[&document].buffer_id,
|
||||
core.windows[&panel].buffer_id,
|
||||
)
|
||||
};
|
||||
assert_ne!(doc_buf, panel_buf, "fixture: distinct buffers");
|
||||
|
||||
let caps = crate::protocol::NegotiatedCapabilities {
|
||||
multi_frontend: true,
|
||||
crdt_replica: true,
|
||||
semantic_render: true,
|
||||
};
|
||||
let mut registry = SessionRegistry::new();
|
||||
registry.register_session(
|
||||
fid,
|
||||
crate::presence::SessionState::new(PROTOCOL_VERSION, caps, 0),
|
||||
);
|
||||
|
||||
// The DOCUMENT buffer's snapshot must be delivered.
|
||||
{
|
||||
let (server, mut client) = UnixStream::pair().expect("socketpair");
|
||||
// A read timeout on the DELIVERY read too. Without it a
|
||||
// regression that suppresses the snapshot makes this test
|
||||
// HANG rather than fail, which is strictly worse than a red
|
||||
// assertion — found by biting this very test.
|
||||
client
|
||||
.set_read_timeout(Some(Duration::from_millis(500)))
|
||||
.expect("delivery timeout");
|
||||
let mut streams = HashMap::from([(fid, server)]);
|
||||
let message = InstanceMessage::BufferSnapshot {
|
||||
buffer_id: doc_buf,
|
||||
crdt_snapshot: vec![1, 2, 3],
|
||||
};
|
||||
publish_buffer_snapshot_to_replicas(
|
||||
&editor,
|
||||
doc_buf,
|
||||
&message,
|
||||
®istry,
|
||||
&mut streams,
|
||||
&mut HashMap::new(),
|
||||
);
|
||||
let delivered: InstanceMessage =
|
||||
read_message(&mut client).expect("the document snapshot must arrive");
|
||||
assert_eq!(
|
||||
delivered, message,
|
||||
"#21: a buffer on the DOCUMENT surface must still be published while a \
|
||||
panel holds focus"
|
||||
);
|
||||
}
|
||||
|
||||
// The PANEL-only buffer's snapshot must NOT be delivered.
|
||||
{
|
||||
let (server, mut client) = UnixStream::pair().expect("socketpair");
|
||||
let mut streams = HashMap::from([(fid, server)]);
|
||||
let message = InstanceMessage::BufferSnapshot {
|
||||
buffer_id: panel_buf,
|
||||
crdt_snapshot: vec![4, 5, 6],
|
||||
};
|
||||
publish_buffer_snapshot_to_replicas(
|
||||
&editor,
|
||||
panel_buf,
|
||||
&message,
|
||||
®istry,
|
||||
&mut streams,
|
||||
&mut HashMap::new(),
|
||||
);
|
||||
client
|
||||
.set_read_timeout(Some(Duration::from_millis(50)))
|
||||
.expect("timeout");
|
||||
assert!(
|
||||
read_message::<InstanceMessage>(&mut client).is_err(),
|
||||
"#21: a buffer visible only in a PANEL must not replace the peer's \
|
||||
document mirror"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// ---- GPU terminal input: the double terminal-layout sync -------------
|
||||
//
|
||||
// These drive `sync_terminal_layouts_for_tick` — the REAL dispatcher loop
|
||||
|
|
@ -4827,15 +4917,13 @@ mod tests {
|
|||
"#3: CursorByte must describe the DOCUMENT surface"
|
||||
);
|
||||
|
||||
// #21 publication recipient filter, both directions.
|
||||
assert!(
|
||||
peer_displays_buffer_as_document(&editor, fid, doc_buf),
|
||||
"#21: a buffer visible in the document must still receive publications while a panel holds focus"
|
||||
);
|
||||
assert!(
|
||||
!peer_displays_buffer_as_document(&editor, fid, panel_buf),
|
||||
"#21: a buffer visible only in a panel must NOT replace the document mirror"
|
||||
);
|
||||
// #21 is deliberately NOT asserted here. Round 3: pinning it at
|
||||
// this helper left the real producer free to regress — reverting
|
||||
// the call site inside `publish_buffer_snapshot_to_replicas`
|
||||
// kept both this test and the existing socket-pair test green.
|
||||
// It is pinned through the producer instead, in
|
||||
// `snapshot_publication_follows_the_document_under_a_focused_panel`.
|
||||
let _ = panel_buf;
|
||||
}
|
||||
|
||||
/// Bottom-panel §1.3 #2 — the sharpest census case: the lazy CRDT
|
||||
|
|
|
|||
|
|
@ -625,6 +625,22 @@ impl SemanticRenderState {
|
|||
// post-evaluation face inventory must then precede the authoritative
|
||||
// segment replacement in this same frame. Unsupported peers skip the
|
||||
// evaluator entirely and therefore pay no Lua callback/dynamic-face cost.
|
||||
// Bottom-panel A2A-2, round 3: the document identity used to
|
||||
// FILTER the results must be the PRE-CALLBACK one. Both outcome
|
||||
// arms carry phase-1 contexts, and a provider that closes the
|
||||
// primary document split changes `primary_document_window`
|
||||
// mid-evaluation — reading it after the fact would compare
|
||||
// phase-1 contexts against a replacement identity, match
|
||||
// nothing, and silently suppress the authoritative clear.
|
||||
let statusline_document_window = self
|
||||
.peer_knows_statusline_segments
|
||||
.then(|| {
|
||||
state
|
||||
.core
|
||||
.borrow()
|
||||
.primary_document_window(self.frontend_id)
|
||||
})
|
||||
.flatten();
|
||||
let statusline_evaluation = self.peer_knows_statusline_segments.then(|| {
|
||||
evaluate_statusline(
|
||||
state.lua_host.lua(),
|
||||
|
|
@ -810,11 +826,7 @@ impl SemanticRenderState {
|
|||
out.extend(self.font_facts_msg(state));
|
||||
// Q#SL6/Q#SL8: face inventory must precede segment text.
|
||||
if let Some(evaluation) = statusline_evaluation {
|
||||
let document_window = state
|
||||
.core
|
||||
.borrow()
|
||||
.primary_document_window(self.frontend_id);
|
||||
self.emit_statusline_segments(evaluation, document_window, &mut out);
|
||||
self.emit_statusline_segments(evaluation, statusline_document_window, &mut out);
|
||||
}
|
||||
out
|
||||
}
|
||||
|
|
@ -859,6 +871,16 @@ impl SemanticRenderState {
|
|||
// Evaluate callbacks before `ThemeFacts` for the same reason the
|
||||
// document path does: a callback may register a face, and the
|
||||
// face inventory must precede the segment text that names it.
|
||||
// Same pre-callback capture as the document path (round 3).
|
||||
let statusline_document_window = self
|
||||
.peer_knows_statusline_segments
|
||||
.then(|| {
|
||||
state
|
||||
.core
|
||||
.borrow()
|
||||
.primary_document_window(self.frontend_id)
|
||||
})
|
||||
.flatten();
|
||||
let statusline_evaluation = self.peer_knows_statusline_segments.then(|| {
|
||||
evaluate_statusline(
|
||||
state.lua_host.lua(),
|
||||
|
|
@ -885,7 +907,12 @@ impl SemanticRenderState {
|
|||
// a verdict we hold.
|
||||
if self.last_terminal_frame.as_ref() == Some(&frame) {
|
||||
self.terminal_error_latched = false;
|
||||
out.extend(self.terminal_chrome(state, buffer_id, statusline_evaluation));
|
||||
out.extend(self.terminal_chrome(
|
||||
state,
|
||||
buffer_id,
|
||||
statusline_evaluation,
|
||||
statusline_document_window,
|
||||
));
|
||||
return Some(out);
|
||||
}
|
||||
match frame.validate() {
|
||||
|
|
@ -909,7 +936,12 @@ impl SemanticRenderState {
|
|||
}
|
||||
}
|
||||
|
||||
out.extend(self.terminal_chrome(state, buffer_id, statusline_evaluation));
|
||||
out.extend(self.terminal_chrome(
|
||||
state,
|
||||
buffer_id,
|
||||
statusline_evaluation,
|
||||
statusline_document_window,
|
||||
));
|
||||
Some(out)
|
||||
}
|
||||
|
||||
|
|
@ -924,6 +956,7 @@ impl SemanticRenderState {
|
|||
state: &EditorState,
|
||||
buffer_id: BufferId,
|
||||
statusline_evaluation: Option<StatuslineEvaluation>,
|
||||
statusline_document_window: Option<crate::window::WindowId>,
|
||||
) -> Vec<InstanceMessage> {
|
||||
let mut out = Vec::new();
|
||||
out.extend(self.status_facts_msg(state, buffer_id));
|
||||
|
|
@ -933,11 +966,7 @@ impl SemanticRenderState {
|
|||
out.extend(self.font_facts_msg(state));
|
||||
// Q#SL6/Q#SL8: face inventory must precede segment text.
|
||||
if let Some(evaluation) = statusline_evaluation {
|
||||
let document_window = state
|
||||
.core
|
||||
.borrow()
|
||||
.primary_document_window(self.frontend_id);
|
||||
self.emit_statusline_segments(evaluation, document_window, &mut out);
|
||||
self.emit_statusline_segments(evaluation, statusline_document_window, &mut out);
|
||||
}
|
||||
out
|
||||
}
|
||||
|
|
|
|||
|
|
@ -784,3 +784,115 @@ fn consumer_decorations_follow_the_document_selection_not_the_panel() {
|
|||
"a selection living in the focused PANEL must not decorate the document viewport"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_provider_closing_the_document_split_still_clears_the_statusline() {
|
||||
use pmacs::protocol::{ByteRange, InstanceMessage};
|
||||
use pmacs::semantic_render::SemanticRenderState;
|
||||
|
||||
// Round 3 finding 1. `authoritative_empty` carries PHASE-1 contexts,
|
||||
// so the identity used to filter them must be the PRE-CALLBACK one.
|
||||
// A provider that closes the primary document split changes
|
||||
// `primary_document_window` mid-evaluation; reading it afterwards
|
||||
// compares phase-1 contexts against a replacement identity, matches
|
||||
// nothing, and silently suppresses the authoritative clear — leaving
|
||||
// stale statusline text on screen forever.
|
||||
//
|
||||
// Driven on LOCAL, because the Lua window API acts on the ACTIVE
|
||||
// FRONTEND: a synthetic semantic view would be untouched by
|
||||
// `pmacs.window.close()` and the identity would never change, which
|
||||
// is exactly how the first version of this test came back vacuous.
|
||||
// TWO document windows plus the panel: closing the only document
|
||||
// window is structurally refused (Q#BP6 forbids a lone side window
|
||||
// as a resting state), so the first attempt could not change the
|
||||
// identity at all. Distinct buffers make the target selectable from
|
||||
// a Lua provider, which has no focus-by-id.
|
||||
let s = editor();
|
||||
exec(
|
||||
&s,
|
||||
"DOC_A = pmacs.buffer.create(\"*doc-a*\")
|
||||
DOC_B = pmacs.buffer.create(\"*doc-b*\")
|
||||
pmacs.window.display(DOC_A, {})
|
||||
pmacs.window.split_horizontal()
|
||||
pmacs.window.focus_next()
|
||||
pmacs.window.display(DOC_B, {})",
|
||||
);
|
||||
let (_origin, panel) = focused_panel(&s);
|
||||
let (document, doc_buf) = {
|
||||
let core = s.core.borrow();
|
||||
let win = core
|
||||
.primary_document_window(FrontendId::LOCAL)
|
||||
.expect("a primary document window");
|
||||
(win, core.windows[&win].buffer_id)
|
||||
};
|
||||
assert_ne!(document, panel);
|
||||
|
||||
let mut sem = SemanticRenderState::for_peer(FrontendId::LOCAL, 18);
|
||||
sem.set_viewport(doc_buf, ByteRange { start: 0, end: 0 }, 0);
|
||||
|
||||
// Seed a baseline payload so a CLEAR is observable as a change.
|
||||
exec(
|
||||
&s,
|
||||
r"_G.SL_SEED = pmacs.statusline.register {
|
||||
name='seed', side='left', priority=10,
|
||||
fn=function() return 'OLD' end,
|
||||
}",
|
||||
);
|
||||
let seeded = sem.render_frame(&s);
|
||||
assert!(
|
||||
seeded
|
||||
.iter()
|
||||
.any(|m| matches!(m, InstanceMessage::StatuslineSegments { .. })),
|
||||
"non-vacuity: a baseline payload must exist before we test its clear"
|
||||
);
|
||||
|
||||
// A provider that unregisters itself (making the evaluation
|
||||
// Invalidated) AND closes the captured document window. `close()`
|
||||
// closes the ACTIVE window and Lua has no focus-by-id, so step
|
||||
// around the ring until the captured buffer is current.
|
||||
s.lua_host
|
||||
.lua()
|
||||
.globals()
|
||||
.set("TARGET_BUF", pmacs::lua_bindings::BufferIdLua(doc_buf))
|
||||
.expect("expose the target buffer");
|
||||
exec(
|
||||
&s,
|
||||
r"_G.SL_CLOSER = pmacs.statusline.register {
|
||||
name='closer', side='left', priority=100,
|
||||
fn=function()
|
||||
pmacs.statusline.unregister(SL_CLOSER)
|
||||
for _ = 1, 8 do
|
||||
if pmacs.window.buffer() == TARGET_BUF then break end
|
||||
pmacs.window.focus_next()
|
||||
end
|
||||
pmacs.window.close()
|
||||
return 'STALE'
|
||||
end,
|
||||
}",
|
||||
);
|
||||
|
||||
let msgs = sem.render_frame(&s);
|
||||
|
||||
// The fixture must actually have changed the identity, or this test
|
||||
// discriminates nothing.
|
||||
assert_ne!(
|
||||
s.core.borrow().primary_document_window(FrontendId::LOCAL),
|
||||
Some(document),
|
||||
"fixture: the callback must really have changed the document identity"
|
||||
);
|
||||
|
||||
let cleared = msgs.iter().any(|m| match m {
|
||||
InstanceMessage::StatuslineSegments {
|
||||
buffer_id,
|
||||
left,
|
||||
right,
|
||||
..
|
||||
} => *buffer_id == doc_buf && left.is_empty() && right.is_empty(),
|
||||
_ => false,
|
||||
});
|
||||
assert!(
|
||||
cleared,
|
||||
"an invalidated evaluation must still publish the authoritative EMPTY clear for \
|
||||
the phase-1 document identity, even when a callback closed that window; got {msgs:?}"
|
||||
);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue