backup: address review on KEYMAP/MANIFEST (PR #712)

- Adapters fields are now *Adapter pointers (Gemini #98). Nil ->
  excluded, non-nil empty -> included-with-no-scopes, non-nil
  populated -> normal. Previous non-pointer struct collapsed both
  "excluded" and "included-empty" into the same on-disk shape;
  gemini's reference (don't silently drop entries during
  serialization) was the right call. Test
  TestAdaptersStruct_NilVsEmptyDistinguishedOnDisk covers it.
- KeymapReader.Next now validates the base64-encoded `original`
  field at parse time (Codex P1 #179). Previously the JSON parsed
  fine and the bad base64 surfaced lazily on Original() — that
  defers corruption detection past initial ingest. Test
  TestKeymapReader_RejectsMalformedBase64AtParseTime locks in the
  parse-time validation.
- ReadManifest now rejects trailing bytes via dec.More() (Codex P2
  #194). Two manifests concatenated, garbage tails, partial-write
  artifacts: all surface as ErrInvalidManifest. Tests
  TestReadManifest_RejectsTrailingBytes and
  TestReadManifest_RejectsTrailingNonWhitespace.
- Test name TestKeymapReader_AcceptsBlankLinesByPolicy renamed to
  TestKeymapReader_RejectsBlankLines (Gemini #160). The test was
  already asserting rejection; only the name was misleading.

Author: bootjp

internal/backup/keymap.go

@@ -155,6 +155,11 @@ func NewKeymapReader(r io.Reader) *KeymapReader {
 // (zero, false, nil) at end of stream, and (zero, false, err) on parse
 // failure or I/O error. Once an error is returned the reader is sticky:
 // subsequent calls return the same error.
+//
+// The base64-encoded `original` field is validated at parse time rather
+// than lazily: a malformed dump must surface on the first read of the
+// affected line, not propagate silently until a much later
+// rec.Original() call. Same error class either way.
 func (r *KeymapReader) Next() (KeymapRecord, bool, error) {
 	if r.err != nil {
 		return KeymapRecord{}, false, r.err
@@ -176,6 +181,10 @@ func (r *KeymapReader) Next() (KeymapRecord, bool, error) {
 		r.err = errors.Wrap(ErrInvalidKeymapRecord, "missing encoded or kind")
 		return KeymapRecord{}, false, r.err
 	}
+	if _, err := base64.RawURLEncoding.DecodeString(rec.OriginalB64); err != nil {
+		r.err = errors.Wrap(ErrInvalidKeymapRecord, err.Error())
+		return KeymapRecord{}, false, r.err
+	}
 	return rec, true, nil
 }
 

internal/backup/keymap_test.go

@@ -157,13 +157,13 @@ func TestKeymapReader_RejectsRecordWithoutEncodedOrKind(t *testing.T) {
 	}
 }
 
-func TestKeymapReader_AcceptsBlankLinesByPolicy(t *testing.T) {
+func TestKeymapReader_RejectsBlankLines(t *testing.T) {
 	t.Parallel()
 	// bufio.Scanner skips trailing newline but emits an empty line when one
 	// is in the middle of the stream. We require strict JSONL — every
-	// non-empty line must be a record. An empty line in the middle should
-	// surface as ErrInvalidKeymapRecord rather than silently skipped, so
-	// truncated dumps are recognised.
+	// non-empty line must be a record. An empty line in the middle must
+	// surface as ErrInvalidKeymapRecord rather than be silently skipped,
+	// so truncated dumps are recognised.
 	input := `{"encoded":"x","original":"AA","kind":"sha-fallback"}` + "\n\n" +
 		`{"encoded":"y","original":"AA","kind":"sha-fallback"}` + "\n"
 	r := NewKeymapReader(strings.NewReader(input))
@@ -203,3 +203,17 @@ func TestKeymapRecord_OriginalRejectsBadBase64(t *testing.T) {
 		t.Fatalf("err = %v, want ErrInvalidKeymapRecord", err)
 	}
 }
+
+func TestKeymapReader_RejectsMalformedBase64AtParseTime(t *testing.T) {
+	t.Parallel()
+	// JSON parses fine; the structural fields are present; only the
+	// `original` base64 is malformed. The reader must catch this on
+	// the first Next() rather than defer it to a later Original()
+	// call — Codex P1 #179.
+	input := `{"encoded":"x","original":"!!!","kind":"sha-fallback"}` + "\n"
+	r := NewKeymapReader(strings.NewReader(input))
+	_, _, err := r.Next()
+	if !errors.Is(err, ErrInvalidKeymapRecord) {
+		t.Fatalf("err=%v want ErrInvalidKeymapRecord on parse-time base64 validation", err)
+	}
+}

internal/backup/manifest.go

@@ -87,14 +87,27 @@ type Live struct {
 	PinTokenSHA256 string `json:"pin_token_sha256,omitempty"`
 }
 
-// Adapters lists which scopes were dumped per adapter. An empty slice
-// means "no scopes for this adapter were dumped"; a nil slice means
-// "this adapter was not in the dump's scope filter."
+// Adapters lists which scopes were dumped per adapter. The pointer
+// values express two distinguishable on-disk states:
+//
+//   - nil   -> the adapter was excluded from this dump (e.g.
+//     `--adapter dynamodb,s3` filtered it out). The corresponding
+//     JSON key is absent.
+//   - non-nil pointer to Adapter{}  -> the adapter was in scope but
+//     no scopes for it were emitted (no tables, no buckets, etc.).
+//     The JSON key is present with an empty object.
+//   - non-nil pointer to a populated Adapter -> the listed scopes
+//     were emitted.
+//
+// Storing pointers (rather than zero-value Adapter structs) is what
+// keeps "excluded by filter" distinguishable from "included but
+// empty" through json.Marshal — non-pointer fields would collapse
+// both states into the same on-disk shape.
 type Adapters struct {
-	DynamoDB Adapter `json:"dynamodb"`
-	S3       Adapter `json:"s3"`
-	Redis    Adapter `json:"redis"`
-	SQS      Adapter `json:"sqs"`
+	DynamoDB *Adapter `json:"dynamodb,omitempty"`
+	S3       *Adapter `json:"s3,omitempty"`
+	Redis    *Adapter `json:"redis,omitempty"`
+	SQS      *Adapter `json:"sqs,omitempty"`
 }
 
 // Adapter holds the scope identifiers for one adapter. Field names are
@@ -193,6 +206,17 @@ func ReadManifest(r io.Reader) (Manifest, error) {
 	if err := dec.Decode(&m); err != nil {
 		return Manifest{}, errors.Wrap(ErrInvalidManifest, err.Error())
 	}
+	// MANIFEST.json is exactly one JSON object. Trailing bytes
+	// (a second object, junk, even whitespace-only padding) point at
+	// concatenation bugs or partial-write corruption — both of which
+	// must surface here rather than be silently discarded. We use
+	// io.Discard rather than parsing because we only care that
+	// nothing-decodable is present; structural validation lives in
+	// validate().
+	if dec.More() {
+		return Manifest{}, errors.Wrap(ErrInvalidManifest,
+			"trailing bytes after manifest JSON object")
+	}
 	if m.FormatVersion == 0 {
 		return Manifest{}, errors.Wrapf(ErrUnsupportedFormatVersion,
 			"format_version is zero")

internal/backup/manifest_test.go

@@ -20,10 +20,10 @@ func TestManifest_Phase0RoundTrip(t *testing.T) {
 	m.LastCommitTS = 4517352099840000
 	m.Source = &Source{FSMPath: "/data/fsm-snap/0000000000000064.fsm", FSMCRC32C: "deadbeef"}
 	m.Adapters = Adapters{
-		DynamoDB: Adapter{Tables: []string{"orders", "users"}},
-		S3:       Adapter{Buckets: []string{"photos"}},
-		Redis:    Adapter{Databases: []uint32{0}},
-		SQS:      Adapter{Queues: []string{"orders-fifo.fifo"}},
+		DynamoDB: &Adapter{Tables: []string{"orders", "users"}},
+		S3:       &Adapter{Buckets: []string{"photos"}},
+		Redis:    &Adapter{Databases: []uint32{0}},
+		SQS:      &Adapter{Queues: []string{"orders-fifo.fifo"}},
 	}
 	m.Exclusions = Exclusions{} // all defaults
 
@@ -205,6 +205,61 @@ func TestNewPhase0SnapshotManifest_DefaultsArePopulated(t *testing.T) {
 	}
 }
 
+func TestReadManifest_RejectsTrailingBytes(t *testing.T) {
+	t.Parallel()
+	// Two manifests concatenated; the second must surface as a
+	// trailing-bytes error rather than be silently discarded — Codex
+	// P2 #194.
+	m := NewPhase0SnapshotManifest(time.Now())
+	body, err := json.Marshal(m)
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+	bad := append([]byte{}, body...)
+	bad = append(bad, body...)
+	_, err = ReadManifest(bytes.NewReader(bad))
+	if !errors.Is(err, ErrInvalidManifest) {
+		t.Fatalf("err=%v want ErrInvalidManifest on trailing bytes", err)
+	}
+}
+
+func TestReadManifest_RejectsTrailingNonWhitespace(t *testing.T) {
+	t.Parallel()
+	m := NewPhase0SnapshotManifest(time.Now())
+	body, err := json.Marshal(m)
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+	bad := append([]byte{}, body...)
+	bad = append(bad, []byte("garbage")...)
+	_, err = ReadManifest(bytes.NewReader(bad))
+	if !errors.Is(err, ErrInvalidManifest) {
+		t.Fatalf("err=%v want ErrInvalidManifest on trailing garbage", err)
+	}
+}
+
+func TestAdaptersStruct_NilVsEmptyDistinguishedOnDisk(t *testing.T) {
+	t.Parallel()
+	// Gemini #98: an excluded adapter (nil pointer) must serialize
+	// differently from an included-but-empty adapter (non-nil pointer
+	// to Adapter{}).
+	excluded := Adapters{
+		DynamoDB: &Adapter{}, // present, no scopes
+		// S3 / Redis / SQS left nil — out of scope
+	}
+	body, err := json.Marshal(excluded)
+	if err != nil {
+		t.Fatal(err)
+	}
+	out := string(body)
+	if !strings.Contains(out, `"dynamodb":{}`) {
+		t.Fatalf("included-empty must serialise as `dynamodb:{}`, got %s", out)
+	}
+	if strings.Contains(out, `"s3"`) || strings.Contains(out, `"redis"`) || strings.Contains(out, `"sqs"`) {
+		t.Fatalf("excluded adapters must be omitted, got %s", out)
+	}
+}
+
 func TestWriteManifest_ProducesPrettyJSON(t *testing.T) {
 	t.Parallel()
 	m := NewPhase0SnapshotManifest(time.Now())