Skip to content

feat(kernel): render markdown tables as aligned fenced ASCII#52

Merged
chr1syy merged 4 commits into
mainfrom
feat/render-markdown-tables
Jun 26, 2026
Merged

feat(kernel): render markdown tables as aligned fenced ASCII#52
chr1syy merged 4 commits into
mainfrom
feat/render-markdown-tables

Conversation

@chr1syy

@chr1syy chr1syy commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Problem

Agent responses frequently contain GitHub-flavored markdown tables, but no chat platform renders them — Discord has no table syntax and Slack mrkdwn has none. They arrive as raw | a | b | pipes, which is unreadable.

Approach

Normalize tables in one provider-agnostic place in the kernel, just before split/send, so Discord, Slack, and every future provider benefit without adapter changes. Every target platform renders triple-backtick code blocks in a fixed-width font, so a table re-emitted as aligned ASCII inside a fence lines up on every client.

Before (raw, as it renders today):

| Service | Status | Latency |
|---------|:------:|--------:|
| api | ok | 12ms |
| worker | degraded | 480ms |

After:

+---------+----------+---------+
| Service |  Status  | Latency |
+---------+----------+---------+
| api     |    ok    |    12ms |
| worker  | degraded |   480ms |
+---------+----------+---------+

Changes

  • src/core/renderTables.ts (new): detects GFM table blocks and renders aligned, fenced ASCII tables. Honors :-- / --: / :--: alignment, normalizes ragged rows to the header column count, unescapes \|, supports tables with/without outer pipes, truncates cells past MAX_TABLE_WIDTH with an ellipsis (), and leaves already-fenced tables untouched.
  • src/core/splitMessage.ts: now fence-aware — when a long response is split across the per-message length limit, an open fence is closed on one chunk and re-opened on the next, so a rendered table never loses its monospace block. Fence-free behavior is unchanged (existing tests still pass byte-for-byte).
  • src/core/queue.ts: wires renderTables into the outbound agent-response path (one line). Only this path is touched — inbound handling is untouched.
  • Docs: architecture.md gains an Output rendering section; discord.md / slack.md get one-line pointers.

Design notes

  • Kernel stays free of provider/chat-SDK imports (renderTables is pure) — honors the kernel/provider boundary in CLAUDE.md.
  • Width cap (default 56 mono chars) targets Discord mobile; over-wide tables truncate rather than overflow.
  • Tradeoff: inline markdown inside a cell (bold, links) renders literally, since the cell now lives in a code fence. Alignment is the deliberate win.

Testing

  • 10 new renderTables cases (basic, alignment, ragged rows, escapes, no outer pipes, prose preservation, already-fenced passthrough, multiple tables, wide-cell truncation).
  • New fence-aware splitMessage cases + regression guard for fence-free splits.
  • Full suite: 245 passing, npm run build clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Agent replies now render Markdown tables as aligned, fenced ASCII tables for improved display in chat apps (Discord/Slack).
  • Bug Fixes
    • Table rendering is more robust for uneven rows, alignment markers, escaped pipes, and multiple tables per message.
    • Long messages are split more safely, re-fencing when needed to keep Markdown code blocks intact across boundaries and respect max length.
  • Tests
    • Added extensive coverage for table conversion edge cases and fence-aware message splitting.

Agent responses frequently contain GitHub-flavored markdown tables, which
no chat platform renders — Discord has no table syntax and Slack mrkdwn has
none, so they arrive as raw `| a | b |` pipes.

Normalize them in one provider-agnostic place in the kernel:

- `src/core/renderTables.ts`: detect GFM table blocks and re-emit each as a
  width-aligned ASCII table wrapped in a code fence (every target platform
  renders fences in fixed-width font, so columns line up). Honors `:--`/`--:`/
  `:--:` alignment, normalizes ragged rows, unescapes `\|`, truncates cells
  past MAX_TABLE_WIDTH with an ellipsis, and leaves already-fenced tables
  untouched.
