From c4d429d14c94de941ac761333cdac0a33895a05f Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 4 Sep 2026 18:10:53 +0000 Subject: [PATCH] fix: three codex round-6 findings, one premise corrected (TASK-2878) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6 confirmed round 5 and found three. All three are real; one arrived with an account of its own cause that the test refuted, and the corrected route is narrower than the report. ## A NON-STRING RELATION DEFAULT, AND WHERE IT ACTUALLY GETS IN The finding: injected defaults are never type-checked, so `42` or `[]` can persist in a relation field. True, but not by the route described. `MigrateFields` injects destination defaults ITSELF, so in the ordinary case the key is present when `ValidateFieldsDetailed` runs and its type IS checked — a numeric default lands in needs_value with "must be a string", which is correct behaviour. My first test asserted the wrong thing and FAILED against the fixed build, which is how I found this out. The unchecked route is narrower: a NULL OVERRIDE deletes the key after MigrateFields filled it, so validation injects the default itself — and its own injection branch `continue`s PAST the type check. That is the one way a non-string reaches a relation field unchallenged. The late-default pass owns values that arrive from defaults, so it reports this one: `invalid_shape`, a new reason, because every existing reason describes a lookup that never happened. Retargeting the test then exposed a second defect IN MY OWN FIX: the late pass's `if len(late) == 0 { return nil, nil }` discarded the non-string drops it had just recorded. The value vanished from all three buckets. Green on the first route, silent on the second. THE PARITY GATE FROM AN EARLIER COMMIT CAUGHT THE NEW REASON: adding `invalid_shape` failed TestCopyPreflightDropReasonsAreRenderedByTheDialog until the TypeScript union and CopyItemDialog learned it. That gate exists because `referent_not_portable` shipped unrendered in BUG-2674, and it just did its job on its author. ## A WHITESPACE-ONLY VALUE IS "NO REFERENCE", NOT A BAD ONE The store resolver trims and ignores `" "`. The server wrapper checked the UNTRIMMED string, so the value fell through to the visibility loop — and since round 1's vanished-target arm turns a missing lookup into a refusal, `" "` came back as not_found instead of an empty field. A defect my own round-1 fix introduced: before it, that path did `continue`. ## A MALFORMED CARRIED VALUE IS NOT "NOT PORTABLE" The cross-workspace branch dropped every carried value without looking, including non-strings, and labelled them `referent_not_portable` — a false account of why the value is going. It is not a reference at all. Left in place now for ValidateFields to reject on shape, which is what the SAME-workspace branch already did with it: the two modes disagreeing about one malformed value was the defect. ## Counterfactuals Skip non-string defaults again -> TestCopyEndpoint_NonStringRelationDefault DETECTED. Restore the untrimmed skip -> TestRelationDoors_WhitespaceOnly DETECTED. Both build-checked, and the first form of the second mutant did NOT build (unused import) — reported as such rather than scored, since a non-compiling mutant produces no failures and reads as survived. Gates: internal/server ok 324.8s · internal/store ok 344.5s · internal/mcp ok 16.9s · go vet clean · gofmt clean · make lint 0 issues · npm run check 0 errors. Postgres green on the parent commit (store 596.1s, server 299.4s); re-running on this tree. --- .../handlers_items_copy_relation_test.go | 51 +++++++++++++++++++ internal/server/relation_referents.go | 8 ++- .../server/relation_referents_doors_test.go | 21 ++++++++ internal/store/relation_referents.go | 40 +++++++++++++-- .../components/items/CopyItemDialog.svelte | 2 + web/src/lib/types/index.ts | 8 ++- 6 files changed, 125 insertions(+), 5 deletions(-) diff --git a/internal/server/handlers_items_copy_relation_test.go b/internal/server/handlers_items_copy_relation_test.go index cd750e82..0ff60c77 100644 --- a/internal/server/handlers_items_copy_relation_test.go +++ b/internal/server/handlers_items_copy_relation_test.go @@ -672,3 +672,54 @@ func TestCopyEndpoint_NullSourceWithoutDefaultIsNotReportedAsDefault(t *testing. // claiming a default that does not exist. Reaching here means the key was // absent from `carried`, so there is nothing mislabelled. } + +// A relation default that is not a reference at all must be reported, not +// silently stored (codex round 6). +// +// THE ROUTE MATTERS, and the review's account of it was not quite right. +// `MigrateFields` injects destination defaults itself, so in the ordinary case +// the key is present when `ValidateFieldsDetailed` runs and its type IS +// checked — a numeric default lands in needs_value with "must be a string", +// which is correct behaviour and not a defect. +// +// The unchecked route is narrower: a NULL OVERRIDE deletes the key after +// MigrateFields has filled it, so validation injects the default itself — and +// its own injection branch `continue`s PAST the type check. That is the one +// way a non-string reaches a relation field unchallenged, and the +// late-default pass owns values that arrive from defaults. +func TestCopyEndpoint_NonStringRelationDefaultIsReported(t *testing.T) { + f := newCopyRelationFixtureNonStringDefault(t) + body := f.baseBody() + body["field_overrides"] = map[string]any{"owner_ref": nil} + + pre := f.ok(body) + reason, dropped := droppedReason(pre, "owner_ref") + if !dropped { + t.Fatalf("a non-reference default is neither carried nor dropped: %+v", pre.Fields) + } + if reason != "invalid_shape" { + t.Fatalf("dropped for reason %q, want invalid_shape — the value is not a reference, so "+ + "every other reason describes a lookup that never happened", reason) + } + + res := assertPreflightMatchesCopy(t, f.copyPreflightFixture, "non-string relation default", body) + if v, present := f.persistedFields(res.Item.ID)["owner_ref"]; present { + t.Fatalf("the copy stored a non-reference in a relation field: %#v", v) + } +} + +// newCopyRelationFixtureNonStringDefault gives the DESTINATION's relation +// field a numeric default, which no valid schema would declare and which +// nothing in the pipeline type-checks. +func newCopyRelationFixtureNonStringDefault(t *testing.T) *relationFixture { + t.Helper() + f := newCopyRelationFixtureWith(t, noDestDefault, nil, false) + schema := fmt.Sprintf(`{"fields":[ + {"key":"status","label":"Status","type":"select","options":["open","done"],"required":true}, + {"key":"owner_ref","label":"Owner","type":"relation","collection":%q,"default":42} + ]}`, f.targetsB.Slug) + if _, err := f.srv.store.UpdateCollection(f.collB.ID, models.CollectionUpdate{Schema: &schema}); err != nil { + t.Fatalf("UpdateCollection(non-string default): %v", err) + } + return f +} diff --git a/internal/server/relation_referents.go b/internal/server/relation_referents.go index 4949e3b4..cd135cb6 100644 --- a/internal/server/relation_referents.go +++ b/internal/server/relation_referents.go @@ -2,6 +2,7 @@ package server import ( "net/http" + "strings" "github.com/PerpetualSoftware/pad/internal/models" "github.com/PerpetualSoftware/pad/internal/store" @@ -98,7 +99,12 @@ func (s *Server) resolveRelationReferentsAs( continue } id, isStr := raw.(string) - if !isStr || id == "" { + // TRIMMED, matching the store resolver: it ignores a whitespace-only + // value as "no reference", so an untrimmed check here refuses a value + // the store never objected to — and since the vanished-target arm + // below turns a missing lookup into a refusal, `" "` became a + // not_found instead of an empty field (codex round 6). + if !isStr || strings.TrimSpace(id) == "" { continue } if ri, already := issueForKey(issues, def.Key); already { diff --git a/internal/server/relation_referents_doors_test.go b/internal/server/relation_referents_doors_test.go index 4398b86b..c69eed3b 100644 --- a/internal/server/relation_referents_doors_test.go +++ b/internal/server/relation_referents_doors_test.go @@ -721,3 +721,24 @@ func TestRelationDoors_BulkUpdateResolvesInjectedRelationDefault(t *testing.T) { v, ok, f.target.ID, f.target.Ref) } } + +// A whitespace-only relation value is "no reference", not a bad one (codex +// round 6). +// +// The store resolver trims and ignores it. The server wrapper checked the +// UNTRIMMED string, so it fell through to the visibility loop — and since the +// vanished-target arm turns a missing lookup into a refusal, `" "` came back +// as not_found instead of an empty field. A defect my own round-1 fix +// introduced: before it, the same path did `continue`. +func TestRelationDoors_WhitespaceOnlyRelationIsNotRefused(t *testing.T) { + f := newDoorFixture(t) + + rr := f.call(f.srv.handleCreateItem, "POST", + "/api/v1/workspaces/"+f.ws.Slug+"/collections/"+f.tasks.Slug+"/items", + map[string]string{"collSlug": f.tasks.Slug}, + map[string]any{"title": "Blank relation", "fields": map[string]any{"owner_ref": " "}}) + if rr.Code != http.StatusCreated { + t.Fatalf("a whitespace-only relation was refused %d; the store treats it as no value "+ + "at all: %s", rr.Code, rr.Body.String()) + } +} diff --git a/internal/store/relation_referents.go b/internal/store/relation_referents.go index cc34f5cb..f5607d94 100644 --- a/internal/store/relation_referents.go +++ b/internal/store/relation_referents.go @@ -48,6 +48,14 @@ const ( // so nothing can be checked against it. A schema problem, surfaced rather // than treated as permission to store anything. RelationTargetMissing RelationIssueReason = "target_missing" + // RelationTargetInvalidShape — the value is not a string at all, so it + // cannot name anything. Normally `ValidateFields` catches this and the + // resolver deliberately stays out of it (one error per defect), but an + // injected schema DEFAULT is never type-checked: ValidateFields assigns + // it and `continue`s past its own validation. That is the one route by + // which a non-string reaches a relation field unchallenged (codex round + // 6), so the late-default pass reports it rather than skipping it. + RelationTargetInvalidShape RelationIssueReason = "invalid_shape" ) // RelationIssue is one unresolvable relation value, carrying everything a @@ -68,6 +76,8 @@ func (ri RelationIssue) Message() string { return fmt.Sprintf("field %q: %q is not an item in collection %q", ri.Key, ri.Value, ri.Target) case RelationTargetMissing: return fmt.Sprintf("field %q declares no target collection, so %q cannot be resolved", ri.Key, ri.Value) + case RelationTargetInvalidShape: + return fmt.Sprintf("field %q has a default that is not a reference", ri.Key) default: return fmt.Sprintf("field %q: %q does not name an item in collection %q", ri.Key, ri.Value, ri.Target) } @@ -87,6 +97,7 @@ func RelationIssueReasons() []RelationIssueReason { RelationTargetWrongCollection, RelationTargetMissing, RelationTargetNotPortable, + RelationTargetInvalidShape, } } @@ -534,7 +545,16 @@ func (s *Store) MigrateRelationReferentsQ( if !exists { continue } - value, _ := raw.(string) + value, isStr := raw.(string) + if !isStr { + // Not a reference at all, so "cannot cross a workspace + // boundary" is a false account of why it is going. Left in + // place for ValidateFields to reject on shape — which is what + // the SAME-workspace branch already does with it, and the two + // modes disagreeing about one malformed value was the defect + // (codex round 6). + continue + } dropped = append(dropped, RelationIssue{ Key: def.Key, Value: value, Target: def.Collection, Reason: RelationTargetNotPortable, }) @@ -628,13 +648,27 @@ func (s *Store) ResolveLateRelationDefaultsQ( if !exists || raw == nil { continue } - if str, isStr := raw.(string); isStr && strings.TrimSpace(str) == "" { + str, isStr := raw.(string) + if !isStr { + // A default validation injected without type-checking it. The + // resolver cannot use it and nothing else will complain, so it is + // dropped and reported here. + dropped = append(dropped, RelationIssue{ + Key: def.Key, Target: def.Collection, Reason: RelationTargetInvalidShape, + }) + delete(fieldMap, def.Key) + continue + } + if strings.TrimSpace(str) == "" { continue } late[def.Key] = raw } if len(late) == 0 { - return nil, nil + // `dropped` may already carry non-string defaults rejected above, so + // this returns it rather than nil — an early `return nil, nil` here + // silently discarded them. + return dropped, nil } issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, late) if resolveErr != nil { diff --git a/web/src/lib/components/items/CopyItemDialog.svelte b/web/src/lib/components/items/CopyItemDialog.svelte index cf802b03..29283ca0 100644 --- a/web/src/lib/components/items/CopyItemDialog.svelte +++ b/web/src/lib/components/items/CopyItemDialog.svelte @@ -959,6 +959,8 @@ user hunting for an item that provably does not exist. return 'it refers to an item outside the field’s collection'; case 'target_missing': return 'the field declares no collection to link to'; + case 'invalid_shape': + return 'the destination field’s default is not a valid reference'; default: return reason; } diff --git a/web/src/lib/types/index.ts b/web/src/lib/types/index.ts index eab349bf..502cd181 100644 --- a/web/src/lib/types/index.ts +++ b/web/src/lib/types/index.ts @@ -799,7 +799,13 @@ export interface ItemCopyPreflightDropped { */ | 'not_found' | 'wrong_collection' - | 'target_missing'; + | 'target_missing' + /** + * The destination schema's default for this field is not a reference at + * all. Injected defaults are never type-checked, so this is the one + * route by which a non-string reaches a relation field. + */ + | 'invalid_shape'; } export interface ItemCopyPreflightNeedsValue {