Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions src/process/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,16 @@ impl ProcessBuffer {
/// lines into the output.
pub(super) fn into_out(self) -> Vec<String> { self.out }

/// Drains any buffered lines into `out`, reflowing them as a table when
/// the run was recognised as one.
///
/// Upholds the buffer invariant that `buf` is empty and `in_table` is
/// `false` on return, so the next line starts a fresh detection window.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Document the empty-buffer exception to the postcondition

When buf is empty and in_table is true, the guard at the start of flush returns without clearing in_table, so this newly documented unconditional postcondition is false. The existing flush_empty_buffer_is_noop unit test explicitly constructs that state and asserts that in_table remains true; qualify the invariant for non-empty buffers or reset the flag before returning.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai Has this now been resolved in the latest commit?

Use codegraph analysis to determine your answer.

If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the current Rust module before reading the two relevant implementations.
ast-grep outline src/process/buffer.rs --items all --type function,method --match 'flush|flush_empty_buffer_is_noop'

# Retrieve the current flush method and its regression test with enclosing context.
rg -n -A75 -B12 'pub\(super\) fn flush\(&mut self\)' src/process/buffer.rs
rg -n -A35 -B10 'fn flush_empty_buffer_is_noop\s*\(' src/process/buffer/tests.rs 2>/dev/null || true

# Show the current PR revision metadata and the target thread state without modifying it.
git status --short
git rev-parse HEAD
gh api repos/leynos/mdtablefix/pulls/414 --jq '{number, head: .head.sha}'
gh api graphql -F owner='leynos' -F name='mdtablefix' -F number=414 -f query='
query($owner: String!, $name: String!, $number: Int!) {
  repository(owner: $owner, name: $name) {
    pullRequest(number: $number) {
      reviewThreads(first: 100) {
        nodes {
          id
          isResolved
          path
          line
          comments(first: 20) {
            nodes {
              author { login }
              body
              url
            }
          }
        }
      }
    }
  }
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[]
  | select(.comments.nodes[]? | (.url == "https://github.com/leynos/mdtablefix/pull/414#discussion_r3653612534"))
  | {id, isResolved, path, line, comments}'

Length of output: 10181


@leynos, treat this comment as resolved.

Codegraph analysis confirms that ProcessBuffer::flush now documents the empty-buffer exception at Lines 71-76. The empty-buffer guard still returns at Lines 86-88. The regression test flush_empty_buffer_is_noop confirms that in_table remains true for this case.

The checked-out commit matches the latest PR head: 9a0f5c309163398dda8f1df5e62edcab698188c6.

Resolve the thread manually. I cannot use the API to resolve it because chatgpt-codex-connector created the original review comment, and I can resolve only comments created by this assistant identity.


🧬 Code Graph Analysis Results

src/wrap/block.rs:11-28

pub(crate) fn leading_indent(line: &str) -> (usize, usize) {
    let mut width = 0;
    let mut bytes = 0;
    for &b in line.as_bytes() {
        match b {
            b' ' => {
                width += 1;
                bytes += 1;
            }
            0x09 => {
                width += 4;
                bytes += 1;
            }
            _ => break,
        }
    }
    (width, bytes)
}

Returns indentation width and byte length, counting tabs as four columns.

src/wrap/block.rs:91-122

classify_block(line, link_matcher) returns an optional BlockKind. It recognizes unindented headings, bullets, blockquotes, footnote definitions, link-reference definitions, Markdownlint directives, and digit-prefixed blocks. Lines indented four or more columns are not classified by these checks.

src/ellipsis.rs:130-177

replace_ellipsis(lines) processes each line while tracking fences, indented code, link-reference definitions, and link-title continuations. It preserves fence lines, code, and link-reference-related lines verbatim; other prose lines are passed to ellipsis replacement.

You are interacting with an AI system.

/// Ellipsis replacement is applied here, *before* [`reflow_table`], because
/// the substitution must reach the cell text while it is still row-shaped;
/// running it after reflow would have to re-parse the emitted table. An
/// empty buffer short-circuits so ordering against [`push_out`](Self::push_out)
/// is preserved without emitting a spurious blank flush.
pub(super) fn flush(&mut self) {
debug!(
in_table = self.in_table,
Expand All @@ -88,11 +98,26 @@ impl ProcessBuffer {
self.in_table = false;
}

/// Emits `line` verbatim after first flushing any pending table.
///
/// The flush is mandatory: a verbatim line (a code fence, for instance)
/// closes whatever table run preceded it, and appending it directly to
/// `out` without flushing would let it jump ahead of buffered rows that
/// belong earlier in the document. Flushing first keeps source ordering
/// intact.
pub(super) fn push_verbatim(&mut self, line: &str) {
self.flush();
self.out.push(line.to_string());
}

/// Consumes a code-fence marker line, returning `true` when it was handled.
///
/// A fence marker can never be part of a table, so it must terminate the
/// current run; the line is emitted through [`push_verbatim`](Self::push_verbatim)
/// so the pending table flushes first and ordering is preserved. Non-marker
/// lines return `false` immediately, signalling the caller to fall through
/// to its in-fence and table-detection handling; this method deliberately
/// makes no decision about lines *inside* a fence.
pub(super) fn handle_fence_line(&mut self, line: &str, is_fence_marker: bool) -> bool {
if !is_fence_marker {
return false;
Expand All @@ -102,6 +127,19 @@ impl ProcessBuffer {
true
}

/// Routes a non-fence line through table detection, buffering it or handing
/// it back for verbatim emission.
///
/// Returns `None` when the line has been absorbed into the pending table
/// run (`buf`), and `Some(line)` when the caller should emit it after the
/// buffer has been flushed. The invariant is that `buf` only ever holds
/// genuine table rows: every path that meets a line which cannot belong to
/// the current table flushes before yielding it, so a stray row can never
/// make [`reflow_table`] bail on an otherwise valid table. The ordering of
Comment on lines +135 to +140

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Narrow the buf invariant.

The code uses permissive heuristics. It buffers non-indented lines that start with |, and while in_table it also buffers lines that contain | or match SEP_RE. Therefore, buf is not guaranteed to contain only genuine table rows, and an accepted stray line can still reach reflow_table. Replace this guarantee with wording that describes lines accepted by the table-detection heuristics.

Proposed wording
-    /// The invariant is that `buf` only ever holds genuine table rows: every path that meets a line which cannot belong to the current table flushes before yielding it, so a stray row can never make [`reflow_table`] bail on an otherwise valid table.
+    /// The buffer holds lines accepted by the table-detection heuristics.
+    /// Lines that fail those heuristics flush the current run before the
+    /// caller emits them; the heuristics do not prove that every buffered
+    /// line is a valid table row.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/process/buffer.rs` around lines 135 - 140, Update the documentation
comment above the relevant buffer-handling function to remove the claim that
`buf` contains only genuine table rows and cannot contain strays. Describe
instead that it stores lines accepted by the table-detection heuristics, and
that lines failing those heuristics flush the current run before emission;
retain the existing [`reflow_table`] reference only if it remains accurate.

/// the guards is load-bearing — indented code blocks and block boundaries
/// (see the inline comments) must be recognised *before* the permissive
/// pipe heuristic, which would otherwise swallow lines that merely happen
/// to contain a `|`.
pub(super) fn handle_table_line(&mut self, line: String) -> Option<String> {
// A leading indent of four or more columns marks a Markdown indented
// code block, so such a line must stay verbatim and never enter table
Expand Down
35 changes: 35 additions & 0 deletions src/wrap/continuation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,22 @@ pub(super) fn apply_continuation_chunk(
}
}

/// Splits the pending buffer at a close-then-reopen boundary, emitting the
/// resolved prefix and re-seeding `pending` with the freshly reopened span.
///
/// A single continuation chunk can close the currently open code span and open
/// a new one; this helper exists for exactly that case. It flushes everything
/// up to `split_at`, then rebuilds `pending.rest` so it holds only the reopened
/// span, prefixed with its `new_len` backtick run. Two invariants must survive
/// the rewrite: `synthetic_join_spaces` offsets are rebased onto the new,
/// shorter `rest` (dropping any that fall before the split), and
/// `open_fence_len` is re-derived from that `rest` so subsequent chunks scan
/// against the correct fence width. When the reopened opener sits at end of
/// line (`opener_at_eol`) the mode is forced to `TightCodeSpan`: `CommonMark`
/// would turn the following soft break into a space inside the span, and
/// preserving it would emit an MD038 (space-inside-code-span) violation.
/// Returns `true` when the reopened span is already closed, signalling the
/// caller to flush immediately.
fn reopen_pending_span(
writer: &mut ParagraphWriter<'_>,
pending: &mut PendingPrefix,
Expand Down Expand Up @@ -277,6 +293,17 @@ fn leading_run_needs_space(
}
}

/// Reports whether a join space must be suppressed because the open code span
/// ends on a nested, still-unclosed `(`.
///
/// Normally a soft-wrapped continuation is joined with a single space, but that
/// space is wrong when the pending text ends mid-token inside a code span — for
/// example a Rust path such as `foo((bar` split after the inner `(`. The guard
/// only fires when a span is actually open (`open_fence_len > 0`) and `existing`
/// ends with `(`, and only when the parenthesis nesting inside the span is
/// deeper than one: a single open paren is an ordinary word boundary that should
/// still take the space, whereas depth `> 1` signals a nested construct whose
/// halves must abut. See [`unclosed_parenthesis_depth`] for the depth count.
fn suppresses_join_space_after_nested_open_paren(existing: &str, open_fence_len: usize) -> bool {
if open_fence_len == 0 || !existing.ends_with('(') {
return false;
Expand All @@ -289,6 +316,14 @@ fn suppresses_join_space_after_nested_open_paren(existing: &str, open_fence_len:
unclosed_parenthesis_depth(code_tail) > 1
}

/// Counts the net depth of unclosed `(` runs left open by `text`.
///
/// The count saturates at zero on `)`, so a span containing more closers than
/// openers (or a leading `)`) never underflows the `usize` and reports depth
/// `0` rather than panicking. This tolerance is deliberate: the caller only
/// cares whether nesting is deeper than one, and code spans may legitimately
/// carry unbalanced parentheses that must not abort the join-space heuristic in
/// [`suppresses_join_space_after_nested_open_paren`].
fn unclosed_parenthesis_depth(text: &str) -> usize {
text.chars().fold(0usize, |depth, ch| match ch {
'(' => depth.saturating_add(1),
Expand Down
Loading