- `src/core/splitMessage.ts`: make splitting fence-aware so a code block split
  across the length limit is closed on one chunk and re-opened on the next,
  preserving the original fence-free behavior exactly.
- Wire `renderTables` into the queue just before split/send, so Discord, Slack,
  and every future provider benefit without touching adapter code.

Docs: document the behavior in architecture.md (Output rendering) with one-line
pointers from discord.md and slack.md. Tests: 10 new renderTables cases plus
fence-aware splitMessage cases; full suite 245 passing.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@chr1syy, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 41 minutes and 14 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 351274d9-cc8e-4f73-9867-4a006f5ee7ec

📥 Commits

Reviewing files that changed from the base of the PR and between 600415b and f829d59.

📒 Files selected for processing (5)
  • src/__tests__/renderTables.test.ts
  • src/__tests__/splitMessage.test.ts
  • src/core/fences.ts
  • src/core/renderTables.ts
  • src/core/splitMessage.ts
📝 Walkthrough

Walkthrough

Agent responses now render Markdown tables as fenced ASCII tables before splitting. splitMessage also preserves fenced code blocks across chunk boundaries, and the docs and tests were updated for the new flow.

Changes

Markdown table output pipeline

Layer / File(s) Summary
Table rendering
src/core/renderTables.ts, src/__tests__/renderTables.test.ts, docs/architecture.md, docs/discord.md, docs/slack.md
renderTables converts GFM tables into fenced ASCII tables with alignment, width caps, and code-fence passthrough; the renderer tests and platform docs cover that output.
Fence-aware splitting
src/core/fences.ts, src/core/splitMessage.ts, src/core/queue.ts, src/__tests__/splitMessage.test.ts, docs/architecture.md
Fence parsing and repair helpers support splitMessage, which now reopens fenced code blocks across chunk boundaries; the queue renders responses before splitting, and the tests and architecture notes cover that send path.

Sequence Diagram(s)

sequenceDiagram
  participant queue.ts as queue.ts
  participant renderTables as renderTables
  participant splitMessage as splitMessage
  participant provider.send as provider.send
  queue.ts->>renderTables: render result.response
  renderTables-->>queue.ts: fenced ASCII tables
  queue.ts->>splitMessage: split rendered text
  splitMessage-->>queue.ts: fence-balanced parts
  loop send each part
    queue.ts->>provider.send: send part
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nibbled the markdown, crisp and neat,
ASCII tables now hop in tidy feet.
Fences stay snug when the message splits,
Discord and Slack read every bit.
Thump-thump! The carrot code is sweet.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 77.27% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary change: rendering Markdown tables as aligned fenced ASCII in the kernel.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/render-markdown-tables

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with 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.

Inline comments:
In `@src/core/renderTables.ts`:
- Around line 23-25: The fence parsing in renderTables currently treats any
fence delimiter as a toggle, which incorrectly closes an active block when a
different marker appears inside it. Update the fence tracking logic around
isFenceDelimiter and the fence state used by renderTables so it records the
opener’s marker and length, then only exits the fenced state when a matching
closing delimiter with the same marker/length is encountered. Apply the same
matching logic anywhere the fence state is updated so table normalization stays
disabled throughout the entire fenced block.

In `@src/core/splitMessage.ts`:
- Around line 25-35: The dangling fence detection in danglingFence() is
normalizing every opener to a 3-character marker, which loses the original fence
length and can mis-detect valid 4+-backtick fences. Update danglingFence() to
preserve and compare the full opener token from the matched line, and use that
exact token when determining whether a later fence closes it so repairFences()
gets the correct dangling state.
- Around line 45-71: The `splitMessage` contract can be broken because
`repairFences` in `src/core/splitMessage.ts` appends fence markers after
`rawSplit` has already filled each chunk to `maxLength`. Update
`splitMessage`/`repairFences` so fenced code blocks are accounted for before
splitting, or re-split any repaired chunk that exceeds the limit. Use the
`rawSplit`, `repairFences`, and `splitMessage` functions as the main entry
points to adjust the budget handling.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7f5ee877-297c-44c1-b915-2a48cbdb5513

