diff --git a/.context/LEARNINGS.md b/.context/LEARNINGS.md index f142d86df..03efdb83f 100644 --- a/.context/LEARNINGS.md +++ b/.context/LEARNINGS.md @@ -15,6 +15,26 @@ DO NOT UPDATE FOR: --> +--- + +## [2026-07-17-081010] An insert helper that returns content[:i]+x instead of content[:i]+x+content[i:] silently drops the tail + +**Context**: insert.AfterHeader (the fallback beforeFirstEntry takes for a knowledge file with no ## [ entries) returned content[:insertPoint]+entry, truncating the file at the insertion point and discarding everything after it. Masked in practice because a ctx-init'd file has nothing after its comment block, so the dropped tail was empty; it bites the moment any non-entry section sits below the preamble of an as-yet-entry-less file. Same family as the LEARNINGS clobber bug index.Validate guards: silent memory loss, git-only recovery. + +**Lesson**: A string-splice 'insert' must reattach the tail. content[:i]+x is append-with-truncation, NOT insert. The sibling Task() splice did it right (content[:i]+x+sep+content[i:]); AfterHeader was the odd one out and nobody noticed because the package had ZERO tests. EOF-anchored fixtures mask the bug precisely because the tail is empty there. + +**Application**: Audit any result built from content[:i] for a matching content[i:] on the other side of the inserted text; a lone content[:i] is the smell. Test insert/splice helpers with a NON-empty tail, not just an EOF-anchored fixture. When touching a data-mutating helper with no tests, adding the test is part of the fix, not optional. + +--- + +## [2026-07-17-081010] Uninitialized desc.Text() returns empty, and strings.Index(s, "") == 0 makes anchor-based inserts silently match at offset 0 + +**Context**: Writing a layout proof for the insert package, both cases passed on the first run — for the wrong reason. A *_test.go that never calls lookup.Init() (via TestMain) gets "" back from every desc.Text() call, because the embedded asset lookup is uninitialized in that test binary. Anchor logic such as beforeFirstEntry does strings.Index(content, desc.Text(headingKey)); with a "" needle that returns 0, so 'insert before the anchor' prepends to the whole file (above the H1) and any assertion of the form 'entry appears before X' passes trivially. + +**Lesson**: strings.Index(s, "") == 0: an empty needle 'matches' at the start of any string. So a text-driven test whose helper resolves labels through desc.Text MUST initialize the asset lookup, or it exercises a degenerate offset-0 code path that production never takes — and passes for the wrong reason. This nearly caused a false 'measurement gate validated' report when the gate was in fact broken. + +**Application**: Any package test that calls desc.Text (directly, or transitively through the code under test) needs a TestMain calling lookup.Init() (see internal/cli/system/core/session/testmain_test.go for the pattern). When a text/anchor-driven test passes suspiciously easily, dump desc.Text() of the keys involved and assert they are non-empty before trusting the assertions. + --- ## [2026-07-16-120001] OpenCode plugin integration gotchas (consolidated) diff --git a/internal/cli/add/core/insert/insert.go b/internal/cli/add/core/insert/insert.go index ad07e0558..46597e413 100644 --- a/internal/cli/add/core/insert/insert.go +++ b/internal/cli/add/core/insert/insert.go @@ -17,12 +17,15 @@ import ( "github.com/ActiveMemory/ctx/internal/inspect" ) -// AfterHeader finds a header line and inserts content after it. +// AfterHeader finds a header line and inserts content after it, +// preserving any content that follows the insertion point. // // Skips blank lines and HTML comment blocks () between the header // and the insertion point, so new entries land after index tables, format // guides, and other comment-wrapped metadata. Falls back to appending at the -// end if the header is not found. +// end if the header is not found. When content already follows the insertion +// point, the entry is separated from it by a "---" rule; when nothing +// follows, the entry is appended as-is. // // Parameters: // - content: Existing file content @@ -69,7 +72,23 @@ func AfterHeader(content, entry, header string) []byte { insertPoint = inspect.SkipWhitespace(content, insertPoint) } - return []byte(content[:insertPoint] + entry) + // Preserve whatever follows the insertion point. Returning + // content[:insertPoint]+entry truncated the file here, silently + // discarding the tail: on an entry-less knowledge file any section + // below the preamble was destroyed by an add. An empty tail keeps + // the original byte-for-byte output, so only the losing shape + // changes. See specs/fix-afterheader-tail-truncation.md. + tail := content[insertPoint:] + if tail == "" { + return []byte(content[:insertPoint] + entry) + } + + return []byte( + content[:insertPoint] + entry + + token.NewlineLF + token.Separator + + token.NewlineLF + token.NewlineLF + + tail, + ) } // AppendAtEnd appends an entry at the end of content. diff --git a/internal/cli/add/core/insert/insert_test.go b/internal/cli/add/core/insert/insert_test.go new file mode 100644 index 000000000..59d9bd496 --- /dev/null +++ b/internal/cli/add/core/insert/insert_test.go @@ -0,0 +1,109 @@ +// / ctx: https://ctx.ist +// ,'`./ do you remember? +// `.,'\ +// \ Copyright 2026-present Context contributors. +// SPDX-License-Identifier: Apache-2.0 + +package insert_test + +import ( + "strings" + "testing" + + "github.com/ActiveMemory/ctx/internal/cli/add/core/insert" + cfgEntry "github.com/ActiveMemory/ctx/internal/config/entry" +) + +// Regression tests for specs/fix-afterheader-tail-truncation.md. +// +// AfterHeader returned content[:insertPoint] + entry — truncating the +// file at the insert point and discarding everything after it. Its sole +// caller is beforeFirstEntry's fallback, taken when a knowledge file has +// no "## [" entry, so `ctx learning add` against an entry-less file +// destroyed any content below the H1 header and its comment block. + +const preamble = `# Learnings + + + +` + +// tailSection is content that lives below the preamble of a file which +// has no "## [" entries — exactly the shape that triggers the fallback. +const tailSection = `## Notes + +- a hand-written section that must survive an add +` + +const addedEntry = "## [2026-07-17-120000] A brand new learning\n\n" + + "**Context**: freshly added.\n" + +const addedEntryHeader = "## [2026-07-17-120000]" + +// TestAfterHeader_PreservesTail is the bug: content below the preamble +// of an entry-less file must survive, with the new entry above it. +func TestAfterHeader_PreservesTail(t *testing.T) { + out := string(insert.AppendEntry( + []byte(preamble+tailSection), addedEntry, cfgEntry.Learning, "", + )) + + if !strings.Contains(out, "## Notes") { + t.Fatalf("tail section was destroyed by add; got:\n%s", out) + } + if !strings.Contains(out, "a hand-written section that must survive an add") { + t.Fatalf("tail body was destroyed by add; got:\n%s", out) + } + + entryIdx := strings.Index(out, addedEntryHeader) + tailIdx := strings.Index(out, "## Notes") + if entryIdx == -1 { + t.Fatalf("added entry missing; got:\n%s", out) + } + if entryIdx > tailIdx { + t.Errorf("entry landed below the tail (entry=%d tail=%d); "+ + "it must land directly after the preamble", entryIdx, tailIdx) + } +} + +// TestAfterHeader_EmptyTailUnchanged pins the shape that works today: +// an entry-less file with nothing after the comment block must produce +// byte-identical output to the pre-fix implementation (content + entry). +func TestAfterHeader_EmptyTailUnchanged(t *testing.T) { + out := string(insert.AppendEntry( + []byte(preamble), addedEntry, cfgEntry.Learning, "", + )) + + want := preamble + addedEntry + if out != want { + t.Errorf("empty-tail output changed.\n got: %q\nwant: %q", out, want) + } +} + +// TestBeforeFirstEntry_PrimaryPathUnchanged guards the common path: a +// file that already has entries still inserts before the first one and +// keeps every existing entry. +func TestBeforeFirstEntry_PrimaryPathUnchanged(t *testing.T) { + existing := preamble + + "## [2026-07-15-141726] an existing learning\n\n**Context**: old.\n" + + out := string(insert.AppendEntry( + []byte(existing), addedEntry, cfgEntry.Learning, "", + )) + + if !strings.Contains(out, "an existing learning") { + t.Fatalf("existing entry destroyed; got:\n%s", out) + } + + newIdx := strings.Index(out, addedEntryHeader) + oldIdx := strings.Index(out, "## [2026-07-15-141726]") + if newIdx == -1 || oldIdx == -1 { + t.Fatalf("an entry is missing; got:\n%s", out) + } + if newIdx > oldIdx { + t.Errorf("new entry must precede the existing one "+ + "(new=%d old=%d)", newIdx, oldIdx) + } +} diff --git a/internal/cli/add/core/insert/testmain_test.go b/internal/cli/add/core/insert/testmain_test.go new file mode 100644 index 000000000..192ce0250 --- /dev/null +++ b/internal/cli/add/core/insert/testmain_test.go @@ -0,0 +1,24 @@ +// / ctx: https://ctx.ist +// ,'`./ do you remember? +// `.,'\ +// \ Copyright 2026-present Context contributors. +// SPDX-License-Identifier: Apache-2.0 + +package insert_test + +import ( + "os" + "testing" + + "github.com/ActiveMemory/ctx/internal/assets/read/lookup" +) + +// TestMain initializes the embedded asset lookup so desc.Text resolves +// real values. Without it every desc.Text call returns "", and +// strings.Index(s, "") == 0 silently rewrites the insert anchors into +// "match at offset 0" — tests would exercise a path production never +// takes, and pass for the wrong reason. +func TestMain(m *testing.M) { + lookup.Init() + os.Exit(m.Run()) +} diff --git a/specs/fix-afterheader-tail-truncation.md b/specs/fix-afterheader-tail-truncation.md new file mode 100644 index 000000000..647aa6a51 --- /dev/null +++ b/specs/fix-afterheader-tail-truncation.md @@ -0,0 +1,94 @@ +# Spec: fix AfterHeader dropping everything after the insert point + +## Problem + +`insert.AfterHeader` (`internal/cli/add/core/insert/insert.go`) ends with: + +```go +return []byte(content[:insertPoint] + entry) +``` + +It truncates the file at `insertPoint` and appends the entry — the tail +(`content[insertPoint:]`) is **silently discarded**. It is an insert +function that does not insert. + +Its sole caller is `beforeFirstEntry`'s fallback +(`internal/cli/add/core/insert/before.go`), taken when a knowledge file +contains no `## [` entry. So `ctx learning add` / `ctx decision add` on +an entry-less LEARNINGS.md/DECISIONS.md destroys any content that sits +below the H1 header and its comment block. + +Today this is **masked**, not absent: an entry-less file freshly created +by `ctx init` has nothing after the comment block, so `insertPoint` +lands at EOF and the discarded tail is empty. The bug bites the moment +non-entry content exists below the preamble of a file that has no +entries yet — e.g. a hand-written `## Notes` section, or any structural +section a future feature adds. + +This is the same family as the `ctx learning add` clobber bug that +`index.Validate` exists to guard: silent destruction of persisted +memory, recoverable only from git. + +Discovered by the layout proof written for +`specs/progressive-disclosure.md` (plan `pd-m1`, T10), which planted a +`## Themes` section below the preamble of an entry-less root and watched +`add` delete it. The fix is specified and shipped **independently** of +that design: it is a defect in current behavior on its own terms. + +## Design + +Make `AfterHeader` a true insert, preserving the tail — the pattern its +sibling `Task` already follows +(`existingStr[:pendingIdx] + entry + NewlineLF + existingStr[pendingIdx:]`). + +When the tail is non-empty, separate the new entry from the following +content with the same delimiter `beforeFirstEntry`'s primary path uses, +so both paths in the same function family produce consistent files: + +``` +entry + NewlineLF + Separator + NewlineLF + NewlineLF + tail +``` + +When the tail is **empty**, emit `content[:insertPoint] + entry` +unchanged — byte-identical to today's output. This keeps the fix a +strict superset of current behavior: every file shape that works today +produces identical bytes, and only the shape that currently loses data +changes. + +## Implementation + +- `internal/cli/add/core/insert/insert.go`: `AfterHeader` returns + `content[:insertPoint] + entry` when `content[insertPoint:]` is empty, + and `content[:insertPoint] + entry + sep + tail` otherwise. +- No signature change; one caller; no other call sites. + +## Tests + +The package had **no tests**. Add: + +- `testmain_test.go`: `lookup.Init()` in `TestMain`. Without it + `desc.Text` returns `""` and `strings.Index(s, "")` is `0`, which + silently turns every insert anchor into "match at offset 0" — tests + would exercise a path production never takes and pass for the wrong + reason. This is a precondition for any honest test of this package. +- Tail preserved: content below the preamble of an entry-less file + survives an add, and the entry lands above it. +- Empty tail unchanged: an entry-less file with nothing after the + comment block produces byte-identical output to the pre-fix + implementation. +- Primary path untouched: a file with `## [` entries still inserts + before the first one (regression guard for `beforeFirstEntry`). + +## Acceptance + +- `ctx learning add` against an entry-less file carrying a section below + the preamble preserves that section. +- Output for every file shape that works today is byte-identical. +- `make lint` clean; full suite green. + +## Non-Goals + +- No change to `beforeFirstEntry`'s primary (`## [`-anchored) path. +- No change to `Task`, `TaskAfterSection`, `AppendAtEnd`, or `Decision`. +- Not a progressive-disclosure change: that design merely *found* this; + the fix stands alone and ships alone.