mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 21:39:01 +00:00
xarmian
7659ad3cd3
* feat(server): the copy preflight says when a relation's target is not usable (IDEA-2899) TASK-2869 made a `needs_value` relation row collectable as soon as it names a target collection. Naming one is not having one: the slug can name a collection that has been DELETED, or one this caller cannot READ. The dialog then mounts a picker that can return nothing and, because the row is not blocked, Confirm stays disabled carrying only the generic required-field message — the user is told a value is missing and never told that no value is reachable. `collection_unavailable` on the needs_value row is the server saying so. THE CLIENT CANNOT COMPUTE THIS, which is why it belongs here. The dialog's destination collection list is filtered through `canEditCollection`, because it drives the copy-INTO picker; a relation TARGET needs only READ access, so a perfectly usable target routinely does not appear in that list. Testing against it would refuse rows the user could have filled in — over-blocking, which is the worse failure and invisible to whoever hits it. `visibleCollectionIDs` is the read-scoped view, and its NAV-LENIENT shape is right here rather than merely tolerable: it includes a collection reachable only through an item-level grant, and the question is "could a picker here return anything at all". One granted item is a picker with one row. DELETED and UNREADABLE are deliberately not distinguished. Same consequence, no client branch would differ — and separating them would tell a caller who cannot read a collection that it nonetheless exists. `omitempty` on a BOOL drops `false`, so the field is phrased NEGATIVELY. Present-and-true means the server checked and the target is unusable; ABSENT means available, or a server that does not report. A client must block only on an explicit true, so absence stays "no information" rather than becoming a value — the rule `access_epoch` follows on the item doors, and the one whose violation cost two review rounds on IDEA-2898 this morning. Costs nothing on the common path: a destination schema declaring no relation field runs no query at all. Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8 * test(server): pin the type gate on collection_unavailable (IDEA-2899) Found by a surviving mutant rather than by inspection: dropping the `def.Type == "relation"` gate left every other test in the file green. Nothing stops a schema declaring `collection` on a field of another type — the validator does not police keys it has no use for — and such a field would then pick up a flag whose meaning is defined only for relations. The dialog would block a perfectly collectable `select` because some relation elsewhere in the same schema points at a collection that happens to be gone. The fixture is the discriminating one: ONE deleted collection, TWO required rows that name it, and only one of them means anything by it. Six mutants on this half, all killed: flag never set, flag always set, deleted target not flagged, unreadable target not flagged, type gate dropped, and the nil-visible-set case (an admin's "no filtering" read as "nothing visible", which would flag every target for the callers who can see everything). Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8 * feat(web): block a relation whose target is unavailable, and stop advising a command that cannot work (IDEA-2899) The client half. `isCollectable` now refuses a relation row the server has flagged, so the row lands in `blockedFields`, Confirm is disabled with a reason, and no picker mounts that could only come back empty. `collection_unavailable !== true` is STRICT on purpose. The field is absent when the target is fine and absent from a server that predates it, so absence must read as "no information". (Over the domain the type admits — `boolean | undefined` — the truthiness spelling is EQUIVALENT and a mutant swapping it in survives; that is recorded in the source rather than papered over with an off-contract fixture. The strict form is kept because it states the contract where the next edit will read it, and the inverse spelling would block every row against an older server.) THE PART THAT IS NOT WIRING: the existing blocked-field notice said the field "is a required <type> field. This dialog can't collect a value for that type safely" and then printed `pad item copy … --field key=value`. Both halves are FALSE here. The type is perfectly collectable; the TARGET is gone. And the CLI runs as the same user against the same referent validation, so the command it prints is refused for exactly the reason the user is already stuck — advice that sends someone to do work that cannot succeed is worse than no advice. So the message branches on `uncollectableReason`, names the collection and the destination workspace, and the CLI line is now gated on `cliFillableField` — the first blocked row the CLI can ACTUALLY fill. `blockedFields[0]` was correct while every blocked row was type-shaped; with an unavailable relation sorted first it named the one field `--field` cannot set either. Eleven unit tests on `copyNeedsValue`, plus a source pin on the dialog whose own measured limit is in its docblock. Client mutants: 7 real, 6 killed, 1 recorded as equivalent with the domain argument that makes it equivalent. Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8 * feat(cli): the copy preview marks an unavailable relation target and stops suggesting it (IDEA-2899) Caught by `TestItemCopyMirrorsMatchServerShapes`, not by me. The CLI keeps a mirror of the preflight response, and adding a field server-side without mirroring it fails that test by design — a mirror that silently lags is a mirror that lies. Working exactly as intended, and the reason this half exists at all. Mirroring the field turned out to be the smaller part. The CLI already prints `target collection: people` for a relation row, and it builds an `Add: --field owner_ref=<value>` suggestion from every unsupplied row. Both are wrong when the target is unavailable: the first sends a user looking for a ref in a collection they cannot read, and the second hands them a command the referent validation refuses for exactly the reason they are already stuck. So the target line is marked NOT AVAILABLE, and the row is excluded from the suggestion with a sentence saying why — modelled on the empty-key branch, which was written for the identical reason (a `--field =<value>` nobody can run) and is three lines away. That the same defect had to be fixed in two places is the shape worth naming: the dialog and the CLI independently built "here is how to supply it" from "here is a field needing a value", and neither had a notion of a field that CANNOT be supplied. The empty-key case was the first instance and was fixed locally; this is the second. Five mutants on this half, all killed: suppression removed, suppression applied to everything, the unavailable label dropped, the explanation dropped, and the mirror field ignored. The available-target control leg is a separate test so the omitempty contract is exercised on this surface too. Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8 * fix(cli): route all three "how to supply it" sites through one predicate (IDEA-2899) Review found the fix applied at one door and not its siblings — my own recurring shape, arriving again. THREE places tell a CLI user how to resolve an unsatisfied field: the detailed `renderItemCopyNeedsValue`, the `--dry-run` summary, and the error the command returns. The first commit fixed the render. The other two went on printing `--field key=value` at someone for whom no value exists — and the ERROR is the line a script or a hurried reader actually sees, so it was the worst of the three to leave. `itemCopyUnfillable` is now the single definition all three consult. Not because three call sites are tidier than one, but because three sites independently answering "how do I supply this" is exactly how they diverged in the first place. The dry-run summary branches three ways rather than two, because the MIXED case is the one a boolean gets wrong: some fields can be supplied and some cannot, and collapsing that either suppresses advice the user needs or offers advice they cannot use. The error hint is suppressed only when NO field can be supplied — with one fillable field left, `--field key=value` is still true. Also pins the BOUNDARY the same review probed: a target collection that is live and readable but EMPTY is deliberately not flagged. The symptom looks identical — an empty picker — but the cases differ where it matters. An unavailable target is unfixable from inside the dialog, so blocking costs the user nothing they had; an empty collection is resolved by creating the item and retrying, and blocking would refuse a copy they were about to complete. It would also cost a live-visible-item count per relation target on a dry run the UI calls on every keystroke. The weaker case — an empty picker that says nothing about WHY — is filed as IDEA-2905 and belongs to the picker. Ten mutants across this round, all killed, including both directions on the error hint and both directions on the dry-run branch. Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8 * fix: unfillable means EITHER reason, and a select never names a relation target (IDEA-2899) Review round 2, two findings, both real and both about a rule stated in one place and enforced in another. **"Unfillable" answered for one of two reasons.** An EMPTY KEY cannot be supplied either — `--field =value` is rejected by this command's own parser, and the detailed render has explained that since Codex round 6. Only that render knew: the --dry-run summary and the returned error went on advising `--field` for those rows, because the predicate I extracted last commit covered the relation reason alone. A predicate named "unfillable" that answers for half its name is a worse trap than no predicate — right at the site that defined it, wrong everywhere it was reused, which is precisely what extracting it was meant to prevent. Two functions now: `itemCopyUnfillable` (either reason — advice), and `itemCopyUnavailableTarget` (the relation half — the render's own sentence, since the two explanations are not interchangeable to a reader). `itemCopyUnavailableTarget` deliberately does NOT also exclude empty keys, though my first version did. A row can carry both faults, and a mutant removing that exclusion survived every test — correctly, because all it changes is printing two sentences that are both TRUE about such a row. The guard was tidiness dressed as a rule; a condition nothing can distinguish is one the next reader has to re-derive. **`Collection` was emitted for non-relation fields**, while its own doc said it is empty for every other type. That was a claim about the schemas people write, not a property of the code: a `select` carrying `"collection": "people"` is storable — field validation has no use for the key and does not police it — and the value was copied straight through, so the CLI printed "target collection: people" beneath a select. A relation fact asserted about a field that has none. `relationTargetSlug` makes the documented contract true at the only place that can make it true; my own type-gate test had created exactly that shape and asserted only the FLAG, not the slug. Three mutants on these fixes, all killed. Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8 * fix(cli): the explanation now names the reason that actually applies (IDEA-2899) Review round 3, and the sharpest miss of this unit — my own, one commit old. Broadening what a predicate ACTS on silently broadened what a sentence SAYS. Once `itemCopyUnfillable` counted empty keys as well as unavailable relation targets, a set of empty-key rows selected the all-unfillable branch and was explained as "the relation target is not available to you" — a false statement about rows that contain no relation at all. Same in the returned error, which is the line a script sees. The tell was there to be read: a sentence that was TRUE while the predicate was narrower is a sentence to re-read the moment it widens. I broadened the predicate deliberately, wrote a commit message about how a half-answering predicate is a trap, and left the sentence describing the half. `itemCopyUnfillableWhy` names the reasons actually present — relation targets, empty keys, or both — and the two one-sentence sites consult it. The detailed render is unchanged: it explains each reason where the row is printed, which is why it uses the narrower count. Four mutants, all killed, including the two that matter: the explanation always saying "relation" (the defect) and never saying it (the same defect pointing the other way). The test carries a mixed-reason leg, because a sentence that picks one of two true reasons is the failure a single-reason fixture cannot see. Also corrected: three comments claiming `itemCopyUnfillable` is relation-only or that the detailed render consults it. Both stopped being true last commit. Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8 * fix: one row can carry both faults, and four docs said this was simpler than it is (IDEA-2899) Review round 4. Four findings, no P1s, and the first is the one worth the round. **A `continue` between the two counts.** `itemCopyUnfillableWhy` counted a row as an unavailable relation target and then skipped the empty-key check, so ONE row carrying both faults reported only the first. My mixed-case test used TWO rows with one fault each — a different input, and the only one it exercised. Two rows with one fault each and one row with two are not the same fixture, and I built the weaker one while writing a commit message about fixtures that cannot discriminate. **The dialog could still print `--field =value`.** `cliFillableField` excluded unavailable relation targets and not empty keys, so a required `json` field the destination reported with no key was type-shaped, blocked, and still offered a command the CLI's own parser rejects. The CLI has refused those since Codex round 6; the web side had never learned it. Same defect, other surface — which is the third time this unit has fixed one door and not its sibling. **Cardinality.** "no --field can supply it" for several fields, and "reported them with an empty key" for one. Both sites now agree with their counts, and the empty-key phrase is neutral on number so it reads correctly after either. **Four documents claimed every needs_value row is resolvable with an override** — the CLI renderer's docblock, the server's `NeedsValue` field, the CLI mirror type, and the dialog's collectability comment. That was true when each was written and this unit falsified all four; a reader following any of them would conclude the CLI had simply forgotten to print a flag. Two mutants on the fixes, both killed: the `continue` restored, and the dialog's empty-key exclusion removed. Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8 * refactor(cli): one tally, because the rounds said the branching was the problem (IDEA-2899) Four review rounds returned 2, 2, 2 and 4 findings. The counts looked like slow convergence; the DISTRIBUTION was the finding. Every defect after round 1 lived in this one layer — how the CLI and the dialog say "here is how to supply it" — while the server half that computes availability stayed clean throughout. The layer had accreted exactly the way IDEA-2898's cold path did: a count, then a second count for the other reason, then a phrase function, then a `continue` between two counters that made a dual-fault row report half of itself. Round 4 fixed something round 3 introduced to fix something round 2 introduced. That is not a run of bad luck, it is a shape. So this round removes branches instead of adding a seventh guard. `itemCopyTally` walks the rows once and returns what every caller needs; `AllUnfillable()` is the condition both one-sentence sites test, and `Why()` is the phrase both interpolate. Three helpers become one type. There is no second definition of "unfillable" to drift from the first, and no sentence describing a subset of what a predicate counts, because the sentence and the count come from the same walk. `Unfillable` is deliberately NOT `UnavailableTarget + EmptyKey`: one row can carry both, and double-counting makes `Unfillable == Total` false for a set that is entirely unfillable — the comparison every caller makes. A mutant does the addition and dies. Five mutants, all killed. The last needed a new test rather than a new fixture: `AllUnfillable`'s `Total > 0` guard is unreachable from both current callers, so a mutant removing it survived every command-level test. Keeping an unreachable guard and calling it defence is how a promise becomes a lie, so the tally is now unit-tested directly — an empty set is not "entirely unfillable", and a future caller outside the `len() > 0` gate would otherwise be told silently that nothing can be supplied. Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8