📥 Commits

Reviewing files that changed from the base of the PR and between 8424883 and 0cbfac0.

📒 Files selected for processing (8)
  • docs/architecture.md
  • docs/discord.md
  • docs/slack.md
  • src/__tests__/renderTables.test.ts
  • src/__tests__/splitMessage.test.ts
  • src/core/queue.ts
  • src/core/renderTables.ts
  • src/core/splitMessage.ts

Comment thread src/core/renderTables.ts Outdated
Comment thread src/core/splitMessage.ts Outdated
Comment thread src/core/splitMessage.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0cbfac0110

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/core/splitMessage.ts Outdated
export function splitMessage(text: string, maxLength: number = DEFAULT_MAX_LENGTH): string[] {
const parts = rawSplit(text, maxLength);
if (parts.length <= 1) return parts;
return repairFences(parts);

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 Re-split chunks after repairing fences

When a split lands inside a fenced block, repairFences appends/prepends fence lines after rawSplit, so the returned parts can exceed the requested maxLength. For example, splitMessage('```\n' + 'a'.repeat(1990) + '\n```') returns a middle chunk of length 1998 even though DEFAULT_MAX_LENGTH is 1990, and custom smaller limits overflow the same way. Any caller treating maxLength as the hard platform/API limit can still send an oversized message; reserve the fence overhead before splitting or re-split after repair.

Useful? React with 👍 / 👎.

Comment thread src/core/renderTables.ts Outdated
const line = lines[i];

if (isFenceDelimiter(line)) {
inFence = !inFence;

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 Track the active fence marker before converting tables

When a fenced Markdown example uses ~~~ so it can contain literal triple-backtick lines, this boolean toggle treats the inner backtick line as closing the outer tilde fence. That makes inFence false and allows the table detector to rewrite | ... | samples that are still inside the code block, despite the documented guarantee that fenced tables are left untouched. Track the opening fence marker and only close on a matching fence.

Useful? React with 👍 / 👎.

Comment thread src/core/renderTables.ts Outdated
!inFence &&
isTableRow(line) &&
i + 1 < lines.length &&
isSeparator(lines[i + 1])

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 Require delimiter rows to match header columns

This accepts any separator-like next line without checking that its cell count matches the header, but GFM does not recognize a table when the header and delimiter row have different numbers of cells. For inputs such as A | B\n--- |, the renderer now invents a two-column ASCII table and can consume following pipe lines as body even though GitHub would leave the text alone. Compare the parsed header and separator cell counts before rendering.

Useful? React with 👍 / 👎.

Comment thread src/core/renderTables.ts Outdated
const separator = lines[i + 1];
const body: string[] = [];
let j = i + 2;
while (j < lines.length && isTableRow(lines[j]) && !isSeparator(lines[j])) {

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 Keep delimiter-looking body rows in the table

After the header delimiter has already identified the table, a later row such as | --- | --- | is valid table data, but this guard terminates the body before it. Agent outputs that use dashes as cell values or placeholders will render as a header-only ASCII table and leave the remaining rows as raw text, corrupting the reply. Once the delimiter row is consumed, continue accepting pipe rows until a blank line or real block boundary.

Useful? React with 👍 / 👎.

Comment thread src/core/renderTables.ts Outdated
const separator = lines[i + 1];
const body: string[] = [];
let j = i + 2;
while (j < lines.length && isTableRow(lines[j]) && !isSeparator(lines[j])) {

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 Stop table bodies at block-level boundaries

This loop treats every subsequent pipe-containing line as a table row, so a block-level construct immediately after a table is swallowed when it happens to contain |. For example, > note | detail or - note | detail after a table should start a quote/list in GFM, but it will be rendered as an extra ASCII table row. Stop body collection when the next line begins another block-level structure instead of relying only on blank or pipe-free lines.

Useful? React with 👍 / 👎.

Comment thread src/core/splitMessage.ts Outdated
Comment on lines +31 to +33
const marker = m[1][0].repeat(3); // normalize to a 3-char marker
if (open === null) open = marker;
else if (open[0] === marker[0]) open = null; // same fence char closes the block

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 Preserve fence length while balancing chunks

For code blocks opened with four or more backticks so they can contain literal triple-backtick lines, this normalizes the marker to three and later closes on any same-character fence. If such a block contains a literal ``` line before a split, danglingFence thinks the block is already closed and `repairFences` stops re-fencing later chunks, so the split message loses monospace formatting. Track the opening fence length and only close with a fence of at least that length.

Useful? React with 👍 / 👎.

chr1syy added a commit that referenced this pull request Jun 26, 2026
Resolves the two blocking findings plus the non-blocking/nit items from the
Discord Maestro Codex review of PR #52:

- renderTables: a body row that looks like a separator (e.g. `| --- |`) no
  longer terminates the table — it is valid table data. The body scan now ends
  only at the first non-row line, fixing silent data loss where trailing rows
  leaked out as raw markdown.
- Fence handling is now CommonMark-correct via a new shared `src/core/fences.ts`
  (parseFenceLine/closesFence/danglingFence): fences longer than three chars are
  tracked by exact char+length, so a ``` line inside a ```` block is treated as
  content, not a close. Both renderTables and splitMessage use it.
- renderTables now requires the separator's column count to match the header's,
  avoiding false-positive conversion of table-ish prose.
- splitMessage reserves a small budget when fences are present so re-fencing
  never exceeds a caller-supplied maxLength; preserves the original fence marker
  length and info string (language) when re-opening a split block.

Tests: +5 cases (dash-row data-loss regression, inner-``` inside ```` block,
column-count mismatch, custom maxLength, long-fence split). Full suite 250
passing; build clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolves the two blocking findings plus the non-blocking/nit items from the
Discord Maestro Codex review of PR #52:

- renderTables: a body row that looks like a separator (e.g. `| --- |`) no
  longer terminates the table — it is valid table data. The body scan now ends
  only at the first non-row line, fixing silent data loss where trailing rows
  leaked out as raw markdown.
- Fence handling is now CommonMark-correct via a new shared `src/core/fences.ts`
  (parseFenceLine/closesFence/danglingFence): fences longer than three chars are
  tracked by exact char+length, so a ``` line inside a ```` block is treated as
  content, not a close. Both renderTables and splitMessage use it.
- renderTables now requires the separator's column count to match the header's,
  avoiding false-positive conversion of table-ish prose.
- splitMessage reserves a small budget when fences are present so re-fencing
  never exceeds a caller-supplied maxLength; preserves the original fence marker
  length and info string (language) when re-opening a split block.

Tests: +5 cases (dash-row data-loss regression, inner-``` inside ```` block,
column-count mismatch, custom maxLength, long-fence split). Full suite 250
passing; build clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@chr1syy chr1syy force-pushed the feat/render-markdown-tables branch from 65adf15 to 3a81e57 Compare June 26, 2026 14:24
@chr1syy

chr1syy commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

Codex review applied

Reviewed by the Discord Maestro Codex agent (predecessor repo). All findings addressed in 3a81e57.

🔴 Blocking — fixed

  1. Data loss in renderTables — a body row that looked like a separator (e.g. | --- |) terminated the table early, leaking trailing rows as raw markdown. The body scan now ends only at the first non-row line; dash rows are rendered as data.
  2. Long-fence mishandling — fences were collapsed to a 3-char token, so a ``` line inside a ```` block was mistaken for a close. Replaced with CommonMark-correct tracking in a new shared src/core/fences.ts (parseFenceLine / closesFence / danglingFence), used by both renderTables and splitMessage.

🟡 Non-blocking — fixed

  • splitMessage now reserves a small budget when fences are present, so re-fencing never exceeds a caller-supplied maxLength; the original fence marker length + info string (language) are preserved on re-open.
  • renderTables now requires the separator's column count to match the header's, avoiding false-positive conversion of table-ish prose.
  • Layering (transform in the queue) was confirmed correct — no change.

Tests: +5 cases (dash-row data-loss regression, inner-``` inside ```` block, column-count mismatch, custom maxLength, long-fence split). Full suite 250 passing, build clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3a81e57b3b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/core/splitMessage.ts Outdated
* past `maxLength`. Covers a long fence marker plus its newline and info string.
*/
export function splitMessage(text: string, maxLength: number = DEFAULT_MAX_LENGTH): string[] {
const FENCE_RESERVE = 16;

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 Compute fence headroom from the actual fence

Fresh evidence: the fixed 16-character reserve is still smaller than the repair text for common fenced blocks with language labels. A split inside a typescript block can prepend ` typescript\nand append\n````, adding 18 characters after the raw split budgeted only maxLength - 16, so `splitMessage('typescript\n' + 'x'.repeat(5000) + '\n')` produces 1992-character chunks. Because `DEFAULT_MAX_LENGTH` is used to stay under chat platform limits, these repaired chunks can still be rejected; size the reserve from the carried fence or re-split after repair.

Useful? React with 👍 / 👎.

Comment thread src/core/fences.ts Outdated
Comment on lines +21 to +22
export function parseFenceLine(line: string): Fence | null {
const m = line.match(/^\s*(`{3,}|~{3,})\s*(.*)$/);

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 Limit fence detection to valid indentation

When a response contains an indented literal fence line such as ````, CommonMark treats it as an indented-code line rather than a fenced block, but this parser opens a fence because\s*accepts any indentation. In that caserenderTables` will think all subsequent lines are inside a code fence and skip converting later GFM tables until it happens to see a matching close, so a normal table following an indented code example remains raw. Restrict fence recognition to at most three leading spaces.

Useful? React with 👍 / 👎.

Comment thread src/core/renderTables.ts Outdated
// Collect every subsequent table row. A row of dashes (e.g. `| --- |`)
// is valid table data, so the separator check is NOT a terminator here —
// GFM ends a table at the first non-row line (typically a blank line).
while (j < lines.length && isTableRow(lines[j])) {

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 Keep no-pipe continuation rows in tables

After a table has been recognized, GFM treats the following nonblank paragraph line as another body row even if that row has fewer cells and no pipe; for example bar after a two-column row is rendered as a row with an empty second cell. This loop stops as soon as the next line lacks |, so those supported ragged rows are emitted as raw prose outside the ASCII table and the normalized table no longer matches the agent's markdown. Continue collecting nonblank non-block-boundary lines and normalize them like other short rows.

Useful? React with 👍 / 👎.

Two valid P2 findings from the GitHub Codex review of 3a81e57:

- splitMessage: the fixed 16-char fence reserve was smaller than the repair
  text for fences with language labels (e.g. re-opening ```typescript is 14
  chars + a 4-char close = 18), so chunks could land at 1992 > maxLength. The
  reserve is now computed from the actual fences in the text (longest re-open +
  longest close), so it covers any fence length or info string; fence-free
  splitting stays byte-for-byte unchanged.
- fences: parseFenceLine accepted any indentation via \s*, so an over-indented
  ``` line (4+ spaces = indented code per CommonMark, not a fence) wrongly
  opened a fence and suppressed table conversion for everything after it.
  Restricted the indent to at most three spaces.

Tests: +2 cases (language-label reserve, table after an over-indented line).
Full suite 252 passing; build clean.

Declined (with reason): the no-pipe continuation-row finding. cmark-gfm does
continue a table across a pipe-less line, but in a chat bridge requiring a pipe
per row avoids the far more common failure of swallowing a trailing sentence
(written without a blank line) into the table. No-pipe ragged rows are
vanishingly rare in agent output; the trade-off favors not eating prose.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@chr1syy

chr1syy commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

Second review pass (GitHub Codex on 3a81e57) — addressed in 600415b

The earlier CodeRabbit + Codex comments on 0cbfac0 are all outdated/resolved by 3a81e57. Codex re-reviewed the fix and raised 3 new P2 items:

✅ Fixed

  1. Fence reserve too small for language labels (splitMessage.ts) — confirmed: '```typescript\n' + 'x'.repeat(5000) + '\n```' produced 1992-char chunks. A fixed 16-char reserve can't cover unbounded info strings. The reserve is now computed from the actual fences in the text (longest re-open + longest close), so re-fencing never exceeds maxLength for any fence length or label.
  2. Over-indented fence detection (fences.ts) — \s* accepted any indentation, so a 4-space-indented ``` (indented code per CommonMark, not a fence) suppressed table conversion afterwards. Restricted to ≤3 spaces.

🚫 Declined (with reason)
3. No-pipe continuation rows — cmark-gfm does continue a table across a pipe-less line, but in a chat bridge requiring a pipe per row avoids the far more common failure: swallowing a trailing sentence (written without a blank line) into the table as a | sentence | | row. No-pipe ragged rows are vanishingly rare in agent output; the trade-off deliberately favors not eating prose. Happy to revisit if this shows up in practice.

Tests: +2 (language-label reserve, table after an over-indented line). Full suite 252 passing, build clean.

@chr1syy

chr1syy commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with 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.

Inline comments:
In `@src/__tests__/splitMessage.test.ts`:
- Around line 78-83: The splitMessage test is not covering the reopened language
label for two-part splits because parts.slice(1, -1) excludes the last chunk
when there are only two parts. Update the assertion in splitMessage.test.ts to
check every chunk after the first using the splitMessage output, so the language
label is verified consistently for both two-part and multi-part results.

In `@src/core/fences.ts`:
- Around line 25-28: parseFenceLine currently treats any backtick fence opener
as valid even when the info string contains backticks. Update parseFenceLine in
fences.ts to reject lines where the opener char is backtick and the trimmed info
string includes a backtick, while still allowing tildes. Keep the existing Fence
shape and use the parseFenceLine symbol as the gate so renderTables and
splitMessage no longer misclassify invalid fence openers.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 52d1a0db-0fa3-4d97-9091-7b2c844c18dc

📥 Commits

Reviewing files that changed from the base of the PR and between 0cbfac0 and 600415b.

📒 Files selected for processing (6)
  • .gitignore
  • src/__tests__/renderTables.test.ts
  • src/__tests__/splitMessage.test.ts
  • src/core/fences.ts
  • src/core/renderTables.ts
  • src/core/splitMessage.ts
✅ Files skipped from review due to trivial changes (1)
  • .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/core/renderTables.ts
  • src/core/splitMessage.ts

Comment thread src/__tests__/splitMessage.test.ts Outdated
Comment thread src/core/fences.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 600415bc74

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/core/renderTables.ts Outdated
// Collect every subsequent table row. A row of dashes (e.g. `| --- |`)
// is valid table data, so the separator check is NOT a terminator here —
// GFM ends a table at the first non-row line (typically a blank line).
while (j < lines.length && isTableRow(lines[j])) {

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 Stop swallowing following pipe-containing blocks

When a table is immediately followed without a blank by another Markdown block that contains a pipe, such as - next | note or > quote | note, GFM ends the table at that block start, but this loop still consumes it as a body row because isTableRow only checks for |. That folds the following list/quote into the fenced ASCII table and strips its intended semantics; stop collection on block starts, not just non-pipe lines.

Useful? React with 👍 / 👎.

Comment thread src/core/renderTables.ts

if (
open === null &&
isTableRow(line) &&

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 Skip indented code before table detection

If an agent includes an indented code sample that contains markdown-table text, for example four-space-prefixed | A | B | / | - | - | lines, CommonMark treats those lines as an indented code block, but this detector rewrites them because it only tracks fenced code. That changes literal code examples into rendered tables; exclude 4+ space indented code lines/blocks before accepting a table candidate.

Useful? React with 👍 / 👎.

Comment thread src/core/splitMessage.ts
export function splitMessage(text: string, maxLength: number = DEFAULT_MAX_LENGTH): string[] {
const reserve = fenceReserve(text);
const budget = reserve > 0 ? Math.max(1, maxLength - reserve) : maxLength;
const parts = rawSplit(text, budget);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 Badge Keep fitting fenced messages in one chunk

When a fenced message is already within the provider limit but longer than maxLength - reserve (for a normal ``` block, 1983–1990 chars with the default limit), this still calls rawSplit with the reduced budget, so a message that could be sent as one post is unnecessarily split and re-fenced into multiple posts. Preserve the original `text.length <= maxLength` fast path before applying the reserve; the reserve is only needed once a split is actually required.

Useful? React with 👍 / 👎.

Comment thread src/core/renderTables.ts
Comment on lines +84 to +88
while (sum() > cap) {
let idx = 0;
for (let i = 1; i < w.length; i++) if (w[i] > w[idx]) idx = i;
if (w[idx] <= MIN_COL_WIDTH) break; // can't shrink further without losing all signal
w[idx]--;

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 Cap very wide columns without a per-character loop

When an agent response contains a table cell with a very long value, such as a pasted log line or base64 blob, this loop shrinks the widest column one character at a time and recomputes the total width each iteration. A single 100k+ character cell can keep the queue's Node event loop busy before any split/send happens; compute the capped widths directly or reduce by larger deltas.

Useful? React with 👍 / 👎.

Comment thread src/core/renderTables.ts Outdated
});

let widths = Array.from({ length: columns }, (_, i) =>
Math.max(header[i].length, ...body.map((r) => r[i].length), 1),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 Badge Measure table cells by display width

For tables containing CJK characters, emoji, or combining marks, using JavaScript .length does not match monospace display width, so the rendered ASCII columns are still visibly misaligned in Discord/Slack despite being placed in a code fence. This affects common status/result tables with non-ASCII names or emoji indicators; use a display-width calculation consistently for width, truncation, and padding.

Useful? React with 👍 / 👎.

CodeRabbit (on 600415b):
- fences: reject backtick fence openers whose info string contains a backtick
  (invalid per CommonMark), so such lines aren't misread as fences.
- test: assert the language label on every reopened chunk via slice(1) (the
  prior slice(1,-1) was vacuous for two-part splits).

Codex (on 600415b):
- renderTables: end the table body at the start of another block (list,
  blockquote, heading) even when that line contains a pipe, instead of folding
  it into the ASCII table.
- renderTables: skip table detection for 4+ space indented lines — that is an
  indented code block, not a table.
- renderTables: clamp each column width to the cap up front so capWidths' loop
  is bounded by the cap, not by cell length (a pasted 100k-char cell no longer
  stalls the event loop: ~22ms for a 200k cell).
- splitMessage: restore the `text.length <= maxLength` fast path so a fenced
  message that already fits isn't needlessly split by the fence reserve.

Deferred (P3, with reason): measuring cell display width (CJK/emoji). Proper
wcwidth is non-trivial without a dep, and Discord/Slack don't render emoji at a
stable cell width inside code fences anyway, so the math wouldn't reliably fix
alignment. Can add a CJK-focused width helper if it proves needed.

Tests: +3 (block-start termination, indented-code table, fits-in-one-chunk).
Full suite 256 passing; build clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@chr1syy

chr1syy commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

Third review pass (Codex + CodeRabbit on 600415b) — addressed in f829d59

✅ Fixed — CodeRabbit

  • Backtick info strings (fences.ts): a backtick fence opener whose info string contains a backtick is invalid per CommonMark; now rejected so it isn't misread as a fence.
  • Test gap (splitMessage.test.ts): slice(1, -1) was vacuous for two-part splits — now slice(1) so the language-label assertion actually fires.

✅ Fixed — Codex

  • Swallowing following blocks (renderTables.ts): the body scan now ends at the start of another block (list / blockquote / heading) even when that line contains a pipe, instead of folding it into the ASCII table.
  • Indented-code tables (renderTables.ts): 4+ space-indented | … | lines are an indented code block, not a table — detection now requires ≤3-space indent.
  • Wide-cell perf (renderTables.ts): column widths are clamped to the cap up front, so capWidths' loop is bounded by the cap rather than cell length. A 200k-char cell now renders in ~22 ms instead of stalling the event loop.
  • Needless split (splitMessage.ts): restored the text.length <= maxLength fast path so a fenced message that already fits isn't split by the fence reserve.

🚫 Deferred (P3, with reason)

  • Display-width measurement (CJK/emoji): proper wcwidth is non-trivial without a dependency, and Discord/Slack don't render emoji at a stable cell width inside code fences anyway, so the math wouldn't reliably fix alignment. Happy to add a CJK-focused width helper if it proves needed in practice.

Tests: +3 (block-start termination, indented-code table, fits-in-one-chunk). Full suite 256 passing, build clean.

@chr1syy chr1syy merged commit 14c95fc into main Jun 26, 2026
3 checks passed
@chr1syy chr1syy deleted the feat/render-markdown-tables branch June 26, 2026 15:40

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f829d593bb

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/core/renderTables.ts
Comment on lines +154 to +155
let widths = Array.from({ length: columns }, (_, i) =>
Math.min(Math.max(header[i].length, ...body.map((r) => r[i].length), 1), MAX_TABLE_WIDTH),

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 Avoid spreading all table rows to size columns

When an agent returns a very tall table (about 125k+ body rows on current V8, which is under a megabyte for tiny rows), the ...body.map(...) call exceeds the engine's argument limit and throws RangeError: Maximum call stack size exceeded. Because this runs inside the queue response path, the successful agent response is replaced by the generic failure message instead of being split/sent; compute the maximum with an iterative loop.

Useful? React with 👍 / 👎.

Comment thread src/core/renderTables.ts
// table data, so the separator check is NOT a terminator here. GFM ends a
// table at the first non-row line or the start of another block (list,
// blockquote, heading) — even one that happens to contain a pipe.
while (j < lines.length && isTableRow(lines[j]) && !isBlockStart(lines[j])) {

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 Stop before indented code in table bodies

Fresh evidence after the block-start fix: 4-space-indented code lines in the body are still accepted because this loop only checks isTableRow and isBlockStart. If a table is followed immediately by an indented code/log line containing |, GFM ends the table there, but this code folds that literal code into the ASCII table and strips its indentation; also stop when leadingSpaces(lines[j]) > 3.

Useful? React with 👍 / 👎.

Comment thread src/core/renderTables.ts

if (
open === null &&
isTableRow(line) &&

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 Reject block starts as table headers

When a block-start line itself contains a pipe and is followed by a separator-looking line, this header check still treats it as a table header; for example - path | meaning followed by --- | --- is a list item in GFM, but it gets rewritten as a fenced ASCII table. Apply the same block-boundary guard to candidate header lines so lists, blockquotes, and headings are not consumed as tables.

Useful? React with 👍 / 👎.

Comment thread src/core/renderTables.ts
Comment on lines +201 to +202
i + 1 < lines.length &&
isSeparator(lines[i + 1]) &&

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 Reject indented-code delimiter rows

This table candidate check validates indentation only on the header line, so a normal pipe-containing line followed by a 4-space-indented --- | --- code line is rewritten as an empty ASCII table even though GFM treats the second line as indented code, not a table delimiter. Check leadingSpaces(lines[i + 1]) <= 3 before accepting the separator to avoid converting prose plus an indented code sample.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant