fix: memory-ingest partial-failure reporting + CRLF parity (iss-30, partial)

.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.
+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.

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.

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"

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)
+		}
+	}
+}

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) {

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/<sha256>.<ext>")

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)
+	}
+}