mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 15:05:40 +00:00
fix: two codex round-17 findings in internal/store; two recorded, not defects (TASK-2878)
Round 17 (internal/store) returned four findings. Two are real and pinned with
killed, build-checked mutants. Two are recorded with the measurement that says
they are not defects, so round 18 stops re-raising them.
1. A MALFORMED DESTINATION DEFAULT REFUSED OR DROPPED DEPENDING ON AN UNRELATED
REQUEST DETAIL (real, fixed, pinned).
MigrateRelationReferents skipped non-string values on the general rule that
shape is ValidateFields's to reject, so one defect makes one error. That
rule is right for a SUPPLIED or CARRIED value and wrong for a DESTINATION
DEFAULT, because the two disagree about the outcome: ValidateFields REFUSES
the request, and a default is not the caller's assertion, so this unit's
posture for it is drop-and-report.
The observable is worse than the inconsistency. A default MigrateFields
injects is in the map before validation and was REFUSED — 400, the whole
copy blocked, over an OPTIONAL field. The identical default that
ValidateFields injects, which is what a `{"owner_ref": null}` override
causes, reached the late pass and was DROPPED, and the copy completed. Same
malformed schema, opposite answers, chosen by a request detail with nothing
to do with it.
Non-string defaults are now dropped as `invalid_shape` in the early pass,
exactly as ResolveLateRelationDefaultsQ already dropped them, so the two
paths agree BY CONSTRUCTION. Supplied and carried values are untouched: the
comment's reasoning holds there and only there.
The existing non-string-default test sends the null override and so only
ever exercised the forgiving path. The new test runs BOTH legs.
Mutant: remove the drop -> FAIL on the `no override` leg only, `explicit
null override` still passing — which is the same fact stated twice: the old
test could not have caught this.
2. THE ID REMAP CORRUPTED AN UNRESOLVABLE CARRIED VALUE (real, fixed, pinned).
remapFieldIDs rewrote relation values with strings.ReplaceAll over the raw
JSON for every id in the bundle, so a value that merely CONTAINED another id
was partially rewritten: with ids `old-color-1` and a relation value
`"old-color-10"`, the import stored `<new-id>0` — a string that references
nothing and existed on neither side. Bundle ids are whatever the exporting
instance had and an import accepts a caller-supplied file, so this is not
confined to well-formed UUIDs. The same substitution also rewrote ids
appearing inside ordinary text values, which was never the intent.
It matters here because of the carry posture: an unresolvable relation value
is deliberately imported VERBATIM rather than dropped, and verbatim is the
whole promise.
Now a JSON walk matching WHOLE values, recursing into arrays so a
multi-valued relation is covered. An unparseable blob is returned untouched
rather than guessed at.
WORTH RECORDING ABOUT HOW THIS WAS MISSED: my own fixture in
TestImportWorkspace_CarriesUnresolvableRelationValues carries a comment
explaining that its ids "deliberately share no common prefix" because
otherwise ReplaceAll "would partially rewrite" the value. I identified the
mechanism exactly, engineered the fixture AROUND it, and never asked whether
the product had the defect the fixture was dodging. A hazard worth designing
around is a hazard worth filing.
Mutant: restore the ReplaceAll form -> FAIL, the dangling value comes back
rewritten. Control leg included: an EXACT-match value must still remap to
the imported item's new id, so the test fails against a build that stopped
remapping.
3. A CARRIED DROP IS NOT REPORTED WHEN A DESTINATION DEFAULT REFILLS THE KEY
(recorded, not changed). Measured: `fields.dropped` and
`warnings.dropped_fields` are both empty, and the preflight's carried row
says `"from":"default"` — so the preflight DOES tell the reader the value
came from the destination rather than the source, while the copy's
dropped_fields does not. This is round 3's deliberate decision (reporting a
key as dropped while it is populated made three surfaces give two answers),
the two doors agree, and changing it is a response-CONTRACT change — a new
bucket distinguishing "replaced by a default" from "dropped" — not a bug
fix. Left for a ruling rather than taken unilaterally.
4. AN UNRESOLVABLE STRING DEFAULT IS RESOLVED TWICE (recorded, not a defect).
The claim is true and has no observable: probed, the preflight reports
exactly one `dropped` row and the copy exactly one `dropped_fields` entry,
not two. The cost is one redundant lookup for a default that is already
broken. The READ COMMITTED divergence the reviewer raises is the same
target-vanishes-mid-request race already recorded as deliberately untested,
and it resolves the same way — the value is dropped either way.
Gates on this tip: build, go vet, gofmt clean; the store and server relation
suites green. The full SQLite suite and Postgres were green on 8f3f7f57 and are
OWED AGAIN on this tip; they are not claimed here.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
This commit is contained in:
@@ -848,3 +848,60 @@ func TestCopyPreflight_RelationDroppedCarriedKeyDoesNotExemptTheDefault(t *testi
|
||||
v, carried, f.targetB.ID)
|
||||
}
|
||||
}
|
||||
|
||||
// A malformed destination default gets the SAME answer whether or not the
|
||||
// caller sent an unrelated null override (codex round 17).
|
||||
//
|
||||
// `MigrateRelationReferents` skipped non-string values on the general rule
|
||||
// that shape is ValidateFields's to reject, so one defect makes one error.
|
||||
// That rule is right for a supplied or carried value and wrong for a
|
||||
// DESTINATION DEFAULT, because the two disagree about the outcome:
|
||||
// ValidateFields REFUSES, and a default is not the caller's assertion, so
|
||||
// this unit's posture is drop-and-report.
|
||||
//
|
||||
// The observable was worse than the inconsistency. A default MigrateFields
|
||||
// injects sits in the map before validation and was REFUSED; the identical
|
||||
// default that ValidateFields injects — which is what a `{"owner_ref": null}`
|
||||
// override causes — reached the late pass and was DROPPED. So the same
|
||||
// malformed schema either blocked the copy outright or dropped an optional
|
||||
// field, chosen by a request detail with nothing to do with it. The existing
|
||||
// non-string-default test sends the override and therefore only ever
|
||||
// exercised the forgiving path.
|
||||
func TestCopyEndpoint_NonStringDestinationDefaultDropsWithOrWithoutANullOverride(t *testing.T) {
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
override bool
|
||||
}{
|
||||
{"no override", false},
|
||||
{"explicit null override", true},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
f := newCopyRelationFixtureNonStringDefault(t)
|
||||
body := f.baseBody()
|
||||
if tc.override {
|
||||
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)
|
||||
}
|
||||
if !pre.Valid {
|
||||
t.Fatalf("the preflight refuses over a malformed default in an OPTIONAL "+
|
||||
"field; a default is not the caller's assertion and drops: %+v", pre.Fields)
|
||||
}
|
||||
|
||||
res := assertPreflightMatchesCopy(t, f.copyPreflightFixture,
|
||||
"non-string relation default ("+tc.name+")", 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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4,7 +4,6 @@ import (
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"strings"
|
||||
"time"
|
||||
"unicode/utf8"
|
||||
|
||||
@@ -850,11 +849,61 @@ func remapFieldIDs(fieldsJSON string, itemMap, collMap map[string]string) string
|
||||
if fieldsJSON == "" {
|
||||
return "{}"
|
||||
}
|
||||
result := fieldsJSON
|
||||
for oldID, newID := range itemMap {
|
||||
if oldID != "" && newID != "" {
|
||||
result = strings.ReplaceAll(result, oldID, newID)
|
||||
}
|
||||
// WHOLE-VALUE matches only, never a substring (codex round 17).
|
||||
//
|
||||
// This was `strings.ReplaceAll` over the raw JSON text for every id in the
|
||||
// bundle, which corrupts a value that merely CONTAINS one: with a bundle
|
||||
// carrying ids `c-1` and a relation value `"c-10"`, the value came out as
|
||||
// `<new-id-for-c-1>0` — a string that references nothing and that the
|
||||
// importer then stored as though it were the user's data. Bundle ids are
|
||||
// whatever the exporting instance had and an import accepts a
|
||||
// caller-supplied file, so this is not confined to well-formed UUIDs.
|
||||
//
|
||||
// It matters more since the carry posture: an unresolvable relation value
|
||||
// is deliberately IMPORTED VERBATIM rather than dropped, and "verbatim" is
|
||||
// the whole promise. A partial rewrite breaks it silently and produces a
|
||||
// value that never existed on either side.
|
||||
//
|
||||
// The same text substitution also rewrote ids appearing inside ordinary
|
||||
// text values, which was never the intent — the remap exists to repoint
|
||||
// RELATIONS at their clones. Whole-value matching gets both.
|
||||
//
|
||||
// I engineered a fixture AROUND this hazard in the import carry test
|
||||
// rather than asking whether the product had it. It did.
|
||||
var decoded map[string]any
|
||||
if err := json.Unmarshal([]byte(fieldsJSON), &decoded); err != nil {
|
||||
// Not an object we can walk. Left exactly as-is rather than
|
||||
// substring-rewritten: an unparseable blob is not a thing to guess at.
|
||||
return fieldsJSON
|
||||
}
|
||||
for k, v := range decoded {
|
||||
decoded[k] = remapFieldValue(v, itemMap)
|
||||
}
|
||||
out, err := json.Marshal(decoded)
|
||||
if err != nil {
|
||||
return fieldsJSON
|
||||
}
|
||||
return string(out)
|
||||
}
|
||||
|
||||
// remapFieldValue rewrites a value that IS an old item id, and recurses into
|
||||
// arrays so a multi-valued relation is covered. Anything else — a number, a
|
||||
// bool, a nested object, a string that merely contains an id — is returned
|
||||
// unchanged.
|
||||
func remapFieldValue(v any, itemMap map[string]string) any {
|
||||
switch t := v.(type) {
|
||||
case string:
|
||||
if newID, ok := itemMap[t]; ok && t != "" && newID != "" {
|
||||
return newID
|
||||
}
|
||||
return t
|
||||
case []any:
|
||||
out := make([]any, len(t))
|
||||
for i, e := range t {
|
||||
out[i] = remapFieldValue(e, itemMap)
|
||||
}
|
||||
return out
|
||||
default:
|
||||
return v
|
||||
}
|
||||
return result
|
||||
}
|
||||
|
||||
@@ -521,6 +521,42 @@ func (s *Store) MigrateRelationReferentsQ(
|
||||
// Destination defaults resolve against the DESTINATION in both modes — the
|
||||
// destination picked the value, so the mode says nothing about it.
|
||||
if defaults := byOrigin[RelationOriginDestinationDefault]; len(defaults) > 0 {
|
||||
// A NON-STRING default is dropped and reported here, exactly as
|
||||
// ResolveLateRelationDefaultsQ drops one, and NOT left for
|
||||
// ValidateFields (codex round 17).
|
||||
//
|
||||
// The general rule a few lines up — shape is ValidateFields's to
|
||||
// reject, so one defect produces one error — is right for a SUPPLIED
|
||||
// or CARRIED value and wrong for a default, because the two disagree
|
||||
// about what should happen: ValidateFields REFUSES the request, and a
|
||||
// destination default is not the caller's assertion, so this unit's
|
||||
// posture is drop-and-report.
|
||||
//
|
||||
// Leaving it produced an outcome that depended on WHEN the default
|
||||
// arrived. A default MigrateFields injects is in the map before
|
||||
// validation and was refused; the identical default that
|
||||
// ValidateFields injects — which is what happens when the caller
|
||||
// sends an unrelated `{"key": null}` override — reached the late pass
|
||||
// and was dropped. Same malformed schema, opposite answers, selected
|
||||
// by a request detail with nothing to do with it. Dropping here makes
|
||||
// the two paths agree BY CONSTRUCTION rather than by argument.
|
||||
for _, def := range schema.Fields {
|
||||
if def.Type != "relation" {
|
||||
continue
|
||||
}
|
||||
raw, exists := defaults[def.Key]
|
||||
if !exists {
|
||||
continue
|
||||
}
|
||||
if _, isStr := raw.(string); isStr {
|
||||
continue
|
||||
}
|
||||
dropped = append(dropped, RelationIssue{
|
||||
Key: def.Key, Target: def.Collection, Reason: RelationTargetInvalidShape,
|
||||
})
|
||||
delete(fieldMap, def.Key)
|
||||
delete(defaults, def.Key)
|
||||
}
|
||||
issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, defaults)
|
||||
if resolveErr != nil {
|
||||
return nil, nil, resolveErr
|
||||
|
||||
@@ -572,3 +572,104 @@ func TestImportWorkspace_CarriesUnresolvableRelationValues(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// An unresolvable carried relation value survives the ID remap VERBATIM, even
|
||||
// when it contains a resolvable id as a substring (codex round 17).
|
||||
//
|
||||
// `remapFieldIDs` rewrote relation values with strings.ReplaceAll over the raw
|
||||
// JSON for every id in the bundle, so a value that merely CONTAINED another
|
||||
// id was partially rewritten into a string that references nothing and existed
|
||||
// on neither side. The carry posture makes this load-bearing: an unresolvable
|
||||
// value is deliberately imported verbatim rather than dropped, and a partial
|
||||
// rewrite breaks that promise silently.
|
||||
//
|
||||
// THE PREFIX RELATIONSHIP IS THE FIXTURE, not an accident of naming. The
|
||||
// sibling test above deliberately chose ids sharing no prefix so its own
|
||||
// assertion could not fail for this reason — which is how the defect went
|
||||
// unexamined: the fixture was engineered around it instead of at it.
|
||||
func TestImportWorkspace_UnresolvableRelationValueSurvivesAPrefixCollision(t *testing.T) {
|
||||
s := testStore(t)
|
||||
owner, err := s.CreateUser(models.UserCreate{
|
||||
Name: "Owner", Email: "prefix-collision-owner@example.com", Password: "passw0rd!",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("CreateUser: %v", err)
|
||||
}
|
||||
|
||||
const (
|
||||
liveColorID = "old-color-1" // a real item in the bundle; gets remapped
|
||||
danglingID = "old-color-10" // NOT an item; contains liveColorID as a prefix
|
||||
)
|
||||
|
||||
export := &models.WorkspaceExport{
|
||||
Version: 1,
|
||||
ExportedAt: "2026-09-05T00:00:00Z",
|
||||
Workspace: models.WorkspaceExportMeta{Name: "Prefix Archive", Slug: "prefix-archive"},
|
||||
Collections: []models.CollectionExport{
|
||||
{
|
||||
ID: "old-coll-colors", Name: "Colors", Slug: "colors", Prefix: "COLO",
|
||||
Schema: `{"fields":[{"key":"status","type":"select","options":["open","done"],"default":"open","required":true}]}`,
|
||||
CreatedAt: "2026-09-05T00:00:00Z", UpdatedAt: "2026-09-05T00:00:00Z",
|
||||
},
|
||||
{
|
||||
ID: "old-coll-cars", Name: "Cars", Slug: "cars", Prefix: "CAR",
|
||||
Schema: `{"fields":[{"key":"status","type":"select","options":["open","done"],"default":"open","required":true},{"key":"color","type":"relation","collection":"colors"}]}`,
|
||||
CreatedAt: "2026-09-05T00:00:00Z", UpdatedAt: "2026-09-05T00:00:00Z",
|
||||
},
|
||||
},
|
||||
Items: []models.ItemExport{
|
||||
{
|
||||
ID: liveColorID, CollectionID: "old-coll-colors",
|
||||
Title: "Red", Slug: "red", Fields: `{}`, Tags: `[]`,
|
||||
CreatedAt: "2026-09-05T00:00:00Z", UpdatedAt: "2026-09-05T00:00:00Z",
|
||||
},
|
||||
{
|
||||
ID: "old-car-dangling", CollectionID: "old-coll-cars",
|
||||
Title: "Dangling", Slug: "dangling",
|
||||
Fields: `{"color":"` + danglingID + `"}`, Tags: `[]`,
|
||||
CreatedAt: "2026-09-05T00:00:00Z", UpdatedAt: "2026-09-05T00:00:00Z",
|
||||
},
|
||||
{
|
||||
// CONTROL: an EXACT match still gets remapped to the new id.
|
||||
// Without this leg the test passes against a build that
|
||||
// stopped remapping altogether.
|
||||
ID: "old-car-resolvable", CollectionID: "old-coll-cars",
|
||||
Title: "Resolvable", Slug: "resolvable",
|
||||
Fields: `{"color":"` + liveColorID + `"}`, Tags: `[]`,
|
||||
CreatedAt: "2026-09-05T00:00:00Z", UpdatedAt: "2026-09-05T00:00:00Z",
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
ws, err := s.ImportWorkspace(export, "Prefix Archive Target", owner.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("ImportWorkspace: %v", err)
|
||||
}
|
||||
|
||||
byTitle := map[string]string{}
|
||||
items, err := s.ListItems(ws.ID, models.ItemListParams{})
|
||||
if err != nil {
|
||||
t.Fatalf("ListItems: %v", err)
|
||||
}
|
||||
for _, it := range items {
|
||||
var m map[string]any
|
||||
if err := json.Unmarshal([]byte(it.Fields), &m); err != nil {
|
||||
t.Fatalf("parse fields for %q (%q): %v", it.Title, it.Fields, err)
|
||||
}
|
||||
if v, ok := m["color"].(string); ok {
|
||||
byTitle[it.Title] = v
|
||||
}
|
||||
if it.Title == "Red" {
|
||||
byTitle["__red_id__"] = it.ID
|
||||
}
|
||||
}
|
||||
|
||||
if got := byTitle["Dangling"]; got != danglingID {
|
||||
t.Fatalf("the unresolvable value was rewritten to %q; it must carry VERBATIM as %q — "+
|
||||
"a partial rewrite produces a value that existed on neither side", got, danglingID)
|
||||
}
|
||||
if got, want := byTitle["Resolvable"], byTitle["__red_id__"]; got != want {
|
||||
t.Fatalf("the EXACT-match value carried as %q, want the imported item's new id %q; "+
|
||||
"the remap has stopped working", got, want)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user