From 482fa2d0b5e9ae93ae6cd7f6e52e9ad6bd7d1fe1 Mon Sep 17 00:00:00 2001 From: Sav Tripodi <42227195+savtrip@users.noreply.github.com> Date: Thu, 11 Jun 2026 13:08:47 +0200 Subject: [PATCH 1/3] Add table extension to mdConverter goldermark renderer --- internal/richtext/richtext.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index 7361534c1..a1b4309f8 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -128,7 +128,7 @@ var reMarkdownPatterns = []*regexp.Regexp{ // mdConverter is the goldmark Markdown-to-HTML converter configured for Trix compatibility. var mdConverter = goldmark.New( - goldmark.WithExtensions(extension.Strikethrough), + goldmark.WithExtensions(extension.Strikethrough, extension.Table), goldmark.WithRendererOptions(gmhtml.WithUnsafe()), goldmark.WithParserOptions( parser.WithInlineParsers( From 3452fb8558bec3339e99ce36cabaf755b138a6fa Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 22 Jul 2026 14:37:54 -0700 Subject: [PATCH 2/3] Render GFM tables in message and comment bodies (#405) Register goldmark's table extension in mdConverter so Markdown tables convert to HTML instead of leaking as raw pipe rows. Use the align-attribute cell method rather than the default inline style, since BC3's sanitizer whitelists the align attribute but strips style, so column alignment survives sanitization. BC3's WrapTablesFilter supplies the figure wrapper, so a bare
is correct. Because the CLI can now author tables, guard against destroying them: - IsMarkdown gains AST-based table detection so a table-only chat message is sent as HTML rather than verbatim pipes. - IsHTML recognizes
so a plain table response is routed through the display converter instead of leaking raw markup to the reader. - The TUI in-place editors (message body, comment, to-do description) fail closed on table-bearing content: HTMLToMarkdown has no table handling, so they refuse to open rather than flatten the table on resubmit. Round-trip editing waits on server-side Markdown (BC3 #11986). Document the table boundary in the Basecamp skill. --- internal/richtext/richtext.go | 48 ++++++++++- internal/richtext/richtext_test.go | 79 +++++++++++++++++++ internal/tui/workspace/views/detail.go | 9 +++ internal/tui/workspace/views/detail_test.go | 44 +++++++++++ internal/tui/workspace/views/todos.go | 6 ++ internal/tui/workspace/views/todos_test.go | 35 ++++++++ .../tui/workspace/widget/composer_test.go | 13 +++ skills/basecamp/SKILL.md | 9 ++- 8 files changed, 239 insertions(+), 4 deletions(-) diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index a1b4309f8..b6d8ece1c 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -20,6 +20,7 @@ import ( "github.com/yuin/goldmark" "github.com/yuin/goldmark/ast" "github.com/yuin/goldmark/extension" + east "github.com/yuin/goldmark/extension/ast" "github.com/yuin/goldmark/parser" "github.com/yuin/goldmark/renderer" gmhtml "github.com/yuin/goldmark/renderer/html" @@ -110,7 +111,7 @@ var reSGIDMention = regexp.MustCompile(`(^|[\s>(\["'])(@sgid:([\w+=/-]+))`) // Pre-compiled regexes for IsHTML detection var ( - reSafeTag = regexp.MustCompile(`<(p|div|span|a|strong|b|em|i|code|pre|ul|ol|li|h[1-6]|blockquote|br|hr|img|bc-attachment)\b[^>]*>`) + reSafeTag = regexp.MustCompile(`<(p|div|span|a|strong|b|em|i|code|pre|ul|ol|li|h[1-6]|blockquote|br|hr|img|table|bc-attachment)\b[^>]*>`) reFencedBlock = regexp.MustCompile("(?m)^```[^\n]*\n[\\s\\S]*?^```") ) @@ -128,7 +129,16 @@ var reMarkdownPatterns = []*regexp.Regexp{ // mdConverter is the goldmark Markdown-to-HTML converter configured for Trix compatibility. var mdConverter = goldmark.New( - goldmark.WithExtensions(extension.Strikethrough, extension.Table), + goldmark.WithExtensions( + extension.Strikethrough, + // Emit the whitelisted `align` attribute rather than a `style` attribute: + // BC3's sanitizer keeps `align` but strips inline `style` (only color and + // background-color survive), so GFM column alignment is preserved through + // sanitization. Bare
is correct — BC3's WrapTablesFilter wraps it. + extension.NewTable( + extension.WithTableCellAlignMethod(extension.TableCellAlignAttribute), + ), + ), goldmark.WithRendererOptions(gmhtml.WithUnsafe()), goldmark.WithParserOptions( parser.WithInlineParsers( @@ -923,6 +933,26 @@ func unescapeHTML(s string) string { return s } +// tableDetectParser parses input with the GFM table extension so table +// detection matches what MarkdownToHTML actually renders. Using the parser +// (rather than a regex) authoritatively handles single-column tables, CRLF +// line endings, and other pipe-row edge cases without pattern drift. +var tableDetectParser = goldmark.New(goldmark.WithExtensions(extension.Table)).Parser() + +// hasMarkdownTable parses s and reports whether it contains a GFM table node. +func hasMarkdownTable(s string) bool { + doc := tableDetectParser.Parse(text.NewReader([]byte(s))) + found := false + _ = ast.Walk(doc, func(n ast.Node, entering bool) (ast.WalkStatus, error) { + if entering && n.Kind() == east.KindTable { + found = true + return ast.WalkStop, nil + } + return ast.WalkContinue, nil + }) + return found +} + // IsMarkdown attempts to detect if the input string is Markdown rather than plain text or HTML. // This is a heuristic and may not be 100% accurate. func IsMarkdown(s string) bool { @@ -936,7 +966,7 @@ func IsMarkdown(s string) bool { } } - return false + return hasMarkdownTable(s) } // AttachmentRef holds the metadata needed to embed a in HTML. @@ -1362,6 +1392,18 @@ func IsHTML(s string) bool { return false } +// reTableHTML matches a real
tag (open tag or self-closing), distinct +// from the Markdown table detector. Used to gate the fail-closed TUI edit paths. +var reTableHTML = regexp.MustCompile(`(?i)]`) + +// HasTableHTML reports whether s contains an HTML table element. The TUI in-place +// editors use this to refuse table-bearing content: HTMLToMarkdown has no table +// handling and would strip the structure, so those edits fail closed rather than +// silently flatten the table on resubmit. +func HasTableHTML(s string) bool { + return reTableHTML.MatchString(s) +} + func isEscapedAt(s string, pos int) bool { backslashes := 0 for i := pos - 1; i >= 0 && s[i] == '\\'; i-- { diff --git a/internal/richtext/richtext_test.go b/internal/richtext/richtext_test.go index cd7fb1937..2de6822b5 100644 --- a/internal/richtext/richtext_test.go +++ b/internal/richtext/richtext_test.go @@ -216,6 +216,14 @@ func TestMarkdownToHTML(t *testing.T) { input: "intro\n\n```\n
hello
\n```", expected: "

intro

\n
\n
<div>hello</div>\n
", }, + { + // Issue #405: a GFM table renders as a bare
(BC3's + // WrapTablesFilter supplies the figure wrapper). Doubles as the + // heading+table regression. + name: "gfm table with heading (issue #405)", + input: "# Report\n\n| Foo | Bar |\n| --- | --- |\n| Baz | Qux |", + expected: "

Report

\n
\n\n\n\n\n\n\n\n\n\n\n\n\n
FooBar
BazQux
", + }, } for _, tt := range tests { @@ -228,6 +236,21 @@ func TestMarkdownToHTML(t *testing.T) { } } +func TestMarkdownToHTMLTableAlignment(t *testing.T) { + // Column alignment must survive BC3's sanitizer, which whitelists the `align` + // attribute but strips inline `style` (only color/background-color survive). + got := MarkdownToHTML("| a | b |\n|:--|--:|\n| 1 | 2 |") + if !strings.Contains(got, `align="left"`) { + t.Errorf("expected align=\"left\" in %q", got) + } + if !strings.Contains(got, `align="right"`) { + t.Errorf("expected align=\"right\" in %q", got) + } + if strings.Contains(got, "style=") { + t.Errorf("expected no style= (stripped by BC3 sanitizer) in %q", got) + } +} + func TestMarkdownToHTMLBackslashEscapes(t *testing.T) { tests := []struct { name string @@ -1159,6 +1182,31 @@ func TestIsMarkdown(t *testing.T) { input: "> Quote", expected: true, }, + { + name: "multi-column table", + input: "| Foo | Bar |\n| --- | --- |\n| Baz | Qux |", + expected: true, + }, + { + name: "single-column table", + input: "| value |\n| --- |\n| item |", + expected: true, + }, + { + name: "table with CRLF line endings", + input: "| Foo | Bar |\r\n| --- | --- |\r\n| Baz | Qux |", + expected: true, + }, + { + name: "lone delimiter line is not a table", + input: "---|---", + expected: false, + }, + { + name: "prose containing pipes is not a table", + input: "run foo | grep bar to filter the output", + expected: false, + }, } for _, tt := range tests { @@ -1262,6 +1310,14 @@ func TestIsHTML(t *testing.T) { input: `\\`, expected: false, }, + { + // A plain BC3 table response carries no other formatting tags, so + // IsHTML must recognize itself, or the raw markup would leak + // to the reader instead of being converted for display. + name: "bc3-wrapped table", + input: "
Foo
Baz
", + expected: true, + }, } for _, tt := range tests { @@ -1274,6 +1330,29 @@ func TestIsHTML(t *testing.T) { } } +func TestHasTableHTML(t *testing.T) { + tests := []struct { + name string + input string + expected bool + }{ + {name: "lowercase table tag", input: "
x
", expected: true}, + {name: "uppercase table tag with attrs", input: `
`, expected: true}, + {name: "wrapped table", input: "
", expected: true}, + {name: "plain text", input: "just some text", expected: false}, + {name: "pipe markdown", input: "| a | b |\n| --- | --- |\n| 1 | 2 |", expected: false}, + {name: "word starting with table", input: "the tablet is here", expected: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := HasTableHTML(tt.input); got != tt.expected { + t.Errorf("HasTableHTML(%q) = %v, want %v", tt.input, got, tt.expected) + } + }) + } +} + func TestRoundTrip(t *testing.T) { // Test that converting Markdown -> HTML -> Markdown preserves meaning tests := []struct { diff --git a/internal/tui/workspace/views/detail.go b/internal/tui/workspace/views/detail.go index 7c6206883..aa2961ddb 100644 --- a/internal/tui/workspace/views/detail.go +++ b/internal/tui/workspace/views/detail.go @@ -757,6 +757,10 @@ func (v *Detail) startCommentEdit() tea.Cmd { return nil } c := v.data.comments[v.focusedComment] + // Fail closed on table-bearing content (see startEditBody). + if richtext.HasTableHTML(c.content) { + return workspace.SetStatus("This comment contains a table — edit it on Basecamp web", true) + } v.editingComment = true v.commentEditComposer = widget.NewComposer(v.styles, widget.WithMode(widget.ComposerRich), @@ -1074,6 +1078,11 @@ func (v *Detail) startEditBody() tea.Cmd { if v.data == nil { return nil } + // Fail closed on table-bearing content: HTMLToMarkdown has no table handling, + // so entering edit mode and resubmitting would strip the table. Block the edit. + if richtext.HasTableHTML(v.data.content) { + return workspace.SetStatus("This message contains a table — edit it on Basecamp web", true) + } v.editingBody = true v.bodyEditComposer = widget.NewComposer(v.styles, widget.WithMode(widget.ComposerRich), diff --git a/internal/tui/workspace/views/detail_test.go b/internal/tui/workspace/views/detail_test.go index 4697f4ade..1f743d019 100644 --- a/internal/tui/workspace/views/detail_test.go +++ b/internal/tui/workspace/views/detail_test.go @@ -573,6 +573,50 @@ func TestDetail_CommentEdit_Ignored_WhenNoFocus(t *testing.T) { assert.False(t, v.editingComment) } +const tableHTML = "
" + + "
Foo
Baz
" + +func TestDetail_EditBody_BlockedForTable(t *testing.T) { + v := testDetailWithSession("Message", false) + v.data.content = tableHTML + + cmd := v.startEditBody() + assert.False(t, v.editingBody, "must not enter edit mode on table content") + assert.Nil(t, v.bodyEditComposer, "composer must not be built") + + require.NotNil(t, cmd, "should return a status command") + status, ok := cmd().(workspace.StatusMsg) + require.True(t, ok, "cmd should produce a StatusMsg") + assert.Contains(t, status.Text, "table") + assert.True(t, status.IsError) +} + +func TestDetail_EditBody_EntersForNonTable(t *testing.T) { + v := testDetailWithSession("Message", false) + v.data.content = "

plain body

" + + cmd := v.startEditBody() + assert.True(t, v.editingBody, "should enter edit mode on table-free content") + require.NotNil(t, v.bodyEditComposer, "composer should be built") + assert.NotNil(t, cmd) +} + +func TestDetail_CommentEdit_BlockedForTable(t *testing.T) { + v := detailWithComments() + v.focusedComment = 0 + v.data.comments[0].content = tableHTML + + cmd := v.startCommentEdit() + assert.False(t, v.editingComment, "must not enter edit mode on table content") + assert.Nil(t, v.commentEditComposer, "composer must not be built") + + require.NotNil(t, cmd, "should return a status command") + status, ok := cmd().(workspace.StatusMsg) + require.True(t, ok, "cmd should produce a StatusMsg") + assert.Contains(t, status.Text, "table") + assert.True(t, status.IsError) +} + func TestDetail_CommentTrash_DoublePress(t *testing.T) { v := detailWithComments() v.focusedComment = 1 diff --git a/internal/tui/workspace/views/todos.go b/internal/tui/workspace/views/todos.go index f9388e667..069359965 100644 --- a/internal/tui/workspace/views/todos.go +++ b/internal/tui/workspace/views/todos.go @@ -1042,6 +1042,12 @@ func (v *Todos) startEditDescription() tea.Cmd { } } + // Fail closed on table-bearing content: HTMLToMarkdown has no table handling, + // so entering edit mode and resubmitting would strip the table. Block the edit. + if richtext.HasTableHTML(description) { + return workspace.SetStatus("This to-do description contains a table — edit it on Basecamp web", true) + } + v.editingDesc = true v.descTodoID = todoID v.descComposer.Reset() diff --git a/internal/tui/workspace/views/todos_test.go b/internal/tui/workspace/views/todos_test.go index 0da75b720..2c7070506 100644 --- a/internal/tui/workspace/views/todos_test.go +++ b/internal/tui/workspace/views/todos_test.go @@ -942,6 +942,41 @@ func TestTodos_BoostTarget_IncludesAccountID(t *testing.T) { assert.Equal(t, int64(42), picker.Target.ProjectID) } +// --- Edit description: table fail-closed guard --- + +const todoTableHTML = "
" + + "
Foo
Baz
" + +func TestTodos_EditDescription_BlockedForTable(t *testing.T) { + v := testTodosViewWithTodos() + + todos := sampleTodos() + todos[0].Description = todoTableHTML + v.session.Hub().Todos(42, 10).Set(todos) + + cmd := v.startEditDescription() + assert.False(t, v.editingDesc, "must not enter edit mode on table content") + + require.NotNil(t, cmd, "should return a status command") + status, ok := cmd().(workspace.StatusMsg) + require.True(t, ok, "cmd should produce a StatusMsg") + assert.Contains(t, status.Text, "table") + assert.True(t, status.IsError) +} + +func TestTodos_EditDescription_EntersForNonTable(t *testing.T) { + v := testTodosViewWithTodos() + v.descComposer = widget.NewComposer(v.styles, widget.WithMode(widget.ComposerRich)) + + todos := sampleTodos() + todos[0].Description = "

plain description

" + v.session.Hub().Todos(42, 10).Set(todos) + + cmd := v.startEditDescription() + assert.True(t, v.editingDesc, "should enter edit mode on table-free content") + assert.NotNil(t, cmd) +} + // newTextInputWithValue creates a textinput with a preset value for testing. func newTextInputWithValue(val string) textinput.Model { ti := textinput.New() diff --git a/internal/tui/workspace/widget/composer_test.go b/internal/tui/workspace/widget/composer_test.go index 66a939691..d97400ffd 100644 --- a/internal/tui/workspace/widget/composer_test.go +++ b/internal/tui/workspace/widget/composer_test.go @@ -212,6 +212,19 @@ func TestComposerSubmitRich(t *testing.T) { } } +func TestComposerSubmitTableIsRich(t *testing.T) { + // A table-only submission has no other Markdown formatting; without AST-based + // table detection it would be misclassified as plain and sent as raw pipes. + c := NewComposer(testStyles(), WithMode(ComposerRich)) + c.SetValue("| Foo | Bar |\n| --- | --- |\n| Baz | Qux |") + cmd := c.Submit() + msg := cmd() + submitMsg := msg.(ComposerSubmitMsg) + if submitMsg.Content.IsPlain { + t.Error("table-only content should not be plain text") + } +} + func TestComposerSubmitEmpty(t *testing.T) { c := NewComposer(testStyles()) cmd := c.Submit() diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 36f3e696d..814609f6c 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -85,13 +85,20 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, 2. **Parse URLs first** with `basecamp url parse ""` to extract IDs 3. **Comments are flat** - reply to parent recording, not to comments 4. **Check context** via `.basecamp/config.json` before assuming project -5. **Content fields accept Markdown and @mentions** — message body and comment content accept Markdown syntax; the CLI converts to HTML automatically. Use Markdown formatting (lists, bold, links, code blocks) for rich content. Four mention syntaxes are available (prefer deterministic for agents): +5. **Content fields accept Markdown and @mentions** — message body and comment content accept Markdown syntax; the CLI converts to HTML automatically. Use Markdown formatting (lists, bold, links, code blocks, tables) for rich content. Four mention syntaxes are available (prefer deterministic for agents): - **`[@Name](mention:SGID)`** — zero API calls, embeds SGID directly (preferred for agents) - **`[@Name](person:ID)`** — one API call, resolves person ID to SGID via pingable set - **`@sgid:VALUE`** — inline SGID embed for pipeline composability - **`@Name` / `@First.Last`** — fuzzy name resolution (may be ambiguous) For todos, documents, and cards, content is sent as-is — use plain text or HTML directly. + **Table boundary:** GFM tables render in message/comment bodies, but the TUI + in-place editors **refuse to open** table-bearing content (edit it on Basecamp + web, or replace the whole field via `messages update` / `comments update` / + `todos update --description`, which take fresh content and are unaffected), and + human-readable CLI/TUI **display** of such content may lose table structure — + both pending server-side Markdown support (BC3 #11986). + **Multiline / non-ASCII content:** do not rely on bash ANSI-C quoting (`$'...\n...'`) — it is a bash/zsh extension. Under a POSIX `/bin/sh` (dash, busybox-ash, common in sandboxes) the `$` is passed through literally and posts a stray leading `$`, and `\n` stays a literal backslash-n. Pipe the content via stdin instead, using `-` as the content argument: ```bash printf '%s\n' '海报 mockup 方向稿：' '' '' | basecamp comments create - --in --json From cf303692c9308bfa7a18e8b0fd125d8805d8ddae Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 22 Jul 2026 17:00:32 -0700 Subject: [PATCH 3/3] richtext: match self-closing in HasTableHTML The docstring claimed the detector matched self-closing table tags, but the character class [\s>] excluded '/'. Add '/' so
is caught, keeping the boundary that prevents from matching. --- internal/richtext/richtext.go | 8 +++++--- internal/richtext/richtext_test.go | 1 + 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index b6d8ece1c..d1084db99 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -1392,9 +1392,11 @@ func IsHTML(s string) bool { return false } -// reTableHTML matches a real
tag (open tag or self-closing), distinct -// from the Markdown table detector. Used to gate the fail-closed TUI edit paths. -var reTableHTML = regexp.MustCompile(`(?i)]`) +// reTableHTML matches a real
tag — with attributes (`
`), bare +// (`
`), or self-closing (`
`) — distinct from the Markdown table +// detector. The trailing class requires a boundary after the name so longer +// tags like don't match. Used to gate the fail-closed TUI edit paths. +var reTableHTML = regexp.MustCompile(`(?i)]`) // HasTableHTML reports whether s contains an HTML table element. The TUI in-place // editors use this to refuse table-bearing content: HTMLToMarkdown has no table diff --git a/internal/richtext/richtext_test.go b/internal/richtext/richtext_test.go index 2de6822b5..ac03d2894 100644 --- a/internal/richtext/richtext_test.go +++ b/internal/richtext/richtext_test.go @@ -1338,6 +1338,7 @@ func TestHasTableHTML(t *testing.T) { }{ {name: "lowercase table tag", input: "
x
", expected: true}, {name: "uppercase table tag with attrs", input: `
`, expected: true}, + {name: "self-closing table tag", input: "", expected: true}, {name: "wrapped table", input: "
", expected: true}, {name: "plain text", input: "just some text", expected: false}, {name: "pipe markdown", input: "| a | b |\n| --- | --- |\n| 1 | 2 |", expected: false},