From e066e14ee7602d6f45acb380de55e75481488575 Mon Sep 17 00:00:00 2001 From: jonasnobile Date: Fri, 18 Sep 2026 09:59:34 +0200 Subject: [PATCH 01/12] fix(markdown): keep a nested list inside the item that holds it The parser tracked lists in flat state (`in_list`, `list_ordered`, `list_items`), which can only describe one list at a time. A nested list therefore clobbered the list around it: `Start(List)` cleared the outer list's items and overwrote its ordered-ness, and the inner `End(List)` closed the outer list too. The numbering disappeared, every item after the nesting point fell out of the list and rendered as a bare paragraph, and the leftover `End(Item)` events appended empty bullets at the end. Lists and items now live on a stack, and a list item holds blocks rather than a single inline run, so an item can carry several paragraphs, a nested list or a fenced block. Follow-on fixes that fall out of it: ordered lists honour their first number (a list written `3.` no longer restarts at 1), a code block indented under an item stays in that item with syntax highlighting and its own chrome instead of being hoisted after the list, and a nested table renders in place. Code-line and table-row layout moved into `code_line_units` and `table_units` so the nested and top-level paths share the selection offset accounting. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RkbvFzpmyCRY4DBp7oJHut --- crates/okena-markdown/src/lib.rs | 21 +- crates/okena-markdown/src/parser.rs | 298 ++++++++-- crates/okena-markdown/src/render.rs | 560 +++++++++++-------- crates/okena-markdown/src/style.rs | 6 + crates/okena-markdown/src/types.rs | 15 +- crates/okena-markdown/tests/inline_layout.rs | 39 ++ 6 files changed, 652 insertions(+), 287 deletions(-) diff --git a/crates/okena-markdown/src/lib.rs b/crates/okena-markdown/src/lib.rs index 6ffe469ff..eae31ecb4 100644 --- a/crates/okena-markdown/src/lib.rs +++ b/crates/okena-markdown/src/lib.rs @@ -119,12 +119,19 @@ impl MarkdownDocument { /// colour, exactly as before. pub fn highlight_code_blocks(&mut self, is_dark: bool) { for node in &mut self.nodes { - if let Node::CodeBlock { + Self::highlight_node(node, is_dark); + } + } + + /// Walk into list items too: a fenced block inside a numbered step is a + /// block of the item, not a top-level node. + fn highlight_node(node: &mut Node, is_dark: bool) { + match node { + Node::CodeBlock { language, code, highlighted, - } = node - { + } => { *highlighted = okena_highlight::syntax::highlight_code_block( code, language.as_deref(), @@ -134,6 +141,14 @@ impl MarkdownDocument { .map(|line| line.spans) .collect(); } + Node::List { items, .. } => { + for item in items { + for block in &mut item.blocks { + Self::highlight_node(block, is_dark); + } + } + } + _ => {} } } } diff --git a/crates/okena-markdown/src/parser.rs b/crates/okena-markdown/src/parser.rs index c2e27b540..b53e47b63 100644 --- a/crates/okena-markdown/src/parser.rs +++ b/crates/okena-markdown/src/parser.rs @@ -3,7 +3,72 @@ use pulldown_cmark::{CodeBlockKind, Event, HeadingLevel, Options, Parser, Tag, TagEnd}; use super::MarkdownDocument; -use super::types::{FmValue, Frontmatter, Inline, Node}; +use super::types::{FmValue, Frontmatter, Inline, ListItem, Node}; + +/// A block container that is currently open. +/// +/// Lists nest (a list inside an item inside a list), so the parser keeps them +/// on a stack. The flat `in_list` / `list_items` state this replaced could only +/// describe one list at a time: a nested list cleared the outer list's items, +/// took its ordered-ness, and closed it early, which dropped every item after +/// the nesting point out of the list entirely. +enum Frame { + List { + ordered: bool, + start: u64, + items: Vec, + }, + Item { + blocks: Vec, + }, +} + +/// Route a finished block to the innermost open list item, or to the document +/// root when no item is open. +fn push_block(nodes: &mut Vec, frames: &mut [Frame], node: Node) { + match frames.last_mut() { + Some(Frame::Item { blocks }) => blocks.push(node), + _ => nodes.push(node), + } +} + +/// Turn the inline text collected directly under the innermost item into a +/// paragraph block. +/// +/// A *tight* list item (no blank line between items) carries its text as bare +/// inline events with no `Paragraph` around it, so the text has to be closed off +/// by hand: before any block opens inside the item, and when the item ends. +fn flush_item_inlines(inline_stack: &mut [Vec], frames: &mut [Frame]) { + let Some(Frame::Item { blocks }) = frames.last_mut() else { + return; + }; + let Some(pending) = inline_stack.last_mut() else { + return; + }; + if pending.is_empty() { + return; + } + blocks.push(Node::Paragraph { + children: std::mem::take(pending), + }); +} + +/// Whether an event opens or closes a block inside a list item, and so has to +/// be preceded by [`flush_item_inlines`]. +fn is_item_block_boundary(event: &Event) -> bool { + matches!( + event, + Event::Start( + Tag::Paragraph + | Tag::Heading { .. } + | Tag::CodeBlock(_) + | Tag::List(_) + | Tag::BlockQuote(_) + | Tag::Table(_) + ) | Event::End(TagEnd::Item) + | Event::Rule + ) +} /// Append `text` to the innermost inline run, merging it into the preceding text /// rather than starting a new one. The renderer lays each run out as its own @@ -54,9 +119,8 @@ impl MarkdownDocument { let mut in_code_block = false; let mut code_block_lang: Option = None; let mut code_block_content = String::new(); - let mut in_list = false; - let mut list_ordered = false; - let mut list_items: Vec> = Vec::new(); + // Open list/item containers, innermost last. + let mut frames: Vec = Vec::new(); let mut in_blockquote = false; let mut in_table = false; let mut in_table_head = false; @@ -65,6 +129,9 @@ impl MarkdownDocument { let mut current_row: Vec> = Vec::new(); for event in parser { + if is_item_block_boundary(&event) { + flush_item_inlines(&mut inline_stack, &mut frames); + } match event { // Block elements Event::Start(Tag::Heading { level, .. }) => { @@ -81,7 +148,7 @@ impl MarkdownDocument { Event::End(TagEnd::Heading(_)) => { if let Some(level) = in_heading.take() { let children = inline_stack.pop().unwrap_or_default(); - nodes.push(Node::Heading { level, children }); + push_block(&mut nodes, &mut frames, Node::Heading { level, children }); } } Event::Start(Tag::Paragraph) => { @@ -90,23 +157,13 @@ impl MarkdownDocument { } Event::End(TagEnd::Paragraph) if in_paragraph => { let children = inline_stack.pop().unwrap_or_default(); - if in_blockquote { - // Add to blockquote - if let Some(last) = inline_stack.last_mut() { - last.extend(children); - } - } else if in_list { - // Will be collected by Item end - if let Some(last) = inline_stack.last_mut() { - last.extend(children); - } - } else if in_table { - // Table cell content + if in_blockquote || in_table { + // Collected by the blockquote / table-cell end instead. if let Some(last) = inline_stack.last_mut() { last.extend(children); } } else { - nodes.push(Node::Paragraph { children }); + push_block(&mut nodes, &mut frames, Node::Paragraph { children }); } in_paragraph = false; } @@ -119,31 +176,56 @@ impl MarkdownDocument { code_block_content.clear(); } Event::End(TagEnd::CodeBlock) => { - nodes.push(Node::CodeBlock { - language: code_block_lang.take(), - code: std::mem::take(&mut code_block_content), - highlighted: Vec::new(), - }); + push_block( + &mut nodes, + &mut frames, + Node::CodeBlock { + language: code_block_lang.take(), + code: std::mem::take(&mut code_block_content), + highlighted: Vec::new(), + }, + ); in_code_block = false; } Event::Start(Tag::List(first_item)) => { - in_list = true; - list_ordered = first_item.is_some(); - list_items.clear(); + frames.push(Frame::List { + ordered: first_item.is_some(), + start: first_item.unwrap_or(1), + items: Vec::new(), + }); } Event::End(TagEnd::List(_)) => { - nodes.push(Node::List { - ordered: list_ordered, - items: std::mem::take(&mut list_items), - }); - in_list = false; + if let Some(Frame::List { + ordered, + start, + items, + }) = frames.pop() + { + push_block( + &mut nodes, + &mut frames, + Node::List { + ordered, + start, + items, + }, + ); + } } Event::Start(Tag::Item) => { + frames.push(Frame::Item { blocks: Vec::new() }); + // Holds text written straight into the item (a tight list); + // `flush_item_inlines` turns it into a paragraph block. inline_stack.push(Vec::new()); } Event::End(TagEnd::Item) => { - let children = inline_stack.pop().unwrap_or_default(); - list_items.push(children); + // Emptied by the flush that ran for this event. + inline_stack.pop(); + if let Some(Frame::Item { blocks }) = frames.pop() + && let Some(Frame::List { items, .. }) = frames.last_mut() + { + items.push(ListItem { blocks }); + } } Event::Start(Tag::BlockQuote(_)) => { in_blockquote = true; @@ -151,11 +233,11 @@ impl MarkdownDocument { } Event::End(TagEnd::BlockQuote(_)) => { let children = inline_stack.pop().unwrap_or_default(); - nodes.push(Node::Blockquote { children }); + push_block(&mut nodes, &mut frames, Node::Blockquote { children }); in_blockquote = false; } Event::Rule => { - nodes.push(Node::HorizontalRule); + push_block(&mut nodes, &mut frames, Node::HorizontalRule); } // Table elements @@ -168,11 +250,15 @@ impl MarkdownDocument { let headers = std::mem::take(&mut table_headers); let rows = std::mem::take(&mut table_rows); let col_widths = Self::table_col_widths(&headers, &rows); - nodes.push(Node::Table { - headers, - rows, - col_widths, - }); + push_block( + &mut nodes, + &mut frames, + Node::Table { + headers, + rows, + col_widths, + }, + ); in_table = false; } Event::Start(Tag::TableHead) => { @@ -295,8 +381,9 @@ impl MarkdownDocument { } Node::List { items, .. } => { for item in items { - Self::inlines_to_flat_text(item, text); - text.push('\n'); + for block in &item.blocks { + Self::node_to_flat_text(block, text); + } } } Node::Table { headers, rows, .. } => { @@ -473,9 +560,134 @@ let x = 1; assert_eq!(doc.node_offsets.first().copied(), Some(0)); } - use super::super::types::{FmValue, Frontmatter, Node}; + use super::super::types::{FmValue, Frontmatter, ListItem, Node}; use super::split_frontmatter; + fn expect_list(node: &Node) -> (bool, u64, &[ListItem]) { + match node { + Node::List { + ordered, + start, + items, + } => (*ordered, *start, items), + _ => panic!("expected a list"), + } + } + + fn item_text(item: &ListItem) -> String { + let mut out = String::new(); + for block in &item.blocks { + MarkdownDocument::node_to_flat_text(block, &mut out); + } + out + } + + /// A nested list used to clobber the list around it: the inner `Start(List)` + /// reset the single flat list state, so the outer list lost its items, took + /// the inner list's bullet marker, and closed early. Every item after the + /// nesting point fell out of the list and rendered as a bare paragraph. + #[test] + fn nested_list_keeps_the_list_around_it_intact() { + let content = "\ +1. First question. + +2. Second, with sub-points: + - changed since + - paging + +3. Third question. + +4. Fourth question. +"; + let doc = MarkdownDocument::parse(content); + + // The whole thing is one top-level list: nothing leaked out of it. + assert_eq!(doc.nodes.len(), 1, "expected a single top-level list"); + let (ordered, start, items) = expect_list(&doc.nodes[0]); + assert!(ordered, "the outer list is numbered"); + assert_eq!(start, 1); + assert_eq!(items.len(), 4); + + // Item 2 holds its own text plus the nested list, in that order. + assert_eq!(items[1].blocks.len(), 2); + assert!(matches!(items[1].blocks[0], Node::Paragraph { .. })); + let (inner_ordered, _, inner_items) = expect_list(&items[1].blocks[1]); + assert!(!inner_ordered, "the nested list is a bullet list"); + assert_eq!(inner_items.len(), 2); + + // The items after the nesting point are still items, with their text. + assert_eq!(item_text(&items[2]), "Third question.\n"); + assert_eq!(item_text(&items[3]), "Fourth question.\n"); + } + + /// Tight items (no blank line between them) carry their text as bare inline + /// events; each still ends up as one paragraph block inside its item. + #[test] + fn tight_and_multi_paragraph_items_become_blocks() { + let doc = MarkdownDocument::parse("- one\n- two\n - nested\n"); + let (_, _, items) = expect_list(&doc.nodes[0]); + assert_eq!(items.len(), 2); + assert_eq!(item_text(&items[0]), "one\n"); + assert_eq!(items[1].blocks.len(), 2, "text plus the nested list"); + + let doc = MarkdownDocument::parse("- first para\n\n second para\n"); + let (_, _, items) = expect_list(&doc.nodes[0]); + assert_eq!(items[0].blocks.len(), 2); + assert_eq!(item_text(&items[0]), "first para\nsecond para\n"); + } + + /// A fenced block indented under an item belongs to that item. It used to be + /// hoisted to the document root and drawn after the list it sat inside. + #[test] + fn code_block_stays_inside_its_item() { + let doc = MarkdownDocument::parse("1. Run it:\n\n ```sh\n cargo test\n ```\n"); + assert_eq!(doc.nodes.len(), 1); + let (_, _, items) = expect_list(&doc.nodes[0]); + assert!(matches!( + items[0].blocks.as_slice(), + [Node::Paragraph { .. }, Node::CodeBlock { .. }] + )); + } + + /// Markers follow the source numbering rather than always restarting at 1. + #[test] + fn ordered_list_keeps_its_first_number() { + let doc = MarkdownDocument::parse("3. three\n4. four\n"); + let (ordered, start, items) = expect_list(&doc.nodes[0]); + assert!(ordered); + assert_eq!(start, 3); + assert_eq!(items.len(), 2); + } + + /// Selection maps a character offset onto `plain_text`, so every node's + /// reported length must add up to it, nested blocks included. + #[test] + fn nested_block_lengths_match_the_flat_text() { + let content = "\ +# Title + +1. First + +2. Second: + - a + - b + + ```sh + run me + ``` + +3. Third +"; + let doc = MarkdownDocument::parse(content); + let total: usize = doc + .nodes + .iter() + .map(MarkdownDocument::node_text_length) + .sum(); + assert_eq!(total, doc.plain_text.chars().count()); + assert!(doc.plain_text.contains("run me")); + } + #[test] fn detects_frontmatter_and_keeps_markdown() { let content = "\ diff --git a/crates/okena-markdown/src/render.rs b/crates/okena-markdown/src/render.rs index 6a7349d3d..1d593fa82 100644 --- a/crates/okena-markdown/src/render.rs +++ b/crates/okena-markdown/src/render.rs @@ -6,13 +6,14 @@ use gpui_component::{h_flex, v_flex}; use okena_core::theme::ThemeColors; use okena_highlight::styled::build_styled_text_with_backgrounds; use okena_highlight::syntax::HighlightedSpan; +use okena_ui::code_block::code_block_container; use okena_ui::tokens::ui_text_md; use super::style::{ - MdColors, body_line_height, body_size, heading_style, inline_code_size, node_spacing, - table_line_height, + MdColors, body_line_height, body_size, code_block_size, heading_style, inline_code_size, + node_spacing, table_line_height, }; -use super::types::{FmValue, Frontmatter, Inline, Node, char_len}; +use super::types::{FmValue, Frontmatter, Inline, ListItem, Node, char_len}; use super::{MarkdownDocument, MarkdownTextRun, RenderedNode, RenderedTextUnit}; /// Height of one code line. Code blocks are laid out line by line (each line is @@ -186,217 +187,20 @@ impl MarkdownDocument { language, code, highlighted, - } => { - // Return code blocks with individual lines for per-line selection - let selection_bg = rgba(0x3390ff40); - let mut lines = Vec::new(); - let mut line_offset = offset; - - for (line_idx, line) in code.lines().enumerate() { - let line_len = char_len(line); - let line_end = line_offset + line_len + 1; // +1 for newline - - let line_sel = node_selection.and_then(|(s, e)| { - let rel_offset = line_offset - offset; - let rel_end = rel_offset + line_len + 1; - if e <= rel_offset || s >= rel_end { - None - } else { - Some((s.saturating_sub(rel_offset), (e - rel_offset).min(line_len))) - } - }); - - let spans = highlighted.get(line_idx).map(Vec::as_slice).unwrap_or(&[]); - let display_line = if line.is_empty() { " " } else { line }; - let styled = if !spans.is_empty() { - highlighted_code_line(line, spans, line_sel, selection_bg) - } else { - plain_text_run(display_line, line_sel, selection_bg) - }; - let text_runs = vec![MarkdownTextRun::new( - styled.layout().clone(), - line.to_string(), - line_offset, - )]; - let line_div = div().h(CODE_LINE_HEIGHT).child(styled); - - lines.push(RenderedTextUnit { - div: line_div, - start_offset: line_offset, - end_offset: line_end, - text_runs, - }); - line_offset = line_end; - } - - RenderedNode::CodeBlock { - language: language.clone(), - lines, - } - } + } => RenderedNode::CodeBlock { + language: language.clone(), + // Individual lines, for per-line selection. + lines: Self::code_line_units(code, highlighted, node_selection, offset), + }, Node::Table { headers, rows, col_widths, } => { - // Return tables with individual rows for per-row selection. - // Column widths are precomputed at parse time. - let c = MdColors::new(t); - let mut row_offset = offset; - let mut rendered_rows = Vec::new(); - let mut rendered_header = None; - - // Header row - if !headers.is_empty() { - let header_len: usize = headers - .iter() - .map(|h| Self::inlines_text_length(h)) - .sum::() - + headers.len().saturating_sub(1) - + 1; // tabs + newline - let header_end = row_offset + header_len; - - let header_sel = node_selection.and_then(|(s, e)| { - let rel_start = row_offset - offset; - let rel_end = rel_start + header_len; - if e <= rel_start || s >= rel_end { - None - } else { - Some((s.saturating_sub(rel_start), (e - rel_start).min(header_len))) - } - }); - - let mut header_row = h_flex(); - let mut header_runs = Vec::new(); - let mut cell_offset = 0usize; - for (i, header) in headers.iter().enumerate() { - let cell_len = - Self::inlines_text_length(header) + if i > 0 { 1 } else { 0 }; - let cell_sel = header_sel.and_then(|(s, e)| { - let cell_start = cell_offset + if i > 0 { 1 } else { 0 }; - let cell_end = cell_offset + cell_len; - if e <= cell_start || s >= cell_end { - None - } else { - Some(( - s.saturating_sub(cell_start), - (e - cell_start).min(Self::inlines_text_length(header)), - )) - } - }); - - let width = col_widths.get(i).copied().unwrap_or(10); - let min_w = ((width * 8) + 24).max(80) as f32; - header_row = header_row.child( - div().min_w(px(min_w)).px(px(12.0)).py(px(8.0)).child( - Self::render_inlines_with_selection_and_targets( - header, - t, - cx, - cell_sel, - row_offset + cell_offset + if i > 0 { 1 } else { 0 }, - &mut header_runs, - ) - .text_size(ui_text_md(cx)) - .line_height(table_line_height(cx)) - .font_weight(FontWeight::SEMIBOLD) - .text_color(rgb(c.heading)), - ), - ); - cell_offset += cell_len; - } - - let header_div = header_row - .bg(rgb(c.surface)) - .border_b_1() - .border_color(rgb(c.surface_border)); - rendered_header = Some(RenderedTextUnit { - div: header_div, - start_offset: row_offset, - end_offset: header_end, - text_runs: header_runs, - }); - row_offset = header_end; - } - - // Data rows - for (row_idx, row) in rows.iter().enumerate() { - let row_len: usize = row - .iter() - .map(|cell| Self::inlines_text_length(cell)) - .sum::() - + row.len().saturating_sub(1) - + 1; // tabs + newline - let row_end = row_offset + row_len; - - let row_sel = node_selection.and_then(|(s, e)| { - let rel_start = row_offset - offset; - let rel_end = rel_start + row_len; - if e <= rel_start || s >= rel_end { - None - } else { - Some((s.saturating_sub(rel_start), (e - rel_start).min(row_len))) - } - }); - - let mut row_div = h_flex(); - let mut row_runs = Vec::new(); - if row_idx % 2 == 1 { - row_div = row_div.bg(rgb(c.surface)); - } - if row_idx < rows.len() - 1 { - row_div = row_div.border_b_1().border_color(rgb(c.surface_border)); - } - - let mut cell_offset = 0usize; - for (i, cell) in row.iter().enumerate() { - let cell_len = Self::inlines_text_length(cell) + if i > 0 { 1 } else { 0 }; - let cell_sel = row_sel.and_then(|(s, e)| { - let cell_start = cell_offset + if i > 0 { 1 } else { 0 }; - let cell_end = cell_offset + cell_len; - if e <= cell_start || s >= cell_end { - None - } else { - Some(( - s.saturating_sub(cell_start), - (e - cell_start).min(Self::inlines_text_length(cell)), - )) - } - }); - - let width = col_widths.get(i).copied().unwrap_or(10); - let min_w = ((width * 8) + 24).max(80) as f32; - row_div = row_div.child( - div().min_w(px(min_w)).px(px(12.0)).py(px(6.0)).child( - Self::render_inlines_with_selection_and_targets( - cell, - t, - cx, - cell_sel, - row_offset + cell_offset + if i > 0 { 1 } else { 0 }, - &mut row_runs, - ) - .text_size(ui_text_md(cx)) - .line_height(table_line_height(cx)) - .text_color(rgb(c.body)), - ), - ); - cell_offset += cell_len; - } - - rendered_rows.push(RenderedTextUnit { - div: row_div, - start_offset: row_offset, - end_offset: row_end, - text_runs: row_runs, - }); - row_offset = row_end; - } - - RenderedNode::Table { - header: rendered_header, - rows: rendered_rows, - } + // Individual rows, for per-row selection. + let (header, rows) = + Self::table_units(headers, rows, col_widths, t, cx, node_selection, offset); + RenderedNode::Table { header, rows } } _ => { // Other nodes are simple blocks @@ -421,6 +225,229 @@ impl MarkdownDocument { Some(rendered) } + /// Lay out a code block line by line, each line its own selectable unit. + /// + /// `selection` is a character range relative to the start of the block; + /// `base_offset` is the block's own offset in the document's flat text. + fn code_line_units( + code: &str, + highlighted: &[Vec], + selection: Option<(usize, usize)>, + base_offset: usize, + ) -> Vec { + let selection_bg = rgba(0x3390ff40); + let mut lines = Vec::new(); + let mut line_offset = base_offset; + + for (line_idx, line) in code.lines().enumerate() { + let line_len = char_len(line); + let line_end = line_offset + line_len + 1; // +1 for newline + + let line_sel = selection.and_then(|(s, e)| { + let rel_offset = line_offset - base_offset; + let rel_end = rel_offset + line_len + 1; + if e <= rel_offset || s >= rel_end { + None + } else { + Some((s.saturating_sub(rel_offset), (e - rel_offset).min(line_len))) + } + }); + + let spans = highlighted.get(line_idx).map(Vec::as_slice).unwrap_or(&[]); + let display_line = if line.is_empty() { " " } else { line }; + let styled = if !spans.is_empty() { + highlighted_code_line(line, spans, line_sel, selection_bg) + } else { + plain_text_run(display_line, line_sel, selection_bg) + }; + let text_runs = vec![MarkdownTextRun::new( + styled.layout().clone(), + line.to_string(), + line_offset, + )]; + let line_div = div().h(CODE_LINE_HEIGHT).child(styled); + + lines.push(RenderedTextUnit { + div: line_div, + start_offset: line_offset, + end_offset: line_end, + text_runs, + }); + line_offset = line_end; + } + + lines + } + + /// Lay out a table row by row, each row its own selectable unit. Column + /// widths come precomputed from parse time. + /// + /// `selection` is relative to the start of the table; `base_offset` is the + /// table's offset in the document's flat text. + #[allow(clippy::too_many_arguments)] + fn table_units( + headers: &[Vec], + rows: &[Vec>], + col_widths: &[usize], + t: &ThemeColors, + cx: &App, + selection: Option<(usize, usize)>, + base_offset: usize, + ) -> (Option, Vec) { + let c = MdColors::new(t); + let mut row_offset = base_offset; + let mut rendered_rows = Vec::new(); + let mut rendered_header = None; + + // Header row + if !headers.is_empty() { + let header_len: usize = headers + .iter() + .map(|h| Self::inlines_text_length(h)) + .sum::() + + headers.len().saturating_sub(1) + + 1; // tabs + newline + let header_end = row_offset + header_len; + + let header_sel = selection.and_then(|(s, e)| { + let rel_start = row_offset - base_offset; + let rel_end = rel_start + header_len; + if e <= rel_start || s >= rel_end { + None + } else { + Some((s.saturating_sub(rel_start), (e - rel_start).min(header_len))) + } + }); + + let mut header_row = h_flex(); + let mut header_runs = Vec::new(); + let mut cell_offset = 0usize; + for (i, header) in headers.iter().enumerate() { + let cell_len = Self::inlines_text_length(header) + if i > 0 { 1 } else { 0 }; + let cell_sel = header_sel.and_then(|(s, e)| { + let cell_start = cell_offset + if i > 0 { 1 } else { 0 }; + let cell_end = cell_offset + cell_len; + if e <= cell_start || s >= cell_end { + None + } else { + Some(( + s.saturating_sub(cell_start), + (e - cell_start).min(Self::inlines_text_length(header)), + )) + } + }); + + let width = col_widths.get(i).copied().unwrap_or(10); + let min_w = ((width * 8) + 24).max(80) as f32; + header_row = header_row.child( + div().min_w(px(min_w)).px(px(12.0)).py(px(8.0)).child( + Self::render_inlines_with_selection_and_targets( + header, + t, + cx, + cell_sel, + row_offset + cell_offset + if i > 0 { 1 } else { 0 }, + &mut header_runs, + ) + .text_size(ui_text_md(cx)) + .line_height(table_line_height(cx)) + .font_weight(FontWeight::SEMIBOLD) + .text_color(rgb(c.heading)), + ), + ); + cell_offset += cell_len; + } + + let header_div = header_row + .bg(rgb(c.surface)) + .border_b_1() + .border_color(rgb(c.surface_border)); + rendered_header = Some(RenderedTextUnit { + div: header_div, + start_offset: row_offset, + end_offset: header_end, + text_runs: header_runs, + }); + row_offset = header_end; + } + + // Data rows + for (row_idx, row) in rows.iter().enumerate() { + let row_len: usize = row + .iter() + .map(|cell| Self::inlines_text_length(cell)) + .sum::() + + row.len().saturating_sub(1) + + 1; // tabs + newline + let row_end = row_offset + row_len; + + let row_sel = selection.and_then(|(s, e)| { + let rel_start = row_offset - base_offset; + let rel_end = rel_start + row_len; + if e <= rel_start || s >= rel_end { + None + } else { + Some((s.saturating_sub(rel_start), (e - rel_start).min(row_len))) + } + }); + + let mut row_div = h_flex(); + let mut row_runs = Vec::new(); + if row_idx % 2 == 1 { + row_div = row_div.bg(rgb(c.surface)); + } + if row_idx < rows.len() - 1 { + row_div = row_div.border_b_1().border_color(rgb(c.surface_border)); + } + + let mut cell_offset = 0usize; + for (i, cell) in row.iter().enumerate() { + let cell_len = Self::inlines_text_length(cell) + if i > 0 { 1 } else { 0 }; + let cell_sel = row_sel.and_then(|(s, e)| { + let cell_start = cell_offset + if i > 0 { 1 } else { 0 }; + let cell_end = cell_offset + cell_len; + if e <= cell_start || s >= cell_end { + None + } else { + Some(( + s.saturating_sub(cell_start), + (e - cell_start).min(Self::inlines_text_length(cell)), + )) + } + }); + + let width = col_widths.get(i).copied().unwrap_or(10); + let min_w = ((width * 8) + 24).max(80) as f32; + row_div = row_div.child( + div().min_w(px(min_w)).px(px(12.0)).py(px(6.0)).child( + Self::render_inlines_with_selection_and_targets( + cell, + t, + cx, + cell_sel, + row_offset + cell_offset + if i > 0 { 1 } else { 0 }, + &mut row_runs, + ) + .text_size(ui_text_md(cx)) + .line_height(table_line_height(cx)) + .text_color(rgb(c.body)), + ), + ); + cell_offset += cell_len; + } + + rendered_rows.push(RenderedTextUnit { + div: row_div, + start_offset: row_offset, + end_offset: row_end, + text_runs: row_runs, + }); + row_offset = row_end; + } + + (rendered_header, rendered_rows) + } + /// Calculate the text length of a node (for selection offset tracking, in characters). pub(crate) fn node_text_length(node: &Node) -> usize { match node { @@ -436,10 +463,7 @@ impl MarkdownDocument { .sum::() .max(1) } - Node::List { items, .. } => items - .iter() - .map(|item| Self::inlines_text_length(item) + 1) - .sum(), + Node::List { items, .. } => items.iter().map(Self::list_item_text_length).sum(), Node::Table { headers, rows, .. } => { let header_len: usize = headers.iter().map(|h| Self::inlines_text_length(h)).sum::() + headers.len().saturating_sub(1) // tabs @@ -459,6 +483,12 @@ impl MarkdownDocument { } } + /// Text length of one list item: the sum of its blocks, each of which + /// already accounts for its own trailing newline. + pub(crate) fn list_item_text_length(item: &ListItem) -> usize { + item.blocks.iter().map(Self::node_text_length).sum() + } + /// Calculate the text length of inline elements (in characters, not bytes). pub(crate) fn inlines_text_length(inlines: &[Inline]) -> usize { inlines @@ -511,7 +541,11 @@ impl MarkdownDocument { text_runs, ) .w_full(), - Node::List { ordered, items } => { + Node::List { + ordered, + start, + items, + } => { // One marker column for both list kinds, right-aligned in it, so // the text hangs at the same indent whatever the marker is — // including two-digit numbers. @@ -519,24 +553,35 @@ impl MarkdownDocument { let mut list = v_flex().w_full().gap(px(6.0)).pl(px(4.0)); let mut offset = 0usize; - for (i, item_inlines) in items.iter().enumerate() { - let item_len = Self::inlines_text_length(item_inlines) + 1; - let item_sel = selection.and_then(|(s, e)| { - if e <= offset || s >= offset + item_len { - None - } else { - Some(( - s.saturating_sub(offset), - (e - offset).min(item_len - 1), // -1 to exclude newline - )) - } - }); + for (i, item) in items.iter().enumerate() { + let item_len = Self::list_item_text_length(item); + let item_sel = sub_selection(selection, offset, item_len); let marker = if *ordered { - format!("{}.", i + 1) + // Honour the source numbering: a list written `3.` first + // keeps starting at 3. + format!("{}.", start.saturating_add(i as u64)) } else { "\u{2022}".to_string() }; + + // An item is a block container: several paragraphs, a code + // block, or a nested list all stack in this column. + let mut content = v_flex().flex_1().min_w_0().gap(px(6.0)); + let mut block_offset = 0usize; + for block in &item.blocks { + let block_len = Self::node_text_length(block); + content = content.child(Self::render_node_with_selection( + block, + t, + cx, + sub_selection(item_sel, block_offset, block_len), + base_offset + offset + block_offset, + text_runs, + )); + block_offset += block_len; + } + list = list.child( div() .flex() @@ -554,17 +599,7 @@ impl MarkdownDocument { .text_right() .child(marker), ) - .child( - Self::render_inlines_with_selection_and_targets( - item_inlines, - t, - cx, - item_sel, - base_offset + offset, - text_runs, - ) - .flex_1(), - ), + .child(content), ); offset += item_len; } @@ -593,8 +628,53 @@ impl MarkdownDocument { Node::Frontmatter { block, .. } => { Self::render_frontmatter(block, t, cx, selection, base_offset, text_runs) } - // These are rendered by the specialized branches in `render_node`. - Node::CodeBlock { .. } | Node::Table { .. } => div(), + // A top-level code block or table is drawn by `render_node`, which + // hands the viewer its lines/rows as separate selectable units. One + // nested in a list item has no such unit of its own, so it is drawn + // here (chrome included), and folds its runs into the parent's. + Node::CodeBlock { + language, + code, + highlighted, + } => { + let mut lines = Vec::new(); + for unit in Self::code_line_units(code, highlighted, selection, base_offset) { + text_runs.extend(unit.text_runs); + lines.push(unit.div); + } + code_block_container(language.as_deref(), t, cx) + .w_full() + .child( + v_flex() + .px(px(12.0)) + .py(px(8.0)) + .font_family("monospace") + .text_size(code_block_size(cx)) + .text_color(rgb(c.body)) + .children(lines), + ) + } + Node::Table { + headers, + rows, + col_widths, + } => { + let (header, rows) = + Self::table_units(headers, rows, col_widths, t, cx, selection, base_offset); + let mut units = Vec::new(); + for unit in header.into_iter().chain(rows) { + text_runs.extend(unit.text_runs); + units.push(unit.div); + } + v_flex() + .items_start() + .max_w_full() + .overflow_hidden() + .rounded(px(6.0)) + .border_1() + .border_color(rgb(c.surface_border)) + .children(units) + } } } diff --git a/crates/okena-markdown/src/style.rs b/crates/okena-markdown/src/style.rs index 61b3646cf..4213ebc61 100644 --- a/crates/okena-markdown/src/style.rs +++ b/crates/okena-markdown/src/style.rs @@ -35,6 +35,12 @@ pub(crate) fn inline_code_size(cx: &App) -> Pixels { ui_text(INLINE_CODE_PT, cx) } +/// Code size for blocks this crate draws itself, one nested inside a list +/// item, say. A top-level block is sized by the viewer's own file font size. +pub(crate) fn code_block_size(cx: &App) -> Pixels { + ui_text(13.0, cx) +} + /// Table cells run at UI size, with their own leading — body leading would make /// a dense table too airy. pub(crate) fn table_line_height(cx: &App) -> Pixels { diff --git a/crates/okena-markdown/src/types.rs b/crates/okena-markdown/src/types.rs index 79d836fd6..f63798b8d 100644 --- a/crates/okena-markdown/src/types.rs +++ b/crates/okena-markdown/src/types.rs @@ -24,7 +24,10 @@ pub(crate) enum Node { }, List { ordered: bool, - items: Vec>, + /// First number of an ordered list: `3.` starts the markers at 3. + /// Always 1 for a bullet list. + start: u64, + items: Vec, }, Table { headers: Vec>, @@ -48,6 +51,16 @@ pub(crate) enum Node { }, } +/// One item of a list, as a sequence of blocks rather than a single inline run. +/// +/// An item is a block container in markdown: it can hold several paragraphs, a +/// code block, or (the case that matters most) a nested list. Flattening it to +/// inlines is what used to make a nested list overwrite the list containing it. +#[derive(Clone)] +pub(crate) struct ListItem { + pub(crate) blocks: Vec, +} + /// Parsed YAML frontmatter block. #[derive(Clone)] pub(crate) enum Frontmatter { diff --git a/crates/okena-markdown/tests/inline_layout.rs b/crates/okena-markdown/tests/inline_layout.rs index 5a6248ae5..1f5acd0fc 100644 --- a/crates/okena-markdown/tests/inline_layout.rs +++ b/crates/okena-markdown/tests/inline_layout.rs @@ -222,6 +222,45 @@ fn table_text_runs_follow_flat_text_offsets(cx: &mut TestAppContext) { assert_eq!(doc.plain_text, "Hé\tB🙂\none\ttwo\n"); } +/// A nested list renders inside the item that holds it, and the text runs of +/// every block (nested ones included) still carry their global character +/// offsets, which is what selection and copy are built on. +#[gpui::test] +fn nested_list_text_runs_follow_flat_text_offsets(cx: &mut TestAppContext) { + let doc = MarkdownDocument::parse("1. First\n\n2. Second:\n - a\n - b\n\n3. Third\n"); + assert_eq!(doc.plain_text, "First\nSecond:\na\nb\nThird\n"); + // One list, not a list plus the paragraphs that used to fall out of it. + assert_eq!(doc.node_count(), 1); + + let captured: Rc>> = Default::default(); + let captured_for_draw = captured.clone(); + let vcx = cx.add_empty_window(); + + vcx.draw( + Point::default(), + Size { + width: AvailableSpace::Definite(px(500.0)), + height: AvailableSpace::MinContent, + }, + |_window, cx| { + let Some(RenderedNode::Simple { div, text_runs, .. }) = + doc.render_node(0, &DARK_THEME, cx, None) + else { + return div(); + }; + captured_for_draw.borrow_mut().extend(text_runs); + div + }, + ); + + let starts = captured + .borrow() + .iter() + .map(run_start_offset) + .collect::>(); + assert_eq!(starts, [0, 6, 14, 16, 18]); +} + #[gpui::test] fn frontmatter_text_runs_follow_flat_text_offsets(cx: &mut TestAppContext) { let doc = MarkdownDocument::parse("---\ntitle: Žluť\nitems:\n - one\n---\n"); From 9aeab9ea85401e646ef96cec90f1292ba041089f Mon Sep 17 00:00:00 2001 From: jonasnobile Date: Fri, 18 Sep 2026 10:11:53 +0200 Subject: [PATCH 02/12] fix(markdown): render the blocks a blockquote holds A quote collected only its inline content, which is the same modelling mistake list items had. Every quoted paragraph merged onto one line, and a list inside a quote was emitted after it as a separate top-level node, because the quote was not on the block stack that routes finished blocks. `Node::Blockquote` now holds blocks and a quote is a `Frame` like a list item, so quotes and lists nest in each other either way round. Quoted text reads dimmer and italic, and a paragraph sets its own colour, so an ancestor cannot paint over it. That style now travels down as a `BlockStyle` the container hands to the blocks inside it, which also drops the hardcoded body colour from the inline renderer. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RkbvFzpmyCRY4DBp7oJHut --- crates/okena-markdown/src/parser.rs | 76 ++++++++++++---- crates/okena-markdown/src/render.rs | 93 +++++++++++++++----- crates/okena-markdown/src/types.rs | 5 +- crates/okena-markdown/tests/inline_layout.rs | 37 ++++++++ 4 files changed, 174 insertions(+), 37 deletions(-) diff --git a/crates/okena-markdown/src/parser.rs b/crates/okena-markdown/src/parser.rs index b53e47b63..d1399e252 100644 --- a/crates/okena-markdown/src/parser.rs +++ b/crates/okena-markdown/src/parser.rs @@ -21,13 +21,16 @@ enum Frame { Item { blocks: Vec, }, + Blockquote { + blocks: Vec, + }, } -/// Route a finished block to the innermost open list item, or to the document -/// root when no item is open. +/// Route a finished block to the innermost open block container (a list item or +/// a quote), or to the document root when none is open. fn push_block(nodes: &mut Vec, frames: &mut [Frame], node: Node) { match frames.last_mut() { - Some(Frame::Item { blocks }) => blocks.push(node), + Some(Frame::Item { blocks } | Frame::Blockquote { blocks }) => blocks.push(node), _ => nodes.push(node), } } @@ -119,9 +122,8 @@ impl MarkdownDocument { let mut in_code_block = false; let mut code_block_lang: Option = None; let mut code_block_content = String::new(); - // Open list/item containers, innermost last. + // Open block containers (lists, items, quotes), innermost last. let mut frames: Vec = Vec::new(); - let mut in_blockquote = false; let mut in_table = false; let mut in_table_head = false; let mut table_headers: Vec> = Vec::new(); @@ -157,8 +159,8 @@ impl MarkdownDocument { } Event::End(TagEnd::Paragraph) if in_paragraph => { let children = inline_stack.pop().unwrap_or_default(); - if in_blockquote || in_table { - // Collected by the blockquote / table-cell end instead. + if in_table { + // Collected by the table-cell end instead. if let Some(last) = inline_stack.last_mut() { last.extend(children); } @@ -228,13 +230,12 @@ impl MarkdownDocument { } } Event::Start(Tag::BlockQuote(_)) => { - in_blockquote = true; - inline_stack.push(Vec::new()); + frames.push(Frame::Blockquote { blocks: Vec::new() }); } Event::End(TagEnd::BlockQuote(_)) => { - let children = inline_stack.pop().unwrap_or_default(); - push_block(&mut nodes, &mut frames, Node::Blockquote { children }); - in_blockquote = false; + if let Some(Frame::Blockquote { blocks }) = frames.pop() { + push_block(&mut nodes, &mut frames, Node::Blockquote { blocks }); + } } Event::Rule => { push_block(&mut nodes, &mut frames, Node::HorizontalRule); @@ -367,12 +368,15 @@ impl MarkdownDocument { /// Convert a node to flat text (in characters, not bytes). pub(crate) fn node_to_flat_text(node: &Node, text: &mut String) { match node { - Node::Heading { children, .. } - | Node::Paragraph { children } - | Node::Blockquote { children } => { + Node::Heading { children, .. } | Node::Paragraph { children } => { Self::inlines_to_flat_text(children, text); text.push('\n'); } + Node::Blockquote { blocks } => { + for block in blocks { + Self::node_to_flat_text(block, text); + } + } Node::CodeBlock { code, .. } => { for line in code.lines() { text.push_str(line); @@ -649,6 +653,44 @@ let x = 1; )); } + /// A quote holds blocks, so its paragraphs stay separate instead of being + /// merged into one inline run, and a quoted list stays inside the quote + /// rather than being emitted after it. + #[test] + fn blockquote_keeps_its_blocks() { + let doc = MarkdownDocument::parse("> first para\n>\n> second para\n"); + assert_eq!(doc.nodes.len(), 1); + let Node::Blockquote { blocks } = &doc.nodes[0] else { + panic!("expected a blockquote"); + }; + assert_eq!(blocks.len(), 2); + assert_eq!(doc.plain_text, "first para\nsecond para\n"); + + let doc = MarkdownDocument::parse("> Note:\n>\n> - one\n> - two\n"); + assert_eq!(doc.nodes.len(), 1, "the list must not escape the quote"); + let Node::Blockquote { blocks } = &doc.nodes[0] else { + panic!("expected a blockquote"); + }; + assert!(matches!( + blocks.as_slice(), + [Node::Paragraph { .. }, Node::List { .. }] + )); + } + + /// The containers nest both ways round. + #[test] + fn quotes_and_lists_nest_in_each_other() { + let doc = MarkdownDocument::parse("1. Step:\n\n > watch out\n\n2. Next\n"); + assert_eq!(doc.nodes.len(), 1); + let (_, _, items) = expect_list(&doc.nodes[0]); + assert_eq!(items.len(), 2); + assert!(matches!( + items[0].blocks.as_slice(), + [Node::Paragraph { .. }, Node::Blockquote { .. }] + )); + assert_eq!(item_text(&items[0]), "Step:\nwatch out\n"); + } + /// Markers follow the source numbering rather than always restarting at 1. #[test] fn ordered_list_keeps_its_first_number() { @@ -677,6 +719,10 @@ let x = 1; ``` 3. Third + +> A quote, +> +> in two paragraphs. "; let doc = MarkdownDocument::parse(content); let total: usize = doc diff --git a/crates/okena-markdown/src/render.rs b/crates/okena-markdown/src/render.rs index 1d593fa82..b51e142bb 100644 --- a/crates/okena-markdown/src/render.rs +++ b/crates/okena-markdown/src/render.rs @@ -131,6 +131,38 @@ fn word_tokens(text: &str) -> Vec<&str> { tokens } +/// Text style a block inherits from the container it sits in. +/// +/// Quoted content reads dimmer and italic, and that has to travel down to the +/// blocks inside the quote rather than being painted over them: a paragraph sets +/// its own text colour, so a colour on an ancestor would lose to it. +#[derive(Clone, Copy)] +struct BlockStyle { + text_color: u32, + italic: bool, +} + +impl BlockStyle { + fn body(t: &ThemeColors) -> Self { + Self { + text_color: MdColors::new(t).body, + italic: false, + } + } + + fn quoted(t: &ThemeColors) -> Self { + Self { + text_color: MdColors::new(t).muted, + italic: true, + } + } + + fn apply(self, el: Div) -> Div { + el.text_color(rgb(self.text_color)) + .when(self.italic, |el| el.italic()) + } +} + /// Narrow a character selection range to the `len` characters at `offset`. fn sub_selection( selection: Option<(usize, usize)>, @@ -212,6 +244,7 @@ impl MarkdownDocument { node_selection, offset, &mut text_runs, + BlockStyle::body(t), ); RenderedNode::Simple { div: node_div, @@ -348,6 +381,7 @@ impl MarkdownDocument { cell_sel, row_offset + cell_offset + if i > 0 { 1 } else { 0 }, &mut header_runs, + BlockStyle::body(t), ) .text_size(ui_text_md(cx)) .line_height(table_line_height(cx)) @@ -427,6 +461,7 @@ impl MarkdownDocument { cell_sel, row_offset + cell_offset + if i > 0 { 1 } else { 0 }, &mut row_runs, + BlockStyle::body(t), ) .text_size(ui_text_md(cx)) .line_height(table_line_height(cx)) @@ -451,11 +486,10 @@ impl MarkdownDocument { /// Calculate the text length of a node (for selection offset tracking, in characters). pub(crate) fn node_text_length(node: &Node) -> usize { match node { - Node::Heading { level: _, children } - | Node::Paragraph { children } - | Node::Blockquote { children } => { + Node::Heading { level: _, children } | Node::Paragraph { children } => { Self::inlines_text_length(children) + 1 // +1 for newline } + Node::Blockquote { blocks } => blocks.iter().map(Self::node_text_length).sum(), Node::CodeBlock { code, .. } => { // Sum of character lengths of each line + 1 newline per line code.lines() @@ -504,7 +538,10 @@ impl MarkdownDocument { .sum() } - /// Render a node with selection highlighting. + /// Render a node with selection highlighting. `style` is what the container + /// around the node imposes on its text, which is how a quote dims and + /// italicises the blocks inside it. + #[allow(clippy::too_many_arguments)] fn render_node_with_selection( node: &Node, t: &ThemeColors, @@ -512,6 +549,7 @@ impl MarkdownDocument { selection: Option<(usize, usize)>, base_offset: usize, text_runs: &mut Vec, + style: BlockStyle, ) -> Div { let c = MdColors::new(t); match node { @@ -539,6 +577,7 @@ impl MarkdownDocument { selection, base_offset, text_runs, + style, ) .w_full(), Node::List { @@ -578,6 +617,7 @@ impl MarkdownDocument { sub_selection(item_sel, block_offset, block_len), base_offset + offset + block_offset, text_runs, + style, )); block_offset += block_len; } @@ -605,23 +645,32 @@ impl MarkdownDocument { } list } - Node::Blockquote { children } => div() - .pl(px(14.0)) - .border_l_2() - .border_color(rgb(c.surface_border)) - .child( - Self::render_inlines_with_selection_and_targets( - children, + Node::Blockquote { blocks } => { + // A quote stacks whatever it holds, so several quoted paragraphs + // stay separate and a quoted list keeps its markers. + let quoted = BlockStyle::quoted(t); + let mut quote = v_flex() + .w_full() + .gap(px(8.0)) + .pl(px(14.0)) + .border_l_2() + .border_color(rgb(c.surface_border)); + let mut block_offset = 0usize; + for block in blocks { + let block_len = Self::node_text_length(block); + quote = quote.child(Self::render_node_with_selection( + block, t, cx, - selection, - base_offset, + sub_selection(selection, block_offset, block_len), + base_offset + block_offset, text_runs, - ) - .w_full() - .text_color(rgb(c.muted)) - .italic(), - ), + quoted, + )); + block_offset += block_len; + } + quote + } // Whitespace is what separates sections here, so an explicit rule // stays as a hairline that barely registers. Node::HorizontalRule => div().w_full().h(px(1.0)).bg(rgb(c.rule)), @@ -973,6 +1022,7 @@ impl MarkdownDocument { list } + #[allow(clippy::too_many_arguments)] fn render_inlines_with_selection_and_targets( inlines: &[Inline], t: &ThemeColors, @@ -980,6 +1030,7 @@ impl MarkdownDocument { selection: Option<(usize, usize)>, base_offset: usize, text_runs: &mut Vec, + style: BlockStyle, ) -> Div { let mut elements: Vec
= Vec::new(); Self::push_inlines( @@ -993,7 +1044,7 @@ impl MarkdownDocument { text_runs, ); - div() + let row = div() .flex() .flex_wrap() // `min-width: 0` lets this inline-flow container shrink below its @@ -1005,8 +1056,8 @@ impl MarkdownDocument { .items_baseline() .text_size(body_size(cx)) .line_height(body_line_height(cx)) - .text_color(rgb(MdColors::new(t).body)) - .children(elements) + .children(elements); + style.apply(row) } /// Text length of one inline element, in characters. diff --git a/crates/okena-markdown/src/types.rs b/crates/okena-markdown/src/types.rs index f63798b8d..7f8f072f9 100644 --- a/crates/okena-markdown/src/types.rs +++ b/crates/okena-markdown/src/types.rs @@ -36,8 +36,11 @@ pub(crate) enum Node { /// rendering does not re-measure every cell on every frame. col_widths: Vec, }, + /// A quote is a block container too: it can hold several paragraphs, or a + /// list. Collecting only its inlines merged every quoted paragraph onto one + /// line and left a quoted list to render after the quote instead of in it. Blockquote { - children: Vec, + blocks: Vec, }, HorizontalRule, /// YAML frontmatter at the top of the document, rendered as a metadata card diff --git a/crates/okena-markdown/tests/inline_layout.rs b/crates/okena-markdown/tests/inline_layout.rs index 1f5acd0fc..190db0290 100644 --- a/crates/okena-markdown/tests/inline_layout.rs +++ b/crates/okena-markdown/tests/inline_layout.rs @@ -261,6 +261,43 @@ fn nested_list_text_runs_follow_flat_text_offsets(cx: &mut TestAppContext) { assert_eq!(starts, [0, 6, 14, 16, 18]); } +/// A quote renders the blocks it holds, so a quoted list keeps its markers and +/// the offsets behind selection stay in step with the flat text. +#[gpui::test] +fn blockquote_text_runs_follow_flat_text_offsets(cx: &mut TestAppContext) { + let doc = MarkdownDocument::parse("> Note:\n>\n> - one\n> - two\n"); + assert_eq!(doc.plain_text, "Note:\none\ntwo\n"); + assert_eq!(doc.node_count(), 1); + + let captured: Rc>> = Default::default(); + let captured_for_draw = captured.clone(); + let vcx = cx.add_empty_window(); + + vcx.draw( + Point::default(), + Size { + width: AvailableSpace::Definite(px(500.0)), + height: AvailableSpace::MinContent, + }, + |_window, cx| { + let Some(RenderedNode::Simple { div, text_runs, .. }) = + doc.render_node(0, &DARK_THEME, cx, None) + else { + return div(); + }; + captured_for_draw.borrow_mut().extend(text_runs); + div + }, + ); + + let starts = captured + .borrow() + .iter() + .map(run_start_offset) + .collect::>(); + assert_eq!(starts, [0, 6, 10]); +} + #[gpui::test] fn frontmatter_text_runs_follow_flat_text_offsets(cx: &mut TestAppContext) { let doc = MarkdownDocument::parse("---\ntitle: Žluť\nitems:\n - one\n---\n"); From d128bd3471de0657bafc7f4419a0941784f42ae3 Mon Sep 17 00:00:00 2001 From: jonasnobile Date: Mon, 21 Sep 2026 14:09:05 +0200 Subject: [PATCH 03/12] fix(git): survive a worktree delete that loses a race, and say why when it does not Closing a worktree reported "failed to remove directory ''" and nothing else. `GitError::RemoveFailed` kept the io cause in `#[source]`, which neither the toast nor the log walks, so the one sentence the user gets repeated the path it had already named and dropped the reason. The cause is in the message now. `remove_dir_all` walks the tree and then removes the directory itself, so anything writing into the checkout during that walk (a watcher that outlived the shell it came from, Spotlight, Finder) leaves the final rmdir reporting a directory that is not empty. That is a race, not a verdict: it now gets a few more attempts, and only for the error kinds that can be transient. A permission error still fails on the first try, since retrying it only delays the same report. A quarantine that can be neither deleted nor put back used to stay on disk as a whole checkout under a hidden name, invisible to the user and swept up by nothing. The next removal in that directory reclaims it, skipping any quarantine a concurrent removal is using. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RkbvFzpmyCRY4DBp7oJHut --- crates/okena-git/src/error.rs | 21 +- crates/okena-git/src/repository/worktree.rs | 241 +++++++++++++++++++- 2 files changed, 255 insertions(+), 7 deletions(-) diff --git a/crates/okena-git/src/error.rs b/crates/okena-git/src/error.rs index 36de7a24a..c0c2fbedd 100644 --- a/crates/okena-git/src/error.rs +++ b/crates/okena-git/src/error.rs @@ -19,8 +19,11 @@ pub enum GitError { #[error("directory '{path}' is already an active worktree")] WorktreeExists { path: PathBuf }, - /// Failed to remove a directory. - #[error("failed to remove directory '{path}'")] + /// Failed to remove a directory. The cause belongs in the message: this + /// error reaches the user as a toast and the log as `{e}`, and neither + /// walks the source chain, so without it a failed worktree close reports + /// only the path it already named. + #[error("failed to remove directory '{path}': {source}")] RemoveFailed { path: PathBuf, #[source] @@ -100,6 +103,20 @@ mod tests { ); } + /// The io cause is what says whether the close failed on a busy directory, + /// a permission, or a vanished path. Dropping it leaves the user with a + /// sentence that only repeats the path. + #[test] + fn a_failed_removal_names_its_cause() { + let err = GitError::RemoveFailed { + path: PathBuf::from("/tmp/wt"), + source: std::io::Error::from(std::io::ErrorKind::DirectoryNotEmpty), + }; + let message = err.user_detail(); + assert!(message.contains("/tmp/wt"), "{message}"); + assert!(message.contains("not empty"), "{message}"); + } + #[test] fn the_last_failure_line_wins() { let err = GitError::GitExitError { diff --git a/crates/okena-git/src/repository/worktree.rs b/crates/okena-git/src/repository/worktree.rs index 4a87d4015..1ccedfc37 100644 --- a/crates/okena-git/src/repository/worktree.rs +++ b/crates/okena-git/src/repository/worktree.rs @@ -1,6 +1,9 @@ //! Worktree operations: create / remove / list. +use std::collections::BTreeSet; use std::path::{Path, PathBuf}; +use std::sync::Mutex; +use std::time::Duration; use okena_core::process::{command, safe_output}; @@ -586,7 +589,7 @@ pub fn remove_worktree_fast(verified: &VerifiedWorktree) -> GitResult<()> { fn remove_worktree_fast_with( verified: &VerifiedWorktree, - remove_dir_all: impl FnOnce(&Path) -> std::io::Result<()>, + remove_dir_all: impl FnMut(&Path) -> std::io::Result<()>, ) -> GitResult<()> { revalidate_verified_worktree(verified)?; quarantine_and_delete( @@ -597,6 +600,130 @@ fn remove_worktree_fast_with( ) } +/// Prefix of the hidden directory a checkout is renamed to before deletion. +const QUARANTINE_PREFIX: &str = ".okena-removing-"; + +/// How many times a delete is attempted before the failure stands, and how long +/// to wait between attempts. +const DELETE_ATTEMPTS: usize = 4; +const DELETE_RETRY_DELAY: Duration = Duration::from_millis(150); + +/// Quarantine directories this process is deleting right now. +/// +/// The reclaim sweep runs in the same parent directory as live removals, so a +/// worktree being closed at this moment must not have its quarantine pulled out +/// from under it by a sweep running for another project. +static QUARANTINES_IN_FLIGHT: Mutex> = Mutex::new(BTreeSet::new()); + +/// A quarantine path registered for as long as its removal is running. +struct InFlightQuarantine(PathBuf); + +impl InFlightQuarantine { + fn register(path: &Path) -> Self { + let owned = path.to_path_buf(); + in_flight_quarantines().insert(owned.clone()); + Self(owned) + } +} + +impl Drop for InFlightQuarantine { + fn drop(&mut self) { + in_flight_quarantines().remove(&self.0); + } +} + +/// The in-flight set, usable even after a panic poisoned the lock: the set is +/// plain paths, so a poisoned one is no less valid than a healthy one. +fn in_flight_quarantines() -> std::sync::MutexGuard<'static, BTreeSet> { + QUARANTINES_IN_FLIGHT + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) +} + +/// Whether a failed delete is worth another attempt. +/// +/// `remove_dir_all` walks the tree and then removes the directory itself, so +/// anything that writes into the checkout during that walk (a watcher that +/// outlived the shell it was started from, Spotlight, Finder) leaves the final +/// `rmdir` reporting a directory that is not empty, with nothing actually +/// wrong. A permission error is not a race: it fails the same way every time, +/// and retrying only delays the report. +fn is_transient_delete_error(error: &std::io::Error) -> bool { + matches!( + error.kind(), + std::io::ErrorKind::DirectoryNotEmpty + | std::io::ErrorKind::ResourceBusy + | std::io::ErrorKind::Interrupted + ) +} + +/// Delete a directory, retrying while the failure looks like a race with +/// something still writing into it. An already absent directory is a success. +fn delete_with_retries( + path: &Path, + mut remove_dir_all: impl FnMut(&Path) -> std::io::Result<()>, +) -> std::io::Result<()> { + let mut attempt = 1; + loop { + match remove_dir_all(path) { + Ok(()) => return Ok(()), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => { + if attempt >= DELETE_ATTEMPTS || !is_transient_delete_error(&error) { + return Err(error); + } + log::info!( + "worktree removal: delete of '{}' hit a transient failure ({error}), attempt {attempt} of {DELETE_ATTEMPTS}", + path.display() + ); + std::thread::sleep(DELETE_RETRY_DELAY); + attempt += 1; + } + } + } +} + +/// Whether a directory name is one this module generated. +fn is_quarantine_name(name: &std::ffi::OsStr) -> bool { + name.to_str() + .and_then(|name| name.strip_prefix(QUARANTINE_PREFIX)) + .is_some_and(|id| uuid::Uuid::parse_str(id).is_ok()) +} + +/// Delete quarantines an earlier removal could neither delete nor put back. +/// +/// Such a directory is a whole checkout under a hidden name: left alone it is a +/// disk leak the user has no way to see, and the worktree it holds was already +/// confirmed for deletion. Best effort by design, and never fatal to the +/// removal that is starting: a quarantine that still refuses to go gets logged +/// and waits for the next attempt. +fn reclaim_abandoned_quarantines(parent: &Path) { + let Ok(entries) = std::fs::read_dir(parent) else { + return; + }; + for entry in entries.flatten() { + if !is_quarantine_name(&entry.file_name()) + || !entry.file_type().is_ok_and(|kind| kind.is_dir()) + { + continue; + } + let path = entry.path(); + if in_flight_quarantines().contains(&path) { + continue; + } + match delete_with_retries(&path, |path| std::fs::remove_dir_all(path)) { + Ok(()) => log::info!( + "worktree removal: reclaimed an abandoned quarantine at '{}'", + path.display() + ), + Err(error) => log::warn!( + "worktree removal: abandoned quarantine at '{}' could not be reclaimed: {error}", + path.display() + ), + } + } +} + /// Rename the checkout aside, re-prove it is still the directory whose /// `identity` was verified, delete it, then prune the parent's stale worktree /// metadata. Shared by the verified and orphaned removal paths so both get the @@ -606,17 +733,20 @@ fn quarantine_and_delete( worktree_path: &Path, identity: &FilesystemObjectIdentity, parent_path: &Path, - remove_dir_all: impl FnOnce(&Path) -> std::io::Result<()>, + remove_dir_all: impl FnMut(&Path) -> std::io::Result<()>, ) -> GitResult<()> { let parent = worktree_path .parent() .ok_or_else(|| unsafe_worktree(worktree_path, "checkout directory has no parent"))?; - let quarantine = parent.join(format!(".okena-removing-{}", uuid::Uuid::new_v4())); + reclaim_abandoned_quarantines(parent); + let quarantine = parent.join(format!("{QUARANTINE_PREFIX}{}", uuid::Uuid::new_v4())); std::fs::rename(worktree_path, &quarantine).map_err(|source| GitError::RemoveFailed { path: worktree_path.to_path_buf(), source, })?; + let _in_flight = InFlightQuarantine::register(&quarantine); + let quarantined_identity = filesystem_object_identity(&quarantine); if quarantined_identity.as_ref() != Some(identity) { let restore = if !worktree_path.exists() { @@ -637,9 +767,8 @@ fn quarantine_and_delete( return Err(unsafe_worktree(worktree_path, reason)); } - match remove_dir_all(&quarantine) { + match delete_with_retries(&quarantine, remove_dir_all) { Ok(()) => {} - Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} Err(error) => { // `remove_dir_all` can have already removed the checkout and leave // only Finder metadata behind. Delete that narrow, verified class of @@ -941,6 +1070,108 @@ mod tests { ); } + /// A checkout is deleted while the machine keeps running, so a watcher or + /// an indexer can drop a file into the tree between the walk and the final + /// `rmdir`. That is a race, not a verdict: the removal used to report the + /// whole close as failed and put the checkout back. + #[test] + fn a_delete_losing_a_race_is_retried() { + let (_tmp, repo) = init_temp_repo(); + let wt_tmp = tempfile::tempdir().expect("create worktree tempdir"); + let wt_path = wt_tmp.path().join("wt-feat"); + git_in( + &repo, + &["worktree", "add", wt_path.to_str().unwrap(), "-b", "feat"], + ); + let verified = verify_linked_worktree_fresh(&repo, &wt_path).expect("verify worktree"); + + let attempts = std::cell::Cell::new(0usize); + let result = remove_worktree_fast_with(&verified, |quarantine| { + attempts.set(attempts.get() + 1); + if attempts.get() == 1 { + return Err(std::io::Error::from(std::io::ErrorKind::DirectoryNotEmpty)); + } + std::fs::remove_dir_all(quarantine) + }); + + assert!(result.is_ok(), "{result:?}"); + assert_eq!(attempts.get(), 2, "the second attempt should have run"); + assert!(!wt_path.exists(), "the checkout is gone"); + assert!( + quarantines_in(wt_tmp.path()).next().is_none(), + "no quarantine is left behind" + ); + } + + /// A permission failure repeats identically however often it is tried, and + /// the cause has to survive into the message: that sentence is the whole of + /// what the user is told when a close fails. + #[test] + fn a_permission_failure_is_reported_once_with_its_cause() { + let (_tmp, repo) = init_temp_repo(); + let wt_tmp = tempfile::tempdir().expect("create worktree tempdir"); + let wt_path = wt_tmp.path().join("wt-feat"); + git_in( + &repo, + &["worktree", "add", wt_path.to_str().unwrap(), "-b", "feat"], + ); + let verified = verify_linked_worktree_fresh(&repo, &wt_path).expect("verify worktree"); + + let attempts = std::cell::Cell::new(0usize); + let result = remove_worktree_fast_with(&verified, |_| { + attempts.set(attempts.get() + 1); + Err(std::io::Error::from(std::io::ErrorKind::PermissionDenied)) + }); + + let error = result.expect_err("a permission failure must not be swallowed"); + assert_eq!(attempts.get(), 1, "retrying only delays the same failure"); + assert!( + error.to_string().contains("permission denied"), + "the cause must reach the message: {error}" + ); + assert!(wt_path.exists(), "the checkout is restored, not lost"); + } + + /// A quarantine that could be neither deleted nor restored is a whole + /// checkout under a hidden name. The next removal in that directory + /// reclaims it; anything else hidden there is left alone. + #[test] + fn the_next_removal_reclaims_an_abandoned_quarantine() { + let (_tmp, repo) = init_temp_repo(); + let wt_tmp = tempfile::tempdir().expect("create worktree tempdir"); + let abandoned = wt_tmp + .path() + .join(format!("{QUARANTINE_PREFIX}{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(abandoned.join("src")).expect("create abandoned quarantine"); + std::fs::write(abandoned.join("src").join("main.rs"), "leaked checkout") + .expect("fill abandoned quarantine"); + let foreign = wt_tmp.path().join(format!("{QUARANTINE_PREFIX}not-a-uuid")); + std::fs::create_dir(&foreign).expect("create lookalike directory"); + + let wt_path = wt_tmp.path().join("wt-feat"); + git_in( + &repo, + &["worktree", "add", wt_path.to_str().unwrap(), "-b", "feat"], + ); + let verified = verify_linked_worktree_fresh(&repo, &wt_path).expect("verify worktree"); + remove_worktree_fast(&verified).expect("remove worktree"); + + assert!(!abandoned.exists(), "the leaked checkout is reclaimed"); + assert!( + foreign.exists(), + "a name this module never wrote is not ours" + ); + } + + /// Every quarantine left in `parent`, whatever its uuid. + fn quarantines_in(parent: &Path) -> impl Iterator { + std::fs::read_dir(parent) + .expect("inspect worktree parent") + .filter_map(Result::ok) + .filter(|entry| is_quarantine_name(&entry.file_name())) + .map(|entry| entry.path()) + } + #[test] fn guarded_fast_removal_rejects_a_replaced_checkout() { let (_tmp, repo) = init_temp_repo(); From dfa8268f55f023012bc2414ebb1c3678ba825300 Mon Sep 17 00:00:00 2001 From: jonasnobile Date: Mon, 21 Sep 2026 14:09:13 +0200 Subject: [PATCH 04/12] test(daemon): run hook PTYs in a bare shell instead of the developer's `successful_hook_pty_exit_removes_real_worktree` failed on macOS and `hook_exit_after_stash_keeps_changes_written_since_the_stash` failed whenever the machine was busy. Both created the hook PTY through `ShellType::for_command`, which is the production path: `$SHELL -ic`, an interactive shell, so a hook sees the user's aliases and environment. In a test that only buys a dependency on what the developer's profile costs to start, and an interactive zsh that takes 1.6s to reach the command spends most of a 2 second budget before the hook has run. What these tests cover is PTY exit handling and worktree removal, not shell resolution, so the command runs in a bare shell. The wait is a named constant and generous: it is there to turn a hang into a failure, not to assert how fast a PTY starts. The module now runs in 0.24s rather than 2.4s. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RkbvFzpmyCRY4DBp7oJHut --- crates/okena-daemon-core/src/pty_loop.rs | 40 +++++++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/crates/okena-daemon-core/src/pty_loop.rs b/crates/okena-daemon-core/src/pty_loop.rs index a6d11b1fb..311c46c18 100644 --- a/crates/okena-daemon-core/src/pty_loop.rs +++ b/crates/okena-daemon-core/src/pty_loop.rs @@ -1137,6 +1137,32 @@ mod tests { (repo, worktree) } + /// A shell for a hook PTY a test waits on. + /// + /// Production hooks go through `ShellType::for_command`, which runs + /// `$SHELL -ic` so the hook sees the user's aliases and environment. In a + /// test that only buys a dependency on what the developer's interactive + /// profile costs to start: an interactive zsh that takes 1.6s to reach the + /// command runs these waits out of budget on a machine where nothing is + /// wrong. What is under test is PTY exit handling, not shell resolution, so + /// run the command in a bare shell. + /// How long a test waits for a hook PTY to report its exit. + const HOOK_EXIT_BUDGET: Duration = Duration::from_secs(20); + + fn hook_shell(command: &str) -> ShellType { + if cfg!(windows) { + ShellType::Custom { + path: "cmd".to_string(), + args: vec!["/C".to_string(), command.to_string()], + } + } else { + ShellType::Custom { + path: "/bin/sh".to_string(), + args: vec!["-c".to_string(), command.to_string()], + } + } + } + fn workspace_with_pending_close( main_repo: &Path, worktree: &Path, @@ -1744,7 +1770,7 @@ mod tests { let hook_terminal_id = pty_manager .create_terminal_with_shell( worktree.to_str().expect("utf-8 worktree path"), - Some(&ShellType::for_command("exit 0".to_string())), + Some(&hook_shell("exit 0")), ) .expect("create before-remove hook PTY"); let pty_manager = Arc::new(pty_manager); @@ -1782,7 +1808,9 @@ mod tests { let mut exit_events = Vec::new(); let mut dirty_terminal_ids = Vec::new(); let mut budget = TurnBudget::default(); - tokio::time::timeout(Duration::from_secs(2), async { + // Generous on purpose: this budget is here to turn a hang + // into a failure, not to assert how fast a PTY starts. + tokio::time::timeout(HOOK_EXIT_BUDGET, async { while exit_events.is_empty() { let event = pty_events.recv().await.expect("receive hook PTY event"); process_event( @@ -1868,7 +1896,7 @@ mod tests { let hook_terminal_id = pty_manager .create_terminal_with_shell( worktree.to_str().expect("utf-8 worktree path"), - Some(&ShellType::for_command("exit 0".to_string())), + Some(&hook_shell("exit 0")), ) .expect("create before-remove hook PTY"); let pty_manager = Arc::new(pty_manager); @@ -1907,7 +1935,9 @@ mod tests { let mut exit_events = Vec::new(); let mut dirty_terminal_ids = Vec::new(); let mut budget = TurnBudget::default(); - tokio::time::timeout(Duration::from_secs(2), async { + // Generous on purpose: this budget is here to turn a hang + // into a failure, not to assert how fast a PTY starts. + tokio::time::timeout(HOOK_EXIT_BUDGET, async { while exit_events.is_empty() { let event = pty_events.recv().await.expect("receive hook PTY event"); process_event( @@ -1978,7 +2008,7 @@ mod tests { let hook_terminal_id = pty_manager .create_terminal_with_shell( worktree.to_str().expect("utf-8 worktree path"), - Some(&ShellType::for_command("sleep 30".to_string())), + Some(&hook_shell("sleep 30")), ) .expect("create keep-alive before-remove hook PTY"); let pty_manager = Arc::new(pty_manager); From 2f5d983ada21aed6bceec804822caa50664797fd Mon Sep 17 00:00:00 2001 From: jonasnobile Date: Mon, 21 Sep 2026 14:09:22 +0200 Subject: [PATCH 05/12] test(cli): look where the daemon actually publishes, and let it wake up first The two daemon-backed tests in this file never passed on macOS. They watched for `remote.json` under `$XDG_CONFIG_HOME`, but `config_root()` resolves through `dirs::config_dir()`, which is `~/Library/Application Support` there. The test redirects both that and `HOME` into its isolated root, so the daemon was contained either way; it published a second into the run, to the path nobody was watching, and each test then waited out its full 60 second timeout. They look for whichever path the platform uses now. With the daemon found, a second race showed up under load: `remote.json` says only that the port is published, and the first CLI call carries a one-off token registration on top of its command. The CLI gives a request 5 seconds, which a busy machine can spend on that round trip alone. Startup now ends when a CLI call has actually been answered, so the wait sits where it proves nothing rather than inside an assertion. The target runs in 4s instead of 120s, and holds up with a full `cargo test --workspace` running beside it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RkbvFzpmyCRY4DBp7oJHut --- tests/cli_over_the_remote_api.rs | 50 ++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/tests/cli_over_the_remote_api.rs b/tests/cli_over_the_remote_api.rs index 4199f595d..1f4864ebe 100644 --- a/tests/cli_over_the_remote_api.rs +++ b/tests/cli_over_the_remote_api.rs @@ -43,9 +43,37 @@ impl Daemon { let daemon = Self { child, root }; daemon.wait_for_remote_json(); + daemon.wait_until_the_cli_gets_an_answer(); daemon } + /// Ready means a CLI call has actually been answered. + /// + /// `remote.json` says only that the port is published; the daemon is still + /// finishing startup behind it, and the first CLI call carries the one-off + /// token registration on top of the command itself. The CLI gives a request + /// 5 seconds, which a busy machine can spend on that first round trip + /// alone, so a test that starts asserting the moment the file lands fails + /// on a daemon that is merely still waking up. Spend the wait here, where + /// it proves nothing, instead of inside an assertion where it looks like a + /// verdict. + fn wait_until_the_cli_gets_an_answer(&self) { + let deadline = Instant::now() + Duration::from_secs(60); + loop { + let output = self.cli(&["ls", "--json"]); + if output.status.success() { + return; + } + if Instant::now() >= deadline { + panic!( + "the daemon never answered `okena ls --json`: {}", + String::from_utf8_lossy(&output.stderr).trim() + ); + } + std::thread::sleep(Duration::from_millis(100)); + } + } + fn command(root: &Path) -> Command { let mut command = Command::new(BIN); command @@ -56,9 +84,27 @@ impl Daemon { command } + /// Where the daemon keeps the active profile, mirroring + /// `okena_core::profiles::config_root`. + /// + /// That resolves through `dirs::config_dir()`, which is `$XDG_CONFIG_HOME` + /// on Linux but `~/Library/Application Support` on macOS. Both are + /// redirected into the isolated root, so the daemon is contained either + /// way; only the path to look at differs. Watching the Linux one alone made + /// every daemon-backed test in this file fail on macOS, waiting out the + /// full timeout for a file that was published elsewhere a second in. + fn profile_dir(&self) -> PathBuf { + let config_root = if cfg!(target_os = "macos") { + self.root.join("home/Library/Application Support") + } else { + self.root.join("cfg") + }; + config_root.join("okena/profiles/default") + } + /// The daemon publishes its port here, and the CLI discovers it from here. fn wait_for_remote_json(&self) { - let published = self.root.join("cfg/okena/profiles/default/remote.json"); + let published = self.profile_dir().join("remote.json"); let deadline = Instant::now() + Duration::from_secs(60); while Instant::now() < deadline { if published.exists() { @@ -90,7 +136,7 @@ impl Daemon { /// The bearer token the first CLI call registered for this daemon. fn cli_token(&self) -> String { - let path = self.root.join("cfg/okena/profiles/default/cli.json"); + let path = self.profile_dir().join("cli.json"); let config: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&path).expect("cli.json")) .expect("cli.json is JSON"); From 03342c268c12d1fe642da81ed01141d29222608c Mon Sep 17 00:00:00 2001 From: jonasnobile Date: Mon, 21 Sep 2026 14:09:34 +0200 Subject: [PATCH 06/12] test(workspace): build the git fixture on a resolved path Two legacy worktree recovery tests failed on macOS. `GitFixture` builds its root from `std::env::temp_dir()`, which there is handed out behind a symlink (`/var/folders/...` is `/private/var/folders/...`). Git's worktree registry reports resolved paths, and `registered_checkout_root` matches a row against them lexically, on purpose: it must not touch the filesystem, or a checkout deleted from disk would stop being sweepable. So the prefix never matched, the recovery under test never ran, and the row was dropped. The fixture starts from the resolved path, which is what a checkout anywhere outside the temp directory already is. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RkbvFzpmyCRY4DBp7oJHut --- crates/okena-workspace/src/persistence.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/okena-workspace/src/persistence.rs b/crates/okena-workspace/src/persistence.rs index 0c7ea9ae5..2a1e123ca 100644 --- a/crates/okena-workspace/src/persistence.rs +++ b/crates/okena-workspace/src/persistence.rs @@ -1793,6 +1793,15 @@ mod tests { let root = std::env::temp_dir().join(format!("okena-wt-registry-{}", uuid::Uuid::new_v4())); std::fs::create_dir_all(&root).expect("create fixture root"); + // Git's worktree registry reports resolved paths, and recovery + // matches a row's path against them without touching the + // filesystem, so that a checkout deleted from disk stays + // sweepable. On macOS the temp directory is handed out behind a + // symlink (`/var/folders/...` is `/private/var/folders/...`), so a + // fixture built from it writes rows the registry can never match + // and the recovery under test never runs. Start from the resolved + // path, which is what a checkout anywhere else already is. + let root = std::fs::canonicalize(&root).expect("resolve fixture root"); Self { root } } From 8663090c30a1484e0795a5cc5569c42381480e2c Mon Sep 17 00:00:00 2001 From: jonasnobile Date: Mon, 21 Sep 2026 15:21:29 +0200 Subject: [PATCH 07/12] fix(git): stop reporting the default branch's CI against a worktree A worktree showed a failed CI run it had nothing to do with: a deploy/staging run belonging to the repo's default branch. Two causes, one behind the other. Okena creates a worktree branch with `git worktree add -b origin/`, and git records a remote-tracking start point as the new branch's upstream. So a branch that has never been pushed comes out tracking `main`. Creation now passes `--no-track`; `git push -u` records the real upstream once there is something to record. Everything keyed on "the branch's pushed commit" then read that upstream without checking whose it was, so for such a branch it resolved to main's tip. The branch CI lookup asked GitHub for the check runs of that commit and got main's, and PR resolution compared a PR head against it. The CI path now takes the upstream only when it is this branch's own counterpart, treating the default branch as a base rather than a counterpart. A branch deliberately tracking a differently named remote branch keeps its own answer. Reported from a checkout whose upstream was `origin/main` at 74aa8f43c while the branch itself was at 815973353. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RkbvFzpmyCRY4DBp7oJHut --- crates/okena-git/src/repository/ci.rs | 93 +++++++++++++++++++-- crates/okena-git/src/repository/mod.rs | 12 +++ crates/okena-git/src/repository/status.rs | 62 ++++++++++++-- crates/okena-git/src/repository/worktree.rs | 86 +++++++++++++++---- 4 files changed, 222 insertions(+), 31 deletions(-) diff --git a/crates/okena-git/src/repository/ci.rs b/crates/okena-git/src/repository/ci.rs index e31f431a3..fc5aff3ba 100644 --- a/crates/okena-git/src/repository/ci.rs +++ b/crates/okena-git/src/repository/ci.rs @@ -13,7 +13,7 @@ use okena_core::process::{command, safe_output_with_timeout}; use serde_json::{Value, json}; use super::github::{ApiError, GithubClient, GithubRepo, resolve_base_repo}; -use super::status::get_pushed_sha; +use super::status::get_upstream_ref; /// Hard cap on the remaining `gh` invocation. `gh` can hang indefinitely — /// auth prompts or a stalled network — and the bus kills the process when @@ -135,6 +135,33 @@ struct PrNode { head_ref_oid: Option, } +/// The pushed commit that belongs to *this* branch, or `None` when the branch +/// tracks somebody else's. +/// +/// Git records an upstream whenever a branch starts from a remote-tracking ref, +/// and a worktree branch starts from `origin/`. Until it is pushed, +/// its upstream is therefore the default branch, and a lookup keyed on that +/// commit answers with the default branch's CI: a nightly deploy reported as a +/// failure against a worktree that never triggered it, and a PR head compared +/// against a commit from another branch. +/// +/// A branch that deliberately tracks a differently named remote branch still +/// gets its own answer. Only the default branch is treated as a base rather +/// than a counterpart, which is the case Okena creates itself. +fn branch_pushed_sha(path: &Path) -> Option { + let upstream = get_upstream_ref(path)?; + let branch = super::status::get_current_branch(path)?; + if upstream.branch == branch { + return Some(upstream.sha); + } + // Only reached for the mismatch, so the default-branch lookup stays off the + // path every well-tracked branch takes. + match super::branch::get_default_branch(path) { + Some(default) if default == upstream.branch => None, + _ => Some(upstream.sha), + } +} + /// Get PR info for the current branch (if any PR exists). /// /// Matches by head branch name in the base repository, like `gh pr list @@ -146,7 +173,7 @@ pub fn fetch_pr_info(path: &Path) -> PrFetch { return PrFetch::Fetched(None); }; let current_sha = super::status::get_head_sha(path); - let pushed_sha = get_pushed_sha(path); + let pushed_sha = branch_pushed_sha(path); let Some((mut client, repo)) = github_client(path) else { return PrFetch::Fetched(None); }; @@ -340,9 +367,9 @@ fn rollup_status(failed: usize, pending: usize) -> crate::CiStatus { /// /// With a known PR number, reads the PR's status-check rollup (Actions + /// external status checks aggregated by the PR, as `gh pr checks` does). -/// Otherwise falls back to `check-runs` + `status` on the current upstream -/// commit, which works for any pushed branch — including default branches -/// without a PR. +/// Otherwise falls back to `check-runs` + `status` on the branch's own pushed +/// commit (see `branch_pushed_sha`), which works for any pushed branch, +/// default branches without a PR included. /// /// `unchanged_sha` is the upstream commit a *settled* cached summary describes. /// Checks on a given commit only move while something is running, so when the @@ -358,7 +385,7 @@ pub fn fetch_ci_checks( unchanged_sha: Option<&str>, ) -> CiFetch { // Read locally (gix, no network) before deciding to spend a request. - let sha = get_pushed_sha(path); + let sha = branch_pushed_sha(path); if let (Some(sha), Some(cached)) = (sha.as_deref(), unchanged_sha) && sha == cached { @@ -1247,13 +1274,65 @@ mod tests { ); super::super::test_support::git_in(&repo, &["push", "-u", "origin", "main"]); - let sha = super::get_pushed_sha(&repo).expect("branch has an upstream"); + let sha = super::super::status::get_pushed_sha(&repo).expect("branch has an upstream"); assert_eq!( super::fetch_ci_checks(&repo, None, Some(&sha)), super::CiFetch::Unchanged ); } + /// The exact shape a worktree branch had before `--no-track`: it tracks the + /// default branch, so its "upstream commit" is main's tip. A lookup keyed + /// on that commit answers with main's CI, which is how a nightly deploy + /// failure ended up reported against a feature worktree. + #[test] + fn a_branch_tracking_the_default_branch_is_not_asked_about() { + let (_tmp, repo, _remote) = super::super::test_support::repo_with_origin(); + super::super::test_support::git_in(&repo, &["checkout", "-q", "-b", "feat/x"]); + // What `git worktree add -b feat/x origin/main` used to record. + super::super::test_support::git_in(&repo, &["config", "branch.feat/x.remote", "origin"]); + super::super::test_support::git_in( + &repo, + &["config", "branch.feat/x.merge", "refs/heads/main"], + ); + + assert!( + super::branch_pushed_sha(&repo).is_none(), + "main's tip is not this branch's pushed commit" + ); + // And no request is spent finding that out. + assert_eq!( + super::fetch_ci_checks(&repo, None, None), + super::CiFetch::Fetched { + sha: None, + summary: None + } + ); + } + + /// A branch that tracks its own counterpart still gets its own answer, and + /// so does one deliberately tracking a differently named remote branch. + #[test] + fn a_branch_tracking_its_own_remote_keeps_its_commit() { + let (_tmp, repo, _remote) = super::super::test_support::repo_with_origin(); + super::super::test_support::git_in(&repo, &["checkout", "-q", "-b", "feat/x"]); + super::super::test_support::git_in(&repo, &["push", "-q", "-u", "origin", "feat/x"]); + let own = super::branch_pushed_sha(&repo).expect("its own upstream counts"); + + // Now point it at a non-default remote branch under another name. + super::super::test_support::git_in(&repo, &["push", "-q", "origin", "feat/x:other"]); + super::super::test_support::git_in(&repo, &["fetch", "-q", "origin"]); + super::super::test_support::git_in( + &repo, + &["config", "branch.feat/x.merge", "refs/heads/other"], + ); + assert_eq!( + super::branch_pushed_sha(&repo).as_deref(), + Some(own.as_str()), + "tracking another branch on purpose is still this branch's answer" + ); + } + #[test] fn branch_without_upstream_never_reaches_the_api() { // Nothing is pushed, so there is nothing CI could have run on. diff --git a/crates/okena-git/src/repository/mod.rs b/crates/okena-git/src/repository/mod.rs index 1bb9692db..876a360d7 100644 --- a/crates/okena-git/src/repository/mod.rs +++ b/crates/okena-git/src/repository/mod.rs @@ -166,6 +166,18 @@ pub(crate) mod test_support { String::from_utf8_lossy(&status.stderr) ); } + /// A repo whose `main` is pushed to a bare `origin`. The second tempdir owns + /// the remote and must stay alive for the repo's lifetime. + pub(crate) fn repo_with_origin() -> (tempfile::TempDir, PathBuf, tempfile::TempDir) { + let (tmp, repo) = init_temp_repo(); + let remote_tmp = tempfile::tempdir().expect("create remote tempdir"); + let remote = remote_tmp.path().join("remote.git"); + let remote_str = remote.to_str().expect("remote path is utf-8"); + git_in(&repo, &["init", "--bare", "-b", "main", remote_str]); + git_in(&repo, &["remote", "add", "origin", remote_str]); + git_in(&repo, &["push", "-q", "origin", "main"]); + (tmp, repo, remote_tmp) + } } #[cfg(test)] diff --git a/crates/okena-git/src/repository/status.rs b/crates/okena-git/src/repository/status.rs index b5f7c52af..048076530 100644 --- a/crates/okena-git/src/repository/status.rs +++ b/crates/okena-git/src/repository/status.rs @@ -197,14 +197,23 @@ pub fn get_head_sha(path: &Path) -> Option { Some(id.to_hex().to_string()) } -/// Full SHA of the current branch's upstream tracking commit — the last commit -/// known (from the latest fetch) to be on the remote. `None` if HEAD is -/// detached or the branch has no upstream (never pushed). +/// The remote branch the current branch tracks, and that branch's commit. /// -/// Branch-level CI lookups (`/commits/{sha}/check-runs` and `/status`) must use -/// this rather than the local HEAD: GitHub runs CI against *pushed* commits, so -/// querying an unpushed local HEAD just returns nothing. -pub fn get_pushed_sha(path: &Path) -> Option { +/// The name matters as much as the commit: a branch does not necessarily track +/// its own counterpart. Git records an upstream whenever a branch is started +/// from a remote-tracking ref, so a branch created from `origin/main` tracks +/// `main` until it is pushed, and its "upstream commit" is the default +/// branch's tip rather than anything this branch did. +pub struct UpstreamRef { + /// Branch name on the remote, without the remote prefix: `main`, `feat/x`. + pub branch: String, + /// Full SHA that remote branch points at, as of the latest fetch. + pub sha: String, +} + +/// The current branch's upstream. `None` if HEAD is detached or the branch has +/// no upstream (never pushed, and not started from a remote-tracking ref). +pub fn get_upstream_ref(path: &Path) -> Option { let repo = crate::gix_helpers::open(path)?; let branch = super::head_branch_short(&repo)?; let head_ref = repo @@ -218,7 +227,29 @@ pub fn get_pushed_sha(path: &Path) -> Option { .rev_parse_single(upstream_name.as_bstr()) .ok()? .detach(); - Some(id.to_hex().to_string()) + Some(UpstreamRef { + branch: remote_branch_name(&upstream_name.as_bstr().to_string())?, + sha: id.to_hex().to_string(), + }) +} + +/// `refs/remotes/origin/feat/x` -> `feat/x`. The remote is one segment, the +/// branch is everything after it, slashes included. +fn remote_branch_name(full_ref: &str) -> Option { + let (_remote, branch) = full_ref.strip_prefix("refs/remotes/")?.split_once('/')?; + (!branch.is_empty()).then(|| branch.to_string()) +} + +/// Full SHA of the current branch's upstream tracking commit: the last commit +/// known (from the latest fetch) to be on the remote. `None` if HEAD is +/// detached or the branch has no upstream (never pushed). +/// +/// Branch-level CI lookups (`/commits/{sha}/check-runs` and `/status`) must use +/// this rather than the local HEAD: GitHub runs CI against *pushed* commits, so +/// querying an unpushed local HEAD just returns nothing. They must also check +/// *whose* upstream it is (see `UpstreamRef`). +pub fn get_pushed_sha(path: &Path) -> Option { + get_upstream_ref(path).map(|upstream| upstream.sha) } /// Tracked per-file diff counts and the untracked-file list, produced by a @@ -665,6 +696,21 @@ mod tests { assert!(get_current_branch(&path).is_none()); } + #[test] + fn a_remote_ref_splits_into_remote_and_branch() { + assert_eq!( + remote_branch_name("refs/remotes/origin/main").as_deref(), + Some("main") + ); + // A slash in the branch name belongs to the branch, not the remote. + assert_eq!( + remote_branch_name("refs/remotes/origin/feat/x").as_deref(), + Some("feat/x") + ); + assert_eq!(remote_branch_name("refs/heads/main"), None); + assert_eq!(remote_branch_name("refs/remotes/origin/"), None); + } + #[test] fn count_unpushed_commits_returns_none_for_invalid_path() { let path = PathBuf::from("/nonexistent/path/that/does/not/exist"); diff --git a/crates/okena-git/src/repository/worktree.rs b/crates/okena-git/src/repository/worktree.rs index 1ccedfc37..63d18fd21 100644 --- a/crates/okena-git/src/repository/worktree.rs +++ b/crates/okena-git/src/repository/worktree.rs @@ -465,6 +465,17 @@ pub fn create_worktree( let mut args = vec!["-C", repo_str, "worktree", "add"]; match &attachment { BranchAttachment::NewBranch(start_point) => { + // The start point is `origin/`, and git records a + // remote-tracking start point as the new branch's upstream + // (`branch.autoSetupMerge`). Nothing has been pushed yet, so that + // upstream would be the default branch: every lookup for "this + // branch's pushed commit" would answer with the default branch's, + // reporting its CI against a worktree that never triggered it. + // `git push -u` records the real one when there is something to + // record. + if start_point.is_some() { + args.push("--no-track"); + } args.push("-b"); args.push(branch); args.push(target_str); @@ -508,9 +519,15 @@ pub fn create_worktree_with_start_point( let repo_str = path_str(repo_path)?; let target_str = path_str(target_path)?; - let mut args = vec!["-C", repo_str, "worktree", "add", "-b", branch, target_str]; - let start_point = start_branch.and_then(|sb| resolve_start_ref(repo_path, sb)); + + let mut args = vec!["-C", repo_str, "worktree", "add"]; + // See `create_worktree`: a branch that has never been pushed must not claim + // the branch it started from as its upstream. + if start_point.is_some() { + args.push("--no-track"); + } + args.extend(["-b", branch, target_str]); if let Some(start_point) = &start_point { args.push(start_point); } @@ -904,7 +921,7 @@ fn path_identity(path: &Path) -> PathBuf { #[cfg(test)] mod tests { use super::*; - use crate::repository::test_support::{git_in, init_temp_repo}; + use crate::repository::test_support::{git_in, init_temp_repo, repo_with_origin}; use std::path::PathBuf; #[test] @@ -1070,6 +1087,56 @@ mod tests { ); } + /// A branch a worktree was just created on has never been pushed, so it has + /// no upstream. Git sets one when the start point is a remote-tracking ref, + /// and the start point here is `origin/`: the new branch would + /// claim the default branch as its upstream, and every lookup for "this + /// branch's pushed commit" would answer with the default branch's tip. + #[test] + fn a_new_worktree_branch_claims_no_upstream() { + let (_tmp, repo, _remote) = repo_with_origin(); + let target_parent = tempfile::tempdir().expect("create target parent"); + let target = target_parent.path().join("wt-feat"); + + create_worktree(&repo, "feat/x", &target, true).expect("create worktree"); + + assert_eq!( + git_out(&target, &["symbolic-ref", "--short", "HEAD"]), + "feat/x" + ); + assert_eq!( + git_out( + &repo, + &["config", "--default", "", "--get", "branch.feat/x.merge"] + ), + "", + "a branch with nothing pushed must not track the branch it started from" + ); + } + + /// Same for the pre-resolved start point path, which skips the fetch. + #[test] + fn a_worktree_created_from_a_start_point_claims_no_upstream() { + let (_tmp, repo, _remote) = repo_with_origin(); + let target_parent = tempfile::tempdir().expect("create target parent"); + let target = target_parent.path().join("wt-feat"); + + create_worktree_with_start_point(&repo, "feat/y", &target, Some("main")) + .expect("create worktree"); + + assert_eq!( + git_out(&target, &["symbolic-ref", "--short", "HEAD"]), + "feat/y" + ); + assert_eq!( + git_out( + &repo, + &["config", "--default", "", "--get", "branch.feat/y.merge"] + ), + "" + ); + } + /// A checkout is deleted while the machine keeps running, so a watcher or /// an indexer can drop a file into the tree between the walk and the final /// `rmdir`. That is a race, not a verdict: the removal used to report the @@ -1358,19 +1425,6 @@ mod tests { git_in(repo, &["-c", "commit.gpgsign=false", "commit", "-m", name]); } - /// A repo whose `main` is pushed to a bare `origin`. The second tempdir owns - /// the remote and must stay alive for the repo's lifetime. - fn repo_with_origin() -> (tempfile::TempDir, PathBuf, tempfile::TempDir) { - let (tmp, repo) = init_temp_repo(); - let remote_tmp = tempfile::tempdir().expect("create remote tempdir"); - let remote = remote_tmp.path().join("remote.git"); - let remote_str = remote.to_str().expect("remote path is utf-8"); - git_in(&repo, &["init", "--bare", "-b", "main", remote_str]); - git_in(&repo, &["remote", "add", "origin", remote_str]); - git_in(&repo, &["push", "-q", "origin", "main"]); - (tmp, repo, remote_tmp) - } - /// Push `feature` and drop the local copy, leaving only `origin/feature` — /// the shape a remote-only entry in the branch picker has. fn push_and_forget_feature(repo: &Path) { From 8ab0b49300467aa9011e2804206c16af2f54e713 Mon Sep 17 00:00:00 2001 From: jonasnobile Date: Mon, 21 Sep 2026 16:16:37 +0200 Subject: [PATCH 08/12] fix(worktree): bring a worktree's compose stack down before deleting it Closing a worktree kept failing on a checkout that nothing was wrong with. The project's own containers bind-mount it: a Postgres data directory, an object store, a dev server's state. Deleting it while they run is a race that cannot be won, because they keep writing into the tree while the delete walks it, and the final rmdir then reports a directory that is not empty. Retrying does not help against a live database. Closing already unloaded the project's services from the ServiceManager, but that only stops Okena from supervising them; nothing ever told Docker anything. A grep for `"down"` across okena-services, okena-daemon-core and okena-workspace found nothing: compose was only ever driven per service, with `up`, `stop` and `restart`. The stack now comes down before the checkout is deleted. That also settles the other half of the same bug: a removal that did succeed left the compose project running against files that no longer existed, which is how a stack outlived its worktree by hours. Best effort by design. A stack that will not come down leaves the removal to fail closed with the io cause, now with the compose failure alongside it, rather than adding a new way to block a close. Docker not running at all means nothing holds the checkout either, and that close should still go through. Diagnosed from a checkout whose delete kept failing while `docker compose ls` showed its stack running(7), with four containers mounting the directory and the Docker VM holding descriptors inside it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RkbvFzpmyCRY4DBp7oJHut --- Cargo.lock | 1 + crates/okena-daemon-core/src/command_loop.rs | 41 ++++++++++++- crates/okena-services/Cargo.toml | 1 + crates/okena-services/src/docker_compose.rs | 63 ++++++++++++++++++++ 4 files changed, 104 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5f0d2eac4..2a370a131 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6627,6 +6627,7 @@ dependencies = [ "serde_json", "serde_yaml_ng", "smol", + "tempfile", "thiserror 2.0.18", "uuid", ] diff --git a/crates/okena-daemon-core/src/command_loop.rs b/crates/okena-daemon-core/src/command_loop.rs index 8e3acfe4e..07bc8268b 100644 --- a/crates/okena-daemon-core/src/command_loop.rs +++ b/crates/okena-daemon-core/src/command_loop.rs @@ -37,7 +37,7 @@ //! before looping. use std::collections::{HashMap, HashSet}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Arc; use std::time::{Duration, Instant}; @@ -64,6 +64,7 @@ use okena_core::api::{ActionRequest, ApiGitStatus, ApiServiceInfo, ApiWindow, Co use okena_core::git_poll::{GitPollTrigger, git_poll_trigger_for_action}; use okena_remote_server::bridge::{BridgeMessage, BridgeReceiver, RemoteCommand}; use okena_services::config::{PreparedProjectConfig, prepare_project_config}; +use okena_services::docker_compose::{ComposeDown, compose_down}; use okena_services::manager::{ ComposeProjectIdentity, ServiceKind, ServiceLoadStatus, ServiceManager, ServiceProjectStateToken, @@ -439,6 +440,33 @@ fn cleanup_created_worktree_if_unclaimed( } } +/// Bring a worktree's own compose stack down before its checkout is deleted, +/// returning why it could not when it could not. +/// +/// Best effort on purpose. A stack that refuses to come down leaves the removal +/// to fail closed with the io cause, which is a better report than a new way to +/// block a close: Docker not running at all also means nothing is holding the +/// checkout, and that close should still go through. +fn stop_project_compose_stack(worktree_path: &Path) -> Option { + match compose_down(worktree_path) { + ComposeDown::NoComposeFile => None, + ComposeDown::Down => { + log::info!( + "worktree-close: compose stack at {} is down", + worktree_path.display() + ); + None + } + ComposeDown::Failed(reason) => { + log::warn!( + "worktree-close: compose stack at {} did not come down: {reason}", + worktree_path.display() + ); + Some(reason) + } + } +} + fn unload_project_services_for_background_removal( project_id: &str, service_manager: &Arc>, @@ -2298,7 +2326,16 @@ pub(crate) fn spawn_background_worktree_removal( .to_string(), ) } else { - plan.remove_fast().map_err(|error| error.to_string()) + // The checkout is bind-mounted into its own containers, so a + // delete that runs while they do keeps losing to whatever they + // write next. + let stack = stop_project_compose_stack(&worktree_path); + plan.remove_fast().map_err(|error| match &stack { + Some(reason) => format!( + "{error} (the project's compose stack did not come down: {reason})" + ), + None => error.to_string(), + }) }; let surviving_branch = if delete_branch && removal.is_ok() { okena_workspace::actions::worktree::delete_closed_worktree_branch( diff --git a/crates/okena-services/Cargo.toml b/crates/okena-services/Cargo.toml index 526a9f4ab..7e94b9e07 100644 --- a/crates/okena-services/Cargo.toml +++ b/crates/okena-services/Cargo.toml @@ -25,3 +25,4 @@ uuid = { version = "1.10", features = ["v4"] } [dev-dependencies] anyhow = "1.0" +tempfile = "3" diff --git a/crates/okena-services/src/docker_compose.rs b/crates/okena-services/src/docker_compose.rs index 07af58b64..8bc1814e4 100644 --- a/crates/okena-services/src/docker_compose.rs +++ b/crates/okena-services/src/docker_compose.rs @@ -87,6 +87,56 @@ pub fn detect_compose_file(project_path: &str) -> Option { None } +/// How long a stack gets to come down. `docker compose down` gives each +/// container a grace period before killing it, and a database with work to +/// flush uses it, so this is far longer than the few seconds a status poll gets. +const COMPOSE_DOWN_TIMEOUT: Duration = Duration::from_secs(120); + +/// What happened when a project's stack was asked to come down. +#[derive(Debug, PartialEq, Eq)] +pub enum ComposeDown { + /// No compose file in the directory, so there is no stack of its own. + NoComposeFile, + /// The stack is down: stopped just now, or not running to begin with. + Down, + /// Docker refused, was unavailable, or ran out of time. + Failed(String), +} + +/// Bring down the compose stack defined in `project_path`. +/// +/// A worktree's containers bind-mount its checkout: a Postgres data directory, +/// an object store, a dev server's state. Deleting that checkout while the +/// stack runs loses a race it cannot win, because the containers keep writing +/// into the tree while the delete walks it. Bringing the stack down first is +/// also what stops a compose project from outliving the directory that defined +/// it, still running against files that are gone. +/// +/// `down` without `-f` picks up the default file set, the override file +/// included, and derives the project name from the directory, which is how the +/// stack was started in the first place. +pub fn compose_down(project_path: &Path) -> ComposeDown { + let Some(path) = project_path.to_str() else { + return ComposeDown::Failed("project path is not valid UTF-8".to_string()); + }; + if detect_compose_file(path).is_none() { + return ComposeDown::NoComposeFile; + } + if !is_docker_compose_available() { + return ComposeDown::Failed("docker compose is not available".to_string()); + } + + let mut cmd = process::command("docker"); + cmd.args(["compose", "down"]).current_dir(path); + match process::safe_output_with_timeout(&mut cmd, COMPOSE_DOWN_TIMEOUT) { + Ok(output) if output.status.success() => ComposeDown::Down, + Ok(output) => { + ComposeDown::Failed(String::from_utf8_lossy(&output.stderr).trim().to_string()) + } + Err(error) => ComposeDown::Failed(error.to_string()), + } +} + /// Cache of parsed service lists keyed by `(project_path, compose_file)`, /// invalidated by the compose file's modification time. `docker compose config` /// is a heavy spawn whose output only changes when the file does, yet the @@ -730,6 +780,19 @@ mod tests { use super::*; use std::sync::atomic::{AtomicUsize, Ordering}; + /// A project without a compose file has no stack of its own, and must not + /// cost a docker spawn to establish that. Worktree removal calls this for + /// every checkout it deletes, most of which have nothing to do with Docker. + #[test] + fn a_directory_without_a_compose_file_has_no_stack() { + let tmp = tempfile::tempdir().expect("create temp dir"); + assert_eq!(compose_down(tmp.path()), ComposeDown::NoComposeFile); + + // A directory that is gone entirely is the orphaned-checkout case. + let missing = tmp.path().join("never-existed"); + assert_eq!(compose_down(&missing), ComposeDown::NoComposeFile); + } + struct FakeDockerPsRunner { calls: AtomicUsize, output: String, From 27b29fb1d6fbcf686a880f963f8ea21e9c4d2c95 Mon Sep 17 00:00:00 2001 From: jonasnobile Date: Mon, 21 Sep 2026 16:35:25 +0200 Subject: [PATCH 09/12] fix(git): name the entry that refused a delete, and admit what it already removed A worktree close reported `Permission denied` against the checkout root. What actually refused was one directory inside it: a `node_modules` that Docker Desktop holds as a mount point for a named volume and protects with a `deny delete` ACL. `std::fs::remove_dir_all` reports the io error without the path it happened on, so the report sent the reader to the wrong directory, and a failure that is completely explainable looked like the checkout itself being unreadable. Removal now walks the tree itself and annotates the failure with the entry that refused, keeping the error kind so the retry decision still reads the real one. The second half is worse and was invisible. A delete removes as it walks, so a refusal part-way through leaves the checkout short of whatever went before it. The failure path renames the directory back and reports that the checkout "remains open", which reads as nothing having happened: in the reported case fifteen tracked files were already gone. The message now says how many, and that `git restore .` brings back the tracked ones while untracked files it removed are gone for good. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RkbvFzpmyCRY4DBp7oJHut --- crates/okena-git/src/repository/worktree.rs | 140 +++++++++++++++++++- 1 file changed, 137 insertions(+), 3 deletions(-) diff --git a/crates/okena-git/src/repository/worktree.rs b/crates/okena-git/src/repository/worktree.rs index 63d18fd21..065a029f3 100644 --- a/crates/okena-git/src/repository/worktree.rs +++ b/crates/okena-git/src/repository/worktree.rs @@ -315,7 +315,7 @@ pub fn remove_orphaned_worktree(orphaned: &OrphanedWorktree) -> GitResult<()> { &orphaned.checkout_path, &orphaned.identity, &orphaned.parent_path, - |path| std::fs::remove_dir_all(path), + remove_tree, ) } @@ -601,7 +601,7 @@ pub fn remove_worktree(verified: &VerifiedWorktree, force: bool) -> GitResult<() /// This is safe because prune only acts on entries whose directories no longer exist, /// and we only delete the single target directory before pruning. pub fn remove_worktree_fast(verified: &VerifiedWorktree) -> GitResult<()> { - remove_worktree_fast_with(verified, |path| std::fs::remove_dir_all(path)) + remove_worktree_fast_with(verified, remove_tree) } fn remove_worktree_fast_with( @@ -700,6 +700,80 @@ fn delete_with_retries( } } +/// Annotate an io error with the path it happened on, keeping its kind so the +/// retry decision still sees the real one. +fn at_path(path: &Path, error: std::io::Error) -> std::io::Error { + std::io::Error::new(error.kind(), format!("{}: {error}", path.display())) +} + +/// Delete a tree, naming the entry that refused. +/// +/// `std::fs::remove_dir_all` reports the io error without the path it happened +/// on, so a refusal deep in the tree reads as a refusal of the whole checkout: +/// "Permission denied" on the worktree root, when what actually refused was a +/// `node_modules` that Docker holds as a mount point. Knowing which entry +/// refused is the difference between a report and a diagnosis. +/// +/// Symlinks are removed, never followed, matching `remove_dir_all`. +fn remove_tree(path: &Path) -> std::io::Result<()> { + let metadata = match std::fs::symlink_metadata(path) { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(at_path(path, error)), + }; + if !metadata.is_dir() { + return match std::fs::remove_file(path) { + Err(error) if error.kind() != std::io::ErrorKind::NotFound => Err(at_path(path, error)), + _ => Ok(()), + }; + } + match std::fs::read_dir(path) { + Ok(entries) => { + for entry in entries { + let entry = entry.map_err(|error| at_path(path, error))?; + // Already annotated by the nested call. + remove_tree(&entry.path())?; + } + } + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(at_path(path, error)), + } + match std::fs::remove_dir(path) { + Err(error) if error.kind() != std::io::ErrorKind::NotFound => Err(at_path(path, error)), + _ => Ok(()), + } +} + +/// What a half-finished deletion left behind, as a clause to append to the +/// failure. Empty when the checkout is whole, or when git cannot say. +/// +/// A delete removes as it walks, so a refusal part-way leaves the checkout +/// short of whatever went before it. Restoring the directory to its old path +/// makes the project openable again, but "the checkout remains" is only half +/// true, and the user has no reason to suspect the rest. +fn partial_checkout_note(worktree_path: &Path) -> String { + let Ok(path) = path_str(worktree_path) else { + return String::new(); + }; + let Ok(output) = safe_output(command("git").args(["-C", path, "status", "--porcelain"])) else { + return String::new(); + }; + if !output.status.success() { + return String::new(); + } + let deleted = String::from_utf8_lossy(&output.stdout) + .lines() + .filter(|line| line.starts_with(" D") || line.starts_with("D ")) + .count(); + if deleted == 0 { + return String::new(); + } + format!( + "; the deletion had already removed {deleted} tracked file(s) before it failed, \ + `git restore .` in the checkout puts them back (untracked files it removed are gone)" + ) +} + /// Whether a directory name is one this module generated. fn is_quarantine_name(name: &std::ffi::OsStr) -> bool { name.to_str() @@ -791,9 +865,14 @@ fn quarantine_and_delete( // only Finder metadata behind. Delete that narrow, verified class of // debris; otherwise restore the still-owned quarantine and fail closed. if let Err(cleanup_error) = cleanup_benign_residual(&quarantine) { + log::warn!( + "worktree removal: quarantine at '{}' still holds the checkout: {cleanup_error}", + quarantine.display() + ); let source = match std::fs::rename(&quarantine, worktree_path) { Ok(()) => std::io::Error::other(format!( - "{error}; residual cleanup refused: {cleanup_error}" + "{error}{}", + partial_checkout_note(worktree_path) )), Err(restore_error) => std::io::Error::new( error.kind(), @@ -1137,6 +1216,61 @@ mod tests { ); } + /// The failure has to name the entry that refused, not the root it was + /// asked to delete. A `node_modules` that Docker holds as a mount point + /// refuses with a permission error, and reporting that against the whole + /// checkout sends the reader looking at the wrong directory. + #[test] + fn a_refusal_names_the_entry_that_refused() { + let tmp = tempfile::tempdir().expect("create temp dir"); + let root = tmp.path().join("tree"); + let locked = root.join("pkg").join("held"); + std::fs::create_dir_all(&locked).expect("create tree"); + std::fs::write(root.join("keep.txt"), "x").expect("write file"); + // Take write permission off the parent, so its entry cannot be unlinked. + let parent = locked.parent().expect("held has a parent"); + let mut perms = std::fs::metadata(parent) + .expect("read permissions") + .permissions(); + let restore = perms.clone(); + std::os::unix::fs::PermissionsExt::set_mode(&mut perms, 0o500); + std::fs::set_permissions(parent, perms).expect("drop write permission"); + + let error = remove_tree(&root).expect_err("a locked entry must refuse"); + let message = error.to_string(); + + std::fs::set_permissions(parent, restore).expect("restore permissions"); + + assert!( + message.contains("held"), + "the entry that refused must be named: {message}" + ); + assert_eq!( + error.kind(), + std::io::ErrorKind::PermissionDenied, + "the kind must survive annotation, the retry decision reads it" + ); + } + + /// A tree with nothing in the way is removed, symlinks included, and a path + /// that is already gone is not an error. + #[test] + fn a_clear_tree_is_removed_whole() { + let tmp = tempfile::tempdir().expect("create temp dir"); + let root = tmp.path().join("tree"); + std::fs::create_dir_all(root.join("nested")).expect("create tree"); + std::fs::write(root.join("nested").join("file.txt"), "x").expect("write file"); + let outside = tmp.path().join("outside.txt"); + std::fs::write(&outside, "must survive").expect("write outside file"); + std::os::unix::fs::symlink(&outside, root.join("link")).expect("create symlink"); + + remove_tree(&root).expect("remove the tree"); + + assert!(!root.exists()); + assert!(outside.exists(), "a symlink is removed, never followed"); + remove_tree(&root).expect("removing what is already gone is not a failure"); + } + /// A checkout is deleted while the machine keeps running, so a watcher or /// an indexer can drop a file into the tree between the walk and the final /// `rmdir`. That is a race, not a verdict: the removal used to report the From ea0285b5e184938d0ea0c8b36818c3575ee0a392 Mon Sep 17 00:00:00 2001 From: jonasnobile Date: Mon, 21 Sep 2026 16:57:07 +0200 Subject: [PATCH 10/12] fix(test): keep the delete-failure tests off unix-only APIs on Windows `check-windows` could not compile them: one stages its refusal with unix permissions, the other creates a symlink, and neither `os::unix` exists there. The refusal test is now unix-only, following the symlink-alias test beside it; the tree-removal test still runs everywhere, with only the symlink part gated, so Windows keeps the coverage that a clear tree is removed whole. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RkbvFzpmyCRY4DBp7oJHut --- crates/okena-git/src/repository/worktree.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/okena-git/src/repository/worktree.rs b/crates/okena-git/src/repository/worktree.rs index 065a029f3..82f90c75a 100644 --- a/crates/okena-git/src/repository/worktree.rs +++ b/crates/okena-git/src/repository/worktree.rs @@ -1220,6 +1220,10 @@ mod tests { /// asked to delete. A `node_modules` that Docker holds as a mount point /// refuses with a permission error, and reporting that against the whole /// checkout sends the reader looking at the wrong directory. + /// + /// Unix-only because the refusal is staged with unix permissions; the + /// annotation it checks is platform-independent. + #[cfg(unix)] #[test] fn a_refusal_names_the_entry_that_refused() { let tmp = tempfile::tempdir().expect("create temp dir"); @@ -1262,11 +1266,14 @@ mod tests { std::fs::write(root.join("nested").join("file.txt"), "x").expect("write file"); let outside = tmp.path().join("outside.txt"); std::fs::write(&outside, "must survive").expect("write outside file"); + // Creating one needs a privilege on Windows that a test cannot assume. + #[cfg(unix)] std::os::unix::fs::symlink(&outside, root.join("link")).expect("create symlink"); remove_tree(&root).expect("remove the tree"); assert!(!root.exists()); + #[cfg(unix)] assert!(outside.exists(), "a symlink is removed, never followed"); remove_tree(&root).expect("removing what is already gone is not a failure"); } From 8b928e67cdaa7e1365f59dbd1d4e144376aaaa88 Mon Sep 17 00:00:00 2001 From: David Matejka Date: Thu, 1 Oct 2026 11:19:31 +0200 Subject: [PATCH 11/12] fix(test): publish the descendant PID atomically The shell creates the redirection target before echo writes the PID. Waiting for file existence could therefore read an empty string and fail CI. Rename a complete temporary file into place before the test proceeds. --- crates/okena-terminal/src/pty_manager.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/okena-terminal/src/pty_manager.rs b/crates/okena-terminal/src/pty_manager.rs index c262644d9..804fef37e 100644 --- a/crates/okena-terminal/src/pty_manager.rs +++ b/crates/okena-terminal/src/pty_manager.rs @@ -2661,7 +2661,8 @@ mod tests { program: "/bin/sh".to_string(), args: vec![ "-c".to_string(), - "sleep 30 & echo $! > \"$1\"; wait".to_string(), + // Publish only the complete PID; redirection creates an empty file first. + "sleep 30 & echo $! > \"$1.tmp\"; mv \"$1.tmp\" \"$1\"; wait".to_string(), "okena-test".to_string(), child_pid_file.to_string_lossy().into_owned(), ], From 016dadbbbd1f5417c9827b988244ecb6c650c9a8 Mon Sep 17 00:00:00 2001 From: David Matejka Date: Thu, 1 Oct 2026 11:32:37 +0200 Subject: [PATCH 12/12] fix(worktree): limit removal to the verified checkout Defer automatic quarantine collection and Compose shutdown until ownership can be proved across processes and checkouts. A quarantine name alone does not authorize deletion, and implicit Compose configuration can target another stack. Keep the standard library's race-resistant directory removal with transient-error retries. --- Cargo.lock | 1 - crates/okena-daemon-core/src/command_loop.rs | 41 +--- crates/okena-git/src/repository/worktree.rs | 217 +++---------------- crates/okena-services/Cargo.toml | 1 - crates/okena-services/src/docker_compose.rs | 63 ------ 5 files changed, 28 insertions(+), 295 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2a370a131..5f0d2eac4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6627,7 +6627,6 @@ dependencies = [ "serde_json", "serde_yaml_ng", "smol", - "tempfile", "thiserror 2.0.18", "uuid", ] diff --git a/crates/okena-daemon-core/src/command_loop.rs b/crates/okena-daemon-core/src/command_loop.rs index 07bc8268b..8e3acfe4e 100644 --- a/crates/okena-daemon-core/src/command_loop.rs +++ b/crates/okena-daemon-core/src/command_loop.rs @@ -37,7 +37,7 @@ //! before looping. use std::collections::{HashMap, HashSet}; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::sync::Arc; use std::time::{Duration, Instant}; @@ -64,7 +64,6 @@ use okena_core::api::{ActionRequest, ApiGitStatus, ApiServiceInfo, ApiWindow, Co use okena_core::git_poll::{GitPollTrigger, git_poll_trigger_for_action}; use okena_remote_server::bridge::{BridgeMessage, BridgeReceiver, RemoteCommand}; use okena_services::config::{PreparedProjectConfig, prepare_project_config}; -use okena_services::docker_compose::{ComposeDown, compose_down}; use okena_services::manager::{ ComposeProjectIdentity, ServiceKind, ServiceLoadStatus, ServiceManager, ServiceProjectStateToken, @@ -440,33 +439,6 @@ fn cleanup_created_worktree_if_unclaimed( } } -/// Bring a worktree's own compose stack down before its checkout is deleted, -/// returning why it could not when it could not. -/// -/// Best effort on purpose. A stack that refuses to come down leaves the removal -/// to fail closed with the io cause, which is a better report than a new way to -/// block a close: Docker not running at all also means nothing is holding the -/// checkout, and that close should still go through. -fn stop_project_compose_stack(worktree_path: &Path) -> Option { - match compose_down(worktree_path) { - ComposeDown::NoComposeFile => None, - ComposeDown::Down => { - log::info!( - "worktree-close: compose stack at {} is down", - worktree_path.display() - ); - None - } - ComposeDown::Failed(reason) => { - log::warn!( - "worktree-close: compose stack at {} did not come down: {reason}", - worktree_path.display() - ); - Some(reason) - } - } -} - fn unload_project_services_for_background_removal( project_id: &str, service_manager: &Arc>, @@ -2326,16 +2298,7 @@ pub(crate) fn spawn_background_worktree_removal( .to_string(), ) } else { - // The checkout is bind-mounted into its own containers, so a - // delete that runs while they do keeps losing to whatever they - // write next. - let stack = stop_project_compose_stack(&worktree_path); - plan.remove_fast().map_err(|error| match &stack { - Some(reason) => format!( - "{error} (the project's compose stack did not come down: {reason})" - ), - None => error.to_string(), - }) + plan.remove_fast().map_err(|error| error.to_string()) }; let surviving_branch = if delete_branch && removal.is_ok() { okena_workspace::actions::worktree::delete_closed_worktree_branch( diff --git a/crates/okena-git/src/repository/worktree.rs b/crates/okena-git/src/repository/worktree.rs index 82f90c75a..4ea85f328 100644 --- a/crates/okena-git/src/repository/worktree.rs +++ b/crates/okena-git/src/repository/worktree.rs @@ -1,8 +1,6 @@ //! Worktree operations: create / remove / list. -use std::collections::BTreeSet; use std::path::{Path, PathBuf}; -use std::sync::Mutex; use std::time::Duration; use okena_core::process::{command, safe_output}; @@ -315,7 +313,7 @@ pub fn remove_orphaned_worktree(orphaned: &OrphanedWorktree) -> GitResult<()> { &orphaned.checkout_path, &orphaned.identity, &orphaned.parent_path, - remove_tree, + |path| std::fs::remove_dir_all(path), ) } @@ -601,7 +599,7 @@ pub fn remove_worktree(verified: &VerifiedWorktree, force: bool) -> GitResult<() /// This is safe because prune only acts on entries whose directories no longer exist, /// and we only delete the single target directory before pruning. pub fn remove_worktree_fast(verified: &VerifiedWorktree) -> GitResult<()> { - remove_worktree_fast_with(verified, remove_tree) + remove_worktree_fast_with(verified, |path| std::fs::remove_dir_all(path)) } fn remove_worktree_fast_with( @@ -625,38 +623,6 @@ const QUARANTINE_PREFIX: &str = ".okena-removing-"; const DELETE_ATTEMPTS: usize = 4; const DELETE_RETRY_DELAY: Duration = Duration::from_millis(150); -/// Quarantine directories this process is deleting right now. -/// -/// The reclaim sweep runs in the same parent directory as live removals, so a -/// worktree being closed at this moment must not have its quarantine pulled out -/// from under it by a sweep running for another project. -static QUARANTINES_IN_FLIGHT: Mutex> = Mutex::new(BTreeSet::new()); - -/// A quarantine path registered for as long as its removal is running. -struct InFlightQuarantine(PathBuf); - -impl InFlightQuarantine { - fn register(path: &Path) -> Self { - let owned = path.to_path_buf(); - in_flight_quarantines().insert(owned.clone()); - Self(owned) - } -} - -impl Drop for InFlightQuarantine { - fn drop(&mut self) { - in_flight_quarantines().remove(&self.0); - } -} - -/// The in-flight set, usable even after a panic poisoned the lock: the set is -/// plain paths, so a poisoned one is no less valid than a healthy one. -fn in_flight_quarantines() -> std::sync::MutexGuard<'static, BTreeSet> { - QUARANTINES_IN_FLIGHT - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) -} - /// Whether a failed delete is worth another attempt. /// /// `remove_dir_all` walks the tree and then removes the directory itself, so @@ -700,50 +666,6 @@ fn delete_with_retries( } } -/// Annotate an io error with the path it happened on, keeping its kind so the -/// retry decision still sees the real one. -fn at_path(path: &Path, error: std::io::Error) -> std::io::Error { - std::io::Error::new(error.kind(), format!("{}: {error}", path.display())) -} - -/// Delete a tree, naming the entry that refused. -/// -/// `std::fs::remove_dir_all` reports the io error without the path it happened -/// on, so a refusal deep in the tree reads as a refusal of the whole checkout: -/// "Permission denied" on the worktree root, when what actually refused was a -/// `node_modules` that Docker holds as a mount point. Knowing which entry -/// refused is the difference between a report and a diagnosis. -/// -/// Symlinks are removed, never followed, matching `remove_dir_all`. -fn remove_tree(path: &Path) -> std::io::Result<()> { - let metadata = match std::fs::symlink_metadata(path) { - Ok(metadata) => metadata, - Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), - Err(error) => return Err(at_path(path, error)), - }; - if !metadata.is_dir() { - return match std::fs::remove_file(path) { - Err(error) if error.kind() != std::io::ErrorKind::NotFound => Err(at_path(path, error)), - _ => Ok(()), - }; - } - match std::fs::read_dir(path) { - Ok(entries) => { - for entry in entries { - let entry = entry.map_err(|error| at_path(path, error))?; - // Already annotated by the nested call. - remove_tree(&entry.path())?; - } - } - Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), - Err(error) => return Err(at_path(path, error)), - } - match std::fs::remove_dir(path) { - Err(error) if error.kind() != std::io::ErrorKind::NotFound => Err(at_path(path, error)), - _ => Ok(()), - } -} - /// What a half-finished deletion left behind, as a clause to append to the /// failure. Empty when the checkout is whole, or when git cannot say. /// @@ -774,47 +696,6 @@ fn partial_checkout_note(worktree_path: &Path) -> String { ) } -/// Whether a directory name is one this module generated. -fn is_quarantine_name(name: &std::ffi::OsStr) -> bool { - name.to_str() - .and_then(|name| name.strip_prefix(QUARANTINE_PREFIX)) - .is_some_and(|id| uuid::Uuid::parse_str(id).is_ok()) -} - -/// Delete quarantines an earlier removal could neither delete nor put back. -/// -/// Such a directory is a whole checkout under a hidden name: left alone it is a -/// disk leak the user has no way to see, and the worktree it holds was already -/// confirmed for deletion. Best effort by design, and never fatal to the -/// removal that is starting: a quarantine that still refuses to go gets logged -/// and waits for the next attempt. -fn reclaim_abandoned_quarantines(parent: &Path) { - let Ok(entries) = std::fs::read_dir(parent) else { - return; - }; - for entry in entries.flatten() { - if !is_quarantine_name(&entry.file_name()) - || !entry.file_type().is_ok_and(|kind| kind.is_dir()) - { - continue; - } - let path = entry.path(); - if in_flight_quarantines().contains(&path) { - continue; - } - match delete_with_retries(&path, |path| std::fs::remove_dir_all(path)) { - Ok(()) => log::info!( - "worktree removal: reclaimed an abandoned quarantine at '{}'", - path.display() - ), - Err(error) => log::warn!( - "worktree removal: abandoned quarantine at '{}' could not be reclaimed: {error}", - path.display() - ), - } - } -} - /// Rename the checkout aside, re-prove it is still the directory whose /// `identity` was verified, delete it, then prune the parent's stale worktree /// metadata. Shared by the verified and orphaned removal paths so both get the @@ -829,15 +710,12 @@ fn quarantine_and_delete( let parent = worktree_path .parent() .ok_or_else(|| unsafe_worktree(worktree_path, "checkout directory has no parent"))?; - reclaim_abandoned_quarantines(parent); let quarantine = parent.join(format!("{QUARANTINE_PREFIX}{}", uuid::Uuid::new_v4())); std::fs::rename(worktree_path, &quarantine).map_err(|source| GitError::RemoveFailed { path: worktree_path.to_path_buf(), source, })?; - let _in_flight = InFlightQuarantine::register(&quarantine); - let quarantined_identity = filesystem_object_identity(&quarantine); if quarantined_identity.as_ref() != Some(identity) { let restore = if !worktree_path.exists() { @@ -1216,66 +1094,31 @@ mod tests { ); } - /// The failure has to name the entry that refused, not the root it was - /// asked to delete. A `node_modules` that Docker holds as a mount point - /// refuses with a permission error, and reporting that against the whole - /// checkout sends the reader looking at the wrong directory. - /// - /// Unix-only because the refusal is staged with unix permissions; the - /// annotation it checks is platform-independent. #[cfg(unix)] #[test] - fn a_refusal_names_the_entry_that_refused() { - let tmp = tempfile::tempdir().expect("create temp dir"); - let root = tmp.path().join("tree"); - let locked = root.join("pkg").join("held"); - std::fs::create_dir_all(&locked).expect("create tree"); - std::fs::write(root.join("keep.txt"), "x").expect("write file"); - // Take write permission off the parent, so its entry cannot be unlinked. - let parent = locked.parent().expect("held has a parent"); - let mut perms = std::fs::metadata(parent) - .expect("read permissions") - .permissions(); - let restore = perms.clone(); - std::os::unix::fs::PermissionsExt::set_mode(&mut perms, 0o500); - std::fs::set_permissions(parent, perms).expect("drop write permission"); - - let error = remove_tree(&root).expect_err("a locked entry must refuse"); - let message = error.to_string(); - - std::fs::set_permissions(parent, restore).expect("restore permissions"); - - assert!( - message.contains("held"), - "the entry that refused must be named: {message}" - ); - assert_eq!( - error.kind(), - std::io::ErrorKind::PermissionDenied, - "the kind must survive annotation, the retry decision reads it" + fn fast_removal_does_not_follow_directory_symlinks() { + let (_tmp, repo) = init_temp_repo(); + let wt_tmp = tempfile::tempdir().expect("create worktree tempdir"); + let root = wt_tmp.path().join("wt-feat"); + git_in( + &repo, + &["worktree", "add", root.to_str().unwrap(), "-b", "feat"], ); - } - - /// A tree with nothing in the way is removed, symlinks included, and a path - /// that is already gone is not an error. - #[test] - fn a_clear_tree_is_removed_whole() { - let tmp = tempfile::tempdir().expect("create temp dir"); - let root = tmp.path().join("tree"); std::fs::create_dir_all(root.join("nested")).expect("create tree"); std::fs::write(root.join("nested").join("file.txt"), "x").expect("write file"); - let outside = tmp.path().join("outside.txt"); - std::fs::write(&outside, "must survive").expect("write outside file"); - // Creating one needs a privilege on Windows that a test cannot assume. - #[cfg(unix)] + let outside = wt_tmp.path().join("outside"); + std::fs::create_dir(&outside).expect("create outside directory"); + std::fs::write(outside.join("keep.txt"), "must survive").expect("write outside file"); std::os::unix::fs::symlink(&outside, root.join("link")).expect("create symlink"); - remove_tree(&root).expect("remove the tree"); + let verified = verify_linked_worktree_fresh(&repo, &root).expect("verify worktree"); + remove_worktree_fast(&verified).expect("remove the worktree"); assert!(!root.exists()); - #[cfg(unix)] - assert!(outside.exists(), "a symlink is removed, never followed"); - remove_tree(&root).expect("removing what is already gone is not a failure"); + assert_eq!( + std::fs::read_to_string(outside.join("keep.txt")).unwrap(), + "must survive" + ); } /// A checkout is deleted while the machine keeps running, so a watcher or @@ -1306,7 +1149,7 @@ mod tests { assert_eq!(attempts.get(), 2, "the second attempt should have run"); assert!(!wt_path.exists(), "the checkout is gone"); assert!( - quarantines_in(wt_tmp.path()).next().is_none(), + std::fs::read_dir(wt_tmp.path()).unwrap().next().is_none(), "no quarantine is left behind" ); } @@ -1340,18 +1183,15 @@ mod tests { assert!(wt_path.exists(), "the checkout is restored, not lost"); } - /// A quarantine that could be neither deleted nor restored is a whole - /// checkout under a hidden name. The next removal in that directory - /// reclaims it; anything else hidden there is left alone. #[test] - fn the_next_removal_reclaims_an_abandoned_quarantine() { + fn removal_preserves_preexisting_quarantines() { let (_tmp, repo) = init_temp_repo(); let wt_tmp = tempfile::tempdir().expect("create worktree tempdir"); let abandoned = wt_tmp .path() .join(format!("{QUARANTINE_PREFIX}{}", uuid::Uuid::new_v4())); std::fs::create_dir_all(abandoned.join("src")).expect("create abandoned quarantine"); - std::fs::write(abandoned.join("src").join("main.rs"), "leaked checkout") + std::fs::write(abandoned.join("src").join("main.rs"), "preserved checkout") .expect("fill abandoned quarantine"); let foreign = wt_tmp.path().join(format!("{QUARANTINE_PREFIX}not-a-uuid")); std::fs::create_dir(&foreign).expect("create lookalike directory"); @@ -1364,22 +1204,17 @@ mod tests { let verified = verify_linked_worktree_fresh(&repo, &wt_path).expect("verify worktree"); remove_worktree_fast(&verified).expect("remove worktree"); - assert!(!abandoned.exists(), "the leaked checkout is reclaimed"); + assert!(!wt_path.exists(), "the requested checkout is removed"); + assert_eq!( + std::fs::read_to_string(abandoned.join("src/main.rs")).unwrap(), + "preserved checkout" + ); assert!( foreign.exists(), "a name this module never wrote is not ours" ); } - /// Every quarantine left in `parent`, whatever its uuid. - fn quarantines_in(parent: &Path) -> impl Iterator { - std::fs::read_dir(parent) - .expect("inspect worktree parent") - .filter_map(Result::ok) - .filter(|entry| is_quarantine_name(&entry.file_name())) - .map(|entry| entry.path()) - } - #[test] fn guarded_fast_removal_rejects_a_replaced_checkout() { let (_tmp, repo) = init_temp_repo(); diff --git a/crates/okena-services/Cargo.toml b/crates/okena-services/Cargo.toml index 7e94b9e07..526a9f4ab 100644 --- a/crates/okena-services/Cargo.toml +++ b/crates/okena-services/Cargo.toml @@ -25,4 +25,3 @@ uuid = { version = "1.10", features = ["v4"] } [dev-dependencies] anyhow = "1.0" -tempfile = "3" diff --git a/crates/okena-services/src/docker_compose.rs b/crates/okena-services/src/docker_compose.rs index 8bc1814e4..07af58b64 100644 --- a/crates/okena-services/src/docker_compose.rs +++ b/crates/okena-services/src/docker_compose.rs @@ -87,56 +87,6 @@ pub fn detect_compose_file(project_path: &str) -> Option { None } -/// How long a stack gets to come down. `docker compose down` gives each -/// container a grace period before killing it, and a database with work to -/// flush uses it, so this is far longer than the few seconds a status poll gets. -const COMPOSE_DOWN_TIMEOUT: Duration = Duration::from_secs(120); - -/// What happened when a project's stack was asked to come down. -#[derive(Debug, PartialEq, Eq)] -pub enum ComposeDown { - /// No compose file in the directory, so there is no stack of its own. - NoComposeFile, - /// The stack is down: stopped just now, or not running to begin with. - Down, - /// Docker refused, was unavailable, or ran out of time. - Failed(String), -} - -/// Bring down the compose stack defined in `project_path`. -/// -/// A worktree's containers bind-mount its checkout: a Postgres data directory, -/// an object store, a dev server's state. Deleting that checkout while the -/// stack runs loses a race it cannot win, because the containers keep writing -/// into the tree while the delete walks it. Bringing the stack down first is -/// also what stops a compose project from outliving the directory that defined -/// it, still running against files that are gone. -/// -/// `down` without `-f` picks up the default file set, the override file -/// included, and derives the project name from the directory, which is how the -/// stack was started in the first place. -pub fn compose_down(project_path: &Path) -> ComposeDown { - let Some(path) = project_path.to_str() else { - return ComposeDown::Failed("project path is not valid UTF-8".to_string()); - }; - if detect_compose_file(path).is_none() { - return ComposeDown::NoComposeFile; - } - if !is_docker_compose_available() { - return ComposeDown::Failed("docker compose is not available".to_string()); - } - - let mut cmd = process::command("docker"); - cmd.args(["compose", "down"]).current_dir(path); - match process::safe_output_with_timeout(&mut cmd, COMPOSE_DOWN_TIMEOUT) { - Ok(output) if output.status.success() => ComposeDown::Down, - Ok(output) => { - ComposeDown::Failed(String::from_utf8_lossy(&output.stderr).trim().to_string()) - } - Err(error) => ComposeDown::Failed(error.to_string()), - } -} - /// Cache of parsed service lists keyed by `(project_path, compose_file)`, /// invalidated by the compose file's modification time. `docker compose config` /// is a heavy spawn whose output only changes when the file does, yet the @@ -780,19 +730,6 @@ mod tests { use super::*; use std::sync::atomic::{AtomicUsize, Ordering}; - /// A project without a compose file has no stack of its own, and must not - /// cost a docker spawn to establish that. Worktree removal calls this for - /// every checkout it deletes, most of which have nothing to do with Docker. - #[test] - fn a_directory_without_a_compose_file_has_no_stack() { - let tmp = tempfile::tempdir().expect("create temp dir"); - assert_eq!(compose_down(tmp.path()), ComposeDown::NoComposeFile); - - // A directory that is gone entirely is the orphaned-checkout case. - let missing = tmp.path().join("never-existed"); - assert_eq!(compose_down(&missing), ComposeDown::NoComposeFile); - } - struct FakeDockerPsRunner { calls: AtomicUsize, output: String,