mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 15:05:40 +00:00
14cb97593f
* feat(store): referent resolution for relation values (TASK-2878)
PLAN-2857 U1, first slice: the rule itself, with no door wired to it yet.
`ResolveRelationReferents` canonicalises every `relation` value in a field
map to the target item's ID and reports the ones that cannot be resolved —
same workspace, and the collection the field DECLARES.
WHERE IT LIVES was forced, not chosen. `internal/items` is DB-free by
construction and keeps the shape check only. `internal/server` cannot own
it either: six of the eight coercion doors live there, but the eighth is
`store.migrateFieldsForCopy`, and `store` does not import `server`. Putting
it here is what lets the cross-workspace copy door and the preflight door
reach the SAME function instead of two implementations of one rule — those
two already carry a comment saying they sit in different packages and that
is how they drift unnoticed.
VISIBILITY IS NOT HERE, deliberately. "Can this requester see that item" is
request-scoped and needs the user, role and auth mode; the server layer
adds it via `checkItemVisible`, which already exists as the context-free
predicate for exactly this reason.
NO SLUG FALLBACK, which is a deliberate divergence from `ResolveItem`
(UUID, then ref, then slug). Found by a test failing rather than by
reading: "red" resolved, because it is the slug of the live Red colour. A
relation field's contract is that it stores an item ID; a slug is neither
an ID nor stable, so the same stored value could point elsewhere tomorrow.
Worse, "red" is exactly the free-text value the pre-U2 editor wrote into
these fields, so accepting it makes the corruption this unit exists to
stop indistinguishable from a legitimate write. The client refuses the
same match for the same reason (TASK-2868). Exact-TITLE resolution is U6.
Issues are reported in SCHEMA order, not map order, because the copy
preflight is one of the callers and is specified to be safe to call
repeatedly and return identical results.
Unresolvable values are left EXACTLY as supplied: the caller quotes them
back, and a half-canonicalised map would make a drop report lie about what
the source held.
Verified rather than asserted: both lookups exclude soft-deleted rows
(`ResolveItem` by contrast with `ResolveItemIncludeDeleted`; `GetItem` via
`getItemScanQ`, which appends `AND i.deleted_at IS NULL`). That is what
keeps "target was deleted" distinguishable from "never resolved" — the
read half U2 shipped.
* feat(server): refuse unresolvable relation values at the four write doors (TASK-2878)
PLAN-2857 U1, second slice: the doors that take CALLER-SUPPLIED field
values now refuse a relation value that does not name a live item in the
declared target collection — create, update (full fields), update
(fields_patch), and bulk update.
The server half adds the one thing the store resolver deliberately does
not: visibility. It folds into the SAME `not_found` reason rather than
getting its own, because "that item exists but you may not see it" is an
existence oracle, and this codebase has a standing rule against handing
one out.
Ordering at every door is after the shape check and after coercion, so one
bad value produces one error rather than two describing it differently,
and so the value is in its final form when it is resolved.
`fields_patch` examines only the keys the patch carries — the resolver
skips absent keys — so an unresolvable value already stored on an item is
not re-litigated by an update that does not touch it. That mirrors the
undeclared-key rule immediately above it, and it is what stops this
turning every edit of a legacy item into a failure.
Refusals use the ORDINARY `validation_error` shape with no new details
key. The MCP stdio transport classifies errors by matching CLI stderr
prose, so a structured field it cannot see would help nobody there, and a
new error shape is a contract change for every client.
Existing suites unchanged: internal/server ok (224.5s), internal/store ok
(258.0s), internal/items ok. Nothing in the tree was writing a bogus
relation value through these doors, which is what made this slice safe to
land before the per-door pins.
* feat(store): one migrate decision for all four carrying doors (TASK-2878)
PLAN-2857 U1, third slice, on the lead's refined ruling: PROVENANCE
decides, not which door you came through.
* SUPPLIED (an explicit `--field` override on a move or copy) is a write
like any other, so an unresolvable value REFUSES.
* CARRIED (everything the source item already held) was asserted by
nobody. `internal/items` has accepted any string for a relation all
along, so most stored values are legacy — refusing them would make
those items unmovable and uncopyable. Dropped and REPORTED instead.
And carried values are not all alike, which is the refinement that keeps
this from being one rule wearing four coats:
* WITHIN a workspace (move, bulk move) the targets are still here, so a
valid relation SURVIVES the move and only an unresolvable one is
dropped, through the `dropped_fields` channel BUG-2674 established.
* ACROSS workspaces (copy, and its preflight) every carried relation is
dropped WITHOUT a lookup: the value names a source-workspace row and
v1 excludes cross-workspace targets, so no amount of resolving in the
destination changes what it means. Reported as `referent_not_portable`
— the same reason `github_pr` uses, because it is the same fact about
the same kind of value.
`MigrateRelationReferents` is one function because the four doors sharing
it is the point, not tidiness: the preflight lives in `internal/server`
and the copy in `internal/store`, and the code already carries a comment
saying those two sit in different packages and that is how they drift
unnoticed. A preflight that says "carried" while the copy drops is one
request answered two ways.
Tests drive both provenances against both modes, because the same bad
value must be a drop when carried and a refusal when supplied — a suite
that only drove carried values would pass against a build that never
refuses anything.
* feat(server): the two same-workspace migrate doors resolve and report (TASK-2878)
PLAN-2857 U1: `handleMoveItem` and `bulkMoveCollection` now take their
relation decision from `store.MigrateRelationReferents` — the same
function the two copy doors will call, which is the point of it existing.
Within a workspace the targets are still present, so a correctly-related
item KEEPS its relation across a move; only an unresolvable value is
dropped, and it joins the `dropped_fields` report BUG-2674 established
rather than failing the move. Refusing carried values here would make
every legacy item permanently unmovable, and `internal/items` has accepted
any string for a relation all along, so "legacy" is most of them.
The bulk path carries no per-field overrides — only `status` — so every
relation value reaching it is CARRIED and nothing there can refuse. It
passes nil for `supplied` to say so, and keeps the refusal branch: it is
unreachable today and stops being a silent no-op the day that path grows
overrides.
internal/server ok (270.8s), internal/store ok.
KNOWN GAP, recorded rather than half-built: the two CROSS-workspace doors
are not wired yet, and the reason is a real constraint rather than
running out of road. `migrateCopyFields` is called from
`copyItemAcrossWorkspacesTx` with a transaction already open
(`s.db.Begin()` at the top of that function), so resolving a SUPPLIED
override there would issue POOL reads while holding a tx — the deadlock
shape this repo keeps a deterministic test for. The carried half needs no
lookup at all and is safe; the supplied half needs
`GetCollectionBySlugQ` / `GetItemByRefQ` so the resolver can run on the
tx's connection, which is exactly the `...Q` convention the store already
uses (`GetItemQ`, `getCollectionInWorkspaceTx`, `uniqueSlugQ`). Adding
those two is the remaining work, and it is what makes one function
genuinely serve all four doors.
* refactor(store): thread a Queryer through referent resolution (TASK-2878)
Preparation for the two cross-workspace copy doors, landed on its own
because it is independently correct and the doors are not.
`migrateCopyFields` runs inside `copyItemAcrossWorkspacesTx`, which opens
a transaction as its second statement. A resolver reading from the POOL
there would issue pool reads while holding a tx — the deadlock this repo
keeps a deterministic test for. So `ResolveRelationReferentsQ` and
`MigrateRelationReferentsQ` take the executor, following the store's own
convention (`GetItemQ`, `uniqueSlugQ`, `getCollectionInWorkspaceTx`); the
pool-backed names stay as one-line shims for the six wired doors.
Two small read helpers come with it. `collectionIDBySlugQ` returns the ID
only — the referent check compares `item.CollectionID`, and the full model
would pull in per-collection counts nothing here uses. `itemByRefQ` keeps
`GetItemByRef`'s fallback to a bare item-number lookup, because a relation
written as COLO-3 must keep resolving after its target collection is
renamed, which is exactly what BUG-2873 made possible.
internal/store ok (341.7s), vet and gofmt clean.
WHY THE COPY DOORS ARE NOT IN THIS COMMIT. They were written and building,
and I reverted them. Team CONVE-29 and the lead's condition both say the
copy pair lands WITH its pin — one case driving BOTH doors, asserting
identical drop-and-report for a carried relation and refusal for a
supplied override — and I measured 58.9% context against a 65% ceiling,
which is not enough for that pin plus the 270s server suite plus the
commit. Landing the behaviour change unpinned would have been worse than
landing nothing: the preflight and the store copy are the pair the code
already warns will drift unnoticed, so they are the last place to accept
an untested agreement.
The design is complete and on the trail: derive the carry mode from the
existing `items.MigrateScope` rather than a second flag, pass `tx` on the
store side and the pool on the preflight side, refusals through the copy's
existing validation-error channel, drops appended to `migrated.Dropped`.
* feat(copy): the two cross-workspace doors resolve referents, with their pin (TASK-2878)
PLAN-2857 U1, doors seven and eight. `migrateCopyFields` and
`handleCopyItemPreflight` now take their relation decision from
`store.MigrateRelationReferents` — the same function the four write doors
and the two move doors already call, which is the entire reason it exists.
The defect this closes: MigrateFields matches on key and TYPE, so a
same-named `relation` field carried a SOURCE-workspace item id across the
boundary and the preflight reported it as a clean carry. What landed in
workspace B was a value naming a row in workspace A — unrenderable, and
indistinguishable on read from a legitimate reference.
Provenance decides, as at the move doors. A CARRIED value on a
cross-workspace copy is dropped without a lookup (no id from A can mean
anything in B) and reported through the `dropped_fields` channel BUG-2674
established; a SUPPLIED override is an ordinary write and an unresolvable
one is refused, 400 validation_error on both doors, rendered by the same
`store.RelationIssuesMessage` so one refusal cannot acquire two phrasings.
`internal/server`'s `relationIssuesMessage` now delegates to it: the eighth
door refuses from inside `store`, so the sentence had to be reachable there.
Two things are threaded rather than re-derived, and both are load-bearing:
- The TRANSACTION, not the pool. `migrateCopyFields` becomes a method
taking a Queryer, and `copyItemAcrossWorkspacesTx` passes its `tx`. That
function has held a transaction since its second statement, so a pool read
from inside it can wait for a free connection while every pooled
connection is blocked on this transaction's locks — the starvation shape
BUG-2409 fixed for the attachment planner and this repo keeps a
deterministic test for. This is what the day-70 handoff named as the
reason these two doors were not wired with the other six.
- The MODE comes from the `scope` MigrateFields was already given, not from
a second boundary test. Two independent answers to "is this crossing a
workspace" is how one request gets migrated one way and validated the
other, and this path also serves a copy whose target IS the source
workspace, where relations resolve and survive exactly as on a move.
The destination workspace id is the resolution scope: a supplied override
is a write into B and must name something that exists there.
THE PIN, and why it is not a per-door table. These two doors sit in
different PACKAGES and the code at both sites says so is how they drift
unnoticed. A table with a row per door can be fully green while the two
disagree about one request, which is the defect rather than a gap in
coverage of it. So every case sends ONE body to BOTH endpoints:
- carried relation — must drop on both, and the preflight must say
referent_not_portable rather than the generic no_target_field, which is
false here (the destination DOES declare the key, so that answer sends
the reader to fix a schema that is fine);
- supplied + unresolvable — both refuse, same status, same code, both name
the offending field and value, and nothing is written;
- supplied + resolvable — the positive control, supplied as a REF so
resolution is visible in the result. Without it the first two legs are
equally consistent with "relations always fail".
Negative controls run, all three mutants BUILD-CHECKED first (a
non-compiling mutant produces no `--- FAIL` lines and reads as survived):
both doors unwired = DETECTED; preflight unwired alone = DETECTED; store
unwired alone = DETECTED. Each single-door mutant failing is the pin's
whole claim — neither door can be wired without the other.
CONVE-23 sweep: the preflight's LIMITATION comment said this gap belonged
in MigrateFields "for both callers at once". That is now false in its
prescription as well as its premise — `internal/items` is DB-free by
construction and cannot ask whether a string names a live item — so the
comment records where the fix actually went and what of it remains open
(`computed`, `terminal_options`, `unique_scope`).
Gates: internal/server ok 170.0s · internal/store ok 296.1s · internal/items
ok · go vet clean · gofmt clean · make lint 0 issues.
* test(server): the per-door x per-provenance table, and the defect it found (TASK-2878)
PLAN-2857 U1. `internal/store` already tests the resolver exhaustively, but
those tests call it DIRECTLY: they vouch for the component and say nothing
about whether any door is bound to it. A door that never calls the resolver
passes every one of them. This table is the binding claim — one leg per
(door, provenance) pair, driven through the handler a client reaches.
PROVENANCE IS THE SECOND AXIS BECAUSE THE ANSWER DEPENDS ON IT, not for
symmetry. A SUPPLIED value is the caller's assertion and an unresolvable one
is refused; a CARRIED value was asserted by nobody, and refusing it makes
legacy items un-updatable, un-movable and un-copyable — the failure this
unit would otherwise CAUSE while fixing another. Which provenance a door
sees is a property OF THE DOOR, and getting it wrong is invisible until a
legacy item meets it.
THE TABLE FOUND ONE, ON ITS FIRST RUN. `bulkFieldUpdate` merges the item's
STORED fields blob with the caller's `changes` before validating, and the
resolver was pointed at the MERGED map — so a bulk status move or
set-priority re-litigated every stored relation value and REFUSED the item.
An item carrying a legacy relation value had its status and priority frozen
by a field the operation never mentioned. Fixed the way the fields_patch
door already handles it: resolve only the keys the operation CHANGES, read
out of the coerced map so the value is final, written back so a supplied ref
is still canonicalised. Verified by prediction before the run and by the leg
failing against the unfixed code.
THE DISPATCH MUTANT, which is what makes the table's coverage a measurement
rather than a hope. Wire ONLY the two `extractParentLink` doors (update
fields, update fields_patch) and neuter the other six by swapping their
field-map argument for an empty one — types unchanged, so the mutant
compiles and its verdict means something:
build OK · failing legs: create (both), move (both), bulk move,
bulk update supplied, copy, preflight. Update and update-patch pass.
Exactly the six unwired doors, and only those. Then each door alone, eight
runs: every one detected by its own legs and no other door's. That is the
claim the table exists to make — each leg reaches its OWN door rather than
being satisfied by a neighbour's check.
ONE DOOR NEEDS A WEAKER INSTRUMENT, AND THE TEST SAYS SO. No bulk op puts a
relation key into `changes` — `op` is a closed list and the only field
values any of them set are `status` and `priority` — so that door's SUPPLIED
branch is unreachable from outside. The first mutant run proved it: unwiring
door 4 alone left every black-box leg passing. It gets a direct-call leg,
labelled as vouching for the FUNCTION and not for a binding that does not
exist yet, and kept for the same reason `bulkMoveCollection`'s refusal
branch is kept: the day the bulk path grows per-field overrides, the branch
must already refuse rather than be a silent no-op nobody notices is missing.
Every refusing leg has a resolvable counterpart. Without them the table is
equally consistent with a build that refuses every relation value.
Gates: internal/server ok 156.1s · go vet clean · gofmt clean · make lint 0
issues.
* feat(mcp): ToolSurfaceVersion 0.29, and the drop-reason renderer it exposed (TASK-2878)
PLAN-2857 U1. The bump, its documentation sweep, and the consumer this
change turned from a rare wart into a routine one.
THE BUMP, at 0.29 rather than 0.28. Rebasing onto main found b437cc58
(IDEA-2641, reminders) had ALSO taken 0.28 — a SEMANTIC collision, not a
textual one: two different contracts under one number, and a client pinning
"0.28" would have had no way to know which it got. Renumbered to 0.29, and
main's 0.28 entry kept intact ahead of it.
Also swept while resolving: main's CLAUDE.md carried v0.28 straight after
v0.26, because the v0.27 bump (BUG-2850) never reached that file. Both
places in CLAUDE.md now read v0.26 -> v0.27 -> v0.28 -> v0.29, and the
README changelog line gains the v0.28 entry it never got.
A BEHAVIOR bump on the 0.27 / 0.26 / 0.16 / 0.10 / 0.9 grounds — not on
0.28's, which was purely additive —
no tool name, action enum or parameter shape changed, and `pad_item` now
refuses calls it used to accept. A `relation` value must name a live item in
the collection the field declares; a caller writing a resolvable value sees
no difference, and one writing an unresolvable value was storing something
no surface could render, so the break is the fix. No escape hatch,
deliberately: unlike 0.10's `allow_draft` there is no legitimate call this
refuses, and the case with a real claim to leniency — a CARRIED value —
is already exempt by provenance rather than by a flag.
Full entry in internal/mcp/version.go. instructions.md and README.md follow
the constant because `internal/mcp`'s own drift gates require it; CLAUDE.md
does not, which is exactly why it drifted.
CLAUDE.md's `pad item move` / `pad item copy` reference also gains the
relation semantics, which is the part a reader of that file is most likely
to need and the part that just changed.
THE CONSUMER, which is the interesting half. `dropped[].reason` is a wire
enum with its renderer on the other side of a language boundary, and nothing
made the two meet. BUG-2674 added `referent_not_portable` server-side; the
TypeScript union and CopyItemDialog's `dropReason` switch never learned it,
so it fell through `default: return reason` and the dialog showed a user the
raw string. It went unnoticed because only `github_pr` produced it — and
this change makes it the reason for EVERY carried relation on a
cross-workspace copy, plus three more (`not_found`, `wrong_collection`,
`target_missing`). A latent defect going live because my emission made the
form routine.
So the reasons are ENUMERATED rather than left as literals:
`store.RelationIssueReasons()` owns the four the store decides,
`preflightDropReasons()` composes them with the five this package
originates, and the emission sites now use the constants. The new gate
requires every entry to have a union member and a switch case.
The gate is SCOPED to `dropReason`'s own body rather than searching the
component, because another switch matching `case 'not_found':` for an
unrelated purpose would satisfy it while the dialog still rendered the raw
enum — a guard passing on the wrong evidence. If that function is renamed
the test FAILS rather than silently passing on a body it can no longer find.
Negative controls, all DETECTED: remove the dialog's `referent_not_portable`
case; remove `wrong_collection` from the TS union; rename `dropReason`
(which checks the scoping guard's own premise).
What the gate does not claim: that a case EXISTS, not that its sentence is
good — no test judges that. It is also blind to a renderer handling a reason
the server never sends; that direction is a dead branch, the other is the
defect.
Gates: internal/server ok 162.8s · internal/mcp ok 13.6s · internal/store ok
192.2s · `npm run check` 0 errors (6 pre-existing warnings, unrelated files)
· go vet clean · gofmt clean · make lint 0 issues.
* fix: four codex round-1 findings — origin, visibility, bulk reporting (TASK-2878)
Codex round 1 named four; three were real P1s and I verified each in the
code before accepting it. Codex could not run anything ("Go could not
create its build cache because the workspace is read-only"), so every
finding here is a static read that I confirmed and pinned.
## A THIRD ORIGIN, not two
`items.MigrateFields` injects the DESTINATION schema's defaults for keys the
source item has nothing for. My classifier split on `supplied` versus
everything-else, so a destination default was filed as CARRIED — and on a
cross-workspace copy every carried relation drops without a lookup. The
destination's own default was discarded and reported `referent_not_portable`,
which is flatly false about a value the destination chose.
There are three origins: SUPPLIED (refuse on failure), CARRIED from the
source item (cross-workspace: drop as not-portable), and DESTINATION DEFAULT
(resolve against the destination in BOTH modes; drop with the resolver's own
reason on failure, because nobody in this request typed it). Telling the last
two apart needs the source field map, which all four migrate doors have as
`currentFields`, so it is now a parameter.
Empty values are skipped at every origin. An empty relation is a cleared
field, not a referent, and reporting it as dropped tells a user they lost
something they never had.
WHAT THE FIRST VERSION OF THIS TEST PROVED: nothing. The mutant that reverts
the classifier SURVIVED it. `ValidateFields` re-injects the default after my
resolver deleted the key, so the value comes back either way and
`StillDropped` filters the false report out — the end state is identical
unless the default is a REF. A UUID default is already its own canonical
form, so "resolved" and "dropped then re-injected raw" produce the same
bytes. With `PEOP-1` as the default the mutant is DETECTED, because only a
resolved default lands as the id.
## SUPPLIED OVERRIDES AT THE MIGRATE DOORS SKIPPED THE VISIBILITY CHECK
The four write doors go through `s.resolveRelationReferents`, which adds
`checkItemVisible` on top of the store resolver. The migrate doors called
`store.MigrateRelationReferents` directly — it is a store function and cannot
answer a request-scoped question — so their SUPPLIED half, which this unit's
own rule calls an ordinary write, resolved against the database alone. A
caller able to edit both collections could point a relation at an item they
cannot see.
The ROLE is the part worth getting right. For `move` it is `workspaceRole(r)`.
For copy and preflight it is the caller's role in the DESTINATION, and
`CrossWorkspaceAccess.Role` is exactly that — its own doc says never to
substitute `workspaceRole(r)`. So `resolveRelationReferents` now takes the
role explicitly (`resolveRelationReferentsAs`), and the new
`refuseInvisibleRelationOverrides` runs at all three doors with the right one.
At the copy it runs in the HANDLER, before the store call: a pre-write
refusal must not open a transaction to roll it back, and the preflight runs
the identical check — DR-6's "the preview IS the copy" only holds if both
doors refuse the same request.
## BULK COLLECTION MOVE DISCARDED FIELDS SILENTLY
`bulkMoveCollection` has populated `result.Dropped` since MigrateFields
existed and NOTHING read it — the only reference in the file was my own
append. BUG-2674 fixed the single-item door and left this one, so a bulk move
discarded values with no record anywhere. Pre-existing, and routing relation
drops into the same dead list is what made it mine to fix.
Reported on the activity row, same key, same joined-string shape and the same
BUG-2628 reason as `handleMoveItem`, filtered against the final map so the
report is true when written. Threaded as an out-parameter, deliberately: only
this branch produces drops, the caller needs them for ONE activity row per
item, and a third return value would put `nil` in fourteen unrelated returns.
## THE P2, AND WHAT IT IS NOT PINNED BY
`resolveRelationReferents` did `if item == nil { continue }` after the
visibility read — "treat a race as someone else's 404". It now refuses with
the same `not_found` the resolver would have given moments later. This whole
unit exists to keep a dangling referent out of the blob, and a target that
vanished mid-request is the one case where waving it through would have been
deliberate.
NO TEST. Reproducing it means deleting a row between two reads inside one
request, and a test that faked that would pin the fake. Stated here rather
than left to look covered.
## Counterfactuals
Every fix has a mutant that its own test detects, each build-checked first:
classifier reverted -> DETECTED (destination default); empty-skip removed ->
DETECTED; visibility helper neutered -> DETECTED at both the move door and
the copy/preflight pair; bulk-move report removed -> DETECTED.
Gates: internal/server ok 224.6s · internal/store ok 261.5s · internal/items
ok · internal/mcp ok 15.1s · go vet clean · gofmt clean · make lint 0 issues.
* fix: two codex round-2 findings — late defaults, UUID disclosure (TASK-2878)
Round 2 confirmed the three round-1 fixes and found two more. Both real,
both verified in the code before acceptance; codex still could not run
anything ("the read-only filesystem prevented Go cache/temp creation").
## THE DESTINATION-DEFAULT FIX ONLY COVERED HALF THE WAYS IN
The migrate doors resolve BEFORE they validate, and that order is
load-bearing: the required-field check has to see a value referent
resolution dropped, or a dropped value in a REQUIRED relation field would
store the item with the field absent instead of refusing. But
`ValidateFields` INJECTS schema defaults, so a default can land after the
resolver has finished. Two ways in, both now closed:
- a NULL OVERRIDE deletes the key and the default fills the hole, arriving
uncanonicalised — a `PEOP-1` default reached the row as the literal
string;
- a default the resolver DELETED as unresolvable is put straight back by
validation, and `StillDropped` then suppresses the warning about it.
Dropped, reported, restored, and reported as not-dropped.
`ResolveLateRelationDefaults` is a narrow second pass over exactly the keys
validation added. Reordering the two would have traded this defect for the
required-field one; a second pass costs one lookup in the rare case a
relation field declares a default and nothing otherwise. A default is
asserted by nobody, so an unresolvable one is DROPPED and reported, never
refused — the same disposition the main pass gives
RelationOriginDestinationDefault, which is what this is: the same origin,
arriving late.
THE SNAPSHOT IS TAKEN AFTER THE MAIN PASS, NOT BEFORE, and getting that
wrong is what the second sub-case caught. Snapshotting before the pass
treats a key the pass DELETED as already examined, so the late pass skips
the very value validation just put back — the arrangement that hid it. My
first version did exactly this and the second sub-case failed on it.
## A REFUSAL FOR AN INVISIBLE ITEM DISCLOSED ITS UUID
The store resolver rewrites a supplied ref into its target's id before the
visibility check runs, so a message built from the resolved value handed
back the canonical UUID of an item the requester may not see — confirming
both its existence and its identity. That is the existence oracle the
`not_found` collapse exists to prevent, reopened by the message. Every issue
this function raises now quotes what the CALLER sent.
## Prior findings, per codex
Override visibility: fixed. Bulk move dropped list: fixed. Nil-target race:
no dereference. Destination defaults: this commit.
## Counterfactuals
Neuter the late-default pass -> TestCopyEndpoint_LateInjectedRelationDefault
DETECTED on both sub-cases. Restore the canonical UUID in the message ->
TestCopyEndpoint_InvisibleRelationOverrideIsRefused DETECTED. Both mutants
build-checked first.
Gates: internal/server ok 346.3s · internal/store ok 351.3s · internal/items
ok · internal/mcp ok 22.9s · go vet clean · gofmt clean · make lint 0 issues.
Postgres green on the parent commit (store 534.5s, server 268.5s, private
container port 5481); re-running on this tree.
* fix: three codex round-3 findings — oracle, bulk defaults, required (TASK-2878)
Round 3 confirmed round 2's fixes and found three more. Codex still could
not execute anything, so all three were static reads I verified myself.
## `wrong_collection` DISCLOSED EXISTENCE
The store emits `wrong_collection` when a value names a LIVE item outside the
field's declared target, and the server's visibility layer skipped any key
that already carried an issue. So the message — "is not an item in
collection X" — told a caller the value EXISTS, distinguishably from the
`not_found` a nonexistent value gets. An existence oracle for anyone who
cannot see that item, and the exact shape the `not_found` collapse exists to
prevent.
Collapsed to `not_found` when the requester cannot see the target, and KEPT
otherwise: "you linked a task where a person belongs" is the useful half of
this reason, and blanket-collapsing would have passed the security leg while
destroying it. Both legs are in the test for that reason.
Needed one new store export, `ResolveRelationTarget`: deciding what to
disclose about a live item requires the item, and the resolver is the only
thing that maps a value to one under the no-slug rule.
## BULK STATUS / PRIORITY BYPASSED RELATION DEFAULTS
`bulkFieldUpdate` resolves only the keys `changes` names — correctly, since
re-litigating stored values would freeze legacy items — but `ValidateFields`
runs BEFORE it and INJECTS schema defaults. A defaulted relation was
persisted raw: never canonicalised, never checked against its collection.
The same late-arrival the migrate doors hit, reached by a different route,
and the narrow pass built for them covers it unchanged.
`dropped_fields` now rides every bulk op's activity row, not just the move
branch: a status or priority change can discard a relation default too, and
a drop nobody records is the defect BUG-2674 closed.
## A REQUIRED RELATION COULD END UP ABSENT AND REPORTED VALID
The late pass deletes a key AFTER validation passed, so nothing re-checked
required-ness: a required relation whose default did not resolve left the
item with the field absent, and the preflight reported `valid: true`.
Re-running validation is not the fix — it would re-inject the same broken
default. There is no valid value for that field, so the doors refuse:
`missing_required_fields` on move and bulk move, a FieldValidationError on
the copy, and on the preflight a `needs_value` row with `valid: false`. That
split is the one this pair has everywhere else — the preview says what is
wrong, the copy refuses.
## Counterfactuals
Remove the oracle collapse -> the visibility leg DETECTED. Remove the
bulk-update late pass -> DETECTED. Remove the required-relation refusal at
the copy door -> DETECTED. All build-checked first.
Gates: internal/server ok 352.1s · internal/store ok 358.7s · internal/items
ok · internal/mcp ok 21.0s · go vet clean · gofmt clean · make lint 0 issues.
Postgres green on the parent commit (store 596.7s, server 294.7s, private
container port 5481); re-running on this tree.
* fix: two codex round-4 findings — oracle race, null-source provenance (TASK-2878)
Round 4 confirmed round 3 and dropped from P1 to P2, which is what
convergence looks like. Both still real.
## THE ORACLE COLLAPSE HAD A HOLE, AND MY OWN COMMENT ASSERTED IT DID NOT
Round 3's fix collapses `wrong_collection` to `not_found` when the requester
cannot see the target. It needs the item to ask that, and when the second
lookup came back nil — deleted between the resolver's read and this one — it
did `continue` under a comment reading "vanished since; already the safe
answer".
That comment was wrong. Nothing had rewritten the reason, so the issue still
said `wrong_collection`, whose message tells the caller the value named
something a moment ago. The disclosure round 3 closed, reachable by a race.
Collapsed now.
Worth stating plainly because it is the same defect twice: I wrote a comment
asserting a state ("already safe") that the code did not establish, and I
wrote it in the fix for exactly that disclosure.
## A NULL SOURCE VALUE IS NOT A CARRIED ONE
The preflight marked any key PRESENT in the source's field map as
`from: "migrated"`. A source key holding `null` is present and carries
nothing: MigrateFields keeps the key, the relation pass skips a nil value,
and validation then treats it as missing and fills the DESTINATION's default
in its place. So the response reported the source as the origin of a value
the destination chose.
`from` is what a dialog uses to say "this came across" versus "this is the
destination's default", so naming the wrong one is the preview lying about
the thing it exists to report. Presence with a VALUE is the test now.
Not relation-specific — any nulled source field with a destination default
had it — so the guard is on the shared origin loop rather than beside the
relation pass.
## Counterfactuals
Remove the null guard from the origin loop ->
TestCopyEndpoint_NullSourceRelationIsNotReportedAsMigrated DETECTED
(build-checked first).
The oracle-race collapse has NO test, for the reason the vanished-target
refusal has none: reproducing it means deleting a row between two lookups
inside one request, and a test that faked the interleaving would pin the
fake. Both are listed in the PR body under "Not tested, and why".
Gates: internal/server ok 343.8s · internal/store ok 349.9s · internal/mcp ok
22.1s · go vet clean · gofmt clean · make lint 0 issues. Postgres green on
the parent commit (store 553.9s, server 297.1s, private container port 5481);
re-running on this tree.
* fix: codex round-5 finding — the other direction of the origin guard (TASK-2878)
One finding, P2, and it is the tail of round 4's fix.
Round 4 stopped labelling a null-source key `migrated`, because validation
treats null as missing and fills the DESTINATION's default in its place — so
`migrated` named the source for a value the destination chose. With NO
destination default there is nothing to fill it: the null is what carries,
and `default` names a value the schema never declared.
One guard, wrong in both directions, and the reason is the same in both:
"was the key present in the source" is not "where did the final value come
from". The label now follows the value:
- source value, non-nil -> migrated
- source null, destination default -> default
- source null, NO default -> migrated
`from` is what a dialog uses to say "this came across" versus "this is the
destination's default", so either error is the preview misreporting the thing
it exists to report.
## Counterfactual
Remove the round-5 branch so a null source always reads as `default` ->
TestCopyEndpoint_NullSourceWithoutDefaultIsNotReportedAsDefault DETECTED,
build-checked first. The mutant also confirms the key really is in `carried`
in that scenario — the test returns without asserting if it is not, so
without the mutant its green would have been consistent with a vacuous pass.
Gates: internal/server ok 288.8s · internal/store ok 302.4s · internal/mcp ok
13.7s · go vet clean · gofmt clean · make lint 0 issues. Postgres green on
the parent commit (store 626.4s, server 311.9s, private container port 5481);
re-running on this tree.
* fix: three codex round-6 findings, one premise corrected (TASK-2878)
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.
* fix: codex round-7 finding — the write doors had the same default hole (TASK-2878)
One finding, and it is round 6's defect in the two places round 6 did not
reach. `ValidateFields` assigns a schema default and `continue`s PAST its own
type check, and the resolver skips a non-string — so `default: 42` on a
relation field reached the blob unchallenged at CREATE and at a full `fields`
UPDATE. Only the default escapes; a value the caller supplies is type-checked
and refused like any other.
Closed with the same pass the migrate doors use, so there is one
implementation of "a default that arrived after the resolver" rather than
two.
## DROPPED, NOT REFUSED, AND REPORTED
Nobody in the request typed it. Refusing would make every write into that
collection fail on a schema defect its author has to fix elsewhere — the
same reasoning that makes a CARRIED value a drop rather than a refusal. A
REQUIRED relation is the exception and still refuses, because dropping there
would store the item with a required field absent.
Silence was not an option either, so item write responses gain
`warnings.dropped_fields`, additive and omitempty exactly like 0.27's
`undeclared_fields`, naming the discarded keys. `internal/mcp/version.go`'s
0.29 entry and CLAUDE.md's API section both carry it — the response shape is
documented in two places and a change to it owes both.
## Counterfactual
Remove the late pass from createItemChecked ->
TestRelationDoors_NonStringDefaultIsDroppedAndReportedOnWrite DETECTED,
build-checked first.
Gates: internal/server ok 470.1s · internal/store ok 463.5s · internal/mcp ok
29.6s (the four drift gates included) · internal/models ok · go vet clean ·
gofmt clean · make lint 0 issues. Postgres green on the parent commit (store
608.1s, server 343.1s); re-running on this tree.
* fix: codex round-8 finding — the import door forwarded half the warnings (TASK-2878)
One finding, P2. `createItemChecked` records dropped relation defaults
alongside undeclared keys; the artifact-import handler enumerated
`UndeclaredFields` and nothing else, so an import that DISCARDED a value said
nothing about it.
The shape is worth naming: a forwarding loop written per-member of a warnings
struct is a gap that widens every time that struct grows. `DroppedFields` was
added one commit earlier and this loop kept reporting only what it already
knew about. An import discarding a value is the same class of news as one
storing an unrecognized key, and worse — that value is gone.
## Counterfactual
Forward only undeclared fields again -> TestImportArtifactReportsDropped
RelationDefault DETECTED, build-checked first.
## A GATE FAILURE THAT WAS NOT A DEFECT, read from the elapsed time
`internal/store` reported FAIL at 600.017s while the Postgres suite was
running concurrently at load average ~10. That is the default `go test`
10-minute timeout, not a test failure: no `--- FAIL` line anywhere in the
output, and the same package under `-timeout=45m` on Postgres passed in
824.9s on the same tree. Re-run alone with an explicit timeout: ok 332.1s.
Recorded rather than quietly re-run, because "600.017s" is the whole
diagnosis and a second glance at the exit code would not have produced it.
Gates on this tree: internal/store ok 332.1s · internal/server ok 338.3s ·
internal/mcp ok 26.5s · internal/models ok · go vet clean · gofmt clean ·
make lint 0 issues. Postgres green on the parent commit: store 824.9s, server
703.4s (private container port 5481; both slow for the contention reason
above).
* refactor: classify by survival, not key presence — and the mutant survives (TASK-2878)
Codex round 9, one P1: the relation classifier calls a value CARRIED because
its key exists in the source's field map. `MigrateFields` matches on key AND
type, so an incompatible source value is DROPPED and the default loop then
refills the same key from the DESTINATION schema — the key is still in the
source map while the value in hand came from the destination. A
cross-workspace copy would discard that default as non-portable.
`CarriedSourceValues` narrows the source map to the values that actually
survived, using MigrateFields' own Dropped list, and all four migrate doors
plus the preflight's ORIGIN loop now use it. Both were answering "did this
value come across?" from a map that answers "did the source declare this
key?".
## THE MUTANT SURVIVES THIS TEST, AND THAT IS THE FINDING
Restoring the misclassification leaves the test green. I chased that rather
than accepting it, and the reason is that THE DEFECT HAS NO OBSERVABLE
EFFECT. Two independent downstream repairs erase it:
- the misclassified default is dropped, `ValidateFields` re-injects it, and
the late-default pass resolves it — landing the same bytes;
- the drop deletes the key's `origin` entry, and the carried loop's own
fallback for a missing origin is `"default"` — the same label.
Value and label agree on both builds. My first attempt to discriminate added
an origin assertion, which is the sharper instrument and STILL survives,
because of the second repair.
So this is a robustness change, not a bug fix: it stops the classifier
depending on two rescues to produce a right answer, and makes "carried" mean
what the word says. The test says so in its own comment and claims nothing
about the classification — it pins the OUTCOME, which is worth holding
however it is reached.
Recording it this way rather than shipping a green labelled "regression
test", because a test that passes on the broken build is exactly the fixture
CONVE-30 warns about, and the honest label is the whole value.
Gates: internal/server ok 226.0s · internal/store ok 247.9s · internal/mcp ok
13.8s · go vet clean · gofmt clean · make lint 0 issues. Postgres owed on this
tip.
* fix: codex round-10 finding — the write doors refused schema defaults (TASK-2878)
One finding, P2, and it is the asymmetry this unit's own rule forbids.
Validation injects a schema default BEFORE the resolver runs, and the
resolver treats the whole field map as caller input. So an optional relation
whose schema default names nothing turned every create and full-fields update
into a 400 — on a defect the caller neither caused nor can fix from that
call. The migrate doors have always dropped such a value and said so; the
write doors refused it.
`IssuesForCallerInput` keeps only the issues raised against keys that were
present BEFORE validation. Anything validation injected falls through to the
late-default pass, which drops it and reports it in
`warnings.dropped_fields` — a REQUIRED one still refuses, because dropping
there would store the item with a required field absent.
This is the third place the same rule has had to be stated: a value asserted
by nobody is dropped and reported, never refused. Carried values (round 1),
late-injected defaults (round 2), and now defaults at the write doors.
## Counterfactuals, both directions
Neuter the filter so defaults refuse again -> the new test DETECTED. Make the
filter drop everything so the door refuses nothing -> four tests DETECTED,
including the caller-supplied-bad-value legs. The second is the one that
matters: without it this fix could have quietly disabled the write doors'
refusal entirely and still looked green.
Gates: internal/server ok 330.5s · internal/store ok 339.1s · internal/mcp ok
20.5s · go vet clean · gofmt clean · make lint 0 issues.
* fix: two codex round-11 findings — default visibility, bulk status (TASK-2878)
## A SCHEMA DEFAULT NAMING A HIDDEN ITEM HANDED BACK ITS ID
Round 10 filtered the refusal set down to caller input, which correctly
stopped a dangling default refusing the write — and also removed the
VISIBILITY issues raised against those same keys. `ResolveLateRelationDefaults`
then re-resolved them in the store, which has no visibility layer by
construction, and the write's response carries the item's fields. So a caller
received the canonical id of an item they cannot see.
A fix from the previous round opening a hole in the round before it. The two
changes are individually right and the seam between them is where it went.
DROPPED, not refused, because the origin has not changed: the schema author
chose the value, and the caller can neither fix it nor be blamed for it. What
they must not get is the id. Reported through the same `not_found` every
other visibility failure collapses to, so "names something you may not see"
and "names nothing" stay indistinguishable.
Six doors wired. The seventh — the cross-workspace copy inside `store` — is
NOT, and cannot be from here: it has no request, and handing the store a
request-scoped visibility callback is the structural change IDEA-2886 files.
Stated rather than left as an apparent oversight.
## `req.Status` ON A BULK MOVE IS CALLER INPUT
That path merges exactly one field and passed `supplied=nil` on the grounds
that it carries no per-field overrides — true of every field except that one.
A destination schema may declare `status` as a relation, and then a value the
caller typed was classified as CARRIED: silently dropped instead of refused,
and never checked for visibility.
The refusal branch there had been written as "unreachable today, kept so it
stops being a silent no-op the day this path grows overrides". It was already
reachable when that sentence was written. **A branch nobody can reach is a
branch nobody checks** — the comment asserting unreachability is what stopped
anyone testing the classification that made it false.
## Counterfactuals
Neuter the late-default visibility helper -> DETECTED. Pass `nil` for supplied
on the bulk move again -> DETECTED, but only AFTER I wrote the test: the first
run of that mutant SURVIVED, because I had no case exercising a destination
whose `status` is a relation. The survivor is what said the test was missing.
Both build-checked. Every fix carries a control leg — the visibility one
asserts that the OWNER's identical create still lands the default, without
which it would pass against a build that dropped every default.
Gates: internal/server ok 403.8s · internal/store ok 406.8s · internal/mcp ok
24.7s · go vet clean · gofmt clean · make lint 0 issues. Postgres green on the
parent commit (store 524.8s, server 302.9s).
* refactor: one write-door relation path, and neutral wording for two reasons (TASK-2878)
## The web round, which had never had one
The TypeScript union and CopyItemDialog changed across three commits with
only the Go-side parity gate looking at them. First review round on those
files found two, both in the sentences a user actually reads:
`not_found` said "the item it refers to no longer exists". That reason is
ALSO what the server collapses a hidden target to — telling the two apart is
the existence oracle the collapse exists to prevent — so asserting
non-existence is wrong for half the cases and is a claim the response cannot
support. Now "could not be found".
`target_missing` said "the field declares no collection to link to". It is
emitted for two causes: no target declared, AND a declared target that is not
a collection in this workspace. The wording named only the first and
misdiagnosed the second as a schema that says nothing when it says something
broken. Now "has no valid collection to link to", and the store's own message
for the same reason was over-claiming identically and is fixed with it.
Neither is a code defect. Both are the write-up half of the same rule this
unit keeps enforcing: say what is true, not what is convenient.
## The consolidation, per the lead's CONVE-139 ruling
Create and full-`fields` update each spelled out the SAME four steps —
resolve the whole map, keep only the caller's issues, resolve the defaults
validation injected after the main pass, drop a default whose target the
caller cannot see. Round 10 was the third time one of those steps had to be
stated separately, which is what showed the shape. Extracted into
`resolveRelationsForWrite`, called twice, with the rule written once above it.
NOT extended to the migrate doors, and stated in the code rather than left to
look like an omission: they reach the same rule through
`store.MigrateRelationReferents`, which cannot call this — the visibility
layer is request-scoped by construction and `store` cannot import `server`.
Unifying the families needs a caller-supplied visibility predicate on the
store API. That is IDEA-2886, filed, with the shape it would take.
Gates: internal/server ok 359.4s · internal/store ok 365.5s · go vet clean ·
gofmt clean · make lint 0 issues · npm run check 0 errors. Postgres green on
the parent commit: store 543.9s, server 298.4s.
* fix: an overflowing issue ref resolved to a different item (TASK-2878)
First per-package round on `internal/store`, per the lead's split. One P1,
one P2 accepted with reasoning.
## OVERFLOW IS A WRONG ANSWER, NOT A BIG ONE
`parseItemRef` accumulates the numeric half into a machine int with no bound:
`num = num*10 + digit`, unchecked. So `COLO-18446744073709551617` wraps to 1
and names the item numbered 1. A caller-supplied ref canonicalising to a
DIFFERENT item is precisely the corruption relation referent validation
exists to stop — reached through the PARSER rather than through the lookup,
which is why every door-level instrument in this unit was blind to it.
Bounded at 1<<31, far above any real item number and below the wrap point, so
an out-of-range value is rejected rather than silently reinterpreted.
`parseItemRef` is shared, so this fixes every caller, not only the relation
resolver. It is outside this unit's eight doors and I fixed it anyway: my
resolver hands untrusted input straight to it, and the defect is a wrong
resolution, which is the unit's subject.
## The P2, accepted rather than fixed, with the reasoning
The resolver caches the target COLLECTION id and does not re-check it, so on
Postgres a concurrent `DeleteCollection` can archive that collection between
the lookup and the insert, leaving a relation into an archived collection.
Not fixed, for the reasons the copy path already records about its own
attachment-staleness window: on SQLite there is no window at all (BEGIN
IMMEDIATE holds the write lock, so the archive blocks until commit); on
Postgres closing it would mean putting the workspace advisory lock on every
collection writer; and the outcome of losing the race is benign and
self-describing — the read half renders a reference into an archived
collection honestly rather than as a live link. Same shape, same disposition,
recorded rather than left silent.
## Counterfactual
Remove the bound -> TestResolveRelationReferents_OverflowingRefDoesNotResolve
DETECTED, build-checked. The test carries a control leg asserting the honest
ref for the same item still resolves, so a failure is about the overflow and
not about the fixture.
Gates: internal/store ok 273.1s · internal/server ok 272.7s · go vet clean ·
gofmt clean · make lint 0 issues.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
* fix: three codex round-13 findings — the visibility helper checked the wrong set (TASK-2878)
First per-package round on `internal/server`. Three findings; two are defects
in the two rounds immediately before, which is what a per-package read buys
that a whole-diff read had stopped buying.
## THE VISIBILITY HELPER SKIPPED EXACTLY WHAT IT EXISTS TO CHECK
`dropInvisibleRelationDefaults` skipped keys "present before the late pass".
At the WRITE doors that set is approximately the caller's own values, which is
right. At the MIGRATE doors it also contains every default `MigrateFields`
injected — so the values the helper was written for were the ones it skipped,
and only the write doors were ever covered.
The predicate is now "not a destination default": the caller's own values and
the values carried from the source, excluded for two different reasons. A
caller's value is checked on its own door. A CARRIED value must never be
dropped for visibility at all, or an item referencing something the mover
cannot see becomes unmovable — the failure mode this unit has refused since
round 1.
Getting that predicate wrong was easy because both sets are "keys already in
the map"; they are the same shape and mean different things.
## A SIBLING LIST ADDED BESIDE A CHECK IS NOT COVERED BY IT
The required-relation check was written for `lateDropped`. The visibility
drops were added beside it one round later, and nothing extended the check —
so a REQUIRED relation whose default the caller cannot see was deleted after
validation had passed, leaving the field absent and the write reported valid.
Same shape as the round-3 finding it repeats: deleting a key after validation
means nothing re-checks required-ness, whichever list recorded the deletion.
## LOOKUP ERRORS WERE ECHOED VERBATIM
`relErr.Error()` went into the response body at create and at three bulk
sites. Those errors can carry SQL and driver detail. Replaced with a fixed
sentence; the real error still reaches the log.
## Counterfactuals
Skip destination defaults again -> DETECTED at the create door AND the move
door. Remove the required check on the visibility drops -> DETECTED, but only
after I wrote the test: the first run SURVIVED, and the survivor is what said
the case was missing. Both build-checked.
Every visibility test carries a control leg asserting the OWNER's identical
request still lands the default, without which they would pass against a build
that dropped every default.
Gates: internal/server ok 305.6s · internal/store ok 317.2s · internal/mcp ok
15.8s · go vet clean · gofmt clean · make lint 0 issues. Postgres green on the
parent commit: store 506.6s, server 269.8s.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
* fix: two codex round-14 findings, two recorded unfixed (TASK-2878)
Second `internal/server` round. Four findings: two fixed here, two recorded
with reasons rather than half-done at the end of a run.
## A NIL IS NOT A VALUE — third time in this unit
`notDefaultKeys` counted every key present in the supplied or carried map,
nil included. `ValidateFields` treats a present-but-nil key as ABSENT and
injects the destination default in its place, so a nil counted the key OUT of
exactly the visibility check that injected default needs.
Rounds 4 and 5 were the same distinction in the origin label; this is the
same mistake in a set-membership test. The shape worth carrying: **"the key is
in the map" and "the map has a value for the key" are different predicates,
and the second is almost always the one meant.**
## A VANISHED TARGET LEFT A DANGLING CANONICAL ID
The visibility helper did `continue` when its lookup came back nil — leaving
the resolved id in the map and skipping the required-field handling below.
Now dropped like any other default that does not stand.
That is the THIRD time a `continue` on a nil lookup has been the defect
(rounds 1 and 4 were the other two), each in a different function, each
written by me while fixing something else.
## Recorded, not fixed
**The mutating copy still applies no visibility check to destination
defaults.** The handler checks explicit overrides only; the defaults are
injected inside `store` during the copy transaction, and the handler cannot
know which without duplicating `MigrateFields`. Closing it needs the
caller-supplied visibility predicate on the store API — IDEA-2886, filed,
with the shape it would take. Stated here so the gap is inherited as a known
one rather than found again.
**Two bulk error sites still echo `err.Error()` verbatim** (item resolution
and the visibility gate in `handleBulkItems`). Both PRE-DATE this unit and
cover every bulk op, not relations — changing them changes messages for all
of them and belongs with whoever owns that door.
## Counterfactuals
Count nils as carried again -> DETECTED. Both fixes build-checked; the
vanished-target arm has no test for the reason the other two race arms have
none, and joins them in the PR body's "Not tested, and why".
Gates: internal/server ok 434.4s · internal/store ok 438.4s · go vet clean ·
gofmt clean · make lint 0 issues. Postgres green on the parent commit: store
713.6s, server 375.8s.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
* feat: artifact import CARRIES junk relation values (TASK-2878, Dave's ruling)
Dave, day 57, on the write posture: "all of that sounds like exactly what we
want. we should ensure import is allowed as I would want to explicitly allow
import of the junk to avoid breaking import."
On this branch artifact import routed through `createItemChecked` and was
therefore REFUSING any artifact whose relation value did not resolve — which
is every artifact written before referent validation existed. That is a live
break introduced by this unit, not a pre-existing one.
## IMPORT IS A CARRY DOOR
An artifact was written elsewhere, possibly years ago; the person importing it
did not choose its field values and cannot fix them from the import call. So
`relationPosture` splits the two callers of `createItemChecked`: create
refuses, import carries.
NOT the migrate doors' carry, and the difference is the point. Those DROP what
they cannot resolve, because the value has no home in the destination. An
import's value has a home — the artifact IS the record — so it is stored
verbatim and REPORTED, through a new `warnings.unresolved_relations`. Kept is
not the same as unreported; an import that quietly changed what it imported
would be the worse failure.
Additive and omitempty, like `undeclared_fields` and `dropped_fields` before
it. Distinct from both: `dropped_fields` means the value is gone, and
`undeclared_fields` is about the KEY rather than what it points at.
The door count in the PR body becomes 4 write / 6 migrate-or-carry.
## The fixture was unreachable, and the file already said why
My first version declared an invented relation key and asserted a carry that
`artifact.Decode` had already dropped: the artifact FORMAT decides which keys
reach the field map at all (FieldKeysForKind), so a key the format does not
know never gets near this door. It has to be a CANONICAL artifact key that the
DESTINATION declares as a relation — `role`, here.
The warning about this shape is in the same test file, from BUG-2850's round
10, and I walked into it anyway. Recorded in the test so the next person gets
it from the fixture rather than from the failure.
## Not pinned, and where to pick it up
The OTHER import door — `store.ImportWorkspace` — writes raw rows and does not
go through `createItemChecked`, so it already carries and this change cannot
have altered it. It is unpinned only because I am at the context bar. Model it
on TestImportWorkspace_CoercesTitles in
internal/store/items_title_validation_test.go, which builds the export blob
this needs.
## Counterfactual
Flip artifact import back to `relationsRefuse` ->
TestImportArtifactCarriesUnresolvableRelationValue DETECTED, build-checked.
Gates: internal/server ok 290.0s · internal/store ok 305.2s · internal/models
ok · go vet clean · gofmt clean · make lint 0 issues.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
* test(store): pin the workspace-import carry door for unresolvable relation values (TASK-2878)
Checkpoint 12 left this open: Dave's import ruling was implemented for the
ARTIFACT import door (relationPosture splits create-refuses from
import-carries) but store.ImportWorkspace — the other import door — was
unpinned. It writes raw rows and never goes through createItemChecked, so
referent validation cannot have reached it. That is an argument the behaviour
is unchanged, not evidence it is correct or that it will stay: the carry is a
ruling, and an unpinned ruling is one refactor away from turning every archive
written before referent validation existed into a hard import failure.
The question the test answers, stated before the result (CONVE-30): does
ImportWorkspace store a relation value whose referent is not in the bundle
VERBATIM — neither refusing the import nor dropping the value — while still
remapping the relation values it CAN resolve?
The resolvable leg is a CONTROL, not decoration. "Carried verbatim" and "never
processed" produce byte-identical output for the junk rows, so without a leg
whose expected output DIFFERS from its input, a build that deleted the
second-pass remap outright would pass every carry assertion (CONVE-30
instance 5, day 71: when a test asserts a TRANSFORMATION happened, pick an
input whose output differs from it).
Mutation matrix — three mutants, each BUILD-CHECKED (go build ./internal/store
clean) and each killed by a named `--- FAIL:` line, export.go restored from a
backup file and re-verified byte-identical afterwards:
M1 import REFUSES an unresolvable relation value
-> FAIL relation_referents_test.go:513, the must-SUCCEED assertion
M2 import DROPS unresolvable relation values from the field map
-> FAIL both carry subtests at :567; the control leg still PASSED,
which is what shows the two legs discriminate different faults
M3 second-pass remapFieldIDs removed
-> FAIL relation_referents_test.go:550, the control leg only
Fixture note carried in the test: the exported ids deliberately share no
common prefix, because remapFieldIDs rewrites by strings.ReplaceAll over every
id in the bundle — an unresolvable value CONTAINING a resolvable one
("old-color-1" inside "old-color-10") would be partially rewritten and the
carry assertion would fail for a reason unrelated to the rule under test.
Gates on this tip: internal/store ok 327.9s (SQLite), go vet clean, gofmt
clean. Postgres not yet run on this tip — stated as a boundary, not implied
currency; it is the next step on the successor list.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
* fix: three codex round-15 findings — two real, one unobservable (TASK-2878)
Round 15 (internal/server) returned four findings. One re-reported the known
IDEA-2886 gap, already recorded unfixed. The other three are handled here, and
two of the three were only settled by trying to kill them.
1. IMPORT SKIPPED THE DEFAULT CLEANUP (real, fixed, pinned). Step 2 of
resolveRelationsForWrite returned early whenever the caller's values had
issues — correct while every caller of that path REFUSED, since steps 3 and
4 only prepare a field map nobody stores. Checkpoint 12's carry posture
added a caller that does not refuse, and the early return stayed: an
artifact holding one junk relation skipped the late-default pass AND the
default-visibility drop, stored whatever the destination schema injected,
and reported none of it. The early return is now posture-aware; the carry
report is threaded through the remaining steps rather than lost.
This is the fourth time in this unit that one of my own fixes opened the
next round's defect, and the first where the interval was a day.
Mutant: restore the unconditional early return -> FAIL, warnings list
`role` only and `owner_ref` is stored raw as 42.
2. LATE DEFAULTS BYPASSED THE VISIBILITY COLLAPSE (real, fixed, pinned).
store.ResolveLateRelationDefaults cannot know who is asking, so every issue
it returns carries the raw reason, and every door renders those into a
caller-visible message. `wrong_collection` is the one reason that names a
LIVE item, so a schema default pointing at an item the caller cannot see
announced that it EXISTS — the oracle round 3 closed on the main pass,
reopened through the door round 10 added. Collapse hoisted into
collapseInvisibleRelationIssues and applied at ALL FIVE late-default sites,
not the one the reviewer named (CONVE-18).
Mutant: remove the collapse at the move door -> FAIL, the 400 reads
`"LV-2" is not an item in collection "people"` to a caller who cannot see
LV-2. Control leg included: a caller who CAN see the target keeps the
specific reason, so collapsing everything to not_found fails too.
3. STALE CARRIED-SOURCE SET (kept as robustness, NOT a bug fix, documented).
Reviewer named the preflight. Grepping the class found three doors with the
shape — and the mutants say only one of them could ever have been wrong,
and even that one is unobservable:
- move and bulk move fold their relation drops into result.Dropped and then
recompute CarriedSourceValues INLINE at the visibility call, so they read
the already-extended list. Restoring the stale form leaves their tests
green because there is nothing there to break. Both edits REVERTED.
- the preflight does hand its visibility call a variable captured before
that append, but restoring it leaves every test green too: a default
whose target the caller cannot see is already collapsed to not_found by
the MAIN pass and dropped before this check runs. Probed on both builds —
the owner's preflight discloses the default's id, a restricted editor's
reports owner_ref dropped as not_found either way.
Kept because carriedSource should mean what its name says at every use
rather than being correct at three uses and stale at the fourth because two
later passes repair it. The disposition is in the helper's doc comment.
A regression test was WRITTEN for this and then DELETED: it passed against
the unfixed build, and a test that cannot fail is worse than none — it reads
as a guard while guarding nothing. CONVE-18 asks for the population of a
class; it does not license assuming every member is defective.
Gates: build, go vet, gofmt clean on this tip. The full SQLite suite
(server/store/items/mcp) was STILL RUNNING when this was committed and its
result is NOT claimed here. Postgres has NOT been re-run since 21dfb115.
Both are the successor's first two steps.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
* revert: drop the preflight carried-source robustness change (TASK-2878)
Lead ruling, day-57 02:4xZ: drop round-15 finding 3 from a119cf39 in one
commit. It shipped as a documented robustness change whose mutants survived at
every door, with no regression test, because the test written for it passed
against the unfixed build and was deleted.
The ruling's reasoning, which I agree with: a change with no observable
behaviour and no test is not robustness, it is prose in code. The instrument
answered "not a defect", so there is nothing to fix, and CONVE-30 says the
answer is only as wide as the question I asked — I asked whether the stale set
was reachable, got "no" at three doors and "no, unobservably" at the fourth,
and then shipped a fix anyway on a naming argument.
Reverted, exactly and only:
- handlers_items_copy_preflight.go: the visibility call takes `carriedSource`
again, the pre-relation-drop snapshot.
- internal/server/relation_referents.go: `carriedAfterRelationDrops` and its
50-line doc comment removed.
- internal/store/relation_referents.go: `RelationIssueKeys` removed — it had
exactly one caller, the helper above, and is dead without it.
Findings 1 and 2 from a119cf39 are UNTOUCHED and still pinned. Verified by
diffing against the parent rather than by reading this diff: against 21dfb115
the preflight now shows only the `collapseInvisibleRelationIssues` call
(finding 2), and internal/store/relation_referents.go is byte-identical to the
parent.
The finding stays recorded — here and on TASK-2878's trail — as REPORTED, NOT
A DEFECT, CLASS-CHECKED, so round 16 and later stop re-reporting it. The class
grep and both probe legs are in a119cf39's message, which remains the record of
what was measured.
Gates on this tip: build, `go vet` (server+store), `gofmt -l internal/` all
clean. `go test ./internal/server -run 'TestRelationDoors|TestImportArtifact'`
ok 10.479s — the pins for findings 1 and 2, run because this commit touches
the file one of them lives in. The full SQLite suite and Postgres are owed on
this tip and are NOT claimed here.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
* fix: four codex round-16 findings, all four real and pinned (TASK-2878)
Round 16 (internal/server) returned five findings. Four are real, confirmed by a
test that FAILS against the build without its own fix and passes with it; the
fifth is a true observation whose proposed remedy closes nothing, and is
recorded rather than fixed. Every test ships with an owner control leg, so none
of them passes against a build that simply refuses or drops everything.
1. BULK MOVE NEVER RAN THE SUPPLIED-HALF VISIBILITY CHECK (real, fixed).
Round 11 established that `req.Status` on a bulk collection move is CALLER
INPUT and wired `suppliedByCaller` so the store classifier would REFUSE an
unresolvable value rather than drop it. It did not carry across the other
half of the supplied contract: the store resolver cannot answer a
request-scoped question, which is why the single move door, the copy and the
preflight all call refuseInvisibleRelationOverrides first. Bulk move was the
only one of the three MigrateRelationReferents call sites that never did.
So a caller who cannot see the relation's target collection could name a
live item in it and have the value stored, receiving its canonical id back.
Mutant: remove the refuseInvisibleRelationOverrides block -> FAIL, "a caller
who cannot see People pointed a relation at one of its items:
21c93e0b-8340-423c-a528-4eb78340bbf5".
2. THE PREFLIGHT'S CARRIED DROPS NEVER PASSED THE VISIBILITY COLLAPSE (real,
fixed). Round 15 hoisted the collapse so ResolveLateRelationDefaults' issues
stop naming live items to callers who cannot see them.
MigrateRelationReferents returns its issues down a SECOND path —
`relationDropReason`, rendered straight into `fields.dropped[].reason` — and
that path was never collapsed. `wrong_collection` is the reason that names a
LIVE item, so a hidden-but-live target reported differently from a value
naming nothing.
Class: this is the ONLY site where a MigrateRelationReferents drop reason
reaches a caller. Move and bulk move report dropped KEYS and no reasons, so
the class is one site, not five, and the grep says so rather than the
assumption.
Mutant: remove the collapse -> FAIL, "a LIVE hidden item reports
'wrong_collection' and a nonexistent value reports 'not_found'".
3. THE STALE CARRIED SET IS REAL AFTER ALL, AND ROUND 15's CLEARANCE WAS MY
INSTRUMENT'S FAULT (real, fixed, and it reverses ace2d3f5).
Round 15 reported this; every mutant survived; it shipped as a documented
robustness change; the lead ruled it out as unobservable and I reverted it in
ace2d3f5. The ruling required a concrete request under which the two sets
differ before it could be re-reported. Round 16 supplied one, and the test
fails against the unfixed build:
"carried":[{"key":"owner_ref","type":"relation",
"value":"a8fdfa7a-3487-4acb-9ad4-53b67cd96a19","from":"default"}]
with valid:true and no dropped row — a live item in a collection the caller
cannot see.
WHY EVERY MUTANT SURVIVED, which is the part worth keeping: my probe used a
source value that RESOLVED. The relation pass never drops a resolvable
value, so the pre-drop and post-drop carried sets were identical BY
CONSTRUCTION and the probe could not have failed. That is the same defect as
the fixture I recorded yesterday — two outcomes with identical bytes — with
the instrument being a probe rather than a test, which is why the CONVE-30
habit did not catch it. The discriminating shape is a non-nil DANGLING
source value plus a required destination default naming a hidden item.
The fix is NOT the one that was dropped. That was 71 lines across three
files with a new exported store helper. This is one expression at the call
site — store.CarriedSourceValues(currentFields, migrated.Dropped) — because
the loop above already extended migrated.Dropped with the relation pass's
drops. No new export, no helper, and it makes the preflight agree with move
and bulk move BY CONSTRUCTION rather than by argument, which is what the
lead's ruling was right to object to in the first version.
4. A NULL OVERRIDE DID NOT CLEAR A STALE NON-NIL CARRIED KEY (real, fixed).
`notDefaultKeys` learned in round 14 that a nil value is not a value, and
skipped nils PER MAP. A caller who nulls a key whose STORED value is non-nil
still had it counted — out of `carried`, on the strength of a value the
request had just discarded — so the destination default injected in its
place was exempted from the visibility check. The relation pass does not
rescue this: an override of nil leaves nothing to resolve, so the key is
never dropped and never leaves the carried set. The existing null-source
test cannot reach it; that fixture has no stored value to go stale.
Fixed at the shared helper, so move, bulk move and preflight get it at once.
Mutant: drop the explicit-nil-in-supplied clause -> FAIL, the move stores
and returns 550d947c-5970-4d61-9db2-d8e22c8a25a1 to a caller who cannot see
People.
5. CANONICALISING A CARRIED REF DISCLOSES A HIDDEN TARGET'S UUID (true
observation, NOT fixed, and the reason is a measurement rather than a
judgement). A carried ref naming a live item in a hidden collection
canonicalises to its UUID and survives; a dangling one drops. The caller can
therefore tell the two apart, and the reviewer asked for the response to
redact the UUID.
Redacting the response closes nothing. The canonical UUID is written into
the stored blob of an item the caller CAN read, so the same bytes come back
from the ordinary read door: probed as the same restricted editor,
GET of the moved item returned 200 with the UUID present. A response-only
fix would move the disclosure one request to the left.
The remedies that would actually close it are not this unit's to make: stop
canonicalising carried values, or drop carried values by visibility — and
the second is precisely the data destruction this unit's carry rule exists
to avoid, since it would silently delete a valid relation because the MOVER
cannot see its target. Recorded as an open design question alongside
IDEA-2886.
Gates on this tip: build, go vet, gofmt clean;
go test ./internal/server -run 'TestRelationDoors|TestCopyEndpoint|TestCopyPreflight|TestImportArtifact'
ok 44.309s. The full SQLite suite and Postgres are owed on this tip and are NOT
claimed here.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
* 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
* fix: the remap must preserve bytes; round-18 web findings (TASK-2878)
Three changes: one is a regression I introduced two commits ago and the full
suite caught, two are codex round 18 (web + internal/models).
1. remapFieldIDs PRESERVES EVERY BYTE IT DOES NOT DELIBERATELY CHANGE.
3e7a6751 fixed the substring-corruption defect by unmarshalling the fields
blob, walking it, and re-marshalling. That fixed the defect and broke
TestImportRepairsANULInsideAFieldsBlob: re-encoding rewrites the WHOLE blob,
so a stored `�` ESCAPE came back as the literal replacement CHARACTER.
stored fields = "{\"note\":\"x<U+FFFD>y\"}", want "{\"note\":\"x\\ufffdy\"}"
The rule I broke is one my own harness memory already carries in another
costume: an escape sequence and the character it denotes are not the same
artifact, and a round-trip through a decoder silently converts one into the
other. A function whose job is to substitute ids must not be the thing that
re-encodes everything else.
Now a textual substitution of the QUOTED JSON token — `"c-1"` does not occur
inside `"c-10"`, which is the whole-value property the fix needed, and it
still reaches ids inside arrays for a multi-valued relation. Untouched bytes
are untouched.
WORTH KEEPING: my four targeted tests all passed on the broken build. Only
the full SQLite suite failed, in a test named for NUL handling that has
nothing to do with relations, in a package I was not editing. This is the
argument for running the whole gate rather than the tests I judged relevant
— the fix and the test that catches it were three directories apart.
Mutant: replace the quoted token with the raw id -> FAIL, the dangling value
is rewritten. Both the prefix-collision test and the NUL test are green on
this tip.
2. THE `referent_not_portable` MESSAGE ASSERTED SOMETHING THE RESPONSE DOES NOT
SAY (codex round 18). The dialog rendered "it points at something in the
source workspace". That reason is emitted for EVERY carried cross-workspace
relation WITHOUT resolving the target, so the value may name a live item, a
deleted one, one the caller cannot see, or nothing at all — and `github_pr`
reaches the same reason, where the referent is not in any workspace. The
sentence claimed both existence and location.
Now "this reference cannot be carried to the destination".
Same class as round 12's `not_found` overclaim, and the neutral-wording
comment explaining why THAT one is careful sits four lines below the one
that was not. A rule written next to its own exception is easy to read as
already applied everywhere it belongs.
SHIPS WITHOUT A TEST, named here rather than left looking covered: the
reason-to-message mapper is inline in the .svelte component and not
exported, and there is no test file for the dialog at all. Extracting it is
a refactor this unit should not take mid-flight — filed as an idea instead,
because two separate review rounds have now found defects in this one
unexported function and nothing can regression-test either fix.
3. THE WEB `Item` TYPE NOW MIRRORS `models.ItemWriteWarnings` (codex round 18).
The Go item has carried `warnings` on create/update responses since
BUG-2850, and TASK-2878 added `dropped_fields` and `unresolved_relations` to
it. Nothing on the TypeScript side mirrored any of it, so typed frontend
code could not read a warning the server was already sending. Additive and
fully optional, matching the Go struct's `omitempty` on every member.
Gates on this tip: build, go vet, gofmt, `npm run check` 0 errors (6
pre-existing warnings in unrelated files), `make web-test` 124 files / 2108
tests passed. The full SQLite suite and Postgres are owed on this tip and are
NOT claimed here — the SQLite run on 3e7a6751 is the one that failed above.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
* fix: bulk move refused a required field the same request supplied (TASK-2878)
Codex round 19 reviewed whether rounds 16-18's fixes broke anything. They did
not — it states explicitly that the collapse, the carried-set recomputation,
the notDefaultKeys change and the non-string-default drop do not narrow owner
behaviour or alter non-nil / never-supplied override behaviour. That negative
result is the round's main product, given that three earlier fixes in this unit
each opened the next round's defect.
It raised three findings. One is fixed here; two are recorded for a ruling.
FIXED — BULK MOVE CHECKED A REQUIRED-FIELD ERROR COMPUTED BEFORE THE OVERRIDE
EXISTED. `MigrateFields` records `required field "status" has no value` when a
source `status` holding a select value cannot migrate into a destination
`status` declared as a required RELATION. The caller supplies a perfectly good
referent in the same request, `result.Fields["status"]` is set from it — and
the check that follows reads `result.Errors`, which was computed before any of
that. So the move was refused for a field the request had just filled, and the
item stayed put.
This is the defect PLAN-2357 DR-12 fixed at the SINGLE move door. The comment
there says it in as many words: "an override that SATISFIED a required
destination field still 400'd". Nobody swept the fix to the bulk door, and it
became reachable when round 11 established that `req.Status` is caller input.
Third instance in this unit of a rule fixed at one door and not at its
siblings, which is the CONVE-18 shape at the level of DOORS rather than call
sites.
Filtering the error list rather than adopting the single door's
validate-the-merged-map shape, deliberately: `ValidateFields` below already
covers the merged map, and switching this check to it would change the error
code for a genuinely-missing required field from `missing_required_fields` to
`validation_error` — a compatibility break to fix a defect that does not need
one. The filter matches MigrateFields' exact rendering per supplied key, not a
substring, so a key whose name contains another key's name cannot collide.
Mutant: restore the unfiltered `result.Errors` -> FAIL, the move is refused for
the supplied field. Control leg included: with NOTHING supplied, the move must
STILL be refused AND still carry `missing_required_fields`, so the test fails
against a build that dropped the check rather than narrowing it.
RECORDED, NOT FIXED — the copy and its preflight disagree about error
PRECEDENCE in two cases. Both doors REFUSE in both cases and both refusals are
non-disclosing; what differs is which error wins.
a. Overrides `{"owner_ref":"<invisible live item>", "ghost":"x"}`: the
preflight returns 400 `malformed_override` for the undeclared key, the
copy returns 400 `validation_error` for the invisible relation.
b. Override `{"owner_ref":42}`: the preflight returns 400 `invalid_override`,
the copy returns 400 `validation_error` from store field validation.
Both are real — the pair is specified to give one answer to one body — and
neither is a security or data defect. Closing them means choosing a precedence
and applying it at two doors, which is a response-contract decision and a
reordering of error handling in the MUTATING path. In a unit where three fixes
have each opened the next round's defect, that is not a change to make
unilaterally at the end of a session. Left for a ruling, with the two concrete
bodies above so whoever takes it does not have to re-derive them.
Gates: build, go vet, gofmt clean on this tip. SQLite and Postgres were BOTH
green on f601a2a1 (SQLite server 372.7s / store 376.0s / items / mcp 24.7s,
EXIT=0; PG store 525.8s / server 267.8s, EXIT=0) and are owed again on this
tip; they are NOT claimed here.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
* fix: a legacy item with a non-string relation value was unmovable (TASK-2878)
Codex round 20 was an ENUMERATION round rather than another confirmation
round: every door and its three origin classes, every path a relation reason
can reach a caller, every place a value is deleted from the field map, every
pair of doors specified to agree, and every guard added by this diff with what
breaks if it is removed. Nineteen rounds had each found something, and CONVE-24
is the reason the twentieth asked for the population instead.
It returned five candidates. FOUR are this unit's deliberate decisions read
against a literal contract, and all four are already pinned by tests that
assert the current behaviour on purpose:
- `fields_patch` and bulk field update leave an UNTOUCHED carried relation
value alone. That is the carry rule itself. Resolving keys the operation
does not mention would refuse an item's status change because of a legacy
value in a field nobody touched.
- Required destination defaults REFUSE rather than drop. A default is never
refused for being invalid; a REQUIRED field left with no value is refused
because the item cannot be stored valid. Round 3 established this.
- Workspace archive import carries unresolved values with no relation
validation, per Dave's import ruling.
The fifth is real and is fixed here.
A NON-STRING CARRIED VALUE MADE A LEGACY ITEM PERMANENTLY UNMOVABLE.
`internal/items` has accepted anything in a relation field for as long as the
type has existed, and this unit's carry rule exists precisely so those items
stay usable — the move door's own comment says refusing carried values "would
make every legacy item permanently unmovable". A carried value that is not a
string was the case where that happened anyway. Both carried branches
deliberately left it for `ValidateFields`, and `ValidateFields` REFUSES:
single move: 400 invalid_fields, `field "owner_ref" must be a string (item ID)`
bulk move: per-item validation_error, same sentence
On every attempt, with no way for the owner to fix it through these doors at
all — the value cannot be corrected by a move and the move cannot proceed
past it.
Round 6 established that the two migration MODES must not disagree about a
malformed value, and they did not: they agreed on the wrong outcome. The drop
is hoisted ABOVE the mode switch, so they keep agreeing by construction and
what they agree on is drop-and-report. `invalid_shape`, not `not_portable` or
`not_found`, because the value is not a reference and every reason describing
a LOOKUP is a false account of why it is going — the same reason and
disposition the destination-default branch uses and that
`ResolveLateRelationDefaultsQ` has always used. All three now agree.
The cross-workspace branch's non-string check is kept as an unreachable type
assertion rather than deleted, so that branch cannot silently start treating a
non-string as a ref if the hoisted loop is ever narrowed.
Mutant: remove the hoisted drop -> FAIL on both the `single move` and `bulk
move` legs. Control leg included: a carried value that RESOLVES must still
survive the move, so the test fails against a build that dropped every carried
relation rather than only the malformed ones.
Round 20's direct answers, recorded so the next round starts from them: the
only remaining ways to store an unresolvable value, to learn an invisible item
exists, or to have a value discarded without a report are the already-recorded
ones — IDEA-2886, IDEA-2893, the archive-import ruling, the carry rule's own
untouched-key posture, and the two precedence divergences awaiting a ruling.
It found NO additional uncollapsed `wrong_collection` path and NO additional
validation-induced silent deletion.
Gates: build, go vet, gofmt clean; the full relation suites green
(`internal/server` 43.041s, `internal/store` 0.936s). SQLite and Postgres were
both green on 7573d042 and are owed again on this tip; NOT claimed here.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
* fix: the eighth door's visibility pass, override precedence, refilled carried drops (TASK-2878)
The lead's day-58 ruling, all three items, plus the regression my own first
attempt at item 2 introduced.
1. THE EIGHTH DOOR GETS THE VISIBILITY PASS (night-11 finding, ruling shape (b)).
`dropInvisibleRelationDefaults` reached five call sites, all in
`internal/server`. The SIXTH caller of the late-default resolver is
`store.migrateCopyFields`, inside the cross-workspace copy's transaction,
where a *Server method cannot go — so the copy STORED a destination
default's canonical id for a caller who cannot see the collection it points
into, and handed it back in the 201, while the PREFLIGHT reported the same
key dropped for the identical request. One request, two answers.
Round 15's sweep said "applied at all five late-default sites, per
CONVE-18" and was right about the sites it could see. THE CLASS IS ONE
WIDER THAN THE PACKAGE. That is this unit's recurring lesson — a rule
applied at one door and not its siblings — one level up, at the package
boundary rather than the call site, and it is why a sweep should name the
VANTAGE POINT it enumerated from.
Shape (b) as ruled: `checkItemVisibleQ` — which already exists
parameterised over its executor for exactly this (BUG-2409) — is bound into
a `store.RelationVisibilityFunc` closure by the server and threaded into the
copy request, so the copy runs the rule on its own transaction while holding
both workspaces' advisory locks.
The body moved to `store.DropInvisibleRelationDefaultsQ` and the server's
method now delegates to it, so both doors run ONE implementation rather than
two kept in step by hand. `notDefaultKeys` moved to `store.NotDefaultKeys`
for the same reason. A nil callback means no check, for internal callers
with no requester whose visibility could be evaluated.
Pinned with the night reviewer's three legs. Leg 3 is what makes it a test:
the OWNER's identical copy must still resolve and store the default, so a
build that dropped every default cannot pass.
Mutant: remove the pass -> FAIL.
2. OVERRIDE PRECEDENCE: STRUCTURAL BEFORE SEMANTIC, ONE CLASSIFIER (ruling).
The preflight's order is the contract. `structuralOverrideError` decides
both structural problems — an undeclared key (`malformed_override`) and a
wrong-shaped value (`invalid_override`) — and BOTH doors call it before any
semantic check.
The copy previously reached `refuseInvisibleRelationOverrides` first, because
that check lived in the handler while the structural ones lived inside the
store call. Nobody chose that order; it was an emergent property of where
each check happened to live, which is why the fix is one function rather
than two orderings maintained by hand.
Both of round 19's bodies are pinned AT BOTH DOORS in one table test, on one
body per case: `{"owner_ref":"<invisible>","ghost":"x"}` -> malformed_override,
`{"owner_ref":42}` -> invalid_override, with a leg asserting nothing was
written. Asserting them per-door is what let them drift, since each door's
own test passed.
Mutant: restore the semantic-first order at the copy -> FAIL on both cases.
MY FIRST VERSION OF THIS BROKE COERCION, and the full suite caught it, not
my targeted tests. The classifier validated the raw overrides, so
`{"cost":"42"}` against a number field — coercible, and pinned as acceptable
at both doors by TestCopyAndPreflightCoerceIdentically — was refused
`invalid_override`. It now coerces the probe first, as both doors do.
Hoisting a check above the coercion that makes it pass is the same class of
error as the ordering this function exists to fix, committed while fixing it.
3. A CARRIED RELATION DROP REFILLED BY A DESTINATION DEFAULT IS REPORTED (ruling).
Round 3's `StillDropped` filter suppresses a key the final map has a value
for, because reporting a populated key as dropped made three surfaces give
two answers. Right for a key `MigrateFields` dropped and a default refilled
— the caller never had anything else there. Wrong for a CARRIED RELATION:
the source's value was genuinely discarded and what sits in the key is a
different value the destination chose, so the row said nothing was lost when
something was.
Scoped to relation drops deliberately; the type/schema-mismatch case is
round 3's, unchanged. The preflight already discloses this in its carried
row's `"from":"default"`, so the copy adds no new information — it makes the
copy say it too, which is the agreement. The test asserts the drop IS
reported AND that the destination default still lands.
Gates on this tip: build, go vet, gofmt clean; `internal/server` ok 283.247s
and `internal/store` ok 301.461s (SQLite). Postgres, the full four-package
SQLite gate and CI are owed on this tip and are NOT claimed here.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
567 lines
25 KiB
Go
567 lines
25 KiB
Go
package server
|
|
|
|
import (
|
|
"net/http"
|
|
"sort"
|
|
"strings"
|
|
|
|
"github.com/PerpetualSoftware/pad/internal/items"
|
|
"github.com/PerpetualSoftware/pad/internal/models"
|
|
"github.com/PerpetualSoftware/pad/internal/store"
|
|
)
|
|
|
|
// Server-side half of referent validation for `relation` values (PLAN-2857 U1
|
|
// / TASK-2878). The store owns the question "does this value name a live item
|
|
// in the declared target collection"; this file adds the one part that is
|
|
// request-scoped — whether the caller may SEE that item — and turns the result
|
|
// into whatever each door owes its caller.
|
|
//
|
|
// Eight doors reach `items.CoerceFields`, and they do NOT all owe the same
|
|
// thing, which is why this is a value-returning helper rather than a
|
|
// write-the-response one in the `extractParentLink` mould:
|
|
//
|
|
// - The six WRITE doors (create, update-fields, update-fields_patch,
|
|
// same-workspace move, bulk update, bulk move) refuse: a caller who
|
|
// supplied a value asserted it, and an unresolvable assertion is a 400.
|
|
// - The two COPY doors report instead. A relation value CARRIED from a
|
|
// source item points at a source-workspace row and cannot resolve in the
|
|
// destination by construction; that is an unportable referent, not a bad
|
|
// write, and the copy already drops such things (`github_pr`, reason
|
|
// `referent_not_portable`). Refusing would make every copy of a related
|
|
// item start failing and would reintroduce the cross-workspace relation
|
|
// case PLAN-2857 excludes from v1. An override SUPPLIED to a copy is a
|
|
// write like any other and still refuses.
|
|
|
|
// resolveRelationReferents runs the store resolver and then drops any target
|
|
// the requester cannot see, reporting it as `not_found`.
|
|
//
|
|
// Visibility is folded into the SAME issue vocabulary rather than given its
|
|
// own reason on purpose: telling a caller "that item exists but you may not
|
|
// see it" is an existence oracle, which this codebase has a standing rule
|
|
// against. The store's `not_found` and a visibility failure must be
|
|
// indistinguishable on the wire.
|
|
func (s *Server) resolveRelationReferents(
|
|
r *http.Request,
|
|
workspaceID string,
|
|
schema models.CollectionSchema,
|
|
fieldMap map[string]any,
|
|
) ([]store.RelationIssue, error) {
|
|
return s.resolveRelationReferentsAs(r, workspaceID, workspaceRole(r), schema, fieldMap)
|
|
}
|
|
|
|
// resolveRelationReferentsAs is resolveRelationReferents with the requester's
|
|
// effective role passed EXPLICITLY rather than read from the request.
|
|
//
|
|
// The cross-workspace copy and its preflight need this. `workspaceRole(r)` is
|
|
// the role the middleware stashed for the workspace in the URL — the SOURCE —
|
|
// and a relation override on a copy names an item in the DESTINATION, where
|
|
// the caller's role can be different or absent. `CrossWorkspaceAccess.Role`
|
|
// is that role, derived fresh from membership and grants, and its own doc says
|
|
// in as many words never to substitute `workspaceRole(r)` for it.
|
|
func (s *Server) resolveRelationReferentsAs(
|
|
r *http.Request,
|
|
workspaceID string,
|
|
role string,
|
|
schema models.CollectionSchema,
|
|
fieldMap map[string]any,
|
|
) ([]store.RelationIssue, error) {
|
|
// The ORIGINAL values, captured before the store resolver rewrites a ref
|
|
// into its target's UUID. Every issue this function raises quotes what the
|
|
// CALLER sent, never the canonical form: a refusal for an item the
|
|
// requester may not see must not hand back that item's UUID, which would
|
|
// confirm both its existence and its canonical identity — the existence
|
|
// oracle the `not_found` collapse exists to prevent, reopened by the
|
|
// message (codex round 2).
|
|
supplied := make(map[string]string, len(fieldMap))
|
|
for k, v := range fieldMap {
|
|
if str, isStr := v.(string); isStr {
|
|
supplied[k] = str
|
|
}
|
|
}
|
|
quoted := func(key, canonical string) string {
|
|
if orig, ok := supplied[key]; ok && orig != "" {
|
|
return orig
|
|
}
|
|
return canonical
|
|
}
|
|
|
|
issues, err := s.store.ResolveRelationReferents(workspaceID, schema, fieldMap)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
|
|
// Everything the store resolved is now a canonical ID in fieldMap. Check
|
|
// each one against the requester before it is allowed to stand.
|
|
for _, def := range schema.Fields {
|
|
if def.Type != "relation" {
|
|
continue
|
|
}
|
|
raw, exists := fieldMap[def.Key]
|
|
if !exists || raw == nil {
|
|
continue
|
|
}
|
|
id, isStr := raw.(string)
|
|
// 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 {
|
|
// `wrong_collection` is the one issue that names a LIVE item, so
|
|
// its message ("is not an item in collection X") tells the caller
|
|
// the value EXISTS — distinguishable from the `not_found` a
|
|
// nonexistent value gets, and therefore an existence oracle for
|
|
// anyone who cannot see that item (codex round 3). Collapse it to
|
|
// `not_found` when the requester may not see the target; keep the
|
|
// specific message when they may, because "you linked a task
|
|
// where a person belongs" is the useful half of this reason.
|
|
//
|
|
// Any other issue is already `not_found`-shaped and needs nothing.
|
|
if ri.Reason != store.RelationTargetWrongCollection {
|
|
continue
|
|
}
|
|
target, terr := s.store.ResolveRelationTarget(workspaceID, ri.Value)
|
|
if terr != nil {
|
|
return nil, terr
|
|
}
|
|
if target == nil {
|
|
// Deleted between the resolver's lookup and this one. The
|
|
// issue still SAYS `wrong_collection`, and that message
|
|
// reveals the value named something a moment ago — the same
|
|
// disclosure for a caller who cannot see it (codex round 4).
|
|
// My first comment here read "already the safe answer", which
|
|
// was wrong: nothing had rewritten the reason.
|
|
collapseIssue(issues, def.Key, store.RelationTargetNotFound)
|
|
continue
|
|
}
|
|
seen, verr := s.checkItemVisible(workspaceID, target, currentUser(r), role, isBearerAuth(r))
|
|
if verr != nil {
|
|
return nil, verr
|
|
}
|
|
if !seen {
|
|
collapseIssue(issues, def.Key, store.RelationTargetNotFound)
|
|
}
|
|
continue
|
|
}
|
|
item, err := s.store.GetItem(id)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
if item == nil {
|
|
// It resolved moments ago and is gone now — soft-deleted between
|
|
// the two reads. Treated as unresolvable rather than waved
|
|
// through: this whole unit exists to stop a dangling referent
|
|
// reaching the blob, and "the target vanished mid-request" is the
|
|
// one case where letting it through would be a deliberate one.
|
|
// Same `not_found` the resolver would have given a moment later,
|
|
// so a retry reports it identically.
|
|
issues = append(issues, store.RelationIssue{
|
|
Key: def.Key, Value: quoted(def.Key, id), Target: def.Collection,
|
|
Reason: store.RelationTargetNotFound,
|
|
})
|
|
continue
|
|
}
|
|
visible, err := s.checkItemVisible(workspaceID, item, currentUser(r), role, isBearerAuth(r))
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
if !visible {
|
|
issues = append(issues, store.RelationIssue{
|
|
Key: def.Key, Value: quoted(def.Key, id), Target: def.Collection,
|
|
Reason: store.RelationTargetNotFound,
|
|
})
|
|
}
|
|
}
|
|
return issues, nil
|
|
}
|
|
|
|
// issueForKey returns the issue already raised for key, if any.
|
|
func issueForKey(issues []store.RelationIssue, key string) (store.RelationIssue, bool) {
|
|
for _, ri := range issues {
|
|
if ri.Key == key {
|
|
return ri, true
|
|
}
|
|
}
|
|
return store.RelationIssue{}, false
|
|
}
|
|
|
|
// collapseIssue rewrites the reason of the issue already raised for key. In
|
|
// place, because the slice is what the caller renders.
|
|
func collapseIssue(issues []store.RelationIssue, key string, reason store.RelationIssueReason) {
|
|
for i := range issues {
|
|
if issues[i].Key == key {
|
|
issues[i].Reason = reason
|
|
return
|
|
}
|
|
}
|
|
}
|
|
|
|
// collapseInvisibleRelationIssues rewrites `wrong_collection` to `not_found`
|
|
// on any issue whose target the requester cannot see — the same collapse
|
|
// resolveRelationReferentsAs applies to the MAIN pass, hoisted so the LATE
|
|
// pass gets it too.
|
|
//
|
|
// `store.ResolveLateRelationDefaults` is a store function and cannot know who
|
|
// is asking, so every issue it returns carries the raw reason. Those issues
|
|
// reach a caller: each door feeds them to RequiredRelationIssues and renders
|
|
// the result into a 400 or a preflight `needs_value` row. `wrong_collection`
|
|
// is the one reason that names a LIVE item, so an invisible target announced
|
|
// that way is the existence oracle round 3 closed, reopened through the door
|
|
// round 10 added (codex round 15).
|
|
//
|
|
// Reviewer named ONE site; this is applied at all five late-default sites,
|
|
// per CONVE-18 — the class is "a store-resolved issue reaching a caller
|
|
// without passing the visibility collapse", not the one call it was spotted at.
|
|
func (s *Server) collapseInvisibleRelationIssues(r *http.Request, workspaceID, role string, issues []store.RelationIssue) error {
|
|
for i := range issues {
|
|
if issues[i].Reason != store.RelationTargetWrongCollection {
|
|
continue
|
|
}
|
|
target, terr := s.store.ResolveRelationTarget(workspaceID, issues[i].Value)
|
|
if terr != nil {
|
|
return terr
|
|
}
|
|
if target == nil {
|
|
// Vanished between the two reads. The reason still SAYS the value
|
|
// named something a moment ago, which is the same disclosure.
|
|
issues[i].Reason = store.RelationTargetNotFound
|
|
continue
|
|
}
|
|
seen, verr := s.checkItemVisible(workspaceID, target, currentUser(r), role, isBearerAuth(r))
|
|
if verr != nil {
|
|
return verr
|
|
}
|
|
if !seen {
|
|
issues[i].Reason = store.RelationTargetNotFound
|
|
}
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// refuseRelationIssues writes the 400 a write door owes and reports whether it
|
|
// did. One function so the six refusing doors cannot phrase the same refusal
|
|
// three different ways, and so a client matching on the error code sees one
|
|
// answer from every door.
|
|
//
|
|
// The message names the field, the value AS SUPPLIED, and the target
|
|
// collection — the three things a caller needs to fix it without a second
|
|
// request. Every issue is rendered, joined; a schema has a handful of relation
|
|
// fields at most, and reporting one at a time would make fixing a bulk import
|
|
// an N-round-trip exercise.
|
|
//
|
|
// Deliberately the ORDINARY `validation_error` shape, with no new details key.
|
|
// Two reasons: the MCP stdio transport classifies errors by matching CLI
|
|
// stderr PROSE, so a structured field it cannot see would help nobody there;
|
|
// and a new error shape is a contract change for every client, which is not
|
|
// what this unit is for.
|
|
func refuseRelationIssues(w http.ResponseWriter, issues []store.RelationIssue) bool {
|
|
if len(issues) == 0 {
|
|
return false
|
|
}
|
|
writeError(w, http.StatusBadRequest, "validation_error", relationIssuesMessage(issues))
|
|
return true
|
|
}
|
|
|
|
// relationIssuesMessage is the same rendering for the doors that RETURN an
|
|
// error instead of writing a response — `createItemChecked` (*itemCreateError)
|
|
// and the two bulk operations (*bulkOpError). Split out rather than duplicated
|
|
// so a caller cannot accidentally produce a different sentence for the same
|
|
// refusal depending on which door it came through.
|
|
//
|
|
// DELEGATES to store.RelationIssuesMessage rather than joining here (TASK-2878).
|
|
// The eighth door refuses inside `internal/store` — the cross-workspace copy,
|
|
// through *FieldValidationError — so the sentence has to be reachable from
|
|
// there too. Two joins in two packages is how one refusal acquires two
|
|
// phrasings, which is the drift this unit exists to remove.
|
|
func relationIssuesMessage(issues []store.RelationIssue) string {
|
|
return store.RelationIssuesMessage(issues)
|
|
}
|
|
|
|
// refuseInvisibleRelationOverrides is the visibility check the MIGRATE doors
|
|
// owe their SUPPLIED half.
|
|
//
|
|
// The four write doors go through resolveRelationReferents, which adds
|
|
// checkItemVisible on top of the store resolver. The migrate doors call
|
|
// store.MigrateRelationReferents directly — it is a store function and cannot
|
|
// answer a request-scoped question — so without this their supplied overrides
|
|
// resolved against the database alone. This unit's own rule says a supplied
|
|
// value is an ordinary write; an ordinary write cannot name an item the
|
|
// requester may not see, and a caller able to edit both collections could
|
|
// otherwise point a relation at a hidden one.
|
|
//
|
|
// Only relation keys are probed, on a COPY of the values: the store call that
|
|
// follows does the canonicalising write, and a helper that also mutated would
|
|
// leave two functions writing one map.
|
|
//
|
|
// `role` is explicit for the reason resolveRelationReferentsAs documents — at a
|
|
// cross-workspace copy the relevant role is the caller's in the DESTINATION.
|
|
func (s *Server) refuseInvisibleRelationOverrides(
|
|
r *http.Request,
|
|
workspaceID string,
|
|
role string,
|
|
schema models.CollectionSchema,
|
|
supplied map[string]any,
|
|
) ([]store.RelationIssue, error) {
|
|
if len(supplied) == 0 {
|
|
return nil, nil
|
|
}
|
|
probe := make(map[string]any, len(supplied))
|
|
for _, def := range schema.Fields {
|
|
if def.Type != "relation" {
|
|
continue
|
|
}
|
|
if v, ok := supplied[def.Key]; ok && v != nil {
|
|
probe[def.Key] = v
|
|
}
|
|
}
|
|
if len(probe) == 0 {
|
|
return nil, nil
|
|
}
|
|
return s.resolveRelationReferentsAs(r, workspaceID, role, schema, probe)
|
|
}
|
|
|
|
// notDefaultKeys delegates to store.NotDefaultKeys.
|
|
//
|
|
// The rule moved into `store` because the cross-workspace COPY needs it from
|
|
// inside its transaction, and the lead's day-58 ruling is that both doors run
|
|
// ONE classifier rather than two kept in step by hand. Kept as a wrapper so
|
|
// the four server call sites read unchanged.
|
|
func notDefaultKeys(supplied, carried map[string]any) map[string]bool {
|
|
return store.NotDefaultKeys(supplied, carried)
|
|
}
|
|
|
|
// dropInvisibleRelationDefaults is the visibility check a LATE-RESOLVED
|
|
// DEFAULT owes (codex round 11).
|
|
//
|
|
// `ResolveLateRelationDefaults` lives in `store` and resolves against the
|
|
// database alone. At the write doors the caller's own values are filtered out
|
|
// of the refusal set before it runs — correctly, since a default is not caller
|
|
// input — but that also removed the visibility issues raised against those
|
|
// keys, and the store then re-resolved them with no visibility layer at all.
|
|
// The write's RESPONSE carries the item's fields, so the caller received the
|
|
// canonical id of an item they cannot see.
|
|
//
|
|
// DROPPED, not refused, because the origin has not changed: the schema author
|
|
// chose the value and the caller can neither fix it nor be blamed for it. What
|
|
// they must not get is the id.
|
|
//
|
|
// Reported through the same `not_found` reason every other visibility failure
|
|
// collapses to, so a caller cannot tell "the default names something you may
|
|
// not see" from "the default names nothing".
|
|
//
|
|
// `notADefault` is every key that is NOT a destination default — the caller's
|
|
// own values and the values carried from a source item. Both are excluded
|
|
// deliberately and for different reasons: a caller's value is refused
|
|
// elsewhere by the visibility check on its own door, and a CARRIED value must
|
|
// never be refused or dropped for visibility at all, because that would make
|
|
// an item referencing something the mover cannot see unmovable. The first
|
|
// version of this took "present before the late pass", which at a migrate door
|
|
// includes the defaults MigrateFields injected — so exactly the values it
|
|
// dropInvisibleRelationDefaults removes destination-schema defaults whose
|
|
// target this requester cannot see, reporting each as a `not_found` drop.
|
|
//
|
|
// The body moved to store.DropInvisibleRelationDefaultsQ so the cross-workspace
|
|
// COPY — which resolves its late defaults inside its own transaction and could
|
|
// not reach a *Server method — runs the SAME code rather than a second
|
|
// implementation of the same rule. That door was the one site of five that
|
|
// never got this pass, because round 15's sweep enumerated the sites reachable
|
|
// from `internal/server` and the class is one wider than the package
|
|
// (night-11 finding, lead ruling day 58).
|
|
//
|
|
// The request-scoped part stays here, as the closure: who is asking, their role
|
|
// in this workspace, and whether the call is bearer-authenticated.
|
|
func (s *Server) dropInvisibleRelationDefaults(
|
|
r *http.Request,
|
|
workspaceID string,
|
|
role string,
|
|
schema models.CollectionSchema,
|
|
fieldMap map[string]any,
|
|
notADefault map[string]bool,
|
|
) ([]store.RelationIssue, error) {
|
|
return s.store.DropInvisibleRelationDefaultsQ(s.store.Q(), workspaceID,
|
|
s.relationVisibility(r, role), schema, fieldMap, notADefault)
|
|
}
|
|
|
|
// relationVisibility binds the request-scoped half of the visibility rule into
|
|
// a callback the store can run on ITS executor. This is what lets the copy
|
|
// apply the rule inside the transaction that holds both workspaces' advisory
|
|
// locks — the shape `checkItemVisibleQ` is parameterised for (BUG-2409).
|
|
func (s *Server) relationVisibility(r *http.Request, role string) store.RelationVisibilityFunc {
|
|
user := currentUser(r)
|
|
bearer := isBearerAuth(r)
|
|
return func(q store.Queryer, workspaceID string, item *models.Item) (bool, error) {
|
|
return s.checkItemVisibleQ(q, workspaceID, item, user, role, bearer)
|
|
}
|
|
}
|
|
|
|
// resolveRelationsForWrite is the whole relation decision for a WRITE door —
|
|
// create and the full-`fields` update, which ran the identical four steps.
|
|
//
|
|
// Extracted after codex round 10 showed the same rule stated at a third site
|
|
// (CONVE-139: consolidate when the move is mechanical, do not defer for
|
|
// effort). The rule is: the CALLER's values are refused, and everything
|
|
// validation filled in is dropped and reported. Four steps expressed that, and
|
|
// two doors each spelled all four out:
|
|
//
|
|
// 1. resolve the whole map, which is caller input plus injected defaults;
|
|
// 2. keep only the issues on keys the caller actually supplied — a default is
|
|
// asserted by nobody, so an unresolvable one must not refuse the write;
|
|
// 3. resolve the defaults validation injected AFTER the main pass, dropping
|
|
// what does not resolve — except in a REQUIRED field, where dropping
|
|
// would store the item with that field absent;
|
|
// 4. drop a default whose target the caller cannot see, so the response does
|
|
// not hand back its id.
|
|
//
|
|
// NOT extended to the migrate doors, and not for effort: they reach the same
|
|
// rule through `store.MigrateRelationReferents`, which is in `internal/store`
|
|
// and cannot call this — the visibility layer here is request-scoped by
|
|
// construction, and `store` cannot import `server`. Unifying the two families
|
|
// needs a caller-supplied visibility predicate on the store API; that is
|
|
// IDEA-2886, with its own door table.
|
|
//
|
|
// `refusals` are the caller's to fix; `dropped` are keys the write discarded
|
|
// and must report.
|
|
func (s *Server) resolveRelationsForWrite(
|
|
r *http.Request,
|
|
workspaceID string,
|
|
role string,
|
|
schema models.CollectionSchema,
|
|
fieldMap map[string]any,
|
|
presentBefore map[string]bool,
|
|
posture relationPosture,
|
|
) (refusals []store.RelationIssue, dropped []string, err error) {
|
|
issues, err := s.resolveRelationReferents(r, workspaceID, schema, fieldMap)
|
|
if err != nil {
|
|
return nil, nil, err
|
|
}
|
|
callerIssues := store.IssuesForCallerInput(issues, presentBefore)
|
|
if len(callerIssues) > 0 && posture == relationsRefuse {
|
|
// The write is about to be REFUSED, so steps 3 and 4 would only
|
|
// prepare a field map nobody stores.
|
|
return callerIssues, nil, nil
|
|
}
|
|
// A CARRYING door does not stop here, and this is the round-15 defect:
|
|
// the early return was written when every caller of this path refused, and
|
|
// stayed when the artifact-import door began carrying instead. An import
|
|
// holding ONE unresolvable caller value would skip the late-default pass
|
|
// AND the default-visibility drop entirely, storing whatever the
|
|
// destination schema injected — including a default the caller cannot see
|
|
// — and reporting none of it. The refusals are still returned; they are
|
|
// the carry report, not a stop.
|
|
|
|
lateDropped, err := s.store.ResolveLateRelationDefaults(workspaceID, schema, fieldMap, presentBefore)
|
|
if err != nil {
|
|
return nil, nil, err
|
|
}
|
|
if cerr := s.collapseInvisibleRelationIssues(r, workspaceID, role, lateDropped); cerr != nil {
|
|
return nil, nil, cerr
|
|
}
|
|
if required := store.RequiredRelationIssues(schema, lateDropped); len(required) > 0 {
|
|
return append(callerIssues, required...), nil, nil
|
|
}
|
|
|
|
invisible, err := s.dropInvisibleRelationDefaults(r, workspaceID, role, schema, fieldMap, presentBefore)
|
|
if err != nil {
|
|
return nil, nil, err
|
|
}
|
|
// The required check covers the INVISIBLE drops too. It was written for
|
|
// `lateDropped` and a sibling list was added beside it a round later —
|
|
// deleting a key after validation has passed leaves a required field
|
|
// absent regardless of which list recorded it (codex round 13).
|
|
if required := store.RequiredRelationIssues(schema, invisible); len(required) > 0 {
|
|
return append(callerIssues, required...), nil, nil
|
|
}
|
|
for _, ri := range append(lateDropped, invisible...) {
|
|
dropped = append(dropped, ri.Key)
|
|
}
|
|
// callerIssues is EMPTY on a refusing door — it returned above — and holds
|
|
// the carry report on a carrying one. Returning it here rather than `nil`
|
|
// is what keeps an import's unresolvable values named in the response now
|
|
// that the carrying door runs to the end.
|
|
return callerIssues, dropped, nil
|
|
}
|
|
|
|
// structuralOverrideError classifies the problems with a caller's field
|
|
// overrides that are about the REQUEST rather than about the workspace's
|
|
// state, and is the one classifier both copy doors run before any semantic
|
|
// check (lead ruling, day 58).
|
|
//
|
|
// Two kinds, in this order:
|
|
//
|
|
// malformed_override — an override names a field the destination schema does
|
|
// not declare.
|
|
// invalid_override — an override's VALUE fails the destination's type for
|
|
// that field.
|
|
//
|
|
// STRUCTURAL BEFORE SEMANTIC, and the preflight's order is the contract. The
|
|
// copy used to run refuseInvisibleRelationOverrides — a question about what
|
|
// this caller may SEE — before the store call that performs these checks, so
|
|
// `{"owner_ref":"<invisible>","ghost":"x"}` returned `malformed_override` from
|
|
// the preflight and `validation_error` from the copy, and `{"owner_ref":42}`
|
|
// returned `invalid_override` from the preflight and `validation_error` from
|
|
// the copy. One body, two answers, which is the DR-6 divergence this pair
|
|
// exists to prevent (codex round 19).
|
|
//
|
|
// One function rather than two orderings kept in step by hand: the previous
|
|
// arrangement was not that anyone chose the wrong order, it was that the order
|
|
// was an emergent property of where each check happened to live.
|
|
//
|
|
// Returns ok=true when nothing structural is wrong.
|
|
func structuralOverrideError(overrides map[string]any, targetSchema models.CollectionSchema) (code, message string, ok bool) {
|
|
if len(overrides) == 0 {
|
|
return "", "", true
|
|
}
|
|
schema := items.SchemaForMigratedFields(targetSchema)
|
|
if bad := items.UndeclaredOverrideKeys(overrides, schema.Fields); len(bad) > 0 {
|
|
return "malformed_override", "Destination collection has no field(s): " + summarizeKeys(bad), false
|
|
}
|
|
// Validated in ISOLATION — the overrides alone, never merged over the
|
|
// migrated map — so the answer cannot depend on the source item's
|
|
// contents. That is the same property the undeclared check has, and it is
|
|
// what lets this run before anything reads the source.
|
|
probe := make(map[string]any, len(overrides))
|
|
for k, v := range overrides {
|
|
if v == nil {
|
|
// An explicit null is a CLEAR, not a value, and has no type to
|
|
// fail. Validating it here would refuse the documented way to
|
|
// blank a field.
|
|
continue
|
|
}
|
|
probe[k] = v
|
|
}
|
|
if len(probe) == 0 {
|
|
return "", "", true
|
|
}
|
|
// COERCED FIRST, exactly as both doors coerce before validating (BUG-2850).
|
|
// Without this the classifier refuses a value the copy would happily
|
|
// accept — `{"cost":"42"}` against a number field is coercible, and
|
|
// TestCopyAndPreflightCoerceIdentically pins that it must be accepted at
|
|
// both doors. Hoisting a check above the coercion that makes it pass is
|
|
// the same class of error as the ordering this function exists to fix.
|
|
probe = items.CoerceFields(probe, schema)
|
|
var bad []string
|
|
for _, iss := range items.ValidateFieldsDetailed(probe, schema) {
|
|
if iss.Kind != items.IssueInvalid {
|
|
// IssueRequired and friends are about the FINAL map, which this
|
|
// probe is not — a required field the overrides do not mention is
|
|
// not an override problem.
|
|
continue
|
|
}
|
|
if _, overridden := overrides[iss.Key]; !overridden {
|
|
continue
|
|
}
|
|
bad = append(bad, iss.Message)
|
|
}
|
|
if len(bad) > 0 {
|
|
sort.Strings(bad)
|
|
// Bounded for the same reason the malformed_override message is:
|
|
// validateFieldType quotes the offending VALUE verbatim, so a single
|
|
// large override string would otherwise be reflected back in full.
|
|
return "invalid_override", "Invalid override value(s): " + summarizeMessages(bad), false
|
|
}
|
|
return "", "", true
|
|
}
|