diff --git a/.abcd/work/issues/open/iss-30-memory-ingest-boundary.md b/.abcd/work/issues/open/iss-30-memory-ingest-boundary.md index 25c6a8b0..4a3b52b9 100644 --- a/.abcd/work/issues/open/iss-30-memory-ingest-boundary.md +++ b/.abcd/work/issues/open/iss-30-memory-ingest-boundary.md @@ -9,4 +9,26 @@ found_during: "2026-07-08 multi-agent review" found_at: "internal/core/memory/ingest.go" --- -memory ingest input-boundary defects: HTTP status is never checked so 404/500 error pages are silently ingested as source content (internal/core/memory/ingest.go:558-575); tilde expansion mangles ~user paths into home+user concatenations (ingest.go:579-584); a --keep-original failure after the page write reports total failure although pages and registry were durably mutated (ingest.go:301-311); CRLF pages are accepted by parseFrontmatter but rejected by splitFileFrontmatter so hashes and summaries silently degrade (yaml.go:558-591); the URL-ingest success path, content-type handling, PDF extraction, and original-storage are untested, as are YAML block scalars and double-quoted escapes. Detector: an ingest-boundary test suite — fetch status matrix, content-type matrix, CRLF round-trip, tilde cases, partial-failure reporting, parser-parity cases. Acceptance corpus: the six instances above. \ No newline at end of file +memory ingest input-boundary defects: HTTP status is never checked so 404/500 error pages are silently ingested as source content (internal/core/memory/ingest.go:558-575); tilde expansion mangles ~user paths into home+user concatenations (ingest.go:579-584); a --keep-original failure after the page write reports total failure although pages and registry were durably mutated (ingest.go:301-311); CRLF pages are accepted by parseFrontmatter but rejected by splitFileFrontmatter so hashes and summaries silently degrade (yaml.go:558-591); the URL-ingest success path, content-type handling, PDF extraction, and original-storage are untested, as are YAML block scalars and double-quoted escapes. Detector: an ingest-boundary test suite — fetch status matrix, content-type matrix, CRLF round-trip, tilde cases, partial-failure reporting, parser-parity cases. Acceptance corpus: the six instances above. +--- + +**Progress (2026-07-12, /abcd:run burst 2 — partial, issue stays OPEN):** two +instances drained behind their detectors: + +- `--keep-original` partial-failure reporting — FIXED. A `storeOriginal` failure + after the durable page+registry write now records `IngestResult.KeepOriginalError` + and returns the successful ingest (both the `ingested` and `registry_only` + paths); the CLI renders a warning + non-zero exit. The message is path-safe + (strips `*os.PathError`/`*os.LinkError` paths; repo-relative `sourcesRelPath`). + Tests: `TestIngestKeepOriginalFailureStillReportsIngest`, + `TestKeepOriginalErrorMessageNoPathLeak`, `TestMemoryIngestKeepOriginalPartialFailure`. +- CRLF parser-parity — FIXED. `splitFileFrontmatter` normalises line endings like + `parseFrontmatter`/`frontmatterKeyLine`, so CRLF documents split identically to + their LF form. Test: `TestSplitFileFrontmatterCRLFParity`. + +HTTP-status and tilde-expansion instances were addressed earlier (PR #38). + +**Remainder (keeps this issue open):** the untested ingest surfaces named in the +detector — URL-ingest success path, content-type matrix, PDF extraction, and +YAML block-scalar / double-quoted-escape parser cases. PDF extraction coverage +may require a dependency; assess against the no-new-dep STOP before adopting. diff --git a/CHANGELOG.md b/CHANGELOG.md index cb309aa8..0dd499f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -137,6 +137,16 @@ called out in a **Breaking** section. ### Fixed +- **Memory-ingest boundary — partial-failure reporting and CRLF parity** + (iss-30, continued). When `abcd memory ingest --keep-original` fails to store + the original *after* the pages and registry are durably written, it no longer + reports total failure: the successful ingest is reported (pages listed) with a + warning and a non-zero exit, and the failure message names only the + repo-relative store location — no absolute path, in text or `--json`. CRLF + documents now split identically to their LF form (`splitFileFrontmatter` + normalises line endings like its sibling parsers), so a `\r\n` closing `---` + delimiter is no longer rejected and hashes/summaries no longer silently + degrade. - **Fail-closed capture surface** (iss-29). A mistyped `capture` subcommand (e.g. `abcd capture resovle iss-1 …`) is no longer swallowed as free text and filed as a new issue; it is refused with a did-you-mean and writes nothing. diff --git a/internal/core/memory/ingest.go b/internal/core/memory/ingest.go index 01d8e305..62b61067 100644 --- a/internal/core/memory/ingest.go +++ b/internal/core/memory/ingest.go @@ -2,6 +2,7 @@ package memory import ( "errors" + "fmt" "io" "net" "net/http" @@ -75,9 +76,14 @@ type IngestResult struct { Pages []string `json:"pages"` Citation map[string]any `json:"citation"` KeptOriginal string `json:"kept_original"` - Linked [][2]string `json:"linked"` - Contradictions [][2]string `json:"contradictions"` - WriteReport *WriteReport `json:"write_report"` + // KeepOriginalError records a --keep-original copy failure that occurred + // AFTER the pages and registry were durably written. The ingest itself + // succeeded; only the best-effort original copy did not. Empty when + // --keep-original was not requested or the copy succeeded. + KeepOriginalError string `json:"keep_original_error,omitempty"` + Linked [][2]string `json:"linked"` + Contradictions [][2]string `json:"contradictions"` + WriteReport *WriteReport `json:"write_report"` } type sourceMaterial struct { @@ -174,11 +180,14 @@ func Ingest(req IngestRequest) (IngestResult, error) { if err != nil { return IngestResult{}, err } - kept := "" + // Best-effort keep-original: a failure after the durable write is + // recorded, never reported as total failure (iss-30). + kept, keepErr := "", "" if req.KeepOriginal { - kept, err = storeOriginal(root, material, contentHash) - if err != nil { - return IngestResult{}, err + if k, serr := storeOriginal(root, material, contentHash); serr != nil { + keepErr = keepOriginalErrorMessage(serr) + } else { + kept = k } } cachedEntry, _ := newRegistry[contentHash].(map[string]any) @@ -192,7 +201,7 @@ func Ingest(req IngestRequest) (IngestResult, error) { return IngestResult{ Status: "registry_only", ContentHash: contentHash, Licence: resultLicence, SourceTokenCount: tokenCount, Pages: recorded, Citation: deepCopyMap(cachedCitation), - KeptOriginal: kept, WriteReport: &report, + KeptOriginal: kept, KeepOriginalError: keepErr, WriteReport: &report, }, nil } } @@ -302,11 +311,15 @@ func Ingest(req IngestRequest) (IngestResult, error) { if err != nil { return IngestResult{}, err } - kept := "" + // storeOriginal runs AFTER the durable page + registry write. A failure + // here does not un-ingest anything, so it must not be reported as total + // failure — record it and return the successful result (iss-30). + kept, keepErr := "", "" if req.KeepOriginal { - kept, err = storeOriginal(root, material, contentHash) - if err != nil { - return IngestResult{}, err + if k, serr := storeOriginal(root, material, contentHash); serr != nil { + keepErr = keepOriginalErrorMessage(serr) + } else { + kept = k } } @@ -317,7 +330,8 @@ func Ingest(req IngestRequest) (IngestResult, error) { return IngestResult{ Status: status, ContentHash: contentHash, Licence: licence, SourceTokenCount: tokenCount, Pages: dedupPages, Citation: citation, - KeptOriginal: kept, Linked: plan.Linked, Contradictions: plan.Contradictions, + KeptOriginal: kept, KeepOriginalError: keepErr, + Linked: plan.Linked, Contradictions: plan.Contradictions, WriteReport: &report, }, nil } @@ -755,6 +769,26 @@ func safeExt(ext string) string { // Original storage (--keep-original) // --------------------------------------------------------------------------- +// sourcesRelPath is the repo-relative location of the kept-originals store, +// used in user-facing errors so no absolute path leaks into rendered output. +var sourcesRelPath = filepath.Join(".abcd", "memory", "sources") + +// keepOriginalErrorMessage renders a --keep-original failure without leaking the +// absolute sources path: filesystem errors embed the full path(s), so report +// only their bare cause against the repo-relative store location (iss-30). Both +// *PathError (Lstat/MkdirAll/OpenFile/Write/Sync) and *LinkError (Rename, which +// carries TWO absolute paths) are stripped; the only other storeOriginal error +// already names the repo-relative sourcesRelPath. +func keepOriginalErrorMessage(err error) string { + if pe := (*os.PathError)(nil); errors.As(err, &pe) { + return fmt.Sprintf("could not store original under %s: %s", sourcesRelPath, pe.Err.Error()) + } + if le := (*os.LinkError)(nil); errors.As(err, &le) { + return fmt.Sprintf("could not store original under %s: %s", sourcesRelPath, le.Err.Error()) + } + return err.Error() +} + func storeOriginal(repoRoot string, material sourceMaterial, contentHash string) (string, error) { sourcesDir := filepath.Join(Dir(repoRoot), "sources") if fi, err := os.Lstat(sourcesDir); err != nil { @@ -766,7 +800,7 @@ func storeOriginal(repoRoot string, material sourceMaterial, contentHash string) return "", err } } else if fi.Mode()&os.ModeSymlink != 0 || !fi.IsDir() { - return "", newIngestError("sources dir is a symlink or non-directory: %s", sourcesDir) + return "", newIngestError("sources dir is a symlink or non-directory: %s", sourcesRelPath) } target := filepath.Join(sourcesDir, contentHash+material.ext) tmp := target + "." + itoa(os.Getpid()) + ".memtmp" diff --git a/internal/core/memory/memory_test.go b/internal/core/memory/memory_test.go index 235976a3..64ac0939 100644 --- a/internal/core/memory/memory_test.go +++ b/internal/core/memory/memory_test.go @@ -436,3 +436,100 @@ func TestMaterialFromLocalTildeUser(t *testing.T) { t.Fatal("~baduser must be left literal, not expanded to home+baduser (which would wrongly succeed)") } } + +// --------------------------------------------------------------------------- +// iss-30: ingest-boundary — partial-failure reporting + CRLF parser parity +// --------------------------------------------------------------------------- + +// TestIngestKeepOriginalFailureStillReportsIngest proves the partial-failure +// instance: when --keep-original's storeOriginal fails AFTER WritePages has +// durably written the pages and registry, Ingest must report the successful +// ingest (not a bare total-failure error) and record the keep-original failure. +// The failure is forced by pre-creating the sources dir as a regular file. +func TestIngestKeepOriginalFailureStillReportsIngest(t *testing.T) { + repo := t.TempDir() + src := writeSource(t, repo, "article.txt", "Token rotation policy: rotate tokens every 24 hours.") + + // Make .abcd/memory/sources a regular file so storeOriginal fails. + if err := os.MkdirAll(Dir(repo), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(Dir(repo), "sources"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + + res, err := Ingest(IngestRequest{ + RepoRoot: repo, + Source: src, + KeepOriginal: true, + Distiller: oneTopicDistiller("topic", "auth", "tokens", "# Token rotation\nRotate tokens every 24 hours."), + Now: fixedNow, + }) + if err != nil { + t.Fatalf("ingest must not report total failure when only keep-original failed: %v", err) + } + if res.Status != "ingested" { + t.Fatalf("status = %q, want ingested", res.Status) + } + if len(res.Pages) != 1 || res.Pages[0] != "topic_auth_tokens.md" { + t.Fatalf("pages = %v, want [topic_auth_tokens.md]", res.Pages) + } + // The page reached disk — the mutation the old code hid behind a total error. + if _, err := os.Stat(filepath.Join(Dir(repo), "topic_auth_tokens.md")); err != nil { + t.Fatalf("page not durably written: %v", err) + } + if res.KeepOriginalError == "" { + t.Fatalf("keep-original failure must be recorded in the result") + } + if strings.Contains(res.KeepOriginalError, repo) { + t.Fatalf("keep-original error leaked the absolute repo path:\n%s", res.KeepOriginalError) + } + if res.KeptOriginal != "" { + t.Fatalf("KeptOriginal = %q, want empty when the copy failed", res.KeptOriginal) + } +} + +// TestSplitFileFrontmatterCRLFParity proves the parser-parity instance: a +// CRLF-terminated document must split identically to its LF twin. Before the +// fix splitFileFrontmatter's exact-match closing delimiter ("---" != "---\r") +// rejected CRLF, while parseFrontmatter accepted it — degrading hashes. +func TestSplitFileFrontmatterCRLFParity(t *testing.T) { + lf := "---\ntitle: Rotation\nyear: 2026\n---\nbody line one\nbody line two\n" + crlf := strings.ReplaceAll(lf, "\n", "\r\n") + + lfRegion, lfBody, err := splitFileFrontmatter(lf) + if err != nil { + t.Fatalf("LF split: %v", err) + } + crlfRegion, crlfBody, err := splitFileFrontmatter(crlf) + if err != nil { + t.Fatalf("CRLF split must not error (parser parity): %v", err) + } + if crlfRegion != lfRegion { + t.Fatalf("region parity broken:\n LF=%q\nCRLF=%q", lfRegion, crlfRegion) + } + if crlfBody != lfBody { + t.Fatalf("body parity broken:\n LF=%q\nCRLF=%q", lfBody, crlfBody) + } +} + +// TestKeepOriginalErrorMessageNoPathLeak locks the no-absolute-path invariant +// for every filesystem error shape storeOriginal can return: *os.PathError +// (single path) and *os.LinkError (rename, two paths). Both must be reduced to +// their bare cause against the repo-relative store location. +func TestKeepOriginalErrorMessageNoPathLeak(t *testing.T) { + abs := "/Users/someone/secret/repo/.abcd/memory/sources" + cases := []error{ + &os.PathError{Op: "open", Path: abs + "/deadbeef.pdf.tmp", Err: os.ErrPermission}, + &os.LinkError{Op: "rename", Old: abs + "/deadbeef.pdf.1.memtmp", New: abs + "/deadbeef.pdf", Err: os.ErrExist}, + } + for _, in := range cases { + msg := keepOriginalErrorMessage(in) + if strings.Contains(msg, "/Users/someone") { + t.Fatalf("leaked absolute path for %T: %q", in, msg) + } + if !strings.Contains(msg, sourcesRelPath) { + t.Fatalf("message for %T omits the repo-relative store path: %q", in, msg) + } + } +} diff --git a/internal/core/memory/yaml.go b/internal/core/memory/yaml.go index 0e181940..be9251c6 100644 --- a/internal/core/memory/yaml.go +++ b/internal/core/memory/yaml.go @@ -552,10 +552,16 @@ func indentOf(s string) int { // File-level split / join // --------------------------------------------------------------------------- -// splitFileFrontmatter splits full file text into (yaml region, body). The -// region is the text between the --- delimiters (each inner line -// newline-terminated); body is everything after the closing --- verbatim. +// splitFileFrontmatter splits full file text into (yaml region, body). Line +// endings are normalised to \n first (parser parity, below); the region is the +// text between the --- delimiters (each inner line newline-terminated); body is +// everything after the closing --- with normalised newlines. func splitFileFrontmatter(text string) (string, string, error) { + // Normalise line endings first, exactly as parseFrontmatter and + // frontmatterKeyLine do — otherwise a CRLF closing delimiter ("---\r") + // never equals "---" and the whole document is wrongly rejected, diverging + // from the parser that accepts it and degrading hashes/summaries (iss-30). + text = strings.ReplaceAll(strings.ReplaceAll(text, "\r\n", "\n"), "\r", "\n") lines := strings.Split(text, "\n") start := 0 for start < len(lines) { diff --git a/internal/surface/cli/cli.go b/internal/surface/cli/cli.go index 84803b22..c3b6bb48 100644 --- a/internal/surface/cli/cli.go +++ b/internal/surface/cli/cli.go @@ -1245,7 +1245,7 @@ func newMemoryCommand(asJSON *bool) *cobra.Command { if err != nil { return err } - return render(cmd.OutOrStdout(), *asJSON, res, func(w io.Writer) { + if err := render(cmd.OutOrStdout(), *asJSON, res, func(w io.Writer) { fmt.Fprintf(w, "abcd memory ingest — %s\n", res.Status) fmt.Fprintf(w, " content hash: %s\n", res.ContentHash) fmt.Fprintf(w, " licence: %s\n", res.Licence) @@ -1255,7 +1255,19 @@ func newMemoryCommand(asJSON *bool) *cobra.Command { if res.KeptOriginal != "" { fmt.Fprintf(w, " kept original: %s\n", res.KeptOriginal) } - }) + if res.KeepOriginalError != "" { + fmt.Fprintf(w, " warning: --keep-original failed (the source was still ingested): %s\n", res.KeepOriginalError) + } + }); err != nil { + return err + } + // The ingest succeeded but an explicitly-requested --keep-original + // copy did not: signal it with a non-zero exit while leaving the + // rendered result (which reports what was durably written) intact. + if res.KeepOriginalError != "" { + return &exitError{Code: 1} + } + return nil }, } ingestCmd.Flags().BoolVar(&keepOriginalFlag, "keep-original", false, "store the original at .abcd/memory/sources/.") diff --git a/internal/surface/cli/memory_ingest_surface_test.go b/internal/surface/cli/memory_ingest_surface_test.go new file mode 100644 index 00000000..cfae1a97 --- /dev/null +++ b/internal/surface/cli/memory_ingest_surface_test.go @@ -0,0 +1,55 @@ +package cli + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestMemoryIngestKeepOriginalPartialFailure proves the iss-30 fix is wired to +// the CLI surface: when --keep-original fails after the pages are durably +// written, `abcd memory ingest` reports the successful ingest with a warning, +// exits non-zero, and leaks no absolute path. The failure is forced by making +// the kept-originals directory a regular file. +func TestMemoryIngestKeepOriginalPartialFailure(t *testing.T) { + repo := t.TempDir() + t.Chdir(repo) + + src := filepath.Join(repo, "article.txt") + if err := os.WriteFile(src, []byte("Rotate tokens every 24 hours."), 0o644); err != nil { + t.Fatal(err) + } + // Make .abcd/memory/sources a regular file so storeOriginal fails. + if err := os.MkdirAll(filepath.Join(repo, ".abcd", "memory"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(repo, ".abcd", "memory", "sources"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + pages := filepath.Join(repo, "pages.json") + if err := os.WriteFile(pages, []byte(`[{"type":"topic","domain":"auth","slug":"tokens","body":"# Rotation\nRotate tokens every 24 hours."}]`), 0o644); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := Run([]string{"memory", "ingest", src, "--keep-original", "--pages-json", pages}, &stdout, &stderr) + if code == 0 { + t.Fatalf("expected a non-zero exit when --keep-original fails\nstdout: %s\nstderr: %s", stdout.String(), stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "warning: --keep-original failed") { + t.Fatalf("expected a --keep-original warning in the output:\n%s", out) + } + if !strings.Contains(out, "topic_auth_tokens.md") { + t.Fatalf("expected the durably-written page to be reported:\n%s", out) + } + if strings.Contains(out, repo) || strings.Contains(stderr.String(), repo) { + t.Fatalf("ingest output leaked the absolute repo path:\nstdout: %s\nstderr: %s", out, stderr.String()) + } + // The page really did reach disk despite the keep-original failure. + if _, err := os.Stat(filepath.Join(repo, ".abcd", "memory", "topic_auth_tokens.md")); err != nil { + t.Fatalf("page not durably written: %v", err) + } +}