Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .context/LEARNINGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
25 changes: 22 additions & 3 deletions internal/cli/add/core/insert/insert.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
109 changes: 109 additions & 0 deletions internal/cli/add/core/insert/insert_test.go
Original file line number Diff line number Diff line change
@@ -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

<!--
UPDATE WHEN:
- Discover a gotcha worth recording
-->

`

// 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)
}
}
24 changes: 24 additions & 0 deletions internal/cli/add/core/insert/testmain_test.go
Original file line number Diff line number Diff line change
@@ -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())
}
94 changes: 94 additions & 0 deletions specs/fix-afterheader-tail-truncation.md
Original file line number Diff line number Diff line change
@@ -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.
Loading