diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index 7361534c1..33359610e 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), + 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,36 @@ 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 { + // Cheap-out before the full goldmark parse: a GFM table needs cell-delimiting + // pipes and a delimiter row on its own line, so it can't exist without both a + // '|' and a '\n'. IsMarkdown calls this on every input that matches none of the + // regex patterns — typically plain text on TUI submit/editor return — so + // skipping the parse in that common case avoids the cost. This matches + // goldmark's own behavior exactly (it likewise treats a CR-only "table" with + // no '\n' as not a table), so the guard introduces no false negatives. + if !strings.Contains(s, "|") || !strings.Contains(s, "\n") { + return false + } + 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 +976,7 @@ func IsMarkdown(s string) bool { } } - return false + return hasMarkdownTable(s) } // AttachmentRef holds the metadata needed to embed a in HTML. @@ -1362,6 +1402,20 @@ func IsHTML(s string) bool { return false } +// 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 +// 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..ac03d2894 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,30 @@ 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: "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}, + {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