mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 15:05:40 +00:00
xarmian
367aae8e18
* fix(collections): a DERIVED prefix is A-Z only (BUG-2943) DerivePrefix took the first BYTE of each word, so a collection named 'TEMP Rook A 2870' got the prefix 'TRA2'. parseItemRef resolves a PREFIX-NUMBER ref only when every prefix character is A-Z and otherwise falls through to a slug lookup, so every item in that collection printed an issue ID the CLI then refused: 'pad item show TRA2-2942' answered 'item not found' while the slug resolved fine. Two functions, each locally reasonable, disagreeing about what a prefix may contain — and the generator was the permissive one, so the failure surfaced at read time on an identifier the product itself minted and printed. The first-BYTE bug had a second half: a word starting with a multi-byte rune contributed a UTF-8 lead byte, so a collection named in most non-Latin scripts produced a prefix that is not even valid text. Non-letters are SKIPPED rather than mapped — there is no honest A-Z substitute for '2' or 'Omega', and inventing one puts a character in the ID that is in nobody's collection name. A name with no ASCII letters yields the empty string, which store.CreateCollection already turns into its ITEM fallback. SCOPE, stated because the first draft of this message overstated it (codex round 1 [P2]): DERIVED prefixes are safe now; the INVARIANT IS NOT ENFORCED. Three other doors store a prefix verbatim and unvalidated — CreateCollection with an explicit input.Prefix, UpdateCollection, and workspace import — so the same unresolvable-ID defect is still reachable through the API, the --prefix flag and a restore. Named on the trail with their call sites, held for a ruling rather than swept into this commit, because the import door wants a different answer from the other two: refusing a restore is not obviously right. The parity test lives in internal/store, where parseItemRef is: it asserts the generator against the RESOLVER rather than against a restatement of the resolver's rule, which is how these two drifted apart in the first place. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * fix(store): one prefix grammar at all four doors, and the parser widens to it (BUG-2943) The ruled shape, which dissolves the import dilemma rather than choosing a side of it: collections.IsValidPrefix is the single definition — an uppercase letter followed by uppercase letters or digits — and parseItemRef now asks it instead of carrying its own stricter A-Z rule. Because the PARSER widened, a workspace already carrying a prefix like AB1 resolves every item by its printed ID the moment this ships. No migration, no rewrite of an identifier a user's other records may reference. The four doors: - derive: unchanged from the previous commit, still letters-only, still within the grammar; - create with an explicit prefix: REFUSED if outside the grammar, with a message naming the rule. The caller typed it, so a refusal is actionable; - update: same, and it matters more here — update is the door someone reaches for to FIX a bad prefix, so it must not accept another one; - import: the most permissive door that can still be honest. Anything the parser resolves is accepted (which now includes digits); only a prefix NO surface could resolve is refused, naming the collection and saying the export can be edited. Carrying that verbatim would restore a workspace whose items print IDs the CLI answers 'not found' to, which is this item's defect rather than a compatibility owed. An ABSENT prefix on import is not an unresolvable one. Old exports and every fixture in the suite carry "", and the first version of this check refused them — turning a fix for unresolvable IDs into one that cannot restore an old bundle at all (caught by three server tests). It now takes the same derive-then-ITEM fallback CreateCollection applies, which also upgrades it: an empty prefix is itself unresolvable, since the ref would begin with a dash. A prefix accepted only because the parser widened is logged at WARN, so an operator can see an id-space that would have been rejected before rather than inferring it from a resolve failure that no longer happens. Tests for each door follow in the next commit. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * test(store): one test per prefix door, plus the parser round trip (BUG-2943) Each door asserted separately: 'they all call the same helper' is a claim about the code, not about behaviour, and the bug was two definitions disagreeing. - create with an explicit prefix: AB1 accepted and resolves; ab1, 1AB, 'A B', A-B, A!, a non-Latin letter and a bare digit refused, with the rule named; - update: AB1 accepted, a bad replacement refused AND the stored prefix unchanged after the refusal — update is the door someone uses to FIX a bad prefix, so it must not swap one unresolvable id-space for the next; - import: a digit-bearing prefix restores unrewritten and resolves; one no surface can resolve is refused naming the collection and the export; an ABSENT prefix takes the create-path fallback and comes back resolvable; - the parser: every prefix the doors accept round-trips, and 1AB / 9 / 'A B' / A! / a trailing dash / a bare prefix stay refused. One correction: my first version of the parser test asserted that 'ab1-42' is refused. It is not, and the code is right — parseItemRef upper-cases before splitting, which is what makes "pad item show task-5" work. Case-insensitivity is now PINNED rather than mis-asserted, because a later reader working from the grammar comment alone would otherwise 'fix' it. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR (This message was rewritten once: the sentence above lost its example because the original was written with backticks inside a double-quoted shell string, which the shell EXECUTED and replaced with the command's empty output. The span was blank in the commit as first written.) * fix: the widened grammar reaches its consumers too (BUG-2943) Codex round 2. Widening parseItemRef without widening what CONSUMES a ref would have left the same two-definitions bug this unit is about, introduced by its own fix: - cmd/pad/cmd_github.go matched [A-Z]+-\d+, so 'pad github link' on a branch carrying a digit-bearing ref silently found nothing; - web localSearch's palette Enter fast-path could not recognise one either. Both now match collections.IsValidPrefix. Tests strengthened, both on codex's reading: - the import fallback pinned the VALUE, not just resolvability — asserting 'non-empty and parseable' passes an implementation that stamps ITEM on every absent prefix, giving every collection in a restored workspace the same id-space. Two legs now: an ordinary name derives TASK, a letterless name falls through to ITEM, which is what makes it 'derive, THEN ITEM'; - the WARN the ruling asked for had no test, so it was a line nobody would notice was gone. Now asserted, with a control that an ordinary prefix does NOT warn — a log everything trips is a log an operator learns to skip. Three comments still described the parser's A-Z rule as current, including one in the file that changed it. STILL OPEN, on the trail for a ruling: web paneTarget.ts keeps the narrow grammar on PURPOSE — its comment argues a digit-permitting shape would misclassify a slug like 'roadmap2-5' as a ref — and that argument cited the server rule this unit just widened. Whether the guard follows or stays is a question about the widening's blast radius, not a line to change quietly. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * fix(web,store): the pane guard follows the server, and the precedence is written down (BUG-2943) Both lead-ruled after codex round 2 surfaced them. paneTarget's REF_SHAPE kept a LETTERS-ONLY grammar deliberately, citing the server's A-Z loop as its warrant. The server dropped that rule, so the guard was holding a grammar nothing else holds — which does not avoid a wrong answer, it produces a different one. It now matches IsValidPrefix. The cost is real and is stated in the test rather than buried: an HREF whose last segment is ref-shaped under the wider grammar is compared by NUMBER with the prefix discarded, so a genuine slug like 'roadmap2-5' now counts as the same pane target as TASK-5. The existing test pinned the opposite and is REPLACED, naming what changed and why. The prefix is dropped because a moved item keeps a stale one (the server's own number-only fallback) and PaneGuardItem carries no prefix to compare; tightening that means widening that type and its callers, which is a separate change and is on the trail. The SLUG-channel leg is kept as its own test: provenance, not grammar, is what protects it — a target naming an item by slug is judged only as a slug. ResolveItem's ref-before-slug precedence is now documented on the function and pinned in both directions: 'ab1-42' resolves as a SLUG when no AB1-42 exists, and a live ref wins when it does (case-insensitively). The widening made more strings ref-shaped, so 'is my slug still findable' needed an answer that does not depend on reading the resolver. Web unit tests run here via a node_modules SYMLINK to the main checkout, which CLAUDE.md permits; npm ci was not run and must not be. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * fix(web): the self-pane guard was the fourth copy of the ref grammar (BUG-2943) Codex round 3 [P1]. The route-level refNumber() still matched [A-Za-z]+, so a master whose ref is R2-1 parsed as null, its item_number fell back to 0, and the same-item guard stopped recognising ?item=R2-1 as the master — mounting a second provider for the item already on screen. That is the FOURTH consumer found carrying its own copy of this grammar (github branch extraction, the search palette, the pane target guard, and now this). Four independent copies is the argument for the shared definition rather than for four careful edits, and it is why the widening had to be swept rather than applied where it was noticed. Also from round 3: comments saying these client regexes 'match collections.IsValidPrefix' were imprecise — the validator accepts uppercase only, while the client patterns accept either case on purpose, because a user types a ref however they like and the server upper-cases before splitting. They mirror the ref GRAMMAR, and now say so. Web gates run here through a node_modules symlink to the main checkout (permitted; npm ci is not): vitest 2195 passed, svelte-check 0 errors. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * fix(collections,e2e): the fifth and sixth copies, and the docs that taught the old rule (BUG-2943) Codex round 4, after I claimed the sweep was complete twice. - Two SEEDED PLAYBOOK BODIES carry their own ref grammar and instruct agents with it: playbook_library_plan.go and templates_sdd_spec.go both said a ref 'matches ^[A-Z]+-\d+$'. An agent following those literally would refuse to treat AB1-42 as a ref — a grammar copy that lives in PROSE and is executed by a reader rather than a regexp engine, which is why two sweeps of the code missed it. - Three e2e comments taught the defect as a rule: one of them carries the empirical confirmation ('GET /items/BS1-10 404'd while the slug worked'), which is precisely this bug. They now say the by-ref 404 is fixed and that the explicit prefix those suites pass buys DETERMINISM rather than dodging it. Counting honestly: six live copies of one grammar, found in four rounds of review, two of which I opened by asserting there were no more. The shared definition is the fix; every one of these was a place that had quietly made its own. Gates: go test ./... 0, make lint 0 issues, vitest 2195 passed, svelte-check 0 errors (web run through a node_modules symlink to the main checkout — permitted; npm ci is not). Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR