IDEA-2893 + IDEA-2894: record the accepted carry disclosure, and make the drop-reason mapper testable - #1254
Merged
Merged
Conversation
… be tested (IDEA-2894)
The mapping from a server drop reason to the sentence a user reads lived inline
in `CopyItemDialog.svelte`, unexported, with no test file for the component at
all. Two separate review rounds found defects in it and NEITHER FIX WAS PINNED
BY ANYTHING:
- round 12: the UI asserted NON-EXISTENCE from `not_found`, which the server
also emits for a target the caller merely cannot see. Telling those apart
is the existence oracle the collapse exists to prevent.
- round 18: `referent_not_portable` read "it points at something in the
source workspace", claiming both existence and location for a reason
emitted WITHOUT resolving the target — and which `github_pr` reaches too,
where the referent is in no workspace at all.
A third defect of the same shape would have been found the same way, by a
reviewer happening to read it, or not at all.
Moved to `$lib/items/copyDropReasons` with the reason vocabulary as an explicit
exported list, which makes two tests possible that could not be written before:
1. EVERY reason the server can emit has a sentence. This is round 12's other
finding as a test: BUG-2674 added `referent_not_portable` server-side,
nothing here learned it, and it rendered through the fallback as a raw enum
string in front of a user. A reason that maps to itself IS that defect.
Mutant: delete `referent_not_portable`'s message -> FAIL.
2. THE TWO HAZARDOUS REASONS STAY NEUTRAL. `not_found` and
`referent_not_portable` must not claim a target exists, does not exist, or
say where it is. Asserted over those two rather than all ten:
`wrong_collection` legitimately says the target is outside the field's
collection, and it may, because the server only emits it to a caller who
can SEE the target.
Mutant: restore round 18's wording -> FAIL, naming the sentence and why.
The unknown-reason fallback returns the raw string, and a third test pins that
deliberately: a reason this build has never heard of means the server is ahead
of the client, and showing the enum is more honest than inventing a sentence or
hiding the row. It is also what keeps test 1 from being vacuous.
WHAT THIS DOES NOT FIX, stated because the list is the thing a future reader
will trust: the reason vocabulary is DUPLICATED from Go (five constants in
`handlers_items_copy_preflight.go`, five in `internal/store/relation_referents.go`)
rather than generated, so it can still go stale in the one direction that
matters — a reason added to Go and not added here. The test cannot see that.
What it can see is a reason listed here without a sentence, and the list is now
the single place to update. Generating it from the Go constants would close the
gap properly and is a bigger change than this one.
Gates: `npm run check` 0 errors (6 pre-existing warnings in unrelated files),
`make web-test` 126 files / 2118 tests passed. Frontend-only; no Go touched.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
…nd accepted (IDEA-2893)
Comment only; no behaviour change.
A carried relation value naming a live item in a collection the mover cannot
see resolves and survives a same-workspace move, while one naming nothing is
dropped — so a mover can tell those apart, and on a stored REF they also learn
the target's canonical id. The lead's ruling is ACCEPT AND DOCUMENT, and this
is the documentation, placed at the branch that produces the behaviour rather
than in a doc nobody reading that code will open.
Four measurements decided it, and the comment carries the two that matter so a
future reviewer reaches the reasoning instead of re-deriving it:
- NOT ENUMERABLE. A caller cannot choose what to test: create, update and
fields_patch all refuse a caller-supplied ref naming an item they cannot
see, with the COLLAPSED `not_found` wording, so no door turns a chosen
value into a carried one. It can only confirm a value already sitting in
an item the caller can read and did not put there.
- THEY ALREADY HAVE THE VALUE. An ordinary GET returns the raw stored
relation value verbatim; reads apply no redaction. The increment is "it
currently resolves" plus the ref-to-id mapping — not the target's
existence in any general sense, not its title, and backlinks do not widen
it either.
The comment also records why each candidate fix costs more than the increment,
because "we considered it" is worth nothing to a successor without the reasons:
redacting the response closes nothing (the id is in the blob and comes back
from a plain GET); not canonicalising removes only the id half and makes a
relation value stop meaning one thing everywhere; dropping by the MOVER's
visibility silently destroys a valid relation because of who moved the item;
and canonicalising only for movers who can see the target would make the
STORED BYTES depend on who performed the move.
The last paragraph is the one I most want read. The ONE change that would close
this is carrying unresolvable values verbatim instead of dropping them — which
is precisely the drop-and-report rule three lines below, the rule #1246 exists
to enforce. So the comment doubles as a warning: changing that rule changes
this, in the other direction. A design decision that is load-bearing for a
second decision should say so at the place where someone would change it.
I also WITHDREW my own earlier proposal rather than leaving it standing:
"evaluate the carry against the ITEM's access rather than the mover's" was
confused, because an item has no access identity and resolving without a
requester IS the status quo. A plausible-sounding option left in a trail is a
successor's wasted afternoon.
Gates: `gofmt` clean, `go build ./internal/store/`, `go vet ./internal/store/`,
`go test ./internal/store -run 'TestRelation|TestMigrateRelation'` ok. Scoped
to the package because this worktree has no `web/build` for the root embed, and
the change is a comment in one file.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
…e (IDEA-2894)
`f2c4a722` moved the drop-reason mapper out of `CopyItemDialog.svelte` and
broke `TestCopyPreflightDropReasonsAreRenderedByTheDialog`, which reads the
component for that function. CI caught it; my local gates did not, because this
worktree had no `web/build` for the root embed so I had scoped the Go run to
`internal/store` — and I stated that boundary in the commit message while it
was hiding a real failure. Naming a gate's scope is not the same as the scope
being adequate.
THE GATE FAILED THE RIGHT WAY, and that is worth recording. It does not search
the file for `case 'not_found':` and shrug when the file changes; it looks for
the declaration by name and calls `t.Fatalf` if it is gone, saying "this gate
is reading for a function that moved or was renamed, so its green means nothing
until it is repointed". A parity gate that cannot tell "no such reason" from
"no such function" is worse than none, because the second reads as the first
passing.
Repointed at `web/src/lib/items/copyDropReasons.ts` and STRENGTHENED, because
the extraction split the thing it was checking in two. It now requires each
server reason to appear in BOTH:
- `COPY_DROP_REASONS`, the exported list;
- the `MESSAGES` map.
They fail differently, and the first is the one that matters. The module's own
completeness test ITERATES that list, so a reason missing from the list is
invisible to that test as well — this gate is the only place it shows. A test
driven by a list cannot notice something absent from the list.
I ALSO HAVE A CORRECTION TO MAKE, to my own prose in `f2c4a722`. That commit
message and the PR body say the Go-to-TypeScript direction "can still go stale
in the one direction that matters — a reason added to Go and not added here.
The test cannot see that." That is FALSE, and I wrote it without checking: this
parity gate has enumerated the Go vocabulary and required a renderer for every
entry since BUG-2674, which is precisely that direction. My TS test cannot see
it; the repo already had a test that could, and I asserted its absence rather
than looking. Same failure as the fixture I designed around a hazard yesterday
instead of asking whether the product had it — a claim about what is NOT
covered owes a grep exactly as much as a claim about what is.
The PR body is corrected in the same push.
Mutants, all three arms, each restored after:
- remove a reason from the LIST -> FAIL, naming the list and why the module's
own test cannot see it;
- remove its entry from the MESSAGES map -> FAIL, naming the fallthrough;
- rename the `MESSAGES` declaration -> FATAL with the repoint message, so the
fail-safe itself is exercised rather than assumed.
Gates: `go test ./internal/server` ok **279.535s** — the full package this
time, with `web/build` populated so the root embed resolves. `gofmt` clean.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
…rson (IDEA-2893)
Comment wording only.
The attribution now reads `(IDEA-2893, lead ruling day 58; the measurements it
rests on are on that idea's trail, which is where to check this reasoning
rather than take it)`.
The lead asked for `confirmed by Dave in chat` and I declined to write it: Dave
had said nothing to me about this disposition, so the only evidence was a relay
through a channel BUG-2542 proved cannot carry provenance, and the artifact is
a permanent comment asserting what a specific person decided. The lead withdrew
the line and agreed the hold was right. Dave then ruled the general case — no
code comment needs to name him — which is the wording above and is better than
either version, because a ref is CHECKABLE and a name is not. A reader who
doubts this comment can open the idea and read the measurement; a reader who
meets a name can only take it or leave it.
Recorded team-side as CONVE-32 so the successor does not relearn it: code
comments cite the trail, never a person by name. Its scope is source comments
only — commit messages, PR bodies and trail comments are where naming who
decided something is often the entire content, and those artifacts sit beside
their own evidence.
Two things the convention says out loud rather than gloss:
- The rule reached me as a RELAY, and I acted on it because it only ever
REMOVES a claim about a person. Acting on a relay to stop asserting
something is safe in a way that acting on a relay to start asserting it is
not — which is the same distinction that made the hold correct an hour
earlier. Read as general licence to act on relayed instruction it would be
a misreading; the direction is the whole point.
- About nineteen comments already in `internal/` name a person. They are NOT
rewritten. Churning merged history to apply a new rule retroactively costs
more than it returns and a sibling rebasing onto it pays the bill. Fix one
only while editing that comment for another reason.
Gates: `gofmt` clean, `go build ./internal/store/`, and the parity gate green
(`TestCopyPreflightDropReasonsAreRenderedByTheDialog` ok) since this touches
the same file the previous commit repointed it away from.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
xarmian
force-pushed
the
idea/2893-carried-canonicalisation
branch
from
September 5, 2026 17:33
d35ae2f to
c8d0574
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two small, independent units the TASK-2878 review surfaced: one comment recording a decision, one extraction that makes an untested mapping testable. Kept as separate commits because they share nothing but their origin.
92c994a2— record why a carried relation's survival is observable and accepted (IDEA-2893)Comment only, no behaviour change, at the branch that produces the behaviour rather than in a doc nobody reading that code will open.
A carried relation value naming a live item in a collection the mover cannot see resolves and survives a same-workspace move; one naming nothing is dropped. So a mover can distinguish them, and on a stored ref they also learn the target's canonical id.
Four measurements decided the disposition, and two of them are in the comment:
create,updateandfields_patchall refuse a caller-supplied ref naming an item they cannot see, with the already-collapsednot_foundwording. No door turns a chosen value into a carried one, so this can only confirm a value already sitting in an item the caller can read and did not put there.GETreturns the raw stored relation value verbatim; reads apply no redaction. The increment is "it currently resolves" plus the ref→id mapping. Backlinks do not widen it.Every candidate fix costs more than the increment, and the comment says why for each, because "we considered it" is worth nothing without the reasons: redacting the response closes nothing (the id is in the blob and comes back from a plain
GET); not canonicalising removes only the id half and makes a relation value stop meaning one thing everywhere; dropping by the mover's visibility silently destroys a valid relation because of who moved the item; canonicalising only for movers who can see the target would make the stored bytes depend on who performed the move.The paragraph worth reading is the last one. The one change that would close this is carrying unresolvable values verbatim instead of dropping them — which is exactly the drop-and-report rule three lines below, the rule #1246 exists to enforce. The comment doubles as a warning: change that rule and you change this, in the other direction. A decision that is load-bearing for a second decision should say so where someone would change it.
I also withdrew my own earlier proposal rather than leaving it standing. "Evaluate the carry against the ITEM's access rather than the mover's" was confused — an item has no access identity, and resolving without a requester is the status quo, which is what produces the signal. A plausible-sounding option left in a trail is a successor's wasted afternoon.
f2c4a722— extract the copy dialog's drop-reason mapper so it can be tested (IDEA-2894)The mapping from a server drop reason to the sentence a user reads lived inline in
CopyItemDialog.svelte, unexported, with no test file for the component at all. Two separate review rounds found defects in it and neither fix was pinned by anything:not_found, which the server also emits for a target the caller merely cannot see. Telling those apart is the existence oracle the collapse exists to prevent.referent_not_portableread "it points at something in the source workspace", claiming both existence and location for a reason emitted without resolving the target, and whichgithub_prreaches too, where the referent is in no workspace at all.A third defect of the same shape would have been found the same way — by a reviewer happening to read it — or not at all.
Moved to
$lib/items/copyDropReasonswith the reason vocabulary as an exported list, which makes two tests possible that could not be written before:referent_not_portable's message → FAILnot_foundandreferent_not_portablenever claim a target exists, or say where it isThe neutrality test covers those two reasons rather than all ten on purpose:
wrong_collectionlegitimately says the target is outside the field's collection, and it may, because the server only emits it to a caller who can see the target.A third test pins the unknown-reason fallback returning the raw string. That is deliberate rather than a rough edge — a reason this build has never heard of means the server is ahead of the client, and showing the enum is more honest than inventing a sentence or hiding the row. It is also what keeps the completeness test from being vacuous.
A correction to my own claim, and the third commit
An earlier draft of this section said the Go→TypeScript direction — a reason added to Go and never added here — "cannot be seen" by any test. That was false, and I wrote it without checking.
TestCopyPreflightDropReasonsAreRenderedByTheDialoghas enumerated the Go vocabulary and required a renderer for every entry since BUG-2674, which is exactly that direction. My TS test cannot see it; the repo already had one that could, and I asserted its absence rather than grepping for it. A claim about what is NOT covered owes a grep exactly as much as a claim about what is.That gate also broke on the extraction, because it reads the component for the function that moved — and it broke the RIGHT way, which is the third commit's subject. It does not search the file for
case 'not_found':and shrug when the file changes; it looks for the declaration by name and callst.Fatalfif it is gone, saying "this gate is reading for a function that moved or was renamed, so its green means nothing until it is repointed". A parity gate that cannot tell "no such reason" from "no such function" is worse than none, because the second reads as the first passing.cb042921repoints it at the extracted module and strengthens it, since the extraction split the thing it was checking in two. Each server reason must now appear in bothCOPY_DROP_REASONSand theMESSAGESmap. They fail differently, and the first matters more: the module's own completeness test iterates that list, so a reason missing from the list is invisible to that test as well — this gate is the only place it shows. A test driven by a list cannot notice something absent from the list.Mutants, each restored after: remove a reason from the list → FAIL; remove its
MESSAGESentry → FAIL; rename theMESSAGESdeclaration → FATAL with the repoint message, so the fail-safe is exercised rather than assumed.CI caught the break and my local gates did not, because this worktree had no
web/buildfor the root embed, so I had scoped the Go run tointernal/store— and said so in the commit message while that boundary was hiding a real failure. Naming a gate's scope is not the same as the scope being adequate.What this still does not fix
The vocabulary is duplicated from Go — five constants in
handlers_items_copy_preflight.go, five ininternal/store/relation_referents.go— rather than generated. The parity gate makes that duplication safe (a reason added to Go without a renderer fails there), but it does not make it unnecessary. Generating the list from the Go constants is a bigger change than this one.Gates
npm run checkmake web-testgo test ./internal/servergo build/go vet—internal/storego test ./internal/store -run 'TestRelation|TestMigrateRelation'gofmtinternal/serveris run in full after populatingweb/buildso the root embed resolves — the earlierinternal/store-only scope is what let the parity break reach CI.https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8