fix(server,web): close the four gaps Codex round 1 found on the carry-through (BUG-2674)

Round 1 raised no P1 and four P2s. Three were real defects introduced or exposed
by the carry-through; one was a genuine overclaim in the previous commit. All
four closed here, each mutation-verified rather than asserted.

## A schema may no longer declare a reserved key

MigrateFields carries these keys by identity, but every caller then validates
against the target schema — and ValidateFieldsDetailed iterates schema.Fields,
so it DOES see a declared key. A target declaring implementation_notes as `text`
would receive the carried array and reject it, turning a move that previously
destroyed the notes into one that fails outright. That is a worse failure than
the one being fixed: loud, but it blocks an operation that used to work.

The gate already existed — validateNoReservedFieldKeys, with its
grandfathering — and listed only parent/plan. The four metadata keys join it,
sourced from models.ReservedItemFieldKeys() so the two lists cannot drift.
Forbidding the declaration is the honest fix; coercing the value, or skipping
validation for a key the schema genuinely declares, would be guessing at which
meaning the author wanted.

The web's RESERVED_FIELD_KEYS gains the same four, preserving the existing
deliberate asymmetry (the client lowercases and is therefore stricter than the
server's exact match) so the UI steers authors away before the 400.

## The copy preflight no longer under-reports

`carried` is built by walking the DESTINATION SCHEMA, and these keys are declared
by no schema anywhere — so after the carry-through they appeared in NEITHER
bucket. A copy of an item whose content is its notes would report "nothing
carries over" while in fact retaining them. Before the carry-through they at
least showed under `dropped`, accurately. Reporting in neither is a regression
in the preflight's honesty, which is the same defect class as the move that
reported nothing.

They are now appended to `carried` after the schema-ordered entries, marked
`type: "system"` with a rendered label since they have no author-supplied one.
The bucket's doc comment says so: a client must no longer assume every `carried`
entry resolves to a destination FieldDef.

## The audit report now reaches a human

The previous commit claimed the activity timeline is where someone asks "what
happened to my item" — true, and the timeline renderer ignored the key, so the
report existed only for API and CLI consumers. Stored-but-invisible is not
reported. TimelineActivityCard renders the dropped keys on a move.

## Test aliasing

The "untouched" assertions compared the result against the SAME objects passed
in, so an in-place mutation would change both sides and DeepEqual would stay
true. The expectations are now independent deep copies — the only thing that
makes "untouched" mean untouched.

## Mutants, each run

Preflight pass removed -> the carried assertion fails. Timeline block disabled
-> the render assertion fails. Timeline action guard dropped -> the non-move
negative leg fails (a presence-only test would have passed it). Reserved-set
helper returning everything -> the IsReservedItemField control leg fails.

## Not fixed here

Codex's remaining observation — that a cross-workspace copy now carries
github_pr into a workspace whose repository it does not describe, and leaves a
convention blob detectable on an item outside the conventions collection — is a
product question about what a copy MEANS, not a defect in this mechanism. Raised
for a ruling rather than decided inside a bug fix.

Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

internal/items/migrate_test.go

@@ -136,6 +136,16 @@ func TestMigrateFields_ReservedKeysCarryThroughUntouched(t *testing.T) {
 	pr := map[string]any{"number": float64(42), "url": "https://example.invalid/42"}
 	convention := map[string]any{"trigger": "always", "enforcement": "must"}
 
+	// The expectations are INDEPENDENT deep copies, not aliases of the values
+	// handed to MigrateFields (Codex round 1). Comparing the result against the
+	// same backing objects that were passed in cannot detect an in-place
+	// mutation: both sides change together and DeepEqual stays true. These
+	// copies are the only thing that makes "untouched" mean untouched.
+	wantNotes := []any{map[string]any{"id": "note-1", "summary": "carried"}}
+	wantDecisions := []any{map[string]any{"id": "decision-1", "decision": "carried"}}
+	wantPR := map[string]any{"number": float64(42), "url": "https://example.invalid/42"}
+	wantConvention := map[string]any{"trigger": "always", "enforcement": "must"}
+
 	fields := map[string]any{
 		"implementation_notes": notes,
 		"decision_log":         decisions,
@@ -147,10 +157,10 @@ func TestMigrateFields_ReservedKeysCarryThroughUntouched(t *testing.T) {
 	result := MigrateFields(fields, source, target)
 
 	for key, want := range map[string]any{
-		"implementation_notes": notes,
-		"decision_log":         decisions,
-		"github_pr":            pr,
-		"convention":           convention,
+		"implementation_notes": wantNotes,
+		"decision_log":         wantDecisions,
+		"github_pr":            wantPR,
+		"convention":           wantConvention,
 	} {
 		got, ok := result.Fields[key]
 		if !ok {
@@ -188,13 +198,41 @@ func TestMigrateFields_ReservedKeysBypassSchemaMatching(t *testing.T) {
 	target := []models.FieldDef{{Key: "implementation_notes", Type: "text"}}
 
 	notes := []any{map[string]any{"id": "note-1", "summary": "carried"}}
+	want := []any{map[string]any{"id": "note-1", "summary": "carried"}} // independent copy — see above
 	result := MigrateFields(map[string]any{"implementation_notes": notes}, source, target)
 
 	got, ok := result.Fields["implementation_notes"]
 	if !ok {
 		t.Fatal("implementation_notes must carry through even when the target declares the key")
 	}
-	if !reflect.DeepEqual(got, notes) {
-		t.Errorf("payload must not be coerced by the target FieldDef:\n got %#v\nwant %#v", got, notes)
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("payload must not be coerced by the target FieldDef:\n got %#v\nwant %#v", got, want)
+	}
+}
+
+// The schema collision the test above simulates must not be REACHABLE: a
+// collection schema may not declare a reserved key in the first place. Without
+// this, MigrateFields hands the array through untouched and
+// ValidateFieldsDetailed — which iterates schema.Fields and therefore DOES see
+// a declared key — rejects it, turning a move that used to silently destroy the
+// notes into one that fails outright (Codex round 1 P2). Pinning the set here
+// rather than only at the server handler keeps the two lists honest.
+func TestReservedItemFieldKeysAreStableAndComplete(t *testing.T) {
+	got := models.ReservedItemFieldKeys()
+	want := []string{"convention", "decision_log", "github_pr", "implementation_notes"}
+	if !reflect.DeepEqual(got, want) {
+		t.Fatalf("reserved key set changed:\n got %#v\nwant %#v\n\nIf this is intentional, the new key must ALSO be added to the web's RESERVED_FIELD_KEYS (field-editor-types.ts) so the UI steers authors away before the server 400s.", got, want)
+	}
+	for _, k := range want {
+		if !models.IsReservedItemField(k) {
+			t.Errorf("IsReservedItemField(%q) = false, want true", k)
+		}
+	}
+	// Control: a key that merely LOOKS internal is not reserved. Without
+	// this, a helper returning true for everything passes.
+	for _, k := range []string{"status", "priority", "notes", "github", ""} {
+		if models.IsReservedItemField(k) {
+			t.Errorf("IsReservedItemField(%q) = true, want false", k)
+		}
 	}
 }

internal/models/item.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"sort"
 	"time"
 )
 
@@ -48,6 +49,19 @@ func IsReservedItemField(key string) bool {
 	return ok
 }
 
+// ReservedItemFieldKeys returns the reserved keys in a stable order, for
+// callers that need to enumerate rather than test membership (schema-key
+// validation, error messages). Sorted so the output is deterministic — an
+// error message that lists these must not reorder between runs.
+func ReservedItemFieldKeys() []string {
+	keys := make([]string, 0, len(reservedItemFieldKeys))
+	for k := range reservedItemFieldKeys {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	return keys
+}
+
 type Item struct {
 	ID             string     `json:"id"`
 	WorkspaceID    string     `json:"workspace_id"`

internal/server/handlers_collections.go

@@ -23,7 +23,19 @@ import (
 // true, which makes those sites silently skip fields-JSON extraction —
 // disabling subtask linking for the collection with no error anywhere
 // (TASK-1912, stage 1 of the IDEA-1746 consolidation plan).
-var reservedSchemaFieldKeys = []string{"parent", "plan"}
+// The system-metadata keys (implementation_notes, decision_log, github_pr,
+// convention) are reserved here for a DIFFERENT reason, added with BUG-2674.
+// They are written by Pad itself and deliberately live outside every schema —
+// MigrateFields now carries them across a move by identity rather than
+// schema-matching them. A schema that declares one of those keys re-creates
+// the collision the carry-through exists to avoid: the migrated array arrives
+// at ValidateFieldsDetailed, which now DOES see the key (it iterates
+// schema.Fields), and a `text` FieldDef rejects an array — turning a move that
+// previously destroyed the notes into one that fails outright. Forbidding the
+// declaration is the honest fix; coercing or silently skipping validation for
+// a key the schema genuinely declares would be guessing at which meaning the
+// author wanted.
+var reservedSchemaFieldKeys = append([]string{"parent", "plan"}, models.ReservedItemFieldKeys()...)
 
 // validateNoReservedFieldKeys rejects a schema that newly introduces a
 // field keyed "parent" or "plan". prevSchema is nil on collection create

internal/server/handlers_items_copy_preflight.go

@@ -204,6 +204,17 @@ type ItemCopyPreflightDestination struct {
 type ItemCopyPreflightFields struct {
 	// Carried are the fields the copy WOULD have, in destination-schema
 	// order.
+	//
+	// Since BUG-2674 this bucket can also contain RESERVED system metadata
+	// (implementation_notes, decision_log, github_pr, convention), appended
+	// after the schema-ordered entries. Those keys are declared by no schema
+	// anywhere — the copy carries them by identity — so a client must not
+	// assume every `carried` entry resolves to a destination FieldDef. They
+	// are distinguishable by `type: "system"` and carry a rendered `label`
+	// rather than an author-supplied one. Before the carry-through they
+	// appeared under `dropped`, which was accurate then; reporting them in
+	// neither bucket would have made the preflight claim "nothing carries
+	// over" for an item whose notes in fact survive.
 	Carried []ItemCopyPreflightCarried `json:"carried"`
 
 	// Dropped are values that will not survive the copy: schema fields
@@ -703,6 +714,31 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request)
 		})
 	}
 
+	// Reserved system metadata carries too (BUG-2674), and it is invisible to
+	// the loop above because that walks the DESTINATION SCHEMA and these keys
+	// are declared by no schema anywhere. Reporting them here keeps the
+	// preflight honest in both directions: before the carry-through they at
+	// least showed up under `dropped`, so omitting them now would turn an
+	// accurate "these will be lost" into a silent "nothing carries over" while
+	// the copy in fact retains them — a preflight that under-reports what it
+	// will do is the same defect class as the move that reported nothing.
+	//
+	// Appended after the schema walk, in the enumeration's stable order, so
+	// the destination-schema section of the response is untouched.
+	for _, key := range models.ReservedItemFieldKeys() {
+		val, present := final[key]
+		if !present {
+			continue
+		}
+		resp.Fields.Carried = append(resp.Fields.Carried, ItemCopyPreflightCarried{
+			Key:   key,
+			Label: reservedFieldLabel(key),
+			Type:  "system",
+			Value: val,
+			From:  "migrated",
+		})
+	}
+
 	// dropped — schema fields first, in SOURCE-schema order (MigrateFields
 	// ranges a map, so its Dropped slice arrives in nondeterministic
 	// order and must be re-sorted or repeated calls would differ).
@@ -1081,3 +1117,21 @@ func fieldDefByKey(defs []models.FieldDef, key string) (models.FieldDef, bool) {
 	}
 	return models.FieldDef{}, false
 }
+
+// reservedFieldLabel renders a human label for a reserved metadata key in the
+// copy preflight. These keys have no FieldDef and therefore no author-supplied
+// Label, so the alternative is showing the raw snake_case key in a dialog whose
+// every other row is titled prose.
+func reservedFieldLabel(key string) string {
+	switch key {
+	case models.ItemFieldImplementationNotes:
+		return "Implementation notes"
+	case models.ItemFieldDecisionLog:
+		return "Decision log"
+	case models.ItemFieldGitHubPR:
+		return "Linked pull request"
+	case models.ItemFieldConvention:
+		return "Convention metadata"
+	}
+	return key
+}

internal/server/handlers_items_copy_preflight_test.go

@@ -1965,3 +1965,72 @@ func TestCopyPreflight_PartialMarkerDoesNotLeakHiddenCount(t *testing.T) {
 		t.Fatalf("warning block byte length leaks the hidden count: %d vs %d", len(one), len(many))
 	}
 }
+
+// BUG-2674 / Codex round 1 P2. MigrateFields now carries reserved system
+// metadata across a collection or workspace change instead of dropping it. The
+// preflight builds `carried` by walking the DESTINATION SCHEMA, and these keys
+// are declared by no schema anywhere — so without an explicit pass they land in
+// neither bucket, and a copy of an item whose only content is notes reports
+// "nothing carries over" while in fact retaining them.
+//
+// The assertion is on the preflight PROMISE matching what the copy DOES, in
+// both directions: the key is reported carried, and it is genuinely present on
+// the copied item. Asserting only the report would pass if the copy silently
+// dropped it; asserting only persistence would pass if the report stayed
+// silent — which is the exact defect being closed.
+func TestCopyPreflight_ReportsReservedMetadataAsCarried(t *testing.T) {
+	f := newCopyPreflightFixture(t)
+
+	notes := `[{"id":"note-1","summary":"must be reported and must survive"}]`
+	seeded := `{"status":"open","priority":"low","impact":"large","count":7,"code":"abc",` +
+		`"implementation_notes":` + notes + `}`
+	updated, err := f.srv.store.UpdateItem(f.source.ID, models.ItemUpdate{Fields: &seeded})
+	if err != nil || updated == nil {
+		t.Fatalf("seed reserved metadata: %v", err)
+	}
+
+	body := f.resolvableBody()
+	body["field_overrides"] = map[string]any{"ticket": "T-1", "code": "123"}
+
+	pre := f.ok(body)
+
+	var found *ItemCopyPreflightCarried
+	for i := range pre.Fields.Carried {
+		if pre.Fields.Carried[i].Key == models.ItemFieldImplementationNotes {
+			found = &pre.Fields.Carried[i]
+			break
+		}
+	}
+	if found == nil {
+		var keys []string
+		for _, c := range pre.Fields.Carried {
+			keys = append(keys, c.Key)
+		}
+		t.Fatalf("implementation_notes missing from carried; got %v", keys)
+	}
+	if found.Type != "system" {
+		t.Errorf("carried type = %q, want %q so clients can tell it resolves to no FieldDef", found.Type, "system")
+	}
+	if found.Label == models.ItemFieldImplementationNotes {
+		t.Errorf("carried label is the raw key %q; reserved keys have no author-supplied label and must render a readable one", found.Label)
+	}
+
+	// It must not ALSO be reported dropped — the two buckets are exclusive
+	// and a key in both is worse than a key in neither.
+	for _, d := range pre.Fields.Dropped {
+		if d.Key == models.ItemFieldImplementationNotes {
+			t.Error("implementation_notes reported both carried and dropped")
+		}
+	}
+
+	// And the promise has to be true. assertPreflightMatchesCopy walks every
+	// `carried` entry and compares it against the COPY's persisted fields, so
+	// running it here is what turns the report into a checked claim rather
+	// than a label — it fails if the copy dropped what the preflight promised.
+	res := assertPreflightMatchesCopy(t, f, "reserved metadata carries", body)
+
+	got := f.persistedFields(res.Item.ID)
+	if _, ok := got[models.ItemFieldImplementationNotes]; !ok {
+		t.Fatalf("implementation_notes was reported carried but is absent on the copy: %#v", got)
+	}
+}

web/src/lib/components/collections/field-editor-types.ts

@@ -100,6 +100,17 @@ export const RESERVED_FIELD_KEYS: ReadonlySet<string> = new Set([
 	// them here too so the UI steers authors away before they hit the 400.
 	'parent',
 	'plan',
+	// BUG-2674: system-written metadata that deliberately lives OUTSIDE every
+	// schema. MigrateFields carries these across a collection move by identity
+	// rather than schema-matching them, so a schema that declares one puts a
+	// FieldDef in front of a value the migration hands through untouched —
+	// e.g. a `text` field receiving the notes array, which then fails
+	// validation and blocks the move. Server rejects newly-added occurrences;
+	// reserved here too so the UI steers authors away before the 400.
+	'implementation_notes',
+	'decision_log',
+	'github_pr',
+	'convention',
 	'created_by',
 	'last_modified_by',
 	'source',

web/src/lib/components/timeline/TimelineActivityCard.svelte

@@ -82,6 +82,20 @@
 			{/each}
 		</div>
 	{/if}
+	<!--
+		BUG-2674. A move can discard field values the destination schema has no
+		home for. The server has always computed that list and now records it in
+		the move's audit metadata; without this row it stayed invisible to the
+		one surface most likely to be asked "what happened to my item", which
+		would leave the loss silent in exactly the place the fix exists to make
+		it loud. Server joins the keys, so this renders a string, not an array.
+	-->
+	{#if activity.action === 'moved' && metadata.dropped_fields}
+		<div class="dropped">
+			<span class="dropped-label">Dropped on move:</span>
+			<span class="dropped-keys">{metadata.dropped_fields}</span>
+		</div>
+	{/if}
 </div>
 
 <style>
@@ -146,6 +160,27 @@
 		padding-left: var(--space-1);
 	}
 
+	.dropped {
+		display: flex;
+		flex-wrap: wrap;
+		align-items: baseline;
+		gap: 0.3em;
+		padding-left: var(--space-1);
+		font-size: 0.75em;
+		color: var(--text-muted);
+	}
+
+	.dropped-label {
+		font-weight: 600;
+	}
+
+	.dropped-keys {
+		/* Long key lists wrap rather than widening the card, which lives in
+		   a pane whose width is not the card's to negotiate. */
+		min-width: 0;
+		overflow-wrap: anywhere;
+	}
+
 	.change-pill {
 		display: inline-flex;
 		align-items: center;

web/src/lib/components/timeline/timelineActivityDroppedFields.svelte.test.ts

@@ -0,0 +1,77 @@
+import { describe, it, expect } from 'vitest';
+import { render } from '@testing-library/svelte';
+import TimelineActivityCard from './TimelineActivityCard.svelte';
+import type { Activity } from '$lib/types';
+
+// BUG-2674. A move can discard field values the destination schema has no home
+// for. The server has always computed that list (items.MigrateFields' Dropped)
+// and now records it in the move's audit metadata — but the report only means
+// anything if the surface a user actually reads displays it. Codex round 1
+// caught that the timeline renderer ignored the key, which would have left the
+// loss silent in the one place someone asks "what happened to my item".
+//
+// The negative legs are the load-bearing ones: a row that renders
+// unconditionally, or renders on every action, would pass a presence-only test.
+
+function activity(overrides: Partial<Activity> = {}): Activity {
+	return {
+		id: 'act-1',
+		workspace_id: 'ws-1',
+		item_id: 'item-1',
+		action: 'moved',
+		actor: 'user',
+		actor_name: 'Dave',
+		source: 'web',
+		metadata: JSON.stringify({
+			from_collection: 'tasks',
+			to_collection: 'ideas',
+			dropped_fields: 'due_date, effort'
+		}),
+		created_at: new Date('2026-08-19T12:00:00Z').toISOString(),
+		...overrides
+	} as Activity;
+}
+
+describe('TimelineActivityCard dropped-fields row', () => {
+	it('renders the dropped keys on a move that discarded values', () => {
+		const { getByText } = render(TimelineActivityCard, { activity: activity() });
+
+		expect(getByText('Dropped on move:')).toBeTruthy();
+		// The exact keys, not merely the label — a row that rendered a fixed
+		// string would satisfy a label-only assertion while telling the user
+		// nothing about WHICH fields went.
+		expect(getByText('due_date, effort')).toBeTruthy();
+	});
+
+	it('renders nothing when the move dropped no fields', () => {
+		const { queryByText } = render(TimelineActivityCard, {
+			activity: activity({
+				metadata: JSON.stringify({ from_collection: 'tasks', to_collection: 'ideas' })
+			})
+		});
+
+		expect(queryByText('Dropped on move:')).toBeNull();
+	});
+
+	it('renders nothing for a non-move action carrying the same key', () => {
+		// Guards against keying the row on the metadata alone. `updated`
+		// activities share the metadata blob shape, and a dropped_fields key
+		// arriving on one is not a move report.
+		const { queryByText } = render(TimelineActivityCard, {
+			activity: activity({
+				action: 'updated',
+				metadata: JSON.stringify({ dropped_fields: 'due_date, effort' })
+			})
+		});
+
+		expect(queryByText('Dropped on move:')).toBeNull();
+	});
+
+	it('survives unparseable metadata without throwing', () => {
+		const { queryByText } = render(TimelineActivityCard, {
+			activity: activity({ metadata: 'not json' })
+		});
+
+		expect(queryByText('Dropped on move:')).toBeNull();
+	});
+});