backup: enforce field presence + version-first decode (PR #712, round 5)

Codex P2 round 5 raised two correctness issues:

1. KeymapReader.Next accepted records missing the `original` field
   because base64.RawURLEncoding.DecodeString("") succeeds. A
   corrupted line dropping `original` silently rewrote the
   encoded->original mapping to empty bytes, breaking exact key
   recovery for SHA-fallback / collision-renamed entries. Now the
   reader decodes into a presence-aware map first and rejects
   records missing any of {encoded, original, kind}. Explicit empty
   string remains valid.

2. ReadManifest decoded into Manifest before applying the
   format_version gate. A future-major manifest that also changed
   the JSON type of a known field (e.g. `phase` string -> int) was
   surfacing as ErrInvalidManifest instead of
   ErrUnsupportedFormatVersion, breaking the advertised
   version-branching contract. Now ReadManifest probes
   format_version with a relaxed shape first, branches on the
   result, and only runs the strict struct decode on a
   known-supported version.

Tests: TestKeymapReader_RejectsMissingOriginalField,
TestKeymapReader_AcceptsExplicitEmptyOriginal,
TestReadManifest_FutureMajorVersionTakesPrecedenceOverTypeMismatch.

Author: bootjp

internal/backup/keymap.go

@@ -172,20 +172,47 @@ func (r *KeymapReader) Next() (KeymapRecord, bool, error) {
 		return KeymapRecord{}, false, nil
 	}
 	line := r.sc.Bytes()
+	rec, err := decodeKeymapLine(line)
+	if err != nil {
+		r.err = err
+		return KeymapRecord{}, false, r.err
+	}
+	return rec, true, nil
+}
+
+// decodeKeymapLine parses one JSONL record. It enforces three properties:
+//
+//  1. The record must contain `encoded`, `original`, and `kind` fields —
+//     a missing `original` would otherwise be silently rewritten to empty
+//     bytes by base64.RawURLEncoding.DecodeString(""). Codex P2 round 5.
+//  2. `encoded` and `kind` must be non-empty strings.
+//  3. `original` (the base64) must be parseable at parse time so a
+//     corrupted dump fails on first read rather than at later
+//     Original() call. Codex P1 #179.
+func decodeKeymapLine(line []byte) (KeymapRecord, error) {
+	// Two-phase decode: first into a presence-aware map so we can
+	// distinguish "field absent" from "field present and empty
+	// string"; then into the typed struct for value extraction.
+	var fields map[string]json.RawMessage
+	if err := json.Unmarshal(line, &fields); err != nil {
+		return KeymapRecord{}, errors.Wrap(ErrInvalidKeymapRecord, err.Error())
+	}
+	for _, name := range [...]string{"encoded", "original", "kind"} {
+		if _, ok := fields[name]; !ok {
+			return KeymapRecord{}, errors.Wrapf(ErrInvalidKeymapRecord, "missing field %q", name)
+		}
+	}
 	var rec KeymapRecord
 	if err := json.Unmarshal(line, &rec); err != nil {
-		r.err = errors.Wrap(ErrInvalidKeymapRecord, err.Error())
-		return KeymapRecord{}, false, r.err
+		return KeymapRecord{}, errors.Wrap(ErrInvalidKeymapRecord, err.Error())
 	}
 	if rec.Encoded == "" || rec.Kind == "" {
-		r.err = errors.Wrap(ErrInvalidKeymapRecord, "missing encoded or kind")
-		return KeymapRecord{}, false, r.err
+		return KeymapRecord{}, errors.Wrap(ErrInvalidKeymapRecord, "missing encoded or kind")
 	}
 	if _, err := base64.RawURLEncoding.DecodeString(rec.OriginalB64); err != nil {
-		r.err = errors.Wrap(ErrInvalidKeymapRecord, err.Error())
-		return KeymapRecord{}, false, r.err
+		return KeymapRecord{}, errors.Wrap(ErrInvalidKeymapRecord, err.Error())
 	}
-	return rec, true, nil
+	return rec, nil
 }
 
 // LoadKeymap reads every record from r into an in-memory map keyed by

