diff --git a/internal/items/migrate.go b/internal/items/migrate.go index f6a7bf36..4706d081 100644 --- a/internal/items/migrate.go +++ b/internal/items/migrate.go @@ -17,14 +17,39 @@ type MigrateResult struct { Errors []string } +// MigrateScope says how far the item is travelling. It is a required argument +// rather than an option with a default because BOTH wrong answers lose +// something (BUG-2674): using SameWorkspace for a cross-workspace copy carries +// a github_pr into a workspace whose repository it does not describe, and using +// CrossWorkspace for an ordinary move DROPS that metadata from an item whose +// repo context never changed. A caller that has to name its scope cannot pick +// one by omission. +type MigrateScope int + +const ( + // SameWorkspace — a collection change within one workspace. The + // surrounding context (repository, members, conventions) is unchanged, + // so referential system metadata still describes something true. + SameWorkspace MigrateScope = iota + // CrossWorkspace — the item is landing in a different workspace, where + // a referent belonging to the source's context no longer holds. + CrossWorkspace +) + // MigrateFields maps field values from a source schema to a target schema. // Fields with matching keys and compatible types are transferred. // Incompatible or missing fields are dropped. Required target fields without // values after migration are reported as errors. +// +// Reserved system metadata (models.IsReservedItemField) bypasses schema +// matching entirely — it is declared by no schema, so matching it against one +// is what destroyed it before BUG-2674. Referential reserved keys additionally +// depend on scope; see MigrateScope. func MigrateFields( currentFields map[string]any, sourceSchema []models.FieldDef, targetSchema []models.FieldDef, + scope MigrateScope, ) MigrateResult { result := MigrateResult{ Fields: make(map[string]any), @@ -60,6 +85,15 @@ func MigrateFields( // them. There is no migration to attempt: they have no source or // target FieldDef to migrate BETWEEN. if models.IsReservedItemField(key) { + // Referential metadata travels only as far as its referent's + // context. Reported through the ordinary Dropped channel with + // no special casing — a user losing a PR link should learn it + // the same way they learn about any other dropped value + // (PLAN-2357 DR-17: "None of this may be silent"). + if scope == CrossWorkspace && models.IsReferentialItemField(key) { + result.Dropped = append(result.Dropped, key) + continue + } result.Fields[key] = value continue } diff --git a/internal/items/migrate_test.go b/internal/items/migrate_test.go index 528c877f..ad4b1d39 100644 --- a/internal/items/migrate_test.go +++ b/internal/items/migrate_test.go @@ -18,7 +18,7 @@ func TestMigrateFields_MatchingTypes(t *testing.T) { } fields := map[string]any{"status": "open", "priority": "high"} - result := MigrateFields(fields, source, target) + result := MigrateFields(fields, source, target, SameWorkspace) if result.Fields["status"] != "open" { t.Errorf("status: got %v, want 'open'", result.Fields["status"]) @@ -40,7 +40,7 @@ func TestMigrateFields_SelectValueNotInTarget(t *testing.T) { } fields := map[string]any{"status": "in-progress"} - result := MigrateFields(fields, source, target) + result := MigrateFields(fields, source, target, SameWorkspace) // "in-progress" is not in target options, should be dropped and default applied if result.Fields["status"] != "todo" { @@ -58,7 +58,7 @@ func TestMigrateFields_DropsExtraFields(t *testing.T) { } fields := map[string]any{"severity": "high", "browser": "Chrome"} - result := MigrateFields(fields, source, target) + result := MigrateFields(fields, source, target, SameWorkspace) if len(result.Dropped) != 2 { t.Errorf("dropped: got %d, want 2", len(result.Dropped)) @@ -76,7 +76,7 @@ func TestMigrateFields_TypeConversion(t *testing.T) { } fields := map[string]any{"count": 42, "status": "open"} - result := MigrateFields(fields, source, target) + result := MigrateFields(fields, source, target, SameWorkspace) if result.Fields["count"] != "42" { t.Errorf("count: got %v, want '42'", result.Fields["count"]) @@ -93,7 +93,7 @@ func TestMigrateFields_RequiredFieldMissing(t *testing.T) { } fields := map[string]any{} - result := MigrateFields(fields, source, target) + result := MigrateFields(fields, source, target, SameWorkspace) if len(result.Errors) != 1 { t.Errorf("errors: got %d, want 1", len(result.Errors)) @@ -107,7 +107,7 @@ func TestMigrateFields_DefaultApplied(t *testing.T) { } fields := map[string]any{} - result := MigrateFields(fields, source, target) + result := MigrateFields(fields, source, target, SameWorkspace) if result.Fields["status"] != "open" { t.Errorf("status: got %v, want 'open'", result.Fields["status"]) @@ -154,7 +154,7 @@ func TestMigrateFields_ReservedKeysCarryThroughUntouched(t *testing.T) { "severity": "high", // an ordinary key with no target home } - result := MigrateFields(fields, source, target) + result := MigrateFields(fields, source, target, SameWorkspace) for key, want := range map[string]any{ "implementation_notes": wantNotes, @@ -199,7 +199,7 @@ func TestMigrateFields_ReservedKeysBypassSchemaMatching(t *testing.T) { 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) + result := MigrateFields(map[string]any{"implementation_notes": notes}, source, target, SameWorkspace) got, ok := result.Fields["implementation_notes"] if !ok { @@ -236,3 +236,67 @@ func TestReservedItemFieldKeysAreStableAndComplete(t *testing.T) { } } } + +// The carry rule's own qualifier (BUG-2674, lead ruling): system-minted +// NON-REFERENTIAL data carries everywhere; REFERENTIAL system data carries only +// where its referent's context still holds. github_pr names a repository that +// belongs to the source workspace's context — carried into a different +// workspace it renders as a live PR link on an item whose project may have no +// relationship to that repo, which is a false statement rather than a preserved +// one. Notes and decisions describe the item's own history and are true wherever +// the item is. +// +// Both scopes are asserted in one test on purpose. The interesting property is +// the DIFFERENCE: a implementation that ignored scope entirely, in either +// direction, passes whichever half you write alone. +func TestMigrateFields_ReferentialKeysTravelOnlyWithinTheirContext(t *testing.T) { + source := []models.FieldDef{} + target := []models.FieldDef{} + + pr := map[string]any{"number": float64(42), "url": "https://example.invalid/42"} + notes := []any{map[string]any{"id": "note-1", "summary": "history is true anywhere"}} + build := func() map[string]any { + return map[string]any{"github_pr": pr, "implementation_notes": notes} + } + + t.Run("same workspace carries it", func(t *testing.T) { + result := MigrateFields(build(), source, target, SameWorkspace) + if _, ok := result.Fields["github_pr"]; !ok { + t.Error("github_pr must carry within one workspace — the repo context is unchanged") + } + for _, d := range result.Dropped { + if d == "github_pr" { + t.Error("github_pr must not be reported dropped within one workspace") + } + } + }) + + t.Run("cross workspace drops it AND reports it", func(t *testing.T) { + result := MigrateFields(build(), source, target, CrossWorkspace) + if _, ok := result.Fields["github_pr"]; ok { + t.Error("github_pr must not carry into another workspace — its referent's context is gone") + } + // Reported, not silently discarded. PLAN-2357 DR-17: "None of this + // may be silent." A drop with no report is the defect this whole + // unit exists to remove, and it would be perverse to reintroduce it + // in the fix's own new branch. + var reported bool + for _, d := range result.Dropped { + if d == "github_pr" { + reported = true + } + } + if !reported { + t.Errorf("github_pr dropped without a report; Dropped = %#v", result.Dropped) + } + }) + + t.Run("non-referential metadata is unaffected by scope", func(t *testing.T) { + for _, scope := range []MigrateScope{SameWorkspace, CrossWorkspace} { + result := MigrateFields(build(), source, target, scope) + if _, ok := result.Fields["implementation_notes"]; !ok { + t.Errorf("scope %v: implementation_notes must carry — it describes the item, not its surroundings", scope) + } + } + }) +} diff --git a/internal/models/item.go b/internal/models/item.go index e9339507..128dcf6b 100644 --- a/internal/models/item.go +++ b/internal/models/item.go @@ -49,6 +49,34 @@ func IsReservedItemField(key string) bool { return ok } +// referentialItemFieldKeys are the reserved keys whose VALUE points at +// something outside the item — a resource whose meaning depends on the +// surrounding workspace's context rather than on the item itself. +// +// The distinction decides how far they travel (BUG-2674, lead ruling). The +// carry rule is one sentence: system-minted NON-REFERENTIAL data carries; +// referential system data carries only where its referent's context still +// holds. implementation_notes and decision_log describe the item's own history +// and are true wherever the item is. github_pr names a repository that is a +// property of the SOURCE workspace's context — carried into a different +// workspace it renders as a live PR link on an item whose project may have no +// relationship to that repo, which is a false statement rather than a preserved +// one. +// +// So this is not an exception to the rule; it is the rule's own qualifier doing +// its job. A same-workspace move leaves the referent's context unchanged, so +// these carry there. +var referentialItemFieldKeys = map[string]struct{}{ + ItemFieldGitHubPR: {}, +} + +// IsReferentialItemField reports whether a reserved key's value depends on the +// workspace context around it. See referentialItemFieldKeys. +func IsReferentialItemField(key string) bool { + _, ok := referentialItemFieldKeys[key] + 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 diff --git a/internal/server/handlers_items.go b/internal/server/handlers_items.go index 0967afe8..037c5201 100644 --- a/internal/server/handlers_items.go +++ b/internal/server/handlers_items.go @@ -1908,7 +1908,12 @@ func (s *Server) handleMoveItem(w http.ResponseWriter, r *http.Request) { } // Migrate fields - result := items.MigrateFields(currentFields, sourceSchema.Fields, targetSchema.Fields) + // SameWorkspace is a property of the endpoint, not a guess: a move + // changes the item's COLLECTION and cannot change its workspace — the + // cross-workspace path is the copy endpoint. So the repo/member + // context around the item is unchanged and referential system + // metadata still describes something true (BUG-2674). + result := items.MigrateFields(currentFields, sourceSchema.Fields, targetSchema.Fields, items.SameWorkspace) // Apply overrides for k, v := range input.FieldOverrides { diff --git a/internal/server/handlers_items_bulk.go b/internal/server/handlers_items_bulk.go index 22bdf449..823a4c1d 100644 --- a/internal/server/handlers_items_bulk.go +++ b/internal/server/handlers_items_bulk.go @@ -602,7 +602,12 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m currentFields = make(map[string]any) } - result := items.MigrateFields(currentFields, sourceSchema.Fields, targetSchema.Fields) + // SameWorkspace is a property of the endpoint, not a guess: a move + // changes the item's COLLECTION and cannot change its workspace — the + // cross-workspace path is the copy endpoint. So the repo/member + // context around the item is unchanged and referential system + // metadata still describes something true (BUG-2674). + result := items.MigrateFields(currentFields, sourceSchema.Fields, targetSchema.Fields, items.SameWorkspace) if req.Status != "" { result.Fields["status"] = req.Status } diff --git a/internal/server/handlers_items_copy_preflight.go b/internal/server/handlers_items_copy_preflight.go index 1b2d5d73..473b696f 100644 --- a/internal/server/handlers_items_copy_preflight.go +++ b/internal/server/handlers_items_copy_preflight.go @@ -271,6 +271,22 @@ type ItemCopyPreflightDropped struct { // destination workspace (DR-8) // "agent_role_not_portable" — role slugs are workspace-local and // never carry (DR-8) + // "referent_not_portable" — system metadata whose VALUE points at + // something belonging to the SOURCE + // workspace's context, so it describes + // nothing true in the destination + // (BUG-2674). Today that is github_pr: + // the repository is a property of the + // source's project, and carrying it + // would render a live PR link on an + // item whose project may have no + // relationship to that repo. Distinct + // from no_target_field, which would + // otherwise be reported here and is + // simply wrong: no schema declares this + // key ANYWHERE, so "the destination has + // no such field" is true of the source + // too and explains nothing Reason string `json:"reason"` } @@ -589,7 +605,13 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request) // deliberately never read here; the authoritative answer comes from // items.ValidateFieldsDetailed run over the MERGED map, which also // applies destination defaults and type/option/pattern checks. - migrated := items.MigrateFields(currentFields, sourceSchema.Fields, targetSchema.Fields) + // Scope is COMPUTED, not assumed cross-workspace: this endpoint accepts a + // target_workspace equal to the source, and hardcoding CrossWorkspace + // would drop a github_pr from a duplicate whose repo context never + // changed (BUG-2674). Same computation in the mutating copy, or the two + // disagree — the divergence DR-6 exists to prevent. + migrated := items.MigrateFields(currentFields, sourceSchema.Fields, targetSchema.Fields, + migrateScopeFor(item.WorkspaceID, dst.WorkspaceID())) final := make(map[string]any, len(migrated.Fields)+len(input.FieldOverrides)) origin := make(map[string]string, len(final)) @@ -745,6 +767,17 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request) for _, key := range sortedDroppedKeys(migrated.Dropped, sourceSchema.Fields) { reason := "no_target_field" label := key + // A reserved key in Dropped can only have got there one way: it is + // referential and this is a cross-workspace copy. The generic + // no_target_field would be actively misleading — no schema declares + // these keys anywhere, so it is equally true of the source and + // explains nothing about why the value is being left behind. + if models.IsReservedItemField(key) { + resp.Fields.Dropped = append(resp.Fields.Dropped, ItemCopyPreflightDropped{ + Key: key, Label: reservedFieldLabel(key), Kind: "field", Reason: "referent_not_portable", + }) + continue + } srcDef, declaredBySource := fieldDefByKey(sourceSchema.Fields, key) if def, exists := targetDefs[key]; exists { // The key exists downstream, so migration rejected the VALUE. @@ -1135,3 +1168,13 @@ func reservedFieldLabel(key string) string { } return key } + +// migrateScopeFor answers the one question items.MigrateScope asks: is the +// item landing in a different workspace? Shared by the preflight and the +// mutating copy so the preview cannot promise a carry the copy then drops. +func migrateScopeFor(sourceWorkspaceID, targetWorkspaceID string) items.MigrateScope { + if sourceWorkspaceID == targetWorkspaceID { + return items.SameWorkspace + } + return items.CrossWorkspace +} diff --git a/internal/server/handlers_items_copy_preflight_test.go b/internal/server/handlers_items_copy_preflight_test.go index 74377518..af17b594 100644 --- a/internal/server/handlers_items_copy_preflight_test.go +++ b/internal/server/handlers_items_copy_preflight_test.go @@ -2034,3 +2034,60 @@ func TestCopyPreflight_ReportsReservedMetadataAsCarried(t *testing.T) { t.Fatalf("implementation_notes was reported carried but is absent on the copy: %#v", got) } } + +// BUG-2674, lead ruling. github_pr is REFERENTIAL system metadata: it names a +// repository belonging to the source workspace's context. It carries on an +// intra-workspace move (context unchanged) and drops on a cross-workspace copy, +// where carrying it would render a live PR link on an item whose project may +// have no relationship to that repo. +// +// The drop must be REPORTED — PLAN-2357 DR-17, "None of this may be silent" — +// and with a reason that explains itself. The generic no_target_field would be +// misleading here: no schema declares this key anywhere, so it is equally true +// of the source and says nothing about why the value is being left behind. +func TestCopyPreflight_ReportsReferentialMetadataAsNotPortable(t *testing.T) { + f := newCopyPreflightFixture(t) + + seeded := `{"status":"open","priority":"low","impact":"large","count":7,"code":"abc",` + + `"github_pr":{"number":42,"url":"https://example.invalid/42","state":"OPEN"},` + + `"implementation_notes":[{"id":"note-1","summary":"travels anywhere"}]}` + if _, err := f.srv.store.UpdateItem(f.source.ID, models.ItemUpdate{Fields: &seeded}); err != nil { + t.Fatalf("seed referential metadata: %v", err) + } + + body := f.resolvableBody() + body["field_overrides"] = map[string]any{"ticket": "T-1", "code": "123"} + pre := f.ok(body) + + var dropped *ItemCopyPreflightDropped + for i := range pre.Fields.Dropped { + if pre.Fields.Dropped[i].Key == models.ItemFieldGitHubPR { + dropped = &pre.Fields.Dropped[i] + break + } + } + if dropped == nil { + t.Fatalf("github_pr must be reported dropped on a cross-workspace copy; dropped = %+v", pre.Fields.Dropped) + } + if dropped.Reason != "referent_not_portable" { + t.Errorf("reason = %q, want %q — no_target_field is true of the source too and explains nothing", + dropped.Reason, "referent_not_portable") + } + + // It must not also be reported carried, and the non-referential sibling + // must NOT be swept up with it — that pair is the whole distinction. + for _, c := range pre.Fields.Carried { + if c.Key == models.ItemFieldGitHubPR { + t.Error("github_pr reported both dropped and carried") + } + } + var notesCarried bool + for _, c := range pre.Fields.Carried { + if c.Key == models.ItemFieldImplementationNotes { + notesCarried = true + } + } + if !notesCarried { + t.Error("implementation_notes must still carry across workspaces — it describes the item, not its surroundings") + } +} diff --git a/internal/store/attachments_copy_plan_test.go b/internal/store/attachments_copy_plan_test.go index 3ab12f98..e5dd500f 100644 --- a/internal/store/attachments_copy_plan_test.go +++ b/internal/store/attachments_copy_plan_test.go @@ -309,7 +309,7 @@ func TestPlanAttachmentCopy_DroppedFieldNotCloned(t *testing.T) { "screenshot": "pad-attachment:" + dropped.ID, } - migrated := items.MigrateFields(rawFields, sourceSchema, targetSchema) + migrated := items.MigrateFields(rawFields, sourceSchema, targetSchema, items.SameWorkspace) if len(migrated.Dropped) != 1 || migrated.Dropped[0] != "screenshot" { t.Fatalf("precondition: MigrateFields dropped %v, want [screenshot]", migrated.Dropped) } diff --git a/internal/store/items_cross_workspace_copy.go b/internal/store/items_cross_workspace_copy.go index 50a4498b..12a6aac1 100644 --- a/internal/store/items_cross_workspace_copy.go +++ b/internal/store/items_cross_workspace_copy.go @@ -602,7 +602,17 @@ func (s *Store) copyItemAcrossWorkspacesTx(req CrossWorkspaceCopyRequest, source // malformed_override from its own preview. A bad request is a bad request // whether or not the destination happens to be full, and a client told // "you are out of room" cannot fix an override it was never told about. - finalFields, dropped, err := migrateCopyFields(source.Fields, sourceColl.Schema, targetColl.Schema, req.FieldOverrides) + // Scope is COMPUTED from the two workspace ids rather than assumed + // cross-workspace: this path also serves a copy whose target IS the source + // workspace, and hardcoding CrossWorkspace would drop a github_pr from a + // duplicate whose repo context never changed (BUG-2674). The preflight + // computes it the same way — a divergence here would have the preview + // promising a carry the copy drops, which DR-6 exists to prevent. + scope := items.SameWorkspace + if sourceWorkspaceID != req.TargetWorkspaceID { + scope = items.CrossWorkspace + } + finalFields, dropped, err := migrateCopyFields(source.Fields, sourceColl.Schema, targetColl.Schema, req.FieldOverrides, scope) if err != nil { return nil, err } @@ -1006,7 +1016,7 @@ func (s *Store) getCollectionInWorkspaceTx(tx *sql.Tx, collectionID, workspaceID // // Returns the final field map (the planner's input, pre-rewrite) and the keys // migration dropped. -func migrateCopyFields(sourceFieldsJSON, sourceSchemaJSON, targetSchemaJSON string, overrides map[string]any) (map[string]any, []string, error) { +func migrateCopyFields(sourceFieldsJSON, sourceSchemaJSON, targetSchemaJSON string, overrides map[string]any, scope items.MigrateScope) (map[string]any, []string, error) { var sourceSchema, targetSchema models.CollectionSchema if err := json.Unmarshal([]byte(sourceSchemaJSON), &sourceSchema); err != nil { return nil, nil, fmt.Errorf("copy item across workspaces: parse source schema: %w", err) @@ -1032,7 +1042,7 @@ func migrateCopyFields(sourceFieldsJSON, sourceSchemaJSON, targetSchemaJSON stri } } - migrated := items.MigrateFields(currentFields, sourceSchema.Fields, targetSchema.Fields) + migrated := items.MigrateFields(currentFields, sourceSchema.Fields, targetSchema.Fields, scope) for k, v := range overrides { if v == nil { // An explicit null means "leave this unset". DELETE rather than