From 129aef2f31034b8bc3a5c4cbea31fb5dc690298e Mon Sep 17 00:00:00 2001 From: Chris Hall Date: Tue, 19 May 2026 20:08:01 -0400 Subject: [PATCH 1/3] fix(docs): markdown 3+ tables drift placeholder offset by one per table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #607. The per-table offset accumulator in replaceDocsMarkdownRange and insertDocsMarkdownAt added `(tableEnd - tableIndex) - 1` after each table, on the theory that the placeholder "\n" we wrote into plainText was consumed by the InsertTable call. Empirically the placeholder remains in the doc, so each subsequent table inserted one character too early — invisible for table 2 (the placeholder it shifted onto became its own leading newline), but for table 3 the off-by-2 drift landed mid-paragraph, splitting the preceding paragraph's trailing punctuation into a standalone paragraph after the table. Extracted the formula into `nextTableInsertOffset(currentOffset, tableIndex, tableEnd)` which accumulates the full (tableEnd - tableIndex) and updated both call sites. The helper is directly unit- tested against the issue repro shape. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/cmd/docs_multi_table_offset_test.go | 63 ++++++++++++++++++++ internal/cmd/docs_mutation.go | 8 +-- internal/cmd/docs_table_inserter.go | 15 +++++ 3 files changed, 80 insertions(+), 6 deletions(-) create mode 100644 internal/cmd/docs_multi_table_offset_test.go diff --git a/internal/cmd/docs_multi_table_offset_test.go b/internal/cmd/docs_multi_table_offset_test.go new file mode 100644 index 000000000..31a7a5b8d --- /dev/null +++ b/internal/cmd/docs_multi_table_offset_test.go @@ -0,0 +1,63 @@ +package cmd + +import "testing" + +// Regression for #607: with three or more tables in a single markdown render +// pass, the running placeholder offset under-counted by one character per +// table, causing the trailing punctuation of the paragraph immediately before +// the third (and any subsequent) table to be re-ordered as a standalone +// paragraph after the table. The fix in nextTableInsertOffset accumulates the +// FULL (tableEnd - tableIndex) instead of (tableEnd - tableIndex) - 1. + +func TestNextTableInsertOffset_AccumulatesFullTableSize(t *testing.T) { + off := int64(0) + + // Table 1: spans [10, 30) → size 20. + off = nextTableInsertOffset(off, 10, 30) + if off != 20 { + t.Fatalf("after table 1: offset = %d, want 20", off) + } + + // Table 2 was originally at placeholder 50; after shift it is at 50+20=70. + // It spans [70, 90) → size 20. Offset must accumulate to 40. + off = nextTableInsertOffset(off, 70, 90) + if off != 40 { + t.Fatalf("after table 2: offset = %d, want 40 (previous bug: 38)", off) + } + + // Table 3 was originally at placeholder 80; after shift it is at 80+40=120. + // It spans [120, 135) → size 15. Offset accumulates to 55. + off = nextTableInsertOffset(off, 120, 135) + if off != 55 { + t.Fatalf("after table 3: offset = %d, want 55 (previous bug: 52)", off) + } +} + +func TestNextTableInsertOffset_ZeroSizedTableLeavesOffsetUnchanged(t *testing.T) { + // If InsertNativeTable failed to grow the doc (tableEnd <= tableIndex), the + // offset must not change so subsequent placeholders stay at their plainText + // positions. + if got := nextTableInsertOffset(7, 10, 10); got != 7 { + t.Fatalf("equal indices: offset = %d, want 7 unchanged", got) + } + if got := nextTableInsertOffset(7, 10, 5); got != 7 { + t.Fatalf("tableEnd < tableIndex: offset = %d, want 7 unchanged", got) + } +} + +func TestNextTableInsertOffset_MatchesIssueRepro(t *testing.T) { + // The repro from #607 is three identical 2x2 tables interleaved with + // paragraphs. The exact API-side table size depends on the Docs service, + // but for ANY table size G > 0, the cumulative offset after N tables of + // size G must equal N*G. The previous (G-1) formula produced N*(G-1), + // undercounting by N and explaining why the corruption only became + // visually obvious starting at the 3rd table (when drift = 2). + const G = int64(17) + off := int64(0) + off = nextTableInsertOffset(off, 100, 100+G) + off = nextTableInsertOffset(off, 200, 200+G) + off = nextTableInsertOffset(off, 300, 300+G) + if off != 3*G { + t.Fatalf("3 tables of size %d → offset %d, want %d", G, off, 3*G) + } +} diff --git a/internal/cmd/docs_mutation.go b/internal/cmd/docs_mutation.go index c579fe6af..38fa8cc8e 100644 --- a/internal/cmd/docs_mutation.go +++ b/internal/cmd/docs_mutation.go @@ -169,9 +169,7 @@ func replaceDocsMarkdownRange(ctx context.Context, svc *docs.Service, doc *docs. if tableErr != nil { return fmt.Errorf("insert native table: %w", tableErr) } - if tableEnd > tableIndex { - tableOffset += (tableEnd - tableIndex) - 1 - } + tableOffset = nextTableInsertOffset(tableOffset, tableIndex, tableEnd) } } @@ -234,9 +232,7 @@ func insertDocsMarkdownAt(ctx context.Context, svc *docs.Service, docID string, if tableErr != nil { return len(requests), len(textToInsert), fmt.Errorf("insert native table: %w", tableErr) } - if tableEnd > tableIndex { - tableOffset += (tableEnd - tableIndex) - 1 - } + tableOffset = nextTableInsertOffset(tableOffset, tableIndex, tableEnd) } } diff --git a/internal/cmd/docs_table_inserter.go b/internal/cmd/docs_table_inserter.go index 77e327d54..ecb6825a3 100644 --- a/internal/cmd/docs_table_inserter.go +++ b/internal/cmd/docs_table_inserter.go @@ -198,3 +198,18 @@ func (ti *TableInserter) updateIndicesAfter(afterIndex, length int64, cellIndice *tableEndIndex += length } } + +// nextTableInsertOffset returns the running offset to apply to subsequent +// markdown-table placeholder positions after inserting a native table that +// spans [tableIndex, tableEnd). InsertTable inserts the new table BEFORE the +// existing character at tableIndex, so the placeholder "\n" we wrote into +// plainText for that table position stays in the doc; every subsequent +// placeholder therefore shifts forward by (tableEnd - tableIndex). The +// previous formula subtracted an extra 1, which accumulated one missing +// character of drift per table — see #607. +func nextTableInsertOffset(currentOffset, tableIndex, tableEnd int64) int64 { + if tableEnd <= tableIndex { + return currentOffset + } + return currentOffset + (tableEnd - tableIndex) +} From e9d6077b169b42df218a4174162740ba6fd72861 Mon Sep 17 00:00:00 2001 From: Chris Hall Date: Tue, 19 May 2026 20:37:15 -0400 Subject: [PATCH 2/3] docs(changelog): note #607 multi-table offset-drift fix Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f5c5c50f..ec4d49c27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ - Release: update the Homebrew handoff to publish through `openclaw/tap`. - Version: `gog --version` now reports an informative fallback (for example, `v0.17.0-dev`) when built from source with plain `go build` instead of returning `dev`. - Docs: `gog docs insert` now defaults to end-of-doc when `--index` is omitted, instead of always inserting at position 1 (which silently reversed iterative inserts across multiple calls). Pass `--index 1` explicitly to keep the previous behaviour. (#606) +- Docs: `docs write --append --markdown` with three or more markdown tables in a single render no longer drifts the per-table insertion offset by one character per table — the trailing punctuation of the paragraph immediately before the third (and any subsequent) table is preserved instead of being split into a standalone paragraph after the table. (#607) - Docs: `docs write --append --markdown` now expands inline markdown markers (`**bold**`, `*italic*`, `` `code` ``, `[link](url)`) inside table cells into character runs, matching the behaviour outside of tables — previously the markers rendered as literal characters because the table inserter bypassed the inline-formatting pass. (#608) - Docs: markdown empty-header table rows (e.g. `| | |`) no longer collide with the separator detection — previously `docs write --append --markdown` swallowed both the empty header and the real `|---|---|` separator, leaving the last data row re-parsed as a literal pipe paragraph after the table. (#609) From e0f6f4b5e4ab4dbdf95ccaf7d018c61ad73dc530 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 20 May 2026 21:03:30 +0100 Subject: [PATCH 3/3] style: use ascii in multi-table offset tests --- internal/cmd/docs_multi_table_offset_test.go | 10 +++++----- internal/cmd/docs_table_inserter.go | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/cmd/docs_multi_table_offset_test.go b/internal/cmd/docs_multi_table_offset_test.go index 31a7a5b8d..3e93ccc3d 100644 --- a/internal/cmd/docs_multi_table_offset_test.go +++ b/internal/cmd/docs_multi_table_offset_test.go @@ -7,26 +7,26 @@ import "testing" // table, causing the trailing punctuation of the paragraph immediately before // the third (and any subsequent) table to be re-ordered as a standalone // paragraph after the table. The fix in nextTableInsertOffset accumulates the -// FULL (tableEnd - tableIndex) instead of (tableEnd - tableIndex) - 1. +// full (tableEnd - tableIndex) instead of (tableEnd - tableIndex) - 1. func TestNextTableInsertOffset_AccumulatesFullTableSize(t *testing.T) { off := int64(0) - // Table 1: spans [10, 30) → size 20. + // Table 1: spans [10, 30), size 20. off = nextTableInsertOffset(off, 10, 30) if off != 20 { t.Fatalf("after table 1: offset = %d, want 20", off) } // Table 2 was originally at placeholder 50; after shift it is at 50+20=70. - // It spans [70, 90) → size 20. Offset must accumulate to 40. + // It spans [70, 90), size 20. Offset must accumulate to 40. off = nextTableInsertOffset(off, 70, 90) if off != 40 { t.Fatalf("after table 2: offset = %d, want 40 (previous bug: 38)", off) } // Table 3 was originally at placeholder 80; after shift it is at 80+40=120. - // It spans [120, 135) → size 15. Offset accumulates to 55. + // It spans [120, 135), size 15. Offset accumulates to 55. off = nextTableInsertOffset(off, 120, 135) if off != 55 { t.Fatalf("after table 3: offset = %d, want 55 (previous bug: 52)", off) @@ -58,6 +58,6 @@ func TestNextTableInsertOffset_MatchesIssueRepro(t *testing.T) { off = nextTableInsertOffset(off, 200, 200+G) off = nextTableInsertOffset(off, 300, 300+G) if off != 3*G { - t.Fatalf("3 tables of size %d → offset %d, want %d", G, off, 3*G) + t.Fatalf("3 tables of size %d: offset %d, want %d", G, off, 3*G) } } diff --git a/internal/cmd/docs_table_inserter.go b/internal/cmd/docs_table_inserter.go index ecb6825a3..12e195353 100644 --- a/internal/cmd/docs_table_inserter.go +++ b/internal/cmd/docs_table_inserter.go @@ -201,12 +201,12 @@ func (ti *TableInserter) updateIndicesAfter(afterIndex, length int64, cellIndice // nextTableInsertOffset returns the running offset to apply to subsequent // markdown-table placeholder positions after inserting a native table that -// spans [tableIndex, tableEnd). InsertTable inserts the new table BEFORE the +// spans [tableIndex, tableEnd). InsertTable inserts the new table before the // existing character at tableIndex, so the placeholder "\n" we wrote into // plainText for that table position stays in the doc; every subsequent // placeholder therefore shifts forward by (tableEnd - tableIndex). The // previous formula subtracted an extra 1, which accumulated one missing -// character of drift per table — see #607. +// character of drift per table; see #607. func nextTableInsertOffset(currentOffset, tableIndex, tableEnd int64) int64 { if tableEnd <= tableIndex { return currentOffset