internal/backup/keymap_test.go

@@ -217,3 +217,47 @@ func TestKeymapReader_RejectsMalformedBase64AtParseTime(t *testing.T) {
 		t.Fatalf("err=%v want ErrInvalidKeymapRecord on parse-time base64 validation", err)
 	}
 }
+
+// TestKeymapReader_RejectsMissingOriginalField exercises Codex P2 round 5:
+// a record that omits `original` entirely must not be accepted as if the
+// original key were empty bytes, because base64.DecodeString("") succeeds
+// silently. A truncated dump that drops `original` would otherwise rewrite
+// the encoded->original mapping to empty bytes and break exact key recovery
+// for SHA-fallback or collision-renamed entries.
+func TestKeymapReader_RejectsMissingOriginalField(t *testing.T) {
+	t.Parallel()
+	// All structural keys present except `original`. Without the
+	// presence check this passes, because rec.OriginalB64 defaults to
+	// "" and base64 decode of "" succeeds.
+	input := `{"encoded":"x","kind":"sha-fallback"}` + "\n"
+	r := NewKeymapReader(strings.NewReader(input))
+	_, _, err := r.Next()
+	if !errors.Is(err, ErrInvalidKeymapRecord) {
+		t.Fatalf("err=%v want ErrInvalidKeymapRecord on missing `original` field", err)
+	}
+	// Sticky: a subsequent Next must keep returning the same error class.
+	_, _, err2 := r.Next()
+	if !errors.Is(err2, ErrInvalidKeymapRecord) {
+		t.Fatalf("non-sticky error: %v", err2)
+	}
+}
+
+// TestKeymapReader_AcceptsExplicitEmptyOriginal sanity-checks that an
+// explicitly-empty `original` (the field is present, value is "") still
+// parses. The contract is that absence is rejected, not emptiness.
+func TestKeymapReader_AcceptsExplicitEmptyOriginal(t *testing.T) {
+	t.Parallel()
+	input := `{"encoded":"x","original":"","kind":"sha-fallback"}` + "\n"
+	r := NewKeymapReader(strings.NewReader(input))
+	rec, ok, err := r.Next()
+	if err != nil || !ok {
+		t.Fatalf("err=%v ok=%v want a record", err, ok)
+	}
+	got, err := rec.Original()
+	if err != nil {
+		t.Fatalf("Original(): %v", err)
+	}
+	if len(got) != 0 {
+		t.Fatalf("Original = %q, want empty", got)
+	}
+}

internal/backup/manifest.go

