390 Commits

Author	SHA1	Message	Date
xarmian	c67c167c43	feat(backlinks): title-form wiki-links + rename cascade (Phase 2a) (#621) ... * feat(backlinks): title-form wiki-links + rename cascade (Phase 2a) Phase 2a of PLAN-1593 (TASK-1595). Extends the server-side wiki-link reverse index from Phase 1's `[[REF-N]]` coverage to also handle `[[Title]]` and `[[collection/Title]]`. Cross-workspace `[[ws::REF]]` forms stay gated until TASK-1597 (Phase 2b) ships the request- independent ACL helper. What changed - internal/links/extract.go: lift the Phase-1 emit gate for WikiLinkKindTitle; keep WikiLinkKindWorkspaceRef gated. - internal/store/wiki_links.go: title branch in replaceWikiLinks (verbatim target_title storage + case-insensitive resolution), resolveTitleTx with full-key match first / `/`-split fallback (mirrors renderer order — Codex review caught the inverse on the planning round), cascadeTitleRename + resolveBrokenTitleLinks. - internal/store/items.go: rename cascade fires in-tx on title change; create path flips pre-existing broken title rows. - internal/store/migrations/062 + pgmigrations/041: one-shot `DELETE FROM item_wiki_links` so the startup backfill repopulates with the Phase 2a vocabulary. - Tests cover title round-trip, case-insensitive resolution, broken- link persistence + later resolution, full rename cascade with content rewrite, collection-qualified form, full-key-beats-split precedence (regresses Codex finding #3), broken-row flip on rename, no-op rename, and self-reference filtering. PLAN-1593 / TASK-1595. * fix(backlinks): rename cascade preserves display aliases (Codex round 1) Codex round 1 against PR #621 caught: the title-rename cascade selects sources via target_item_id (correctly hitting all rows that resolve to the renamed item — including aliased and mixed-case shapes), but the literal `strings.ReplaceAll` rewrite step only matched `[[Old Title]]` / `[[<slug>/Old Title]]`. Rows from `[[Old Title|alias]]`, `[[old title]]` (mixed case), or `[[<slug>/Old Title|alias]]` slipped past the rewrite; the trailing replaceWikiLinks re-parse then saw the same body, failed to resolve under the new title, and converted the row to broken — exactly the regression the cascade exists to prevent. Fix: new internal/links/RewriteWikiTitle helper handles all four title-form shapes with case-insensitive title matching and verbatim display-alias preservation. Cascade swaps from ReplaceAll to this helper. Unit tests in links_test.go cover the matrix; integration test in wiki_links_test.go regresses the original failure mode by mixing all four shapes in one source and asserting all four stay resolved after rename. Known limitation documented in the helper: titles containing wiki- link escape characters (`]`, `|`, `\`) — stored escaped in source content — don't match the regex's literal old-title segment. Same limitation exists in the legacy ReplaceTitle helper; promotable if a real user hits it. PLAN-1593 / TASK-1595. * fix(backlinks): retarget qualified-fallback rows on literal-title arrival (Codex round 2) Codex round 2 against PR #621 (P2): resolveBrokenTitleLinks only flipped target_item_id IS NULL rows, missing the arrival-order case where a literal `[[tasks/Setup]]` should win stage 1 over a row previously resolved via stage 2 (qualified fallback to item "Setup" in collection "tasks"). The index would stay stale until the source's content was rewritten — a latent inconsistency between the persisted backlink and what the renderer would actually show. Fix: drop the IS NULL constraint on the stage-1 UPDATE. Stage 1 ALWAYS wins per the renderer's order at markdown.ts:541, so any row with a matching literal title flips to the new item — including rows currently pointing at a qualified-fallback target. Added a target_item_id != ? guard so we don't churn rows that already point at us. Stage 2 keeps the NULL constraint so already-resolved qualified rows don't churn when another fallback candidate appears. Regression test in wiki_links_test.go reproduces the exact scenario Codex described: fallback resolves first, literal arrival steals the row from the fallback target. PLAN-1593 / TASK-1595. * fix(backlinks): renderer parity for [[A|B]] + scope arrival retarget (Codex round 3) Two findings from Codex round 3 against PR #621: P1 — parser/renderer parity for [[A|B]] matching literal title "A|B". The renderer (web/src/lib/utils/markdown.ts:516-525) tries the FULL body as a title FIRST when a pipe is present, only falling through to the split interpretation on miss. Our parseBody always split on the first unescaped pipe, so an item literally titled "A|B" was indexed as title="A" with display="B" — the index would miss backlinks the UI shows, or point them at a different "A" item. Fix: in replaceWikiLinks for title kind, when HasDisplay, try the full body (Title+"|"+Display) as a title FIRST via resolveTitleTx; on hit, store target_title=fullBody and drop the display override (the display segment was actually part of the title). On miss, fall back to the existing split-key interpretation. The cascade's RewriteWikiTitle regex correctly handles both storage shapes via QuoteMeta on the title. P2 — stage-1 UPDATE was too aggressive after the round-2 fix. The broad UPDATE (no IS NULL constraint) silently stole backlinks from legitimately-resolved rows when a SECOND item was created/renamed to the same title. Titles aren't unique, the renderer's Array.find() is order-dependent, and silent churn is worse than no-op stability. Fix: split resolveBrokenTitleLinks into three updates: (1) Plain literal flip — NULL only. (2) Qualified literal flip — NULL only. (3) Literal-arrival retarget — gated on title containing `/`. Only fires for `[[<slug>/Title]]` rows, which are the only ones that COULD have been stage-2 qualified-fallback resolved. Rows with target_title='Foo' (no slash) can only have been stage-1 literal — we don't steal those. Regression tests: - TestWikiLinks_LiteralPipeInTitleResolves — `[[A|B]]` resolves to item literally titled "A|B". - TestWikiLinks_LiteralPipeInTitleFallsThroughToSplit — `[[A|B]]` falls back to item "A" when no "A|B" item exists. - TestWikiLinks_SecondItemSameTitleDoesNotStealBacklinks — adding a second "Foo" item doesn't redirect the existing backlink. PLAN-1593 / TASK-1595. * fix(backlinks): broken pipe-in-body rows keyed on full body (Codex round 4) Codex round 4 against PR #621: when a source body `[[A|B]]` is written before any matching item exists, the broken row was stored with target_title="A" (the split key). If an item literally titled "A|B" was later created, resolveBrokenTitleLinks looking for target_title="A|B" couldn't find the row — index went stale while the renderer's preferred full-body interpretation would correctly resolve the link. Fix: when nothing resolves AND a pipe was present (HasDisplay), key the broken row on the FULL body (Title+"|"+Display) instead of the split key. resolveBrokenTitleLinks then naturally finds it via the literal-arrival path. The remaining asymmetry — a broken row keyed on full body won't pick up a future split-fallback resolution to a new item titled "A" — is documented in the code as a v3-promotable limitation. The full-body path is the renderer's PREFERRED interpretation (markdown.ts:516), so prioritizing it is the right tradeoff in the rare case both interpretations could apply. Regression test in wiki_links_test.go reproduces the scenario: source written first with `[[A|B]]`, item titled "A|B" created later, backlink should resolve. PLAN-1593 / TASK-1595. * fix(backlinks): scope stage-3 retarget + cascade self-refs (Codex round 5) Two findings from Codex round 5 against PR #621: Finding 1 — stage-3 literal-arrival retarget could still steal backlinks from a legitimate stage-1 literal-match row when a SECOND item with the same slash-containing title is created. The previous fix (round 3) gated stage-3 on title containing `/`, which let through the qualified-fallback retarget case correctly but didn't distinguish stage-1-resolved rows pointing at a literal twin from stage-2-resolved rows pointing at the fallback target. Fix: add an EXISTS check that scopes the flip to rows whose CURRENT target has a title NOT matching ours. Stage-1 (literal) resolutions point at items literally titled the same as the row's target_title; stage-2 (qualified-fallback) resolutions point at items titled just the trailing segment. The EXISTS clause picks out only the latter. Finding 2 — self-references on the renamed item went stale on title-only renames. The cascade's `s.id != renamedItemID` filter excluded self, but items.go only re-indexes content when input.Content != nil. So a title-only rename of an item whose body mentions itself by its old title kept the body's now-broken `[[Old Title]]` literal in place while the index still recorded a "working" backlink — drift between renderer state and index state. Fix: drop the self-exclusion from the cascade SELECT. RewriteWikiTitle rewrites the renamed item's own content along with everyone else's; GetBacklinks still hides self-links at query time, so the backlinks panel behavior is unchanged. Tests: - TestWikiLinks_DuplicateSlashTitleNoTheft regresses Finding 1. - TestWikiLinks_TitleRenameRewritesSelfReferences asserts Finding 2's new correct behavior (replaces the prior test that asserted the old buggy behavior). PLAN-1593 / TASK-1595. * fix(backlinks): ref→title fallback + reorder cascade self-ref (Codex round 6) Two findings from Codex round 6 against PR #621: Finding 1 — ref-shaped title fallback missing. parseBody returns WikiLinkKindRef for `[[ISO-9001]]` (matches refPattern), but if no ISO-9001 ref-item exists, the renderer falls through to legacy title lookup (markdown.ts:513) and resolves to an item literally titled "ISO-9001". The store inserted only a broken ref-kind row with target_title=NULL, so GetBacklinks never surfaced the backlink even when the renderer rendered it. Fix: in replaceWikiLinks' WikiLinkKindRef branch, when resolveRefTx misses, try resolveTitleTx on the same body. If title resolves, INSERT as title-kind row with target_title=ref-shaped-body. The rename cascade catches these correctly via target_kind='title' + target_item_id. The asymmetry — a future ref-item creation can't auto-retarget these title-stored rows — is documented as a v3 limitation. Finding 2 — combined title+content update broke self-ref cascade. The original order (main UPDATE → replaceWikiLinks(self) → cascade) wiped self's `target_item_id=renamedItemID` row before cascade ran: when input.Content contains `[[Old Title]]`, re-indexing self resolved it as broken (target_item_id=NULL), so cascade's SELECT missed self for the title+content path. Fix: reorder so cascade runs BEFORE the self re-index — the pre-existing wl rows are still intact at cascade time. The final re-index re-reads items.content from the DB (since cascade may have rewritten it in-band) rather than using *input.Content directly; otherwise the re-index would undo the cascade's self-ref rewrite. Tests: - TestWikiLinks_RefShapedFallsThroughToTitle — `[[ISO-9001]]` resolves to an item titled "ISO-9001". - TestWikiLinks_TitleAndContentRenameCascadesSelfRef — combined title+content rename with self-ref in new content gets the self-ref rewritten by cascade. PLAN-1593 / TASK-1595. * fix(backlinks): ref+pipe→title parity, position-based cascade, scoped self-rewrite (Codex round 7) Three intertwined fixes addressing Codex round 7 findings against PR #621: Finding 1 — ref→title fallback missed pipe-bodies. For `[[ISO-9001|Spec]]`, renderer tries full-body title "ISO-9001|Spec" BEFORE falling to bare "ISO-9001" (markdown.ts:516). Our ref-fallback only tried bare. Extended ref-branch's fallback to try full body first when HasDisplay, then bare — same order as the title-branch's stage (a)/(b) pattern. Finding 2 — cascade corrupted UNRELATED literal-pipe titles. Items A "Old Title" and B "Old Title|alias" both referenced from one source; renaming A previously triggered RewriteWikiTitle's regex `(?i:Old Title)((?:\|...)?)` which matched BOTH A's `[[Old Title]]` AND B's `[[Old Title|alias]]` — corrupting the B link. Refactored cascadeTitleRename to be POSITION-BASED: SELECT each individual wl row with its position + target_title (no longer DISTINCT sources). Per-row, rewrite the bracket AT THAT EXACT POSITION via new links.RewriteBracketAt helper. Process rows in descending position order per source so earlier offsets don't shift. Brackets whose wl row doesn't resolve to the renamed item are never visited. Scoped self-rewrite — title-only renames cascade self (input.Content == nil); combined title+content renames EXCLUDE self (input.Content != nil). User-supplied content is authoritative; auto-rewriting their just-submitted brackets would surprise them. Mirrors documents.go::updateLinksInTx, which also leaves the renamed entity's own content alone. Codex round 6 finding 2 is fully addressed: title-only path still rewrites self-refs, combined path respects user submission and the index correctly records the bracket as broken (matching what the renderer would render). New links.RewriteBracketAt helper with full unit-test coverage: plain/aliased/qualified/qualified+aliased shapes, case-insensitive matching, slug-prefix preservation, full-body vs split-key target disambiguation, out-of-bounds defensive guards. Integration test TestWikiLinks_CascadeDoesNotCorruptLiteralPipeNeighbor reproduces Codex round 7 finding 2's scenario. PLAN-1593 / TASK-1595. * fix(backlinks): retarget rows pointing at soft-deleted targets (Codex round 8) Codex round 8 P2: resolveBrokenTitleLinks only considered target_item_id IS NULL rows as eligible for flip. A row that resolved to item A and then had A soft-deleted stayed pointing at deleted A; creating a new item B titled the same as A wouldn't flip the row, so GetBacklinks(B) missed the backlink the renderer would actually show (renderer hides deleted-target links). Fix: introduce a "broken-in-practice" predicate (target_item_id IS NULL OR NOT EXISTS ( SELECT 1 FROM items t WHERE t.id = item_wiki_links.target_item_id AND t.deleted_at IS NULL )) applied to stages 1 (plain literal flip) and 2 (qualified literal flip). Stage 3 (slash-title literal-arrival retarget) already considered "current target deleted" implicitly via its title- mismatch EXISTS check. Regression test in wiki_links_test.go covers the exact scenario: A "Foo" resolves a backlink → A soft-deleted → B "Foo" created → backlink flips to B. Known limitation deferred to v3: the symmetric case (delete A while B with same title already exists) doesn't fire any hook that re-resolves the row. A dedicated DeleteItem hook would close that gap; not blocking Phase 2a. PLAN-1593 / TASK-1595. * fix(backlinks): preserve title whitespace to match renderer (Codex round 9) Codex round 9 P2: parseBody trimmed whitespace from the body BEFORE deciding it was a title kind. The renderer doesn't trim before title matching (markdown.ts:541-543) — `[[ Foo ]]` is matched against items.title with the surrounding spaces intact, so an item titled "Foo" wouldn't match. Trimming server-side created index entries the UI couldn't click — backlinks showed in the panel for links that the renderer rendered as broken. Fix: keep an UNTRIMMED unescaped body for title-kind fallthrough, and a trimmed copy only for ref / workspace_ref SHAPE detection (refs are whitespace-free by construction, the renderer's key.trim() at L503 is just typing forgiveness for the ref form). Tests in extract_test.go: - `[[ Foo ]]` emits title-kind with Title=" Foo " (whitespace preserved). - `[[ TASK-5 ]]` still parses as ref (shape detection trims). - `[[Project Goals]]` (two spaces inside) preserves internal whitespace. PLAN-1593 / TASK-1595. * fix(backlinks): ref→title fallback uses raw untrimmed key (Codex round 10) Codex round 10 P2: after round 9's title-kind whitespace fix, the ref→title FALLBACK path still used canonical-trimmed link.Ref for its title lookup. The renderer's fallback at markdown.ts:541-543 uses the UNTRIMMED key (no .trim() on the title-lookup path), so `[[ TASK-5 ]]` falling through to title would search " TASK-5 " (with whitespace) — an item literally titled " TASK-5 " resolves in the UI but not in our index. Fix: add a RawKey field to WikiLinkRef capturing the untrimmed unescaped key for ref kinds. parseBody populates it alongside the canonical Ref. replaceWikiLinks' ref→title fallback path uses RawKey (with defensive fallback to Ref for old rows) when constructing title candidates. Mirrors the renderer's untrimmed title lookup across both bare and pipe forms. Regression test in wiki_links_test.go covers the exact scenario: item titled " TASK-5 " (with whitespace), source body `[[ TASK-5 ]]`, backlink resolves via the untrimmed ref→title fallback. PLAN-1593 / TASK-1595.	2026-05-24 12:01:05 -04:00
xarmian	8e7d4040fd	feat(backlinks): server-side reverse index for [[...]] (Phase 1) (#620) ... * feat(backlinks): server-side reverse index for [[...]] wiki-links (Phase 1) First phase of PLAN-1593. Today [[REF]] is parsed only at render time on the client and there's no way to ask "who links to TASK-5?" without a full-text scan. This change adds a materialized reverse index (item_wiki_links) that's written every time an item's content changes and exposes it via REST + CLI. Phase 1 covers ref-form links only (`[[TASK-5]]` / `[[TASK-5|Display]]`). Phase 2 (TASK-1595) will extend to titles + cross-workspace; Phase 3 (TASK-1596) adds the web UI panel + MCP action. What lands here: * Migrations 061 (SQLite) and 040 (Postgres) create item_wiki_links with partial indexes on target_item_id, (target_workspace_id, target_ref), and target_title — the schema accommodates all 5 wiki-link forms up-front so Phase 2 doesn't ALTER. * internal/links/extract.go is the canonical parser. It strips fenced and inline code regions before extracting [[...]] occurrences, so example refs in docs / code blocks don't pollute the index. Phase 1 emits only WikiLinkKindRef rows; title and workspace_ref kinds parse successfully but are gated out until Phase 2. * internal/store/wiki_links.go (replaceWikiLinks + GetBacklinks + helpers) handles write-time bookkeeping and the read query. Resolution to target_item_id happens at parse time inside the same transaction as the items INSERT/UPDATE, so partial state never lands. Broken refs (target_item_id IS NULL) intentionally persist — they feed a future broken-links report. * internal/store/wiki_links_backfill.go + cmd/pad/main.go hook the idempotent backfill into server startup. Existing items get indexed on first boot after the migration; subsequent boots are near-no-ops via an EXISTS short-circuit. * internal/store/items.go is amended in two places: tryCreateItem always calls replaceWikiLinks (empty content → no-op DELETE), and UpdateItemWithPreCheck re-parses whenever input.Content was supplied. * internal/server/handlers_backlinks.go serves `GET /api/v1/workspaces/{ws}/items/{itemSlug}/backlinks` with visibility + guest-grant filtering on the source items. * internal/cli/client.go adds GetBacklinks; cmd/pad/main.go adds the `pad item backlinks <ref>` command (registered in groups.go). Behavior decisions (per PLAN-1593): - code blocks excluded (fenced + inline) - self-links filtered at query time (kept in storage) - repeated mentions stored as separate rows by position - ordering: source updated_at DESC, position ASC Tests: - internal/links/extract_test.go: 26 sub-cases covering ref/title/ workspace-ref discrimination, code-block exclusion (fenced + inline + unclosed fence), position-is-byte-offset (UTF-8 safety), and edge inputs. - internal/store/wiki_links_test.go: 8 integration tests covering the create/update/delete/self-link/broken-ref/repeated/code-block scenarios plus backfill idempotence. All pass. `make check` clean (lint + go test + web build). Refs: TASK-1594, PLAN-1593, IDEA-1577 * fix(backlinks): visibility-aware pagination + case-insensitive refs per Codex review (round 1) Two fixes from Codex code review: P1 — GetBacklinks now takes a visibleCollectionIDs []string argument that's applied INSIDE the SQL WHERE clause. Previously the handler fetched LIMIT raw rows and filtered visible ones in Go, so a restricted user asking for limit=50 could receive an empty page even when later visible backlinks existed. Pushing visibility into SQL makes LIMIT/OFFSET count visible rows. nil → no restriction (owners, editors, root tokens) [] → see nothing (returns early, no SQL) [..] → AND s.collection_id IN (?, ?, ...) Item-level guest grants still apply post-fetch — they're rare enough that the residual page shrink is acceptable and pushing them into SQL would balloon the query. P2 — refPattern now accepts mixed/lowercase refs and parseBody canonicalizes the prefix to uppercase at the single chokepoint. Previously the renderer accepted `[[task-5]]` as a real link (its REF_PATTERN is case-insensitive) but the indexer's ^[A-Z]... pattern silently dropped it — divergent parsing on the same input. Storage shape is canonical uppercase so the (workspace, prefix, number) lookup against collections.prefix (also uppercase) has one shape. New helper: canonicalizeRef("task-5") → "TASK-5". Regressions: internal/links/extract_test.go + TestCanonicalizeRef — helper unit tests + TestExtractWikiLinks_RefVsTitleFallback updated to assert mixed/lowercase parses-as-ref-and-uppercases + edge-case test renamed from "lowercase ref" to "number-led not a ref" (lowercase IS a ref now per Codex P2) internal/store/wiki_links_test.go + TestWikiLinks_MixedCaseRefIndexed — `[[task-5]]` produces a backlink row whose target_ref is "TASK-5" + TestWikiLinks_VisibilityAwarePagination — three sub-cases: nil → all 3, visible-only limit=2 → 2 visible rows (not 1 with hidden one consuming a slot), empty → 0 All call sites updated (8 in tests + 1 in handler). `make check` clean (lint + tests + web build). Refs: TASK-1594, PLAN-1593 * fix(backlinks): SQL-level item-grant filter per Codex review (round 2) Round 1 fixed pagination for collection-level visibility but Codex round 2 correctly flagged the same class of bug at the item-grant layer: `visibleCollectionIDs` returns the UNION (full grants ∪ collections containing granted items), and the handler then filtered each row's item-level visibility in Go AFTER fetching — letting hidden rows in a granted-item's collection consume LIMIT slots. The refactor moves the precise predicate into SQL. New shape: type BacklinksVisibility struct { Unrestricted bool // admin / full-access member FullCollectionIDs []string // direct collection grants GrantedItemIDs []string // item-level grants } // SQL predicate when Unrestricted=false: // AND (s.collection_id IN (?...) OR s.id IN (?...)) This matches `guestResourceFilter` (which returns the precise primitives), so the handler now passes them straight through and drops the post-fetch filter loop entirely. Pagination is correct for guests, restricted members, and unrestricted users alike. New test: TestWikiLinks_ItemGrantPagination — guest with item-grant on ONE item in an otherwise-hidden collection sees exactly that one item; hidden siblings in the same collection do NOT leak in, and limit=2 returns 1 row (not silently shrunken). Other call sites updated: - TestWikiLinks_VisibilityAwarePagination → uses BacklinksVisibility{FullCollectionIDs: ...} and BacklinksVisibility{} for the no-access case. - 8 existing tests → BacklinksVisibility{Unrestricted: true}. - handlers_backlinks.go → no longer calls visibleCollectionIDs; uses guestResourceFilter exclusively and skips the Go-side filter. Verification: - make check clean - All TestWikiLinks_* pass Refs: TASK-1594, PLAN-1593 * fix(backlinks): scan EXISTS into bool not int for Postgres parity (Codex round 3) `SELECT EXISTS(...)` returns boolean on Postgres but integer 0/1 on SQLite. Scanning into `int` happened to work on SQLite (the modernc.org driver coerces) but would fail on Postgres — silently disabling the backfill short-circuit there and meaning upgraded Postgres installs wouldn't populate backlinks for pre-existing content until each item got edited. Fix: scan into bool. Both database/sql drivers in use (modernc.org/ sqlite and lib/pq) coerce their native representation into Go's bool, so this single shape works on both engines. make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): allow CommonMark 0-3 space indented fences per Codex (round 5) Round 5 flagged two edge cases in the code-stripping pass: 1. Multi-backtick inline code (``see [[X]]``) — traced through the parser; my permissive close-on-next-backtick logic already covers it correctly (range = [opener-start, after-closer-run]). Added a regression test to lock this in: TestExtractWikiLinks_CodeBlocksExcluded / "multi-backtick inline code excludes ref" 2. Indented fenced blocks — CommonMark allows 0-3 leading spaces of indentation before a fence opener (4+ spaces makes it an indented code block, a different construct). My fencedCodeRanges only matched fences at column 0, so ` ```\n[[X]]\n```` ` would render as code in the UI but leak a false backlink. Fixed both fencedCodeRanges (opener) and findFenceCloser (closer) to skip up to 3 leading spaces, with a hard cap at 4 (which would be indented-code, not a fence). Regression test: TestExtractWikiLinks_CodeBlocksExcluded / "indented fenced block (CommonMark 0-3 spaces)" Not addressed: - Round-4 escape-body parity finding. extract.go mirrors renderMarkdown's regex (web/src/lib/utils/markdown.ts:300), which is the actual render-time link parser; wikiLinksToMarkdown's more permissive escape grammar is editor-serializer-side and the renderer can't even consume its escaped output. Indexing what the user actually sees as a link is the correct invariant. make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): tilde fences + strict closer lines per Codex (round 6) Two CommonMark conformance gaps in the code-block stripping pass: 1. Tilde-fenced code blocks (~~~) were ignored. marked() treats them the same as backtick fences, so a [[REF]] inside a tilde block would render as code in the UI but leak as a false backlink. Fixed by parameterizing fenceChar across fencedCodeRanges and findFenceCloser, with separate handling for the backtick-specific "no backtick in info string" rule (CommonMark §4.5). 2. Closer-line strictness — CommonMark requires the closing fence line to contain only the fence + optional trailing spaces. The previous accept-any-fence-prefixed-line check would terminate a still-open fence prematurely on a line like ```not-closed, leaking later refs in the still-rendered code block. Refs reside in 4 new sub-tests under TestExtractWikiLinks_CodeBlocksExcluded: - tilde fence excludes refs inside - tilde fence with language tag - mixed fence types don't pair - closer-line strictness — backticks plus other text is not a closer - closer-line strictness — trailing spaces OK make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): inline code closer must match opener length per Codex (round 7) CommonMark §6.1 requires an inline-code span opened with N backticks to close on a run of EXACTLY N backticks. The previous "close on next backtick run of any length" logic would prematurely end the excluded range on a stray single backtick inside a ``...`` span, leaking any [[REF]] in the latter half of the code text as a false backlink. Concrete failure case: ``has ` inside [[X-1]] and more`` → old: range [0, 7], [[X-1]] indexed (bug) → new: range [0, end-of-closer], [[X-1]] excluded (correct) Fix: track the opener-run length and scan only for matching-length closer runs. Wrong-length runs in between are code text. Two new regression sub-cases under TestExtractWikiLinks_CodeBlocksExcluded: - inline code closer matches opener length — the main case - single-backtick span unaffected by adjacent multi-backtick run — asserts the opposite direction (opener=1 doesn't close on ``) Not addressed: - Re-flagged round-4/round-7 escape-body parity finding. extract.go intentionally mirrors renderMarkdown's regex (markdown.ts:300), not wikiLinksToMarkdown's more permissive escape grammar (markdown.ts:461). renderMarkdown is the actual link parser at display time; its regex rejects escaped-`]` bodies, so any link with an escaped `]` in its body is NOT shown as a clickable link in the UI. Indexing it would produce phantom backlinks the user can't see. The wikiLinksToMarkdown permissive grammar is paranoid serialization that the renderer can't consume — that's a pre-existing inconsistency in the editor pipeline, not a backlinks bug. make check clean (lint + tests + web build). Refs: TASK-1594, PLAN-1593 * fix(backlinks): rune-align snippet end-edge to keep UTF-8 valid (Codex round 8) The previous snippetAround() trimmed `start` to a rune boundary (so the leading edge of the snippet was always at a valid codepoint) but left `end` as a raw +40-byte clamp. When that landed in the middle of a multi-byte rune — common around emoji or accented text — the resulting slice was invalid UTF-8 and the JSON encoder would emit replacement characters in backlink snippets. Fix: same forward-advance pattern at the end as at the start. Continuation bytes (10xxxxxx) get skipped until we land on a leading byte. Going forward keeps the snippet anchored slightly past the match rather than slightly before it, which is a small UX win (emoji or accented text right after the link survives intact). Regression test: TestWikiLinks_SnippetIsValidUTF8 — pads body with enough 4-byte emoji on each side that the ±40-byte window cuts through one; asserts utf8.ValidString on the resulting snippet. make check clean (lint + tests + web build). Refs: TASK-1594, PLAN-1593 * fix(backlinks): inline code spans cross newlines, break on blank lines (Codex round 9) CommonMark §6.1: an inline-code span can cross single newlines but terminates at a blank line (a line containing no chars or only whitespace, which ends the enclosing paragraph). My previous scanner broke at every newline, so multi-line spans like `pre [[INSIDE-1]] post` would treat the opener as unclosed and leak [[INSIDE-1]] as a false backlink. Fixed by: 1. The newline branch in the closer scan now peeks ahead via the new isBlankLineAt() helper. Same-paragraph newlines are traversed; blank-line breaks terminate the span unmatched. 2. isBlankLineAt() treats any line with only space/tab as blank (mirroring CommonMark's blank-line definition). Three new regression sub-cases under TestExtractWikiLinks_CodeBlocksExcluded: - inline code spans single newline (CommonMark §6.1) - inline code breaks at blank line (paragraph boundary) - inline code breaks at whitespace-only blank line Trade-off: a truly-unclosed inline backtick now consumes from the opener up to the next blank line instead of just the rest of the line. False-positive on wiki-links in that span, but the surface area is small (unclosed backticks are rare in published prose) and matches the renderer's behavior. make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): accept escaped wiki-link bodies per editor grammar (Codex round 10) After 3 rounds of disagreement, capitulating on the escape-body parity finding. My position was technically correct for the CURRENT renderMarkdown behavior (which uses [^\]]+ and can't parse escaped- bracket bodies), but the editor's wikiLinksToMarkdown grammar at markdown.ts:461 explicitly produces such bodies — making the renderer's regex the inconsistent half of the pipeline, not mine. Mirroring the editor's grammar in the extractor makes the index forward-compatible: when the renderer eventually gets fixed, no change here is needed. The cost is a few "phantom" rows in the interim (indexed links the renderer doesn't currently display as clickable), but those are harmless and aligned with author intent. Changes: - wikiLinkPattern now uses `\[\[((?:\\.|[^\]\\])+)\]\]` — mirrors markdown.ts:461 verbatim. - New splitOnUnescapedPipe() helper — scans for the first `|` that isn't preceded by `\`. Mirrors splitWikiBody at markdown.ts:664. - New unescapeWikiBody() helper — undoes `\]`, `\|`, `\\` escapes in display text and key. Mirrors unescapeWikiBody at markdown.ts:657. - parseBody() now uses both helpers — split on unescaped `|`, unescape both sides. Regression coverage: - TestExtractWikiLinks_EscapedBodyChars (5 sub-cases): escaped `]`, escaped `|`, escaped `\`, non-escape backslash passes through, Position still points at opening `[[` despite escapes. - TestSplitOnUnescapedPipe + TestUnescapeWikiBody: direct unit tests for the helpers (round-trip safety vs the editor's escape/unescape pair). make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): preserve display text verbatim per Codex round 11 P3 The previous parseBody trimmed the display side of [[X|Display]] but the WikiLinkRef.Display contract promises verbatim storage and the renderer at markdown.ts doesn't trim either. Trimming would silently diverge on padded display text like [[TASK-1| spaces ]] (renderer keeps the spaces, extractor stripped them). Fix: drop TrimSpace from the suffix half of the split. Keep trimming the key/ref side because refPattern is anchored — a leading or trailing space in the key would force the body to fall through to the title kind even though the renderer resolves it as a ref. Regression test: TestExtractWikiLinks_EscapedBodyChars / "display text preserved verbatim (no TrimSpace)" make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): distinguish empty display override from no-pipe per Codex round 12 [[REF|]] (explicit empty display) and [[REF]] (no display) are distinct shapes in the editor: splitWikiBody returns displayOverride="" for the former, null for the latter, and the renderer uses `displayOverride ?? title` (nullish coalescing, NOT empty-string fallback) so "" is preserved. The previous extractor collapsed both into display_text=NULL, violating verbatim-display preservation for the empty-string edge case. Fix: - WikiLinkRef gains a HasDisplay bool. parseBody sets HasDisplay=true iff splitOnUnescapedPipe found a pipe; downstream uses HasDisplay (not Display!="") to decide whether to persist the override. - replaceWikiLinks in store: NullString.Valid is keyed off HasDisplay. display_text='' for explicit empty, NULL for no override. Regression coverage: - internal/links/extract_test.go: "explicit empty display override is distinguished from no pipe" - internal/store/wiki_links_test.go: TestWikiLinks_EmptyDisplayDistinct (two-source assert: NOT NULL for [[REF|]], NULL for [[REF]]) make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): pointer-typed DisplayText to preserve empty distinction over JSON (Codex round 13) Round 12 added HasDisplay on the parser side and made the store preserve display_text='' vs NULL on the DB row, but the wire model collapsed the distinction at JSON-serialization time: DisplayText string `json:"display_text,omitempty"` `omitempty` drops empty strings, so [[REF|]] (empty override) and [[REF]] (no override) serialized identically on the API and CLI JSON output. The end-to-end goal of round 12 wasn't reached. Fix: change DisplayText to *string. nil → no override (field omitted from JSON via omitempty), pointer to "" → explicit empty override (field present with empty value). The store's NullString.Valid drives the assignment, so the SQL round-trip matches the JSON shape. Knock-on: the CLI's `pad item backlinks` now dereferences the pointer and prints both populated and empty overrides ("displayed as: "). Regression coverage: - TestWikiLinks_EmptyDisplayDistinct extended to assert withBL.DisplayText is non-nil-pointing-at-"" and noBL.DisplayText is nil after a GetBacklinks round-trip. make check clean. Refs: TASK-1594, PLAN-1593	2026-05-23 23:09:44 -04:00
xarmian	8cf460b381	fix(ci): unbreak Go (PostgreSQL) test + clear x/net govulncheck findings (#618) ... * fix(test): encode bools as bools in TestGetUserWorkspacesDetailed (BUG-1582) The raw INSERT in this test passed integer literals `0, 0, 1` for `sort_order, is_default, is_system`. SQLite coerces int → bool but pgx refuses, so the Go (PostgreSQL) CI job has failed every run since #600 landed: workspace_members_admin_detail_test.go:42: seed system collection: failed to encode args[10]: unable to encode 0 into binary format for bool (OID 16): cannot find encode plan Pass `false, true` for the two bool columns so both drivers accept the args. * chore(deps): bump golang.org/x/net to v0.55.0 (TASK-1583) Clears 5 govulncheck findings (GO-2026-5025..5030) reachable via internal/urlimport/generic.go's call to html.Parse. The Go CI job has been failing on every main run since these advisories were published. Vulnerability #1: GO-2026-5030 — XSS via duplicate attributes Vulnerability #2: GO-2026-5029 — character refs in DOCTYPE Vulnerability #3: GO-2026-5028 — DoS parsing arbitrary HTML Vulnerability #4: GO-2026-5027 — HTML elements in foreign content Vulnerability #5: GO-2026-5025 — namespaced elements in foreign content `go mod tidy` pulls along the usual x/* sibling bumps. Local govulncheck after the bump: *No vulnerabilities found.* Full `go test ./...` passes. * fix(store): is_system check uses NOT bool, not = 0 (BUG-1582) GetUserWorkspacesDetailed's collections_count subquery had `c.is_system = 0`. SQLite stores BOOLEAN as INTEGER so the comparison worked there, but Postgres' boolean column rejects the integer literal: ERROR: operator does not exist: boolean = integer STATEMENT: SELECT ... AND c.is_system = 0) The first push at this BUG only fixed the test-side encoder issue; this commit fixes the production query that the test exercises. `NOT c.is_system` evaluates correctly on both drivers without needing to thread another placeholder through the args. Verified locally against a real Postgres 17 instance: TestGetUser- WorkspacesDetailed and the full ./internal/store/... suite pass.	2026-05-23 11:36:16 -04:00
xarmian	66dce32c7e	chore(mcp): point library-activate not-found hint at pad_library (TASK-1564) (#616) ... The dispatcher's `library activate` not-found error referenced a `pad_project action=library-list` action that never existed — leftover from an earlier design draft for IDEA-1514 (the now-shipped `pad_library` tool, PLAN-1560 / TASK-1563). The correct hint is `pad_library action=list`. One-line hint fix; no behavior change. Closes the remaining scope of TASK-1564 — the onboard playbook body update shipped in PR #615 (commit 1638bdf) per Codex review. No CHANGELOG entry added: the project has no top-level CHANGELOG.md and tracks history via git + Pad items. Closes TASK-1564 → completes PLAN-1560 (IDEA-1514).	2026-05-21 20:14:58 -04:00
xarmian	6433cc51ea	feat(mcp): pad_library catalog tool + ToolSurfaceVersion 0.5 (TASK-1563) (#615) ... * feat(mcp): pad_library catalog tool + ToolSurfaceVersion 0.5 (TASK-1563) MCP catalog wiring for PLAN-1560 (`pad_library` MCP tool + matching CLI surface). Closes IDEA-1514 — pure-MCP agents (notably the /pad onboard playbook from PLAN-1496) can now browse and activate library entries without shelling out. ## New tool `pad_library` joins the v0.5 catalog as the ninth resource × action tool. Three actions, all passThrough to the `pad library` CLI: - `list` — Browse conventions + playbooks. Defaults to summary mode for playbooks (compact bodies via the ?summary=true endpoint flag); conventions always carry full content. Optional type / category / full inputs. - `get` — Full body of one entry by exact title. Conventions-first precedence mirrors `activate`. - `activate` — Create a workspace item from a library entry by title. `Workspace: true` on the tool — list/get ignore it; activate validates and uses it. The schema-level declaration gives activate automatic pad_set_workspace session-default resolution (same precedent as pad_meta's mixed-workspace actions). ## Dispatcher extensions - `dispatchLibraryList` forwards `category` to BOTH endpoints and passes `summary=true` to the playbook endpoint by default (unless input.full=true). MCP-default summary mode keeps agent context budgets tight; CLI default already aligned in TASK-1562. - `library get` added to the routeTable as a clean GET to /api/v1/library/entry with `title` mapped to the query string. Cleaner than another explicit dispatcher case — matches playbook list / playbook show shape. ## Version bump ToolSurfaceVersion bumped from 0.4 → 0.5. Pure addition; no existing tool/action/param/bootstrap shapes changed. Backwards-compatible for any v0.4 consumer that doesn't enumerate the new tool. Documented in version.go with the same comment-block structure as prior bumps. ## Test coverage - catalog_readonly_test.go — pad_library added to the want{} map; three library action → cmdPath entries in expected{}; library list / get / activate added to liveCmdhelpDoc stubs. - dispatch_http_project_test.go — 4-case table test (defaults, category, full=true, category+full) pins category/summary query-param forwarding; library get routing test confirms the routeTable entry resolves. ## Live MCP verification - `initialize` handshake advertises padToolSurface.version=0.5. - `pad_meta version` returns tool_surface_version=0.5. - `pad_library list type=playbooks category=agent-workflows` returns 4 playbooks in summary mode (content stripped, summary populated, invocation_slug + arguments present). - `pad_library get title='Ship tasks'` returns {type: playbook, playbook: {…, content (9512 chars), invocation_slug: ship}}. Parent: PLAN-1560. Unblocks TASK-1564 (cleanups). * fix(onboard): update playbook body to use pad_library MCP tool per Codex review (round 1) Codex P2 on PR #615: the /pad onboard playbook body in internal/collections/playbook_library_onboard.go still told MCP-only agents that the library catalog was "not yet exposed as an MCP tool" and to work from memory — directly contradicting the pad_library tool this PR just landed and breaking the main advertised consumer of the new surface. Updated step B3 (conventions) to mention both surfaces side-by-side (`pad library list --type conventions` / `pad_library` with `action: list, type: conventions`), and rewrote step B5 (playbooks) the same way so the activate path doesn't drift either. Pre-PLAN-1560 IDEA-1514 reference removed from the body — the idea is now closed. No test pins the playbook body content; `make check` passes; the playbook seed still validates against the playbooks collection schema since trigger/scope/invocation_slug/arguments are unchanged. Closes the onboard-side scope of TASK-1564 (stale dispatch_http_slice4 hint + CHANGELOG still pending there).	2026-05-21 19:18:19 -04:00
xarmian	9a47c36ea6	chore(store): rephrase comment to unblock gofmt (BUG-1565) (#614) ... Go 1.26's gofmt rewrites paired ASCII apostrophes (''), used here as a literal SQL empty string, to a typographic right-curly double quote (U+201D). The rewrite is applied even inside markdown backticks, so escaping the SQL fragment in code-span syntax doesn't help. Rephrase the comment to describe the COALESCE/LOWER pattern in words instead of embedding the SQL literal, preserving the original meaning while sidestepping the heuristic. The behavior of adminOpenItemsCountClause is unchanged — comment-only edit. Unblocks `make check` for local pre-commit and CI gates. Surfaced during PLAN-1560's TASK-1561 ship loop.	2026-05-21 17:14:21 -04:00
xarmian	de1beb47a9	feat(cli): pad library get + list --full + server-side category filter (TASK-1562) (#613) ... CLI layer for PLAN-1560 (`pad_library` MCP tool + matching CLI surface). Wires the HTTP work landed in TASK-1561 through to the `pad library` subcommands. ## `pad library list` changes - `--category` is now a server-side filter (the old client-side display-only skip-loop is dead and removed). - New `--full` flag. Default JSON output for playbooks now returns the `summary` field (first non-heading paragraph, ~240 char cap) instead of the full `content`; `--full` opts back into full bodies for callers that want to pipe everything. - Table output gains a summary hint line under each playbook and a `/pad <slug>` chip when an invocation slug is declared, so the library becomes self-documenting as a discovery surface. - `--type` now validates explicitly instead of silently producing an empty list for unknown values. ## NEW `pad library get <title>` Calls `GET /api/v1/library/entry?title=X` and renders either a conventions card (title, category, trigger, surfaces, enforcement, commands, body) or a playbooks card (title, category, trigger, scope, invocation slug, argument count, body). Conventions-first precedence matches `pad library activate`. JSON output returns the full envelope. 404 errors return a clean `not found in library: "<title>"` message with exit code 1. ## CLI client - `GetConventionLibrary(category)` — pass category as a server-side query param. - `GetPlaybookLibrary(category, summary)` — same plus the summary toggle; `summary=true` strips Content and returns Summary instead. - NEW `GetLibraryEntry(title)` returning `*LibraryEntryResponse`. - `LibraryPlaybook` gained an omitempty `Summary` field so a single type round-trips both the legacy and summary shapes. ## Drive-by Switched `/library/entry` 400/404 from a flat `{error: "..."}` body to the canonical `writeError(code, message)` envelope used by the rest of the API. The CLI's `parseError` now hands back a typed `APIError` that `pad library get` pattern-matches on `Code=="not_found"` for the clean 404 message. Updated `TestLibraryEntry_MissingTitle` and `_NotFound` to assert the new envelope. ## Verification go build / go vet / go test ./... all green. golangci-lint clean on cmd/pad/..., internal/cli/..., internal/server/.... End-to-end smoke tests via the installed binary confirmed: list summary mode, list --full, list --category filter, get convention card, get playbook envelope, get 404 exit-1, --type validation. Parent: PLAN-1560. Unblocks TASK-1563 (MCP catalog wiring).	2026-05-21 16:59:57 -04:00
xarmian	2df6edeaab	feat(server): library endpoints gain ?category, ?summary, /library/entry (TASK-1561) (#612) ... Extends the convention + playbook library HTTP layer to support the shape the upcoming `pad_library` MCP tool and the updated `pad library` CLI need: - `GET /api/v1/convention-library?category=X` — server-side filter, case-sensitive exact match. Unknown categories return an empty slice, not 404. - `GET /api/v1/playbook-library?category=X&summary=true` — same filter plus a new summary mode that strips Content and injects Summary (first non-heading paragraph, ~240 char cap). Web UI and existing consumers omit the flag and see the legacy full-body shape. Summary mode deep-copies category slices so a request never mutates the package-level library data — TestPlaybookLibrary_SummaryDoesNotMutate Global pins this. - `GET /api/v1/library/entry?title=X` — NEW. Returns one matched entry in a `{type, convention|playbook}` envelope. Conventions-first precedence mirrors the dispatcher's `library activate` so a title resolves to the same kind in both surfaces. 400 on missing title, 404 on no match. Hoisted `playbookSummary` to `collections.PlaybookSummary` so the bootstrap handler and the new library endpoints share one algorithm. Bootstrap continues to call it for every playbook entry it returns. Adds 12 handler tests + the existing bootstrap-summary test stays green after the move. Lint clean on touched packages; `make check` gate is blocked by a pre-existing gofmt issue in internal/store/workspace_members.go captured as BUG-1565. Parent: PLAN-1560. Unblocks TASK-1562 (CLI) and TASK-1563 (MCP catalog).	2026-05-21 13:08:02 -04:00
xarmian	8a85eca713	feat(web): admin user table — cheap aggregation columns (TASK-1548) (#603) ... * feat(web): add cheap aggregation columns to admin user table (TASK-1548) Surfaces the per-user aggregations T1544 added to GET /admin/users: - Workspaces (numeric, after Role) - Storage (used bytes, formatted via the existing formatStorageBytes helper — same units the storage-override field accepts) - Last Write (relative time, color-coded by writeRecency: green <7d, yellow <30d, red ≥30d, gray italic when never) - Status pill (replaces the standalone "disabled" badge; renders for disabled / no-workspace / inactive; suppressed for "active" to keep the table calm) Implementation: - AdminUser interface in admin.svelte.ts gains last_write_at, workspace_count, storage_bytes, status — matching the server-side JSON shape from T1544. - writeRecency() helper in +page.svelte buckets the timestamp into a CSS class. Visual half of the API's status pill; same age windows. - .num-cell utility: right-aligned, tabular-nums so digits line up across rows (workspace_count and storage cells share it). - edit-row colspan updated to 9 (cloud_mode) / 8 (self-hosted) to span the new columns. Frontend purely additive — no API changes, no Go changes. T1549 wires up pagination + sort + filter; T1550-T1555 build the modal. Part of PLAN-1542. * fix: address Codex review on TASK-1548 1. handleAdminGetUser response now includes last_write_at, storage_bytes, and status — matching handleAdminListUsers' shape. Without these, the row-merge after PATCH (role change, disable, plan edit) kept stale values; e.g. disabling an active user wouldn't show the new "disabled" status pill until the full list reloaded. - Store.UserStorageUsage: new helper that sums attachments across all workspaces owned by the user. Mirrors WorkspaceStorageUsage's definition. - Store.ComputeAdminUserStatusValue: exported wrapper around the existing private helper so handler code can compute the pill value without round-tripping through SearchUsers. 2. writeRecency threshold: 30d is now "stale" (inclusive) rather than "cold". Matches server-side computeAdminUserStatus which only flips to "inactive" on > 30 days. Eliminates a 1-day boundary mismatch between the recency color and the status pill. 3. A11y: write-recency cell now carries aria-label with the bucket name ("Last write: 12d ago (stale)"), so screen-reader users get the same meaning the color conveys.	2026-05-20 18:10:25 -04:00
xarmian	48323e229e	feat(admin): GET /admin/users/{id}/metrics windowed engagement metrics (TASK-1547) (#602) ... Final backend task for PLAN-1542. Returns three engagement signals that power the metric tiles on the admin user modal's Overview tab (T1553): - days_since_write: derived from users.last_write_at (T1543). nil when the user has never had a write recorded. - writes_7d: COUNT of write-class activities (created/updated/archived/ restored/moved/commented) authored in the last 7 days. - collections_touched_30d: COUNT(DISTINCT collection_id) of items the user has authored writes for in the last 30 days. Goes through activities.user_id (not items.last_modified_by, which is an attribution string — see T1543's architecture note). api_requests_7d is intentionally NOT included; no per-request log exists. Filed as a follow-up (IDEA-1556) that will add this as an additive, non-breaking field once the request-log table lands. Implementation: - Store.GetUserMetrics in users.go runs three small queries: scalar SELECT for last_write_at, one COUNT(*) over activities, and a JOIN(activities, items) for the DISTINCT collection count. All three are index-backed (idx_activities_user from migration 022). - No caching layer in this PR. The queries are cheap, and a per-user short cache fits more naturally at the handler boundary if needed — premature here. - Handler handleAdminGetUserMetrics wired at GET /admin/users/{userID}/metrics. requireAdmin gate; 404 on missing user. Tests: TestGetUserMetrics seeds a workspace with two collections, six activities (five inside 7d, one ancient outside both windows), verifies all three metrics. TestGetUserMetricsEmptyUser covers the no-activity case (nil days_since_write, zero counts, no error).	2026-05-20 17:13:28 -04:00
xarmian	a04e5217c6	feat(admin): GET /admin/users/{id}/activity paginated feed (TASK-1546) (#601) ... * feat(admin): GET /admin/users/{id}/activity paginated feed (TASK-1546) New endpoint returns activities originated by the user — item writes, comments, account-level actions the user took themselves — in reverse- chronological order with offset pagination. Scope decision: feed shows activities where activities.user_id = userID (events the user authored). Admin actions targeting this user as a subject (role_changed where target_user_id is in metadata) are NOT included; that "received" sub-feed needs a JSON predicate and is filed as a follow-up. T1554 (modal Activity tab) consumes the current shape. Implementation: - Store.ListUserActivity mirrors the existing ListWorkspaceActivity / ListDocumentActivity helpers in activities.go. Same column projection, same LEFT JOIN users u for actor name. Hard cap at limit=50. - Handler asks for limit+1 rows so it can flag "next_offset" without a separate COUNT query — trims the extra before responding. Returns next_offset=null when the page is the last. - Route wired at GET /api/v1/admin/users/{userID}/activity. 404 on missing user; requireAdmin gate. - Pagination is offset-based (matching the sibling endpoints) rather than cursor-based as the task body suggested. For per-user feeds the dataset is bounded and between-page drift is acceptable for an admin tool. Cursor can be added later if needed; the response shape is forward-compatible (next_offset → next_cursor would just rename). Test: TestListUserActivity covers cross-user isolation, action filter, offset pagination across pages without overlap, and the 50-row hard cap. Part of PLAN-1542. * fix: address Codex review on TASK-1546 Lift the store-side cap on ListUserActivity from 50 to 100. The handler caps the public per-page at 50 and asks the store for limit+1 (51) to flag "more available" without a separate COUNT. Previous store-side cap of 50 silently truncated that probe, so next_offset would be null even when row 51 existed — clients iterating at page-size=50 would stop one page short of the actual end. The HTTP layer remains the source of truth for the per-page maximum; the inner cap is now just protection against pathological internal callers. Regression test seeds 51 activities and verifies the store returns all 51 when asked.	2026-05-20 17:07:57 -04:00
xarmian	eff0824238	feat(admin): GET /admin/users/{id}/detail per-workspace breakdown (TASK-1545) (#600) ... * feat(admin): GET /admin/users/{id}/detail per-workspace breakdown (TASK-1545) New endpoint that returns the user vitals plus a per-workspace breakdown enriched with the aggregations the admin user modal's Workspaces tab needs: collections_count (excluding system collections — playbooks, conventions, anything else is_system=1), items_open (status NOT IN a hardcoded terminal set), items_total, members_count, storage_bytes (matches WorkspaceStorageUsage's definition), and last_activity_at (MAX items.updated_at across non-deleted items). Implementation: - Store.GetUserWorkspacesDetailed in workspace_members.go uses correlated subqueries rather than a wide JOIN+GROUP BY — a single user belongs to at most tens of workspaces in practice, so the readability wins over micro-optimizing. Caps at 50 rows (frontend caps at 20 in T1552). - AdminUserWorkspaceDetail embeds the existing AdminUserWorkspace to keep the JSON shape backward-compatible with /workspaces consumers. - adminOpenItemTerminalStatuses is a hardcoded list (done/completed/ rejected/archived/implemented/cancelled). A schema-aware terminal_options check is a separate follow-up — flagged in the struct doc. - Handler handleAdminGetUserDetail wired at GET /admin/users/{userID}/detail. Returns 404 on missing user; defensive []AdminUserWorkspaceDetail{} serialization so JSON consumers see [] rather than null. Test: TestGetUserWorkspacesDetailed seeds a workspace with two user-facing collections, one system collection, three items (two open, one terminal), two members, and one attachment; verifies all six aggregations. Part of PLAN-1542. T1552 (modal Workspaces tab) consumes this. * fix: address Codex review on TASK-1545 Three real issues in the items_open count clause, all in the same SQL fragment: - Use s.dialect.JSONExtractText("i.fields", "status") instead of the SQLite-only JSON_EXTRACT(i.fields, '$.status'). The endpoint would have failed on Postgres deployments. - Wrap the extracted value in LOWER(COALESCE(..., '')) so items with NULL/missing status fields still register as "open" (NULL NOT IN (...) is not TRUE in SQL, which would have undercounted), and so case-variant statuses match. Matches the interpretation used in search.go and items.go. - Source the terminal list from models.DefaultTerminalStatuses rather than a private list — previous local list was missing 'resolved', 'wontfix', 'fixed', 'disabled', 'deprecated' (overcounting open items in workspaces that use those statuses). adminOpenItemsCountClause is now a Store method (was a package-level fn) because it needs the dialect.	2026-05-20 17:00:57 -04:00
xarmian	0c5ec04fac	feat(admin): extend user list with aggregations + sort/filter (TASK-1544) (#599) ... * feat(admin): extend user list with aggregations + sort/filter (TASK-1544) GET /admin/users now returns per-user workspace_count, storage_bytes, last_write_at, and a computed status pill (disabled / no-workspace / inactive / active, with documented precedence). Adds sort and filter knobs so the table can scale beyond the existing fixed offset/limit. Store layer: - AdminUserSearchParams gains Role, Sort, Order, ActiveWithinDays, HasWorkspaces, Disabled. Pointer types where tri-state ("no filter" vs. "filter to false") matters. - AdminUserListEntry wraps models.User with WorkspaceCount, StorageBytes, Status — returned in AdminUserSearchResult.Users. - SearchUsers SQL rewritten: LEFT JOIN against grouped subqueries so one user owning N workspaces with M attachments each still produces exactly one row (no aggregation explosion). Both subqueries filter deleted_at IS NULL to match WorkspaceStorageUsage's existing definition. Allow-listed sort clause prevents injection. - computeAdminUserStatus exported for unit tests; precedence locked in by TestComputeAdminUserStatus. - TestSearchUsersAggregations covers workspace_count + storage_bytes + status across a three-user fixture and each new filter/sort knob. Model + scanner: - models.User gains LastWriteAt. userColumns + scanUser updated; the legacy callers (GetUser, ListUsers, etc.) inherit the new field for free via the shared scanner. Handler: - handleAdminListUsers accepts the new params: role, disabled, has_workspaces, active_within_days, sort, order. Tri-state bools only fire when the query param is present. Response now embeds workspace_count / storage_bytes / last_write_at / status. Part of PLAN-1542. Frontend consumption lands in T1548 (cheap columns) and T1549 (sort/filter UI). * fix: address Codex review on TASK-1544 - Tri-state bool parsing in handler now uses strconv.ParseBool — accepts the canonical truthy/falsy variants ("True"/"TRUE"/"t"/"1" and the parallel falses), and silently ignores garbage values rather than treating them as false. Closes the "disabled=TRUE silently means enabled-only" surprise. - SearchUsers count query no longer joins the storage aggregation when HasWorkspaces isn't an active filter. The page query still needs both joins (the row carries the data), but a typical "give me a count" call no longer scans every live attachment. The workspace_count join remains conditional on HasWorkspaces filtering. Status threshold (>30d vs >=30d): the documented spec and impl both say ">30d" — no change.	2026-05-20 16:50:16 -04:00
xarmian	0a09c1dca7	feat(store): add users.last_write_at column + write-path hook (TASK-1543) (#598) ... * feat(store): add users.last_write_at column + write-path hook (TASK-1543) Engagement metrics need a "last write" signal distinct from last_active_at (which is bumped on any authenticated request, so it includes reads). Adds: - Migration 060: users.last_write_at + index. Backfills from activities table (the canonical record of who-did-what) using the action set that handlers_items.go / handlers_comments.go actually emit: created/updated/archived/restored/moved/commented. - Store.TouchUserWrite(ctx, userID): mirrors TouchUserActivity. Same 5-minute throttle to avoid write-amplification, silent no-op on empty userID so callers don't have to guard. - Hook in logActivityWithMetaReturningID (handlers_documents.go) — every item-write action funnels through this single helper, so one TouchUserWrite call covers item create/update/archive/restore/move and comment authoring. - Explicit hook in handlers_attachments.go after CreateAttachment, since uploads don't go through logActivity. Test: TestTouchUserWrite covers empty-userID no-op, first-write set, in-throttle suppression, and out-of-throttle advance. Architecture note: items.last_modified_by / comments.created_by are attribution strings ("user"/"agent"/"cli"), not user IDs. The user identity lives in activities.user_id, populated at the handler layer where currentUser is in scope. That's why the hook lives in handlers, not the store layer — and why the backfill reads from activities. Part of PLAN-1542 (admin user management enhancements). * fix: address Codex review on TASK-1543 - Add Postgres migration 039 (pgmigrations counterpart to 060). Same ALTER + index + activities-backfill, using TEXT to match the existing last_active_at / disabled_at column types in pgmigrations/020-021. Without this, Postgres deployments silently no-op TouchUserWrite because the column doesn't exist (and the call's UPDATE error is swallowed by design). - Hook TouchUserWrite in handleCreateCommentReply. The reply handler doesn't go through logActivity (no "commented" activity emitted for replies — verified by grep), so the activity-helper hook misses it. Explicit call after a successful CreateComment.	2026-05-20 16:39:23 -04:00
xarmian	db87b47754	fix(cli): pin server URL in .pad.toml for remote workspaces (BUG-1535) (#595) ... * fix(cli): pin server URL in .pad.toml for remote workspaces (BUG-1535) Two fixes: 1. Replace stale api.getpad.dev references with app.getpad.dev in the --url flag help, NewClientFromURL doc, and Config.URL doc. Also fix internal/mcp/dispatch_http.go's comment to use the canonical mcp.getpad.dev/mcp URL. 2. Persist the server URL into .pad.toml when linking a directory to a non-local workspace. WriteWorkspaceLink now takes a serverURL arg; pad init / workspace link / workspace switch pass cfg.BaseURL() when Mode != local. getConfig() reads .pad.toml's URL as an override above ~/.pad/config.toml and below the --url flag, so commands like `pad collection list` from a remote-linked directory hit the right server without --url on every call. Passing --url explicitly also promotes local → remote so the directory pin is written even when the existing global config has mode=local. * fix(cli): scope .pad.toml URL override to client paths per Codex review (round 1) Round 1 review flagged that applying the .pad.toml URL override inside getConfig() contaminates server/admin commands: pad server start would advertise the wrong PublicLinkBaseURL, and pad auth setup would refuse to run locally because Mode flipped to remote. Extract the override into applyPadTomlOverride() and call it only from client-API entry points — getConfiguredConfig() and the pad init client phase. Server/admin commands (pad server start/stop, pad auth setup, pad auth configure) keep using raw getConfig() and are unaffected. Also skip the override when --url was explicitly passed (LoadedFromFlags), so the flag retains unambiguous priority. * fix(cli): preserve .pad.toml URL on workspace link/switch per Codex review (round 2) Round 2 review noted workspace link / workspace switch reached the server via getClient() (override applied) but then wrote the new .pad.toml URL using a raw getConfig() — which would drop or miswrite the url field when relinking inside a remote-pinned directory whose global config is local. Reuse the cfg returned by getClient() for padTomlURLFor so the write matches the API client.	2026-05-19 17:02:46 -04:00
xarmian	e27f805ffc	feat(web): unify dashboard onboarding banners around needs_onboarding signal (TASK-1530) (#594) ... IDEA-1516 Phase 3. The pre-IDEA-1516 design split workspace onboarding guidance across two banners — OnboardingIdeaBanner (gated on the retired IDEA-1 / BACK-1 / FEAT-1 seed-item pattern from PLAN-1496) and OnboardingChecklist (gated on a totalItems === 0 heuristic that predates the canonical needs_onboarding flag from TASK-1504). Both fired competing CTAs on the same screen; neither read the canonical signal. Backend (internal/server/handlers_dashboard.go): - Add `NeedsOnboarding bool json:"needs_onboarding"` to DashboardResponse, populated via the existing Store.WorkspaceHasUserCreatedItems EXISTS query (same predicate AgentBootstrap.NeedsOnboarding uses). Web reads it from the dashboard fetch the page already does — no second round-trip against the heavier bootstrap endpoint. Frontend: - Add `needs_onboarding: boolean` to TS DashboardResponse type - Delete OnboardingIdeaBanner.svelte entirely (signal retired, no remaining consumers); the back-end onboarding_seed field stays for now per spec — separate cleanup - Delete OnboardingChecklist.svelte; replace with OnboardingNudgeBanner.svelte — single message + "Connect agent →" CTA that opens the workspace's already-mounted ConnectWorkspaceModal. Dismissible, preserves the existing `pad-onboarding-dismissed-{wsSlug}` localStorage key so users who dismissed the old checklist don't get re-prompted - Workspace +page.svelte: collapse the two banner blocks into one gated on `needsOnboarding && !onboardingDismissed`; reshow button follows the same signal. Drop the now-orphaned `.connect-card` CSS — its function is subsumed by the banner's CTA. Drop the unused OnboardingIdeaBanner / OnboardingChecklist imports and the `onboardingSeed` derived state Smart-suppression deferred to a follow-up. The existing api.workspaces.claimCode endpoint returns suppression info but generates a real claim code as a side effect in the not-suppressed case — calling it on every workspace page-load with needs_onboarding=true is awkward. The CTA still opens the modal, which renders its own suppression state correctly; users get the right experience with one extra click on the rare suppressed case. A dedicated read-only GET /workspaces/{ws}/connect-status endpoint is a separate piece of work.	2026-05-19 13:17:57 -04:00
xarmian	8041b46e36	fix(sse): surface write errors and link keepalive to IdleTimeout (BUG-1532) (#590) ... Two SSE-handler tidy-ups flagged during the BUG-1531 investigation. 1. writeSSEEvent now returns the underlying fmt.Fprintf error. Previously every event/keepalive write swallowed any error from the response writer — when the client TCP went away the handler kept looping, pulling events off the bus channel, and discarding them while waiting for the ctx.Done() cancellation to propagate. Now any write failure exits the handler immediately so the bus subscription is released and we stop fanning broadcast traffic into a dead socket. Marshal errors stay local (don't tear down a healthy stream over one un-marshalable payload). All five callsites + the keepalive Fprintf are updated to log at DEBUG (broken-pipe on client disconnect is normal traffic, not an error worth WARN-level noise) and return. 2. The 30s keepalive interval and the 120s IdleTimeout now live in named constants (sseKeepaliveInterval, httpIdleTimeout) with an init() guard that panics if `3 × keepalive >= IdleTimeout`. Used to be magic numbers in two files; bumping one without the other in lockstep silently created a window where idle SSE streams would get TCP-reset by the http.Server's idle deadline. The init guard fires at process start so a misconfigured constant is visible immediately, not three months from now when someone notices intermittent reconnect storms. Three new tests pin the contracts: - TestWriteSSEEvent_SurfacesWriteErrors - TestWriteSSEEvent_MarshalErrorIsLocal - TestSSEKeepaliveIdleTimeoutInvariant Closes BUG-1532. Full ./internal/server suite passes (~78s).	2026-05-18 18:29:28 -04:00
xarmian	b801867053	chore(web): npm audit fix — svelte 5.55.8, devalue 5.8.1, mermaid 11.15.0 (#589) ... * chore(web): npm audit fix — patch-bump svelte, devalue, mermaid Resolves three advisories surfaced by the Web CI job (and visible on recent main commits before this): - svelte: 5.55.5 → 5.55.8 (moderate × 4: SSR XSS via spread, hydratable Promise XSS, DOM clobbering, ReDoS in <svelte:element>) - devalue: 5.6.x → 5.8.1 (high: DoS via sparse array deser) - mermaid: 11.14 → 11.15.0 (moderate × 4: Gantt infinite-loop DoS, classDef CSS/HTML injection, config CSS injection) All three are in-range patch bumps — package.json untouched, only the lockfile changes. No major bumps, no API churn. Tiptap deliberately untouched: @tiptap/core, @tiptap/extension- collaboration, @tiptap/y-tiptap stay at 3.22.5 — the CLAUDE.md lockstep rule (coordinated bumps + Y.Doc schema-version bump) only applies when those three move together, and nothing here does. Verified: - npm audit: 0 vulnerabilities - npm run check: 0 errors (existing 6 warnings unchanged) - make build + make install: clean, server boots and serves the new bundle. * test(oauth): TestConsent_ApproveWithSpecificWorkspaces locates connection by shape The test asserts the persisted oauth_connections row from a specific- workspaces consent (allowed_workspaces=[alpha,beta]) has AllCurrentWorkspaces=false and the right slug list. It indexed via conns[0], which only worked when the approve flow was the most-recent connection — but the test also calls runAuthCodeFlow above the assertion to mint a bearer for the introspect call, and that helper posts allowed_workspaces=["*"] → wildcard connection. With ListUserOAuthConnections ordered ConnectedAt DESC, conns[0] is now the wildcard bearer row, not the alpha+beta row this test is verifying. We can't move the bearer-mint below the assertion (the introspect call needs the bearer first), and the auth-code response doesn't surface the request_id we'd need to look up the right connection directly. The specific-workspaces flow is the only one in this test with AllCurrentWorkspaces=false, so locate by that shape — surgical fix that matches the test's actual intent. Verified with `go test -count=3 -run TestConsent_ApproveWithSpecificWorkspaces` (deflakes across orderings) and the full ./internal/server suite.	2026-05-18 17:17:42 -04:00
xarmian	bfce069f28	fix(web,build): search palette hang on numeric query + graceful SSE shutdown (BUG-1531) (#588) ... * fix(web,build): search palette hang on numeric query + graceful SSE shutdown (BUG-1531) CommandPalette's reactive `$effect` subscribed to every workspace's `localSearch.epoch` + `localIndex.bootstrapStateFor`. Bare-digit queries short-circuit to `exactItemNumberLookup` (synchronous, very fast) and stacked re-fires of the effect inside one microtask tick whenever an SSE delta arrived — Svelte tripped `effect_update_depth_exceeded` and the palette froze. Treat bare-digit queries the same as `body:` queries (skip the subscription reads) and wrap `doSearch()` in `untrack()` so its internal reactive reads can't smuggle hidden dependencies into the effect. The SSE churn that fanned the loop was rooted in `make install` using `killall -9` — SIGKILL drops every open SSE stream mid-chunk so every browser tab logs `ERR_INCOMPLETE_CHUNKED_ENCODING` and reconnects. Switch to SIGTERM + 5s wait + SIGKILL fallback so the server's existing graceful-shutdown path (cmd/pad/main.go:811-857) actually runs and the http.Server writes a final 0-chunk on each open stream. Follow-up tidy-ups (unchecked write errors in writeSSEEvent, link the 30s keepalive to the 120s IdleTimeout in code) tracked in BUG-1532. * fix(web): track workspace slug in palette $effect per Codex review (round 1) After wrapping doSearch() in untrack(), the workspaceStore.current?.slug read that doSearch performs at line 209 no longer registered as a tracked dep of the search effect. The non-body / non-bare-digit branch still reads the slug via localIndex.bootstrapStateFor(...), so workspace switches re-fire the effect for that branch — but body: and bare-digit queries skip that block entirely. Without an explicit slug subscription they wouldn't re-dispatch on workspace switch; an in-flight server response would land stale, get discarded by isSameDispatch(), and loading could stick true. Hoist `void workspaceStore.current?.slug` into the unconditional void block so all four query shapes re-fire on workspace switch. Refs BUG-1531. * chore: gofmt handlers_claim_code_test.go Drive-by formatting fix to unblock CI on this PR. The file landed slightly unaligned in #586 (TASK-1525) — gofmt straightens the struct tag column on claimCodeResponse.	2026-05-18 15:25:27 -04:00
xarmian	fc6afd01be	feat(connect): unified Connect-to-agent modal + claim-code endpoint (TASK-1525) (#586) ... * feat(connect): unified Connect-to-agent modal + claim-code endpoint (TASK-1525) Phase E of PLAN-1519. Repurposes the avatar-menu "Connect a project…" modal as a one-stop hub where users can connect ANY agent surface (claim-code → existing OAuth grant, fresh MCP OAuth, or local CLI) to the current workspace. Backend - GET /api/v1/workspaces/{slug}/claim-code — generates a stateless 6-digit HMAC claim code (re-uses the verifier's secret + bucket math) for the calling member, OR reports `suppressed: true` when smart-suppression detects the workspace is already covered by one of the user's active OAuth connections (wildcard OR explicit allow-list rows). Returns `expires_at` at the current bucket boundary so the UI can drive a countdown. - store.IsWorkspaceCoveredForUser — single indexed query against oauth_connections + oauth_*_tokens; filters by ACTIVE tokens so a dangling revoked connection row doesn't suppress fresh modals. - Tests cover 412 (disabled), 404 (non-member), 200 + matching code, wildcard suppression, explicit-allow-list suppression, and the revoked-connection-doesn't-suppress invariant. Frontend - ConnectWorkspaceModal rewritten as a tabbed unified modal: - Agent (claim code) — fetches on activate, live countdown, auto-refetches at bucket roll-over, renders smart-suppression panel that links to /console/connected-apps, and renders the locked prompt block `Authorize the pad workspace '<slug>' with claim code <code>.` per IDEA-1517 §4. - MCP setup — subsumed from the now-deleted ConnectMCPModal: URL block + client-card grid linking to per-client docs. - CLI — existing install + `pad init` flow, unchanged. - Default tab: Agent when mcpPublicUrl is set; CLI when not. MCP tab hidden entirely on self-host without a public MCP URL. - ConnectBanner simplified: single modal state, generic "Connect an AI agent to this workspace" copy, no MCP/CLI dual-modal branching. - ConnectMCPModal.svelte deleted (fully subsumed). - API client gets `workspaces.claimCode(slug)` + `ClaimCodeResponse` TypeScript type. - TopBar + workspace home callsites pass `mcpPublicUrl` from authStore so the unified modal can pick the right default tab. Verification - go build ./... clean - go test ./... — all packages green (server + store) - cd web && npm run build — clean Parent: PLAN-1519. Phases A-D already shipped (oauth_connections schema, MCP claim action, /authorize redesign, connections-page mutation UI); this lands Phase E. Phase F (TASK-1526) will wire post-create auto-open from IDEA-1516's new-workspace modal; Phase G (TASK-1527) is cross-agent paste validation of the locked prompt string. * fix(connect): require membership at claim-code generation; guard modal against stale-response races per Codex review (round 1) 1. Guest-grant generation gap. RequireWorkspaceAccess admits item-grant guests who aren't workspace members; claim-code REDEMPTION requires full membership. Generating without the same check handed guests a valid-looking code + prompt that the claim endpoint always 404s. Add an explicit GetWorkspaceMember check after getWorkspace and return 403 not_a_member to fail closed on the same response shape the redemption path would have used. 2. Stale-response race in the modal. ConnectWorkspaceModal stays mounted across workspace switches (TopBar reuses the same instance), so an older claimCode fetch can resolve AFTER a newer one and stomp claimState with suppression or a code for the wrong workspace. Add a monotonic seq + captured-slug guard mirroring the refreshHasAgentActivity pattern already in ConnectBanner. Test additions: - TestHandleWorkspaceClaimCode_GrantOnlyGuest_403 asserts a non-member authenticated caller never gets a 200 + code from the generation endpoint.	2026-05-18 10:25:08 -04:00
xarmian	26aa800b3b	feat(oauth): connected-apps mutation endpoints + edit UI (TASK-1524) (#585) ... * feat(oauth): connected-apps mutation endpoints + edit UI (TASK-1524) Phase D for PLAN-1519. Adds four mutation endpoints under /api/v1/connected-apps/{id}/... and extends the console page with an inline Edit panel per connection card. Backend (internal/server/handlers_connected_apps.go + server.go) - PATCH .../name — rename, trim + cap at 120 chars - PATCH .../flags — atomic set of may_create / all_current / include_future (rejects toggling all_current=false when the join table is empty — empty-allow-list invariant from IDEA-1517 §3 Acceptance) - POST .../workspaces — add workspace; membership-checked, 404 uniform when the user isn't a member (no enumeration leak) - DELETE .../workspaces/{slug} — remove; idempotent for missing slugs; rejects last-workspace removal when all_current=false (same orphan guard as the flags handler) All four route through requireConnectionOwner which returns the same 404 envelope for non-owned connections as the existing Revoke endpoint. Each handler echoes the updated DTO so the page can re-render in place; respondWithConnection handles both active-token chains (via ListUserOAuthConnections) and connections without token rows (direct fetch of oauth_connections + the access projection). DTO (connectedAppDTO) gains name + the three scope flags. Model already carried the fields (TASK-1522). Frontend (web/...) - ConnectedApp TS type extended; api.connectedApps gains rename/updateFlags/addWorkspace/removeWorkspace methods. - Connections page: Edit button per card opens an inline panel with: connection name input (debounced save), three scope-flag toggles (auto-save), allow-list chips with X-to-remove plus a workspace picker that lists memberships not already in the list. - Last-workspace removal disabled at the UI level (chip-remove disabled when list length <= 1); API enforces the same invariant if a tampered call slips through. - Workspaces fetched lazily on first Edit open (cached for the page lifetime). Tests - 7 handler tests cover happy paths + edge cases: - rename trims/caps, non-owner 404 - flags happy path + empty-allow-list block - add workspace happy path + non-member 404 - remove workspace happy path + last-blocked + idempotent missing - loginTestUserAs helper to seed a second user for the non-owner case (the existing loginTestUser hardcodes a single email). Parent: PLAN-1519. * fix(oauth): wildcard→specific toggle now works after pre-stage per Codex review (round 1) PR #585 round 1 caught that switching all_current_workspaces from true to false was effectively impossible: the flags handler's empty-allow-list guard called GetOAuthConnectionAccess, which intentionally short-circuits on wildcard and reports zero slugs. Even with join rows present, the guard always tripped. The UI compounded the issue by hiding the allow-list editor while in wildcard mode, so users had no way to pre-stage workspaces. Backend fix: new Store.ConnectionWorkspaceCount(requestID) returns the raw join-row count regardless of the parent's wildcard flag. The flags handler now uses this; the remove-workspace handler's "orphan guard" also routes through the new count (plus an IsConnectionWorkspaceAllowed probe so a no-op removal of a slug that isn't even in the list doesn't trip the guard). Frontend fix: the allow-list editor renders unconditionally inside the Edit panel. While wildcard is on, an "Inert while wildcard is on" badge clarifies that staged workspaces don't take effect until the user flips the flag. Chip-remove disabled only when actively in specific mode AND about to drop to zero — wildcard-mode removes are always allowed. Added TestHandleUpdateConnectedAppFlags_WildcardToSpecific_AfterPrestage as the regression guard: seeds a wildcard connection, adds one workspace via the API, then asserts the flag flip succeeds and the resulting DTO carries the pre-staged slug. Parent: PLAN-1519. * fix(oauth): staged-while-wildcard rows now visible per Codex review (round 2) PR #585 round 2 caught that the round-1 fix (always-rendered allow-list editor + Backend ConnectionWorkspaceCount) was incomplete: pre-staging a workspace while wildcard=true succeeded on the server but the response DTO still suppressed the staged slugs (ListUserOAuthConnections + respondWithConnection both set AllowedWorkspaces=nil when AllCurrentWorkspaces=true). The UI rendered "No workspaces staged" even after a successful add, so users had no way to see or remove a mistaken staged row. Backend fix: new Store.ListConnectionWorkspaceSlugs returns the join table's slugs regardless of the wildcard flag. ListUserOAuthConnections + respondWithConnection both route through it now. The hot-path GetOAuthConnectionAccess still short-circuits on wildcard (correct for the introspection path — when wildcard is on, slugs are irrelevant for gating); the read- for-display path needs to surface them. Frontend fix: isAnyWorkspace now reads the all_current_workspaces flag directly (drives the "Any workspace" badge), independent of the slug list. The slug list drives the edit panel chips. Legacy fallback for missing-flag wire shapes preserved. Added TestHandleAddConnectedAppWorkspace_VisibleUnderWildcard as the regression guard: seeds a wildcard connection, adds a workspace, asserts the slug appears in DTO.AllowedWorkspaces while AllCurrentWorkspaces stays true. Parent: PLAN-1519.	2026-05-18 08:33:39 -04:00
xarmian	93b9590bf9	feat(oauth): consent screen rewrite + new-tables write path (TASK-1523) (#584) ... Phase C2 for PLAN-1519. Rewrites the /oauth/authorize consent flow per IDEA-1517 §2a: collects a display name, three scope flags (may_create_workspaces, all_current_workspaces, include_future_workspaces), and the per-workspace allow-list — then writes oauth_connections + oauth_connection_workspaces instead of session.Extra. Backend (internal/server/handlers_oauth.go) - parseConsentPayload returns a structured consentDecision carrying name, three flags, and resolved workspace IDs. Two-radio + checkbox UI maps to flags per §2a's table; backward-compat shim still accepts the legacy allowed_workspaces=["*"] / explicit-list shape so pre-TASK-1523 clients and fixtures keep working. - handleOAuthAuthorizeDecide INSERTs oauth_connections (parent) + oauth_connection_workspaces (children) BEFORE NewAuthorizeResponse. Ordering matters: an INSERT failure prevents code minting, so the failure mode is "user retries" rather than "token issued with no connection-level state" (which the dual-read gate would silently broaden to no allow-list). A successful INSERT followed by NewAuthorizeResponse failure leaves an orphan oauth_connections row — harmless, no token references it. Per-slug INSERT failures roll back via DeleteOAuthConnection (FK CASCADE clears the join). - session.SetAllowedWorkspaces call retired; the dual-read introspection gate from Phase A still consults legacy session.Extra for pre-TASK-1523 tokens during the soak period. - /oauth/authorize accepts ?suggested_name= URL param, trimmed + 120-char-capped, threaded through to the template for prefill. Frontend (consentTmpl) - Per §2a mockup: identity header, name input (with prefill), data-permissions block (capability_tier radios — unchanged from pre-TASK-1523), workspace-access radio ("All my workspaces" default vs "Only specific workspaces"), conditional picker that slides in for the specific mode, may_create_workspaces checkbox (default-on), footer disclosure, and Authorize/Deny buttons. - Picker shows a search input when the user has >10 workspaces; per-row role label. - Mobile layout (max-width: 480px) stacks the actions vertically with Authorize above Deny. - "Pick at least one workspace" helper text appears when the user is in specific mode with zero workspaces selected. - Empty-workspace case still renders the "create or join one first" empty state; server validates regardless of JS. Tests - 2 new tests: TestConsent_ApproveWithNewShape (end-to-end with connection_name + workspace_access + may_create_workspaces + allowed_workspaces — asserts the values land on the oauth_connections row and join table); TestConsent_SuggestedNamePrefill (?suggested_name= URL param prefills the form input). - Existing approve-flow tests updated: the introspection response no longer carries allowed_workspaces (we stopped writing session.Extra), so assertions now read the connection row + join table via ListUserOAuthConnections — same guarantee, new shape. TestOAuth_Authorize_RendersConsentWhenLoggedIn seeds a workspace so the consent form renders its allow-list section (the new template hides it when the user has zero memberships). Parent: PLAN-1519.	2026-05-18 07:51:12 -04:00
xarmian	905baaa010	feat(oauth): backfill session.Extra into oauth_connections + switch read path (TASK-1522) (#583) ... * feat(oauth): backfill session.Extra into oauth_connections + switch read path (TASK-1522) Phase C1 for PLAN-1519. Seeds existing OAuth grant chains into the new connection tables (Phase A) and switches /console/connected-apps to read from them, retiring the session.Extra parse on the read path. Backfill (internal/store/oauth_connections_backfill.go) - Walks oauth_access_tokens + oauth_refresh_tokens to find every distinct request_id chain (including refresh-only chains). - Picks the newest token row per chain — its session.Extra drives the seeded shape, so a chain whose user re-scoped recently reflects the latest decision. - Maps session.Extra shapes to the new tables per IDEA-1517 §2: no key → all_current=1; ["*"] → all_current=1; explicit slugs → all_current=0 + one join row per slug (added_by='user'). - Resolves slugs → workspace IDs; unresolved slugs (deleted / renamed workspace) are counted + logged at WARN, not fatal. - Idempotent on every INSERT (OR IGNORE / ON CONFLICT DO NOTHING) so re-running on every startup is a cheap no-op once stable. - Returns a BackfillOAuthConnectionsResult so the startup log reports chains_seen / connections_created / workspaces_added / unresolved_slugs — operators see fresh work and notice drift. Read-path rewrite (internal/store/connected_apps.go) - ListUserOAuthConnections projects AllowedWorkspaces from GetOAuthConnectionAccess (oauth_connection_workspaces JOIN workspaces) instead of parsing session.Extra strings. - Hydrates Name + MayCreate + AllCurrent + IncludeFuture from oauth_connections so Phase D's mutation UI has them. - Defensive fallback for chains without an oauth_connections row (any leftover the backfill missed): treats as legacy "any workspace, default-on flags" so the connection still renders. Backfill at startup keeps this branch unreachable in production. - Retires parseAllowedWorkspacesFromSession; the new extractAllowedWorkspacesFromSessionExtra helper in oauth_connections_backfill.go is the only consumer of the session.Extra shape on the store side. Model (internal/models/connected_apps.go) - Adds Name / MayCreateWorkspaces / AllCurrentWorkspaces / IncludeFutureWorkspaces. AllowedWorkspaces semantics stay stable (nil = "any"; explicit slugs = chip list) so the existing DTO + frontend continue working unchanged. Phase D exposes the new fields on the wire. Startup wiring (cmd/pad/main.go) - After srv.SetOAuthServer / SetClaimSecret, run the backfill once. Non-fatal on error (partial state is consistent and the next run completes). Quiet at the Debug level on steady-state re-runs; INFO when fresh work landed. Tests - 8 BackfillOAuthConnections cases: empty DB, pre-TASK-952 (no key), wildcard, explicit list, mixed resolvable/unresolved slugs, multi-row chain newest-row-wins, refresh-only chain, idempotent re-run (verified via post-run row count). - TestExtractAllowedWorkspacesFromSessionExtra replaces the retired parseAllowedWorkspacesFromSession test — covers all three IDEA-1517 §2 input shapes + malformed/non-array defensive cases. - TestListUserOAuthConnections_DeduplicatesChain + TestHandleListConnectedApps_DTOShapeAndAuditEnrichment updated to call BackfillOAuthConnections (the production startup hook) before asserting on AllowedWorkspaces — mirrors the real-world flow now that the read path no longer parses session.Extra inline. Parent: PLAN-1519. * fix(oauth): backfill counters reflect actual new rows per Codex review (round 1) PR #583 Codex review round 1 flagged that the backfill counters over-report on steady-state restarts: - wasFreshlyInserted compared updated_at vs created_at — true for every untouched existing row, so every restart counted every pre-existing connection as "created." - slugsAdded++ ran after AddConnectionWorkspace regardless of whether the INSERT OR IGNORE / ON CONFLICT DO NOTHING hit an existing row. Net effect: startup logs "backfill complete" with non-zero counts on every restart instead of the intended quiet "no-op" path — making real fresh work indistinguishable from steady-state. Fix: probe existence BEFORE the insert on both sides. - backfillOneChain reads GetOAuthConnection first; only sets created=true and runs insertOAuthConnectionIfAbsent on a miss. - Per-slug: IsConnectionWorkspaceAllowed pre-check; skip + don't increment when the row already exists. Two cheap PK / indexed lookups per chain. Pre-Phase-C deployments have small chain counts so the added cost is well below the scan already running. Removed the now-unused wasFreshlyInserted helper. Added an assertion in TestBackfillOAuthConnections_Idempotent that both ConnectionsCreated and WorkspacesAdded report 0 on the second run — the regression guard for this exact finding. Parent: PLAN-1519. * fix(oauth): backfill skips slug re-seed on existing rows per Codex review (round 2) PR #583 round 2 caught that the round-1 fix protected the parent oauth_connections row from re-seed but left the join table mutable from stale session.Extra: When a user removes a workspace from their connection's allow-list via Phase D's mutation UI (RemoveConnectionWorkspace), the next server restart would re-run the backfill, find the parent row intact, and re-INSERT the removed slug from the original session.Extra. The user's removal would silently revert every restart. Fix: backfill is a one-shot seed. Once the parent row exists, the new tables are authoritative — legacy session.Extra is frozen reference data, not a reconciliation source. The slug loop only runs when we just inserted a fresh parent row. Added TestBackfillOAuthConnections_DoesNotResurrectRemovedWorkspace as the regression guard: seeds two slugs, removes one, runs backfill again, asserts the removed slug stays gone and the kept slug is untouched. Parent: PLAN-1519. * fix(oauth): atomic per-chain backfill transaction per Codex review (round 3) PR #583 round 3 caught that round 2's "only seed slugs on fresh parent" gate introduced a permanent-partial-state risk: if the process crashes (or AddConnectionWorkspace errors) between inserting the parent row and finishing the slug loop, the next backfill sees created=false, short-circuits the slug seeding, and leaves the connection permanently scoped to a partial allow-list. Fix: per-chain transaction. Parent insert + every slug insert land in one BEGIN/COMMIT pair; any mid-loop failure rolls everything back. The next backfill then sees the chain as un-seeded and retries from scratch — preserving both round 2's "no-resurrection of user-removed slugs" (existence probe inside the tx) and round 3's "no permanent partial seed" (atomic commit). Scope: per-chain (small tx), not whole-backfill. The original no-transaction rationale was about lock-hold duration across thousands of chains; that doesn't apply at chain granularity (one parent + a handful of join rows = sub-millisecond hold). Removed the now-unused insertOAuthConnectionIfAbsent helper; the INSERTs live inline within the transaction. Added TestBackfillOAuthConnections_AtomicOnMidLoopFailure as the regression guard: forces a mid-loop INSERT failure via a duplicate slug in session.Extra (which violates the join table's PK on the second insert), asserts the parent row rolled back, then runs a clean retry and verifies full seed completion. Parent: PLAN-1519. * fix(oauth): surface store errors from backfill + list path per Codex review (round 4) PR #583 round 4 caught two silent-fallthrough paths that could leak partial/incorrect state instead of failing loudly: 1. Backfill slug loop: GetWorkspaceBySlug errors were treated the same as "workspace not found" — both incremented slugsMissed and continued. A real I/O error mid-loop would commit a partial allow-list, and the next backfill's parent-exists short-circuit would make that partial scope permanent. Fix: distinguish (nil, nil) "not found" from (nil, err) "real failure" — return the error so the per-chain transaction rolls back and the next run retries cleanly. 2. ListUserOAuthConnections hydration: GetOAuthConnectionAccess and GetOAuthConnection errors collapsed into the "no oauth_connections row" defensive-fallback branch, returning the legacy "any workspace, default-on flags" shape. On a real store failure that silently broadens a user's scope — e.g. a connection the user explicitly removed a slug from would render as "Any workspace" until the store recovered. Fix: surface store errors from both calls; the defensive fallback path is now exclusively for HasConnection=false, not for error masking. Both findings tighten the failure mode from "silently emit broadened/partial state" to "surface the error so retries happen against accurate data." Existing tests cover the happy paths; the failure paths are exercised by I/O errors against the same store interfaces (no new test added — the change is "return err instead of swallow it" and the assertion of NOT swallowing is the diff itself). Parent: PLAN-1519.	2026-05-18 03:32:42 -04:00
xarmian	aec67e202e	feat(mcp): workspace.create + workspace.claim actions + claim-code mechanics (TASK-1521) (#582) ... Phase B for PLAN-1519. Adds two MCP actions so agents can bring a workspace into an OAuth connection without re-auth — the agent-first onboarding story IDEA-1517 §1 set out to fix. pad_workspace.action: create - New action on the shared catalog (stdio + cloud both pick it up). - POSTs to /api/v1/workspaces; handler auto-adds the new workspace to the calling OAuth connection's allow-list (added_by='agent-create') when the grant carries may_create_workspaces=true. Phase A wired the oauth_connection_workspaces table this writes to. PAT / CLI-session callers fall through silently (no request_id → no side effect). - Backed by a new non-interactive `pad workspace create <name>` Cobra command for the stdio MCP shell-out path. `pad workspace init` remains the guided human flow. pad_workspace.action: claim - New POST /api/v1/oauth/claim endpoint redeems a 6-digit stateless HMAC code minted from (user_id, workspace_id, 5-min time bucket) with a sliding 5–10 minute lifetime. Constant-time compare. Code format derived per IDEA-1517 §4. - Verifies workspace membership before code (privilege-escalation guard); uniform 404 envelope so the endpoint can't be used to probe existence vs. membership. - Side effect inserts a row in oauth_connection_workspaces with added_by='claim'. Idempotent — re-claiming returns 200 with already_added=true. - 412 connection_not_persisted when the OAuth grant predates Phase C (no oauth_connections row); 412 claim_disabled when the deployment hasn't wired SetClaimSecret. - `pad workspace claim <code> --workspace <slug>` Cobra command backs the stdio MCP shell-out. MCP server instructions - Appended IDEA-1517 §5 paragraph teaching agents the claim flow as a peer top-level section. Same string lands universally on every MCP handshake response (stdio + cloud both read instructions.md). Tests - 10 claim-code unit tests: determinism, zero-pad, length-prefix collision guard, current/previous bucket accept, aged-out reject, wrong-everything rejects, short-secret fails closed. - 7 handler tests: 412 when secret disabled, 400/404/401 vocabulary, PAT caller note path, 412 connection_not_persisted, idempotent insert. - 5 MCP-catalog tests: actions registered, schema params advertised, description mentions both actions, route mappers produce correct HTTP shape, routeTable carries the entries. - Bumped the existing read-only catalog bijection + fixture-input fixtures so the new actions resolve cleanly. Parent: PLAN-1519.	2026-05-18 00:43:46 -04:00
xarmian	3f6bcc0ff2	feat(oauth): per-connection state tables + dual-read introspection gate (TASK-1520) (#581) ... * feat(oauth): per-connection state tables + dual-read introspection gate (TASK-1520) Phase A foundation for PLAN-1519 / IDEA-1517's per-OAuth-connection state overhaul. Promotes the consent-time workspace allow-list out of session.Extra (per-token, re-minted on every refresh-token rotation) into dedicated tables keyed by request_id (the grant chain identifier preserved across rotations). Schema (SQLite migration 059 + Postgres migration 038): - oauth_connections: one row per grant chain with name + three scope flags (may_create_workspaces, all_current_workspaces, include_future_workspaces). - oauth_connection_workspaces: mutable allow-list join table; PK on (request_id, workspace_id); FK ON DELETE CASCADE; added_by audit column. Store (internal/store/oauth_connections.go): Create/Get/Rename/SetScopeFlags/ Add+Remove+IsAllowed/Delete CRUD. GetOAuthConnectionAccess is the hot-path projection — one PK lookup + one indexed join when the wildcard flag is off, nothing else when it's on. Dual-read gate (internal/server/middleware_mcp_auth.go): OR-merges the legacy session.Extra allow-list with the new-table projection. A workspace is allowed iff either source allows it; either source's wildcard makes the gate unrestricted. New tables stay empty until Phase C writes the consent screen, so the dual-read is a no-op until then — and existing OAuth grants keep working unchanged through the Extra path. I/O errors on the new path fall back to the Extra path so a transient outage of the new tables can't regress existing connections. Tests: - 10 store tests cover CRUD, FK cascade, wildcard short-circuit, sorted slug projection, idempotent add/remove, ErrOAuthConnectionNotFound on missing rows. - 11-case table-driven test on mergeAllowedWorkspaces directly verifies PLAN-1519's acceptance criterion: "token with allow-list in session.Extra still passes; token with empty session.Extra but row in oauth_connection_workspaces also passes." Plus wildcard precedence, union dedup, and fail-closed-on-empty-scope. - BenchmarkMergeAllowedWorkspaces measures policy-function overhead on the hot path (the store-side lookup is the other half of the dual-read cost). Parent: PLAN-1519. * fix(oauth): fail-closed on connection lookup error per Codex review (round 1) PR #581 Codex review round 1 caught two issues: 1. middleware_mcp_auth.go: GetOAuthConnectionAccess errors fell through to "no connection" + nil allow-list = unrestricted. Post-Phase-C (when the new tables are authoritative and session.Extra is empty), a DB read error on a scoped token would silently grant access to every workspace the user belongs to. Now fails closed with a 401 matching the IntrospectToken storage-error policy, increments MCPAuthzDenialsTotal{connection_lookup_error} for ops visibility. 2. oauth_connections.go: CreateOAuthConnection's docstring claimed "scope flags default ON if not supplied," but the method writes the three Go bools verbatim — and Go zero-values for bool are false, not true. The schema-level DEFAULT TRUE is unreachable through this path. Docstring updated to clarify that defaults live at the form-rendering layer; the store is a faithful pass-through. Parent: PLAN-1519. * test(oauth): add store-side bench for GetOAuthConnectionAccess per Codex review (round 2) PR #581 Codex review round 2 flagged the docstring references to bench_oauth_connections_test.go pointing at a file that didn't exist — only the in-memory mergeAllowedWorkspaces bench was wired. Add the store-side bench so the documented file is real and PLAN-1519 Phase A's "Hot-path benchmark: dual-read overhead measured and documented" acceptance bullet is satisfied end-to-end. Three shapes covered: Wildcard (PK lookup, join short-circuited), Explicit (PK + indexed scan + small workspaces join), and NoRow (the dominant Phase-A path until Phase C wires the write path). Local numbers (Ryzen 3 5300U, SQLite WAL): 12µs/12µs/37µs respectively — all comfortably sub-millisecond. Parent: PLAN-1519.	2026-05-18 00:03:20 -04:00
xarmian	2a1a00e385	test,docs: blank+onboard+needs_onboarding integration test + CLAUDE.md update (TASK-1507,1508) (#580) ... PLAN-1496's final consolidation pair, shipping together because both are small cleanup passes that close the plan out. TASK-1507 (tests): Most of the test coverage required by this task was already added incrementally in the prior PRs that built each surface: - Blank template (4 focused tests, PR #575): TestSeedFromBlankTemplate, TestBlankTemplateShape, TestBlankTemplateExcludesSoftwareCollections, TestBlankTemplateUsesMinimalVocabularies, TestBlankTemplateAppearsInPicker - Onboard auto-seed (PR #576): TestSeedFromTemplateAlwaysIncludesOnboardPlaybook (walks all six templates), TestSeedWithEmptyTemplateNameSkipsOnboard (locks the empty-templateName escape-hatch invariant), TestOnboardPlaybook_Contract (invocation_slug, trigger, mode-enum, ADAPT-DON'T-CURATE rule in the body) - needs_onboarding (PR #578): TestBootstrapNeedsOnboardingFlag (lifecycle: fresh → user item → flag flips), TestBootstrapNeedsOnboardingIgnoresTemplateSeeds (template seeds don't count) - Retired-pattern updates (PR #577): TestSoftwareTemplatesShipNoSeedItems inverse invariant; TestDashboardOnboardingSeed_NilForAllTemplates collapsed from three IDEA-1/BACK-1/FEAT-1 tests. This commit adds ONE integration smoke test that ties the three subsystems together at the bootstrap layer: - TestBootstrapBlankWorkspaceOnboardReady creates a blank-template workspace, fetches bootstrap, and asserts: needs_onboarding=true (nudge fires) AND the onboard playbook is in bootstrap.playbooks AND its status is "active" AND its trigger is "manual" (in the blank template's seeded vocabulary). If any one of the three pieces regresses silently, the integration breaks and this test catches it before /pad onboard stops dispatching on day one. TASK-1508 (docs): - CLAUDE.md "Data Model / Templates" section: added Blank under a new "Custom" category bullet pointing at the new Onboarding section; called out the PLAN-1496 retirement of the IDEA-1 / BACK-1 / FEAT-1 first-person seed pattern; updated design history reference to include PLAN-1496. - CLAUDE.md "API" section: added the /api/v1/workspaces/{ws}/agent/bootstrap endpoint with a note about the needs_onboarding flag (was previously documented only inline in the Playbooks section). - CLAUDE.md: new top-level "Onboarding" section between Playbooks and Testing. Covers: auto-seeded everywhere, surface-agnostic body, adaptation posture (library entries are starting points), the three TASK-1510/1511/1512 mutation primitives, the needs_onboarding bootstrap flag + skill nudge, the four retired surfaces (pad onboard cobra, OnboardingPrimaryRef, *OnboardingItems generators, standalone skill workflow section), and a code map. Verification: - go test ./...: clean (full suite passes including the new integration test) - make lint: 0 issues Parent: PLAN-1496.	2026-05-17 16:53:29 -04:00
xarmian	96a32aa39d	feat(bootstrap,skill): add needs_onboarding flag + retire legacy Onboarding workflow (TASK-1504,1505) (#578) ... PLAN-1496's bootstrap-signal + skill-cleanup pair, shipped together because TASK-1505's nudge rendering depends on TASK-1504's bootstrap field. TASK-1504 (bootstrap: needs_onboarding): - internal/store/items.go: new WorkspaceHasUserCreatedItems(workspaceID) store method. Backed by SELECT EXISTS with the predicate `source != 'template'` — defined as the inverse of template seeding rather than enumerating user-side source values, so new attribution surfaces (mcp, api, future) count automatically. - internal/server/handlers_bootstrap.go: AgentBootstrap struct gets the NeedsOnboarding bool field (always emitted — not omitempty, since the agent reads it on every /pad invocation). BuildAgentBootstrap computes it via the new store method. On query error the flag falls back to false (safe default: don't nag). - Visibility filtering deliberately omitted — needs_onboarding is a workspace-level state signal, not a per-user view. Two members reading bootstrap concurrently should see the same answer. - Two focused tests: - TestBootstrapNeedsOnboardingFlag walks the lifecycle (fresh workspace → create user item → flag flips). - TestBootstrapNeedsOnboardingIgnoresTemplateSeeds locks the template-seeds-don't-count invariant on the startup template, which ships seeded conventions, playbooks, and the onboard playbook itself. TASK-1505 (skill update): - skills/pad/SKILL.md: - Context Loading section: new bullet documenting needs_onboarding with the exact nudge wording the agent should render when true, plus the "don't nag past first user item" + "respect prior decline" rules. - Onboarding workflow section: deleted (~30 lines). Replaced with a one-paragraph pointer at the /pad onboard playbook. The skill is the dispatcher; the playbook body is the script. - Routing entry under "set up my workspace": simplified from the bloated PR #577 round-3/4 text into a clean two-bullet form (canonical phrasing + legacy IDEA-1 phrasing both → /pad onboard). - internal/mcp/prompts_data.go: the pad_onboard MCP prompt body was duplicating the same step-by-step script the SKILL.md section carried. Replaced with the same dispatch-to-playbook pointer. internal/mcp/prompts_test.go: TestPromptsLockstep_CoreCommands fragments updated to assert the new dispatch fragments (`pad playbook list`, `pad playbook show onboard`). Parent: PLAN-1496.	2026-05-17 13:57:12 -04:00
xarmian	0930743304	feat: retire IDEA-1 seed pattern + 'pad onboard' cobra + surface blank in init (TASK-1501/1502/1503) (#577) ... * feat: retire IDEA-1 seed pattern + 'pad onboard' cobra + surface blank in init (TASK-1501,1502,1503) PLAN-1496's legacy-onboarding teardown: TASK-1501 (remove seed items + update banner): - internal/collections/templates_onboarding.go (and the _product/_scrum siblings) deleted — these generated the IDEA-1/PLAN-2/TASK-3/DOC-4 + BACK-1/SPRINT-2/BUG-3/DOC-4 + FEAT-1/FB-2/ROAD-3/DOC-4 first-person seeds. The /pad onboard playbook (TASK-1499 / TASK-1500) is the replacement. - startup/scrum/product templates: SeedItems lines removed. - post-init banner in printOnboardingHints: now points at "/pad onboard" in one line, then web UI link, then dashboard hint. The "use pad to get IDEA-1 / BACK-1 / FEAT-1" branch is gone. TASK-1502 (retire cobra + OnboardingPrimaryRef plumbing): - OnboardingPrimaryRef struct field on WorkspaceTemplate removed. The dashboard's banner auto-discovers seeds via item_number=1 + source="template" + created_by="system", so the field was redundant even before retirement. - onboardingPrimaryRef() helper in cmd/pad/main.go removed. - 'pad onboard' Cobra subcommand removed (~160 lines). It scanned the project directory for build/test/CI markers and seeded library conventions — useful behavior but CLI-only, unreachable from MCP-only agents. The /pad onboard PLAYBOOK now covers it. - internal/cli/detect.go and workspace_context_detect.go stay; still used by the web-side workspace-context save path. TASK-1503 (Blank in interactive picker): - The picker already surfaces Blank because templates_picker.go iterates GroupTemplatesByCategory, and the IDEA-1479 Blank template entry lives in CategoryCustom. Verified the output renders correctly with the TASK-1498 description + icon update. - 'pad workspace init --help' Long now mentions Blank explicitly + points users at /pad onboard. Helps discoverability without restructuring the picker. Test changes (delete or rewrite tests that exercised the retired pattern): - internal/collections/templates_test.go: six tests deleted (StartupOnboardingItemsOrderAndShape, ScrumOnboardingItemsOrderAndShape, ProductOnboardingItemsOrderAndShape, Startup/ScrumProduct/TemplatesDeclareOnboardingPrimaryRef). New TestSoftwareTemplatesShipNoSeedItems replaces them with the inverse invariant: software templates ship zero seed items. - internal/server/handlers_dashboard_test.go: three IDEA-1/BACK-1/FEAT-1 expectation tests collapsed into TestDashboardOnboardingSeed_NilForAllTemplates, which asserts the auto-discovery finds no seed because seeds no longer ship. (Hiring + EmptyWorkspace tests untouched — they already expect nil for unrelated reasons.) - internal/store/items_test.go: TestSeedCollectionsFromTemplate{Startup,Scrum,Product}RefSequence and TestOnboardingFlow_FullWalkthrough_{Startup,Scrum,Product} deleted; these locked the IDEA-1 ref-sequence + walkthrough behavior. Unused helpers (findItemByTitle, extractStatus, safeFields, setItemStatus, countItemsInCollection) deleted alongside them. - internal/mcp/resources_test.go: TestReadItem_PreservesIDEAOneOnboardingBodyVerbatim → TestReadItem_PreservesBodyVerbatim. Property is the same (resource pipeline doesn't mangle markdown), but the fixture is now synthetic markdown instead of the IDEA-1 seed. Note: handlers_dashboard.go still has the auto-discovery code path (onboardingPrimaryCollectionSlugs map + the loop that probes for item_number=1 + source="template"). It's now dead code — no item will ever match the criteria after this PR. Left in place for a follow-up cleanup pass to keep this PR focused. Parent: PLAN-1496. * docs: replace 'pad workspace onboard' references with /pad onboard (Codex round 1) P2 finding on PR #577: README + CLAUDE.md still advertise the 'pad workspace onboard' subcommand in four places (README §Onboard agents to a new codebase, README §3 Teach your agents the rules, README CLI Reference, CLAUDE.md CLI). After this branch lands, those instructions return "unknown command." Replaced each with guidance pointing at /pad onboard (the playbook, auto-seeded into every workspace). The library-list commands still work and stay where they are. Parent: PLAN-1496. * docs: replace 'use pad to get IDEA-1' guidance with /pad onboard (Codex round 2) P1 finding on PR #577: README.md:33-39 and CLAUDE.md:111-117 still told users to 'use pad to get IDEA-1' after the post-init banner. Since this branch deletes templates_onboarding.go and stops seeding IDEA-1/PLAN-2/TASK-3/DOC-4, the quickstart instructions in both top-level docs pointed at items that no longer exist. Replaced each with /pad onboard guidance (the playbook is auto-seeded into every new workspace by TASK-1500). CLAUDE.md's CLI reference gets a one-line historical note explaining the pre-PLAN-1496 IDEA-1 pattern so readers reviewing older code/blame have context. Parent: PLAN-1496. * docs(skill): retire 'use pad to get IDEA-1' guidance in agent skill (Codex round 3) P1 finding on PR #577: skills/pad/SKILL.md:175 still taught agents that '"use pad to get IDEA-1"' should dispatch to 'pad item show IDEA-1'. This branch deletes the seed items, so any agent following the shipped skill in a fresh workspace would try to fetch a missing ref instead of running /pad onboard. Updated the routing entry to dispatch the legacy phrasing (kept as a recognized intent so older docs/conversations still work) to the /pad onboard playbook. Explicit "do NOT try to fetch IDEA-1 directly" to short-circuit the previously-trained behavior. A broader skill cleanup — removing the standalone Onboarding workflow section and adding the bootstrap nudge rendering — is TASK-1505's scope. This PR's update is the minimal change needed to unbreak the agent-facing routing. Parent: PLAN-1496. * docs(skill): add library-activation caveat to onboard routing entry (round 4) P2 finding on PR #577: the routing entry said /pad onboard is 'always invokable because every workspace auto-seeds it.' True for newly-created workspaces, but pre-existing workspaces (created before PLAN-1496 lands) won't have it. Auto-upgrade is intentionally not wired into SeedCollectionsFromTemplate for empty-template-name paths. Mirrored the same activation-fallback caveat /pad plan and /pad decompose carry: 'activate via library if the bootstrap's playbooks array lacks invocation_slug=onboard, status=active.' Parent: PLAN-1496.	2026-05-17 13:40:15 -04:00
xarmian	507793e565	feat(playbooks): author canonical /pad onboard library playbook (TASK-1499) (#576) ... * feat(playbooks): author canonical /pad onboard library playbook (TASK-1499) The fourth invokable library playbook (alongside ship/plan/decompose). This is the workspace bootstrap interview the agent runs to turn a freshly-created workspace into one whose collections, conventions, playbooks, and roles actually match the user's project. Files: - internal/collections/playbook_library_onboard.go (new): - onboardPlaybookBody — surface-agnostic instruction set teaching the agent to ADAPT seeded artifacts, not curate from the library. Mode-aware: build (blank workspace), audit (templated workspace), revisit (already-onboarded), defaults (escape hatch). The body explicitly tells the agent to use pad_item/pad_collection/pad_role MCP actions OR pad CLI — never assumes a shell. Lean on the TASK-1510/1511/1512 mutation primitives shipped earlier in PLAN-1496. - onboardPlaybookArguments — mode (enum), defaults (flag), skip-codebase (flag). Mirrors the body's ## Arguments section for the strict CLI parser. - OnboardPlaybook() — LibraryPlaybook constructor. - internal/collections/playbook_library.go: register OnboardPlaybook() in the agent-workflows category alongside ship/plan/decompose. - internal/collections/playbook_library_test.go: - TestPlaybookLibrary_InvokableEntriesPresent now expects 4 invokable entries (was 3) and includes onboard in wantSlugs. - New TestOnboardPlaybook_Contract locks the design contract: invocation_slug=onboard, trigger=manual (compatible with the blank template's minimal vocab), mode/defaults/skip-codebase argument shape, and presence of the "ADAPT, DON'T CURATE" posture in the body. Design notes captured at top of playbook_library_onboard.go: 1. Adapt, don't curate — library entries are starting points, rewrite using the project's actual commands. 2. Surface-agnostic — describe intent, not specific CLI commands; pure MCP users must follow the same flow. 3. Mode-aware — blank/audit/revisit/defaults paths. 4. Confirmation before mutation. 5. Self-removing nudge — the playbook produces user-created items which clear the bootstrap onboarding flag (TASK-1504, separate). Parent: PLAN-1496. Unblocked by TASK-1497 + TASK-1510/1511/1512. * fix: auto-seed onboard playbook + correct CLI form in body (Codex round 1) Addresses two PR #576 findings: 1. P1 — folding TASK-1500 into this PR: without auto-seed, the library entry alone makes /pad onboard manually-activatable but not invokable on day one. Codex correctly flagged that the PR as originally drafted shipped a half-feature. Wiring (PLAN-1496 / TASK-1500): - OnboardSeedPlaybook() in playbook_library_onboard.go returns the playbook as a SeedPlaybook with status=active, trigger=manual, scope=all, invocation_slug=onboard, arguments=onboardPlaybookArguments. Body + args are shared with the library entry (same pattern ShipPlaybook uses for ship) so they cannot drift. - SeedCollectionsFromTemplate appends OnboardSeedPlaybook to EVERY workspace created with a non-empty templateName — blank, startup, scrum, product, hiring, interviewing, demo. The empty-templateName path is preserved as the explicit backward-compat escape hatch (tests + direct API callers that want a bare workspace with zero items). cmd/pad/init.go always supplies a non-empty template (interactive picker or defaultTemplateName), so real user-facing workspace creation always lands in the seeded branch. Tests: - TestSeedFromTemplateAlwaysIncludesOnboardPlaybook walks all six real templates and confirms the onboard playbook is seeded into each. - TestSeedWithEmptyTemplateNameSkipsOnboard locks the escape-hatch invariant. - TestSeedFromBlankTemplate updated: blank workspace now ships exactly one item (the onboard playbook) instead of zero, because that's TASK-1500's whole point. 2. P2 — the body referenced 'pad library list-conventions', which doesn't exist. Corrected to 'pad library list --type conventions' (the actual CLI form), with a parenthetical pointing MCP users at pad_meta.action: bootstrap for the same data. This PR now covers both TASK-1499 (author playbook) and TASK-1500 (auto-seed) — combining them because Codex's P1 made it clear they ship together or not at all. * docs: correct MCP library-browse fallback in onboard body (Codex round 2) P2 finding on PR #576: the body told MCP-only users to read the convention library via 'pad_meta.action: bootstrap'. Bootstrap returns workspace STATE (collections, conventions, playbooks actually present in the workspace), not the global library catalog. So MCP users following that instruction would see only what's already activated, not what they could activate. The honest answer is that there is no MCP library-browse surface today. Updated the body to say so explicitly: if the agent has a shell, use 'pad library list'; if not, work from domain knowledge and have the user paste any library bodies they want as starting text. Captured the underlying gap as IDEA-1514 (Expose library catalog via MCP) and linked from the playbook body. Three options outlined there: new pad_library tool, pad_meta.action: library, or embed in bootstrap. Parent: PLAN-1496.	2026-05-17 11:52:20 -04:00
xarmian	cc0b1c0bf3	feat(templates): finalize 'blank' template with minimal-vocab seeds for /pad onboard (TASK-1498) (#575) ... * feat(templates): finalize 'blank' template for /pad onboard flow (TASK-1498) A blank template entry was already present in templates.go (drafted for IDEA-1479) but its seeded trigger/scope vocabularies leaked the software domain — on-commit, on-pr-create, on-implement, etc., baked into a template whose whole point is being domain-agnostic. The /pad onboard playbook (PLAN-1496 / TASK-1499) needs a true blank starting point so the interview can broaden vocabulary to match the project's actual domain, whatever it is. This commit: - Replaces the software-flavored seed with minimal vocab: trigger= always for conventions, trigger=manual for playbooks, scope=all on both. The constants live in templates_blank.go so future tweaks to the seed surface have a focused diff. The agent broadens via pad collection update (TASK-1510) during onboarding. - Updates the template's description and icon to point at the onboard flow ("Empty workspace — run /pad onboard to build it out", sparkles instead of memo). - Adds an in-place comment explaining the design choice so the next reader doesn't re-leak software triggers into the seed. - New test: TestBlankTemplateUsesMinimalVocabularies locks the minimal-seed posture; any regression that adds domain-flavored triggers fails this test and triggers a fresh design conversation. Pre-existing IDEA-1479 tests (Shape, ExcludesSoftwareCollections, AppearsInPicker) still pass — the contract they describe is preserved (2 system collections only, no user-facing leaks, Custom group placement). Parent: PLAN-1496. * fix(test): blank-vocab assertions use literal slices, not the vars they came from (round 1) P3 finding on PR #575: TestBlankTemplateUsesMinimalVocabularies compared template output to BlankConventionTriggers / BlankPlaybookTriggers — the same vars used to build the template. Widening either var would silently widen the "minimal" definition and the test would still pass, defeating the drift-guard intent. Switched to literal expected slices. Now any change to the var that adds a domain trigger fails the test loudly.	2026-05-17 07:52:58 -04:00
xarmian	8c9974f6fb	feat(cli,mcp): expose 'role update' via CLI and MCP catalog (TASK-1512) (#574) ... * feat(cli,mcp): expose 'role update' via CLI and MCP catalog (TASK-1512) Third of three TASK-1497 capability-spike follow-ups (after #572 and #573). The handlers_agent_roles.go::handleUpdateAgentRole PATCH handler and the internal/cli/client.go::UpdateAgentRole HTTP client method already existed. Only the agent-facing surfaces were missing. - cmd/pad: new 'pad role update <slug-or-uuid>' Cobra subcommand with --name / --slug / --description / --icon / --tools / --sort-order flags. Uses cmd.Flags().Changed for omit-if-unset. Positional arg = lookup ref; --slug = new slug value (rename). Empty-string clears for description and icon (the store treats *string("") as "clear", matching collection update semantics). - internal/mcp/catalog_role: new 'update' action + supporting params (new_slug, sort_order). The catalog disambiguates lookup-slug (in path) from rename-target (in body) with the new_slug input, avoiding the conflated-semantics footgun. - internal/mcp/dispatch_http_routes: new mapRoleUpdate mapper. Path uses input.slug for the lookup; body's "slug" key is sourced from input.new_slug. String fields use key-presence semantics so empty-string clears round-trip to the store. - Tests cover canonical body (with AgentRoleUpdate round-trip), new_slug-to-body-slug mapping, empty-string clearing, and required-arg validation. - README.md + internal/mcp/instructions.md pad_role action lists updated to include "update". Pairs with TASK-1510 + TASK-1511 to complete the workspace-mutation trio the /pad onboard playbook (TASK-1499) needs to adapt seeded roles, collections, and schemas to each project's actual shape. Parent: PLAN-1496. * fix(cli,mcp): rename role-update flag --slug → --new-slug (Codex round 1) P1 finding on PR #574: pad_role.update via local stdio MCP was silently broken. BuildCLIArgs translates MCP property "slug" to the CLI's positional <slug> AND to the --slug flag (same key reused), so: pad_role.update slug=<uuid> → pad role update <uuid> --slug <uuid> → tries to rename the role's slug to the literal UUID. BAD. pad_role.update slug=implementer new_slug=engineer → pad role update implementer --slug implementer → new_slug ignored entirely, no rename. The HTTP dispatcher had the disambiguation right (mapRoleUpdate already mapped MCP new_slug → body slug). The CLI flag name was the problem. Renamed --slug to --new-slug. Now MCP "slug" maps to the positional only (lookup), and MCP "new_slug" maps to --new-slug (rename target). Both transports symmetric. Updated example in --help, the liveCmdhelpDoc fake, and the change-detect block. Parent: PLAN-1496, Codex round 1 on PR #574 / TASK-1512.	2026-05-17 02:33:36 -04:00
xarmian	f76520f6e7	feat(cli,mcp): expose 'collection delete' via CLI and MCP catalog (TASK-1511) (#573) ... * feat(cli,mcp): expose 'collection delete' via CLI and MCP catalog (TASK-1511) Mirrors TASK-1510 (collection update). The HTTP handler at handlers_collections.go::handleDeleteCollection already supported DELETE on a collection (owner-only, soft-deletes the collection and every item in it). Wires both agent-facing surfaces: - internal/cli/client.go: new DeleteCollection client method. - cmd/pad: new 'pad collection delete <slug>' Cobra subcommand (no --force; the help text is the confirmation contract). - internal/mcp/catalog_collection: 'delete' action passes through to the CLI; tool description updated. - internal/mcp/dispatch_http_routes: simple routeSpec entry for DELETE /api/v1/workspaces/{workspace}/collections/{slug}. No custom mapper needed — no body, no field coercion. Pairs with TASK-1510 as the second adaptation primitive for the /pad onboard playbook (TASK-1499): when the onboard interview discovers a seeded collection that doesn't fit the project, the agent now has a way to remove it before creating the right one. Tests: - TestRouteTable_CollectionDelete (route substitutes correctly) - catalog_readonly bijection + liveCmdhelpDoc fake updated. Parent: PLAN-1496. * docs: correct collection delete contract per Codex review (round 1) Two findings on PR #573 — both documentation, no code behavior change: 1. CLI Long help / Short blurb / MCP description claimed delete "removes seeded collections" and the onboard use case targets template-seeded collections. But store.DeleteCollection refuses any collection where is_default=true, and every template seed is is_default=true. The advertised use case wouldn't actually work. Updated docs to clarify: delete is for USER-CREATED collections; template seeds must be adapted via 'pad collection update'. 2. Both CLI help and MCP description claimed "AND every item in it" gets archived. The store delete path only sets collections.deleted_at and never touches items. The web UI hides them via the join, but raw API queries still surface them. Updated docs to be honest: items are NOT cascaded. Captured the underlying behavior limitation as a follow-up: IDEA-1513 ("Lift is_default restriction on collection delete or add a cascade-items option") — surfaces options 1-4 for lifting the guard plus the items-orphan issue. Parent: PLAN-1496, addressing Codex round 1 on PR #573 / TASK-1511. * docs: tighten collection delete contract per Codex review (round 2) Three P3 documentation-drift findings: 1. internal/cli/client.go::DeleteCollection Go doc still said "and all items in it" — missed it in round 1. Updated to describe the actual behavior (collections.deleted_at only; items orphaned with soft-deleted collection_id; is_default rejected). 2. CLI Long help and MCP description claimed restore is available "via the API," but there is no restore endpoint and no RestoreCollection client method. Recovery is database-backup only. Both docs updated. 3. catalog_collection.go:33 slug ParamDef only mentioned action=update; action=delete needs it too. And the headline description still said "list, create, and update" — three actions when there are now four. Both fixed. Parent: PLAN-1496, addressing Codex round 2 on PR #573 / TASK-1511. * docs: update pad_collection action lists in instructions.md + README (round 3) Codex round 3 finding: two top-level reference docs still advertised pad_collection as list/create only. internal/mcp/instructions.md is embedded into the MCP initialize() handshake instructions — stale guidance there means MCP clients miss update/delete entirely. README's catalog table had the same drift. Parent: PLAN-1496, Codex round 3 on PR #573 / TASK-1511.	2026-05-17 02:15:37 -04:00
xarmian	f5579300fb	feat(cli,mcp): expose 'collection update' via CLI and MCP catalog (TASK-1510) (#572) ... * feat(cli,mcp): expose 'collection update' via CLI and MCP catalog (TASK-1510) The HTTP handler at handlers_collections.go::handleUpdateCollection already supported PATCHing a collection's name, icon, description, prefix, schema, settings, and sort_order (plus field-value migrations). The CLI and MCP surfaces never exposed it, so agents couldn't rename collections, swap icons, or reshape schemas — a hard blocker for the adaptive /pad onboard playbook (TASK-1499) which needs to rewrite seeded collections to match each project's actual vocabulary. This wires both agent-facing surfaces to the existing handler: - cmd/pad: new 'pad collection update <slug>' Cobra subcommand with --name / --icon / --description / --prefix / --schema / --fields / --sort-order flags. Only flags explicitly set are sent (uses cmd.Flags().Changed); --schema and --fields reuse the existing collectionSchemaJSONFromFlags helper so DSL parity stays. - internal/mcp/catalog_collection: add 'update' action plus the slug, prefix, and sort_order params on padCollectionTool. - internal/mcp/dispatch_http_routes: new mapCollectionUpdate handles the schema-object-vs-string coercion. The catalog declares schema as a JSON object for MCP ergonomics, but models.CollectionUpdate.Schema is *string — and its UnmarshalJSON only flexes settings, not schema. The mapper re-marshals object input to its JSON-string form before sending, symmetric to what the CLI does via collectionSchemaJSONFromFlags. Tests cover canonical body, schema-object-to-string coercion (round-trip through CollectionUpdate.UnmarshalJSON), schema-string pass-through, empty-field omission, and required-arg validation. catalog_readonly_test bijection + liveCmdhelpDoc fake updated. Parent: PLAN-1496. * fix(mcp): collection update — clear-on-empty + fields DSL parity per Codex review (round 1) Addresses two P2 findings on PR #572: 1. The catalog advertises `icon=""` / `description=""` / `prefix=""` as clear-the-field, and the CLI flag help says the same, but the HTTP mapper filtered empty strings via `v != ""` — leaving MCP HTTP callers unable to clear fields the CLI can. Switched to key-presence semantics for the four string fields so explicit empty strings round-trip to the store (which honors *string("") as "clear"). 2. The catalog advertises `fields OR schema` as mutually exclusive (mirroring `pad collection create`), but the mapper only consumed `schema`. An MCP HTTP request with `fields=...` produced a `{}` PATCH body silently. Extracted the DSL parser to a shared package (internal/collections/dsl.go::ParseFieldsDSL + FieldsDSLToSchemaJSON) so the CLI and the mapper share one parser; mapper now resolves fields-or-schema with the same mutual-exclusion guard the CLI has. Tests added in dispatch_http_routes_extras_test.go: - TestMapCollectionUpdate_EmptyStringClearsField - TestMapCollectionUpdate_AcceptsFieldsDSL (round-trips through models.CollectionSchema to confirm the parsed shape) - TestMapCollectionUpdate_RejectsFieldsAndSchemaTogether cmd/pad/main.go's parseFieldsDSL becomes a one-line alias for collections.ParseFieldsDSL so the CLI's behavior stays identical. Parent: PLAN-1496, fixing PR #572 / TASK-1510. * fix(mcp): collection update — use encodeSchemaForBody + normalize empty schema (round 2) Addresses two more findings from Codex round 2 on PR #572: 1. P2: mapCollectionUpdate bypassed encodeSchemaForBody, so structured schemas didn't get label backfill and string schemas weren't validated before PATCH — diverged from collection create + CLI. Now reuses encodeSchemaForBody (the same encoder collection create uses at dispatch_http_routes.go:418), getting label-backfill via the Title-Case-of-key heuristic and shape validation for free. 2. P3: schema=null or schema="" plus a real fields=... update tripped the mutual-exclusion check. Now normalizes empty inputs as absent BEFORE checking exclusivity, matching the relaxed handling collection create has for optional empty params. Tests: - Renamed TestMapCollectionUpdate_PassesSchemaStringVerbatim to TestMapCollectionUpdate_AcceptsSchemaString — the new property is round-trip parity + label backfill, not verbatim pass-through. - New TestMapCollectionUpdate_EmptySchemaDoesNotBlockFields covers both nil and empty-string schema combined with a real fields value. Parent: PLAN-1496, addressing Codex round 2 on PR #572 / TASK-1510.	2026-05-17 01:45:40 -04:00
xarmian	e59d3904c9	feat(server): refuse to mark item terminal while it has open children (IDEA-1494) (#571) ... * feat(server): refuse to mark item terminal while it has open children (IDEA-1494) Server-side guard inside handleUpdateItem that rejects a non-terminal → terminal done-field transition when the item still has at least one non-terminal child. Returns HTTP 409 with code=open_children plus a structured details payload listing each blocking child's {ref, title, status, collection_slug} so MCP-driven agents can self-recover (ship the listed children, then retry) and the CLI can render the same list verbatim. Escape hatch: `--force` on `pad item update` / `pad item bulk-update` and `force: true` on the MCP pad_item.action: update / bulk-update inputs both forward into the same ItemUpdate.Force transport field the handler consumes before any store mutation. Trigger conditions are tight: the PATCH must change the done-field key (resolved via TerminalValuesForDoneField against the parent's schema + settings) AND the new value must be terminal AND the current value must NOT already be terminal. Terminal → terminal and no-op terminal transitions bypass the guard; only entering the terminal set is gated. Per-child evaluation uses the child's own collection schema so hierarchical workspaces with custom typed collections work without extra plumbing. Tests cover: rejection with one open child (with mutation-safety assertion on the parent), no children, all-terminal children, --force override, no-op terminal → terminal, terminal → terminal, non-terminal → non-terminal, custom collection terminal_options honored, and a parent task (not a plan) — IDEA-1494 optional extra #3. MCP coverage asserts --force round-trips through both ExecDispatcher and HTTPHandlerDispatcher and is omitted when force=false. * fix(server): open-children guard round 2 — visibility, MCP pass-through, TOCTOU (IDEA-1494) Three Codex round-1 issues, each fixed with the recommended shape: P1 — visibility leak. The 409 response previously listed every blocking child by ref/title/status, including children in collections the caller couldn't see. The INVARIANT still evaluates against ALL children (it's a data-integrity gate — a restricted user must not be able to close a parent whose blockers they can't see), but the response payload now filters to caller-visible children only. Hidden blockers surface as a new `details.hidden_blocker_count` field plus an alternate human message when every blocker is hidden ("blocked by N open children you don't have access to"). Mirrors the visibility helpers (`visibleCollectionIDs` + `isItemVisibleToGuest`) used by the per-parent progress endpoint so the two paths can't drift. P2 — MCP code/details pass-through. The HTTP classifier was collapsing 409 into the generic `conflict` code and dropping `details`; the stdio classifier was matching the human "cannot " message against the validation regex and surfacing `validation_failed`. Both now surface `open_children` with the structured details intact: - HTTP: classifyHTTPStatusKind's 409 branch extracts the upstream code; any non-empty, non-"conflict" code is passed through with its `details` RawMessage. Generalizes beyond open_children — any future structured 409 from a handler gets the same treatment. - Stdio: the CLI writes a `pad-error: {json}\n` marker line on stderr before the human-readable block (single source of truth for both views), and classifyExecError detects the marker and lifts the envelope verbatim. Marker is duplicated as a const between internal/cli and internal/mcp to avoid pulling the cli package into the classifier just for one string. A new ErrOpenChildren error code constant + `Details json.RawMessage` field on ErrorPayload back the wire shape. P2 — TOCTOU. The guard previously ran in the handler before the store transaction began; a concurrent child insert / child status flip could slip between the children-list read and the parent's UPDATE. Fix: - New `Store.UpdateItemWithPreCheck(id, input, precheck)` runs the caller's invariant check inside the same tx, after acquiring the workspace seq lock AND a new parent-children advisory lock keyed on the parent ID. UpdateItem is now a thin wrapper passing nil. - Every UpdateItem unconditionally acquires the parent-children advisory lock for its own parent (if any) AND for itself-as-parent, in a fixed order (parent first) so two updaters touching the same parent always grab that key before the more-specific one — no AB/BA deadlock. - New `GetChildItemsTx` reads via the caller's tx; on Postgres the advisory lock provides the snapshot guarantee (DISTINCT precludes `FOR UPDATE`), on SQLite the global BEGIN IMMEDIATE write lock serializes all writers. - Handler now passes a precheck closure into UpdateItemWithPreCheck at all three call sites (collab-snapshot path, applier-direct-write path, main path). The guard's openChildrenGuardError sentinel is unwrapped after each call so the 409 surfaces cleanly. Tests: - TestOpenChildrenGuard_VisibilitySanitizesPayload — restricted editor sees parent + visible child, hidden child contributes to hidden_blocker_count, no leak of ref/title/slug. - TestOpenChildrenGuard_AllBlockersHiddenSurfaceGenericMessage — open_children=[], hidden_blocker_count>0, message mentions "you don't have access to." - TestOpenChildrenGuard_TOCTOURace — 8 iterations of a child-flip racing a parent-terminal update; asserts the forbidden outcome (parent=completed AND child=open) never occurs. - TestClassifyHTTPStatus_OpenChildrenPreservesCodeAndDetails + inverse generic-409 test. - TestClassifyExecError_OpenChildrenMarkerLiftsStructuredPayload + no-marker-falls-through inverse. * fix(server): open-children guard round 3 — 7 Codex findings closed (IDEA-1494) P1 — visibility fail-closed. The handler was swallowing visibleCollectionIDs errors, leaving visIDs==nil which the guard treats as unrestricted, leaking hidden-child metadata. Now surfaces the error as 500 BEFORE installing the precheck. Test: TestOpenChildrenGuard_VisibilityLookupErrorFailsClosed closes the store DB and asserts no 409+children leak. P1 — link mutations acquire the advisory lock. SetParentLink, ClearParentLink, CreateItemLink (when link_type ∈ childLinkTypes via new isChildLinkType helper), DeleteItemLink (same condition), and RestoreItem now take `pad:parent-children:<id>` in canonical sorted order via new AcquireParentChildrenLocks helper. SetParentLink locks BOTH old and new parents (re-parenting case). Race test TestOpenChildrenGuard_LinkMutationRace asserts the forbidden "link-committed-before-parent-flip AND parent flip succeeded" never occurs by comparing link.created_at to parent.updated_at. Documented semantics: status-wins + link-after-commit is legal under the invariant "no open children EXIST AT THE MOMENT of transition" — the post-condition variant ("no open child may EVER attach to a terminal parent") is intentionally deferred. P1 — MoveItem bypass closed. New MoveItemWithPreCheck mirrors UpdateItemWithPreCheck — acquires workspace seq lock + parent-children locks, re-reads in tx, runs caller precheck. handleMoveItem builds the same guard closure using the DESTINATION schema for done-field resolution (conservative — honors the schema the item moves INTO). CLI gains `pad item move --force`, client gains MoveItemWithForce that appends `?force=true` to the move endpoint. MCP catalog + mapItemMove forward `force` through the route mapper. Tests: TestOpenChildrenGuard_MoveItem_RejectsTerminalWithOpenChildren and …_ForceOverrides. P2 — pre-tx field-read TOCTOU. UpdateItemWithPreCheck and MoveItemWithPreCheck now re-read the item via new getItemTx INSIDE the tx (after locks) and pass that fresh snapshot to the precheck closure; the precheck classifies the transition against the in-tx view, not the handler-side pre-tx capture. Handler precheck closure swaps `currentFieldsJS` from the in-tx snapshot. Test: TestOpenChildrenGuard_PrecheckReadsInTxSnapshot stages a between-load status mutation and asserts the precheck observes the post-mutation fields. P2 — bulk-update carries structured errors. cmd/pad/main.go's updateFailure struct extended with Code + Details (json.RawMessage). When client.UpdateItem returns *cli.APIError, the row preserves the structured envelope. Human-text output also renders the open-children list inline. Chose JSON-envelope route over per-row stderr markers because bulk-update already produces a structured envelope and ExecDispatcher returns stdout verbatim on exit-0 — no classifier change needed. Test: TestBulkUpdateStructuredFailuresCarryOpenChildrenDetails confirms the wire shape the CLI lifts. P3 — marker hardening. Marker bumped to versioned form `pad-structured-error/v1:` (was `pad-error:`). cli.StructuredErrorMarker + mcp.structuredErrorMarker kept in lockstep with cross-references. mcp.allowedStructuredErrorCodes whitelists known codes (currently just open_children); unknown codes fall back to regex classification. Marker must start the line after whitespace trim (embedded markers ignored). Last-marker-wins to defeat pre-emption attacks. Tests: TestClassifyExecError_{UnknownStructuredCode,OldMarkerVersion, MarkerEmbeddedMidLine,LastMarker}. P3 — soft-deleted collection schemas honored. New GetCollectionAnyState mirrors childrenDoneFiltersForParent's inclusion rule; guard uses it so a child still attached to a soft-deleted collection is evaluated against ITS schema (custom terminal_options) instead of the default- status fallback (which would mis-classify and false-block). Test: TestOpenChildrenGuard_SoftDeletedCollectionSchemaHonored seeds a custom collection, soft-deletes it while a child remains, and asserts the terminal status is correctly recognized. Comprehensive store-mutation audit results recorded in the PR description (every method touching items.fields / items.collection_id or item_links). * fix(server): open-children guard round 4 — multi-parent locks, enum parity, PATCH atomicity (IDEA-1494) Four Codex round-3 (blast-radius lens) findings, each fixed with the recommended shape. P1 — multi-parent lock set. acquireParentChildrenLocksForUpdate and RestoreItem previously used `LIMIT 1` against item_links, so a child with BOTH a `parent` link to P1 AND an `implements` link to P2 only locked one of them. The other parent's open-children precheck could race against the child's status flip and miss it. Fix: new listParentChildLockKeys helper runs the same query GetChildItems' inclusion rule uses (childLinkTypes), returns ALL distinct parent target_ids, and feeds them into the canonical multi-lock helper. Both UpdateItemWithPreCheck and RestoreItem now acquire locks on {self} ∪ {all-parents-via-childLinkTypes}. Test: TestOpenChildrenGuard_MultiParentChildLocksAll races a child status flip against terminal-updates on both parents simultaneously. P2 — lock-order asymmetry. The pre-fix codebase had multiple lock- acquisition shapes: parent-then-self in acquireParentChildrenLocksForUpdate, single-key in RestoreItem / CreateItemLink / DeleteItemLink / ClearParentLink, and a sorted multi-key in SetParentLink. Two concurrent callers using different ad-hoc orderings could AB/BA deadlock. Fix: removed the per-call-site AcquireParentChildrenLock helper entirely. Every site now goes through AcquireParentChildrenLocks (the canonical sorted multi-lock helper) — including ones that need only one ID (the variadic call still sorts a one-element slice). The helper's doc comment explicitly states the contract: "Ad-hoc single-key acquisition outside this helper is FORBIDDEN — two call sites taking distinct keys in different orders WILL deadlock." Test: TestOpenChildrenGuard_NoDeadlockUnderReverseOrderConcurrency runs reverse-order re-parents with a 5-second timeout; assertion fails on hang. P2 — HTTP/stdio code-surface parity. Round 2's HTTP pass-through ("any non-conflict upstream code") silently widened the ErrorCode enum beyond stdio's allow-list (`open_children` only). Agents saw different code surfaces depending on which dispatcher delivered the response. Fix: HTTP 409 branch in classifyHTTPStatusKind now consults the same allowedStructuredErrorCodes whitelist stdio does. Codes outside the set collapse to ErrConflict (no details), matching what stdio does for an unknown-code structured marker. Doc on allowedStructuredErrorCodes updated to make the dual-consumer contract explicit: "Adding a new structured code is a TWO-WAY change." Tests: TestClassifyHTTPStatus_UnknownConflictCodeFallsBackToErrConflict and TestStructuredErrorCodeParityAcrossTransports. P3 — PATCH atomicity. A combined PATCH with `parent` + `status=terminal` on an item with open children used to commit the parent-link change INLINE (before the guard ran) and then reject the field write. Caller saw 409 but the parent had already moved. Fix: parent-link mutation is now DEFERRED — captured into outer- scope vars during fields validation, executed AFTER UpdateItemWithPreCheck succeeds. A guard rejection returns before the link write block, so on rejection the link is untouched. Documented choice: "reorder, don't tx-wrap" — wrapping SetParentLink into the same store tx would require threading a *sql.Tx through the SetParentLink API (which is also called from the handler_item_links path); reordering is the smaller surgery and gives the correct outcome on the failure direction. A residual window remains in the OTHER direction (field write commits, link write fails) — not made worse by the reorder, and called out inline for a future tx-wrap pass. Test: TestOpenChildrenGuard_PatchAtomicRejectionPreservesParentLink sets up target → oldParent → openChild, sends PATCH {parent=newParent, status=completed}, asserts 409 AND target.parent_link still points at oldParent. * fix(server): open-children guard — emit details.open_children as [] not null on hidden-only rejection (IDEA-1494)	2026-05-17 00:15:52 -04:00
xarmian	9ebdfb503e	Revert "feat(cli): pad session shape — Claude Code context-window telemetry (IDEA-1491) (#569)" (#570) ... This reverts commit 351f83af3f.	2026-05-16 21:12:40 -04:00
xarmian	351f83af3f	feat(cli): pad session shape — Claude Code context-window telemetry (IDEA-1491) (#569) ... * feat(cli): pad session shape — Claude Code context-window telemetry (IDEA-1491) New `pad session shape [--session <id|path>] [--format json|table|markdown]` command that reads the active Claude Code session JSONL and reports tokens, context_pct (vs hardcoded per-agent-version budget), message counts, and elapsed time. Default format is JSON because agents are the primary caller. - internal/cli/claudecode.go: project-slug derivation, JSONL streaming parser, env/cwd/autodetect cascade resolver, per-version budget table (seeded with 2.1.* → 1M tokens per the IDEA-1491 recon update). - cmd/pad/session.go: top-level `session` group + `shape` subcommand, three output formats, registered via cmd/pad/main.go's rootCmd. - Tests cover slug derivation (live ~/.claude/projects/ verified cases), JSONL parse (normal / no-usage / sidechain fixtures), budget lookup, context-class bucketing, and the resolver cascade in t.TempDir(). Sidechain/sub-agent JSONL summing and the IDEA-body Pad-invocation-count fallback are intentionally deferred to v2 (TODOs in session.go). * fix(cli): session shape — TotalPrompt as context numerator + count parallel tool_use (IDEA-1491) Codex R1 review findings: P1 — context_pct numerator was CacheRead, which is a steady-state proxy that under-counts at turn boundaries when fresh content sits in cache_creation/input before being folded into the cached prefix. The correct denominator-of-budget is the full prompt footprint sent this turn: cache_read + cache_creation + input = TotalPrompt. Markdown renderer's context-tokens line follows suit; the explicit per-component breakdown rows keep CacheRead so the components remain visible. P2 — ToolInvocations was counting assistant-turns-with-any-tool-use, not tool invocations. A single assistant turn can emit multiple parallel tool_use blocks in message.content[]; the field name promises a count of invocations. Drop the early break. Test fixture normal.jsonl gains a 3-parallel-tool-use final turn; assert ToolInvocations==4 (up from 2) to pin the new behavior. * fix(cli): session shape — path-traversal, oversized-line, content-variant, explicit-flag errors (IDEA-1491) Codex R2 review findings: P1 — session-ID inputs (--session flag-id branch AND $CLAUDE_CODE_SESSION_ID env var) are now validated before they become filename fragments under ~/.claude/projects/<slug>/. Reject path separators, '..' segments, and anything that doesn't match a UUID-ish shape (hex+dashes, 8+ chars). New ErrInvalidSessionID wraps a descriptive message. Without this, `--session ../../../../../tmp/foo` or a poisoned env var could escape the projects dir on the candidate os.Stat. End-to-end smoke confirms `pad session shape --session '../../../etc/passwd'` now errors with "invalid session id: ... contains a path separator" and exits non-zero. P2a — Switch the JSONL line reader from bufio.Scanner (8 MiB cap, hard fail on overflow with "token too long") to bufio.Reader.ReadBytes('\n') (no cap). encoding/json has no size limit either, so oversized records — e.g. inline file attachments — parse cleanly. Both ParseSessionJSONL and tailLineCWD updated for parity. P2b — jsonlLine.Message.Content was []json.RawMessage at the outer decode, which made a schema variant where content is a string or object fail the WHOLE line's decode, losing type/timestamp/version/ usage data. Now Content is a raw json.RawMessage; the tool-use scan re-decodes it as []json.RawMessage on a best-effort basis and skips tool-counting when that fails, while preserving the rest of the line. P3 — `pad session shape --session <id|path>` no longer silently falls back when the resolver errors. An explicit flag means the caller has a specific session in mind; a typo or wrong UUID should fail loudly so automation surfaces the bug instead of emitting `agent: "unknown"`. The implicit (no-flag) path still falls back for non-Claude-Code harnesses. Tests added/extended: - TestResolveSessionLog_RejectsPathTraversal — flag-id AND env-id branches, full bad-input matrix. - TestParseSessionJSONL_OversizedLine — 10 MiB single record. - TestParseSessionJSONL_NonArrayContent — content-as-string variant must still contribute timestamp/version/usage. - TestParseSessionJSONL_ParallelToolUse — tight hermetic check of the R1 P2 multi-tool-per-turn fix. - TestBuildSessionShape_ExplicitFlagErrors — verify --session errors propagate. - TestBuildSessionShape_ImplicitFallback — verify implicit path still falls back.	2026-05-16 20:18:37 -04:00
xarmian	8094883869	feat(links): cross-workspace wiki-link resolution (IDEA-1492) (#568) ... * feat(links): cross-workspace wiki-link resolution (IDEA-1492) Adds [[workspace::REF]] and [[workspace::REF|Display]] wiki-link syntax that resolves cross-workspace, plus a Go route GET /{username}/{workspace}/ref/{REF} that 302-redirects to the canonical item URL. 404 leaks no info about workspace existence — malformed refs short-circuit before the workspace lookup, and access-denied returns 404 not 403. Frontend (web/src/lib/utils/markdown.ts): - renderMarkdown recognizes the workspace-prefix form and emits cross-workspace anchors (class doc-link cross-workspace) pointing at the resolver route. Same-workspace prefix is stripped and behaves identically to the legacy [[REF]] form. - wikiLinksToMarkdown emits the resolver URL for cross-workspace storage and same-workspace items resolve through the in-memory list. - markdownToWikiLinks rolls /<user?>/<ws>/ref/<REF> URLs back to [[workspace::REF]] (or |Display when display text differs from the ref). Legacy same-workspace round-trip is preserved. Backend (internal/server/handlers_ref_resolver.go): - Validates the ref shape before the DB hit (no oracle). - Reuses resolveWorkspace + GetItemByRef for ACL + lookup. - refResolverItemVisible mirrors requireItemVisible without depending on RequireWorkspaceAccess middleware (this route is reachable outside the workspace-scoped route group). - Redirect target matches itemUrlId() so the post-redirect URL is indistinguishable from a direct in-app navigation. Tests: 302 success, 404 unknown-workspace, 404 unknown-ref, 404 on a matrix of malformed refs (including url-encoded traversal). The no-access matrix is partially covered — the existing test surface doesn't compose a multi-user ACL fixture, so production-grade "member of A probes B" is gated by the real auth middleware stack and documented in TestRefResolver_NoAccess_DocumentsPreSetupBypass. * fix(links): codex round-1 fixes for cross-workspace resolver (IDEA-1492) P1.1 — Reserve "ref" as a collection slug. A collection slug of "ref" would shadow every item URL under the resolver's /{u}/{ws}/ref/... route. Added to reservedCollectionSlugs in internal/store/collections.go so it auto-suffixes to "ref-collection", matching the existing treatment of settings/activity/roles/etc. P1.2 — Extract checkItemVisible as a context-free helper. The previous refResolverItemVisible silently diverged from requireItemVisible by ignoring direct collection grants and member_collection_access for restricted members — a member with "specific" access on collection A plus a direct grant on collection B would 404 through the resolver even though they could see the item via the API. checkItemVisible now replays the same rules requireItemVisible inlined; requireItemVisible is now a thin wrapper, and the resolver derives its workspace role via resolverWorkspaceRole and delegates to checkItemVisible. Drift between the two paths is structurally impossible. P2.1 — Cross-workspace round-trip preserves explicit display overrides. The strip condition was `displayText === ref`, which would drop the override on [[other::TASK-1|TASK-1]] — then re-rendering would emit the default `other::TASK-1` and silently change visible link text. Fixed: only strip when displayText matches the actual render default `${ws}::${ref}`. P2.2 — Two-segment route /{workspace}/ref/{REF}. TimelineCommentCard and CommentThread call renderMarkdown without a username, so links in timeline comments emit the two-segment href shape. Registered the shorter route against the same handler; when the URL-path username is absent the handler falls back to the workspace owner's username via a new resolverOwnerUsername helper. Sanity sweep: - Dropped refItoa wrapper; use strconv.Itoa directly. The non-test `itoa` collision was a test-only `itoa` in handlers_admin_users_test.go, not a real symbol in non-test builds. - Removed encodeURIComponent on workspace + ref in renderMarkdown. parseCrossWorkspaceBody validates both against URL-safe regexes, and wikiLinksToMarkdown doesn't encode — both functions now emit identical bytes. Tests: - TestRefResolver_RejectsRefAsCollectionSlug — pins the reservation. - TestRefResolver_TwoSegmentRouteResolves — both URL shapes resolve; two-seg synthesizes the owner username. - TestRefResolver_RestrictedMemberWithCollectionGrant — the codex-flagged ACL case (restricted member + collection grant on a different collection) now resolves to 302, not 404. Frontend test for the round-trip override fix is documented as a gap (no vitest infra in the repo). * fix(links): codex round-2 fixes for cross-workspace resolver (IDEA-1492) P1.1 — Tokenized roles bypass user-nil check. Pre-fix, checkItemVisible rejected (nil user, "editor") tuples — exactly what RequireWorkspaceAccess synthesizes for legacy workspace-scoped API tokens — false-404'ing every requireItemVisible-gated handler hit by those tokens. Reordered the checkItemVisible rules so any tokenized role (owner / editor) bypasses the user-nil guard. checkItemVisible regression test (no HTTP layer) pins the bypass. P1.2 — System collections folded into item-grants branch. The pre-round-1 guestResourceFilterCore unioned ListSystemCollectionIDs into the fullCollIDs set; the round-1 refactor dropped that union, so a restricted member with conventions/playbooks (system collection) access plus an unrelated item grant could LIST system items but 404 on detail-fetch / ref-resolve. Restored the union inside checkItemVisible's item-grants branch (non-guest path only — matches the original guestResourceFilterCore semantics). P1.3 — Empty owner-username 404s instead of emitting broken redirect. When the workspace owner has no username on file (pre-setup ownerless workspaces, legacy accounts), the synthesized redirect target became `"/" + "" + "/" + slug + ...` → `//slug/...` — a protocol-relative URL browsers interpret as a network-path reference. Now 404s via refResolverNotFound rather than emitting the malformed Location header. P1.4 — URL shape changed to /-/r/{workspace}/{ref} (Option B). Pre-fix, the resolver lived at /{username}/{workspace}/ref/{ref}, which would intercept item URLs in workspaces with pre-existing `ref`-slugged collections (upgraded data; the round-1 reservation only blocks NEW creates). Picked Option B over a migration because the feature is unshipped, the new shape is more defensive (no future risk under any collection slug), and the only cost is the frontend emit-shape change. The leading `/-/r/` prefix can never collide with a user-namespace URL because username + slug grammar both require letter-led. Frontend renderMarkdown, wikiLinksToMarkdown, and markdownToWikiLinks all emit and parse the new shape; the round-1 collection-slug reservation stays as defense in depth. Tests: - TestCheckItemVisible_TokenizedRoleAllowsNilUser — P1.1 regression. - TestRefResolver_RestrictedMemberWithSystemCollection — P1.2 regression. - TestRefResolver_PreSetupBypass — P1.3 (ownerless workspace returns 404, not a broken `//slug/...` redirect). - TestRefResolver_URLShapeNonOverlap — P1.4 (resolver doesn't intercept `/{user}/{ws}/ref/{slug}` URLs). - Existing TestRefResolver_* updated to the /-/r/ shape; the previous two-segment fallback test is removed (the new URL shape has no username component, so there's no two-segment vs three-segment distinction to test). * fix(links): scope round-2 bypass to nil user (Codex round-3 P1) Round-2's checkItemVisible bypass for role in {"owner", "editor"} fired unconditionally, including for real authenticated members. Result: a member with workspace role "editor" and collection_access="specific" short-circuited the per-collection filter — they could GET/PATCH/DELETE items in collections their member_collection_access list excluded. Scoped the bypass to the tokenized-nil-user case only: if user == nil && (role == "owner" || role == "editor") This is the exact set the bypass was supposed to address — fresh-install mode (UserCount==0, role="owner") and legacy workspace-scoped API tokens (tokenWorkspaceID matches, role="editor"). Both paths set currentUser to nil; both are authorized by RequireWorkspaceAccess before checkItemVisible runs. Real authenticated members with the same roles now correctly fall through to the existing per-collection visibility filter. Workspace owners with default access still pass via the rule-4 "all access" short-circuit (member.CollectionAccess == "all"); restricted editors are now gated as intended. Updated the rule-1 doc comment to make the scope-to-nil-user discipline explicit — the prior wording conflated the tokenized and authenticated paths, which is what led to the over-broad bypass. Test: TestCheckItemVisible_AuthenticatedEditorWithRestrictedAccess seeds a real editor with collection_access="specific" granting only collection A, asserts visibility on a collection-B item returns false, and adds a sanity assertion that the same editor sees collection-A items. The existing TestCheckItemVisible_TokenizedRoleAllowsNilUser still passes — it covers the (nil, "editor") tuple the corrected bypass still allows. Direct callers of checkItemVisible (grep): only requireItemVisible (server.go) and resolverItemVisible (handlers_ref_resolver.go). Both pass real (user, role) from request context, so the narrower scope doesn't break any prior-green path. * fix(links): allow digit-leading workspace slugs in xw wiki-links Frontend WORKSPACE_SLUG_PATTERN was tighter than store.slugify (the canonical rule): slugify keeps digit-leading inputs (e.g. "2026 Roadmap" → "2026-roadmap") but the frontend regex rejected them. Effect: `[[2026-roadmap::TASK-1]]` fell through as a legacy title link, and `/-/r/2026-roadmap/TASK-1` URLs didn't round-trip back to wiki syntax. Two regex hunks, no behavior change beyond accepting the digit-led case: - WORKSPACE_SLUG_PATTERN: ^[a-z][a-z0-9-]*$ → ^[a-z0-9][a-z0-9-]*$ - markdownToWikiLinks reverse-regex workspace class: same widening Stale doc-comment citing the old pattern updated to match. Collection-slug grammar stays letter-led (the upstream rule differs; only workspace slugs accept digit-led). Only functional consumer of WORKSPACE_SLUG_PATTERN is parseCrossWorkspaceBody, which uses the match boolean — no other downstream code relied on the leading-letter constraint (Codex round-4).	2026-05-16 18:20:17 -04:00
xarmian	713421670a	feat(store): corrective backfill for collections.settings shape violations (IDEA-1489) (#567) ... * feat(store): corrective backfill for collections.settings shape violations (IDEA-1489) Migration 055 / pgmigrations/034 (PR #562) hardened collections.settings to NOT NULL DEFAULT '{}' but its backfill only matched `settings IS NULL`. Pre-existing rows with wrong-shape settings (empty string, JSON "null" literal, "[]" array, non-JSON garbage on SQLite; JSONB null, arrays, primitives on Postgres) survived the filter — NOT NULL satisfied but the IDEA-1484 contract (settings is always a JSON object) violated. PR #566 established the per-driver `json_valid()` / `jsonb_typeof()` widening pattern for its own four migrations (056, 057, pg/035, pg/036) but was scope-bounded out of retroactively repairing 055 / pg/034. This commit closes that gap. - migrations/058_collections_settings_shape_repair.sql: UPDATE collections SET settings = '{}' WHERE settings IS NULL OR json_valid(settings) = 0 OR json_type(settings) != 'object'; - pgmigrations/037_collections_settings_shape_repair.sql: UPDATE collections SET settings = '{}'::jsonb WHERE settings IS NULL OR jsonb_typeof(settings) != 'object'; - internal/store/collections_settings_shape_repair_test.go: TestCollectionsSettingsShapeRepair_SQLite seeds rows with every observable pre-055 pathology, applies 055-058, asserts each is repaired to '{}' and valid rows are preserved. TestCollectionsSettingsShapeRepair_Postgres seeds JSONB-valid-but- wrong-shape rows (NULL is unseedable post-pg/034) and asserts the pg/037 UPDATE clause repairs them. Toggle-verified: with the WHERE predicate reverted to `IS NULL` only, both tests fail on the non-NULL malformed seeds; with the full predicate both pass. Full ./... suite green on SQLite and Postgres. * chore: gofmt comment in shape repair test	2026-05-16 11:58:56 -04:00
xarmian	ec71903be7	feat(store): JSONB NOT NULL hardening on items/views + handler shape validation (IDEA-1486+1488) (#566) ... * feat(store): NOT NULL hardening on items.fields/tags + views.config (IDEA-1486) Paired ship of IDEA-1486 (sibling-table JSONB NOT NULL hardening) and IDEA-1488 (handler-layer shape validation for ViewUpdate/CollectionUpdate). Generalizes the IDEA-1484 / collections.settings precedent (PR #562) to the remaining nullable JSON columns and closes the shape-validation gap that NOT NULL alone doesn't cover. Schema layer (IDEA-1486 floor): - migrations/056_items_jsonb_not_null.sql: rebuild items with fields TEXT NOT NULL DEFAULT '{}' and tags TEXT NOT NULL DEFAULT '[]', preserving all 7 indexes, recreating the 3 items_fts triggers, and rebuilding the FTS5 index. Foreign-keys-off / on bookends are lifted outside the IDEA-1485 atomic-tx wrapper. - migrations/057_views_config_not_null.sql: rebuild views with config TEXT NOT NULL DEFAULT '{}'. - pgmigrations/035 + 036: SET NOT NULL + SET DEFAULT on the three JSONB columns. Split per-table to mirror the SQLite per-table file granularity. Store layer (IDEA-1486 floor): - items.go UpdateItem and views.go UpdateView normalize "" -> "{}" / "[]" before writing. Same boundary pattern as CreateItem and the IDEA-1484 precedent at collections.go:248. - export.go ImportWorkspace coerces empty-string AND malformed JSON at import time on items.fields, items.tags, and collections.settings. Malformed input is coerce-and-log via slog.Warn (length only, never raw value) so legacy bundles don't fail-stop on one bad row. - remapFieldIDs early-returns "{}" on empty input so the second-pass UPDATE can't write "" verbatim. - Migrated the existing fmt.Printf at export.go:329 to slog.Warn for consistency. Handler layer (IDEA-1488 ceiling): - ViewCreate / ViewUpdate UnmarshalJSON via flexJSONToString with new ErrInvalidConfigType sentinel. - CollectionCreate / CollectionUpdate UnmarshalJSON with new ErrInvalidSettingsType sentinel. - handlers_views.go and handlers_collections.go surface both sentinels as 400 with the domain-level message (mirrors the BUG-1144 precedent at handlers_items.go:641). Tests: - internal/store/items_views_jsonb_test.go: store-coercion + import coercion + log-and-coerce-on-malformed + SQLite schema introspection (7 indexes + 3 FTS triggers + items_fts virtual table survival) + Postgres NOT NULL enforcement + migration re-apply idempotency + item_links round-trip after rebuild. - internal/server/handlers_views_collections_jsonb_test.go: PATCH/POST flexible-shape coverage for views.config and collections.settings, including domain-level 400 message assertions that the response does not leak Go unmarshal internals. Refs: IDEA-1486, IDEA-1488, IDEA-1484 (precedent), IDEA-1485 (substrate). * fix(store,models): codex R1 follow-ups for IDEA-1486 / IDEA-1488 Three concrete defects surfaced by codex R1 against the initial paired ship. All three close holes that defeated parts of the original contract. P1.1: migration 056 missed the playbook invocation_slug unique index. - migrations/056_items_jsonb_not_null.sql: recreate the partial UNIQUE index idx_items_invocation_slug_per_collection from migration 054 verbatim after the other 7 indexes. Without it, the application-layer pre-check in handlers_items.go:checkUniqueFields would be a TOCTOU race with no DB-level guard — the original index that 054 explicitly added as the actual uniqueness backstop would be silently dropped during the items rebuild. - items_views_jsonb_test.go: the schema-introspection test now asserts 8 indexes, not 7. Verified via `grep -rn "ON items(" migrations/` that no other items-touching indexes were missed. P1.2: flexJSONToString didn't validate inner content of JSON-encoded strings. Pre-fix, `{"config": "[]"}` / `{"settings": "not json"}` / `{"fields": "[]"}` / `{"tags": "{}"}` slipped past the shape validators because the `case '"'` branch unmarshalled the envelope and returned the inner string verbatim — bypassing the whole point of IDEA-1488. - models/item.go: after unmarshalling the JSON-encoded string, validate that the trimmed inner content's first byte matches expectedStart ('{' / '[') AND parses as JSON. Empty inner strings still pass through to the store-layer empty-string coercion (IDEA-1486 floor), so legacy "" → default normalization is preserved. - The pre-existing ItemUpdate fields/tags path inherits the same tightening because it routes through this helper — covered by new test file handlers_items_jsonb_inner_shape_test.go. - Parallel handler tests for views.config and collections.settings added to handlers_views_collections_jsonb_test.go. P2: coerceJSONForImport accepted JSON null as well-formed. - store/export.go: json.Unmarshal("null", &m) returns err=nil with m staying nil; the prior code returned the raw "null" string verbatim, which lands as JSONB null on Postgres (satisfies NOT NULL since SQL NULL ≠ JSONB null) or text "null" on SQLite. The non-nil check on the unmarshalled value routes JSON null to the existing log-and-coerce path with the rest of the malformed shapes. - items_views_jsonb_test.go: extended import test with an item carrying fields=null / tags=null; expects both coerced to "{}" / "[]" and the structured slog.Warn emitted. Verified: make test (SQLite) and the full ./... suite against the existing port-5445 Postgres container both pass cleanly. Refs: IDEA-1486, IDEA-1488, codex R1 review. * fix(store,server): codex R2 follow-ups for IDEA-1486 / IDEA-1488 Two defects surfaced by codex R2. P1 is a real ship-breaker; P2 closes a parity gap that R1 missed. P1: migration backfill normalized only SQL NULL, not malformed/wrong- shape JSON. The four new migrations originally wrote `WHERE x IS NULL`. Rows with fields = '' / 'null' / '[]' / 'not json' all survived the filter, then violated the post-migration NOT NULL+shape contract. Concrete ship- breaker on SQLite: 056 recreates the partial UNIQUE index on json_extract(fields, '$.invocation_slug') from migration 054, and json_extract errors on rows whose fields fails json_valid — a single bad row breaks CREATE INDEX mid-migration. Toggle-verified: with the NULL-only WHERE, the new SQLite test fails at exactly that CREATE INDEX with "SQL logic error: malformed JSON (1)". Widened the backfill clauses: - migrations/056: UPDATE items WHERE fields IS NULL OR json_valid(fields)=0 OR json_type(fields)!='object' (same trio for tags with 'array'). - migrations/057: same trio for views.config. - pgmigrations/035: WHERE fields IS NULL OR jsonb_typeof(fields)!='object'. JSONB rejects invalid JSON on write so the json_valid leg isn't needed on Postgres; only the shape check matters. - pgmigrations/036: same shape check on views.config. Regression tests in internal/store/items_views_jsonb_test.go: - TestItemsViewsJSONB_SQLiteBackfillRepairsMalformedShapes: applies migrations through 053 (skipping 054 which would itself error on malformed rows), seeds every observable shape pathology — SQL NULL, empty string, JSON null literal, wrong-shape JSON, non-JSON garbage — then applies 055/056/057. Asserts every malformed row is repaired AND the partial UNIQUE index actually fires on duplicate invocation_slug post-rebuild (proving the CREATE INDEX path executed end-to-end). - TestItemsViewsJSONB_PostgresBackfillRepairsMalformedShapes: parallel Postgres coverage; seeds JSONB null / array / primitive via direct ::jsonb cast and asserts the widened WHERE clause repairs each. P2: handleCreateItem didn't unwrap ErrInvalidFieldsType/ErrInvalidTagsType. R1's flexJSONToString tightening propagated the sentinels through every UnmarshalJSON path, but handleCreateItem (POST /items) still returned 'invalid JSON: <wrapped>' from decodeJSON. PATCH and the view/collection POST/PATCH handlers already unwrapped — POST was the outlier. - internal/server/handlers_items.go: mirror the PATCH-side errors.Is handling at the POST path. Brief, three-line diff. - handlers_items_jsonb_inner_shape_test.go: new TestCreateItem_ JSONEncodedStringInnerShapeValidated covers POST with fields=`[]`, fields=42, tags=`{}`, tags={"x":1}, plus a valid positive control. Asserts no "invalid JSON:" wrapper and presence of the sentinel message verbatim. Backfill-pattern audit (codex R2's grep prompt): only 055 / pg-034 (collections.settings, already shipped) exhibits the same NULL-only WHERE gap. Per the brief: NOT touched — retroactive repair belongs to a separate IDEA. Other NULL-only backfills (043/pg-023's oauth_providers, 044/pg-024's expires_at) handle their respective shapes correctly or aren't JSON columns. Verified: make test (SQLite) clean. Full ./... suite against the existing port-5445 Postgres container clean (one unrelated flake in internal/collab passed on rerun). Refs: IDEA-1486, IDEA-1488, codex R2 review.	2026-05-16 10:15:59 -04:00
xarmian	853a7cd453	feat(store): wrap migrations in atomic transactions (IDEA-1485) (#565) ... * fix(store): wrap each migration in a transaction (IDEA-1485) Pad's SQLite migration runner exec'd statements one-by-one on the raw *sql.DB, then INSERT'd into schema_migrations afterwards. A crash between statement N and the bookkeeping INSERT left the schema in an intermediate state, and the next startup re-ran the migration against the already-mutated database — permanent data loss for the table-rebuild migrations 022 / 055. Wrap each migration body + the schema_migrations INSERT in a single BEGIN/COMMIT so they commit (or roll back) atomically. Same pattern for Postgres migrations. PRAGMA-foreign-keys handling for SQLite is load-bearing: the existing 022 / 055 migrations rely on `PRAGMA foreign_keys = OFF` around their DROP TABLE, and that PRAGMA is a no-op inside a SQLite transaction. The new runner lifts PRAGMA statements out of the migration body, pins the migration to a single connection via db.Conn() (foreign_keys is per- connection, so the PRAGMA must hit the same conn that opens BEGIN), and classifies pragmas: foreign_keys=OFF runs BEFORE BEGIN, foreign_keys=ON runs AFTER COMMIT. If a migration disables FKs but never re-enables them, the runner emits PRAGMA foreign_keys = ON itself so the pool conn doesn't leak foreign_keys=OFF. Regression coverage in migration_atomicity_test.go: a deliberately- failing multi-statement migration must NOT record a schema_migrations row AND its partial DDL must roll back, on both SQLite and Postgres. A separate test exercises the PRAGMA-lift path with a real FK rebuild. * fix(store): restore PRAGMA foreign_keys=ON on every exit path (IDEA-1485 P2) Codex R1 caught a P2 in applySQLiteMigration: once `PRAGMA foreign_keys = OFF` exec'd successfully on the pinned conn, any subsequent failure (BeginTx, execMulti, INSERT INTO schema_migrations, Commit, or a post-tx PRAGMA error) returned early before the success-path FK-restore block could run. The conn went back to the pool with foreign_keys=OFF, silently bypassing enforcement for whichever caller next checked it out. Replace the post-success restore block with an inline `defer` registered the moment a before-tx `foreign_keys=OFF` lands on the connection. The defer fires on every return path. The restore is best-effort: a failure is logged via slog.Error (so a real leak isn't silent) but does NOT override the migration's primary error. The migration's own `PRAGMA foreign_keys = ON` (if present) still runs via afterTx on the success path; the deferred re-set is then a harmless idempotent no-op. Drop the now-redundant `disabledFKs` / `reEnabledFKs` tracking and the unused `isForeignKeysOn` helper. Regression test `TestMigrationAtomicity_FailedSQLite_RestoresForeignKeysOnError` pins the pool to a single connection via SetMaxOpenConns(1), runs a migration that disables FKs and then fails on a duplicate PK, and asserts that on a follow-up checkout `PRAGMA foreign_keys` returns 1. Verified the test FAILS when the defer is removed.	2026-05-15 23:57:38 -04:00
xarmian	7e37dfc34e	refactor(store): collections.settings boundary normalization + scan revert (IDEA-1484 follow-up) (#563) ... * refactor(store): drop defensive sql.NullString scans on collections.settings (IDEA-1484 follow-up) PR #562 (squash 0766d7e) hardened collections.settings to NOT NULL DEFAULT '{}' at the schema level. The defensive sql.NullString scans introduced by PR #561 (BUG-1482, squash 714da48) and the paired import-side ""→"{}" coercion in ImportWorkspace are no longer load-bearing — the database now enforces the invariant the readers were defensively reconstructing. Reverted sites: - internal/store/collections.go: GetCollection, ListCollectionsMinimal, ListCollections — direct &c.Settings scans. - internal/store/export.go: ExportWorkspace scan + ImportWorkspace coercion. - internal/store/items.go: scanCollectionDoneFilters helper. - internal/store/item_stars.go: buildCollectionDoneContextMap helper. Test changes: - Removed TestExportImportRoundTripWithEmptyStringSettings, whose purpose evaporated with the import-side coercion. The constraint-outcome tests (TestCollectionsSettingsNotNullEnforced, TestCollectionsSettingsDefaultsToEmptyObject) from PR #562 remain — they assert the load-bearing schema invariant. * fix(store): restore import-side settings coercion (codex R1 P1) Codex R1 caught that the prior commit reverted the import-side `""→"{}"` coercion incorrectly. The NOT NULL DEFAULT '{}' schema constraint added by PR #562 only fires when the INSERT omits the settings column — but ImportWorkspace explicitly supplies the value. A legacy bundle or external JSON workspace import whose `collections[].settings` is "" would therefore bypass the default: Postgres rejects "" at JSONB type-validation; SQLite silently stores invalid JSON. The coercion was doing two jobs (BUG-1482 had folded them together): 1. Defending against NULL-materialized-as-"" on read — obsolete now that the column cannot hold NULL. 2. Defending against legacy/external "" settings on the import boundary — still required because schema constraints don't validate JSON. Job #1's defense (the sql.NullString scans) stays reverted; the column cannot hold NULL. Job #2's defense (the import-side coercion) is restored and renamed in the comment to reflect that it's a boundary normalizer for external data, not a transitional NULL-handler. `TestExportImportRoundTripWithEmptyStringSettings` is restored with an updated comment that makes the boundary-normalization framing explicit. The constraint-outcome tests from PR #562 remain unchanged. Tests pass on both drivers (SQLite full ./..., Postgres internal/store + internal/server). * fix(store): coerce empty-string settings in UpdateCollection (codex R2 P2) Codex R2 surfaced UpdateCollection as the last writer path in the collections.settings contract that didn't enforce the JSON-validity invariant. A PATCH sending {"settings": ""} would write the empty string verbatim, bypassing the NOT NULL DEFAULT '{}' constraint (which only fires on column omission). Same failure mode as the ImportWorkspace bug R1 caught: Postgres rejects "" at JSONB type-validation, SQLite silently stores invalid JSON. Mirrors the boundary normalization restored in ImportWorkspace at 6f22f94. Closes the contract loop for collections.settings — every writer path (CreateCollection via json marshalling, ImportWorkspace, UpdateCollection) now enforces JSON validity at the API boundary. Added TestUpdateCollectionCoercesEmptyStringSettings to guard the boundary. Also corrected a stale comment in TestCollectionsSettingsDefaultsToEmptyObject that referenced GetCollection's removed defensive scan. The sibling-table UPDATE paths (UpdateItem at items.go:1442, UpdateView at views.go:152) have the same defect class on items.fields/tags and views.config; they are pre-existing, not introduced by this PR, and are tracked in the sibling-table follow-up IDEA.	2026-05-15 19:57:22 -04:00
xarmian	0766d7ecf1	feat(store): enforce NOT NULL on collections.settings (IDEA-1484) (#562) ... * feat(store): enforce NOT NULL on collections.settings (IDEA-1484) Adds migration 055 (SQLite) and pg 034 (Postgres) to backfill any NULL collections.settings rows to '{}' and then enforce NOT NULL DEFAULT '{}' at the column level. Eliminates the bug class that BUG-1482 / PR #561 plugged defensively in the four reader sites. SQLite uses the standard table-rebuild recipe (PRAGMA foreign_keys=OFF, copy via COALESCE, RENAME, recreate the single dependent index from 032_permission_indexes.sql). Same PK values are preserved so FKs in items, views, collection_access, and grants remain valid. Postgres uses the simple in-place ALTER TABLE; SET DEFAULT is a no-op belt-and-braces since 001_initial.sql:115 already had DEFAULT '{}'. The defensive sql.NullString scans in collections.go / export.go and the import-side ""→"{}" coercion in export.go remain in place — they revert in a separate follow-up PR after this migration ships everywhere. Removes the four BUG-1482 NULL-only regression tests from collections_test.go (their `UPDATE collections SET settings = NULL` setup is now a hard write error against the new constraint and the NULL-scan branch they guarded is no longer reachable). Reworks TestExportImportRoundTripWithNullSettings into TestExportImportRoundTripWithEmptyStringSettings — it now mutates the exported bundle in-memory to carry the "" sentinel rather than forcing a NULL row, still exercising the import-side coercion path that survives this PR. * test(store): cover collections.settings NOT NULL outcome (IDEA-1484) Addresses Codex R1 P2: the migration test surface lacked direct constraint-check coverage. Adds two focused outcome tests against the post-migration schema: - TestCollectionsSettingsNotNullEnforced — raw INSERT with settings=NULL must fail. Error shape differs across SQLite (NOT NULL constraint failed) and Postgres (SQLSTATE 23502); we only assert err != nil. - TestCollectionsSettingsDefaultsToEmptyObject — raw INSERT omitting the settings column entirely must materialize the column DEFAULT as the Go string "{}" when read back via GetCollection. Same assertion on both drivers; the defensive sql.NullString scan + Postgres JSONB normalization both surface "{}". Both tests reuse createTestWorkspace + the testStore harness, so they run automatically on whichever driver the test invocation selects. R1 P1 (migration runner atomicity) is out of scope per established codebase precedent (022, 025 use the same pattern); will be filed as a follow-up IDEA.	2026-05-15 18:47:01 -04:00
xarmian	714da48442	fix(store): handle nullable collections.settings end-to-end (BUG-1482) (#561) ... * fix(store): make ListCollectionsMinimal Postgres-safe (BUG-1482) `COALESCE(settings, '')` failed at planner time on Postgres because `collections.settings` is JSONB and `''` is not valid JSON (SQLSTATE 22P02). The query failed regardless of row contents; SQLite is type-loose and accepted it, leaving the bug latent in the two production callers (`handlers_dashboard.go`, `handlers_items.go`). Switch the query to a plain `SELECT ... settings ...` and scan into `sql.NullString`, materializing NULL as the empty-string sentinel. This preserves the existing contract that downstream consumers (`buildDoneContextMap`, `ListCollections`'s own scan loop) gate on via `if c.Settings != ""`, so no caller-side changes are needed. Adds two regression tests in `collections_test.go` that exercise the NULL-settings case (the planner-time failure mode) and the happy-path JSON round-trip. Both run against SQLite and Postgres via the existing PAD_TEST_POSTGRES_URL switch in `testStore`. * test(store): tighten ListCollectionsMinimal happy-path assertion (BUG-1482) Codex review round 1 flagged TestListCollectionsMinimalReturnsSettingsJSON as too permissive: `Settings != ""` would pass for `{}` or any wrong JSON payload. Postgres JSONB also normalizes formatting/key order, so a string compare against the input literal would be brittle across drivers. Switch to a semantic compare: unmarshal both sides into map[string]any and reflect.DeepEqual. This actually verifies the JSON round-trips through the (now fixed) ListCollectionsMinimal path on both drivers. * fix(store): NULL-safe settings scan in GetCollection / ListCollections / ExportWorkspace (BUG-1482) Round-2 extension of the same fix shape. Direct `Scan(... &c.Settings ...)` into a Go string fails on Postgres for any row holding a real NULL with "Scan error: converting NULL to string is unsupported". The column is nullable on both drivers (TEXT DEFAULT '{}' / JSONB DEFAULT '{}'), so legacy or manually-poisoned rows can 500 every handler that goes through these readers — `GetCollection` is the hot reader on every item handler, `ListCollections` powers dashboard + sidebar, `ExportWorkspace` crashes the export pipeline before any data is emitted. Same fix as ListCollectionsMinimal: scan into sql.NullString, materialize NULL as "" to preserve the existing sentinel contract that downstream consumers gate on via `if c.Settings != ""` (handlers_dashboard.go:247, handlers_items.go:1626, collections.go:196 in ListCollections's own post-scan loop). Audited; no caller depends on a non-empty default. Adds TestGetCollectionHandlesNullSettings, TestListCollectionsHandlesNullSettings, and TestExportWorkspaceHandlesNullSettings — each forces a NULL via direct UPDATE (bypassing CreateCollection's empty→`{}` coercion) and asserts the function returns without error and surfaces "" downstream. All pass on SQLite and Postgres. * fix(store): coerce empty-string settings to {} on workspace import (BUG-1482) The earlier commits in this PR made ExportWorkspace, GetCollection, and ListCollections all return `""` for a NULL `collections.settings` row, preserving the in-process sentinel contract that downstream consumers (buildDoneContextMap and friends) already gate on via `c.Settings != ""`. That fix surfaced a paired contract gap: ImportWorkspace previously inserted `c.Settings` verbatim into the collections table. After the reader fixes, an exported NULL-settings row materializes as `""` in the bundle, which Postgres's JSONB column rejects at INSERT time. Without this commit, exporting a workspace with any NULL-settings row and re-importing it would have crashed on Postgres — turning one half of a symmetric contract green while leaving the other half broken. Mirror the same coercion CreateCollection applies on the normal create path: when the bundle's settings field is the empty-string sentinel, write `"{}"` instead. Add a round-trip regression test (TestExportImportRoundTripWithNullSettings) that NULL-poisons a workspace's settings, exports, re-imports, and asserts the re-imported collections hold valid JSON. Verified on both drivers. * style(store): rewrite doc comment to avoid gofmt apostrophe-pair rewrite Go 1.19+ gofmt's doc-comment formatter collapses `''` (two ASCII apostrophes) inside backtick code spans into a single `”` (U+201D right double quotation mark) — a typographic-pair heuristic that doesn't quite fit when the literal pair is the load-bearing thing being described (here: SQL's empty-string literal in COALESCE). CI's golangci-lint flagged the file as gofmt-dirty for this reason. Rewrite the prose to describe the bug without using `''` literally: "coalesced settings against an empty SQL string literal" reads more clearly than `COALESCE(settings, '')` becoming `COALESCE(settings, ”)` after gofmt normalization. Functionally identical comment; lint-clean.	2026-05-15 17:22:57 -04:00
xarmian	7c663a3d3f	feat(collections): add blank workspace template + retire auto-upgrade hook (IDEA-1479) (#560) ... * feat(collections): add blank workspace template (IDEA-1479) Introduces a `blank` workspace template that seeds only the two system collections (Conventions, Playbooks) — no Tasks/Ideas/Plans/Docs, no seeded items, no starter conventions or playbooks. Solves the agent-self / non-template-fit use case where the existing software templates leave undeletable ghost collections in the workspace. Adds a new `CategoryCustom` ("Custom") top-level category so the blank template doesn't mis-group with `startup` / `scrum` / `product`. Category is appended last in `CategoryOrder` so it doesn't displace recommended-path templates in the picker. Tests: - TestBlankTemplateShape — exactly 2 system collections, no seeds. - TestBlankTemplateExcludesSoftwareCollections — no tasks/ideas/plans/docs. - TestBlankTemplateAppearsInPicker — surfaces under a Custom group. - TestSeedFromBlankTemplate — bootstrapping produces 2 collections, 0 items. * fix: address codex review for blank template (IDEA-1479) - CreateWorkspaceModal: remove hard-coded 'blank' picker entry that silently fell through to collections.Defaults(). The API-driven blank template (under the Custom category) is now the canonical surface. - Dashboard: gate '+ New Task' button on tasks collection existence so blank workspaces don't render a button that targets a missing collection. - OnboardingChecklist: accept collectionSlugs prop and filter steps whose target collection (plans/tasks/docs) is absent. Conventions step remains unconditional since the conventions collection ships with every template, including blank. Empty-steps guard added to progressPct to avoid NaN. - web/src/lib/utils/templates.ts: add 'custom' -> 'Custom' to mirror the Go CategoryOrder + categoryLabels updates. - cmd/pad/templates_picker_test.go: extend the visible-template assertion list to include 'blank' and assert the Custom category header renders. * fix(store): gate SeedDefaultCollections on zero-collection workspaces (IDEA-1479) The server's startup auto-upgrade hook (cmd/pad/main.go) called SeedDefaultCollections against every workspace at boot. That hook dates to the initial release — long before workspace templates existed — and was written as a backfill for workspaces created before tasks/ideas/plans/docs landed in Defaults(). Post-templates, the hook unconditionally re-materialized the Software-template collections into any workspace missing them — including blank-template workspaces (IDEA-1479), which ship only Conventions + Playbooks by design. Result: every restart silently regrew the ghost user-facing collections the blank template was explicitly built to avoid. Fix: SeedDefaultCollections now returns nil immediately when the workspace has any existing collection (system or user-facing). The rescue path still triggers for genuinely-empty workspaces, preserving the original backfill intent. Tests: - TestBlankWorkspaceSurvivesSeedDefaultCollections — blank workspace remains 2 collections after auto-upgrade (and after a second pass). - TestEmptyWorkspaceStillGetsDefaults — zero-collection workspace still gets the full Software default set. * refactor(server): remove SeedDefaultCollections auto-upgrade at startup (IDEA-1479) The startup auto-upgrade hook in cmd/pad/main.go dated to the initial release, predating workspace templates entirely. Its original intent was per-collection backfill — workspaces created before a new entry landed in Defaults() would acquire it on next boot. Post-templates, that semantic is incompatible with templates that legitimately diverge from Defaults() (e.g. `blank`, which ships only Conventions + Playbooks by design). Round-2 of the IDEA-1479 review attempted to keep the hook by adding a "zero collections" guard, but Dave (after codex round 3) decided the cleanest fix is removing the hook entirely. The codebase has proper migration infrastructure now; any future "add a default collection" work should land as an explicit migration where the author chooses which workspaces to backfill. SeedDefaultCollections itself is preserved (with the round-2 guard) as a building block for any future explicit rescue command or migration. Its doc comment is updated to note it's no longer auto-invoked at startup. The round-2 regression tests (TestBlankWorkspaceSurvivesSeedDefaultCollections, TestEmptyWorkspaceStillGetsDefaults) still apply and pass unchanged. * fix(store): rescue gate uses COUNT(*), not ListCollectionsMinimal (IDEA-1479) Postgres CI on PR #560 caught a regression introduced in commit 3e71fe8: SeedDefaultCollections's zero-collection guard called ListCollectionsMinimal, whose SELECT uses COALESCE(settings, '') against a JSONB column. Postgres parses the '' literal as JSON at plan time and fails with SQLSTATE 22P02 (invalid input syntax for type json), breaking the rescue gate and ~12 cascade test fixtures that depend on the seeder succeeding. The gate only needs to know whether any collection exists, not their schema or settings. Switch to a direct COUNT(*) on the collections table: portable across both drivers, cheaper than the minimal lister, and avoids the broken JSON COALESCE path entirely. Verified locally against both drivers: - SQLite (default): go test ./... — all PASS - Postgres (make test-pg infra): PAD_TEST_POSTGRES_URL=... go test ./... — all PASS, including the three direct failures (TestBlankWorkspaceSurvives…, TestEmptyWorkspaceStillGetsDefaults, TestSeedDefaultCollections) and the cascade FTS/search fixtures. Note: ListCollectionsMinimal's COALESCE(settings, '') expression appears to also affect production callers (handlers_dashboard, handlers_items) on Postgres, but fixing that is out of scope for this PR — those paths have their own tests that aren't failing in CI. Flagged for separate follow-up.	2026-05-15 14:46:26 -04:00
xarmian	be68292e03	fix(store): workspace list freshness reflects item activity (BUG-1481) (#559) ... `pad workspace list` was showing `workspaces.updated_at`, which only moves on workspace-row mutations (rename, settings, members). After this fix the effective UpdatedAt surfaces item activity inside the workspace — answering "where is work happening?" instead of "when was this row last UPDATEd?". Implementation: scalar `MAX(items.updated_at)` subquery in `ListWorkspaces` and `GetUserWorkspaces`, then `effectiveWorkspaceUpdatedAt` picks the later of the two timestamps (portable across SQLite + Postgres, no GREATEST). Read-time approach per the bug's design notes. Visibility-aware: codex review surfaced that a naive MAX leaks activity timing for items the caller can't see. The member subquery mirrors `VisibleCollectionIDs` (`collection_access='all'` short-circuits; for `'specific'` members, system collections + `member_collection_access` + `collection_grants` + `item_grants` gate visibility). The guest subquery limits MAX to items reachable via `collection_grants` / `item_grants`. All grant lookups are workspace-scoped for defense-in-depth. Four regression tests cover: admin `ListWorkspaces`, member all-access, member specific-access leak guard, and guest leak guard.	2026-05-15 12:30:07 -04:00
xarmian	e621eacb9b	feat(server): POST /api/v1/import/url endpoint + integration tests (TASK-1472) (#556) ... Wires internal/urlimport into the API: Fetcher → Detect → converters. Side-effect-free; the editor's "Insert from URL" modal owns any item mutation (TASK-1474). Endpoint: - POST /api/v1/import/url, body {"url"}, response {markdown, detected_type, title?, source_url, fetched_at, content_type}. - Status mapping: 400 invalid URL / SSRF / malformed body; 502 upstream failure (incl. size cap); 504 fetch timeout; 422 conversion failure. - Swagger 2.0 fallback: detected as "openapi" by the sniffer but rejected by ConvertOpenAPI → falls through to ConvertGeneric and re-classifies the response as "generic" so the UI shows the right affordance. - 30s wall-clock budget on the whole pipeline via context.WithTimeout (Fetcher's own timeout is the HTTP-level cap). Package-level Fetcher is memoized via the existing sync.Once-cached safe transport (shared keep-alive pool, no per-request leak). The handler skips the pre-flight ValidateURL when the package fetcher has AllowLocal=true so tests can swap in a loopback-friendly fetcher without bypassing the production guard. Integration tests (handlers_import_test.go): - HTML happy path (httptest upstream, asserts detected_type/title/ source_url/fetched_at/content_type all wired up). - OpenAPI 3.x happy path (inline YAML upstream, asserts ConvertOpenAPI was invoked and detected_type=openapi). - Swagger 2.0 fallback (asserts detected_type re-classified to generic, markdown non-empty). - SSRF rejection (default fetcher, loopback URL → 400 mentioning "private"/"reserved"). - file:// scheme rejected (400). - Missing/malformed body rejected (400). - Upstream 5xx surfaces as 502. - Size cap exceeded surfaces as 502. - No-side-effects check: item count unchanged before/after import. Parent: PLAN-1467.	2026-05-15 00:36:41 -04:00
xarmian	8771f95ab2	feat(urlimport): OpenAPI 3.x → Markdown converter (TASK-1471) (#555) ... * feat(urlimport): OpenAPI 3.x → Markdown converter (TASK-1471) Adds ConvertOpenAPI to internal/urlimport — the "openapi" branch of the v1 importer. Built on pb33f/libopenapi. Layout: - H1 with the API title + version + description - Contact + License lines - Servers list - Endpoints section grouped by primary tag (or "Other" for the untagged). Per operation: `METHOD /path` heading, summary, description, deprecation marker, operation ID, parameter table, request-body summary (with media-type fences and YAML-rendered example), and response code table. - Schemas section with component schemas as Property/Type/Required/ Description tables, schema names sorted for stable output. Scope: - OpenAPI 3.x only. Swagger 2.0 detection returns an explicit "only 3.x" error so the import endpoint (TASK-1472) can fall through to the generic converter. - Recoverable libopenapi build errors (unresolved refs, etc.) are swallowed when the model is still produced — partial spec > no output. Tests: - testdata/petstore-openapi.yaml — full v3 fixture: tags, params, requestBody example, deprecated op, ref-typed schema array, two component schemas with required-field markers. - TestConvertOpenAPI_Petstore — 30+ markdown-substring assertions on the rendered output. - TestConvertOpenAPI_RejectsSwagger2 — explicit v2 error. - TestConvertOpenAPI_RejectsGarbage — non-spec input. - TestConvertOpenAPI_MinimalSpec — empty paths short-circuits. - Helpers: schemaTypeBrief(nil), escapeTableCell, singleLine. Dependency: github.com/pb33f/libopenapi v0.36.3 (MIT-licensed). Parent: PLAN-1467. * fix(urlimport): merge path-level + operation-level parameters per Codex review (round 1) MEDIUM: OpenAPI path-item-level parameters apply to every operation on the path. Previously only slot.op.Parameters was rendered, so common specs that hoist a shared {id} parameter to the path-item level emitted operations with the path parameter missing from the docs. Now opSlot carries item.Parameters as pathParams, and a new mergeParameters helper produces the spec-conformant union: - Path-level parameters first, in declared order. - Operation-level parameters with matching (name, in) override the path-level entry in place. - Operation-only parameters appended after. Tests: - TestConvertOpenAPI_PathLevelParametersMerged — inline fixture with a path-level widgetId + trace and an operation-level trace override + fields op-only param. Asserts widgetId survives, trace shows op-level (required=yes), no duplicate path-level trace row, fields appears. - TestMergeParameters_EmptyInputs — nil/nil short-circuit. * fix(urlimport): no double-backticks on array-of-ref schema types per Codex review (round 2) MEDIUM: schemaTypeBrief() previously wrapped refs in inline backticks ("`Pet`"). For array-of-ref schemas the brief became "array of `Pet`", and the table-cell call site (codeOrBlank) then wrapped the entire value in another pair, producing broken markdown like "`array of `Pet``". Schema properties whose type is an array of a component schema are a normal OpenAPI shape — `Litter.pets: array of Pet` — so this would have hit real specs immediately. Fixes: - schemaTypeBrief now returns plain text — ref names without surrounding backticks. Docstring updated to make the contract explicit ("never contains backticks; caller wraps"). - codeOrBlank strips any stray backticks from input before wrapping so the resulting cell always carries exactly one balanced pair. Defensive: the contract from schemaTypeBrief is plain text now, but stray backticks from any future caller can't corrupt the table. Tests added: - TestConvertOpenAPI_ArrayOfRefTypeCell — inline spec with a `Litter.pets: array of Pet` property. Asserts the type cell is exactly `` `array of Pet` `` and no malformed variants leak. - TestCodeOrBlank — 7-case table covering empty, plain, whitespace, pre-backticked, embedded-backtick, and backtick-only inputs.	2026-05-15 00:24:14 -04:00
xarmian	d1560606cb	feat(urlimport): generic HTML→Markdown converter (TASK-1470) (#553) ... * feat(urlimport): generic HTML→Markdown converter (TASK-1470) Adds ConvertGeneric to internal/urlimport — the v1 catch-all converter for "non-OpenAPI" URLs. Pipeline: 1. go-shiori/go-readability strips chrome/nav/ads/scripts and returns the page's primary article. 2. JohannesKaufmann/html-to-markdown/v2 converts the cleaned HTML to markdown. 3. cleanupMarkdown normalizes line endings, trims trailing whitespace, collapses blank-line runs, and ensures a single trailing newline. Fallback path: when Readability cannot identify an article (directory listings, single paragraphs, pages with no clear content container), the converter falls back to a whole-body conversion so callers still get usable markdown. Dependencies (license-checked): - github.com/JohannesKaufmann/html-to-markdown/v2 v2.5.1 (MIT) - github.com/go-shiori/go-readability (Apache-2.0) Fixtures + tests: - testdata/availity-shape.html — Availity-style div soup with heavy chrome (nav, ads, sidebar, footer, analytics script). Asserts the article content survives and the chrome is stripped. - testdata/mdn-shape.html — MDN-style semantic HTML (article/main + proper heading levels + code fences). Asserts structure preserved. - TestConvertGeneric_EmptyBody — empty-input rejection. - TestConvertGeneric_PlainTextFallback — Readability-can't-find-article fallback path. - TestCleanupMarkdown — 5-case table-driven cleanup verification. Parent: PLAN-1467. * fix(urlimport): preserve hard-line-break markers + apply WithDomain on fallback per Codex review (round 1) - MEDIUM: cleanupMarkdown was stripping the markdown two-trailing- spaces hard-line-break idiom. html-to-markdown emits <br> as " \n" — bulk-stripping trailing whitespace was demoting hard breaks to soft wraps. Now the cleanup steps line-by-line, keeps exactly-two trailing spaces (no tab), strips 1/3+/tab-mixed runs. - MEDIUM: The raw-HTML fallback path (used when Readability cannot identify an article) now passes converter.WithDomain(pageURL) so relative links/images resolve against the source URL rather than the Pad host where they'd 404. Tests added: - cleanupMarkdown: hard-line-break preserved, single/triple trailing spaces stripped, tab-mixed spaces stripped, blank-line-with-spaces collapsed (5 new cases). - TestConvertGeneric_RelativeURLsResolvedOnFallback: relative href in non-article HTML resolves to absolute URL via pageURL.	2026-05-15 00:03:16 -04:00
xarmian	0aa3988319	feat(urlimport): URL fetcher with SSRF guard + content-type detection (TASK-1469) (#552) ... * feat(urlimport): URL fetcher with SSRF guard + content-type detection (TASK-1469) First slice of PLAN-1467's "Insert from URL" feature. Adds the internal/urlimport package with: - fetch.go: SSRF-guarded HTTP GET (10s timeout, 5 MB body cap, redirect re-validation, redacted-error formatting). Blocks loopback, RFC1918, CGNAT, IPv4/IPv6 link-local (incl. 169.254.169.254 cloud-metadata), IPv6 unique-local, and the unspecified address. Hostnames are resolved and every returned IP is checked. - detect.go: Content-type + body-prefix sniff returning "openapi" (JSON or YAML, OpenAPI 3.x or Swagger 2.0) or "generic". Inspects at most 64 KiB. - fetch_test.go: Table-driven SSRF tests covering 24 cases plus happy-path, size-cap, timeout, non-2xx, context-cancel, and a stubbed-transport redirect re-validation. - detect_test.go: 20 detection cases including OpenAPI JSON, Swagger YAML, vendor media types, leading comments, indented-key negatives, and charset-parameter normalization. Package name is urlimport (not "import" — reserved word). No callers yet; the endpoint that consumes Fetcher + Detect lands in TASK-1472. Parent: PLAN-1467. * fix(urlimport): close DNS-rebinding gap + handle >64 KiB OpenAPI JSON per Codex review (round 1) - HIGH: Add safe dialer transport (newSafeTransport). ValidateURL no longer does DNS — the dialer resolves once and validates the resolved IP at dial time, then dials that exact IP. DNS rebinding can no longer slip a public-IP validation past a loopback fetch. ValidateURL becomes a pre-flight (scheme/credentials/IP-literal only) with the canonical guarantee now at the transport layer. - MEDIUM: For JSON bodies over the 64 KiB sniff cap, switch from full Unmarshal (which fails on a truncated tail) to a streaming json.Decoder scan that walks top-level keys and short-circuits as soon as `openapi` or `swagger` is seen. Real-world specs over 64 KiB are now classified correctly, including the case where the `openapi` key is not the first top-level entry. Tests added: - TestFetch_DialerBlocksLoopbackHostname (dial-time rebinding guard) - TestDetect_LargeOpenAPIJSON (>200 KiB OpenAPI body, key first) - TestDetect_LargeOpenAPIJSON_KeyNotFirst (openapi key after huge info) - TestDetect_LargeJSONNotOpenAPI (huge non-OpenAPI stays generic) Removed the DNS-resolution case from TestValidateURL's notes and added a positive case proving hostnames pass the pre-flight (the dial-time check is now the canonical guard). * fix(urlimport): disable env proxy and reuse safe transport per Codex review (round 2) - HIGH: Set Proxy=nil on the safe transport. ProxyFromEnvironment would route via HTTP_PROXY/HTTPS_PROXY, where the dialer connects to the proxy host instead of the target — silently bypassing the hostname-resolution SSRF check inside DialContext. Operators who need an outbound proxy can wire their own trusted transport into Fetcher.Transport. - MEDIUM: Memoize the default safe transport per Fetcher via sync.Once. Previously each Fetch built a fresh *http.Transport whose keep-alive idle-pool stayed in scope until GC, leaking FDs under repeated imports. Now one transport is shared by all Fetch calls on a Fetcher; AllowLocal is captured at first use.	2026-05-14 23:51:44 -04:00
xarmian	38aa872864	fix(fields,activity): debounce typed-input field saves + collapse same-field activity runs (BUG-1466) (#549) ... * fix(web): debounce typed-input field saves to stop per-keystroke activity rows (BUG-1466) Text / number / URL fields in FieldEditor wired oninput directly to onchange, so every keystroke became an item PATCH and an activity row. Typing `ui/editor/tiptap` into a `component` field produced a 30-step keystroke chain in the audit metadata (visible on BUG-1419's timeline). Wrap the typed-input branches in a 500ms idle debounce, flush on blur so tabbing away commits immediately, and flush on unmount so navigation never drops a pending value. Discrete inputs (select / date / checkbox, number ±1 buttons) keep firing on the user action — they aren't typing. Mirrors the markdown content debounce pattern in the detail page. * fix(activity): collapse same-field runs in merged changes metadata + unify diff separator (BUG-1466) Follow-up to the web-side typing debounce. Two related changes: 1) collapseChanges() walks the merged "; "-delimited changes string and collapses runs of consecutive same-field entries into a single "field: first-old → last-new". Drops net no-ops (typed then backspaced). When the web-side debounce in FieldEditor still produces multiple PATCHes within the 5-minute coalesce window — or for older rows that pre-date the debounce — the timeline now reads as one transition instead of a chain. Run-based (not global) collapse so interleaved edits on different fields keep their chronology. 2) diffFields now joins entries with "; " instead of ", " so the joiner is consistent with mergeActivityMeta and TimelineActivityCard.svelte's split delimiter. Multi-field PATCHes previously rendered as a single unparseable blob in the web timeline because the parser only split on ";" — fixed as a side-effect. Adds TestCollapseChanges (10 cases including the BUG-1419 repro) and TestMergeActivityMeta_CollapsesSameFieldRun. Updates the existing TestDiffFieldsPrimitives expectation to match the new joiner. * fix(fields,activity): two follow-ups per Codex review (round 1) [P1] FieldEditor.svelte::handleNumberStep The ±1 buttons computed `(Number(value) || 0) + delta` AFTER calling flushPendingSave(). But `value` is the parent prop — flushing fires onchange asynchronously, so at the moment of the step computation the prop still holds the pre-typed value. Typing 10 over 5 and clicking + would flush 10 then send 6, overwriting the typed value. Compute `base` from `pendingValue` (if hasPending) BEFORE clearing the timer state, then send `base + delta` in one onchange call. [P2] activities.go::collapseChanges The drop-net-no-op step removed entries where `from == to`. But diffFields intentionally emits same-display entries for same-cardinality structured-field replacements like `implementation_notes: (1 note) → (1 note)` (see TestDiffFieldsSameCardinalityArrayChangeStillReported) — the labels match because formatChangeValue summarizes by count, not content, but the underlying data did change. Dropping them silently hides real updates from the activity feed. Track `mergedCount` per entry: increment when collapsing a run, initialize to 1 on parse. Only drop when `mergedCount > 1 && from == to` — i.e. only when the no-op resulted from collapsing multiple input segments (the typed-then-backspaced case). Tests: 2 new TestCollapseChanges cases (single structured-field preserved, interleaved structured-field around a typed run). * fix(fields,activity): two follow-ups per Codex review (round 2) [P1] FieldEditor.svelte number stepper focus race The number ±1 buttons race with the input's onblur handler: blur fires before click in the natural focus-transfer flow, so flushPendingSave clears hasPending → handleNumberStep reads stale `value` from the parent prop → typing 25 over 10 and clicking + sends 25 then 11, losing the typed value. Add onmousedown={preventDefault} on both ±1 buttons. Mousedown precedes blur, and preventDefault on mousedown suppresses the natural focus transfer — the input keeps focus through the click, so hasPending survives until handleNumberStep reads it. [P2] collapseChanges still drops repeated same-display structured runs Round 1's mergedCount>1 rule still dropped runs like `implementation_notes: (1 note) → (1 note); implementation_notes: (1 note) → (1 note)` — two real updates whose display strings happen to match because formatChangeValue summarises array-valued fields by count. Each PATCH represented a different underlying note (diffFields uses reflect.DeepEqual to detect that), but the merged display showed no transition. Track `hadTransition` per entry: true iff the run had a display- level transition (initial from != to, or a subsequent entry's `to` differed from the anchored from). Drop only when mergedCount > 1 && from == to && hadTransition — i.e. only true net-cancellations (typed-then-backspaced). Same-display structured repeats stay; real `foo → bar → foo` swings still drop. Tests: 2 new TestCollapseChanges cases (repeated same-display preserved, real foo→bar→foo swing still dropped). * fix(fields,activity): two follow-ups per Codex review (round 3) [P1] FieldEditor cross-item leak via debounce timer When the parent reuses a FieldEditor instance across an item swap (same schema, same field.key, different item — common when navigating between items in the same collection), the parent's `updateField` closure reads `item.id` at CALL time. A pending timer set while item A was active would fire after item B mounted, patching B with A's typed value. Two-pronged fix in FieldEditor: - Add a $effect that tracks the `value` prop and drops any pending save the moment the parent re-props us. The user actively typed for a now-stale context; aborting is safer than silently writing to the new context. Also covers external collab/SSE rebases of the same field on the same item. - Switch the unmount cleanup from flush → drop. When the parent navigates to an item whose schema lacks this field, the FieldEditor unmounts AFTER the parent's `item` has already been replaced, so a final onchange call would route through updateField → wrong item. Blur is the supported commit gesture (clicking elsewhere within the page, ±1 buttons, tab-out — all flush eagerly); unmount-without-blur is treated as "user abandoned the edit." [P2] collapseChanges still dropped structured count-return swings Round 2's hadTransition rule still dropped a run like `implementation_notes: (1 note) → (2 notes); implementation_notes: (2 notes) → (1 note)` — the user added a note then removed the original, ending with a different single note. The merged display reads `(1 note) → (1 note)` with hadTransition=true, indistinguishable from a typed-then- backspaced cancellation. But formatChangeValue summaries are LOSSY: same display label can wrap entirely different raw values. Add a `hasLossySummary` flag per entry — true iff either `from` or `to` matches the `(text)` format formatChangeValue emits for structured fields. Propagate stickily across the run (any lossy entry pins the whole run). Skip the drop step when the run carries lossy summaries — we can't recover the raw delta from the merged string, so we preserve the entry rather than risk hiding a real change. Tests: 2 new TestCollapseChanges cases (count-return swing preserved, lossy-on-one-side still pins the run). * fix(fields): demote hasPending from \$state to plain let — \$effect was cancelling every keystroke (round 4 [P1]) Critical regression in round 3: the value-track \$effect read \`hasPending\` inside its body, which Svelte 5 promotes to a reactive dependency. scheduleSave() setting \`hasPending = true\` retriggered the same \$effect, whose body cleared the typing timer + pendingValue + hasPending before the debounce could fire. Net effect: typing into any text / number / URL field was silently dropped — onchange never ran, the field never saved. hasPending is only read from imperative handlers (scheduleSave, flushPendingSave, handleNumberStep, the value-track \$effect, the unmount cleanup) — never from a template or other reactive context. Demoting it to a plain \`let\` removes the unwanted subscription while preserving the round-3 behaviour: external value-prop changes still trigger the \$effect (it tracks \`value\`), the body reads hasPending imperatively to decide whether to clear pending state. No tests added — this is a Svelte reactivity edge case that can't be unit-tested without a DOM. svelte-autofixer's pre-existing "variable assigned inside \$effect" suggestion previously flagged the hasPending mutation; that signal is gone now. Per Codex review round 4.	2026-05-14 22:10:12 -04:00