diff --git a/internal/server/handlers_artifact_import_warnings_test.go b/internal/server/handlers_artifact_import_warnings_test.go index e131d400..8db240ed 100644 --- a/internal/server/handlers_artifact_import_warnings_test.go +++ b/internal/server/handlers_artifact_import_warnings_test.go @@ -234,3 +234,79 @@ func TestImportArtifactCarriesUnresolvableRelationValue(t *testing.T) { t.Fatalf("import carried an unresolvable relation without saying so: %v", resp.Warnings) } } + +// An import carrying ONE unresolvable caller value must still clean up the +// destination schema's own broken defaults (codex round 15). +// +// The write path's step 2 returned early when the caller's values had issues — +// correct while every caller of that path REFUSED, since steps 3 and 4 only +// prepare a field map nobody stores. The carry posture made a caller that does +// not refuse, and the early return stayed: an artifact holding one junk +// relation skipped the late-default pass AND the default-visibility drop +// entirely, storing whatever the destination schema injected and reporting +// none of it. +// +// This fixture is the two existing artifact-relation tests composed, and the +// composition is the whole point — each half passes on its own against the +// unfixed build, and only together do they reach the skipped steps: +// +// - `role`, an unresolvable CALLER value, which is what fires the early +// return (TestImportArtifactCarriesUnresolvableRelationValue); +// - `owner_ref`, a broken destination DEFAULT that step 3 is what drops +// (TestImportArtifactReportsDroppedRelationDefault). +func TestImportArtifactCleansBrokenDefaultsDespiteACarriedJunkValue(t *testing.T) { + srv := testServer(t) + ws := createWSForTest(t, srv) + + rr := doRequest(srv, "PATCH", "/api/v1/workspaces/"+ws+"/collections/conventions", map[string]any{ + "schema": `{"fields":[{"key":"status","type":"select","options":["draft","active"]},{"key":"role","type":"relation","collection":"nobody"},{"key":"owner_ref","type":"relation","collection":"nobody","default":42}]}`, + }) + if rr.Code != http.StatusOK { + t.Fatalf("narrow conventions schema: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + + art := artifact.Artifact{ + Kind: artifact.KindConvention, + FormatVersion: artifact.FormatVersion, + Title: "Convention with junk AND a broken default", + Fields: map[string]any{"status": "active", "role": "just some text"}, + Body: "Body.\n", + } + data, err := artifact.Encode(art) + if err != nil { + t.Fatalf("encode: %v", err) + } + + rr2 := doArtifactRequest(srv, "POST", "/api/v1/workspaces/"+ws+"/import-artifact", data) + if rr2.Code != http.StatusCreated { + t.Fatalf("import REFUSED (%d); import carries, never refuses: %s", rr2.Code, rr2.Body.String()) + } + var resp struct { + Ref string `json:"ref"` + Warnings []string `json:"warnings"` + } + if err := json.Unmarshal(rr2.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v\n%s", err, rr2.Body.String()) + } + warnings := strings.Join(resp.Warnings, "\n") + + // CONTROL: the carried junk value still reaches the report. If this leg + // fails the fixture never fired the early return, and the assertion below + // would be measuring a path that was never skipped. + if !strings.Contains(warnings, "role") { + t.Fatalf("the carried junk value was not reported, so the early return this test "+ + "exists to reach was never taken: %v", resp.Warnings) + } + + show := doRequest(srv, "GET", "/api/v1/workspaces/"+ws+"/items/"+resp.Ref, nil) + if show.Code != http.StatusOK { + t.Fatalf("GET the imported item: %d %s", show.Code, show.Body.String()) + } + if strings.Contains(show.Body.String(), `"owner_ref":42`) { + t.Fatalf("a carried junk value made the import skip the late-default pass, and the "+ + "schema's broken default was stored raw: %s", show.Body.String()) + } + if !strings.Contains(warnings, "owner_ref") { + t.Fatalf("the broken destination default was neither cleaned nor reported: %v", resp.Warnings) + } +} diff --git a/internal/server/handlers_items.go b/internal/server/handlers_items.go index cbae06ae..f820dc4d 100644 --- a/internal/server/handlers_items.go +++ b/internal/server/handlers_items.go @@ -785,7 +785,7 @@ func (s *Server) createItemChecked(r *http.Request, workspaceID string, coll *mo // for one value, and after coercion so the value is in its final form. // The four steps live in one place — see resolveRelationsForWrite. relRefusals, droppedDefaults, relErr := s.resolveRelationsForWrite( - r, workspaceID, workspaceRole(r), schema, fieldMap, relBefore) + r, workspaceID, workspaceRole(r), schema, fieldMap, relBefore, posture) if relErr != nil { return nil, &itemCreateError{http.StatusInternalServerError, "internal_error", "Failed to resolve relation references"} } @@ -1267,7 +1267,7 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) { // Referent validation for relation values (TASK-2878) — the same four // steps the create door runs; see resolveRelationsForWrite. relRefusals, writeDropped, relErr := s.resolveRelationsForWrite( - r, workspaceID, workspaceRole(r), schema, fieldMap, relBefore) + r, workspaceID, workspaceRole(r), schema, fieldMap, relBefore, relationsRefuse) if relErr != nil { writeInternalError(w, relErr) return @@ -2469,6 +2469,10 @@ func (s *Server) handleMoveItem(w http.ResponseWriter, r *http.Request) { writeInternalError(w, lateErr) return } + if cerr := s.collapseInvisibleRelationIssues(r, workspaceID, workspaceRole(r), lateDropped); cerr != nil { + writeInternalError(w, cerr) + return + } // A REQUIRED relation whose default did not resolve cannot be left as a // drop: the key is deleted AFTER validation passed, so nothing re-checks // it and the item would land with a required field absent, reported valid diff --git a/internal/server/handlers_items_bulk.go b/internal/server/handlers_items_bulk.go index 67f43f5a..447fe85b 100644 --- a/internal/server/handlers_items_bulk.go +++ b/internal/server/handlers_items_bulk.go @@ -567,6 +567,9 @@ func (s *Server) bulkFieldUpdate(r *http.Request, workspaceID string, item *mode if lateErr != nil { return nil, &bulkOpError{message: "Failed to resolve relation references", code: "internal_error"} } + if cerr := s.collapseInvisibleRelationIssues(r, workspaceID, workspaceRole(r), lateDropped); cerr != nil { + return nil, &bulkOpError{message: "Failed to resolve relation references", code: "internal_error"} + } if req := store.RequiredRelationIssues(schema, lateDropped); len(req) > 0 { return nil, &bulkOpError{ message: "required fields missing: " + relationIssuesMessage(req), @@ -819,6 +822,9 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m if lateErr != nil { return nil, &bulkOpError{message: "Failed to resolve relation references", code: "internal_error"} } + if cerr := s.collapseInvisibleRelationIssues(r, workspaceID, workspaceRole(r), lateDropped); cerr != nil { + return nil, &bulkOpError{message: "Failed to resolve relation references", code: "internal_error"} + } // A REQUIRED relation whose default did not resolve cannot be left as a // drop: the key is deleted AFTER validation passed, so nothing re-checks // it and the item would land with a required field absent, reported valid diff --git a/internal/server/handlers_items_copy_preflight.go b/internal/server/handlers_items_copy_preflight.go index 0f843ca7..3f186a1c 100644 --- a/internal/server/handlers_items_copy_preflight.go +++ b/internal/server/handlers_items_copy_preflight.go @@ -805,6 +805,10 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request) writeInternalError(w, fmt.Errorf("copy preflight: resolve relation defaults: %w", lateErr)) return } + if cerr := s.collapseInvisibleRelationIssues(r, dst.WorkspaceID(), dst.Role, lateDropped); cerr != nil { + writeInternalError(w, fmt.Errorf("copy preflight: relation issue visibility: %w", cerr)) + return + } // A REQUIRED relation whose default did not resolve becomes a needs_value // row rather than a drop, so the preview says what the copy will do: // the copy REFUSES this request, and `valid` must be false (codex round @@ -819,7 +823,7 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request) } invisibleDefaults, invErr := s.dropInvisibleRelationDefaults(r, dst.WorkspaceID(), dst.Role, items.SchemaForMigratedFields(targetSchema), final, - notDefaultKeys(input.FieldOverrides, carriedSource)) + notDefaultKeys(input.FieldOverrides, carriedAfterRelationDrops(currentFields, migrated.Dropped, relDropped))) if invErr != nil { writeInternalError(w, fmt.Errorf("copy preflight: relation default visibility: %w", invErr)) return diff --git a/internal/server/relation_referents.go b/internal/server/relation_referents.go index bbdadddd..a5a184ca 100644 --- a/internal/server/relation_referents.go +++ b/internal/server/relation_referents.go @@ -197,6 +197,48 @@ func collapseIssue(issues []store.RelationIssue, key string, reason store.Relati } } +// collapseInvisibleRelationIssues rewrites `wrong_collection` to `not_found` +// on any issue whose target the requester cannot see — the same collapse +// resolveRelationReferentsAs applies to the MAIN pass, hoisted so the LATE +// pass gets it too. +// +// `store.ResolveLateRelationDefaults` is a store function and cannot know who +// is asking, so every issue it returns carries the raw reason. Those issues +// reach a caller: each door feeds them to RequiredRelationIssues and renders +// the result into a 400 or a preflight `needs_value` row. `wrong_collection` +// is the one reason that names a LIVE item, so an invisible target announced +// that way is the existence oracle round 3 closed, reopened through the door +// round 10 added (codex round 15). +// +// Reviewer named ONE site; this is applied at all five late-default sites, +// per CONVE-18 — the class is "a store-resolved issue reaching a caller +// without passing the visibility collapse", not the one call it was spotted at. +func (s *Server) collapseInvisibleRelationIssues(r *http.Request, workspaceID, role string, issues []store.RelationIssue) error { + for i := range issues { + if issues[i].Reason != store.RelationTargetWrongCollection { + continue + } + target, terr := s.store.ResolveRelationTarget(workspaceID, issues[i].Value) + if terr != nil { + return terr + } + if target == nil { + // Vanished between the two reads. The reason still SAYS the value + // named something a moment ago, which is the same disclosure. + issues[i].Reason = store.RelationTargetNotFound + continue + } + seen, verr := s.checkItemVisible(workspaceID, target, currentUser(r), role, isBearerAuth(r)) + if verr != nil { + return verr + } + if !seen { + issues[i].Reason = store.RelationTargetNotFound + } + } + return nil +} + // refuseRelationIssues writes the 400 a write door owes and reports whether it // did. One function so the six refusing doors cannot phrase the same refusal // three different ways, and so a client matching on the error code sees one @@ -279,6 +321,56 @@ func (s *Server) refuseInvisibleRelationOverrides( return s.resolveRelationReferentsAs(r, workspaceID, role, schema, probe) } +// carriedAfterRelationDrops is the carried-source set a migrate door hands +// notDefaultKeys, with BOTH drop passes subtracted. +// +// A door computes its carried set from MigrateFields' drops, then runs +// store.MigrateRelationReferents, which drops further keys whose referents did +// not resolve. Reusing the FIRST set for the visibility call classifies a key +// the second pass dropped as still carried; when the destination schema then +// refills that key with a default, the default is exempted from the visibility +// check on the strength of a value that is no longer there, and the response +// can hand back the id of an item the caller cannot see. +// +// The pre-relation-drop set is still the right input to the origin label and +// to the relation classifier itself, which are asking "did this value come +// across from the source?" — a different question from "is the value in hand +// now a destination default?" (codex round 15). +// +// ONLY the preflight needs this, and the reason is worth stating so nobody +// "fixes" the other doors to match. Move and bulk move fold their relation +// drops into `result.Dropped` and then recompute CarriedSourceValues INLINE at +// the visibility call, so they read the already-extended list and were never +// wrong. The preflight extends `migrated.Dropped` the same way but hands the +// visibility call a `carriedSource` VARIABLE captured before that append — +// the snapshot is the defect, not the door. +// +// SHIPS WITH ITS MUTANTS SURVIVING, AND THAT IS RECORDED RATHER THAN HIDDEN. +// Restoring the stale set — at the move door OR here — leaves every test +// green, because no configuration I could construct makes the difference +// observable: a destination default whose target the caller cannot see is +// already collapsed to `not_found` by the MAIN pass and dropped before this +// check is reached. Measured with a probe on both builds — the owner's +// preflight discloses the default's id, and a restricted editor's reports +// `owner_ref` dropped as `not_found` on the fixed AND the unfixed build. +// +// So this is a robustness change, not a bug fix, kept for one reason: +// `carriedSource` should mean what its name says at every use, rather than +// being correct at three uses and stale at the fourth because two later passes +// happen to repair it. A regression test WAS written for this and then +// DELETED, because it passed against the unfixed build — a test that cannot +// fail is worse than no test, since it reads as a guard while guarding +// nothing. +func carriedAfterRelationDrops(sourceFields map[string]any, migrateDropped []string, relDropped []store.RelationIssue) map[string]any { + if len(relDropped) == 0 { + return store.CarriedSourceValues(sourceFields, migrateDropped) + } + lost := make([]string, 0, len(migrateDropped)+len(relDropped)) + lost = append(lost, migrateDropped...) + lost = append(lost, store.RelationIssueKeys(relDropped)...) + return store.CarriedSourceValues(sourceFields, lost) +} + // notDefaultKeys is the set a migrate door hands // dropInvisibleRelationDefaults: the caller's own values plus the values // carried from the source item. Everything else in the map came from the @@ -412,21 +504,36 @@ func (s *Server) resolveRelationsForWrite( schema models.CollectionSchema, fieldMap map[string]any, presentBefore map[string]bool, + posture relationPosture, ) (refusals []store.RelationIssue, dropped []string, err error) { issues, err := s.resolveRelationReferents(r, workspaceID, schema, fieldMap) if err != nil { return nil, nil, err } - if callerIssues := store.IssuesForCallerInput(issues, presentBefore); len(callerIssues) > 0 { + callerIssues := store.IssuesForCallerInput(issues, presentBefore) + if len(callerIssues) > 0 && posture == relationsRefuse { + // The write is about to be REFUSED, so steps 3 and 4 would only + // prepare a field map nobody stores. return callerIssues, nil, nil } + // A CARRYING door does not stop here, and this is the round-15 defect: + // the early return was written when every caller of this path refused, and + // stayed when the artifact-import door began carrying instead. An import + // holding ONE unresolvable caller value would skip the late-default pass + // AND the default-visibility drop entirely, storing whatever the + // destination schema injected — including a default the caller cannot see + // — and reporting none of it. The refusals are still returned; they are + // the carry report, not a stop. lateDropped, err := s.store.ResolveLateRelationDefaults(workspaceID, schema, fieldMap, presentBefore) if err != nil { return nil, nil, err } + if cerr := s.collapseInvisibleRelationIssues(r, workspaceID, role, lateDropped); cerr != nil { + return nil, nil, cerr + } if required := store.RequiredRelationIssues(schema, lateDropped); len(required) > 0 { - return required, nil, nil + return append(callerIssues, required...), nil, nil } invisible, err := s.dropInvisibleRelationDefaults(r, workspaceID, role, schema, fieldMap, presentBefore) @@ -438,10 +545,14 @@ func (s *Server) resolveRelationsForWrite( // deleting a key after validation has passed leaves a required field // absent regardless of which list recorded it (codex round 13). if required := store.RequiredRelationIssues(schema, invisible); len(required) > 0 { - return required, nil, nil + return append(callerIssues, required...), nil, nil } for _, ri := range append(lateDropped, invisible...) { dropped = append(dropped, ri.Key) } - return nil, dropped, nil + // callerIssues is EMPTY on a refusing door — it returned above — and holds + // the carry report on a carrying one. Returning it here rather than `nil` + // is what keeps an import's unresolvable values named in the response now + // that the carrying door runs to the end. + return callerIssues, dropped, nil } diff --git a/internal/server/relation_referents_doors_test.go b/internal/server/relation_referents_doors_test.go index 92d7c43f..5895cf35 100644 --- a/internal/server/relation_referents_doors_test.go +++ b/internal/server/relation_referents_doors_test.go @@ -1087,3 +1087,70 @@ func TestRelationDoors_NullSourceDoesNotExemptDefaultFromVisibility(t *testing.T t.Fatalf("the hidden default was stored: %#v", v) } } + +// A relation issue raised by the LATE default pass must pass through the same +// visibility collapse the main pass applies (codex round 15). +// +// `store.ResolveLateRelationDefaults` cannot know who is asking, so it returns +// the raw reason. `wrong_collection` is the one reason that names a LIVE item, +// and every door renders these issues into a caller-visible message — so a +// schema default pointing at an item the caller cannot see announced that the +// item EXISTS. Same oracle round 3 closed on the main pass, reopened through +// the late-default door round 10 added. +func TestRelationDoors_LateDefaultWrongCollectionDoesNotDiscloseExistence(t *testing.T) { + f := newDoorFixture(t) + other := mustSchemaCollection(t, f.srv, f.ws.ID, "Late Vaults", `{"fields":[]}`) + secret, err := f.srv.store.CreateItem(f.ws.ID, other.ID, models.ItemCreate{ + Title: "Late Secret", CreatedBy: f.owner.ID, + }) + if err != nil { + t.Fatalf("CreateItem(secret): %v", err) + } + + // REQUIRED, so the unresolved default becomes a refusal the caller reads + // rather than a silent drop. + dst := mustSchemaCollection(t, f.srv, f.ws.ID, "Late Default Door", fmt.Sprintf(`{"fields":[ + {"key":"status","label":"Status","type":"select","options":["open","done"]}, + {"key":"owner_ref","label":"Owner","type":"relation","collection":%q,"required":true,"default":%q} + ]}`, f.people.Slug, secret.Ref)) + + move := func(u *models.User, role string) string { + t.Helper() + item := f.seed(`{"status":"open"}`) + rr := f.callAs(u, role, f.srv.handleMoveItem, "POST", + "/api/v1/workspaces/"+f.ws.Slug+"/items/"+item.Slug+"/move", + map[string]string{"itemSlug": item.Slug}, + map[string]any{"target_collection": dst.Slug}) + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for an unresolved REQUIRED relation default, got %d: %s", + rr.Code, rr.Body.String()) + } + return rr.Body.String() + } + + // CONTROL: a caller who CAN see the target keeps the specific, useful + // reason. Without this leg, collapsing every reason to not_found would + // pass the assertion below while destroying the message's value. + seeing := move(f.owner, "owner") + if !strings.Contains(seeing, "is not an item in collection") { + t.Fatalf("a caller who CAN see the target lost the wrong_collection reason: %s", seeing) + } + + blind := mustUser(t, f.srv, "blind-late@example.com", "blindlate", "") + if err := f.srv.store.AddWorkspaceMember(f.ws.ID, blind.ID, "editor"); err != nil { + t.Fatalf("AddWorkspaceMember: %v", err) + } + if err := f.srv.store.SetMemberCollectionAccess(f.ws.ID, blind.ID, "specific", + []string{f.tasks.ID, dst.ID, f.people.ID}); err != nil { + t.Fatalf("SetMemberCollectionAccess: %v", err) + } + + hidden := move(blind, "editor") + if strings.Contains(hidden, "is not an item in collection") { + t.Fatalf("the late-default refusal tells a caller who cannot see the target that it "+ + "EXISTS: %s", hidden) + } + if strings.Contains(hidden, secret.ID) { + t.Fatalf("the late-default refusal handed back the hidden item's canonical id: %s", hidden) + } +} diff --git a/internal/store/relation_referents.go b/internal/store/relation_referents.go index b2b7c713..a96093cd 100644 --- a/internal/store/relation_referents.go +++ b/internal/store/relation_referents.go @@ -745,6 +745,27 @@ func CarriedSourceValues(sourceFields map[string]any, dropped []string) map[stri return out } +// RelationIssueKeys is the key set of a relation-issue slice, for callers that +// need to subtract what the relation pass dropped from a set captured BEFORE +// it ran. +// +// The migrate doors capture their carried-source set from MigrateFields' +// drops, then run MigrateRelationReferents, which drops MORE keys. A set +// captured before the second pass and reused after it names keys that are no +// longer carried — and if the destination schema refills one with a default, +// that default is then treated as carried and skips the visibility check it +// needs (codex round 15). +func RelationIssueKeys(issues []RelationIssue) []string { + if len(issues) == 0 { + return nil + } + out := make([]string, 0, len(issues)) + for _, ri := range issues { + out = append(out, ri.Key) + } + return out +} + // hasKey reports whether m declares key. A nil map has no keys, which is how a // door with no overrides (bulk move) or no source item says so. func hasKey(m map[string]any, key string) bool {