@@ -1,6 +1,7 @@
 package backup
 
 import (
+	"bytes"
 	"encoding/json"
 	"io"
 	"time"
@@ -210,8 +211,38 @@ func WriteManifest(w io.Writer, m Manifest) error {
 // error is wrapped as ErrUnsupportedFormatVersion or ErrInvalidManifest so
 // callers can branch on errors.Is.
 func ReadManifest(r io.Reader) (Manifest, error) {
+	// Read the entire payload once so we can pre-decode just the
+	// format_version before strict struct decoding. Without this
+	// two-phase approach, a manifest produced by a newer major version
+	// that also changed the JSON type of a known field (e.g. `phase`
+	// switched from string to int) would surface as
+	// ErrInvalidManifest instead of ErrUnsupportedFormatVersion,
+	// breaking the documented version-branching contract for callers
+	// that key off errors.Is(err, ErrUnsupportedFormatVersion). See
+	// Codex P2, round 5.
+	payload, err := io.ReadAll(r)
+	if err != nil {
+		return Manifest{}, errors.Wrap(ErrInvalidManifest, err.Error())
+	}
+	// Phase 1: probe format_version with a relaxed shape that tolerates
+	// arbitrary types on every other field.
+	var probe struct {
+		FormatVersion uint32 `json:"format_version"`
+	}
+	if err := json.Unmarshal(payload, &probe); err != nil {
+		return Manifest{}, errors.Wrap(ErrInvalidManifest, err.Error())
+	}
+	if probe.FormatVersion == 0 {
+		return Manifest{}, errors.Wrapf(ErrUnsupportedFormatVersion,
+			"format_version is zero")
+	}
+	if probe.FormatVersion > CurrentFormatVersion {
+		return Manifest{}, errors.Wrapf(ErrUnsupportedFormatVersion,
+			"format_version %d > current %d (newer producer)", probe.FormatVersion, CurrentFormatVersion)
+	}
+	// Phase 2: strict struct decode on a known-supported version.
 	var m Manifest
-	dec := json.NewDecoder(r)
+	dec := json.NewDecoder(bytes.NewReader(payload))
 	// We intentionally do NOT call DisallowUnknownFields here.
 	// The format-version contract (Codex P1, follow-up) is:
 	//   - format_version > CurrentFormatVersion -> hard refuse
@@ -237,14 +268,6 @@ func ReadManifest(r io.Reader) (Manifest, error) {
 		return Manifest{}, errors.Wrap(ErrInvalidManifest,
 			"trailing bytes after manifest JSON object")
 	}
-	if m.FormatVersion == 0 {
-		return Manifest{}, errors.Wrapf(ErrUnsupportedFormatVersion,
-			"format_version is zero")
-	}
-	if m.FormatVersion > CurrentFormatVersion {
-		return Manifest{}, errors.Wrapf(ErrUnsupportedFormatVersion,
-			"format_version %d > current %d (newer producer)", m.FormatVersion, CurrentFormatVersion)
-	}
 	if err := m.validate(); err != nil {
 		return Manifest{}, err
 	}

internal/backup/manifest_test.go

@@ -130,6 +130,35 @@ func TestReadManifest_RejectsFutureFormatVersion(t *testing.T) {
 	}
 }
 
+// TestReadManifest_FutureMajorVersionTakesPrecedenceOverTypeMismatch is the
+// regression test for Codex P2 round 5: a newer-major manifest that also
+// changes the JSON type of a known field (e.g. `phase` from string to int)
+// must surface as ErrUnsupportedFormatVersion, not ErrInvalidManifest. The
+// version-branching contract advertised to callers (errors.Is(err,
+// ErrUnsupportedFormatVersion) means "upgrade required") only holds if the
+// format_version probe runs before the strict struct decode.
+func TestReadManifest_FutureMajorVersionTakesPrecedenceOverTypeMismatch(t *testing.T) {
+	t.Parallel()
+	body := `{
+		"format_version": 999,
+		"phase": 42,
+		"wall_time_iso": "2026-04-29T00:00:00Z",
+		"adapters": {"dynamodb":{}, "s3":{}, "redis":{}, "sqs":{}},
+		"exclusions": {"include_incomplete_uploads":false,"include_orphans":false,"preserve_sqs_visibility":false,"include_sqs_side_records":false},
+		"checksum_algorithm": "sha256",
+		"checksum_format": "sha256sum",
+		"encoded_filename_charset": "rfc3986-unreserved-plus-percent",
+		"key_segment_max_bytes": 240,
+		"s3_meta_suffix": ".elastickv-meta.json",
+		"s3_collision_strategy": "leaf-data-suffix",
+		"dynamodb_layout": "per-item"
+	}`
+	_, err := ReadManifest(strings.NewReader(body))
+	if !errors.Is(err, ErrUnsupportedFormatVersion) {
+		t.Fatalf("err=%v want ErrUnsupportedFormatVersion (must precede strict decode)", err)
+	}
+}
+
 func TestReadManifest_RejectsZeroFormatVersion(t *testing.T) {
 	t.Parallel()
 	m := NewPhase0SnapshotManifest(time.Now())