6 Commits

Author	SHA1	Message	Date
xarmian	b2c303c4bb	docs(links,store,models): cite markdown.ts by symbol, and check it (BUG-2832) ... Go comments describe the web renderer constantly and cite it by LINE NUMBER. Nothing verifies those citations — they cross a language boundary, so no compiler, test or linter has ever checked one — and they had drifted onto unrelated code. This converts all 32 to `markdown.ts::symbolName` form and adds the check that makes the conversion worth something. Scope note, because this is wider than the rider it was dispatched as. The BUG-2834 commit added the pattern constant near the top of markdown.ts, shifting the file by +45 lines and invalidating EVERY line citation into it — including the three BUG-2832 had confirmed were still accurate. Leaving 13 knowingly-wrong citations because they sit outside the files this unit otherwise touched is not the neutral option when this branch is what broke them. Happy to split this commit back out if the lead would rather hold the rider to its stated bound. While converting, five of the filing's six "suspect, not established" citations were settled by reading the shifted positions: :307, :478-481, :485, :513 and :516 point at a @param doc line, unescapeDocLinks, REF_PATTERN, the tail of parseCrossWorkspaceBody, and findItemByRef respectively. All substantively stale, not merely off-by-lines. That answers the filing's open question. Two guard tests, per the filing's own proposed fix shape: TestMarkdownCitationsNameLiveSymbols verifies every cited symbol is really declared in markdown.ts. This is the check a line number could never have. TestMarkdownCitationsAreNotLineNumbers bans the line-number form, so the fix cannot erode the next time someone reads a number off their editor gutter. The first version of the symbol check FAILED its negative control and that is the part worth reading. It asked strings.Contains(ts, "function "+sym) — a PREFIX match. Renaming resolveWikiBody to resolveWikiBodyRENAMED leaves "function resolveWikiBody" a substring of the renamed declaration, so the guard stayed green through precisely the rename it exists to catch. It passed its first real run and would have shipped as coverage. Fixed by requiring the following character to be one that cannot continue a JS identifier; the control now fires and names the symbol. Both guards are non-vacuity-asserted: the sweep fails if it finds fewer than 50 Go files, and the symbol check fails if it finds no citations at all. Currently verifying 7 distinct symbols across 29 citation sites. The line-number guard earned its keep before being committed — it caught three citations silently reverted when a file was restored from a snapshot taken before the conversion.	2026-08-31 23:24:27 +00:00
xarmian	427540706c	fix(documents): bound the rename cascade at the title and at the projected total (BUG-2798, BUG-2796) (#1218) ... * fix(documents): bound the rename cascade at the title and at the projected total (BUG-2798, BUG-2796) A document rename rewrites [[oldTitle]] into every linking document. Neither factor of the output size was bounded: titles had no length validation, and the cascade holds every rewritten body in memory before writing any of them. One rename could project 10 GB from a 500 KB input -- 20,000x, measured -- and OOM while holding the workspace rename lock. Two walls, per Dave's day-63 ruling. 1. Title length, bounded at write time (models.MaxDocumentTitleRunes = 255). Runes, not bytes: "255 characters" is what a user and a UI counter mean. Existing over-limit titles stay valid until their next rename -- no retro-breakage of stored data. 2. The cascade's projected TOTAL, bounded at 16 MiB (store.MaxRenameCascadeProjectedBytes), accumulated across the linking set and refused before the first rewrite is built. The total is the right quantity and a per-document cap would not have been. Measured, with the title bound already in place: one linker holding the largest body a 2 MiB request can carry projects 108,632,370 bytes -- 51.8x -- and the aggregate is linear in the number of linkers (108.6 / 217.3 / 434.5 MB at k = 1/2/4, allocation tracking output at ~1.02x). A per-document cap of C still admits k * C, which is the same unbounded shape one level up. The 16 MiB figure has a receipt in the constant's doc comment: it sits above the absolute ceiling of any cascade this development instance could produce (its entire wiki-linking corpus is 10,077,476 bytes) and 6.5x below the single-document attack. The refusal is permanent-shaped and deliberately NOT in ErrLinkCascadeContention's family: 413 with the projection in the message and no Retry-After. Contention means "someone got there first, try again"; this means "this rename cannot be performed as asked". Answering it from the retryable family would tell a client to retry forever. BUG-2796 folds in at the same validation point, as ruled -- a title containing wiki-link syntax is emitted raw by links.ReplaceTitle, so renaming to `A]] [[A` produced two broken links and reported success. The rule is derived from the two mechanisms that consume a stored bracket (the grammar at markdown.ts:327 and the unescaper at markdown.ts:753) rather than from a character blacklist: the first version of this fix banned `]`, `\` and `|` because all three "look like wiki-link syntax", and the round-trip test refuted two thirds of that. `|` in particular is a title shape resolveWikiBody contains a dedicated branch to support, and `[` passes the grammar untouched. Doors enumerated rather than assumed (CONVE-24): store.CreateDocument and UpdateDocument have exactly two callers between them, both HTTP handlers. No CLI, import, or seed path writes a document title. Update previously validated doc_type and status and NOT title -- the one field that drives the cascade -- so the handler tests drive real requests through both doors (CONVE-19). BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): count retained bytes, bound the retry path, escape the cascade's LIKE pattern (BUG-2798) Codex round 1 on #1218. Three findings, all real, all fixed here. 1. The guard bounded projected OUTPUT, which bounds nothing when the new title is SHORTER than the old one. Renaming a 255-character title to a one-character title makes each 2 MiB linker project ~40 KiB while the cascade still retains its 2 MiB read for the compare-and-set, so hundreds of linkers exhaust memory while the counter reports well under the cap. The counter now sums RETAINED bytes — read plus written, both alive at once — so the cap is a statement about resident memory rather than about output. MaxRenameCascadeProjectedBytes becomes MaxRenameCascadeRetainedBytes and moves 16 -> 32 MiB, because the legitimate ceiling it clears doubles under the new metric (that instance's whole wiki-linking corpus retains ~20,154,952 bytes); the single-document attack retains 110,729,522, so it is still refused by 3.3x. 2. The compare-and-set's retry path bypassed the guard entirely. On contention it re-reads the linker and calls ReplaceTitle on whatever the winner wrote — a NEW input, bounded by nothing the scan had checked — so a content edit landing inside the cascade's window could grow a linker from harmless to enormous and walk the rename back into the amplification it would have been refused for. Each document's compare-and-set now carries the cap less what the other linkers hold, and re-checks the grown body against it. 3. The cascade's `content LIKE ?` search term went in unescaped, so a document TITLE decided how the pattern was read. `\` is the default LIKE escape character on Postgres and NOT on SQLite, so `[[Alpha\Beta]]` was searched for as itself on one dialect and as `[[AlphaBeta]]` on the other: linkers not found, cascade rewrites nothing, rename reports success, every link left stale. Silent and dialect-dependent. Codex named the backslash; `%` and `_` are the rest of the class (CONVE-18) — wildcards on both dialects, so a title carrying them selects documents that do not link it. An explicit `ESCAPE '\'` clause plus escapeLikePattern makes both dialects agree, rather than leaving SQLite correct by accident. Finding 3 also constrains finding 3 of the ORIGINAL fix: models' validator allows a lone backslash in a title on the grounds that both renderers handle it, which was true of rendering and false of cascading. That comment now records the dependency — allowing it is only correct while the cascade's pattern stays escaped. Tests, four new, each mutation-verified against the code it guards: - CountsRetainedBytesNotJustOutput — the shrinking rename. Asserts as a PRECONDITION that the projected-output total stays under the cap, so the test cannot pass for the old reason. - RetryRecheckesTheBudgetAgainstTheGrownBody — drives the real race through the afterLinkCascadeRead seam. POSTGRES ONLY and skipped loudly elsewhere: SQLite's BEGIN IMMEDIATE closes the window structurally, so a green run there would be a property of the DSN. - FindsLinkersWhoseTitleContainsABackslash — Postgres only, same reasoning inverted: SQLite is the dialect that was accidentally right. - DoesNotSpendTheBudgetOnDocumentsThatDoNotLinkTheTitle — `%` and `_`. Its first version asserted the decoy's content was untouched and passed against the unescaped pattern, because over-matched rows rewrite to themselves. The observable harm is that they spend the caller's budget, so that is what it now asserts. Mutation matrix for this round: output-only counter -> only the shrinking test fails; retry check removed -> only the retry test fails (PG); LIKE unescaped -> the budget legs fail on SQLite and the backslash test fails on PG. Gates: `go test ./...` under Postgres 17 EXIT=0; SQLite packages EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): tighten the retry budget, stop charging no-op rewrites, order the typed check first (BUG-2798) Codex round 3 on #1218, an edge-case angle over the new arithmetic and control flow. Three findings fixed, one declined. 1. The retry budget credited back this document's own share, on the reasoning that the retry replaces it. It does not: the original read and rewritten bodies stay reachable through `updates` while the write loop runs, so the re-read and its rewrite are allocated ON TOP of them. The bound could be exceeded by up to one document's share while the arithmetic still reported it satisfied. The budget is now the genuine headroom, `cap - retained`. 2. A concurrent edit that REMOVES the link left a body with no occurrences, which cascadeRetainedBytes still charged twice — once for the read and once for a rewritten copy that does not exist, because strings.Replace returns its input unchanged when there is nothing to replace. That could refuse an otherwise valid rename for memory the cascade never allocates. 3. The handler classified this error by PROSE before testing it by identity. The UNIQUE-constraint arm matches a substring, and the refusal error embeds the caller's title verbatim, so renaming a document to a title containing the words "UNIQUE constraint" came back as a 409 name collision — advice to pick a different name, for a rename that was refused for size and would fail identically under any name. Typed sentinel now tested first. DECLINED: unchecked int64 arithmetic in the projection. The multiplicands are derived from the length of a string already resident in memory, so overflowing int64 needs a single document body of roughly nine exabytes; and the accumulator returns as soon as it passes the cap, so it cannot run away either. Saturating arithmetic here would be guarding a state the machine cannot reach. Tests, three new, each mutation-verified: - RetryBudgetExcludesThisDocumentsOwnStrings — deliberately separate from the existing retry test, because that one catches the check being ABSENT and this one catches it being too GENEROUS. The grown body is sized to fall BETWEEN the two budgets; a body far over the cap cannot tell them apart. - ConcurrentEditThatRemovesTheLinkDoesNotRefuseTheRename — its first version sized the link-free body against the CAP rather than against the retry's real headroom, so the refusal it caught was correct behaviour and the test was wrong, not the code. Re-sized against the headroom: fits when charged once, does not when charged twice. - IsNotMisreportedAsATitleCollision — at the handler, since the defect is entirely in its classification order. Mutation matrix for this round: credit the share back -> only the tight-budget test fails; charge the no-op body twice -> only the link-removed test fails; order the substring arm first -> only the misclassification test fails. Gates: `go test ./...` under Postgres 17 EXIT=0; touched packages re-run after the lint fix EXIT=0; gofmt clean; `make lint` 0 issues. CI green on b09aca12. BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * test(documents): close four ways the cascade-bound tests could pass for the wrong reason (BUG-2798) Codex round 4, aimed at the TESTS rather than the code. No production behaviour changes here; four instruments that were weaker than they read. 1. Every retained-byte case exceeded the cap under `max(read, rewritten)` as well as under `read + rewritten`, so none of them could tell the two arithmetics apart — and taking the larger would hold twice the cap. Added CountsBothStringsNotTheLargerOne: an ordinary same-length rename over content totalling ~60% of the cap, which the sum refuses and the max admits. Its preconditions assert both halves of that gap. 2. Nothing pinned the 255 itself. Every length case derived its inputs from MaxDocumentTitleRunes, so changing the constant to 512 left them all green. That is fine for arithmetic and wrong for this number: it is a product decision Dave ruled, and a silent change to it silently changes how much amplification the cheap door lets through. Deliberately NOT done for MaxRenameCascadeRetainedBytes, which is mine and carries a measured receipt that is expected to be re-measured. 3. The oversize tests asserted only THAT a rename is refused, never that it is refused BEFORE the amplified string is built — which is the entire point of the guard. Moving links.ReplaceTitle above it would have kept them green. Added RefusesBeforeBuildingTheRewrittenBody, measuring cumulative allocation with a ~20x margin: refusing costs the one body it had to scan, building first costs ~108 MB. Verified by mutation — with the guard moved after the rewrite it reports 110,748,144 bytes against a 52,428,800 ceiling. This filing warned that measuring memory to prove the ABSENCE of amplification is flaky by construction. That still holds for the shape it described, a peak-RSS floor. This is the opposite: a generous ceiling on a deterministic counter, with the two outcomes twenty times apart. 4. The "reports the projection" assertions checked for the words `maximum` and `bytes`, which a message saying "maximum bytes exceeded" would satisfy while telling a caller nothing. They now require the cap's actual value and a real byte count. Mutation matrix: charge only the larger string -> only CountsBothStrings fails; move the rewrite above the guard -> only RefusesBeforeBuilding fails. Seventeen mutations across four rounds, each detected by the test that should catch it. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): compose the 413 from typed fields instead of splicing err.Error() (BUG-2798) Codex round 5, on the side effects of a REFUSED rename. Side effects were otherwise clean — rollback removes versions, link rewrites, attachment stamps and the title change, and no activity row, SSE event or webhook is emitted — but the response body was built by appending err.Error() to a public sentence. That published whatever any layer had wrapped around the error on its way up. Today that is "update links: store: ", which is a call path clients have no business seeing; tomorrow it is whatever the next wrapper adds, with no decision point in between. The response is now composed from typed fields on a new store.RenameCascadeTooLargeError (NewTitle, Retained, Max), reached with errors.As. Unwrap keeps errors.Is(err, ErrRenameCascadeTooLarge) true, so every existing sentinel check is unaffected. Both FIGURES stay in the message, deliberately: Dave's day-63 ruling asked the refusal to state what it would hold and what the cap is, so "split the rename" is actionable advice rather than a shrug. The reviewer read those numbers as a content-size oracle; that framing does not survive the trust boundary — a rename requires `editor`, documents are readable at `viewer`, so the caller can already read every document the figure summarises and learns nothing from it. What they had no business receiving was the internal call path, and that is what changed. This is round 3's lesson applied in the other direction. There, prose was being used to CLASSIFY an error and should have been identity. Here, prose was being used to REPORT one and should have been data. The test now asserts both halves: the real byte counts are present (not merely the word "maximum"), and the strings "update links:" and "store:" are ABSENT. Mutation-verified — splicing err.Error() back in fails on both leaked prefixes. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * docs(documents): correct eight claims the prose made that the code does not (BUG-2798) Codex round 6, aimed at the comments rather than the code. Eight findings, all mine, all real, no behaviour changed. This is the failure mode my own trail keeps naming — code right, prose broader than the sweep, always in the same direction — so they are corrected individually rather than smoothed over. Stale after the round-1 rename: - models cited store.MaxRenameCascadeProjectedBytes, which no longer exists. - the constant's own hostile figure read 110,729,522; it is 110,729,520. - the HTTP test said each body is ~135 KB; the formula produces 264,790 bytes. Claims wider than what is true: - The round-trip test's header stated a biconditional over the whole validator. False: a 300-rune title round-trips perfectly and is still refused, for the unrelated reason that it is an amplification factor. The property is about the SYNTAX rule, over titles inside the length bound, and now says so. - The mirrored grammar/unescaper comment claimed that a TypeScript change would make this test start disagreeing. It cannot — they are static copies, and nothing in the repository fails when the two drift. Replaced with what the duplication actually buys and what it does not. - The cap's receipt used `items` measurements to conclude the guard "cannot fire on honest use" for DOCUMENTS, having itself noted that instance's documents table is empty. The proxy is reasonable and it is an assumption, not a measurement of the guarded path; the inference is now named, with the narrower claim that survives without it. - The 413's comment cited the image_too_large precedent as a bound on OUTPUT while this guard bounds retained read-plus-write. What carries across is the shape — a small request refused for what handling it would cost — not the quantity. - TestRenameCascade_RefusesTheSingleDocumentAttack described the 2 MiB / 110,729,520-byte shape it does not build; it sizes from the cap (1,271,000 bytes retaining 67,108,800). The full-strength shape is exercised by the allocation test, and the comment now points there instead of describing a body that is not in the function. One figure was replaced by measurement rather than corrected by arithmetic: the allocation test claimed a "~20x margin". Both sides are now measured and stated separately — refusing allocates 2,114,624 bytes, ~24.8x under the 52,428,800 ceiling; building the rewrite first allocates 110,748,144, ~2.1x OVER it, taken from running the test against the mutation rather than computed. The smaller margin is the binding one, since it is the gap a regression must cross to be caught, and saying "~20x" hid that. Gates: `go test` on the three touched packages under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): stop charging the cascade budget for rows the rewriter cannot touch (BUG-2798) Codex round 7. Third instance of one class, and the one I stopped short of when I extended the previous two. The SELECT that finds candidate linkers is a LIKE; the thing that rewrites them is links.ReplaceTitle. They do not agree on what counts as a match, so the SELECT returns a SUPERSET — and every row in the difference was charged to the caller's retained-byte budget and then handed a no-op UPDATE. Instances one and two were `%` and `_`, wildcards on both dialects, closed by the ESCAPE clause. This is the case half, and it splits the OTHER way from the backslash bug: SQLite's LIKE is ASCII case-insensitive by default while Postgres's is case-sensitive, so renaming `Alpha` scans every body holding `[[alpha]]` on SQLite only. ReplaceTitle is case-sensitive on both and will never touch them, so enough case-variant content could push an otherwise valid rename to a 413 — on one dialect, for content that was never in scope. The fix is to skip a row with no case-sensitive occurrence outright, which closes both halves: no budget is spent, and no pointless UPDATE is issued for a body the cascade was never going to change. The authority on what is a linker is the rewriter's own count, not the pattern that proposed the candidate. The test runs on BOTH dialects deliberately, unlike its Postgres-only backslash sibling: on Postgres it asserts the behaviour was already correct, which is what makes it a regression test rather than a SQLite quirk shim. It also asserts the case variants are left byte-identical — `[[alpha]]` is a different link, not a missed one. Mutation-verified: restoring the charge fails it with 33,554,790 bytes against the 33,554,432 cap. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. An earlier attempt at that gate died on host disk exhaustion, not on this diff — 373 stale go-tmp directories from crashed runs, cleared, re-run green. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): report the whole operation's size when a retry is refused (BUG-2798) Codex round 8, on concurrency and the rest of the rename transaction. The advisory-lock lifetime, the CAS loop's termination, the ordering against attachment stamps and version writes, and the new skip's effect on the transaction's invariants all came back clean. One P2 stood. The retry path bounded correctly and REPORTED wrongly. It compared the re-read body against the headroom — right — and then named only that body in the error. Everything the scan counted is still held, so the operation's real size is the scan total plus the re-read, and a refusal could therefore say it would hold 16,777,200 bytes against a limit of 33,554,432: a refusal whose own figures do not justify it, which reads as a server bug rather than as advice you can act on. The compare-and-set is now handed the scan TOTAL instead of a pre-computed budget, so the same number both bounds and explains: refuse when scanTotal + grown exceeds the cap, and report scanTotal + grown. The test now asserts the refusal justifies itself — the figure reported must exceed the cap it cites — via the typed error added in round 5, which is what makes that property checkable at all rather than a string comparison. Mutation-verified: reporting the re-read alone fails it with exactly the 16,777,200-against-33,554,432 shape. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): refuse titles whose links the cascade cannot find; count retry buffers (BUG-2798) Codex round 9, asking what is MISSING rather than what is wrong. Three findings: two fixed, one already filed. ## Titles validated against the wrong layer The validator ACCEPTED `Alpha|Beta` and `Alpha\Beta`, and I defended that choice with a test. Both round-trip perfectly — the renderer reads each back as exactly the title it started from — and the first version of this fix banned them on vibes, so being shown that was a real correction. It was still the wrong call, for a reason that test could not see. A link to such a title can be STORED escaped, as `[[Alpha\|Beta]]`, and the rename cascade searches for the raw `[[Alpha|Beta]]` only. It does not find those links, so the rename succeeds and leaves them pointing at a title that no longer exists — silently, which is BUG-2796's defect wearing different syntax. So the property a title has to satisfy is stricter than the one I tested: not "the renderer reads it back", but "the renderer reads it back AND the cascade can find its links". Validating against the layer that DISPLAYS a title while the layer that MAINTAINS it disagrees is the same mistake as the unescaped LIKE, met from the other side — twice in one unit, which is the part worth noticing. `[` stays accepted: the cascade's search term matches it literally, so it passes the stricter property too. The two characters get their own test rather than a row in the round-trip table, because the table asserts a biconditional and these are refused for a reason that predicate deliberately does not model. That test asserts its own premise — each title must still round-trip — so if that ever stops being true it fails rather than passing for a new reason. ## Retry buffers were not counted rewriteLinkerCAS bounded each retry against the scan total plus THAT attempt. Earlier attempts' buffers become unreachable when expected/next are reassigned, but unreachable is not reclaimed, so a run of failures could hold several copies while the arithmetic counted one. Now accumulated across attempts, which is conservative — it counts garbage as if live — and errs toward refusing, which is the safe direction for a memory bound. The loop is capped at cascadeRewriteAttempts, so it cannot grow without end. ## Already filed Item renames remaining unbounded, and item titles lacking this validation, are real and out of this unit's scope: BUG-2804 and BUG-2805, filed after round 2 with the code verified rather than taken on report. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): validate a title only when the rename actually changes it (BUG-2798) Codex round 10, on back-compatibility. This one is a regression THIS fix introduced, not one it inherited, and it falsified a promise the fix makes about itself in three places. "Enforced at write time; existing titles stay valid until their next rename" is Dave's ruling, and it is repeated in the constant's doc comment and in two commit messages. Validating every SUPPLIED title broke it for the most ordinary shape of an edit there is: a client that PATCHes the whole object, title included, to change the content. Under that, a document with a legacy title became uneditable rather than merely un-renameable — the opposite of grandfathering. Grandfathering is not something you get by validating at write time. It is something you get by not validating a write that is not a rename. The check now fires only when the supplied title DIFFERS from the stored one, which is also exactly the test the store already applies before cascading, so the validation and the work it guards now agree on what counts as a rename. The regression test seeds its legacy document through the store, because the title it needs can no longer be created through the API — which is precisely the population the grandfathering clause exists for. Three legs: the echoed-title content edit succeeds, a title-less content PATCH succeeds, and renaming to another invalid title is still refused. The last is the control; without it, deleting the validation entirely would pass. Filed rather than folded: BUG-2806, existing documents whose links are stored in escaped form are still orphaned by a rename. Round 9 stopped NEW titles of that shape being created; it did not repair the ones already stored, and the asymmetry — the product refusing to create a shape it still mishandles — belongs in the record rather than in this PR, which is nine commits deep on a bound it has already outgrown. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): validate the rename under the lock, not against a pre-lock read (BUG-2798) Codex round 11. Round 10 moved title validation behind "only when the title actually changes", which is the right rule and was applied at the wrong place. The handler compares the supplied title against a document it read BEFORE the rename lock. UpdateDocument re-reads under the lock. Those can disagree: echo a legacy title back on a content edit while another request renames the document, and the handler sees "unchanged, skip validation" while the store sees a genuine rename — and writes the legacy title through with nothing having checked it. The rule is unchanged; the enforcement point moved to where the rename is actually decided. The store validates inside the transaction, on the same branch that triggers the cascade, and returns a typed InvalidDocumentTitleError carrying the reason. The handler keeps its pre-lock check, which is still worth having — it gives the common case a fast 400 without opening a transaction — and gains an arm that surfaces the store's refusal for the case its own check could not see. Grandfathering survives intact, because the store's check sits on the title-actually-changed branch, which is the same condition the handler uses. The test calls the store directly rather than reproducing the race: driving the interleaving would test the scheduler, while the property worth pinning is that the store refuses regardless of what a caller did. Its control leg is the grandfathering case — a content edit echoing the unchanged legacy title must still succeed — so validating everything here would fail it, which is round 10's regression restated as a guard. Mutation-verified: removing the store-side check leaves the handler tests green and fails this one with a nil error. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * perf(documents): stop counting every linker's occurrences twice (BUG-2798) Codex round 13, on what the guard costs the SUCCESS path rather than what it blocks on the failure path. The cascade counts occurrences because the size guard needs that number before it is willing to build anything. It then handed the same content to links.ReplaceTitle, whose strings.Replace with n < 0 counts it again. Every ordinary rename therefore paid a second full pass over every linking document for a number it already had. links.ReplaceTitleN takes the count the caller already computed. It is a separate function rather than an optional parameter because the obligation is real and silent when broken: passing a number that is too small does not error, it leaves later occurrences unrewritten, which on this path means links left pointing at a title that no longer exists. A name at the call site is cheaper than a comment nobody reads. NO measured speedup is claimed, and the doc comment says so. This removes one linear pass from a path that also allocates a full copy of the same content and issues a write per linker, so the saving is real but not obviously significant. It is here because doing the same work twice needs a reason and there was not one — not because a benchmark asked for it. The test asserts equivalence with ReplaceTitle across several shapes, including the new-title-embeds-old case, and its counterfactual leg asserts that an under-count visibly DIVERGES — if it did not, the caller's obligation would be imaginary and the API misleading. Round 13 also confirmed two things worth recording: no quadratic scan across linkers, and the ESCAPE clause does not materially change the query plan because the leading `%` already forced a content scan. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * docs(store): record the consequence the cascade cap necessarily has (BUG-2798) Codex round 14, on authorization and abuse. The refusal paths came back authorization-clean: reachable only after the workspace-access and `editor` checks, with viewers, guests, non-members and cross-workspace document IDs stopped first, and the reported byte figure exposing nothing an editor cannot already read. One consequence stands, and it is a property of having a cap at all rather than a defect in this one: once a workspace's documents linking a title exceed 32 MiB, that title can no longer be renamed, and any editor can put it in that state. Recorded in the constant's doc comment rather than fixed here, because the comparison that matters is with what it replaces. The same input previously took the server down for everyone; it now denies one operation to a role that can already delete every document in the workspace. Trading an unbounded OOM for a bounded, legible refusal is the point of the guard, not a gap in it. What IS missing is that the state has no exit but manual cleanup, with nothing telling an operator which documents to clean. That is a quota-and-recovery question rather than a cascade question, and it is filed as IDEA-2807 with three candidate shapes and an argument for the cheapest one — a state you can get out of is a different severity from one you cannot. The filing also says what is NOT established: no real workspace is known to approach the cap, and this fix's own receipt suggests none does, so it is a trap that exists rather than one anyone has fallen into. The adjacent concern the review raised — repeated near-cap renames contending for the rename lock and the connection pool — is noted there too, with the observation that the pre-existing behaviour was strictly worse, since each attempt was unbounded. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * revert(links): remove ReplaceTitleN — it optimised nothing (BUG-2798) Codex round 15 caught a claim of mine that was simply false. Reverting the functional half of c00606b0. I added ReplaceTitleN so the cascade could hand strings.Replace the occurrence count it had already computed, and wrote that this "removes one linear pass from a path that also allocates a full copy". It does not. strings.Replace calls Count UNCONDITIONALLY, before it looks at n: func Replace(s, old, new string, n int) string { if old == new || n == 0 { return s } // Compute number of replacements. if m := Count(s, old); m == 0 { return s } else if n < 0 || m < n { n = m } Read from this machine's GOROOT this turn, rather than recalled. Passing n constrains how many replacements are APPLIED; it does not skip the count. So the function bought nothing and cost something: a second way to do the same thing, carrying an obligation that fails SILENTLY when broken — an under-count leaves later occurrences unrewritten, which on this path means links pointing at a title that no longer exists. API surface with a silent failure mode and no payoff is worse than no API, so it goes rather than getting a corrected comment. The failure is the one my own trail keeps naming: I asserted a mechanism without reading it. What makes this instance worse than the earlier ones is that I wrote a careful hedge — "no measured speedup is claimed" — which reads as rigour while the sentence beside it stated the mechanism as fact. Declining to measure a claim is not the same as checking it, and the hedge made the unchecked claim look examined. The occurrence count stays where it is: the guard genuinely needs it before it will build anything, and computing it there is not redundant with anything the guard can avoid. Also declined this round, as already filed: renaming a legacy title with `|`, `\` or `]` leaves escaped links stale — that is BUG-2806, filed at round 10 with the mechanism verified in code. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN	2026-08-27 22:45:27 -04:00
xarmian	dd381e1066	chore: delete 5 unwired document handlers (TASK-769) (#252) ... * chore: delete 5 unwired document handlers (TASK-769) internal/server/handlers_documents.go had 5 dead HTTP handlers that were drafted as Documents-v1 extensions but never wired into the router (server.go:509 already labels Documents itself as "v1, will be replaced by items in Phase 2"): - handleQuickSave (POST /documents/quick-save) — title-based upsert - handleBulkRead (POST /documents/bulk-read) — multi-doc fetch by IDs - handleGetBacklinks (GET /documents/{id}/backlinks) - handleGetLinks (GET /documents/{id}/links) - handleGetContext (GET /documents/context?type=) Investigation confirmed zero consumers: - Not registered in setupRouter (`grep -n "QuickSave\|BulkRead\|Backlinks\|GetLinks\|GetContext" server.go` → empty). - Not used by the SvelteKit frontend (`web/src/`). - Not used by the CLI (`internal/cli/`). - Pre-launch repo, no fork or downstream that could be relying on them. Delete scope is intentionally limited to the HTTP handlers. The underlying `Store.QuickSave / BulkRead / GetBacklinks / GetLinks / GetContext` methods stay — they're tested at the store level (internal/store/store_test.go) and preserve optionality if Phase 2 work needs to revive any of these features. `models.QuickSave` stays for the same reason. After this lands, IDEA-732's lint catalog is fully cleared on main (staticcheck SA* + U1000 returns zero). TASK-771 (flip CI only-new-issues=false) becomes safe. Verified: - `go build ./...` clean - `go vet ./...` clean - `go test ./...` all pass - `staticcheck -checks "SA*,U1000" ./...` clean - All `import "strings"` etc. still used elsewhere in file Parent: PLAN-644. * chore: also delete now-test-only document store helpers (TASK-769) Codex round 1 on PR #252 flagged that the document-store helpers retained for "Phase 2 optionality" are now exclusively kept alive by their own store tests — Store.QuickSave, BulkRead, GetBacklinks, GetLinks, GetContext are not called by any production code path after the handler deletions in the previous commit. Same for the models.QuickSave struct. Pre-launch with no external consumers, optionality preservation has a real cost (dead code on main, ongoing test maintenance). When Phase 2 needs any of these capabilities it is cheaper to re-derive them against the Items model than to drag dead Documents-v1 plumbing forward. So delete them now. Removed: - internal/store/documents.go: QuickSave (38 lines), BulkRead (28), GetBacklinks (15), GetLinks (28), GetContext (41). - internal/models/document.go: QuickSave struct. - internal/store/store_test.go: TestQuickSave (38 lines), TestBulkRead (16), TestDocumentLinking (29), TestContext (23). Kept: - TestDocumentLinkRename — exercises UpdateDocument's internal link-rewriting path, not any of the deleted helpers. - GetDocumentByTitle — still used by TestDocumentLinkRename. - The full CRUD/restore handlers and their store methods — these are still wired into setupRouter and have their own coverage. Verified: - `go build ./...` clean - `go vet ./...` clean - `go test ./...` all pass (TestDocumentLinkRename and the wider doc CRUD/version/activity tests still cover the surviving paths). - `staticcheck -checks "SA*,U1000" ./...` clean - No new unused imports introduced (links package is still used by documents.go for ReplaceTitle in UpdateDocument). Parent: PLAN-644. * chore: drop GetDocumentByTitle and refactor TestDocumentLinkRename (TASK-769) Codex round 2 caught the chain — after deleting QuickSave/BulkRead/ GetBacklinks/GetLinks/GetContext, Store.GetDocumentByTitle was kept alive by exactly one test (TestDocumentLinkRename), which was re-fetching by title only because the test ignored the *Document already returned by createTestDoc. Use the createTestDoc return value instead, then drop GetDocumentByTitle from the store. Same idea, cleaner test, one fewer test-only API on the store. The rename behaviour (the actual thing under test) is unchanged. Verified: - `go build ./...` clean - `go test ./internal/store` and `./internal/server` pass - `staticcheck -checks "SA*,U1000" ./...` still clean Parent: PLAN-644. * chore: drop now-orphaned links.Extract (TASK-769) Codex round 3 caught the next link in the chain: after Store.GetLinks was deleted, links.Extract had no remaining callers — links.ReplaceTitle is the only Extract-package function still used (by UpdateDocument's rename rewrite). The linkPattern regex was only used by Extract. Drop linkPattern, the regexp import, and Extract itself. Leaves ReplaceTitle and its private string helpers (replaceAll, indexOf) intact. The cleanup chain ends here: ReplaceTitle is still wired into a live production path (Documents-v1 rename), and the supporting helpers have no other roles to inherit. Verified: - `go build ./...` clean - `go test ./internal/store` and `./internal/server` pass - `staticcheck -checks "SA*,U1000" ./...` clean Parent: PLAN-644.	2026-04-25 12:22:53 -04:00
xarmian	157ca4e88f	chore: bump Go toolchain to 1.26 (TASK-763) (#247) ... * chore: bump Go toolchain to 1.26 (TASK-763) Bump Go from 1.25 to 1.26 across all toolchain pins: - go.mod — go 1.25.0 → go 1.26.0 - Dockerfile — golang:1.25-alpine → golang:1.26-alpine - .github/workflows/ci.yml — three setup-go steps (Go, Go-Postgres, E2E jobs) - .github/workflows/release.yml — release pipeline No `toolchain` directive: the repo is pre-launch with no external contributors yet, so we set the floor where we want it (hard requirement). Verified locally before commit: - golangci-lint v2.11.4 builds and runs under Go 1.26.2 (pinned in CI) - golang:1.26-alpine and 1.26.2-alpine images present on Docker Hub - go build ./... clean - go vet ./... clean - go test ./... all pass Parent: PLAN-644 (OSS Repo Hygiene and Launch Polish). * chore: gofmt -w under Go 1.26 (TASK-763) Apply Go 1.26's gofmt to the codebase. ~41 files reformatted, all struct-tag whitespace realignment — no semantic changes. Verified: - gofmt -l ./cmd ./internal returns empty after - go build ./... still clean - go test ./... still passes (run before commit) Bundling the gofmt diff with the toolchain bump in the same PR because the formatting drift is a direct consequence of moving from 1.25 to 1.26; splitting them creates a mandatory two-PR ordering for no value. Parent: PLAN-644. * docs: bump documented Go floor to 1.26 (TASK-763) Match go.mod's hard 1.26.0 requirement in the source-build instructions. Caught by Codex review round 1 on PR #247. - README.md:158 — "Go 1.25+" → "Go 1.26+" - CONTRIBUTING.md:9 — "Go 1.25+" → "Go 1.26+"	2026-04-25 11:35:19 -04:00
xarmian	bde15d45ca	Rename Phases to Plans, clean up deprecated aliases (#71) ... * Rename "Phases" to "Plans" and clean up deprecated phase aliases Renames the default "Phases" collection to "Plans" across the full stack: - DB migration renames existing collections in-place (name, slug, prefix PLAN, icon 🗺️) - Removes all deprecated Phase* backward-compat aliases from models and store - Removes --phase CLI flag (use --parent instead) - Updates convention triggers: on-phase-start/complete → on-plan-start/complete - Updates dashboard API: active_phases → active_plans, /phases-progress → /plans-progress - Updates all frontend components, types, and documentation Closes IDEA-124 * Fix CSRF cookie not being cleared on logout The SessionAuth middleware was re-issuing a CSRF cookie before the logout handler could clear it, resulting in two Set-Cookie headers. Skip CSRF re-issue for /api/v1/auth/ paths since auth endpoints manage their own CSRF cookies (login sets, logout clears). * Fix migration issues found in Codex review - P1: Move doc_type UPDATE from migration 024 into 025, which recreates the table with the new CHECK constraint first (SQLite enforces CHECK on UPDATE, so the old constraint would reject 'plan') - P1: Add PostgreSQL migration 005 for the collection rename (phases → plans) — previously only existed on the SQLite path - P2: Recreate FTS triggers, indexes, and rebuild FTS after the table swap in migration 025 (DROP TABLE drops associated objects in SQLite) * Fix parent filter field name and sync .agents skill copy Codex review round 2 findings: - P1: Parent filter compared against `parent_id` (wrong) instead of `parent_link_id` — plan filtering in collection view was broken - P1: .agents/skills/pad/SKILL.md still had old --phase flags and "Phases" references — synced from the updated .claude copy - P2: Accept legacy 'phase' filter key for backward compat with existing saved views that serialized the old key name * Fix PG migration JSONB casting and add slug collision guards Codex PR review bot findings: - P1: PostgreSQL REPLACE/LIKE don't work on JSONB columns — cast schema::text and fields::text before string ops, then back to ::jsonb - P1: If a workspace already has a custom 'plans' collection, the rename hits UNIQUE(workspace_id, slug) — added NOT EXISTS guard to both SQLite and PostgreSQL migrations	2026-04-07 14:55:23 -04:00
xarmian	81579847c6	Initial release ... Pad — project management for developers and AI agents. Single Go binary with embedded SvelteKit web UI, SQLite storage, CLI, and Claude Code /pad skill integration. https://getpad.dev	2026-03-26 01:52:36 +00:00