diff --git a/cmd/pad/cmd_item.go b/cmd/pad/cmd_item.go index da2cf94dd..efa5b10a8 100644 --- a/cmd/pad/cmd_item.go +++ b/cmd/pad/cmd_item.go @@ -253,7 +253,7 @@ Run with --help-collections to see available collections and their status values if coll, err := cli.WithCollectionAliasFallback(rawSlug, nil, func(slug string) (*models.Collection, error) { return client.GetCollection(ws, slug) }); err == nil { - _ = json.Unmarshal([]byte(coll.Schema), &collSchema) + _ = models.UnmarshalItemFieldSchema([]byte(coll.Schema), &collSchema) } for _, kv := range fieldFlags { key, val, ferr := items.SplitFieldEntry(kv) @@ -1197,7 +1197,7 @@ Examples: var collSchema models.CollectionSchema if item.CollectionSlug != "" { if coll, err := client.GetCollection(ws, item.CollectionSlug); err == nil { - _ = json.Unmarshal([]byte(coll.Schema), &collSchema) + _ = models.UnmarshalItemFieldSchema([]byte(coll.Schema), &collSchema) } } for _, kv := range fieldFlags { @@ -1860,7 +1860,7 @@ Examples: Schema: func(wsSlug, collSlug string) models.CollectionSchema { var schema models.CollectionSchema if coll, err := client.GetCollection(wsSlug, collSlug); err == nil { - _ = json.Unmarshal([]byte(coll.Schema), &schema) + _ = models.UnmarshalItemFieldSchema([]byte(coll.Schema), &schema) } return schema }, diff --git a/go.mod b/go.mod index 17337ed1e..2f1813a91 100644 --- a/go.mod +++ b/go.mod @@ -47,6 +47,7 @@ require ( golang.org/x/term v0.45.0 golang.org/x/text v0.41.0 golang.org/x/time v0.16.0 + golang.org/x/tools v0.48.0 modernc.org/sqlite v1.58.0 ) @@ -134,7 +135,6 @@ require ( golang.org/x/net v0.58.0 // indirect golang.org/x/oauth2 v0.36.0 // indirect golang.org/x/sync v0.22.0 // indirect - golang.org/x/tools v0.48.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260803160001-6ac0973c030d // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260803160001-6ac0973c030d // indirect google.golang.org/grpc v1.83.2 // indirect diff --git a/internal/items/migrate.go b/internal/items/migrate.go index 55bf99f00..5363c7c81 100644 --- a/internal/items/migrate.go +++ b/internal/items/migrate.go @@ -257,6 +257,12 @@ func StillDropped(dropped []string, finalFields map[string]any) []string { // every move and copy for a collection whose only sin is a field name someone // was once allowed to pick. // +// The strip itself now lives in models.SchemaWithoutReservedFields, which +// BUG-2685 made the one predicate for every consumer that reasons about an +// ITEM's fields rather than only for migration's output. This name stays +// because its CALLERS are about migration and its doc is where the +// migration-specific reasoning above belongs; there is one implementation. +// // Returns schema unchanged (no copy) when it declares no reserved key, which is // every collection that has not been grandfathered — so the returned Fields // slice may ALIAS the caller's. That is deliberate: this is called on a hot @@ -265,24 +271,5 @@ func StillDropped(dropped []string, finalFields map[string]any) []string { // read-only — treat the result as immutable. Every current caller passes it // straight to a validator or an enumeration and none mutates it. func SchemaForMigratedFields(schema models.CollectionSchema) models.CollectionSchema { - var reserved bool - for _, f := range schema.Fields { - if models.IsReservedItemField(f.Key) { - reserved = true - break - } - } - if !reserved { - return schema - } - - out := schema - out.Fields = make([]models.FieldDef, 0, len(schema.Fields)) - for _, f := range schema.Fields { - if models.IsReservedItemField(f.Key) { - continue - } - out.Fields = append(out.Fields, f) - } - return out + return models.SchemaWithoutReservedFields(schema) } diff --git a/internal/mcp/dispatch_http_advanced.go b/internal/mcp/dispatch_http_advanced.go index 61d466771..0d2d3b870 100644 --- a/internal/mcp/dispatch_http_advanced.go +++ b/internal/mcp/dispatch_http_advanced.go @@ -645,7 +645,7 @@ func (d *HTTPHandlerDispatcher) collectionSchemaShadowsParent( // failed GetCollection there leaves collSchema at its zero value and // --field values just stay untyped strings instead of blocking the // update). - _ = json.Unmarshal([]byte(coll.Schema), &schema) + _ = models.UnmarshalItemFieldSchema([]byte(coll.Schema), &schema) } for _, f := range schema.Fields { if f.Key == "parent" || f.Key == "plan" { diff --git a/internal/models/collection_schema_item_fields.go b/internal/models/collection_schema_item_fields.go new file mode 100644 index 000000000..760c3de67 --- /dev/null +++ b/internal/models/collection_schema_item_fields.go @@ -0,0 +1,89 @@ +package models + +import "encoding/json" + +// SchemaWithoutReservedFields returns schema with any reserved-metadata +// FieldDef removed — the schema as a consumer that reasons about an ITEM's +// field values must see it (BUG-2685). +// +// WHY A GRANDFATHERED DECLARATION IS IGNORED RATHER THAN REFUSED. +// `validateNoReservedFieldKeys` (BUG-2674) stops a schema NEWLY declaring one +// of these keys but deliberately grandfathers an existing declaration, so the +// shape survives wherever it predates that gate. Such a declaration is wrong in +// BOTH directions at once, which is what rules out refusing the write: +// +// - it REFUSES the system's own value — `pad item note` / `pad item decide` +// append an entry and PATCH the whole fields blob back, and a legacy +// `implementation_notes: text` declaration fails that ARRAY on +// validateFieldType, so the collection's items cannot be annotated at all; +// - it ACCEPTS a caller's junk — the same declaration waves a bare string +// through into the key, where the append helpers then refuse to touch it +// (BUG-2627 part 3). +// +// Refusing would also break every system writer on such a collection, and a +// required legacy declaration already refuses ordinary creates over a field no +// client surface offers. Ignoring it restores the invariant these keys were +// always supposed to have: they are system-owned, and no collection schema +// speaks for them. +// +// Returns schema unchanged (no copy) when it declares no reserved key, which is +// every collection that has not been grandfathered — so the returned Fields +// slice may ALIAS the caller's. The contract is therefore read-only: treat the +// result as immutable. +func SchemaWithoutReservedFields(schema CollectionSchema) CollectionSchema { + var reserved bool + for _, f := range schema.Fields { + if IsReservedItemField(f.Key) { + reserved = true + break + } + } + if !reserved { + return schema + } + + out := schema + out.Fields = make([]FieldDef, 0, len(schema.Fields)) + for _, f := range schema.Fields { + if IsReservedItemField(f.Key) { + continue + } + out.Fields = append(out.Fields, f) + } + return out +} + +// UnmarshalItemFieldSchema decodes a collection's stored schema JSON for a +// consumer that reasons about an ITEM's field values, applying +// SchemaWithoutReservedFields to the result. +// +// IT IS THE ONE PREDICATE, and it sits at the DECODE rather than at each use on +// purpose. BUG-2685's instrument counted 76 sites that consult a +// `CollectionSchema.Fields` and only 38 that decode one: a rule applied at the +// consultations is 76 patches that every future consumer has to remember, which +// is the shape of the last four review rounds this bug was filed out of. Every +// consultation inherits whatever the decode produced, so the decode is where +// one rule covers all of them. +// +// Signature mirrors json.Unmarshal's deliberately — same argument order, same +// error semantics — so converting a call site is a one-token edit and the +// remaining raw decodes stay greppable. TestReservedFieldDecodeSitesAreClassified +// enforces that: a NEW raw decode into a CollectionSchema fails the build unless +// it is added to that test's allow-list of collection-definition sites, with a +// reason. +// +// NOT for the sites where the collection DEFINITION is the subject — schema +// create/update input, the grandfather test's own `prevSchema`, and the CLI / +// MCP paths that render or accept a schema. There the declaration is the thing +// being talked about, and stripping it would be wrong rather than merely +// unnecessary: feeding a stripped `prevSchema` to validateNoReservedFieldKeys +// would reclassify every existing declaration as newly introduced and refuse +// every update to such a collection. +func UnmarshalItemFieldSchema(data []byte, dst *CollectionSchema) error { + var schema CollectionSchema + if err := json.Unmarshal(data, &schema); err != nil { + return err + } + *dst = SchemaWithoutReservedFields(schema) + return nil +} diff --git a/internal/models/reserved_field_decode_guard_test.go b/internal/models/reserved_field_decode_guard_test.go new file mode 100644 index 000000000..c6ec6e616 --- /dev/null +++ b/internal/models/reserved_field_decode_guard_test.go @@ -0,0 +1,610 @@ +package models_test + +import ( + "fmt" + "go/ast" + "go/types" + "os" + "path/filepath" + "reflect" + "sort" + "strings" + "sync" + "testing" + + "golang.org/x/tools/go/packages" +) + +// BUG-2685's enforcement half. +// +// The fix is one predicate — models.UnmarshalItemFieldSchema — applied at every +// site that decodes a collection schema FOR ITEM-FIELD PURPOSES. A predicate +// nothing enforces is a convention the next consumer has to remember, and the +// last four review rounds this bug was filed out of are what remembering looks +// like in practice: each round found another site. +// +// So this test enumerates every raw `json.Unmarshal` into a models. +// CollectionSchema in the tree and requires each to be listed below WITH A +// REASON. A new item-field consumer that decodes raw fails here and is told +// which call to use; a genuine collection-definition site is added to the map +// and the reason is the thing review argues about. +// +// The allow-list is deliberately positional (file + the identifier written to) +// rather than a whole-file exemption: a file may hold both kinds, and +// handlers_collections.go does. +// +// IT IS TYPE-RESOLVED, and that was earned rather than chosen. The first +// version approximated types by reading declarations with go/ast, and FIVE +// consecutive codex rounds each named more DESTINATION SPELLINGS it could not +// see — a bare pointer parameter; an inferred-type var; new(T); an aliased +// models import, `var x = new(T)`, `dst := &s`, an inline element type; a type +// alias, `dst := helper()`, a closure parameter. That loop was measuring how +// many spellings a reviewer could think of, not whether the guard was sound +// (CONVE-35). go/packages answers the question the approximation was +// approximating: what is the TYPE of this expression. No spelling escapes it, +// including the ones nobody has thought of yet. +// +// It also removes the need for a registry of parallel schema structs. A +// destination counts when its type is SCHEMA-SHAPED — a struct with a +// `json:"fields"` member that is a slice of structs — so `bootstrapSchema`, +// which is a hand-kept mirror of CollectionSchema under a different name and +// was invisible to a pass keyed on the canonical type, is caught by what it IS. + +// rawDecodeAllowed maps "path::destination" to why that site decodes the schema +// RAW. Every entry is a site where the collection DEFINITION is the subject, so +// stripping the declaration would be wrong rather than merely unnecessary. +var rawDecodeAllowed = map[string]string{ + "internal/models/collection_schema_item_fields.go::schema": "UnmarshalItemFieldSchema IS the predicate — it decodes raw and then strips, which is the whole point of it", + + // INTERFACE destinations. The type checker cannot see what these decode + // into, so they are reported rather than assumed harmless; each is a + // GENERIC decode helper whose callers choose the concrete type, and none + // of them can be converted without breaking every other thing they decode. + // A new one shows up here and has to be reasoned about, which is the point. + "internal/cli/client.go::result": "generic HTTP response decoder: the caller supplies the destination, so this call has no schema to strip — a caller that wants an item-field schema must decode one itself", + "internal/oauth/storage.go::session": "fosite session hydration: decodes an OAuth session blob, never a collection schema", + "internal/server/server.go::v": "the shared request-body decoder (BUG-2803): decodes a handler's own input struct, and a handler taking a schema goes through the collection-definition doors above", + "internal/server/handlers_bootstrap.go::s": "trimRedundantSchemaLabels decodes into the parallel bootstrapSchema struct and strips reserved keys in the same loop — the parallel-struct twin of UnmarshalItemFieldSchema, and it cannot call it because the whole point of that struct is a different FieldDef shape", + + "internal/server/handlers_collections.go::schema": "collection create/update INPUT — the declaration is what is being validated, by validateNoReservedFieldKeys", + "internal/server/handlers_collections.go::prevSchema": "the grandfather test's own baseline: a stripped prevSchema would reclassify every EXISTING declaration as newly introduced and refuse every update to such a collection", + "cmd/pad/cmd_collection.go::schema": "CLI --schema parse and the collection list's own status-option rendering — the collection's definition, not any item's fields", + "internal/mcp/dispatch_http_routes.go::schema": "MCP collection schema INPUT, the remote twin of handlers_collections.go", + + // The move / copy family. These decode raw and strip at the point of use + // via items.SchemaForMigratedFields (BUG-2674), which is the same + // predicate; they are listed rather than converted because the preflight + // distinguishes the raw source schema from the stripped one in its + // reporting, and collapsing the two is a separate change with its own + // regression surface. See the BUG-2685 trail. + "internal/server/handlers_items.go::sourceSchema": "move: stripped at use by items.SchemaForMigratedFields", + "internal/server/handlers_items.go::targetSchema": "move: stripped at use by items.SchemaForMigratedFields", + "internal/server/handlers_items_bulk.go::sourceSchema": "bulk move: stripped at use by items.SchemaForMigratedFields", + "internal/server/handlers_items_bulk.go::targetSchema": "bulk move: stripped at use by items.SchemaForMigratedFields", + "internal/server/handlers_items_copy.go::targetSchema": "copy: stripped at use by items.SchemaForMigratedFields", + "internal/server/handlers_items_copy_preflight.go::sourceSchema": "copy preflight: the RAW source schema is the subject of its dropped-key reporting", + "internal/server/handlers_items_copy_preflight.go::targetSchema": "copy preflight: stripped at use by items.SchemaForMigratedFields", + "internal/store/items_cross_workspace_copy.go::sourceSchema": "store copy: the RAW source schema is the subject of its dropped-key reporting", + "internal/store/items_cross_workspace_copy.go::targetSchema": "store copy: stripped at use by items.SchemaForMigratedFields", +} + +func repoRoot(t *testing.T) string { + t.Helper() + dir, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir + } + parent := filepath.Dir(dir) + if parent == dir { + t.Fatal("no go.mod above the test's working directory") + } + dir = parent + } +} + +type decodeSite struct { + path string + line int + dest string +} + +func (s decodeSite) key() string { return s.path + "::" + s.dest } + +// schemaShaped reports whether t is, under its pointers and named types, a +// struct carrying a `json:"fields"` member that is a slice of structs — the +// shape of a collection schema. +// +// BY SHAPE, NOT BY NAME, for the reason the bootstrap hole taught: a mirror +// struct under a different name is the same contract, and a guard keyed on the +// canonical type cannot see one. An ITEM's `fields` is its VALUES — a string, a +// json.RawMessage, a map, a named struct — and there are seven of those in the +// tree, all excluded by the slice-of-structs test. +func schemaShaped(t types.Type) bool { + for i := 0; i < 8; i++ { + // types.Unalias FIRST: since Go 1.24 a type alias materialises as + // *types.Alias rather than resolving to the aliased Named, so a loop + // that only unwraps Pointer and Named stops dead on + // `type S = models.CollectionSchema`. Found by the fixture, which is + // exactly the job it was kept for. + t = types.Unalias(t) + switch u := t.(type) { + case *types.Pointer: + t = u.Elem() + continue + case *types.Named: + t = u.Underlying() + continue + } + break + } + st, ok := t.(*types.Struct) + if !ok { + return false + } + for i := 0; i < st.NumFields(); i++ { + // An EMBEDDED schema carries the same JSON members at the same level, + // so a struct embedding models.CollectionSchema decodes a schema + // declaration exactly as the schema itself does. + if st.Field(i).Embedded() && schemaShaped(st.Field(i).Type()) { + return true + } + if !jsonNameIsFields(st.Field(i).Name(), st.Tag(i)) { + continue + } + ft := st.Field(i).Type() + for j := 0; j < 8; j++ { + ft = types.Unalias(ft) + if n, ok := ft.(*types.Named); ok { + ft = n.Underlying() + continue + } + break + } + sl, ok := ft.(*types.Slice) + if !ok { + continue + } + el := sl.Elem() + for j := 0; j < 8; j++ { + el = types.Unalias(el) + // `[]*models.FieldDef` decodes the same JSON as `[]models.FieldDef`. + if pt, ok := el.(*types.Pointer); ok { + el = pt.Elem() + continue + } + if n, ok := el.(*types.Named); ok { + el = n.Underlying() + continue + } + break + } + if _, ok := el.(*types.Struct); ok { + return true + } + } + return false +} + +// jsonNameIsFields reports whether a struct field is serialised as `fields`. +// +// encoding/json matches an UNTAGGED field by NAME, case-insensitively, so +// `struct{ Fields []models.FieldDef }` decodes the same JSON as the tagged +// version and is the same schema (codex round 12). Requiring the tag was a +// property of how the shape test was written rather than of what JSON does. +func jsonNameIsFields(name, tag string) bool { + if v, ok := reflect.StructTag(tag).Lookup("json"); ok { + jsonName := v + if idx := strings.Index(v, ","); idx >= 0 { + jsonName = v[:idx] + } + if jsonName == "-" { + return false + } + if jsonName != "" { + return jsonName == "fields" + } + // `json:",omitempty"` keeps the field NAME; fall through. + } + return strings.EqualFold(name, "fields") +} + +// isVendored reports whether a repo-relative path lies under any vendor/ +// directory. Checked per SEGMENT rather than as a prefix, so a nested vendor +// tree is excluded too and a legitimate path like `internal/vendorimport.go` is +// not. +func isVendored(rel string) bool { + for _, seg := range strings.Split(filepath.ToSlash(rel), "/") { + if seg == "vendor" { + return true + } + } + return false +} + +// destName is the allow-list key's second half: the name a reader would use for +// the destination. Derived from the expression rather than from its type, so the +// key stays the one a person would write down. +func destName(e ast.Expr) string { + switch x := e.(type) { + case *ast.UnaryExpr: + return destName(x.X) + case *ast.StarExpr: + return destName(x.X) + case *ast.Ident: + return x.Name + case *ast.SelectorExpr: + return x.Sel.Name + case *ast.IndexExpr: + return destName(x.X) + case *ast.CallExpr: + return destName(x.Fun) + case *ast.CompositeLit: + if x.Type != nil { + return destName(x.Type) + } + } + return "" +} + +// collectSchemaDecodes returns every RAW decode whose destination is +// schema-shaped: a call to encoding/json.Unmarshal, or to a Decode method on an +// encoding/json Decoder, whose last argument has such a type. +// +// Calls to models.UnmarshalItemFieldSchema are not decodes for this purpose — +// that IS the predicate — and are excluded by callee identity rather than by +// name matching. +// loadCache memoises the type-checked walk. Four tests ask the same question of +// the same tree and a full go/packages load is ~5s each; without this the guard +// alone is most of the package's runtime. +var ( + loadMu sync.Mutex + loadCache = map[string][]decodeSite{} +) + +func collectSchemaDecodes(t *testing.T, dir string, patterns ...string) []decodeSite { + t.Helper() + if len(patterns) == 0 { + patterns = []string{"./..."} + } + cacheKey := dir + "|" + strings.Join(patterns, ",") + loadMu.Lock() + if cached, ok := loadCache[cacheKey]; ok { + loadMu.Unlock() + return cached + } + loadMu.Unlock() + cfg := &packages.Config{ + Mode: packages.NeedName | packages.NeedFiles | packages.NeedSyntax | + packages.NeedTypes | packages.NeedTypesInfo | packages.NeedDeps | + packages.NeedImports, + Dir: dir, + Tests: false, + } + pkgs, err := packages.Load(cfg, patterns...) + if err != nil { + t.Fatalf("load: %v", err) + } + var loadErrs []string + packages.Visit(pkgs, nil, func(p *packages.Package) { + for _, e := range p.Errors { + loadErrs = append(loadErrs, p.PkgPath+": "+e.Error()) + } + }) + if len(loadErrs) > 0 { + // A guard that certifies a tree it could not type-check certifies + // nothing — this is the "went blind while passing" failure with the + // type checker's own voice. + t.Fatalf("the tree did not type-check, so this guard proves nothing:\n %s", + strings.Join(loadErrs[:min(len(loadErrs), 5)], "\n ")) + } + + root := dir + var out []decodeSite + seen := map[string]bool{} + packages.Visit(pkgs, nil, func(p *packages.Package) { + if p.TypesInfo == nil { + return + } + for _, f := range p.Syntax { + pos := p.Fset.Position(f.Pos()) + if strings.HasSuffix(pos.Filename, "_test.go") { + continue + } + rel, err := filepath.Rel(root, pos.Filename) + if err != nil || strings.HasPrefix(rel, "..") { + continue // outside the tree under test (a dependency) + } + // VENDORED code is not ours to classify, and it is not hypothetical: + // this guard passed locally and FAILED the Nix job, which builds + // from a `vendor/` tree that `./...` then walks. 57 sites instead of + // 20, every extra one a dependency's own decode caught by the + // interface-destination arm. A guard whose verdict depends on + // whether the checkout happens to be vendored is not a guard. + if isVendored(rel) { + continue + } + ast.Inspect(f, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || len(call.Args) == 0 { + return true + } + if !isDecodeCall(call, p.TypesInfo) { + return true + } + // The DESTINATION is the last argument for both + // json.Unmarshal(data, dst) and dec.Decode(dst). The source + // argument routinely mentions a schema too. + arg := call.Args[len(call.Args)-1] + tv, ok := p.TypesInfo.Types[arg] + if !ok { + return true + } + // An INTERFACE destination hides its concrete type from the + // type checker — `var dst any = &schema` decodes a schema and + // resolves as `any`. Tracking it properly is dataflow analysis; + // instead every such decode is REPORTED for classification. + // There are three in the tree, so the cost is three allow-list + // entries and the hole is closed rather than documented. + // + // A POINTER to an interface (`json.Unmarshal(b, &v)` where v is + // `any`) is DELIBERATELY NOT included, and the number is the + // reason: including it takes the population from 20 to 38, and + // the 18 additions are all the ordinary decode-arbitrary-JSON + // idiom. Eighteen entries whose reason is the same sentence is + // an allow-list nobody reads, which is a worse guard than the + // hole. The residual needs someone to place a schema POINTER + // inside an `any` and then pass its address — a shape no site + // in this tree uses and no converted site could become without + // the conversion being visible in the same diff. + if !schemaShaped(tv.Type) && !types.IsInterface(tv.Type) { + return true + } + cpos := p.Fset.Position(call.Pos()) + s := decodeSite{ + path: filepath.ToSlash(rel), + line: cpos.Line, + dest: destName(arg), + } + k := fmt.Sprintf("%s:%d:%s", s.path, s.line, s.dest) + if seen[k] { + return true + } + seen[k] = true + out = append(out, s) + return true + }) + } + }) + sort.Slice(out, func(i, j int) bool { + if out[i].path != out[j].path { + return out[i].path < out[j].path + } + if out[i].line != out[j].line { + return out[i].line < out[j].line + } + return out[i].dest < out[j].dest + }) + + loadMu.Lock() + loadCache[cacheKey] = out + loadMu.Unlock() + return out +} + +// isDecodeCall reports whether call is a JSON decode — a call to +// encoding/json.Unmarshal or to a Decode method on its Decoder, DIRECTLY or +// through a function VALUE holding one. +// +// The function-value arm exists because `decode := json.Unmarshal` followed by +// `decode(b, &schema)` resolves to no *types.Func at all. It is matched by +// SIGNATURE, which would be far too wide on its own — the tree has 1710 two-arg +// calls through function values (measured) — and is only ever reached for a +// call whose LAST ARGUMENT is already schema-shaped, which is what makes it +// narrow. +func isDecodeCall(call *ast.CallExpr, info *types.Info) bool { + if sel, ok := call.Fun.(*ast.SelectorExpr); ok { + if fn, ok := info.Uses[sel.Sel].(*types.Func); ok { + return isJSONDecodeFunc(fn) + } + } + if id, ok := call.Fun.(*ast.Ident); ok { + if fn, ok := info.Uses[id].(*types.Func); ok { + return isJSONDecodeFunc(fn) + } + } + tv, ok := info.Types[call.Fun] + if !ok { + return false + } + sig, ok := types.Unalias(tv.Type).(*types.Signature) + if !ok || sig.Recv() != nil { + return false + } + // json.Unmarshal: func([]byte, any) error. Decoder.Decode: func(any) error. + if sig.Results().Len() != 1 || sig.Results().At(0).Type().String() != "error" { + return false + } + switch sig.Params().Len() { + case 1: + return types.IsInterface(sig.Params().At(0).Type()) + case 2: + p0, ok := sig.Params().At(0).Type().(*types.Slice) + return ok && p0.Elem().String() == "byte" && types.IsInterface(sig.Params().At(1).Type()) + } + return false +} + +// isJSONDecodeFunc reports whether fn is encoding/json's Unmarshal, or the +// Decode method on its Decoder. +// +// By CALLEE IDENTITY rather than by the receiver's spelling: an aliased import +// and a `json.NewDecoder(r).Decode(...)` chain both resolve to the same object, +// which is what makes the alias hole disappear rather than get another arm. +func isJSONDecodeFunc(fn *types.Func) bool { + sig, _ := fn.Type().(*types.Signature) + if sig == nil { + return false + } + if recv := sig.Recv(); recv != nil { + if fn.Name() != "Decode" { + return false + } + t := recv.Type() + if p, ok := t.(*types.Pointer); ok { + t = p.Elem() + } + named, ok := t.(*types.Named) + if !ok || named.Obj().Pkg() == nil { + return false + } + return named.Obj().Pkg().Path() == "encoding/json" && named.Obj().Name() == "Decoder" + } + return fn.Pkg() != nil && fn.Pkg().Path() == "encoding/json" && fn.Name() == "Unmarshal" +} + +func min(a, b int) int { + if a < b { + return a + } + return b +} + +func TestReservedFieldDecodeSitesAreClassified(t *testing.T) { + root := repoRoot(t) + sites := collectSchemaDecodes(t, root) + if len(sites) == 0 { + t.Fatal("the guard found no decode sites at all — it has gone blind, " + + "which would let every future site through silently") + } + for _, s := range sites { + if _, ok := rawDecodeAllowed[s.key()]; !ok { + t.Errorf("%s:%d decodes a collection schema RAW into %q.\n"+ + " If it reasons about an ITEM's fields, call models.UnmarshalItemFieldSchema instead — a\n"+ + " grandfathered reserved-key FieldDef must not reach it (BUG-2685).\n"+ + " If the collection DEFINITION is the subject, add %q to rawDecodeAllowed with the reason.", + s.path, s.line, s.dest, s.key()) + } + } +} + +// TestGuardAllowListHasNoDeadEntries keeps the allow-list honest: an entry whose +// site was converted or deleted is a standing permission for a site that no +// longer asks for it, and the next raw decode with that name inherits it. +func TestGuardAllowListHasNoDeadEntries(t *testing.T) { + live := map[string]bool{} + for _, s := range collectSchemaDecodes(t, repoRoot(t)) { + live[s.key()] = true + } + for k := range rawDecodeAllowed { + if !live[k] { + t.Errorf("rawDecodeAllowed has no live site for %q — remove the entry", k) + } + } +} + +// rawDecodeSiteCount is how many raw decode sites the guard must find. +// +// A RECEIPT, not a magic number: 20 on the BUG-2685 tree — 16 decoding a +// models.CollectionSchema, plus the one decoding the parallel `bootstrapSchema` +// (which a pass keyed on the canonical type could not see, and which is why +// this guard matches on SHAPE), plus the three whose destination is an +// INTERFACE and so cannot be resolved at all. The last three are reported +// rather than assumed harmless; the allow-list says what each actually decodes. +// +// It is a count and not just a key set because two sites can share a +// `path::dest` key — cmd_collection.go and handlers_collections.go each have +// two — so key-set equality alone would pass if the guard went blind to one of +// a pair. A DROP with no conversion in the same change means the guard stopped +// seeing sites, not that the sites stopped existing. +const rawDecodeSiteCount = 20 + +func TestGuardSeesEveryKnownDecodeSite(t *testing.T) { + sites := collectSchemaDecodes(t, repoRoot(t)) + if len(sites) != rawDecodeSiteCount { + var got []string + for _, s := range sites { + got = append(got, fmt.Sprintf("%s:%d(%s)", s.path, s.line, s.dest)) + } + t.Errorf("guard found %d raw decode sites, expected %d.\n found: %s", + len(sites), rawDecodeSiteCount, strings.Join(got, "\n ")) + } +} + +// TestGuardResolvesEveryDestinationShape runs the resolver over the fixture in +// testdata/decodeshapes and requires it to see every destination spelling +// written down there. +// +// THE FIXTURE OUTLIVED THE GUARD IT WAS WRITTEN FOR, which is why it is still +// here. It was built for the go/ast approximation, where five review rounds +// each named another spelling that approximation could not see; the type +// checker answers all of them by construction, so this is no longer a list of +// holes to plug but a REGRESSION TEST on the resolver — it fails if a future +// change to schemaShaped, isJSONDecodeFunc, or the destination-argument rule +// narrows what the guard can resolve. +// +// It also pins the NEGATIVE half: the fixture's last function decodes through +// the correct helper with a SOURCE argument that mentions a schema, and must +// NOT be reported. +func TestGuardResolvesEveryDestinationShape(t *testing.T) { + root := repoRoot(t) + dir := filepath.Join(root, "internal", "models", "testdata", "decodeshapes") + if _, err := os.Stat(dir); err != nil { + t.Fatalf("fixture missing: %v", err) + } + + sites := collectSchemaDecodes(t, dir, ".") + got := map[string]bool{} + for _, s := range sites { + got[s.dest] = true + } + + want := []string{ + "localDest", // &local + "paramDest", // bare pointer param + "fieldDest", // &struct.field + "new", // new(T) — the call, which is the destination itself + "CollectionSchema", // &T{} names the TYPE, not a variable + "sliceDest", // &container[i] + "makeDest", // the result of a helper returning a schema + "decoderDest", // json.Decoder.Decode, aliased import + "shortDest", // x := T{} + "inferredDest", // var x = T{} + "newDest", // var x = new(T) + "aliasDest", // x := &alreadyASchema + "aliasedPkgDest", // models imported under an alias + "typeAliasDest", // a type alias for the schema + "helperResultDest", // x := helperReturningASchemaPointer() + "closureDest", // a closure parameter + "inlineDest", // a schema-shaped type that is not CollectionSchema + "namedSliceDest", // the fields member is a NAMED slice type + "funcValueDest", // called through a function value holding json.Unmarshal + "ifaceDest", // an interface destination, reported not resolved + "embeddedDest", // a struct embedding a schema + "ptrElemDest", // fields whose element type is a pointer + "untaggedDest", // an UNTAGGED fields member, matched by name + } + // `excludedDest` is deliberately absent: its fields member is `json:"-"`, + // so it is not a schema on the wire and must NOT be reported. + for _, w := range want { + if !got[w] { + t.Errorf("the resolver cannot see the destination shape named by %q in testdata/decodeshapes", w) + } + } + + // Twenty-three decodes are destinations; the fixture's last call is the + // negative case and goes through the helper, so it is not a raw decode at + // all. BELOW twenty-three means a shape went missing; ABOVE means the source + // argument is matching again. + if len(sites) != 23 { + var lines []string + for _, s := range sites { + lines = append(lines, fmt.Sprintf("%s:%d(%s)", s.path, s.line, s.dest)) + } + t.Errorf("fixture yielded %d raw decode sites, expected 23:\n %s", + len(sites), strings.Join(lines, "\n ")) + } +} diff --git a/internal/models/terminal.go b/internal/models/terminal.go index a58327d4d..b98c9d12f 100644 --- a/internal/models/terminal.go +++ b/internal/models/terminal.go @@ -142,7 +142,7 @@ func CollectionCompletedWorkValues(schemaJSON, settingsJSON string) (fieldKey st var schema CollectionSchema var settings CollectionSettings if schemaJSON != "" { - _ = json.Unmarshal([]byte(schemaJSON), &schema) + _ = UnmarshalItemFieldSchema([]byte(schemaJSON), &schema) } if settingsJSON != "" { _ = json.Unmarshal([]byte(settingsJSON), &settings) diff --git a/internal/models/testdata/decodeshapes/aliased.go b/internal/models/testdata/decodeshapes/aliased.go new file mode 100644 index 000000000..8b1aa0c63 --- /dev/null +++ b/internal/models/testdata/decodeshapes/aliased.go @@ -0,0 +1,29 @@ +package decodeshapes + +import ( + jsonx "encoding/json" + + m "github.com/PerpetualSoftware/pad/internal/models" +) + +// 13. the models package under a LOCAL ALIAS. +func s13(b []byte) { + var aliasedPkgDest m.CollectionSchema + _ = jsonx.Unmarshal(b, &aliasedPkgDest) +} + +// 14. a schema shape whose element type is declared INLINE. +type inlineShaped struct { + Fields []struct { + Key string `json:"key"` + Type string `json:"type"` + } `json:"fields"` +} + +// 18. a decode into a schema-shaped type whose ELEMENT type is declared inline +// and which is not models.CollectionSchema at all — the bootstrapSchema case, +// generalised. A guard keyed on the canonical type cannot see this one. +func s18(b []byte) { + var inlineDest inlineShaped + _ = jsonx.Unmarshal(b, &inlineDest) +} diff --git a/internal/models/testdata/decodeshapes/round8.go b/internal/models/testdata/decodeshapes/round8.go new file mode 100644 index 000000000..56f68871e --- /dev/null +++ b/internal/models/testdata/decodeshapes/round8.go @@ -0,0 +1,108 @@ +package decodeshapes + +import ( + jsonx "encoding/json" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// 15. a TYPE ALIAS for a schema. +type aliasType = models.CollectionSchema + +func s15(b []byte) { + var typeAliasDest aliasType + _ = jsonx.Unmarshal(b, &typeAliasDest) +} + +// 16. a variable holding the result of a helper that returns a schema pointer. +func s16(b []byte) { + helperResultDest := makeDest() + _ = jsonx.Unmarshal(b, helperResultDest) +} + +// 17. a CLOSURE parameter. +func s17(b []byte) { + f := func(closureDest *models.CollectionSchema) { + _ = jsonx.Unmarshal(b, closureDest) + } + f(nil) +} + +// 19. a NAMED slice type for the fields member — the shape test must unwrap it. +type fieldList []models.FieldDef + +type namedSliceSchema struct { + Fields fieldList `json:"fields"` +} + +func s19(b []byte) { + var namedSliceDest namedSliceSchema + _ = jsonx.Unmarshal(b, &namedSliceDest) +} + +// 20. a call through a FUNCTION VALUE holding json.Unmarshal. +func s20(b []byte) { + decode := jsonx.Unmarshal + var funcValueDest models.CollectionSchema + _ = decode(b, &funcValueDest) +} + +// 21. an INTERFACE destination, which hides the concrete type. Reported for +// classification rather than resolved. +func s21(b []byte) { + var schema models.CollectionSchema + var ifaceDest any = &schema + _ = jsonx.Unmarshal(b, ifaceDest) +} + +// NOT resolved, and deliberately: a POINTER to an interface. Including it would +// pull in every decode-arbitrary-JSON call in the tree (measured: 20 sites +// become 38). See the comment beside the interface arm in the guard. +func s22(b []byte) { + var schema models.CollectionSchema + var ptrIfaceDest any = &schema + _ = jsonx.Unmarshal(b, &ptrIfaceDest) +} + +// 23. a struct EMBEDDING a schema carries the same JSON members. +type embeddedSchema struct { + models.CollectionSchema + Extra string `json:"extra"` +} + +func s23(b []byte) { + var embeddedDest embeddedSchema + _ = jsonx.Unmarshal(b, &embeddedDest) +} + +// 24. a fields member whose ELEMENT is a pointer. +type ptrElemSchema struct { + Fields []*models.FieldDef `json:"fields"` +} + +func s24(b []byte) { + var ptrElemDest ptrElemSchema + _ = jsonx.Unmarshal(b, &ptrElemDest) +} + +// 25. an UNTAGGED fields member. encoding/json matches it by name, +// case-insensitively, so this decodes the same JSON as the tagged form. +type untaggedSchema struct { + Fields []models.FieldDef +} + +func s25(b []byte) { + var untaggedDest untaggedSchema + _ = jsonx.Unmarshal(b, &untaggedDest) +} + +// 26. a fields member EXCLUDED from JSON is not a schema member at all. +type excludedFieldsSchema struct { + Fields []models.FieldDef `json:"-"` + Real string `json:"real"` +} + +func s26(b []byte) { + var excludedDest excludedFieldsSchema + _ = jsonx.Unmarshal(b, &excludedDest) +} diff --git a/internal/models/testdata/decodeshapes/shapes.go b/internal/models/testdata/decodeshapes/shapes.go new file mode 100644 index 000000000..c0bf98f5f --- /dev/null +++ b/internal/models/testdata/decodeshapes/shapes.go @@ -0,0 +1,104 @@ +// Package decodeshapes is a FIXTURE, not compiled code — it lives under +// testdata so the go tool ignores it, and exists to be parsed by +// TestGuardCatchesEveryDestinationShape. +// +// It is the population table for BUG-2685's guard. Three consecutive review +// rounds each named another destination spelling the guard did not handle, one +// at a time; CONVE-35 says a loop that keeps finding new members of a class is +// measuring the enumeration rather than the code. So the class is written down +// HERE, once, and the guard is tested against the whole of it. +// +// Adding a spelling to this file is how you extend that table. A spelling the +// guard cannot see fails the test, which is the point. +package decodeshapes + +import ( + jsonx "encoding/json" + "io" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +type holder struct { + fieldDest models.CollectionSchema +} + +func makeDest() *models.CollectionSchema { return &models.CollectionSchema{} } + +// 1. address of a local value +func s1(b []byte) { + var localDest models.CollectionSchema + _ = jsonx.Unmarshal(b, &localDest) +} + +// 2. a parameter that is already a pointer, passed bare +func s2(b []byte, paramDest *models.CollectionSchema) { + _ = jsonx.Unmarshal(b, paramDest) +} + +// 3. address of a struct field whose struct is declared elsewhere in the package +func s3(b []byte) { + var h holder + _ = jsonx.Unmarshal(b, &h.fieldDest) +} + +// 4. new(T) +func s4(b []byte) { + _ = jsonx.Unmarshal(b, new(models.CollectionSchema)) +} + +// 5. address of a composite literal +func s5(b []byte) { + _ = jsonx.Unmarshal(b, &models.CollectionSchema{}) +} + +// 6. address of an element of a container of schemas +func s6(b []byte) { + sliceDest := make([]models.CollectionSchema, 1) + _ = jsonx.Unmarshal(b, &sliceDest[0]) +} + +// 7. the result of a helper that returns a schema pointer +func s7(b []byte) { + _ = jsonx.Unmarshal(b, makeDest()) +} + +// 8. a json.Decoder, through an ALIASED import of encoding/json +func s8(r io.Reader) { + var decoderDest models.CollectionSchema + _ = jsonx.NewDecoder(r).Decode(&decoderDest) +} + +// 9. a short declaration with an inferred type +func s9(b []byte) { + shortDest := models.CollectionSchema{} + _ = jsonx.Unmarshal(b, &shortDest) +} + +// 10. a var declaration with an inferred type +func s10(b []byte) { + var inferredDest = models.CollectionSchema{} + _ = jsonx.Unmarshal(b, &inferredDest) +} + +// 11. a var whose inferred type comes from new(T) +func s11(b []byte) { + var newDest = new(models.CollectionSchema) + _ = jsonx.Unmarshal(b, newDest) +} + +// 12. a pointer taken from a schema already named +func s12(b []byte) { + var base models.CollectionSchema + aliasDest := &base + _ = jsonx.Unmarshal(b, aliasDest) +} + +// 13. models imported under an ALIAS (see aliased.go in this directory) + +// NOT a destination: the SOURCE argument mentioning a schema must not match, or +// every correctly-converted site in the tree reports itself as raw. +func notADestination(c models.Collection) { + var out models.CollectionSchema + _ = models.UnmarshalItemFieldSchema([]byte(c.Schema), &out) +} diff --git a/internal/server/bug2685_grandfathered_reserved_key_test.go b/internal/server/bug2685_grandfathered_reserved_key_test.go new file mode 100644 index 000000000..394d4913b --- /dev/null +++ b/internal/server/bug2685_grandfathered_reserved_key_test.go @@ -0,0 +1,312 @@ +package server + +import ( + "encoding/json" + "net/http" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-2685 — a collection schema that DECLARES a reserved metadata key. +// +// `validateNoReservedFieldKeys` (BUG-2674) stops a schema NEWLY declaring one, +// but GRANDFATHERS an existing declaration, so the shape survives in any +// workspace that had it before that gate landed. These tests reproduce that +// shape the only way it can exist — written through the store, which is the +// door a pre-gate row came through — and pin what the declaration does to +// item-field paths that consult a schema. +// +// The declaration is wrong in BOTH directions at once, which is why the class +// remedy is to ignore it rather than to refuse the write: a `text` declaration +// REFUSES the system's own value (a notes ARRAY) and ACCEPTS a caller's junk +// string. + +// grandfatherReservedKey rewrites a collection's STORED schema to declare def, +// bypassing the HTTP gate exactly as a legacy row does. Returns the collection. +func grandfatherReservedKey(t *testing.T, srv *Server, ws, collSlug string, def models.FieldDef) models.Collection { + t.Helper() + rr := doRequest(srv, "GET", "/api/v1/workspaces/"+ws+"/collections/"+collSlug, nil) + if rr.Code != http.StatusOK { + t.Fatalf("load collection %q: %d %s", collSlug, rr.Code, rr.Body.String()) + } + var coll models.Collection + parseJSON(t, rr, &coll) + + var schema models.CollectionSchema + if err := json.Unmarshal([]byte(coll.Schema), &schema); err != nil { + t.Fatalf("parse stored schema: %v (%s)", err, coll.Schema) + } + schema.Fields = append(schema.Fields, def) + raw, err := json.Marshal(schema) + if err != nil { + t.Fatalf("marshal schema: %v", err) + } + s := string(raw) + updated, err := srv.store.UpdateCollection(coll.ID, models.CollectionUpdate{Schema: &s}) + if err != nil { + t.Fatalf("store-level schema write: %v", err) + } + + // PREMISE, asserted rather than assumed: the HTTP door would have REFUSED + // this declaration, so the store write is genuinely reproducing a shape no + // current client can create — not just taking a shortcut to one it could. + rr = doRequest(srv, "POST", "/api/v1/workspaces/"+ws+"/collections", + map[string]interface{}{ + "name": "Gate Check " + def.Key, + "schema": `{"fields":[{"key":"` + def.Key + `","type":"text"}]}`, + }) + if rr.Code != http.StatusBadRequest { + t.Fatalf("the reserved-key gate is not in force: creating a collection declaring %q returned %d %s", + def.Key, rr.Code, rr.Body.String()) + } + return *updated +} + +func bug2685CreateItem(t *testing.T, srv *Server, ws, coll string, body map[string]interface{}) *httpResult { + t.Helper() + rr := doRequest(srv, "POST", "/api/v1/workspaces/"+ws+"/collections/"+coll+"/items", body) + return &httpResult{code: rr.Code, body: rr.Body.String()} +} + +type httpResult struct { + code int + body string +} + +// Leg A — the system's OWN value is refused. +// +// `pad item note` / `pad item decide` read the item's fields, append an entry +// with models.AppendImplementationNote, and PATCH the whole blob back. Against +// a legacy `implementation_notes: text` declaration that blob fails +// validateFieldType, so the collection's items cannot be annotated at all. +func TestBUG2685_LegacyDeclarationRefusesTheSystemsOwnWrite(t *testing.T) { + srv := testServer(t) + ws := createWSWithCollections(t, srv) + grandfatherReservedKey(t, srv, ws, "tasks", models.FieldDef{ + Key: models.ItemFieldImplementationNotes, + Type: "text", + }) + + got := bug2685CreateItem(t, srv, ws, "tasks", map[string]interface{}{"title": "Annotate me"}) + if got.code != http.StatusCreated { + t.Fatalf("create: %d %s", got.code, got.body) + } + var created models.Item + if err := json.Unmarshal([]byte(got.body), &created); err != nil { + t.Fatalf("decode created item: %v (%s)", err, got.body) + } + + // Exactly what `pad item note` sends. + fields, err := models.AppendImplementationNote(created.Fields, models.ItemImplementationNote{ + Summary: "a note", + Details: "the details", + CreatedAt: "2026-09-14T00:00:00Z", + CreatedBy: "agent", + }) + if err != nil { + t.Fatalf("AppendImplementationNote: %v", err) + } + + rr := doRequest(srv, "PATCH", "/api/v1/workspaces/"+ws+"/items/"+created.Slug, + map[string]interface{}{"fields": fields}) + if rr.Code != http.StatusOK { + t.Errorf("the system's own note write was refused by a legacy declaration: %d %s", + rr.Code, rr.Body.String()) + } +} + +// Leg B — the declaration must not DECIDE the answer. +// +// The class rule stated as something a test can refute: two collections, one +// carrying a legacy declaration and one not, must answer the same bytes the +// same way. Unfixed they do not — a legacy `implementation_notes: number` +// refuses the string that the undeclared collection stores, so whether a write +// succeeds turns on a FieldDef nobody may create any more. +// +// The control leg is what makes this evidence rather than an assertion about +// one path: it also pins the direction of the disagreement, so a future change +// that closes the create-mint hole (undeclared reserved keys are ACCEPTED at +// create — BUG-2850 chose accept-and-name, and `items/overrides.go` names the +// hole) fails this test loudly instead of silently inverting it. +// +// Note what is NOT asserted: that the junk is refused. Refusing is the other +// class and is out of this unit — see the BUG-2685 trail. +func TestBUG2685_ADeclarationDoesNotDecideWhetherAWriteSucceeds(t *testing.T) { + srv := testServer(t) + ws := createWSWithCollections(t, srv) + + body := map[string]interface{}{ + "title": "Same bytes", + "fields": `{"implementation_notes":"not a number and not a notes array"}`, + } + + // CONTROL first, before the declaration exists anywhere. + control := bug2685CreateItem(t, srv, ws, "ideas", body) + + grandfatherReservedKey(t, srv, ws, "tasks", models.FieldDef{ + Key: models.ItemFieldImplementationNotes, + Type: "number", + }) + declared := bug2685CreateItem(t, srv, ws, "tasks", body) + + if control.code != declared.code { + t.Errorf("a grandfathered declaration decided the outcome: undeclared collection answered %d, "+ + "declared collection answered %d\n undeclared: %s\n declared: %s", + control.code, declared.code, control.body, declared.body) + } + if control.code != http.StatusCreated { + t.Fatalf("premise broken: the control write did not succeed (%d %s), so the comparison above "+ + "could pass by both paths failing", control.code, control.body) + } +} + +// Leg C — a legacy REQUIRED declaration makes the collection uncreatable. +// +// Nothing in a create request mentions implementation_notes, and no client +// surface offers it, so the 400 names a field the caller cannot supply. +func TestBUG2685_LegacyRequiredDeclarationBlocksCreate(t *testing.T) { + srv := testServer(t) + ws := createWSWithCollections(t, srv) + grandfatherReservedKey(t, srv, ws, "tasks", models.FieldDef{ + Key: models.ItemFieldDecisionLog, + Type: "text", + Required: true, + }) + + got := bug2685CreateItem(t, srv, ws, "tasks", map[string]interface{}{"title": "Ordinary task"}) + if got.code != http.StatusCreated { + t.Errorf("an ordinary create was refused over a reserved key nobody typed: %d %s", got.code, got.body) + } +} + +// Leg D — the two PRESENTATION doors must not publish the declaration either. +// +// These are the "bootstrap / share presentation" row of the filed table, and +// the bootstrap one is why a sweep owes its vantage point: that decode does NOT +// land in a models.CollectionSchema — `bootstrapSchema` is a parallel struct — +// so the type-resolved pass that found the other 23 sites could not see it. It +// surfaced only by chasing a review finding about the OTHER door here. +// +// Bootstrap is the sharper of the two: the blob goes to every agent at session +// start and the skill treats `collections[].schema` as the write contract, so a +// surviving declaration is one every agent is told to write into. +// +// Two tests rather than two subtests on one server, deliberately: the share leg +// has to CREATE A USER for the link's created_by FK, and doing so closes the +// fresh-install no-auth window the bootstrap leg's unauthenticated GET depends +// on. Sharing a server made the second leg 401 — a failure about the harness, +// inside a test whose whole job is to be about the payload. +func bug2685PresentationWorkspace(t *testing.T) (*Server, string) { + t.Helper() + srv := testServer(t) + ws := createWSWithCollections(t, srv) + grandfatherReservedKey(t, srv, ws, "tasks", models.FieldDef{ + Key: models.ItemFieldImplementationNotes, + Label: "Implementation Notes", + Type: "text", + }) + return srv, ws +} + +// Codex round 2 raised the public payload as a response-shape change and +// proposed preserving the raw schema. The change IS the fix, and the reason is +// in the handler's own comment two lines above the decode: the schema is +// emitted "so the public viewer can render" the items. That is an item-field +// purpose, so an anonymous viewer would otherwise be handed a declaration +// telling it to render system metadata as an ordinary field. Pinned here so the +// contract change stays deliberate. +func TestBUG2685_PublicSharePayloadDoesNotPublishAGrandfatheredDeclaration(t *testing.T) { + srv, ws := bug2685PresentationWorkspace(t) + + // Minted through the store rather than the HTTP door: that door records + // created_by and the FK needs a real user, which this workspace (made by + // the no-auth fresh-install path) has none of. Nothing about the READ under + // test depends on how the link was minted. + wsRow, err := srv.store.GetWorkspaceBySlug(ws) + if err != nil || wsRow == nil { + t.Fatalf("load workspace: %v", err) + } + coll, err := srv.store.GetCollectionBySlug(wsRow.ID, "tasks") + if err != nil || coll == nil { + t.Fatalf("load tasks collection: %v", err) + } + owner, err := srv.store.CreateUser(models.UserCreate{ + Email: "bug2685-share@example.com", Name: "Share", Username: "bug2685-share", + Password: "pw-test-12345", + }) + if err != nil { + t.Fatalf("create owner: %v", err) + } + link, err := srv.store.CreateShareLink(wsRow.ID, "collection", coll.ID, "view", owner.ID, nil) + if err != nil { + t.Fatalf("mint share link: %v", err) + } + + rr := doRequest(srv, "GET", "/api/v1/s/"+link.Token, nil) + if rr.Code != http.StatusOK { + t.Fatalf("resolve share link: %d %s", rr.Code, rr.Body.String()) + } + var resolved struct { + Collection struct { + Schema struct { + Fields []struct { + Key string `json:"key"` + } `json:"fields"` + } `json:"schema"` + } `json:"collection"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &resolved); err != nil { + t.Fatalf("decode resolved share: %v (%s)", err, rr.Body.String()) + } + if len(resolved.Collection.Schema.Fields) == 0 { + t.Fatal("premise broken: the public payload carried no schema fields at all, " + + "so the assertion below cannot distinguish a strip from an absent schema") + } + for _, f := range resolved.Collection.Schema.Fields { + if models.IsReservedItemField(f.Key) { + t.Errorf("the public share payload published a grandfathered reserved-key declaration: %q", f.Key) + } + } +} + +func TestBUG2685_BootstrapDoesNotPublishAGrandfatheredDeclarationToEveryAgent(t *testing.T) { + srv, ws := bug2685PresentationWorkspace(t) + + rr := doRequest(srv, "GET", "/api/v1/workspaces/"+ws+"/agent/bootstrap", nil) + if rr.Code != http.StatusOK { + t.Fatalf("bootstrap: %d %s", rr.Code, rr.Body.String()) + } + var resp struct { + Collections []struct { + Slug string `json:"slug"` + Schema struct { + Fields []struct { + Key string `json:"key"` + } `json:"fields"` + } `json:"schema"` + } `json:"collections"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode bootstrap: %v", err) + } + var sawTasks bool + for _, c := range resp.Collections { + if c.Slug != "tasks" { + continue + } + sawTasks = true + for _, f := range c.Schema.Fields { + if models.IsReservedItemField(f.Key) { + t.Errorf("bootstrap published a grandfathered reserved-key declaration to every agent: %q", f.Key) + } + } + if len(c.Schema.Fields) == 0 { + t.Error("premise broken: the tasks schema came back with no fields at all, " + + "so the assertion above cannot distinguish a strip from an empty projection") + } + } + if !sawTasks { + t.Fatal("premise broken: bootstrap returned no tasks collection") + } +} diff --git a/internal/server/handlers_artifact_import.go b/internal/server/handlers_artifact_import.go index 2787c2c47..f097e4ab1 100644 --- a/internal/server/handlers_artifact_import.go +++ b/internal/server/handlers_artifact_import.go @@ -138,7 +138,7 @@ func (s *Server) handleImportArtifact(w http.ResponseWriter, r *http.Request) { } var schema models.CollectionSchema - if err := json.Unmarshal([]byte(coll.Schema), &schema); err != nil { + if err := models.UnmarshalItemFieldSchema([]byte(coll.Schema), &schema); err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Failed to parse collection schema") return } diff --git a/internal/server/handlers_bootstrap.go b/internal/server/handlers_bootstrap.go index 2621bec0d..082c315d3 100644 --- a/internal/server/handlers_bootstrap.go +++ b/internal/server/handlers_bootstrap.go @@ -189,11 +189,24 @@ func trimRedundantSchemaLabels(raw []byte) json.RawMessage { if err := json.Unmarshal(raw, &s); err != nil { return raw } + // BUG-2685: a grandfathered reserved-key FieldDef is dropped here as well. + // This is the one schema decode in the tree that does NOT land in a + // models.CollectionSchema — bootstrapSchema is a parallel struct — which is + // exactly why it was missed by the pass that converted the other 23 sites, + // and why the guard now enumerates schema-SHAPED types rather than one + // named type. The bootstrap blob goes to every agent at session start, so a + // declaration surviving here is a declaration every agent is told to write. + kept := s.Fields[:0] for i := range s.Fields { + if models.IsReservedItemField(s.Fields[i].Key) { + continue + } if s.Fields[i].Label == titleCaseLabel(s.Fields[i].Key) { s.Fields[i].Label = "" } + kept = append(kept, s.Fields[i]) } + s.Fields = kept out, err := json.Marshal(s) if err != nil { return raw diff --git a/internal/server/handlers_dashboard.go b/internal/server/handlers_dashboard.go index 79b85a3f1..5c70aa862 100644 --- a/internal/server/handlers_dashboard.go +++ b/internal/server/handlers_dashboard.go @@ -263,7 +263,7 @@ func buildDoneContextMap(collections []models.Collection) map[string]doneContext // Best-effort parse: a malformed schema or settings just leaves // the corresponding sub-struct zero-valued — the dashboard // continues to function with reduced done-detection fidelity. - _ = json.Unmarshal([]byte(c.Schema), &ctx.schema) + _ = models.UnmarshalItemFieldSchema([]byte(c.Schema), &ctx.schema) if c.Settings != "" { _ = json.Unmarshal([]byte(c.Settings), &ctx.settings) } diff --git a/internal/server/handlers_items.go b/internal/server/handlers_items.go index 11543e307..c43f41848 100644 --- a/internal/server/handlers_items.go +++ b/internal/server/handlers_items.go @@ -571,7 +571,7 @@ func (s *Server) handleListCollectionItems(w http.ResponseWriter, r *http.Reques var collSchema models.CollectionSchema if coll.Schema != "" { - _ = json.Unmarshal([]byte(coll.Schema), &collSchema) + _ = models.UnmarshalItemFieldSchema([]byte(coll.Schema), &collSchema) } if err := s.resolveParentFilter(r, workspaceID, ¶ms, collSchema); err != nil { writeError(w, http.StatusBadRequest, "bad_request", err.Error()) @@ -675,7 +675,7 @@ func (s *Server) handleCreateItem(w http.ResponseWriter, r *http.Request) { // Parse collection schema var schema models.CollectionSchema - if err := json.Unmarshal([]byte(coll.Schema), &schema); err != nil { + if err := models.UnmarshalItemFieldSchema([]byte(coll.Schema), &schema); err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Failed to parse collection schema") return } @@ -1268,7 +1268,7 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) { } var schema models.CollectionSchema - if err := json.Unmarshal([]byte(coll.Schema), &schema); err != nil { + if err := models.UnmarshalItemFieldSchema([]byte(coll.Schema), &schema); err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Failed to parse collection schema") return } @@ -1448,7 +1448,7 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) { return } var schema models.CollectionSchema - if err := json.Unmarshal([]byte(coll.Schema), &schema); err != nil { + if err := models.UnmarshalItemFieldSchema([]byte(coll.Schema), &schema); err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Failed to parse collection schema") return } @@ -3134,7 +3134,7 @@ func (s *Server) handleGetItemProgress(w http.ResponseWriter, r *http.Request) { ctx, cached := ctxCache[child.CollectionID] if !cached { if coll, cerr := s.store.GetCollection(child.CollectionID); cerr == nil && coll != nil { - _ = json.Unmarshal([]byte(coll.Schema), &ctx.schema) + _ = models.UnmarshalItemFieldSchema([]byte(coll.Schema), &ctx.schema) if coll.Settings != "" { _ = json.Unmarshal([]byte(coll.Settings), &ctx.settings) } diff --git a/internal/server/handlers_items_bulk.go b/internal/server/handlers_items_bulk.go index 9800f6131..0adac516e 100644 --- a/internal/server/handlers_items_bulk.go +++ b/internal/server/handlers_items_bulk.go @@ -510,7 +510,7 @@ func (s *Server) bulkFieldUpdate(r *http.Request, workspaceID string, item *mode return nil, &bulkOpError{message: "failed to load collection"} } var schema models.CollectionSchema - if err := json.Unmarshal([]byte(coll.Schema), &schema); err != nil { + if err := models.UnmarshalItemFieldSchema([]byte(coll.Schema), &schema); err != nil { return nil, &bulkOpError{message: "failed to parse collection schema"} } diff --git a/internal/server/handlers_items_open_children_guard.go b/internal/server/handlers_items_open_children_guard.go index 67c466711..b37db5228 100644 --- a/internal/server/handlers_items_open_children_guard.go +++ b/internal/server/handlers_items_open_children_guard.go @@ -212,7 +212,7 @@ func (s *Server) runOpenChildrenGuard(tx *sql.Tx, ctx openChildrenGuardContext) // inside the item-update tx, and a pool read from here needs a // second connection while that tx holds the first. if coll, cerr := s.store.GetCollectionAnyStateTx(tx, child.CollectionID); cerr == nil && coll != nil { - _ = json.Unmarshal([]byte(coll.Schema), &dc.schema) + _ = models.UnmarshalItemFieldSchema([]byte(coll.Schema), &dc.schema) if coll.Settings != "" { _ = json.Unmarshal([]byte(coll.Settings), &dc.settings) } diff --git a/internal/server/handlers_share_links.go b/internal/server/handlers_share_links.go index 3007ca769..968925694 100644 --- a/internal/server/handlers_share_links.go +++ b/internal/server/handlers_share_links.go @@ -624,7 +624,7 @@ func (s *Server) handleResolveShareLink(w http.ResponseWriter, r *http.Request) } if s := strings.TrimSpace(coll.Schema); s != "" { var schema models.CollectionSchema - if err := json.Unmarshal([]byte(s), &schema); err == nil { + if err := models.UnmarshalItemFieldSchema([]byte(s), &schema); err == nil { publicCollection["schema"] = schema } } diff --git a/internal/server/relation_hydration.go b/internal/server/relation_hydration.go index b8f42cf6e..0eb163146 100644 --- a/internal/server/relation_hydration.go +++ b/internal/server/relation_hydration.go @@ -1,7 +1,6 @@ package server import ( - "encoding/json" "net/http" "github.com/PerpetualSoftware/pad/internal/models" @@ -137,7 +136,7 @@ func (s *Server) relationSchemasForWorkspace(workspaceID string) (map[string]mod continue } var schema models.CollectionSchema - if err := json.Unmarshal([]byte(colls[i].Schema), &schema); err != nil { + if err := models.UnmarshalItemFieldSchema([]byte(colls[i].Schema), &schema); err != nil { // A schema that will not parse is a different defect, reported // elsewhere; hydration declines to be the second voice. continue diff --git a/internal/store/collections.go b/internal/store/collections.go index 303b7315f..ccad962a4 100644 --- a/internal/store/collections.go +++ b/internal/store/collections.go @@ -414,7 +414,7 @@ func (s *Store) ListCollections(workspaceID string) ([]models.Collection, error) for idx := range result { c := &result[idx] var schema models.CollectionSchema - if err := json.Unmarshal([]byte(c.Schema), &schema); err != nil { + if err := models.UnmarshalItemFieldSchema([]byte(c.Schema), &schema); err != nil { schema = models.CollectionSchema{} } var settings models.CollectionSettings diff --git a/internal/store/item_stars.go b/internal/store/item_stars.go index 31eee42eb..28ad884fd 100644 --- a/internal/store/item_stars.go +++ b/internal/store/item_stars.go @@ -187,7 +187,7 @@ func (s *Store) buildCollectionDoneContextMap(workspaceID string) (map[string]co return nil, err } var ctx collectionDoneContext - if err := json.Unmarshal([]byte(rawSchema), &ctx.schema); err != nil { + if err := models.UnmarshalItemFieldSchema([]byte(rawSchema), &ctx.schema); err != nil { // Don't drop the entry entirely — we still want an empty // context so the membership test falls back to defaults // rather than silently treating the item as non-terminal. diff --git a/internal/store/items.go b/internal/store/items.go index b2a923ff6..53a066db9 100644 --- a/internal/store/items.go +++ b/internal/store/items.go @@ -4334,7 +4334,7 @@ func scanCollectionDoneFilters(rows *sql.Rows) []collectionDoneFilter { continue } var schema models.CollectionSchema - if err := json.Unmarshal([]byte(schemaJSON), &schema); err != nil { + if err := models.UnmarshalItemFieldSchema([]byte(schemaJSON), &schema); err != nil { // Malformed schema → emit a default-fallback filter so the // collection's items still get evaluated against the status // column + global default terminals. This matches pre-TASK-604 diff --git a/internal/store/relation_links.go b/internal/store/relation_links.go index 99a29b99e..eff38174c 100644 --- a/internal/store/relation_links.go +++ b/internal/store/relation_links.go @@ -167,7 +167,7 @@ func relationKeysFromSchemaJSON(schemaJSON string) map[string]struct{} { return nil } var schema models.CollectionSchema - if err := json.Unmarshal([]byte(schemaJSON), &schema); err != nil { + if err := models.UnmarshalItemFieldSchema([]byte(schemaJSON), &schema); err != nil { return nil } keys := map[string]struct{}{} @@ -351,7 +351,7 @@ func (s *Store) GetRelationBacklinks(targetItemID, workspaceID string, limit, of if !known { byKey = map[string]string{} var schema models.CollectionSchema - if json.Unmarshal([]byte(schemaJSON), &schema) == nil { + if models.UnmarshalItemFieldSchema([]byte(schemaJSON), &schema) == nil { for _, def := range schema.Fields { if def.Label != "" { byKey[def.Key] = def.Label diff --git a/internal/store/reports.go b/internal/store/reports.go index 006320c92..f82bf7f1b 100644 --- a/internal/store/reports.go +++ b/internal/store/reports.go @@ -666,7 +666,7 @@ func (s *Store) resolveReportCollections(workspaceID string, opts ReportOptions) // Parse failures fall back to status + default terminals, matching // scanCollectionDoneFilters' tolerance. if c.Schema != "" { - _ = json.Unmarshal([]byte(c.Schema), &schema) + _ = models.UnmarshalItemFieldSchema([]byte(c.Schema), &schema) } if c.Settings != "" { _ = json.Unmarshal([]byte(c.Settings), &settings) diff --git a/internal/store/status_transitions_backfill.go b/internal/store/status_transitions_backfill.go index 119cfe237..95f7be8e7 100644 --- a/internal/store/status_transitions_backfill.go +++ b/internal/store/status_transitions_backfill.go @@ -93,7 +93,7 @@ func doneFieldKeyFromCollection(col *models.Collection) string { // (e.g. an in-tx read) resolve the key without a models.Collection. func doneFieldKeyFromSchemaJSON(schemaJSON, settingsJSON string) string { var schema models.CollectionSchema - if err := json.Unmarshal([]byte(schemaJSON), &schema); err != nil { + if err := models.UnmarshalItemFieldSchema([]byte(schemaJSON), &schema); err != nil { return "status" } var settings models.CollectionSettings