215 Commits

Author	SHA1	Message	Date
xarmian	aaa581b105	refactor(bootstrap): extend dashboard caps to active_items/active_plans/by_role (TASK-1422) (#541) ... * refactor(bootstrap): extend dashboard caps to active_items/active_plans/by_role/suggested_next (TASK-1422) Implements IDEA-1421 (absorbed into PLAN-1410's v0.4 envelope). Extends the BootstrapDashboard wrapper from TASK-1413 to cap four more dashboard sub-arrays with parallel overflow counts, same shape and semantics as the existing attention/recent_activity caps. ## Struct + caps Four new int fields on BootstrapDashboard (all `,omitempty`): - active_items_overflow_count - active_plans_overflow_count - by_role_overflow_count - suggested_next_overflow_count Four new cap constants alongside the existing two: - bootstrapActiveItemsCap = 5 - bootstrapActivePlansCap = 5 - bootstrapByRoleCap = 5 - bootstrapSuggestedNextCap = 5 capBootstrapDashboard extended with four parallel truncate-and-count blocks — same shallow-copy mutation pattern, source pointer untouched (the dashboard endpoint still returns its full-length arrays per its own contract). ## Tests TestCapBootstrapDashboard rewritten to cover all six caps under one contract. `mk` now takes a `dashCounts` struct (Att/Rec/Items/Plans/ Role/Sugg) so each subtest exercises specific caps without populating the others. Each of the four existing subtests (under-cap-no-overflow, over-cap-truncates-and-counts-overflow, source-pointer-unchanged, exact-cap-no-overflow) now asserts the new caps too. Added tiny assertLen/assertOverflow helpers to keep the per-array assertion noise from drowning the contract being tested. bootstrapSectionBytes extended to surface the four new cap-effect lines when triggered, table-driven so future caps drop in cleanly. seedBootstrapSizeFixture updated to seed 6 in_progress tasks (was 5 open) so the new active_items cap fires visibly in the per-section breakdown: "active_items capped: 5 shown, 1 overflow". The status flip is deliberate — dashboard.active_items filters on isActiveStatus(), which excludes initial/terminal statuses; open tasks never appeared in the section. ## Budget bootstrapSizeBudget 7 KiB → 9 KiB. Note that this is FIXTURE-side growth, not shape-side regression: the fixture now seeds enough active items to exercise the new cap (active_items section was 0 bytes when tasks were status=open). The cap itself is purely a SAVINGS — on docapp it drops active_items from 7 → 5 entries with overflow_count=2. Budget history note in handlers_bootstrap_test.go updated with the TASK-1422 line and an explicit "fixture-side, not shape-side" explanation so future readers understand why the budget moved up. ## Out of scope - Slim BootstrapRole projection — TASK-1423. - Schema label + sort_order trim — TASK-1424. - ToolSurfaceVersion 0.3 → 0.4 bump — TASK-1418 (final PR). Parent: PLAN-1410. Resolves IDEA-1421 once merged. * fix(bootstrap): drop unreachable suggested_next cap (TASK-1422 follow-up) Address Codex P1 finding on PR #541: `suggested_next_overflow_count` was unreachable in production responses because `buildDashboardResponse` already truncates `SuggestedNext` to 3 upstream (see "Take top 3" comment in handlers_dashboard.go:854-858), while my bootstrap cap was 5. The cap-and-overflow logic could only have fired against synthetic test state, never against the real dashboard pipeline. Two responses to consider: 1. Lower bootstrap's cap to a number smaller than 3 — defeats the upstream design choice (3 IS the intentional limit). 2. Drop the bootstrap-side cap — clean, no dead surface. Going with (2). If the upstream cap is ever raised or removed, that's the moment to add a suggested_next_overflow_count back. Removed: - SuggestedNextOverflowCount field on BootstrapDashboard - bootstrapSuggestedNextCap constant - The truncate-and-count block in capBootstrapDashboard - The suggested_next row in TestCapBootstrapDashboard's dashCounts helper and all five subtest assertions - The suggested_next entry in bootstrapSectionBytes's cap-line loop The fixture still seeds tasks and a plan, so the no-cap path on SuggestedNext is naturally exercised through TestBootstrapSizeBudget. The godoc on BootstrapDashboard now explicitly calls out the exclusion + the upstream-cap rationale so a future reader knows why suggested_next is missing from the otherwise-uniform cap set. Parent: PLAN-1410 / TASK-1422. * docs(skill): align SKILL.md dashboard cap description with TASK-1422 Address Codex P3 finding on PR #541: the SKILL.md `Context Loading` section described only the two original cap fields (attention_overflow_count, recent_activity_overflow_count). After TASK-1422 the bootstrap response carries three more (active_items_overflow_count, active_plans_overflow_count, by_role_overflow_count), and the agent needs to know to pull the full set via `pad project dashboard` when any of them are > 0. Updated the bullet to enumerate all five capped sub-arrays and state the overflow-field pattern generically rather than per-field. Same in-PR-sync pattern used for TASK-1413, TASK-1415, TASK-1416. Parent: PLAN-1410 / TASK-1422. * docs(bootstrap): fix three stale comments after dropping suggested_next cap (TASK-1422 follow-up) Address Codex P3 finding on PR #541 round 3: three stale doc strings referenced the old shape (with suggested_next) after the cap was dropped in the prior commit. Updated: 1. handlers_bootstrap_test.go budget-history line for TASK-1422 — removed `suggested_next` from the cap list and added the "deliberately excluded — already capped to 3 upstream" rationale so future readers know why the otherwise-uniform cap set is missing one. 2. handlers_bootstrap.go BootstrapDashboard godoc — changed "two overflow counts" to "five overflow counts (one per capped sub-array)". 3. handlers_bootstrap.go capBootstrapDashboard godoc — changed "both caps are untriggered" to "all caps are untriggered". Tidy-up only, no behavior change. Same skill-↔-code sync hygiene that has been the running theme across PLAN-1410's review loops. Parent: PLAN-1410 / TASK-1422. * docs(bootstrap): update remaining stale call-site comment for capBootstrapDashboard (TASK-1422 follow-up) Final stale-doc cleanup per Codex P3 on PR #541 round 4: the BuildAgentBootstrap dashboard-wrapping call-site comment still listed only attention + recent_activity. Updated to enumerate all five capped sub-arrays for parity with the godoc on BootstrapDashboard / capBootstrapDashboard. Same hygiene as the previous commit; no behavior change. Parent: PLAN-1410 / TASK-1422.	2026-05-13 17:10:56 -04:00
xarmian	9c9aac3ea6	chore(bootstrap): tighten size budget 8 KiB → 7 KiB to lock in PLAN-1410's win (TASK-1417) (#540) ... PLAN-1410's bootstrap shape work is complete on main (TASK-1411..1416). The fixture still measures 6,355 bytes against the 8 KiB budget — too much headroom for a ratchet whose purpose is to detect shape regressions. Tightened to 7 KiB: - Fixture: 6,355 bytes - Budget: 7,168 bytes (7 KiB) - Headroom: 813 bytes (~12.8%) Tight enough to catch any meaningful shape regression (reintroducing a duplicated field, un-capping a dashboard array, re-stringifying schema), loose enough to absorb routine schema reordering or single-field additions without false alarms. Budget-history note updated with the TASK-1417 line and a forward-looking pointer at IDEA-1421 (next-round dashboard sub-array caps) which will land its own win under its own ratchet. The companion measurement (per-section bytes for fixture + live docapp + pre/post per-invocation totals) was recorded directly into PLAN-1410's body via `pad item update PLAN-1410 --stdin`, which is the canonical home for plan results. Cumulative win: fixture: 13,861 → 6,355 (-54.2%) live docapp bootstrap: 52,033 → 33,375 (-35.9%) live SKILL.md: 40,193 → 29,840 (-25.8%) live combined per /pad: 92,226 → 63,215 (-31.5%) The original ~43% target was set against a static-workspace assumption; the live shortfall is workspace-scale-dependent (conventions on docapp are 11.2 KB vs 0.5 KB on the fixture). IDEA-1421 captures the next round (dashboard active_items / active_plans / by_role / suggested_next caps, estimated ~3 KB additional live win, backwards-compatible). Parent: PLAN-1410. Final remaining task: TASK-1418 (ToolSurfaceVersion 0.3 → 0.4 contractual announcement).	2026-05-13 16:25:47 -04:00
xarmian	638a456f98	refactor(bootstrap): dedup recent_activity, drop convention slug, cap dashboard arrays (TASK-1413) (#536) ... * refactor(bootstrap): dedup recent_activity, drop convention slug, cap dashboard arrays (TASK-1413) Three bundled handler-level cleanups against PLAN-1410's bootstrap shape. Total fixture savings: 8,992 → 6,355 bytes (-2,637 b / -29%). 1. Drop duplicate top-level `recent_activity` AgentBootstrap.RecentActivity was bit-for-bit identical to AgentBootstrap.Dashboard.RecentActivity. Removed: - AgentBootstrap.RecentActivity field - capRecentActivity() helper - recentActivityWindow constant - the time import (no longer used) Fixture savings: -1,751 bytes. 2. Drop `slug` from AgentBootstrapConvention Agents address convention items by ref (CONVE-N); slug was dead weight. Removed the field + the population line in collectAlwaysOnConventions. Fixture savings: -78 bytes. 3. Cap dashboard.attention + dashboard.recent_activity to 5 in bootstrap New BootstrapDashboard wrapper embeds *DashboardResponse (so the wire shape stays compatible — same field names, same nesting) and adds two overflow counts: - attention_overflow_count (omitempty when zero) - recent_activity_overflow_count (omitempty when zero) The cap is applied via capBootstrapDashboard which shallow-copies the DashboardResponse before truncating the slices, so callers downstream of buildDashboardResponse (the dashboard endpoint itself, the web UI) see their original full-length arrays unchanged. `pad project dashboard` contract is preserved verbatim. Fixture savings: -789 bytes (recent_activity capped 9 → 5; attention untouched, fixture has 0 attention items). Coverage: - TestCapBootstrapDashboard (4 subtests): under-cap-no-overflow, over-cap-truncates-and-counts-overflow, source-pointer-unchanged, exact-cap-no-overflow. Locks in the cap contract independent of the full bootstrap pipeline. - TestBootstrapEmptyArraysNotNull updated: the top-level recent_activity key was removed from the required-keys list, with a separate assertion that guards against it reappearing. - TestBootstrapEmptyWorkspace updated: removed the b.RecentActivity nil-check; added a (defensive) check that dashboard's nested recent_activity is non-nil when dashboard is present. - bootstrapSectionBytes now surfaces the cap effect ("attention capped: 5 shown, 4 overflow") when triggered, so the trim's value is legible from CI output. bootstrapSizeBudget tightened 11 KiB → 8 KiB to lock in the win. Budget-history comment updated. Out of scope (handled by later PLAN-1410 PRs): - Skill-file trim (TASK-1414/1415/1416) - Final measurement (TASK-1417) - ToolSurfaceVersion 0.3 → 0.4 (TASK-1418, after all shape changes land) Parent: PLAN-1410. * docs(skill): align SKILL.md bootstrap shape with PLAN-1410 / TASK-1413 The skill's `Context Loading` section described the old wire shape: - `dashboard {...}` — active items, attention, suggested next, recent activity - `recent_activity [...]` — capped to the last 24h After TASK-1413 the top-level `recent_activity` field is gone (it was a bit-for-bit duplicate of `dashboard.recent_activity`), and the remaining `dashboard.recent_activity` is capped by COUNT (top 5) not by TIME (24h window). The two cap fields (attention_overflow_count and recent_activity_overflow_count) tell the agent how much was trimmed so it can decide whether to follow up with a full `pad project dashboard` query. Per the Codex P2 finding on PR #536: documenting the new contract in this PR keeps skill ↔ wire-shape strictly synchronized (no window where the docs are wrong about the shape this PR ships). Parent: PLAN-1410 / TASK-1413.	2026-05-13 15:29:59 -04:00
xarmian	8da0473e4e	refactor(bootstrap): slim Collections projection — drop id/timestamps/settings, parse schema inline (TASK-1412) (#535) ... Introduces BootstrapCollection — a purpose-built projection for the bootstrap response that replaces []models.Collection on AgentBootstrap.Collections. Drops fields the /pad skill never reads: - id, workspace_id — agent addresses collections by slug - created_at, updated_at, — irrelevant at context-load time deleted_at - settings — quick_actions + view defaults are web-UI chat-prompt config The remaining schema string is delivered as a nested JSON object (json.RawMessage) rather than a JSON-encoded string, killing the backslash-escape overhead so the agent sees real {}/[] structure instead of double-encoded quotes. json.Valid() gates the emission so a future migration leaving non-JSON in the column can't break agent-side json.Unmarshal — invalid/empty schemas are simply omitted (omitempty). Measured against the bootstrapSizeBudget fixture (TASK-1411): before after delta collections 8,848 b 3,979 b -4,869 b (-55%) total bootstrap 13,861 b 8,992 b -4,869 b (-35%) Budget tightened from 16 KiB to 11 KiB to lock in the win. Later PLAN-1410 PRs (TASK-1413's dedup + dashboard caps, TASK-1417's final measurement) tighten further. Wire-shape change details: - BuildAgentBootstrap holds collections as []models.Collection through the visibility-restricted role+count recompute (which keys lookups by Collection.ID), then projects to []BootstrapCollection at the end of that section. ID-keyed recompute logic is preserved verbatim — only the final wire shape changes. - printBootstrapMarkdown was already reading {slug, name, prefix} via its own anonymous struct; those three are preserved. - No web-UI consumers exist for /agent/bootstrap (grep confirms), so no client-side churn. Out of scope (handled by later PLAN-1410 PRs): - Dedup'ing top-level recent_activity, dropping convention slug, capping dashboard.attention/recent_activity (TASK-1413). - ToolSurfaceVersion bump 0.3 → 0.4 (TASK-1418, after all shape changes land). Parent: PLAN-1410.	2026-05-13 12:18:07 -04:00
xarmian	91f1c0a017	test(bootstrap): add size-budget benchmark for the agent bootstrap response (TASK-1411) (#534) ... Adds TestBootstrapSizeBudget which builds an AgentBootstrap blob against a seeded representative fixture (default template seeds + 2 always-on conventions with bodies + 1 slug-invocable playbook + 5 tasks + 1 plan) and asserts the marshalled JSON byte count stays at or below bootstrapSizeBudget (initially 16 KiB, against a current actual of ~13.8 KiB on the seeded fixture). On every run — pass or fail — the test logs a per-section breakdown (workspace / user / collections / conventions / roles / playbooks / dashboard / recent_activity) so size regressions are diagnosable from CI output alone, and so the cumulative trim across PLAN-1410 is visible as the budget tightens. This is the baseline ratchet for PLAN-1410's bootstrap-shape PRs: - TASK-1412 (slim Collections projection) tightens the budget down once schema-as-string and the redundant ids/timestamps come out. - TASK-1413 (dedup top-level recent_activity, drop convention slug, cap dashboard arrays) tightens further. - TASK-1418 records the final v0.4 actual. The docapp workspace currently measures ~52 KB / ~13K tokens on the real bootstrap payload; the fixture is intentionally small but exercises the same shape contributors so a regression in the projected shape (per-collection settings, schema-as-string, duplicate recent_activity, etc.) trips the budget at fixture scale. Parent: PLAN-1410.	2026-05-13 10:08:41 -04:00
xarmian	fed4b60e65	feat(playbook): add pad playbook CLI (list/show/run) + endpoints (TASK-1382) (#520) ... * feat(playbook): add pad playbook CLI (list/show/run) + endpoints (TASK-1382) PLAN-1377 T5 — first-class invokable-procedure surface. Three HTTP endpoints + three CLI subcommands, with a strict CLI arg parser and a side-effect-free run path. Endpoints GET /workspaces/{ws}/playbooks — metadata array, same projection as bootstrap GET /workspaces/{ws}/playbooks/{ref} — full item, resolved by invocation_slug | ref | slug POST /workspaces/{ws}/playbooks/{ref}/run — bind args, return body + bound + unbound. No execution; the agent owns step playback. Resolution resolvePlaybook walks invocation_slug first (the /pad <slug> user-facing identifier), then falls back to ResolveItem (UUID / ref / slug). A stray TASK-5 hitting /playbooks/TASK-5 returns 404 instead of leaking a non-playbook into the surface. Arg parsing ParsePlaybookCLIArgs implements the strict rules from PLAN-1377: - Required positional args first, in declared order. - Flag types: bareword presence sets true. - Other types: key=value form (number is parsed as float64, enum is validated against declared options). The server takes either pre-parsed args (MCP / programmatic callers) OR raw CLI tokens (CLI caller); merge logic prefers explicit args over raw_args. The CLI sends raw_args, so there is one parser implementation and no drift risk. CLI pad playbook list — table or json pad playbook show <slug|ref> [--format] — markdown / json pad playbook run <slug|ref> [args...] — body + bound args Client method cli.Client.{ListPlaybooks, ShowPlaybook, RunPlaybook(args, rawArgs)} — runtime callers pass args; CLI passes rawArgs. Tests TestPlaybookList, TestPlaybookShowByInvocationSlug, TestPlaybookShowByRef, TestPlaybookShowRejectsNonPlaybook, TestPlaybookRunBindsArgs (with-args, missing-required, with-raw-args), TestParsePlaybookCLIArgsErrors, TestPlaybookListEmptyShape. Parent: PLAN-1377. * fix(playbook): tighten arg parsing per Codex round 1 (TASK-1382) P2.1 — Positional binding now skips optional and flag-typed slots. The PLAN-1377 contract says ONLY required args fill positional slots; other typed args must be key=value. Without this, a spec with an optional arg before a required one (e.g. [merge-strategy?, target!]) bound the caller's bareword TASK-7 to merge-strategy instead of target. P2.2 — number coercion now uses strconv.ParseFloat with NaN/Inf rejection. Sscanf(%g) was sloppy: it accepted '1abc' as 1 (partial match) and accepted NaN/Inf, which json.Marshal then refused after the handler had already written a 200 header. P3 — empty-body run requests now decode cleanly. The decodeJSON wrapper folds io.EOF into 'invalid JSON: EOF' so the previous err.Error() != "EOF" check never fired. errors.Is(err, io.EOF) on the unwrapped chain handles the wrapping correctly. Tests: TestPlaybookRunAcceptsEmptyBody, TestParsePlaybookCLIArgsOptionalNotPositional, TestCoercePlaybookValueNumberRejectsBadInput. Parent: PLAN-1377.	2026-05-12 18:29:11 -04:00
xarmian	24f0445efa	feat(bootstrap): single-roundtrip /pad context-load endpoint (TASK-1379) (#518) ... * feat(bootstrap): single-roundtrip /pad context-load endpoint (TASK-1379) Implements the agent bootstrap surface for PLAN-1377. One HTTP call replaces the four separate /pad context-loading invocations (workspace + collections + conventions + roles + playbooks) the skill used to make, cutting ~200-400ms of startup latency on every /pad command. Wire shape: - GET /api/v1/workspaces/{ws}/agent/bootstrap returns AgentBootstrap: workspace { slug, name, id }, user { name, email, id }, collections [...], conventions [...always-on, status=active], roles [...], playbooks [metadata-only — no bodies], dashboard {...}, recent_activity [... 24h]. - pad bootstrap [--format json|markdown] CLI wrapper. JSON is the canonical wire format that the /pad skill consumes; markdown is a human-readable summary for quick terminal inspection. Implementation notes: - Single source of truth: Server.BuildAgentBootstrap. The HTTP handler is a thin wrapper, and TASK-1380 will reuse it from three MCP surfaces (resource + set_workspace embed + pad_meta tool action). - Dashboard reuse: handleGetDashboard's body extracted into buildDashboardResponse(workspaceID, r) returning (*DashboardResponse, error). The HTTP handler is now a thin wrapper; bootstrap calls the builder directly to embed dashboard data without a second roundtrip. - Playbook bodies are deliberately NOT included — metadata only (~80 bytes/entry vs 5-10KB) so the bootstrap stays small for workspaces with many playbooks. Full bodies load on invocation. - Convention bodies ARE included for the always-on/active subset (must be agent-known up front); trigger-specific conventions stay load-on-demand. - Empty slices serialize as [] not null so the agent doesn't need defensive nil checks. Tests: - TestBootstrapEmptyWorkspace, TestBootstrapEmptyArraysNotNull, TestBootstrapIncludesPlaybookMetadata, TestPlaybookSummaryPrefersFirstParagraph. Parent: PLAN-1377. * fix(bootstrap): respect collection visibility + guest grants (TASK-1379) Codex round 1: BuildAgentBootstrap was bypassing visibility filters, so a guest admitted by RequireWorkspaceAccess could read collections, conventions, and playbook metadata they don't have access to. Now mirrors handleListCollections + handleGetDashboard: 1. Resolve visibleCollectionIDs(r, workspaceID) once. 2. Filter the Collections array through isCollectionVisible. 3. Gate the conventions + playbooks sub-queries on whether the caller can see those collections at all (presence in the filtered slice implies visibility). 4. Empty slices for inaccessible sub-resources, so the wire shape stays consistent — no missing keys to confuse the agent skill. 5. Pass-r=nil callers (future MCP in-process dispatchers) keep the 'full visibility' shortcut, but the doc comment now spells out that those callers MUST verify access out-of-band. Parent: PLAN-1377. * fix(bootstrap): apply guest item-level grants + recompute role counts (TASK-1379) Codex round 2: P1 — Item-level guest grants now flow into the convention + playbook sub-queries. visibleCollectionIDs alone admits the conventions / playbooks collection if a guest has ANY item grant inside it; without ItemIDs filtering, those queries then return the whole always-on convention body set or every playbook's metadata. Now mirrors the handleListItems shape: resolve (fullCollIDs, grantedItemIDs) via guestResourceFilter, and pass the (collIDs, itemIDs) tuple through to collectAlwaysOnConventions + collectPlaybookMetadata so a guest with a single grant only sees that one item. P2 — AgentRole.ItemCount is now recomputed from the visible item set for restricted callers, matching handleListAgentRoles. Without this, a guest could read role counts computed across all workspace items and infer hidden activity per role. Helper signatures updated to accept (collIDs, itemIDs); the doc comments explain the nil/non-nil semantics so future callers can't silently regress this. Parent: PLAN-1377. * fix(bootstrap): rewrite collection counts from visible set for guests (TASK-1379) Codex round 3: Collection.item_count + active_item_count are computed across the whole collection by ListCollections, so a guest with one item grant in a collection still received the collection in the filtered list but with hidden counts. Reuse the visible item set (already computed for role counts) to recompute collection item_count for restricted callers. active_item_count is set equal to item_count to avoid a separate done-rules buildup the bootstrap consumers don't depend on — better a self-consistent number than a leaked one. Parent: PLAN-1377.	2026-05-12 17:51:05 -04:00
xarmian	c38b3bf5cd	feat(playbooks): add invocation_slug + arguments schema fields (TASK-1378) (#517) ... * feat(playbooks): add invocation_slug + arguments schema fields (TASK-1378) Foundational change for PLAN-1377 — playbooks become first-class invokable procedures. Two new optional fields land on the Playbooks collection schema: - `invocation_slug` (text, kebab-case, unique-per-workspace among non-null values): enables `/pad <slug>` direct invocation. Nullable so trigger-only playbooks (e.g. on-release checklists) don't need one. - `arguments` (json, array of {name, type, required, default, description}): declares the playbook's argument contract; mirrors the body's `## Arguments` section in queryable form. Plumbing pieces: - `models.FieldDef` grows two general-purpose options — `Pattern` for regex validation and `UniqueScope` for collection-level uniqueness. Both are opt-in; existing schemas are unaffected. - `items.ValidateFields` learns the `json` field type (accepts any JSON-decodable value) and applies `Pattern` to string-typed values. - `handlers_items.checkUniqueFields` queries `Store.ListItems` to enforce `UniqueScope == "workspace_collection"` on create + update. - Two migrations (SQLite 054, Postgres 033) JSON-patch the playbooks schema on existing workspaces so the new fields show up without a workspace re-init. - TypeScript `FieldDef` mirrors the Go side. Parent: PLAN-1377. * fix(playbooks): address Codex review round 1 findings (TASK-1378) P1 — EditCollectionModal now round-trips opaque pattern/unique_scope metadata. EditableField carries the new keys; the load + save paths preserve them so re-saving the playbooks collection from the UI doesn't strip server-side validation rules the modal doesn't yet expose dedicated controls for. fieldFromDef mirrors the change for templates. P2 — checkUniqueFields' pre-write ListItems check is now backed by a partial unique index (idx_items_invocation_slug_per_collection, SQLite + Postgres) scoped to non-empty, non-deleted rows. The pre-check still gives users a friendly error message in the common case; the index closes the TOCTOU race between two concurrent writers. The create-conflict error message is now generic enough to cover both the slug constraint and the new invocation_slug index. P2 — `json` field type now rejects raw strings, numbers, and bools. Only objects, arrays, and null are accepted, so a generic web text input can't silently corrupt a structured field by emitting "[]" instead of an actual array. FieldEditor.svelte routes `json` fields to a read-only summary in both readonly and edit modes; dedicated editors (like TASK-1384's playbook editor that owns `arguments`) own the structured form. P3 — invocation_slug regex now requires a minimum of two characters (`^[a-z0-9][a-z0-9-]*[a-z0-9]$`) in the Go const, the SQLite migration, the Postgres migration, and the validate tests. Single-letter slugs would shadow plausible NL tokens (e.g. `/pad a ...`) and the doc comment already claimed the two-char floor; this aligns code with intent. Parent: PLAN-1377. * fix(playbooks): address Codex review round 2 findings (TASK-1378) P2.1 — checkUniqueFields no longer passes IncludeArchived=true. The application-layer pre-check now matches the partial unique index's `deleted_at IS NULL` predicate so a soft-deleted playbook releases its slug back to the pool and reclaiming it succeeds instead of 409'ing. P2.2 — handleUpdateItem now maps UNIQUE constraint / duplicate key errors from UpdateItem to HTTP 409, mirroring the create path. A true concurrent-update race that slips past checkUniqueFields and trips the partial unique index used to surface as a misleading 500. (Not addressed in this round: Codex's third finding — concern about the partial unique index applying to "every collection" — is, on close reading, not what the index does. `ON items(collection_id, json_extract(...))` scopes uniqueness to the (collection_id, slug) pair, so two items in different collections with the same `invocation_slug` value coexist fine. The migration-failure risk is theoretical: `invocation_slug` is a brand-new field key, so no pre-existing items can have it set, and no migration-time duplicates can exist. If a future custom collection adopts the same field name, opting into per-collection uniqueness is exactly the intended semantic of FieldDef.UniqueScope.) Parent: PLAN-1377. * fix(playbooks): map restore-path UNIQUE violations to 409 (TASK-1378) Codex round 3: restoring an archived playbook can hit the partial unique index on invocation_slug if a replacement item already claimed the slug. Map UNIQUE constraint / duplicate key errors from RestoreItem to HTTP 409 with a targeted message, matching the create + update paths. Parent: PLAN-1377. * fix(playbooks): map collab-snapshot UNIQUE violations to 409 (TASK-1378) Codex round 4: the collab-snapshot PATCH branch under `s.collab.UnderItemLock` ran its own UpdateItem call and fell through to writeInternalError on any non-stale-snapshot error. A concurrent edit racing the invocation_slug partial unique index would surface as 500 instead of 409. Mirror the main UpdateItem error mapping. Codex's other round-4 finding — the partial unique index applying to "every collection" — is not addressed because the index IS already collection-scoped: `ON items(collection_id, json_extract(fields, '$.invocation_slug'))`. Two items in different collections with the same slug coexist; only same-collection duplicates conflict. Migration duplicates are impossible because `invocation_slug` is a brand-new field key with no pre-existing items setting it. A custom collection that later adopts the same field name opts into per-collection uniqueness, matching the FieldDef.UniqueScope="workspace_collection" semantic. Parent: PLAN-1377.	2026-05-12 17:17:56 -04:00
xarmian	c2351d861d	fix(ci): lower test-only bcrypt cost + re-enable -race on PRs (BUG-1371) (#513) ... The full `internal/server` test suite under `-race` had grown past the 30m CI timeout, failing every push to main since ~TASK-1354. Diagnosis: bcrypt at the production cost (12) takes ~3s per call under the race detector, and dozens of tests now bootstrap a user via the loopback HTTP path (`bootstrapFirstUser` → `store.CreateUser` → `bcrypt.GenerateFromPassword`). Cumulative cost dominated the budget. Two coordinated changes: 1. Lower bcrypt cost in test binaries. `bcryptCost` becomes a package var (still package-private), and a new `SetBcryptCostForTesting` helper lets each test binary's `TestMain` drop it to `bcrypt.MinCost`. Production stays at 12 — only the test process ever mutates the value. 2. Re-enable `-race` on pull requests. The `if: github.ref == 'refs/heads/main'` gate was originally a GitHub Actions minutes cost-control; the repo is public now, so PR minutes are free, and we'd rather catch race regressions on the contributing branch than after merge. Measured impact: - `go test -race ./internal/server`: 1800s timeout → 830s (13m51s). - `go test ./internal/store`: 808s → 35s. - `go test ./internal/server`: 192s → 60s. The 30m timeout stays — it's headroom for genuine deadlocks, which would still hit the goroutine-dump panic the way BUG-851 did. Prior art: BUG-851 (10m → 30m bump, ipRateLimiter goroutine drain). This is a different cause (bcrypt cumulative time) so the fix is different.	2026-05-12 10:01:14 -04:00
xarmian	bd8667eadf	feat: seq-stamped SSE events + stale-event short-circuit (TASK-1358) (#505) ... * feat: seq-stamped SSE events + stale-event short-circuit (TASK-1358) ## Server - events.Event gains a `Seq int64` field (omitempty). Server populates it on item lifecycle events so SSE consumers can reason about ordering and contiguity against their /items-changes cursor. - publishItemEventWithName takes seq as a parameter; all call sites in handlers_items.go pass the item's current seq: - item_created → item.Seq - item_updated → updated.Seq - item_archived → re-fetched via GetItemIncludeDeleted (DeleteItem bumps seq but doesn't return the updated row) - item_restored → restored.Seq - move target → moved.Seq ## Web - ItemEvent gains optional `seq` (matches the server's omitempty). - localIndex.classifySSEEvent(ws, event) returns 'no-seq' | 'stale' | 'contiguous' | 'gap'. The collection page uses this to short-circuit duplicate / replayed events the server's replay buffer re-delivers after tab-resume. Non-stale events still call deltaSync because the SSE wire payload only carries metadata, not the row data; classify does NOT advance the cursor (applyDelta with real row data is the only path that does — preserving the IDB invariant from TASK-1356). Parent: PLAN-1343. * fix(events): version-restore item_updated event carries seq (Codex round 1) Version-restore in handlers_item_versions.go was publishing the item_updated event directly via events.Publish without Seq, bypassing the new seq-stamped SSE contract from TASK-1358. localIndex's classifySSEEvent would always return 'no-seq' for those events, forcing a generic /items-changes refetch instead of allowing the stale/gap fast paths. Now passes updated.Seq from the store response. Parent: PLAN-1343.	2026-05-11 20:29:50 -04:00
xarmian	a5b93c17c9	feat(api): add /items-changes?since=<seq> delta endpoint (TASK-1354) (#494) ... * feat(api): add /items-changes?since=<seq> delta endpoint (TASK-1354) Adds the delta-fetch sibling of /items-index for the local-first read model (PLAN-1343 / DOC-1342 design decision #1). Clients track the workspace-scoped monotonic seq cursor returned by /items-index (TASK-1353) and poll /items-changes?since=<cursor> to apply just the rows that have mutated — without re-downloading the entire workspace. ## Endpoint GET /api/v1/workspaces/{ws}/items-changes?since=<seq>&limit=<n> - `since`: exclusive seq lower bound (returns `seq > since`). Defaults to 0 → full delta == /items-index modulo ordering. Bad input → 400. - `limit`: cap on rows. Defaults to 5000, clamped to 50000. Bad input → 400. ## Response { "changes": [...skinny rows with `deleted: bool`...], "cursor": "<decimal MAX(seq) or unchanged since when empty>" } Soft-deleted rows propagate (no `deleted_at IS NULL` filter on the backing scan) so a delta consumer can remove them from its local index without a second roundtrip. Parent metadata enrichment matches /items-index: the underlying GetItem filters soft-deleted parents so we never leak parent title/ref for an archived parent. Cursor contract: - Sorted ASC by seq → re-passing the response's cursor as `since` on the next poll is no-overlap, no-gap (strictly monotonic seq invariant from TASK-1352). - Empty response preserves the caller's `since` so position isn't lost. - Truncated-by-limit responses set cursor to the last row's seq. ## Tests - FullDeltaFromZero — three creates, since=0, ascending seq, every row deleted=false, cursor=MAX(seq). - IncrementalUpdateAndDelete — typical resume flow: snapshot cursor, mutate, delta returns exactly the mutated + tombstoned rows with the right `deleted` flag. - CursorRoundtripsCleanly — empty-poll after consuming, cursor preserved. - LimitTruncatesAndCursorResumes — paging contract holds end to end with no overlap. - InvalidParams — bad since / limit values rejected with 400. - EmptyWorkspace — cursor round-trips caller's since unchanged. ## Web TypeScript: `ItemChangeRow = ItemIndexRow & { deleted: boolean }`, `ItemChangesResponse = { changes, cursor }`. API client gains `api.items.changes(ws, sinceCursor, opts?)` with the same defensive content-strip as listIndex so a stray `content: ""` key from a Go zero-value can never clobber the canonical store. Parent: PLAN-1343. Depends on TASK-1352 (seq column) and TASK-1353 (seq cursor on /items-index). Unblocks the future client-side localIndex.applyDelta integration task. * fix(api): surface tombstones for item-grant users in /items-changes per Codex review (round 1) Codex round 1 caught that handleListItemsChanges was building its ItemIDs filter from guestResourceFilter, which itself uses GuestVisibleResources whose item-grant query filters out soft-deleted items. The result: a guest or restricted member with an item-level grant on a single item would see that ID disappear from the lookup as soon as the item was soft-deleted — and /items-changes would never emit a `deleted:true` tombstone, so the client would keep the stale row in its local index forever. Fix: - New Store.GuestVisibleResourcesIncludeDeleted that drops the `i.deleted_at IS NULL` / `c.deleted_at IS NULL` filters on both collection and item grants so tombstone IDs flow through. - New Server.guestResourceFilterIncludeDeletedItems delegate pointing at the new store helper. Implementation is shared with the live variant via guestResourceFilterCore so the member-collection-access + system-collection merge logic stays in one place. - handleListItemsChanges swaps to the include-deleted variant. Test: TestGuestVisibleResourcesIncludeDeleted_SurfacesTombstones covers both variants side-by-side — live drops the soft-deleted grant, include-deleted preserves it. * fix(store): assign per-row unique seqs in MigrateItemFieldValues per Codex review (round 2) Codex round 2 caught that the bulk UPDATE inside MigrateItemFieldValues gave every affected row the SAME MAX(seq)+1. A /items-changes?limit=N poll that cut through that equal-seq group would advance the cursor to the shared seq, and the next `seq > cursor` poll would silently miss the rest of the group — the cursor contract requires strict monotonicity. Switched to a per-row loop inside the migration transaction so every UPDATE re-reads MAX(seq) and each affected row ends up with a strictly unique seq. The workspace advisory lock makes the read-modify-write race-free on Postgres; SQLite's single-writer rule handles it implicitly. Trade-off: O(N) statements instead of O(1) for the bulk path. Option-rename is an admin one-off so the cost is acceptable (~1s/1000 rows on a warm SQLite connection). If future use cases demand a larger row budget, a single-statement UPDATE..FROM with ROW_NUMBER() CTE assigning per-row seqs would also work. Test: TestMigrateItemFieldValues_PerRowUniqueSeq confirms 5 rows in a single migration step all get unique seqs.	2026-05-11 13:42:11 -04:00
xarmian	974472799a	feat(api): wire real workspace seq into /items-index cursor + rows (TASK-1353) (#493) ... * feat(api): wire real workspace seq into /items-index cursor + rows (TASK-1353) Replaces the placeholder `updated_at`-derived cursor on the /items-index response with the real workspace-scoped MAX(seq) introduced by TASK-1352. Each returned row carries its own `seq` field so clients can reason about ordering without parsing the cursor. When the requested scope returns zero rows but the workspace has items (e.g. ?collection=docs on a workspace whose docs collection is empty but whose tasks/ideas are not), the cursor falls back to the workspace's true MAX(seq) via a new Store.MaxItemSeq helper. That way the client's next /items-changes?since=cursor poll starts at the right floor instead of replaying every prior mutation from 0. Empty workspaces collapse to "0". Encoding: cursor is the decimal-encoded MAX(seq). Treated as opaque on the wire (clients re-pass it as ?since=). String form leaves room to switch to base32/etc later without an API break. TypeScript: `ItemIndexRow` (via `Item`) adds optional `seq?: number`; `ItemIndexResponse.cursor` docstring updated to reflect the real seq cursor semantics. `api.items.listIndex` docstring updated. Tests: - TestListItemsIndex_SkinnyProjectionAndShape: cursor now asserts decimal-encoded MAX(seq); per-row seq is non-zero. - TestListItemsIndex_EmptyResultFallsBackToWorkspaceMax: new test covering the cursor fallback on filtered-but-empty results. - TestListItemsIndex_CursorMonotonicAcrossMutations: new test confirming cursor advances after every mutation. Parent: PLAN-1343. Depends on TASK-1352 (seq column). Unblocks TASK-1354 (/items-changes endpoint). * fix(api): snapshot workspace MAX(seq) before list to close cursor race per Codex review (round 1) Codex round 1 caught a real race in /items-index cursor computation: ListItemsIndex ran first, then MaxItemSeq ran in a separate query. A concurrent INSERT visible to a future /items-changes call could land between them — the response would be `items: []` with cursor = the new seq, and a subsequent /items-changes?since=cursor poll (seq > cursor) would never return that row. Fix: capture MaxItemSeq BEFORE the list query. Per the workspace's monotonic counter invariant (TASK-1352) any insert after that snapshot has seq > captured M, so /items-changes?since=M will see it. Rows the list DOES observe may have seq > M (a concurrent insert the list query happened to commit-snapshot); MAX(rows.seq) bumps the cursor for that case so the client never re-fetches what was already in the response. Long-form comment on the handler captures the race scenario and the invariant that makes the snapshot order safe.	2026-05-11 13:09:49 -04:00
xarmian	d6894def4f	feat(web): collection page fetches via skinny /items-index endpoint (TASK-1349) (#491) ... * feat(web): collection page fetches via skinny /items-index endpoint (TASK-1349) Replaces every \`api.items.listByCollection(ws, coll)\` call in the collection page with \`fetchSkinnyItems(ws, coll, includeArchived)\`, which calls the local-first \`/items-index\` endpoint (TASK-1344) through the typed client wrapper (TASK-1345). Items now ship without the rich-text \`content\` body — the bulk of the per-row wire size — until the user opens an item detail page, which still goes through its existing full-item fetch. Call sites updated: - loadCollection — primary load + plans-names lookup - SSE handler for item_created / item_archived / item_restored / item_updated - Sync coordinator's full-refresh fallback The skinny rows are widened to \`Item[]\` at the boundary by setting \`content: ''\` on each row. This keeps the existing view component type contract unchanged and means existing call sites that read \`item.content\` see an empty string — already a "nothing to do" sentinel in the markdown-checklist progress branch. Documented regression — out of scope for this task: non-plans collections used to display checklist progress derived from item content's markdown checkboxes. With \`content\` no longer fetched for the list view, that progress no longer appears. Plans progress is unaffected (uses /plans-progress, not content parsing). Re-introducing the feature requires either server-side progress on the index endpoint or a separate lazy fetch — a follow-up rather than a blocker for the bandwidth win. In-scope behavior preserved: - Item create/update flow: server still returns full items, dropped into the array as-is; sync coordinator's incremental updates similarly use the full-item type from the changes feed - Server-side FTS search via \`searchResultIds\`: still id-keyed, works against skinny rows - List / Board / Table view components: already only read fields present on the skinny row (title, fields, tags, sort_order…) - Detail page fetch: unchanged — still goes through \`api.items.get\` which returns the full Item with content Parent: PLAN-1343. * fix(api+web): add /collections/{coll}/checkbox-progress endpoint to preserve list-view checklist progress per Codex review (round 1) Codex round 1 [P2] flagged that the original PR shipped a real regression: non-plans collections used to compute markdown-checkbox progress client-side from `item.content`, and the skinny `/items-index` endpoint dropped `content` from the payload — so list/board/table progress badges silently stopped appearing on docs/tasks/custom collections. This commit closes that gap with a new server endpoint that computes the same `{item_id, total, done}` counts via LENGTH/REPLACE arithmetic on the stored content, returning only the small derived counts. No item bodies cross the wire. Server (Go): - `store.CollectionCheckboxProgress(workspaceID, collectionID)` — SQL: `(LENGTH(content) - LENGTH(REPLACE(content, '- [ ]', ''))) / 5 + (LENGTH(content) - LENGTH(REPLACE(content, '- [x]', ''))) / 5` for total, the second clause alone for done. Same trick on SQLite and PostgreSQL. - `handleCollectionCheckboxProgress` — collection-visibility + item-grant filter so guests / restricted members can't enumerate items they shouldn't see. Mirrors `guestResourceFilter` exactly. - Route: `GET /api/v1/workspaces/{ws}/collections/{coll}/checkbox-progress`. - Test `TestCollectionCheckboxProgress` covers the math (open + done counts), zero-result rows are filtered, unknown collection → 404, empty result → 200 + `[]`. Web: - `api.items.collectionCheckboxProgress(ws, coll)` - Both call sites in `+page.svelte` (initial `loadCollection` non-plans branch + `refreshProgress` non-plans branch) now pull from the endpoint instead of parsing `item.content`. - Drops the previous "documented regression" comment — the feature is fully preserved. Sub-100-byte response per item (vs. the full content body) so the bandwidth win from `/items-index` is preserved. The endpoint scans content server-side, but doesn't transmit it — the original listByCollection call both scanned AND transmitted content. Parent: PLAN-1343. * fix(api+web): plumb include_archived through checkbox-progress per Codex review (round 2) Codex round 2 [P2] caught that the Archived toggle path lost checklist progress badges: `CollectionCheckboxProgress` hard-coded `deleted_at IS NULL`, but the page-side fetch is called with the same `showArchived` flag that toggles whether archived items render. With the toggle on, archived non-plan items appeared in the list but had no `itemProgress` row — the old client-side parse would have counted them. Fix: thread `includeArchived` through the call chain. - store.CollectionCheckboxProgress(workspaceID, collectionID, includeArchived bool) — appends `AND deleted_at IS NULL` only when includeArchived is false. Default match the original archived-off behavior. - handleCollectionCheckboxProgress reads ?include_archived=true and forwards. - api.items.collectionCheckboxProgress(ws, coll, { includeArchived }) on the client. - +page.svelte's two call sites pass `showArchived` / `includeArchived` exactly. TestCollectionCheckboxProgress now archives one of the seeded items and asserts: - default response excludes the archived item (1 row) - ?include_archived=true response includes it (2 rows) Also clarified the const-doc on `checkboxCountSQL` to reflect the dynamic deleted-at clause.	2026-05-11 11:51:06 -04:00
xarmian	e83e7ef413	feat(api): add /items/index skinny-projection endpoint (TASK-1344) (#486) ... * feat(api): add /items/index skinny-projection endpoint (TASK-1344) Foundation for PLAN-1343 (local-first read model). Adds a new GET /api/v1/workspaces/{ws}/items/index endpoint that returns every item in a workspace minus the rich-text `content` body, so the client can hydrate an in-memory + IndexedDB index from a single request and render every collection page from local state without re-fetching. Response: {items, total, cursor}. The cursor placeholder is the max(updated_at) across the result set — Phase 2 replaces it with a monotonic `seq` cursor. Sort is updated_at DESC, id ASC for deterministic, cursor-friendly ordering. Auth uses the same collection-visibility + item-grant filter as handleListItems. Optional ?collection=<slug> filter for use by collection pages. ?include_archived=true mirrors the existing list behavior. Parent: PLAN-1343. * fix(api): move skinny-projection endpoint to /items-index per Codex review (round 1) Codex round 1 [P2] flagged that the original `/items/index` path shadowed the detail URL of any item whose slug is `index` — slugify emits `index` for a title of "Index", and chi's static-over-wildcard preference would route `GET /items/index` to the new index handler instead of the existing `/items/{itemSlug}` detail handler. Move the endpoint up to the workspace level as `/items-index`, sibling to the existing `/plans-progress` route. Slugs cannot contain hyphens adjacent to identifiers in a way that would collide with a static workspace-level path, so this URL space is permanently safe. New test `TestListItemsIndex_DoesNotShadowItemSlug` locks in the contract: a real item titled "Index" still resolves through `/items/{itemSlug}`, while `/items-index` returns the index wrapper.	2026-05-11 09:34:15 -04:00
xarmian	bdcb62e902	fix(api): accept nested object/array for PATCH items fields/tags (BUG-1144) (#485) ... The PATCH /api/v1/workspaces/{ws}/items/{ref} endpoint previously demanded `fields` and `tags` arrive as JSON-encoded strings, because models.ItemUpdate declares them as *string to mirror the storage shape. Sending the natural nested-object shape any reasonable HTTP client would produce returned HTTP 400 with a leaked Go unmarshal error naming the internal struct field — confusing for anyone integrating against Pad over HTTP (webhook reactors, custom dashboards, non-CLI agents, third-party MCP bridges). This is the symmetric input-side counterpart to BUG-991, which was fixed at the MCP boundary in PR #364 with dual-emit normalization rather than the full Plan-sized models.Item migration. Fix: add a custom ItemUpdate.UnmarshalJSON that accepts either shape on the wire and normalizes to the canonical string internally. The struct field type stays *string, so the validation/storage/web/CLI pipeline is untouched. All in-process Go callers construct ItemUpdate literals (15 grepped call sites) and never hit UnmarshalJSON, so the change is invisible to them. Wrong shapes (e.g. `{"fields":42}`, `{"tags":{"x":1}}`) now return a domain-level 400 — `"fields" must be a JSON object or a JSON-encoded string` — surfaced via sentinel errors (ErrInvalidFieldsType / ErrInvalidTagsType) that the handler unwraps from decodeJSON's "invalid JSON: %w" wrapper. Coverage: - models/item_test.go: 10 sub-tests covering object, array, string, null, absent, and wrong-type cases for both fields and tags. - server/handlers_items_test.go: 6 PATCH integration sub-tests asserting back-compat, the BUG-1144 repro now returns 200, and that error responses no longer leak Go struct field names. Smoke-tested against the live server with the exact repro curl from BUG-1144 (HTTP 200), plus malformed (HTTP 400 with clean message) and stringified-string back-compat (HTTP 200).	2026-05-10 23:02:48 -04:00
xarmian	18087463ce	feat(collab): op-log cursor protocol — force-refresh + watermark advance (TASK-1319) (#472) ... * feat(collab): op-log cursor protocol — force-refresh + watermark advance (TASK-1319) Closes both holes left by TASK-1309: 1. Long-disconnected tab + external-write race. A reconnecting client announces its highest applied item_yjs_updates.id via `?since=<id>`. If that id is below MIN(id) for the item, rows it expected to replay have been pruned and the server sends a `force_refresh` control frame and closes the conn. Client recreates the Y.Doc and lazy-seeds from items.content. Without this, Tab A's stale state would silently overwrite an external CLI/MCP write on the next 5s flush. 2. Browser-only-edited items never GC'd. Browser collab-snapshot PATCHes now carry an op_log_cursor body field. The store advances items.content_flushed_op_log_id only when the cursor matches the current MAX(op-log.id) — proving the markdown captures every persisted op. SQL CASE clause re-evaluates MAX at COMMIT time so a peer op landing between client-side cursor capture and the UPDATE leaves the watermark untouched (no over-advancement). Combined cursor mechanism: - Server attaches op_log_cursor JSON control frames after replay, after every successful AppendYjsUpdate (originator), and to every peer's binary fan-out (so all peers stay in lockstep without a round trip). - Client persists per-tab in sessionStorage (NOT localStorage — avoids cross-tab cursor leakage that would force-refresh stable sessions). - Server's MIN(id) check + force_refresh fires only when a non-zero `since` is below MIN; `since=0` is treated as a fresh client. New store methods: MinOpLogID, MaxOpLogID. New ItemUpdate field: OpLogCursor *int64. New control message types: op_log_cursor, force_refresh. New OpEvent.OpLogID for cursor piggyback. Existing collab tests updated to drain TextMessage cursor frames. Tests cover: initial cursor frame after replay (populated + empty op-log), force_refresh fires when since<MIN, delta replay when since>=MIN, cursor broadcast to originator + peers on append, and watermark advancement gated on cursor==MAX. Parent: PLAN-1248. Builds on TASK-1309. * fix(collab): skip stale-Ydoc flush on force_refresh teardown per Codex review (round 1) A force_refresh tear-down means the local Y.Doc cursor is below the server's MIN(item_yjs_updates.id) — its derived markdown is stale. Without this guard the collab $effect cleanup runs flushCollabNow on the way out and silently PATCHes that stale markdown back to items.content, overwriting the canonical content the fresh provider is supposed to lazy-seed from. Per Codex round 1 [P1] of TASK-1319. * fix(collab): force_refresh on empty op-log + cancel pending flush per Codex review (round 2) Two P1 fixes: 1. Manager.Join now force_refreshes when since>0 and the op-log is empty (hasMin==false), not just when since<MIN. After PruneAndApply wipes the entire op-log, MIN is undefined; the original predicate would have admitted the stale tab and let its on-open Y.encodeStateAsUpdate write resurrect the pre-prune document. 2. The +page.svelte onForceRefresh handler now also clears collabFlushTimer. Without this a 5s timer that armed before the force_refresh frame arrived can still fire AFTER the cleanup ran, PATCHing stale Y.Doc-derived markdown to items.content. New test: TestRoomManagerForceRefreshOnEmptyOpLogWithSince covers the empty-op-log branch. Per Codex round 2 [P1] of TASK-1319. * fix(collab): include forceRefreshNonce in Editor key so it remounts on force_refresh per Codex review (round 3) The collab $effect cleanup runs on forceRefreshNonce bump, but the <Editor> {#key} was `${item.id}:true` — itemID doesn't change, so the keyed Editor wasn't unmounting. The Tiptap Collaboration extension only binds in onMount, so the editor stayed wired to the stale (destroyed) Y.Doc while a fresh provider+doc were set up in parallel. Edits would either be unsynced or eventually flush stale markdown again. Adding forceRefreshNonce to the key forces the Editor to remount in lockstep with the doc swap. Per Codex round 3 [P1] of TASK-1319. * fix(collab): refetch item.content before lazy-seed on force_refresh per Codex review (round 4) After force_refresh the collab $effect rebuilds the Y.Doc and the lazy-seed (TASK-1261) seeds it from item.content. But item.content was the cached page-state copy — possibly stale relative to the server (the WS force_refresh can beat the SSE/visibility refresh that would otherwise update it). Lazy-seeding stale content into a fresh op-log re-introduces exactly the staleness force_refresh was supposed to clear: the next 5s flush PATCHes that stale view back to canonical items.content. onForceRefresh now does an api.items.get() before bumping the nonce so the rebuild's lazy seed reads server-fresh content. A failed fetch falls through to the bump anyway (an editor on possibly-stale content is still better than a broken editor). Per Codex round 4 [P1] of TASK-1319. * fix(collab): suppress cursor during replay + move force_refresh check before getOrCreate per Codex review (round 5) Two more findings: 1. [P1] writeLoop sends op_log_cursor frames for live ops broadcast during the replay window. A client disconnecting after one of those cursors lands but BEFORE the rest of replay completes would persist a cursor pointing past unreplayed rows. On reconnect with since=that-cursor, server replays nothing — the client's Y.Doc would be missing causally-required ops. Fix: per-roomConn replayDone atomic.Bool. writeLoop suppresses cursor frames while it's false. runConn flips it after the post-replay initial cursor is on the wire. Live binary frames continue to flow during replay (Yjs CRDT commutativity); only the cursor metadata is gated. 2. [P2] Force-refresh path leaked an empty room. getOrCreate inserted into m.rooms before the force_refresh bail-out left an orphan entry that PruneSweep would later treat as 'active' and skip indefinitely. Fix: schema-rebuild + force_refresh checks now run BEFORE getOrCreate. Both are store-only mutations and the per-item lock is held throughout, so concurrency is unchanged. New test: TestRoomManagerCursorSuppressedDuringReplay regression- guards the cursor-suppression behaviour. Per Codex round 5 [P1+P2] of TASK-1319. * fix(collab): tighten initial cursor + sync-destroy provider on force_refresh per Codex review (round 6) Two more P1 fixes: 1. runConn's empty-replay fallback used MaxOpLogID() to anchor the initial cursor. A live op landing between replayTo returning and the cursor write would be reflected in MAX but its binary frame might not have flowed through this conn's writeLoop yet — the cursor would advertise an id the client hasn't received. Initial cursor is now strictly max(highestReplayed, since); MaxOpLogID is removed from the opLogStore interface. 2. Provider.handleControlMessage's force_refresh branch now calls this.destroy() SYNCHRONOUSLY before invoking the onForceRefresh callback. Previously the consumer's recovery path (async items.get refetch) would race the provider's own onClose-triggered reconnect, which would re-open with since=0 and push Y.encodeStateAsUpdate of the stale Y.Doc — recreating the corruption force_refresh was meant to prevent. destroy() sets destroyed=true so scheduleReconnect short-circuits. Per Codex round 6 [P1] of TASK-1319. * fix(collab): block flush scheduling during force_refresh recovery per Codex review (round 7) Previously, after onForceRefresh fires: 1. Provider is destroyed synchronously. 2. Async items.get refetch is in flight. 3. forceRefreshNonce bumps after refetch resolves. 4. $effect cleanup runs, then rebuild. But during steps 2-3 the editor component is still mounted with the stale Y.Doc, and a local edit fires handleContentUpdate which calls scheduleCollabFlush. clearTimeout earlier in onForceRefresh only canceled the timer at THAT moment; a new edit during the refetch window arms a fresh timer that fires before cleanup. That PATCHes stale Y.Doc-derived markdown back to canonical content, recreating the corruption force_refresh was meant to prevent. Fix: forceRefreshInFlight flag set in onForceRefresh, blocks scheduleCollabFlush, resets after the fresh provider is wired (end of $effect run). Per Codex round 7 [P1]. * fix(collab): gate runCollabFlush itself on force_refresh in-flight per Codex review (round 8) scheduleCollabFlush blocked the 5s timer path, but direct callers of flushCollabNow / runCollabFlush (beforeunload handler, rich-to-raw toggle) bypassed the guard. A page reload or raw toggle DURING the force_refresh recovery window still PATCHed stale Y.Doc-derived markdown to canonical items.content. Pulling the guard into runCollabFlush covers every caller in one spot and returns 'deduped' so the result-shape contract holds. Per Codex round 8 [P1] of TASK-1319. * fix(collab): distinct 'skipped' result for force_refresh path; raw-toggle aborts per Codex review (round 9) runCollabFlush returning 'deduped' on the force_refresh-blocked path was indistinguishable from a legitimate same-content dedupe. The rich→raw toggle treats 'deduped' as 'server already has this markdown' and seeds rawSeedMarkdown from it — letting the user's next raw edit overwrite canonical items.content with content derived from the stale Y.Doc. Add a distinct 'skipped' result for the force_refresh path. Raw toggle aborts on it (with a 'try again in a moment' toast); other callers fall through unchanged because no other call site behaviorally depends on 'deduped' vs 'skipped'. Per Codex round 9 [P1] of TASK-1319. * fix(collab): server-side gate + post-await client guard against stale collab-snapshot per Codex review (round 10) A force_refresh frame can arrive WHILE a collab-snapshot PATCH is already mid-flight to the server. The client-side forceRefreshInFlight check at PATCH-start can't catch this race; the request lands at the server with stale Y.Doc-derived markdown. Two-pronged fix: 1. Server: handler now checks op_log_cursor against MIN(op-log.id) for collab-snapshot PATCHes and returns 409 Conflict when cursor < MIN. Such cursors prove the flushing tab's Y.Doc was built on rows that have been pruned (PruneAndApply, schema rebuild, dormant GC). The markdown is, by construction, stale. 2. Client: post-await check on forceRefreshInFlight returns 'skipped' instead of 'flushed' so saveStatus / lastFlushedContent don't seed from a known-stale base even if the server happened to accept the PATCH (e.g. MIN advanced after handler validation). New tests: TestCollabSnapshotRejectsCursorBelowMin (gate fires), TestCollabSnapshotAcceptsCursorAtOrAboveMin (negative path). Also de-leak an unused slice in the round-5 cursor-suppression test so staticcheck stays clean. Per Codex round 10 [P1] of TASK-1319. * fix(collab): reject collab-snapshot when cursor>0 and op-log empty per Codex review (round 11) The HTTP-layer gate I added in round 10 mirrored only PART of the WS-upgrade force_refresh predicate. Round 5 had already taught us that 'op-log entirely pruned' is a separate stale path from 'cursor below MIN' (PruneAndApply, schema rebuild, dormant GC all leave hasMin=false), and the WS check now uses `since > 0 && (!hasMin || since < minID)`. The HTTP gate had only the second clause. Mirror the WS predicate at the handler so a stale collab-snapshot PATCH against an empty op-log gets a 409 too. New regression: TestCollabSnapshotRejectsCursorOnEmptyOpLog. Per Codex round 11 [P1] of TASK-1319. * fix(collab): reject collab-snapshot cursor=0 on non-empty op-log per Codex review (round 12) Round-11 gate accepted cursor=0 unconditionally. But a stateful tab whose previous session disconnected BEFORE receiving the post-replay cursor frame (network blip during the writeMu burst between replay binaries and the cursor) ends up with sessionStorage cursor=0 + a non-empty Y.Doc populated by prior replay binaries. On reconnect with since=0 the server treats it as fresh, replays nothing if the op-log was meanwhile pruned, and the client's on-open Y.encodeStateAsUpdate resurrects pre-prune ops. The next flush carries cursor=0 + stale-derived markdown. The gate now refuses any incompatible cursor: - cursor>0 + empty op-log (prior rule) - cursor<MIN + non-empty op-log (prior rule, now naturally catches cursor=0 too because 0 < any positive MIN) The WS replay path is unchanged — full replay from since=0 is the recovery for clients that genuinely lost their cursor; the corruption manifested through the flush PATCH which we now gate. New test: TestCollabSnapshotRejectsCursorZeroOnNonEmptyOpLog. Per Codex round 12 [P1] of TASK-1319. * fix(collab): close cursor=0 client/server gaps + lock validation+write atomically per Codex review (round 13) Four P1 issues addressed: 1. Client always sends op_log_cursor (including 0) so the server gate sees the field. Previously cursor=0 was omitted, which silently bypassed the server's stale-snapshot rejection. 2. Provider construction now resets sessionStorage cursor to 0 when the Y.Doc is empty. The Y.Doc isn't persisted across page reload, so a stored cursor=N + fresh empty Y.Doc would announce since=N to the server and miss rows 1..N from replay (server only replays id > N). 3. onOpen skips Y.encodeStateAsUpdate when lastOpLogID === 0. A populated Y.Doc + cursor=0 is the network-blip-during-cursor- write failure mode; pushing that state can resurrect ops the server has pruned. Server replay + lazy-seed handle recovery without our push. 4. Server gate now runs INSIDE the per-item collab setup lock (new RoomManager.UnderItemLock helper) so a concurrent prune (PruneAndApply, schema rebuild, dormant GC) cannot land between the MIN check and the items.content write. Without this, a tight race let stale snapshots overwrite canonical content the prune just installed. Per Codex round 13 [P1] of TASK-1319. * fix(collab): gate handleDocUpdate on cursorAnchored to close stale-Ydoc edit path per Codex review (round 14) Round 13 fix skipped on-open send for lastOpLogID===0, but local edits via handleDocUpdate still propagated. A populated Y.Doc + no-cursor-yet client could type, the edit would land in the op-log with id N, server would send originator cursor=N, and the next 5s flush would carry an 'anchored' cursor that passed the server's MIN check — overwriting items.content with stale- Y.Doc-derived markdown. Add a cursorAnchored boolean. Set on first op_log_cursor frame receipt (including cursor=0 against an empty op-log — that's a legitimate 'server has nothing' signal). handleDocUpdate refuses to send before this. Local edits buffer in the editor; once the cursor arrives (or force_refresh rebuilds the provider), the existing reconnect/edit paths catch them up. Per Codex round 14 [P1] of TASK-1319. * fix(collab): buffer + flush pre-anchor local updates per Codex review (round 15) Round 14 silently dropped local Yjs updates fired before the first op_log_cursor frame anchored the session. Yjs updates are incremental: a dropped keystroke leaves later ops referencing structs no peer can resolve, breaking convergence. Buffer pre-anchor updates in a Uint8Array[] (capped at 1000 to prevent unbounded growth in pathological 'anchor never arrives' scenarios — overflow triggers force_refresh-style recovery). On the first cursor frame, flush the buffer in order so the server gets every causally-required struct before any post- anchor updates land. Per Codex round 15 [P1] of TASK-1319. * fix(collab): destroy provider before force_refresh on pre-anchor buffer overflow per Codex review (round 16) Round 15 overflow path called onForceRefresh but didn't destroy the provider synchronously. A late op_log_cursor arriving before the page-level rebuild (the recovery callback is async — refetches items.content) would flip cursorAnchored=true, the partially- populated buffer would flush, but the DROPPED prefix (the overflowed entries) would leave server-side ops causally incomplete — exactly the bug the buffer was supposed to prevent. destroy() sets destroyed=true, removes message listener, short-circuits scheduleReconnect, closes the socket. Late cursor frames can no longer anchor a doomed provider. Per Codex round 16 [P2] of TASK-1319. * fix(collab): refuse rebuild on refetch fail + broaden on-open gate to cursorAnchored per Codex review (round 17) Two findings: [P1] force_refresh recovery bumped forceRefreshNonce in finally even when the item.content refetch failed. The rebuild then lazy-seeded from the cached (possibly-stale) item.content, and the next flush would PATCH that stale view back to the server. Move the bump into .then() so a failed refetch surfaces a 'please reload' toast and leaves the editor effectively read-only (forceRefreshInFlight stays true, blocking flushes). [P2] Send-on-open gate was lastOpLogID > 0, which silently dropped local edits made during a brief offline window after a legitimate 'cursor=0' anchor (empty op-log session). Switch to cursorAnchored — the boolean specifically distinguishes 'unanchored' (stale Y.Doc + no server confirmation) from 'anchored at cursor=0' (legitimate empty op-log). Per Codex round 17 [P1+P2] of TASK-1319. * fix(collab): force_refresh on cursor=0 against non-empty Y.Doc per Codex review (round 18) cursor=0 means the server's op-log is currently empty. A non-empty Y.Doc at first-cursor receipt implies the ops came from an earlier connection within this provider's life that never reached its post-replay cursor frame, followed by a server-side prune (PruneAndApply, schema rebuild, dormant GC) during our disconnect. Anchoring at cursor=0 in that state would mark a stale Y.Doc as authoritative; the next on-open state push or flush would resurrect pre-prune state and overwrite canonical items.content. Detect the configuration via Y.encodeStateVector length and invoke the same force_refresh-style recovery the explicit server frame triggers: destroy provider, clear sessionStorage, fire onForceRefresh so the page rebuilds from items.content. Per Codex round 18 [P1] of TASK-1319. * fix(collab): gate cursor=0 force_refresh on remoteSyncApplied per Codex review (round 19) Round 18 force_refreshed the provider whenever cursor=0 arrived against a non-empty Y.Doc. But local pre-anchor edits (user typed before the initial cursor=0 of a legitimate empty-op-log session arrived) ALSO populate Y.Doc — yet those edits live in preAnchorUpdates and were supposed to flush on anchor. The predicate spuriously triggered force_refresh, dropping the buffered local edits. Track remoteSyncApplied (set when readSyncMessage applies anything to Y.Doc — replay binary or live peer op). Only force_ refresh on cursor=0 when remoteSyncApplied is true: that's the true 'remote replay landed but server now reports empty op-log => mid-session prune' signature. Per Codex round 19 [P1] of TASK-1319. * fix(collab): repair brace mis-merge in wsProvider cursor=0 guard The round-19 patch overlapped the round-18 inner block, producing an extra brace + over-indented body. Collapsing into a single clean block restores parseability without changing semantics beyond what round 19 already documented. * fix(collab): gate syncStep2 reply on cursorAnchored per Codex review (round 20) readSyncMessage writes an inline syncStep2 reply when it receives a peer's syncStep1. That reply embeds our current Y.Doc state. If a peer's syncStep1 arrives before our first op_log_cursor (pre-anchor window), the reply path bypasses handleDocUpdate's cursorAnchored gate and lets potentially-stale Y.Doc state reach the server before the cursor=0 + remoteSyncApplied force_refresh recovery has a chance to fire. Suppress the reply while unanchored. Peer state propagation still works: the buffered preAnchorUpdates flush on anchor, and the lazy-seed rebuild after a force_refresh seeds canonical content from items.content. Per Codex round 20 [P1] of TASK-1319. * fix(collab): fold mid-replay live op ids into post-replay cursor + remoteSyncApplied only on apply per Codex review (round 21) Two more findings: [P1 server] writeLoop suppresses cursor frames during replay to prevent the client persisting a cursor past unreplayed rows. But binary frames for those live ops still go through (commutativity), so the client APPLIES them to its Y.Doc. The post-replay initial cursor only covered max(highestReplayed, since), leaving the cursor below the highest applied op. On empty-replay sessions this trips the client's 'cursor=0 + remoteSyncApplied' force_refresh path and discards buffered pre-anchor edits. Track maxLiveOpLogIDDuringReplay on the roomConn (atomic compare-and-swap) and fold it into the post-replay cursor. [P1 client] remoteSyncApplied was set on every MESSAGE_SYNC, including syncStep1 (which only carries a state vector — it doesn't apply state). A peer's syncStep1 arriving pre-anchor would falsely flag remote-sync-applied and trip the cursor=0 force_refresh on legitimate empty-op-log sessions. Set the flag only after readSyncMessage returns, and only for syncStep2 / update subtypes. Per Codex round 21 [P1] of TASK-1319. * fix(collab): widen writeMu critical section + drop omitempty on op_log_id per Codex review (round 22) Two more P1s: [P1 server] writeLoop's mid-replay record-max happened OUTSIDE writeMu, so runConn's post-replay read could race the record: runConn loads → writeLoop's atomic store of higher value → runConn sends cursor below the live id. Move the entire per-event sequence (binary write + replayDone observation + record-or-send) inside writeMu, and have runConn acquire writeMu around its read+cursor-write+replayDone-flip. The lock serializes the two paths cleanly: writeLoop events that ran first have already recorded; events that arrive after replayDone flips emit their own cursor frames. [P1 protocol] OpLogID had `omitempty` JSON tag — a legitimate cursor=0 (empty op-log session) serialized as `{"type":"op_log_cursor"}` with no op_log_id field. The client's strict-type check then rejected it as malformed, leaving the session unanchored and local edits buffered forever. Drop omitempty so 0 is wire-visible. Other control types (applier_request/ack) carry an extra op_log_id:0 in their JSON, which their client dispatches ignore. Per Codex round 22 [P1] of TASK-1319. * fix(collab): route originator cursor through writeLoop FIFO per Codex review (round 23) readLoop sent the originator's op_log_cursor directly via sendOpLogCursor right after AppendYjsUpdate, bypassing the bus/ writeLoop ordering. With a peer op already queued in rc.bus, the sequence on the wire could be: 1. originator cursor=N (newer local op) 2. peer binary (older op) 3. peer cursor=M < N (rejected by client's max-take logic) Client persists cursor=N. If the client then disconnects before applying the peer binary, reconnect with since=N replays nothing (server has nothing > N) and the older peer op is lost forever to this client's Y.Doc. Fix: writeLoop now processes self events too — skipping the binary echo (the originator already has Y.Doc state) but routing the cursor frame through the same FIFO bus channel as peer ops. The originator's cursor=N now arrives strictly AFTER all older-id peer events on the same channel. Per Codex round 23 [P1] of TASK-1319.	2026-05-09 21:45:46 -04:00
xarmian	028db39217	feat(collab): periodic op-log GC sweeper for dormant items (TASK-1309) (#471) ... The Yjs collab dumb-relay accumulates op-log rows indefinitely in item_yjs_updates. DOC-1307 surfaced 45-second p50 cold-reconnect latency on a single item with 5000 accumulated rows. Without GC, busy items keep growing. This adds a periodic background sweeper that prunes the entire op-log for items that are both DORMANT (no recent activity) AND FULLY FLUSHED (items.content has captured every op-log row). Whole-log only — Yjs op streams are causally linked, prefix-pruning corrupts replay; future cold connects lazy-seed from items.content. Components: - Store.ListDormantOpLogItemsBefore (joins items, filters watermark) - Store.PruneItemOpLogIfDormantBefore (atomic conditional DELETE) - Store.GetItemContentFlushedOpLogID (per-item watermark getter) - RoomManager.PruneSweep (per-item-locked, active-room-skip) - Server.StartOpLogGC / stopOpLogGC (mirrors orphan_gc.go pattern) - cmd/pad/main.go env vars PAD_OPLOG_GC_INTERVAL / PAD_OPLOG_GC_MIN_AGE - New (item_id, created_at) index for the dormancy query - New items.content_flushed_op_log_id column (id-based watermark, monotonic, no clock-skew or second-granularity false positives) + content_flushed_at (informational timestamp) Watermark policy: - Server-driven full-content writes (CLI / MCP / version restore / PruneAndApply) advance content_flushed_op_log_id to MAX(op-log.id) via subquery, atomic with the content UPDATE - Browser collab-snapshot 5s flushes do NOT advance the watermark — they can't prove their markdown captured every peer's ops, so letting them stamp would risk later GC-pruning unsynced peer edits - Schema-mismatch rebuild (TASK-1268) logs a WARN when it drops unflushed ops (data loss is unavoidable on schema bumps but visible) Stop ordering: collab.Close() now runs BEFORE bg.Wait() so a GC goroutine waiting on an itemLock behind an active Join can drain. Migration backfill: items WITH existing op-log rows keep NULL watermark (don't certify); items WITHOUT op-log rows get a synthetic 0 watermark (vacuous, harmless — no rows to compare against). Tests: - 6 RoomManager.PruneSweep tests (dormant prune / default minAge / empty / bails-on-Close / skips-active-room / skips-row-added-mid- sweep via fakeOpLog hook) - 5 Server.OpLogGC tests (prunes-dormant / start-idempotent / preserves-unflushed / backfill-doesnt-certify-unflushed / no-collab-noop) - TestCollabSnapshotDoesNotAdvanceOpLogWatermark in store - TestCollabSnapshotQueryOverridesBodyVersionSource in server (regression for body-attacker bypass) Seven rounds of Codex review — caught 5 P1s and 4 P2s I would have shipped under self-review: 1. Prefix-prune corrupts Yjs replay 2. Stop ordering deadlock 3. Missing index 4. Best-effort flush ⇒ data loss 5. Backfill over-certifies via metadata-PATCH 6. Second-granularity timestamp comparison 7. Schema-mismatch path drops unflushed silently 8. Browser flush stamps watermark beyond Y.Doc 9. Body version_source bypasses server policy	2026-05-09 18:19:33 -04:00
xarmian	9b46be915a	feat(collab): schema-version handshake + mismatch rebuild (TASK-1268) (#466) ... Adds a client→server schema-version handshake on every WS connect and a per-item op-log rebuild path for the case where the server ships a new SCHEMA_VERSION and finds older rows persisted in the op-log. Client side - New web/src/lib/collab/schemaVersion.ts exporting `SCHEMA_VERSION` (currently '1') with a documented bump rule covering Tiptap extension changes, coordinated multi-package bumps, and Y.Doc fragment-shape changes. - wsProvider's defaultCollabUrl appends ?schema_version=... Server side - handlers_collab.go validates ?schema_version against RoomManager.SchemaVersion() BEFORE upgrading the WS; mismatch returns HTTP 400 with code "schema_mismatch". An empty query is treated as legacy '1' for graceful deploys; once the server bumps past v1, missing query becomes a 400 too. - New RoomManager.SchemaVersion() getter. - RoomManager.Join's setup-phase (under itemLock) now calls maybeRebuildOnSchemaMismatch: if the latest persisted op-log row's schema_version disagrees with the manager's current version, the entire item op-log is pruned via PruneYjsUpdatesBefore. items.content is canonical and untouched, so the lazy-seed path (TASK-1261) re-encodes it into ops at the new schema on the next idle tick. - New store method LatestYjsUpdateSchemaVersion. Tests - internal/collab/manager_test.go: three new tests (mismatch prunes, clean version preserves op-log, post-rebuild connects are clean) + fakeOpLog gets LatestYjsUpdateSchemaVersion + PruneYjsUpdatesBefore. - internal/server/handlers_collab_test.go: rejects-schema-mismatch (400), accepts-explicit-match (101). One round of Codex review (CLEAN with two NITs, both fixed).	2026-05-09 11:06:34 -04:00
xarmian	191b887e23	feat(versions): VersionSource attribution + collab coexistence (TASK-1267) (#465) ... The collab 5s-flush PATCH (TASK-1260) sends `?source=collab-snapshot` with a body of just `{ content }`. Without a handler-side stamp, `Store.UpdateItem`'s default coerced empty input.Source to "web" on the version row and the per-(actor, source) throttle suppressed every collab-driven snapshot following the user's last manual web edit — version-diff effectively went silent during co-edit sessions. Adds: - ItemUpdate.VersionSource: overrides per-version-row Source attribution WITHOUT mutating items.source. The latter feeds WorkspaceHasAgentActivity's `source IN ('cli', 'mcp')` filter, so a CLI/MCP-created item the user opens in the editor would otherwise silently flip out of the agent-activity tally on every auto-flush. - Store.UpdateItem prefers VersionSource over Source for version row creation; falls back to Source then "web" if neither set. - handlers_items.go stamps `input.VersionSource = "collab-snapshot"` for `?source=collab-snapshot` PATCHes (when not already set). Tests: - internal/store/items_collab_versions_test.go: store-level reverse-patch reconstruction over a CLI→web→collab-snapshot edit sequence; verifies IsDiff=true on at least one row. - internal/server/handlers_items_collab_versions_test.go: full HTTP-level test of the route; asserts a collab-snapshot version row is created AND that items.source stays "cli". Four rounds of Codex review.	2026-05-09 09:51:48 -04:00
xarmian	fdc6b221d0	test(collab): regression guard for share-page collab isolation (TASK-1266) (#464) ... The public /s/{token} share page must keep rendering markdown via marked + DOMPurify and never open a WebSocket to /api/v1/collab — anonymous viewers can't authenticate, and exposing per-item Y.Doc traffic to the public internet would be a security regression. This is a verification task: zero production code changes. Adds TestSharePageDoesNotImportCollab in internal/server which: - Walks the import closure rooted at the share-page +page.svelte (resolves $lib/ aliases, relative paths, conventional extensions, static AND dynamic `import(...)` forms; skips external packages) - Asserts no file in the closure contains forbidden tokens: wsProvider, CollabProvider, @tiptap/extension-collaboration, @tiptap/y-tiptap, 'yjs' / "yjs", y-protocols, Editor.svelte (suffix), WebSocket, /api/v1/collab - Asserts the route file still imports + invokes marked and DOMPurify.sanitize (catches a renderer swap) - Strips comments before all checks so leftover commented-out imports can't bypass Verified by injection: direct AND transitive AND dynamic-import regressions all fail the test. Four rounds of Codex review.	2026-05-09 07:13:10 -04:00
xarmian	9b1a91ab00	feat(collab): 5s-idle + on-disconnect markdown flush (TASK-1260) (#458) ... * feat(collab): 5s-idle + on-disconnect markdown flush (TASK-1260) Replaces the temporary handleContentUpdate suppression introduced in TASK-1259 (PR #457) with a proper flush mechanism. Under collab, the Y.Doc + op-log are canonical for live state but items.content needs to stay reasonably fresh for downstream consumers (search index, share-page, exports, plain API readers). ## Mechanism 1. **5s idle timer.** Every editor onUpdate (local OR remote) resets a 5s timer. On fire, PATCHes items.content via the new `?source=collab-snapshot` query param. 2. **Server-side bypass.** handleUpdateItem inspects the source query param. When set, skips the applyContentViaCollab routing entirely and writes directly. Without the bypass, the PATCH would loop back through the applier protocol (the same tab gets asked to apply, acks, server strips input.Content) and leave items.content unchanged forever. The flag is trustworthy because the caller already has edit access. 3. **Dedupe across peers.** Track lastFlushedContent. If our last successful flush already landed this exact markdown, skip the PATCH. Multiple connected tabs would otherwise each fire a redundant flush after every shared edit converges, multiplying server load by the peer count. 4. **On-disconnect flush.** $effect cleanup (item swap or page unmount) calls flushCollabNow(true) BEFORE provider.destroy(). A separate beforeunload listener catches close-tab / reload / external-nav. Both use fetch keepalive: true so the request outlives the page lifecycle. 5. **Item-id race guards.** runCollabFlush captures reqItemId before await; ignores response if item swapped. loadData() clears collabFlushTimer + lastFlushedContent on navigation. ## Files - internal/server/handlers_items.go — accept `?source=collab-snapshot` - web/src/lib/api/client.ts — add api.items.flushCollabContent - web/src/routes/.../[slug]/+page.svelte — handleContentUpdate gains scheduleCollabFlush + runCollabFlush + flushCollabNow; wired to $effect cleanup + beforeunload + loadData reset. Parent: PLAN-1248 * fix(collab): capture ws+itemId at provider mount + apply unescapeDocLinks per Codex review (round 1) Two findings from round 1: 1) [P1] runCollabFlush resolved item.id and wsSlug at execution time, not at schedule time. During item navigation the timer could fire (or $effect cleanup could run) AFTER `item` was already updated to the new item, causing the OLD editor's markdown to be PATCHed against the NEW item's URL — cross-item content corruption. Fix: introduce activeCollabContext = { wsSlug, itemId }, captured at $effect-body time (when the provider is minted). scheduleCollabFlush, runCollabFlush, and flushCollabNow all take their target identity from this captured context, never from live reactive state. Cleared in the $effect's own cleanup (defensive `=== ctx` slot guard so a fast-navigation churn doesn't clobber a successor context). 2) [P2] The disconnect flush read raw editor.storage.markdown .getMarkdown() without unescapeDocLinks, unlike the regular onUpdate path. Closing/navigating before the idle flush could persist escaped wiki links like \[\[TASK-1\]\] which then wouldn't be converted by markdownToWikiLinks. Fix: apply unescapeDocLinks() at the start of runCollabFlush (covers both the timer-driven idle path and the unmount path). * fix(collab): gate UI mutations on foreground+current-item per Codex review (round 2) [P2] runCollabFlush mutated page-scoped state (saveStatus, editorStore.lastSaveTime, lastFlushedContent) before checking whether the captured itemId still matches the foreground item. On item navigation, the cleanup-driven keepalive flush could stamp 'saving' onto the NEW page's saveStatus, leaving it pinned indefinitely (and pollute lastFlushedContent for the new item's dedupe state). Fix: introduce isForegroundCurrent() = !keepalive && item.id === itemId. Gate saveStatus / setLastSaveTime / showSaved on it so background cleanup flushes never touch UI state. Gate lastFlushedContent on item.id === itemId regardless of keepalive so a stale flush can't seed the wrong item's dedupe. * fix(collab): skip cleanup flush on rich→raw transition per Codex review (round 3) [P1] $effect cleanup fires the keepalive flushCollabNow on every provider teardown, including rawMode toggles. The raw-button onclick already pre-populated rawPendingMarkdown with the live editor markdown (which the 1.2s raw debounce will land), so the keepalive PATCH from cleanup is redundant — and worse, can land AFTER the raw save and clobber newer raw edits with the older Y.Doc snapshot. Fix: gate the cleanup flush on `!rawMode`. If rawMode is true at cleanup time, the user just toggled to raw and the raw-mode codepath owns items.content from here. The other cleanup triggers (item nav, canEdit flip, page unmount) all keep firing the flush as before. Note: rawMode === true at cleanup time unambiguously means "transitioning into raw" — the inverse case (already in raw and the cleanup fires for some other reason) is impossible because collabKey gates on !rawMode, so the provider $effect never runs while rawMode is true. * fix(collab): synchronously flush Y.Doc state on rich→raw toggle per Codex review (round 4) [P1] Rich → raw → navigate-without-typing-or-toggling-back never PATCHed the live Y.Doc state to items.content. The previous seed mechanism only set rawPendingMarkdown, which only fires the 1.2s debounce on a subsequent handleRawContentUpdate call — which never happens if the user doesn't type. Fix: await runCollabFlush(ws, itemId, md, true) inside the raw button's async onclick BEFORE flipping rawMode = true. This: - Lands items.content with the live Y.Doc state synchronously (one PATCH, awaited, with keepalive: true so it survives a fast post-toggle navigation). - Seeds lastFlushedContent so any cleanup-driven re-flush is deduped. - Avoids populating rawPendingMarkdown — the raw debounce now only fires for actual user edits in raw mode, eliminating the race where a stale debounce fired after navigation could clobber state. The Round 3 cleanup-skip on rawMode is kept as defense-in-depth (also makes the no-op-when-already-flushed semantics explicit). * fix(collab): loop-flush until stable + cancel timer on rich→raw toggle per Codex review (round 5) Two HIGH findings from round 5: 1) Round 4's single-flush captured md BEFORE the await; concurrent peer edits (e.g. same user's other tab) during the await were lost from the seed and could be overwritten by subsequent raw-mode saves. Fix: loop-flush until stable. Re-read editor markdown after each PATCH; if it changed, flush again. Capped at 3 iterations to bound the transition under aggressive concurrent typing. 2) An onUpdate during the await could schedule a 5s collab flush timer that survived the rawMode flip. The cleanup skipped flushCollabNow on rawMode, but the timer fired its own runCollabFlush — which then PATCHed stale rich markdown on top of subsequent raw saves. Fix: explicitly clearTimeout(collabFlushTimer) at the end of the rich→raw onclick (after the loop-flush, before flipping rawMode). Belt-and-braces with the Round 3 cleanup skip. * fix(collab): seed raw mode from lastFlushed (not unflushed Y.Doc) per Codex review (round 6) [HIGH] Round 5's loop-flush could exit at the 3-iteration cap with md still differing from the last-PATCHed value, then seed rawSeedMarkdown with that unflushed md. An immediate navigation without typing would lose the unpersisted state. Fix: track lastFlushed inside the loop. After the loop, seed rawSeedMarkdown = lastFlushed (the markdown we actually PATCHed), NOT md (potentially a never-flushed in-memory value). If peer edits keep arriving past our cap, items.content lags Y.Doc briefly — but the peer's own 5s flush will catch up shortly, and at least raw mode shows state consistent with items.content rather than holding a value the server never received. * fix(collab): three corner-case fixes per Codex review (round 7) 1) [HIGH] lastFlushed = md was set unconditionally inside the loop-flush, even when runCollabFlush returned false (PATCH failed). rawSeedMarkdown could then be seeded with markdown the server never received. Fix: gate `lastFlushed = md` on runCollabFlush returning true. Failed PATCHes leave lastFlushed at its prior value. 2) [HIGH] lastFlushedContent (the collab-flush dedupe key) was never invalidated by raw-mode direct saves. Scenario: collab flushes A. Raw saves B. User returns to rich + edits back to A. Next collab flush dedupes (lastFlushedContent === A) and skips, leaving items.content stuck on B. Fix: reset lastFlushedContent = null after every successful raw save (both the regular handleRawContentUpdate path and the flushRawIfPending drain loop) so subsequent collab flushes always re-PATCH. 3) [MEDIUM] The async rich→raw onclick applied rawSeedMarkdown + rawMode = true after multiple awaits without verifying the user was still on the same item. A navigation during the loop-flush could let item A's handler resume and seed raw mode on item B. Fix: before mutating component state (rawSeedMarkdown, rawMode), check `item?.id === itemId` (the captured target). Bail with `return` if mismatched. * fix(collab): differentiate flush outcomes + foreground keepalive=false per Codex review (round 8) Two findings from round 8: 1) [P1] runCollabFlush returned `false` for both PATCH failure AND dedupe-skip. The rich→raw toggle treated `false` as "didn't flush" and didn't seed rawSeedMarkdown — but a dedupe means items.content already matches our markdown (the prior successful flush put it there). Raw mode then seeded from the page's stale `item.content` field, and a subsequent raw save could overwrite the current server content with the pre-collab snapshot. Fix: change runCollabFlush's return type to a discriminated string: 'flushed' | 'deduped' | 'failed'. The toggle treats 'flushed' and 'deduped' equivalently for seeding (both mean "server has this markdown") and only bails on 'failed'. 2) [P2] The toggle path used keepalive=true for the awaited flush. Browser keepalive requests can reject for bodies larger than the per-origin keepalive quota (~64KB). On reject, the catch silently fell through and raw mode activated with rawSeedMarkdown null. Fix: switch the toggle path to keepalive=false. The await is synchronous and user-initiated; navigation isn't imminent, so the keepalive escape hatch isn't needed (and risks losing the explicit save). Also added an `aborted` short-circuit so a 'failed' result returns early WITHOUT entering raw mode — user can retry. Cleanup-driven flushes (which DO need to survive page lifecycle) still use keepalive=true.	2026-05-08 22:48:56 -04:00
xarmian	5dc42b60df	feat(collab): wire Yjs WebSocket provider + Y.Doc lifecycle (TASK-1259) (#457) ... * feat(collab): wire Yjs WebSocket provider + Y.Doc lifecycle (TASK-1259) Adds a thin y-websocket-style provider speaking the binary protocol already implemented server-side in internal/collab/room.go. The provider lives in a Svelte 5 .svelte.ts module so connection state (`connected`, `synced`) can be consumed reactively by upcoming UX tasks (TASK-1264 pending-sync indicator, TASK-1265 mobile reconnect). Wire format mirrors the server's first-byte discriminator: 0x00 → y-protocols/sync (persisted to op-log + broadcast) 0x01 → y-protocols/awareness (broadcast only, ephemeral) Lifecycle is bound to the item-detail page via $effect keyed on `${item.id}:${canEdit}` — same key the <Editor> already re-mounts on, so the Y.Doc and provider tear down in lockstep with the editor. View-only viewers (canEdit === false) keep the legacy non-collab editor; their read-only y-binding is deferred to TASK-1266. Reconnect uses 1s/2s/4s/...30s exponential backoff. Sophisticated mobile reconnect (visibility, network state) is TASK-1265. KNOWN TEMPORARY REGRESSION: existing items with non-empty items.content render an empty editor on first open under collab, because the Y.Doc starts empty and TASK-1259 doesn't seed from markdown. TASK-1261 (next in Phase 2) adds the lazy seed-after- initial-sync path. New items + items already round-tripped through collab are unaffected. Drive-by lint cleanup of dead code that escaped Phase 1's make-install-skips-lint loophole: - gofmt -w on internal/collab/{applier,bus,manager}.go - removed unused test/debug helpers Room.peerCount and Room.applierConnCount (re-add with real callers when needed) Parent: PLAN-1248 * fix(collab): gate Editor mount on ydoc + handle applier_request + catch-up state per Codex review (round 1) Three findings from round 1: 1) [P1] $effect constructs ydoc AFTER Editor's onMount runs, so the first mount on an editable item registered StarterKit history instead of the Collaboration extension. The {#key} excluded ydoc, so the editor never re-mounted when ydoc later became truthy → editable users got a non-collab editor while the provider connected to an unused Y.Doc. Fix: gate the editable Editor mount on `ydoc` being ready (`{#if !canEdit} ... {:else if ydoc} ...`). Adds at most one reactive tick of delay; guarantees the first mount has the binding registered. 2) [P1] Provider dropped non-binary WebSocket frames, but the server sends `applier_request` as TextMessage. With TASK-1259 minting active rooms, every concurrent CLI/MCP/API content PATCH would sit blocked for 30s waiting for an ack, then fall back to a direct write — and the in-memory Y.Doc would still hold stale state and clobber it on the next 5s flush. Silent data loss. Fix: parse TextMessage frames as JSON ControlMessage. On `applier_request`, invoke an `onApplierRequest` callback (the page passes `editor.commands.setContent(markdown)`) and send `applier_ack` on success. The ExpiresAtMillis-driven late-apply guard remains TASK-1262's full scope. 3) [P2] Local Y.Doc updates were silently dropped if the socket was closed when handleDocUpdate fired. On reconnect the dumb-relay server can't reconstruct missing updates from a state vector, so any edits made before the first open or during a disconnect could be lost. Fix: after sending syncStep1 in onOpen, also send the current doc state as a single update via `Y.encodeStateAsUpdate(ydoc)`. CRDT idempotency makes this safe on initial open (server already has these ops via op-log replay → sees a no-op update). Larger docs incur a one-time cost on each connection; TASK-1265's mobile-reconnect work can replace this with a buffered queue. * fix(collab): destroy provider during rawMode + enforce ExpiresAtMillis on applier requests per Codex review (round 2) Two findings from round 2: 1) [P1] collabKey ignored rawMode, leaving the WS provider connected while the user edited via RawMarkdownEditor. Raw saves bypass the y-binding (PATCH writes items.content directly), but the server sees an active room → routes the PATCH through the applier flow → no editor mounted → 30s timeout fallback → direct write. The stale Y.Doc still in memory then overwrote the raw save on the next 5s flush after toggling back. Fix: include rawMode in the collabKey derivation so toggling raw destroys the provider (and the in-memory Y.Doc), and toggling back mints a fresh pair that re-seeds from the op-log + TASK-1261's lazy markdown seed. 2) [P1] Provider passed expires_at_millis to the handler but never gated on it. A backgrounded tab that wakes after the server retried or fell back could still apply setContent and overwrite newer peer edits. Fix: enforce the expiry in CollabProvider — check before invoking the handler AND re-check before acking (handlers are awaited and could span the deadline). Suppress the ack if either gate trips; the server interprets "no ack" as "applier unavailable" and falls back cleanly. * fix(collab): prune op-log on direct-write fallback + pre-mutation expiry check per Codex review (round 3) Two findings from round 3: 1) [P1] rawMode toggle to/from rich left a stale op-log: raw saves wrote items.content directly while the destroyed provider's old op-log persisted. Toggling back minted a fresh Y.Doc that replayed the old log → showed pre-raw content → silently overwrote the raw save on the next 5s flush. Fix server-side: when ApplyExternalContent returns ErrNoActiveRoom (no peers in memory, no in-flight Y.Doc state to corrupt), prune the op-log alongside the direct items.content write so future collab sessions start from a clean slate seeded by items.content (TASK-1261's lazy seed). Pruning is intentionally NOT applied to ErrNoApplierAvailable / ErrAllAppliersTimedOut — those paths may have live peers whose Y.Doc state would diverge. 2) [P2] Provider's post-handler expiry check only suppressed the ack, not the actual setContent mutation owned by the page handler. An async handler that crossed the deadline could still write stale markdown into the Y.Doc. Fix: page handler now does its own pre-mutation expiry check inside onApplierRequest before calling setContent. Documented the contract on ApplierRequestHandler — handlers MUST honour expiresAtMillis BEFORE mutating state. * fix(collab): prune op-log on grace-TTL applier-unavailable + suppress autosave when collab active per Codex review (round 4) Two findings from round 4: 1) [HIGH] op-log pruning still skipped ErrNoApplierAvailable. When raw-mode destroys the in-tab provider, the room remains in its 60s grace TTL with zero conns, so the next direct-write PATCH returns ErrNoApplierAvailable (not ErrNoActiveRoom). Stale op-log rows persisted; toggling back within the grace window resurrected pre-raw-save Y.Doc state. Fix: prune op-log on ErrNoApplierAvailable too — the "no live conns" condition makes pruning safe (no peers to corrupt). ErrAllAppliersTimedOut still preserves op-log because peers may still be alive there. 2) [HIGH] Once the WS provider is active the legacy 1.2s content autosave PATCH gets intercepted by the applier path (handleUpdateItem branch added in TASK-1252). On applier success input.Content is nil'd out, so UpdateItem never writes the markdown snapshot. The page's autosave was the only canonical items.content flush in this diff — search / share-page / API consumers would see stale content forever. Fix: short-circuit handleContentUpdate when collabProvider is set. The Y.Doc + op-log are canonical; items.content stays at its pre-collab snapshot until TASK-1260 introduces the proper 5s idle flush with applier-bypass semantics. This is a known Phase-2-internal regression closed by the very next task in this run. * fix(collab): tighten error classification + per-item lock + raw-mode flush per Codex review (round 5) Three findings from round 5: 1) [HIGH] applier.go could return ErrAllAppliersTimedOut even when no applier_request was ever successfully written (a row of write failures followed by no remaining candidates). The handler-side prune skipped that case, leaving stale op-log rows even though no peer received the request. Fix: track `anyWriteSucceeded` across the attempts and return ErrNoApplierAvailable (which prunes) when the loop exits without ever putting bytes on the wire. 2) [HIGH] Race between ApplyExternalContent's no-room classification and the subsequent Prune/UpdateItem: a fresh Join could mint a room and replay the soon-to-be-pruned op-log into a new client, leaving it with stale Y.Doc state that overwrites the freshly-written items.content on the next idle flush. Fix: introduce per-item setup mutex on RoomManager. Join holds the lock across addConn + replayTo and releases it before the long-lived readLoop. New PruneAndApply method wraps the prune+direct-write in the same per-item lock and re-verifies "no live peers" under it (returns ErrRoomActiveDuringPrune if a peer slipped in, in which case the caller falls through to a plain direct write without pruning). Lock order: per-item lock > m.mu > r.mu — Join and PruneAndApply both follow it. 3) [MEDIUM] Raw-mode 1.2s debounce timer could outlive the toggle to rich mode: the deferred PATCH fired post-collab-mint and got routed through the applier path (potentially overwriting newer peer state). Fix: track the latest pending raw markdown in `rawPendingMarkdown`. The Rich-mode button is now an async onclick that awaits a `flushRawIfPending()` synchronous PATCH before flipping `rawMode = false` (which is what activates the collab provider via the collabKey derivation). * fix(collab): evict broken applier conn + retry on prune-race + retain raw pending on PATCH failure per Codex review (round 6) Three findings from round 6: 1) [HIGH] When applier_request write failed, the broken roomConn stayed in r.conns, defeating PruneAndApply's "no live peers" check (which then returned ErrRoomActiveDuringPrune and the handler skipped pruning). Net effect: the prune-safety classification reverted to the round-5 hazard. Fix: in the applier write-failure branch, force-close the conn and call removeConn before continuing to the next applier. Both are idempotent with the readLoop's natural cleanup path (bus.Unsubscribe, conn map delete, conn.Close all tolerate double-invocation). 2) [HIGH] On ErrRoomActiveDuringPrune the handler fell through to a plain direct-write to items.content, bypassing the now-active peer's applier. The peer's stale Y.Doc could still overwrite items.content on the next idle flush. Fix: surface ErrRoomActiveDuringPrune from applyContentViaCollabOnce so the new applyContentViaCollab wrapper can retry the full ApplyExternalContent flow against the freshly-active room. Capped at applyContentMaxRetries=3 to prevent runaway loops if joins keep landing during prune attempts. After exhaustion, returns the same sentinel — the handler's existing `if err == nil { input.Content = nil }` gate falls through to direct write, which is the correct degraded-mode behavior. 3) [MEDIUM] flushRawIfPending cleared rawPendingMarkdown before the PATCH succeeded and the Rich-mode toggle always set rawMode = false regardless of flush outcome. A failed flush could activate collab with unsaved raw edits. Fix: rework flushRawIfPending to return success bool, retain rawPendingMarkdown on PATCH failure, and gate the Rich-button transition on `ok`. Added a re-entrancy guard (rawFlushInFlight) so a rapid double-click waits for the in-flight flush to settle instead of issuing a duplicate PATCH. * fix(collab): drain-loop flushRawIfPending to handle fast-typist edge per Codex review (round 7) [P1] flushRawIfPending snapshotted rawPendingMarkdown then awaited the PATCH; if the user typed during the await, the equality check preserved the newer edit but the function still returned `true` and the Rich-mode handler flipped collab on. The newly-active provider then raced the un-flushed pending raw save — exactly the hazard the guard is meant to close. Fix: rework flushRawIfPending into a bounded drain loop. Each iteration snapshots-PATCHes-clears (with the equality check). The loop runs up to RAW_FLUSH_DRAIN_CAP=5 iterations, returning `true` ONLY when rawPendingMarkdown is null on exit AND no PATCH failed. A fast typist who keeps the queue non-null across the cap returns `false`, leaving the user in raw mode (next click retries). PATCH failure short-circuits with `false` so the toggle stays in raw mode and the unsaved markdown is preserved for retry. * fix(collab): atomic prune+content-write + preserve newer raw edit on stale PATCH response per Codex review (round 8) Two findings from round 8: 1) [P1] PruneAndApply ran the op-log prune under the per-item lock but the items.content write happened later in the post-loop UpdateItem call, OUTSIDE the lock. A fresh Join landing in that gap could replay the now-empty op-log, mint a peer with stale Y.Doc state, and then overwrite the freshly-written items.content on the next idle flush. Fix: applyContentViaCollab now takes a `directWrite` callback that the caller (handleUpdateItem) implements as a content-only UpdateItem. PruneAndApply's applyFn invokes it AFTER the prune so both run inside the same per-item critical section. The trade-off is two DB round-trips when a PATCH carries content + other fields together (rare): the content-only update happens inside the lock; the rest (title, fields, status) flows through the post-loop UpdateItem with input.Content nil'd to suppress the duplicate write. 2) [P1] In flushRawIfPending's drain loop, `item = updated` assigned the server-side snapshot from the just-PATCHed markdown even when a newer raw edit had landed in the meantime. RawMarkdownEditor mirrors `item.content` into its textarea unconditionally (line 16), so the stale assignment would reset the textarea mid-keystroke and lose the queued edit. Fix: only swap in the full updated snapshot when `rawPendingMarkdown === markdown` (no newer edit). Otherwise keep our local content and adopt only the server-side metadata (timestamps, version, modified_by) via spread. * fix(collab): atomic mixed PATCH + raw autosave stale guard + rich→raw seeding per Codex review (round 9) Three findings from round 9: 1) [P1] Toggling FROM rich+collab TO raw mode seeded RawMarkdownEditor from items.content, which is intentionally stale under collab (handleContentUpdate is suppressed while the provider is connected; TASK-1260 closes that gap with a 5s flush). Saving from raw mode would overwrite the live Y.Doc state with a pre-collab snapshot. Fix: when toggling to raw with a connected provider, capture the editor's current Y.Doc-derived markdown via `editor.storage.markdown.getMarkdown()` into a one-shot `rawSeedMarkdown` slot and pre-populate `rawPendingMarkdown` so the first auto-save persists it. RawMarkdownEditor seeds from `rawSeedMarkdown ?? item.content`. Cleared on rich-mode toggle. 2) [P1] The regular debounced raw autosave still assigned `item = updated` from a stale PATCH response. Same stale-snapshot hazard the Round 8 fix closed in flushRawIfPending. Fix: equality-check `rawPendingMarkdown === toSave` before swapping in the server snapshot. On stale, keep local content and adopt only the server-side metadata via spread. 3) [P2] Round 8 split the items.content write (under per-item lock) from the rest of UpdateItem (post-loop), losing atomicity for mixed PATCHes (content + title) and breaking Store.UpdateItem's content-versioning peek at Title. Fix: directWrite callback now invokes the FULL UpdateItem inside the per-item lock. A `fullWriteHandled` flag tells the handler to skip the post-loop UpdateItem entirely (otherwise we'd duplicate the write and create two version-history rows). Mixed PATCHes are atomic again under the lock. * fix(collab): clear raw seed/pending on item navigation per Codex review (round 10) [P1] Navigating between items left rawSeedMarkdown, rawPendingMarkdown, and the contentDebounceTimer set from the previous item. This caused two concrete hazards: (a) Item B's raw editor mounted with item A's live markdown via `rawSeedMarkdown ?? item.content`. (b) Clicking Rich on item B fired flushRawIfPending which PATCHed A's queued markdown INTO item B (cross-item data bleed). Fix: at the top of loadData(), clear contentDebounceTimer, rawSeedMarkdown, and rawPendingMarkdown so each navigation starts from a clean slate. The collab provider's own lifecycle is already keyed on item.id via $effect cleanup, so it doesn't need the same explicit reset. * fix(collab): item-id race guard on raw PATCH responses per Codex review (round 11) [P1] In-flight raw PATCH responses (debounced autosave AND drain loop) could clobber a newly navigated item. Clearing contentDebounceTimer in loadData only cancels timers that have not fired; an awaiting fetch keeps running and its `.then` / `.catch` would assign back to the new page's `item` state. Fix: mirror the existing TASK-754-style race guard pattern (already used in the SSE / sync handlers above). Capture `reqItemId = item.id` BEFORE the PATCH, then in the response handler bail if `!item || item.id !== reqItemId`. Applied to both handleRawContentUpdate's setTimeout body and flushRawIfPending's drain loop. * fix(collab): reset saveStatus on item navigation per Codex review (round 12) [P2] After Round 11's race guard, a stale raw PATCH response that matched a now-different item.id was correctly discarded — but saveStatus had already been set to 'saving' before the await. With loadData not resetting it, the next item could mount with saveStatus pinned at 'saving' indefinitely, which then suppressed all SSE/sync refreshes via the `if (saveStatus === 'saving')` guards above. Fix: in loadData's per-item state reset, clear saveStatusTimer and reset saveStatus to 'idle' alongside the other transient state. Cheap, scoped, no impact on the in-flight save's eventual discard path.	2026-05-08 20:48:57 -04:00
xarmian	50e0936b34	feat(collab): designated-applier protocol for external content updates (TASK-1257) (#455) ... * feat(collab): designated-applier protocol for external content updates (TASK-1257) The keystone task for CLI / API / MCP integration during co-edit sessions. When a content update arrives via PATCH while at least one browser tab is connected to the item's collab room, the server can't write items.content directly — the connected tabs would silently overwrite it on the next 5s idle flush using their (now stale) Y.Doc state and the caller's update would be lost. Solution: nominate one connected tab as the "designated applier", send it a JSON control message with the new markdown, the browser does editor.commands.setContent(markdown) which the y-tiptap binding translates into Y.Doc updates that propagate via the regular sync path. Items.content gets refreshed via the next 5s flush (TASK-1261). Architecture: internal/collab/applier.go (new): - ControlMessage struct — JSON envelope for applier_request / applier_ack frames. Carried over WebSocket TextMessage, which is unambiguous against y-protocol's BinaryMessage. - ApplyExternalContent(itemID, markdown) — public entry point. Returns nil on ack, ErrNoActiveRoom when there's no room (caller falls back to direct write), ErrNoApplierAvailable when the room has no live conns, ErrAllAppliersTimedOut when every attempt expired. - Election: pickApplier returns the longest-connected roomConn that hasn't already been tried, with deterministic tiebreak on conn id. Stable choice — longest connection has the most authoritative cumulative Y.Doc state, fewer flicker risks. - Retry loop: applierMaxAttempts=2, applierFirstTimeoutVar=30s, applierRetryTimeoutVar=15s. The Var-suffixed names exist so test helpers can shrink to ms without sleeping a real minute. - Pending-ack tracking: per-room map[requestID]*pendingApplierAck pairing the channel a PATCH handler is waiting on with the conn the ack is expected from. expectedConn check prevents an unrelated peer from spoofing acks for someone else's request. internal/collab/room.go (extended): - roomConn gains connectedAt for the election. - readLoop branches TextMessage → handleControlMessage which decodes the JSON and routes applier_ack to the room's pending tracker. Unknown control types and malformed JSON are silently dropped so a bad client can't break the loop. internal/collab/manager.go: - Registers connectedAt on Join. - Initialises room.pendingAcks alongside conns map. internal/server/handlers_items.go (extended): - handleUpdateItem now branches on input.Content != nil + s.collab != nil: routes through s.applyContentViaCollab; on success, zeros input.Content so UpdateItem's direct write is suppressed. - Field-only PATCHes skip this branch entirely — backward-compatible. internal/server/handlers_collab.go: - applyContentViaCollab wraps mgr.ApplyExternalContent with per-error-class slog warnings so operators can see degraded paths (timeouts → warn; no-room / no-applier → quiet, the common case for non-co-edit CLI updates). - actorIDFromRequest helper for log fields. Tests (5 new): - TestApplyExternalContentNoActiveRoom — sentinel error path. - TestApplyExternalContentHappyPath — applier echo acks within ms. - TestApplyExternalContentTimeoutsThenFails — applier never acks; we hit applierFirstTimeoutVar then ErrAllAppliersTimedOut. - TestApplyExternalContentTimeoutThenSecondAcks — first applier silent, retry picks second-longest-connected, succeeds. - TestApplyExternalContentRejectsAckFromUnexpectedConn — defence- in-depth: peer B forges an ack for peer A's request; the room's expectedConn check rejects it; ApplyExternalContent runs to timeout instead of being satisfied by the forgery. All tests pass under -race. Full suite green. Parent: PLAN-1248. Phase 1 — Backend foundation. * fix(collab): clean pendingAcks on success + expires_at on applier_request per Codex review (round 1) P2 #1: ApplyExternalContent retained the per-request pendingAcks entry on success. Each successful external update therefore leaked a request_id + channel + expected-conn pointer for the remainder of the room's lifetime — across long-lived sessions the map would grow without bound. Add cancelPendingAck to the ack-success path so the entry is released as soon as the request completes. Test added (TestApplyExternalContentCleansPending- AcksOnSuccess) drives 5 successful applies and asserts the pendingAcks map is empty afterwards. P2 #2: applier_request had no client-enforceable expiry, so a backgrounded tab could process a stale request 60s later and overwrite newer edits with old markdown after the server had already retried with a different applier (or fallen back to direct write). Add ExpiresAtMillis to the ControlMessage envelope, populated per attempt with `now + timeouts[attempt]`. The browser-side handler (TASK-1263) is responsible for the client-side Now() check before applying — without that check the field is documentation-only. Added test (TestApplyExternalContentSendsExpiresAt) regression-tests the server stamp. Server-side cleanup is also reinforced: cancelPendingAck on timeout (already present) means a late ack from a timed-out applier is rejected at the room layer (entry is gone). The expires_at_millis is the second line of defence for the case where the browser sends the Y.Doc setContent BEFORE the ack — the request must not be applied at all.	2026-05-08 16:30:39 -04:00
xarmian	79eb00d2a1	feat(collab): periodic auth revalidation timer (TASK-1256) (#454) ... * feat(collab): periodic auth revalidation timer (TASK-1256) Catches mid-session revocations on a live collab WebSocket the same way handlers_events.go's sseSubscriberStillHasAccess does for SSE. When the WS handler upgrades, it spawns a goroutine that ticks every collabMembershipRevalInterval (60s, jittered across [0, interval) on first fire to avoid post-deploy reconnect-storm spikes). Each tick re-runs authorizeCollabAccess — the same workspace-access ladder used at upgrade time, including the "fresh-fetch user from store" semantics that make admin-demoted-mid-stream visible without waiting for the next request. On access loss the handler routes through a new RoomManager.CloseConn(itemID, conn, code, reason) method which: - Looks up the roomConn in the manager so the close frame can go out under the per-conn writeMu (no concurrent-write panic against the room's writeLoop or replay path). - Sends a websocket.ClosePolicyViolation frame with a human-readable reason ("Your access to this item was revoked.") so the frontend can stop reconnecting in a tight loop. - Falls back to plain conn.Close when the conn isn't tracked yet (race window between Join's getOrCreate and addConn). The goroutine is bound to the handler's lifetime via a `stop` channel that closes when handleCollab returns; no leaked timers or goroutines after disconnect. Test (TestCollabMembershipRevalidationClosesOnRevoke): - Shrinks the reval interval to 30ms so the test runs in tens of ms rather than 60 seconds. - Bootstraps an admin (so the no-users escape hatch is closed), creates a non-admin member user, mints a session, dials in. - Calls RemoveWorkspaceMember while the WS is open. - Asserts the next read returns an error (close frame or transport failure — both are acceptable signals the server tore the connection down). Parent: PLAN-1248. Phase 1 — Backend foundation. * fix(collab): WriteControl for revoke close + tighten test failure modes per Codex review (round 1) P2 #1: CloseConn used rc.writeMessage which acquires the per-conn writeMu. If the room's writeLoop / replay was mid-WriteMessage to a slow peer, revocation would block behind that writer and never force-close the unauthorized conn. Switch to conn.WriteControl, which gorilla documents as concurrency-safe with normal writes (it bypasses the conn's normal write path) and accepts an explicit deadline so a stuck send can't extend the budget indefinitely. The deadline is 1s — generous for a healthy conn, short enough that a half-broken socket falls through to plain Close quickly. The itemID parameter stays in the API for symmetry / future per-room metrics, but is no longer used for the actual close path now that the writeMu lookup is gone. P2 #2: TestCollabMembershipRevalidationClosesOnRevoke previously treated a read-deadline timeout as a log-only branch — the test could pass after waiting 2s with the WS still open, exactly the bug being regression-tested. Restructure to fail fast on timeout (t.Fatalf isTimeout(err)) and prefer ClosePolicyViolation as the expected close code, falling back to "any non-timeout error" only because the underlying TCP teardown can produce different error shapes depending on timing. The isTimeoutOrEOF helper that masked the failure is replaced with a narrowly-scoped isTimeout. * fix(collab): distinguish access denial from transient errors in reval per Codex review (round 2) P-MEDIUM: the revalidation goroutine treated any non-nil error from authorizeCollabAccess as revocation, including transient store errors (GetUser / GetWorkspaceByID / grant-lookup blips). One DB hiccup would close every active collab WS with ClosePolicyViolation, which is a worse UX than the bug being guarded against. Distinguish via errors.As against *statusError (the typed return from authorizeCollabAccess used for all "we know they don't have access" branches). Plain errors fall through to a warn-level log + timer reset so the next tick retries. Three branches in the revalidation switch now: err == nil still authorised — reset timer. isAccessDenial(err) real revocation — close conn with typed reason. default transient — log warn, keep conn open, reset. * fix(collab): re-fetch item on each reval tick per Codex review (round 3) P2: revalidation re-authorized against the *Item captured at upgrade time, so an item moved to a collection the user can't see — or hard-deleted — would not be caught: authorizeCollabAccess kept checking the stale CollectionID, kept passing, and the WS stayed open against an item the user no longer has access to. Re-fetch via s.store.GetItem(itemID) at the start of each tick: - error → log warn, keep conn open, retry next tick (matches the transient-store-error policy from round 2). - nil → item hard-deleted (or never existed): close with ClosePolicyViolation + "This item is no longer available." - otherwise → authorize against the FRESH item, picking up any collection move automatically. Per-tick GetItem is one indexed lookup per minute per active connection — negligible compared to the auth-cascade GetUser / member / grant queries that already run on the same tick.	2026-05-08 16:07:26 -04:00
xarmian	e7b1c3b5ae	feat(collab): per-item Room manager with op-log replay + grace TTL (TASK-1255) (#453) ... * feat(collab): per-item Room manager with op-log replay + grace TTL (TASK-1255) Wires the OpBus + op-log + WS handler from prior phase-1 PRs into a working dumb-relay collab server. Per-item Room created lazily on first Join, kept alive across transient disconnects via a 60s grace TTL, reclaimed when the grace expires with no fresh subscribers. Components: - internal/collab/room.go — Room struct + lifecycle · roomConn pairs (id, conn, bus channel, write mutex). The id is server-assigned per WS so writeLoop can suppress own-event echoes without decoding the Y.Doc to read the Yjs ClientID. · readLoop discriminates yMessageSync vs yMessageAwareness on byte 0. Sync frames are persisted to the op-log AND broadcast; awareness frames are broadcast only (presence is ephemeral). Persistence happens BEFORE broadcast so a crash mid-publish loses at most a live keystroke that the originator will replay on reconnect anyway. · writeLoop drains the bus subscription and writes non-self events to the WS, gated by a per-conn write mutex (gorilla's "one writer at a time" rule). · removeConn arms a 60s graceTimer when the last conn drops; a fresh addConn cancels the timer. onGraceExpired re-checks len(conns) == 0 under the room mutex and only THEN sets closing=true + calls back to the manager. The race between "manager.getOrCreate found us" and "grace timer fired" is handled by addConn returning errRoomClosing; the manager retries via getOrCreate which mints a fresh Room. - internal/collab/manager.go — RoomManager + RoomManagerConfig · NewRoomManager wires production defaults (DefaultGraceTTL = 60s, DefaultSchemaVersion = "1"). NewRoomManagerWithConfig accepts an explicit config so tests can drop graceTTL to a few ms without sleeping a minute. graceTTL is per-manager, not a package var, so parallel tests with different TTLs don't trip the race detector. · Join is the public entry point: getOrCreate → addConn (with retry on errRoomClosing) → replayTo → spawn writeLoop goroutine → run readLoop inline → wait for writeLoop drain → return. The inline read keeps the HTTP handler in scope so its `defer conn.Close()` doesn't fire until both loops exit. · Close is for graceful server shutdown — closes every active conn under the room mutex, then drains the manager's room map. - internal/collab/manager_test.go — 7 tests covering: lazy create, op-log replay-on-connect (two seed rows arrive in order), sync broadcast + persist (peer B sees A's frame, originator does not echo, op-log gains a row), awareness broadcast WITHOUT persist, cross-item isolation (item-a frames don't leak to item-b subscribers), grace-TTL reclaim with a 50ms config TTL, grace cancel on reconnect within window, manager.Close shuts down every active conn. All tests run with -race; the bus's concurrent-publish test was already covered by TASK-1253. - internal/server/handlers_collab.go — wire to RoomManager · Returns 503 when s.collab is nil (matches the SSE handler's "events bus not configured" 503 — fail loud rather than silently accept the upgrade). · Otherwise hands the upgraded conn to s.collab.Join, which blocks until the WS closes. Unexpected close codes get the same warn-log as before; normal closures stay quiet. - internal/server/server.go — adds *collab.RoomManager field + SetCollabRoomManager setter (nil-safe optional, like SetEventBus). - cmd/pad/main.go — wires NewMemoryOpBus + NewRoomManager into the running server alongside the event-bus wiring. Single-instance only today; multi-replica fanout via Redis is a deferred IDEA per the Plan body. - internal/server/handlers_collab_test.go — adds testServerWithCollab helper (so existing collab tests get a real RoomManager) plus TestCollabUpgradeUnavailableWithoutRoomManager which asserts the 503 path for unwired servers. Parent: PLAN-1248. Phase 1 — Backend foundation. * fix(collab): per-room appendMu + Server.Stop closes RoomManager per Codex review (round 1) P1 — concurrent peers raced AppendYjsUpdate, violating the single-writer-per-item contract documented on the store call. Each peer's readLoop runs in its own goroutine, so two peers in the same room could call AppendYjsUpdate concurrently. On Postgres that risks the BIGSERIAL allocation-vs-commit-order cursor gap that TASK-1252's contract was specifically guarding against. Add an appendMu on Room held across the persist+publish sequence; reads, awareness frames, and OTHER rooms remain unserialised. Regression test (TestRoomManagerSerializesSyncAppends) drives 4 peers × 10 writes concurrently and asserts the op-log gains exactly 40 rows. Without appendMu this would intermittently surface fewer rows or out-of-order ids on Postgres; with it the count is deterministic and the race detector stays clean. P2 — Server.Stop did not close s.collab. Active collab WS goroutines + grace timers could keep using s.store after the server's other cleanup paths winding down. Add s.collab.Close() before rateLimiters.Stop so any Join goroutines holding rate-limiter handles can wind down cleanly. nil-safe via the existing collab optional-attachment pattern. * fix(collab): start writer before replay to avoid bus-overflow drops per Codex review (round 2) P2: a joining peer subscribed to live events BEFORE its writer goroutine started. During a long replay, live sync events would pile up in the 64-event bus channel; once full, MemoryOpBus.Publish silently drops them, leaving the new peer connected but permanently missing those updates. Restructure runConn to spawn the writer goroutine FIRST so it drains the bus subscription concurrently with the replay. Both replay and writer go through rc.writeMessage, which holds the per-conn write mutex, so we never violate gorilla's one-writer-at-a-time rule. Yjs CRDTs are commutative — applying live op 100 before replay op 50 yields the same final Y.Doc as the reverse order — so interleaving is correct. The trade-off is a brief "out of causal order" UX wobble during replay, which is acceptable: the alternative would require either an unbounded queue or losing updates the way the original order did. * fix(pad): call srv.Stop() in serveCmd shutdown so collab sessions close per Codex review (round 3) P2: serveCmd's SIGINT/SIGTERM path called srv.Shutdown but never srv.Stop. http.Server.Shutdown does NOT terminate hijacked connections (WebSockets), so active collab sessions kept running until process exit and could race the deferred store close. The RoomManager.Close path added in round 1 only fires inside Stop, so without this call the production shutdown was effectively bypassing the new cleanup. Add srv.Stop() after srv.Shutdown in the serveCmd shutdown sequence. Stop also runs the existing background-loop teardowns (orphan GC, MCP audit writer, MCP session tracker) which were previously already part of Stop's contract — those will continue to fire as they always have, so this commit's only behavioural change is "now also closes the collab room manager". * fix(collab): WaitGroup drain barrier + bigger bus buffer per Codex review (round 3) P1 — RoomManager.Close was not a true drain barrier. closeAll closed the WebSockets but did NOT wait for the corresponding Join goroutines (running runConn) to exit. Server.Stop returned before in-flight collab work finished, racing the deferred store close on process exit. Fix: track every Join in m.activeJoins (sync.WaitGroup); Close iterates closeAll first (waking up every reader by closing the conn), then activeJoins.Wait — guaranteeing no collab goroutine is still running by the time Close returns. P2 — replay-time bus overflow could still drop sync events on a slow drain (writeLoop blocks on the same writeMu replayTo holds, so a long replay starves the bus drain even with the writer goroutine started before replay). Two-part response: (a) Bump the per-subscriber bus channel buffer from 64 to 256. Sized for a 5x safety margin on a 1k-row replay against a chatty 5-peer room (~50 events/sec during a ~1s replay). (b) The architectural fix — force-close subscribers on overflow, honoring the bus's documented slow-peer recovery contract — is filed as TASK-1273 follow-up. That requires extending the OpBus interface (per-subscriber drop callback or counter) and an active health-check tick in the room manager; both are out of scope for TASK-1255's "lazy room + grace TTL" deliverable. For PLAN-1248's single-instance scope and typical editor load, 256 covers realistic workloads. Pathological / load-test scenarios exposing overflow can recover via Yjs's state-vector negotiation on reconnect, and TASK-1273 will tighten that to an active kick. * fix(collab): closed flag gates Join + Close idempotency per Codex review (round 4) P2: http.Server.Shutdown does NOT wait for hijacked WebSocket handlers, so a Join() call from a freshly-upgraded conn could fire AFTER Close() returned. The previous Add-then-Wait pattern was correct for already-started Joins but couldn't catch a Join that hadn't yet hit Add when Close fired. Race: Close iterates the (empty) rooms map, Wait sees zero waiters, Close returns; THEN Join hits Add and proceeds against a torn-down store. Add a `closed` flag gated by the same mutex that wraps activeJoins.Add. Three orderings, all safe: 1. Add before Close.closed=true → Wait blocks until Done. 2. Close.closed=true before Add → Join sees closed=true under the same lock and returns errManagerClosed without ever incrementing the WaitGroup. 3. Close called twice → second call short-circuits (idempotent). getOrCreate also gets a closed-flag short-circuit so a future caller can't bypass the gate by skipping Join. Test: TestRoomManagerJoinAfterCloseFailsFast asserts post-Close Join returns errManagerClosed, plus a second Close() is a no-op. All 15 collab tests pass under -race.	2026-05-08 15:38:45 -04:00
xarmian	2945ee27dd	feat(server): add WebSocket handler at /api/v1/collab/{itemID} (TASK-1254) (#452) ... * feat(server): add WebSocket handler at /api/v1/collab/{itemID} (TASK-1254) WebSocket entry point for Yjs-based collaborative editing on a single item under PLAN-1248. Bare-bones in this PR by design: upgrade + log connect/disconnect + drain reads. Protocol logic (forwarding to OpBus, persisting to op-log, awareness fan-out) arrives in TASK-1255 (room manager). Authorisation mirrors RequireWorkspaceAccess but keyed on the item's workspace ID rather than a {slug} URL param — the WS URL only carries itemID. Implementation re-uses the same access ladder: fresh-install escape hatch (no users) → grant legacy workspace-scoped API token, no user → grant if token's workspace matches the item's workspace OAuth token allow-list (TASK-953) → reject when workspace not on consented list authenticated user → admin OR member OR has guest grants User is re-fetched from the store on each upgrade (not trusted from session-context cache) so a mid-session admin demotion or member removal closes the upgrade path immediately. Mirrors sseSubscriberStillHasAccess. Periodic per-connection revalidation lives in TASK-1256. Route registered alongside SSE (outside the jsonContentType middleware group, but inside the auth middleware chain). Promotes github.com/gorilla/websocket from indirect to direct dep and bumps to v1.5.3 (latest stable; v1.5.0 was already in go.mod transitively via another package). Tests cover: - fresh-install escape hatch grants the upgrade - bootstrapped server rejects unauthenticated upgrade with 401 - non-member with valid session is rejected with 403 (NOT 401 — confirms the access path runs after auth, not before) - unknown item surfaces as 404 (not 401/403 leak) - empty itemID segment doesn't match the route Test infrastructure note: dialCollab takes an explicit User-Agent because pad's session-binding middleware hashes the UA at CreateSession time and re-checks on every request — the dialer must match what was stored, otherwise the cookie is rejected before the workspace check fires (and we'd see a misleading 401 where 403 was expected). Parent: PLAN-1248. Phase 1 — Backend foundation. * style: gofmt handlers_collab_test.go per Codex review (round 1) * fix(server): SetReadLimit + nginx upgrade headers for collab WS per Codex review (round 2) P-MEDIUM #1: handleCollab.ReadMessage had no per-message size cap, so an authenticated client could send an arbitrarily large frame and force unbounded server-side buffering — the HTTP body limit applied by the auth chain doesn't apply once the connection is upgraded. Set SetReadLimit(1 MiB), generous for keystroke-rate Yjs ops and large enough for a typical initial-sync state. ReadMessage returns an error when exceeded, which the existing read loop handles as a normal close. P-MEDIUM #2: deploy/nginx.conf routed /api/v1/collab/ through the default `location /` block, which sets `Connection ""` (cleared so HTTP keepalive works) — that strips the Upgrade header, so WebSocket upgrades silently fail behind the documented nginx deployment. Add a dedicated location block with proxy_set_header Upgrade $http_upgrade / Connection "upgrade", same 24h read/send timeouts as SSE so an idle editor tab does not get cut off mid-session. * fix(server): enforce per-item visibility in collab WS upgrade per Codex review (round 3) P2: authorizeCollabAccess granted upgrade to any workspace member or guest-with-grants without checking whether THIS specific item was visible to that user. A restricted member (collection_access=specific) or a guest with grants on item A could upgrade /api/v1/collab/{itemID} for an item B in a different collection — they'd see live edits to a document they have no right to read. Restructure the access ladder: 1. Workspace-level gate stays as-is: "any access at all?" If no membership AND no grants → 403 (unchanged). 2. Item-level visibility check added on top, mirroring requireItemVisible without depending on middleware-set request context (the WS path doesn't go through RequireWorkspaceAccess): - VisibleCollectionIDs nil → "all" access → grant. - Item's collection in the visible set → grant. - Item-level grant on this exact item → grant (covers guests given access to a single item rather than a whole collection). - Else → 404, mirroring requireItemVisible's "don't leak existence" pattern. Admin path returns nil before this check, so no change there. Legacy workspace-scoped API tokens grant editor-equivalent access on workspace match (predates the grants design); that branch is untouched since legacy tokens don't have a user identity to scope per-item grants against. Test added: TestCollabUpgradeRejectsRestrictedMemberForeignCollection — member with specific access to collA tries to upgrade for an item in collB → 404. Existing 5 tests still pass. * fix(server): strict per-item visibility check + sibling-grant test per Codex review (round 4) P1 (round 4): VisibleCollectionIDs is broader than full-collection access — it includes collections "anchored" by an item-level grant (so the nav can still surface the parent collection of a granted item). Round 3's check treated every visible collection as full access; a guest with grant `item:A` could upgrade /api/v1/collab/{B} for a sibling B in the same collection. Tighten by mirroring guestResourceFilter / requireItemVisible: 1. Coarse stage stays — collection must be in the visible set. 2. NEW strict stage when the user has item-level grants: (a) full collection grant on this collection → grant (b) member's "specific" access list including this collection → grant (c) item grant on THIS exact item → grant Else → 404 (the visible-set hit was anchored by a sibling's grant, not by full collection access). When the user has NO item grants, the coarse-only check is sufficient — visibility came from full collection access (member's "specific" list, full collection grant, or "all" access). Test added: TestCollabUpgradeRejectsGuestWithSiblingItemGrantOnly — guest with item:A grant tries to upgrade for sibling B in the same collection → 404 (the bug being regression-tested) AND verifies the granted item A still upgrades cleanly to 101 Switching Protocols.	2026-05-08 14:36:47 -04:00
xarmian	ce0be1ed0a	fix(auth): TokenAuth falls through invalid Bearer on public API paths (BUG-1227) (#434) ... Prior behavior: TokenAuth middleware rejected any invalid/malformed Bearer with 401 before dispatch — even on paths in isPublicAPIPath (/api/v1/auth/*, /health, share links, public plan-limits). A stale credential in ~/.pad/credentials.json (typically left over after wiping a test DB) made every CLI invocation 401 on the very first CheckSession() call, INCLUDING the endpoints needed to recover (login, forgot-password). Users could only fix it by manually deleting their credentials file. The matching IP-change-revoked branch in the same file already had the right pattern (middleware_auth.go:114-117): when the path is public, fall through to the handler unauthenticated and let it decide. This patch mirrors that across the four invalid-Bearer branches: - Authorization header doesn't start with "Bearer " - padsess_* token doesn't validate (stale or wiped session) - pad_* token format wrong (length, prefix) - pad_* token doesn't match a live API token Extracted into a small rejectInvalidBearer helper so the policy is visible in one place. Protected endpoints continue to 401 — the regression guard in TestTokenAuth_ProtectedPath_StillRejectsInvalidBearer pins that. Pre-existing bug; not introduced by TASK-1216 / TASK-1217. The new bootstrap flows just made it more visible because anyone testing fresh- install scenarios is likely to wipe DBs and end up with stale creds. Tests in middleware_auth_public_paths_test.go cover: - /auth/session with stale padsess_* Bearer → 200 with public payload - /auth/session with malformed Authorization → 200 - /auth/session with garbage token format → 200 - /auth/session with non-matching pad_* token → 200 - /auth/login with stale Bearer + valid creds → 200 (the actual user- visible recovery scenario) - Protected /workspaces with invalid Bearer → still 401 (regression) Closes: BUG-1227. Related: IDEA-1226 (per-server credentials — proper design fix; this is the safety-net fix that complements it).	2026-05-07 20:01:34 -04:00
xarmian	40352a32e1	feat(auth): PAD_BYPASS_SETUP_TOKEN open-bootstrap escape hatch (#429) ... Adds an env-var that lets self-host operators on trusted networks (Unraid behind a firewall, Tailscale-only deployments, homelabs) claim the first admin via the web UI without copying a bootstrap token out of the container logs. Behavior when PAD_BYPASS_SETUP_TOKEN=true: - handleBootstrap accepts non-loopback first-admin POSTs without an X-Bootstrap-Token header. The UserCount==0 invariant is unchanged, so the bypass auto-closes the moment the first admin claims the seat (subsequent bootstrap requests get 409 regardless of bypass). - handleSessionCheck returns setup_method=open so the /setup page skips the paste-token UI and renders the form directly. - Token generation is skipped at startup (no .bootstrap-token file written). A distinct WARN-flavored banner makes the open-mode trade-off obvious in operator logs. - Cloud mode (PAD_CLOUD/PAD_MODE=cloud) ignores the flag entirely. Three layers of defense: cmd/pad masks the env-var with !cfg.IsCloudServer(), Server.openBootstrapEnabled() checks !s.cloudMode, and the cloud branch in handleBootstrap never reads the bypass field. Unraid template gets a new "Bypass Setup Token" field (default false, Display="always") with a description that calls out the trust-the- network trade-off. Tests pin all the security-critical contracts: bypass admits non- loopback, bypass off keeps existing 403, cloud mode hard-ignores, loopback works either way, post-bootstrap gate stays closed, bypass wins over logs_token in session payload, cloud mode never advertises 'open' setup method. Codex review: CLEAN (round 1).	2026-05-06 13:27:12 -04:00
xarmian	05a9665f50	feat(auth): first-run logs-token bootstrap flow (TASK-1167) (#424) ... One-time bootstrap token generated on first start with no users in self-host mode. Token is logged in a banner the operator can grab from `docker logs`, persists at <DataDir>/.bootstrap-token (mode 0600), and bypasses the loopback-only gate via the X-Bootstrap-Token header — letting the user claim the first admin from a remote browser at /setup#token=<x>. Header-only contract + URL-fragment (browser-only, never transmitted) + log-redaction middleware keeps the secret out of access logs, proxy logs, and browser history. Cloud mode unchanged: token never loaded, never honored. Validate → UserCount-check → CreateUser → consume sequence is mutex-serialized to prevent concurrent valid-token requests from creating multiple admins. Part of PLAN-1166 (Pad on Unraid — Community Apps launch).	2026-05-06 08:40:11 -04:00
xarmian	1ff6158468	feat(workspace): expose currentRole + resource-scoped permission helpers (TASK-1101) (#415) ... * feat(workspace): expose currentRole + resource-scoped permission helpers (TASK-1101) Foundation for PLAN-1100 (client-side permission audit). Lands the primitive that every other task in the plan consumes, with no UI behavior changes. Server: - new GET /api/v1/workspaces/{ws}/me — returns role, collection_access, visible_collection_ids (computed via VisibleCollectionIDs / GuestVisibleCollectionIDs so it covers system collections, member access, direct collection grants, and item-grant collections), plus the user's direct collection_grants and item_grants. - admins normalize to "owner"; legacy workspace-scoped tokens normalize to "editor"; non-members with no grants are rejected upstream by RequireWorkspaceAccess and never reach the handler. Frontend: - new $lib/utils/permissions module exporting pure cascade functions: canEditWorkspace / canViewCollection / canEditCollection / canViewItem / canEditItem. - cascade mirrors server's ResolveUserPermission exactly: owner → item grant → collection grant → membership role + visibility so item grant beats collection grant beats role even when less permissive (ItemGrant.view + CollectionGrant.edit on same item → effective view). - workspaceStore wraps the pure functions with currentMembership state fetched in setCurrent. New getters: currentRole, currentMembership, isOwner, canEditWorkspace; new methods: canViewCollection / canEditCollection / canViewItem / canEditItem. - WorkspaceMembership type added. - api.workspaces.me(slug) added. Refactor: - settings/+page.svelte, [collection]/+page.svelte, [collection]/[slug]/+page.svelte: drop open-coded role derivation (members.find + m.role open-codes), consume workspaceStore.isOwner. members.list calls remain — still needed for assignee dropdowns / member rows in settings — only the role-derivation path moves to the store. Tests: - server: handlers_me_test.go covers 6 scenarios (admin, editor with all-access, viewer with collection grant, restricted member, guest with item grant, non-member with no grants). - frontend unit tests deferred — web/ has no unit-test runner today. Pure-function module makes them trivial to add when the runner lands. Cascade is independently covered by store/permissions_test.go and store/grants_test.go on the server. Parent: PLAN-1100. * fix(workspace): per-item visibility uses strict full-access set + setCurrent race guard per Codex review (round 1) P1: canViewItem fell back to canViewCollection, which uses the broad nav set (visible_collection_ids — includes collections containing item-granted items so they appear in nav). This meant a guest with one ItemGrant on TASK-5 in Tasks would see canViewItem(any-other-task-in-Tasks) return true, while the server only allows direct item grants or full collection grants. Fix: /me now also returns full_access_collection_ids — the strict set of collections in which every item is accessible (collection grants + member_collection_access + system collections; item-grant collections intentionally excluded). This mirrors guestResourceFilter's fullCollIDs in handlers. canViewItem and canEditItem now consult full_access_collection_ids on the membership-fallthrough path, NOT the nav set. Test added: TestMe_GuestWithItemGrant now asserts the item-grant collection is in visible_collection_ids (nav) but NOT in full_access_collection_ids (strict). TestMe_RestrictedMember updated to check both sets. P2: workspaceStore.setCurrent had no guard against stale async /me responses. A slow /me for workspace A could clobber a freshly-fetched membership for workspace B if the user navigated mid-flight, briefly exposing permission-gated UI for the wrong workspace. Fix: monotonic membershipSeq counter incremented per setCurrent / create call. Each /me response only writes back if its captured token still matches at resolution time. Also clears currentMembership immediately on setCurrent so helpers don't briefly answer "yes" using the previous workspace's grants while /me is in flight. Parent: PLAN-1100. Refs TASK-1101 PR #415. * fix(workspace): canEditCollection uses strict full-access set per Codex review (round 2) Same nav-vs-strict bug pattern as round 1's canViewItem fix, but in canEditCollection. The editor-membership fallback path previously gated on canViewCollection (broad nav predicate using visible_collection_ids), which incorrectly returned true for a restricted editor whose only access to a collection was an item grant. The collection appears in nav (correct) but the editor must NOT see collection-wide write affordances like "+ New" because the server rejects collection-level writes there. Fix: editor membership fallback now requires either collection_access === "all" or the collection to be in full_access_collection_ids. canEditItem already used full_access_collection_ids on its fallback path (it was added in round 1) — verified unchanged. Parent: PLAN-1100. Refs TASK-1101 PR #415.	2026-05-05 08:52:05 -04:00
xarmian	abf017c4e7	feat(onboarding): make banner + CLI hint template-aware (TASK-1150) (#409) ... The IDEA-1 trigger phrase is no longer hardcoded — fresh scrum workspaces surface "use pad to get BACK-1", product workspaces surface "use pad to get FEAT-1", and any future template that ships an agent-onboarding seed declares its primary ref once and gets the banner / hint for free. Mechanism: 1. WorkspaceTemplate gains an OnboardingPrimaryRef string field — the canonical declaration of "this template's IDEA-1-style primary entry." Set per template that ships the pattern (startup → "IDEA-1", scrum → "BACK-1", product → "FEAT-1"); left empty for hiring/interviewing/demo where the agent-onboarding pattern intentionally doesn't apply. 2. Server: handleGetDashboard identifies the seeded primary by walking allItems looking for item_number=1 + source="template" + created_by="system" + collection_slug ∈ {ideas, backlog, features}. The collection-slug whitelist is what keeps hiring's REQ-1 (also seeded with item_number=1 + source=template) from being flagged as an onboarding entry — those are example items, not agent scripts. The dashboard response gains an onboarding_seed field with ref/title/slug/collection_slug/status plus a server-computed `active` boolean (true iff status equals the schema initial value). 3. CLI: printOnboardingHints accepts the template name, looks up the primary ref via collections.GetTemplate, and prints the right "use pad to get X-1" line. Templates without a declared primary skip the line entirely (so hiring's pad init success doesn't promise a non-existent BACK-1 / IDEA-1). 4. Web frontend: dashboard reads dashboard.onboarding_seed, gates the banner on `active=true`, passes ref/slug/collection to OnboardingIdeaBanner. The component renders the trigger phrase, copy button, and "Read it first" deep link from those props — no more hardcoded IDEA-1. ensureWorkspace's signature gains a returned templateName so init.go + main.go can pass it through to printOnboardingHints. The five existing test call sites updated. New tests: internal/collections/templates_test.go - TestTemplatesDeclareOnboardingPrimaryRef — locks the per-template OnboardingPrimaryRef values (and the explicit emptiness of hiring/interviewing/demo). internal/server/handlers_dashboard_test.go - TestDashboardOnboardingSeed_StartupTemplate - TestDashboardOnboardingSeed_ScrumTemplate - TestDashboardOnboardingSeed_ProductTemplate - TestDashboardOnboardingSeed_HiringTemplate (asserts NO seed — hiring's REQ-1 is example data, not an onboarding entry) - TestDashboardOnboardingSeed_EmptyWorkspace (no template) Removes the loadIdeaOne race-guard from +page.svelte — the dashboard poll itself now carries the onboarding_seed.active flag so the banner state lives entirely in the dashboard response. Drops ~50 lines of frontend code. Parent: PLAN-1146.	2026-05-04 12:05:07 -04:00
xarmian	40621ff58d	feat(metrics): session-id-keyed TTL sweep for mcp_active_sessions (TASK-1120) (#400) ... * feat(metrics): session-id-keyed TTL sweep for mcp_active_sessions (TASK-1120) Replaces the naive +1/-1 active-sessions accounting from TASK-961. The old logic bumped on JSON-RPC `initialize` and decremented on HTTP DELETE — but a client that crashed, lost network, or restarted mid-session never emitted DELETE, so the gauge drifted upward monotonically until the pad-cloud server restarted. Approach: - `internal/server/middleware_mcp_session.go` (new) — mcpSessionTracker is an in-memory map keyed by Mcp-Session-Id (the canonical header set by mcp-go's StreamableHTTPServer on initialize responses and echoed by the client on subsequent requests). Touch updates lastSeen on insert + refresh; evict removes; periodic sweep evicts entries older than the TTL. - Gauge is `Set(len(sessions))` via an onChange callback — single consistent observation per state-changing op, no risk of gauge drifting from map size on a multi-evict sweep. - Lifecycle: spawned by SetMCPTransport (alongside startMCPAuditWriter), shut down from Server.Stop. Idempotent on both sides. - Configurable via PAD_MCP_SESSION_TTL (default 30m) and PAD_MCP_SESSION_SWEEP_INTERVAL (default 5m). cmd/pad calls Server.SetMCPSessionTrackerConfig before SetMCPTransport. Other changes: - `recordMCPCallMetrics` no longer touches the active-sessions gauge. Updated comment + signature kept (callers pass the same args; the unused params are explicitly underscored). - `MCPAuditLog` middleware now calls trackMCPSession after next.ServeHTTP — single new line in the audit hot path. - `TestMCPAudit_BufferFull_DropsAndIncrementsCounter` updated to also shut down the new session tracker before bg.Wait(), since SetMCPTransport now spawns two goroutines on srv.bg. Test coverage (16 tests, all green under -race): - Tracker unit: touch insert/dedup, empty-id no-op, evict remove/non-existent, sweep eviction with single onChange, nil-onChange safety, concurrent touch/evict, run() clean shutdown. - Server-side integration: lifecycle happy path (initialize → call → DELETE leaves gauge at 0), failed initialize doesn't open, no-session-id no-op, nil tracker safety, idempotent start, DELETE evicts on any status (transient 5xx on shutdown still counts). - Regression guard: TestRecordMCPCallMetrics_DoesNotTouchSessionGauge pins that the audit-side helper has migrated off the gauge. Parent: PLAN-943. Follow-up to TASK-961 (PR #398). Closes the "sessions drift on client crashes" caveat documented in the metric's help text + the Grafana panel description. * fix(metrics): emit Mcp-Session-Id + serialize gauge updates per Codex review (round 1) Two findings from Codex review on PR #400: 1. WithStateLess(true) wired StatelessSessionIdManager whose Generate() returns "" — mcp-go never set the Mcp-Session-Id response header in production, so the new tracker no-op'd on every initialize and the active-sessions gauge stayed at 0. Fix: introduce padMCPGenerateOnlySessionIDManager in cmd/pad/main.go. Generates a UUID per initialize (so the response carries the header — tracker can observe), but Validate accepts ANY incoming value (including empty / arbitrary). Preserves the original "stateless server, every request stands alone" contract while making the session-id observable. Documented why mcp-go's two shipped stateless managers don't fit (one breaks observability, the other breaks back-compat for clients that never echo the ID). 2. touch / evict / sweep computed `len(sessions)` under the mutex then released the lock BEFORE invoking onChange. Two concurrent inserts could compute (n=1, n=2) under the lock and then race the callback writes — last writer wins on the gauge, leaving it permanently inconsistent with the map size. Fix: hold the mutex across onChange. Trade-off documented: any future onChange that re-enters the tracker would deadlock, but that's a clear failure mode rather than silent metric corruption. Added TestMCPSessionTracker_OnChangeUnderLock that asserts a strictly-monotonic observation sequence under 32-goroutine concurrent inserts; passes 5x in a row under -race.	2026-05-03 17:40:18 -04:00
xarmian	1c409c8592	feat(metrics): emit mcp_authz_denials_total{reason=tier_mismatch} (TASK-1119) (#399) ... Wire the dispatcher-side scope-deny seam into the pad_mcp_authz_denials_total counter, completing the denial-reason vocabulary documented in TASK-961. internal/mcp/dispatch_http.go: - Add optional OnScopeDenied(method, urlPath) callback on HTTPHandlerDispatcher - Fire it from buildAuthedRequest right before returning the existing permission_denied error — same control flow, just observability added in front internal/server/middleware_auth.go: - Public Server.RecordMCPTierMismatch helper that bumps the counter. No MCP-origin context gate (unlike recordMCPAuthzDenial below) — the dispatcher is by construction MCP-only, so every invocation is inherently MCP-origin. cmd/pad/main.go: - Wire dispatcher.OnScopeDenied = srv.RecordMCPTierMismatch alongside the existing UserResolver / Lister fields. Safe to attach unconditionally — RecordMCPTierMismatch nil-checks metrics internally, mirroring the OAuth observer wiring pattern. Tests: - Three new dispatcher tests covering OnScopeDenied: fires once with the right (method, urlPath) on deny; does NOT fire on allow; nil hook is safe. - Server-side test for RecordMCPTierMismatch: counter increments, other denial reasons untouched, nil-metrics safe. Parent: PLAN-943. Follow-up to TASK-961 (PR #398).	2026-05-03 16:55:30 -04:00
xarmian	98c8b78d06	feat(metrics): MCP + OAuth observability metrics for /mcp (TASK-961) (#398) ... Plug MCP traffic and OAuth flow events into pad's existing internal/metrics Prometheus surface, plus a Grafana dashboard. Metrics (all under pad_*): - Counters: mcp_tool_calls_total{user_id,tool,status}, mcp_authz_denials_total{reason}, oauth_flows_total{stage}, oauth_token_revocations_total{reason} - Histograms: mcp_tool_call_duration_seconds{tool}, oauth_flow_duration_seconds{stage}, oauth_token_ttl_seconds - Gauges: mcp_active_sessions, oauth_active_tokens (callback collector) Wiring seams: MCPAuditLog (per-call), MCPBearerAuth (audience denials), emitMCPAuditDenied (rate-limit denials), RequireWorkspaceAccess (gated to MCP-origin via context — workspace_not_in_allowlist + not_a_member), OAuth handlers (per-stage flow events + per-handler latency), and internal/oauth/storage.go via a new SetRevocationObserver hook so the OAuth package stays metrics-naive. Cmd/pad wires both observers via Server.wireOAuthMetricsObserver(), called from both SetMetrics and SetOAuthServer for order-independence. Store helpers added (with full test coverage): - CountActiveOAuthAccessTokens — backs the active-tokens gauge - OldestAccessTokenIssuedAtByRequestID — backs the TTL observation Grafana dashboard at monitoring/grafana/mcp.json: 13 panels across MCP traffic + OAuth flow rows (rate-by-tool, p50/p95/p99 latency, status breakdown, denial reasons, active sessions, top-10 users, OAuth flow events by stage, OAuth handler p95, active tokens, revocations by reason, TTL p50/p95). Codex review caught one HIGH issue (round 1, fixed in same commit): the active-tokens collector originally emitted NewInvalidMetric on provider error, which propagates through Registry.Gather() and fails the entire /metrics scrape via promhttp's default error handler. Switched to log + skip-the-sample so a transient SQLite blip drops ONE gauge for one scrape rather than the whole observability surface. Added TestRegisterOAuthActiveTokensCollector_ErrorIsScrapeSafe to pin the contract. Tests cover increments, histogram bucket placement, callback collector freshness across mutations + error path, observer hook firing on user- initiated revocation + rotation + nil-safety, and per-helper unit tests for the server-side metric emission. Verified with `make check` (golangci-lint + go test ./... + web build).	2026-05-03 16:37:49 -04:00
xarmian	a1179f1c07	feat(dashboard): broaden agent-activity signal to include MCP source (TASK-1112) (#394) ... Renames the "has_cli_source" signal to "has_agent_activity" — semantically the dashboard flag for "this workspace's agent loop is wired up." Existing behavior is preserved (CLI activity still flips it on); the SQL widens to match source IN ('cli', 'mcp') so the signal stays correct if attribution is later split (today, all MCP-via-HTTPHandlerDispatcher activity persists as source='cli' per dispatch_http_test.go's contract). Renames: - store: WorkspaceHasCLISource → WorkspaceHasAgentActivity - dashboard struct: HasCLISource → HasAgentActivity - JSON tag: has_cli_source → has_agent_activity - Svelte state: hasCliSource → hasAgentActivity - Svelte fn: refreshHasCliSource → refreshHasAgentActivity - TS field: has_cli_source → has_agent_activity (DashboardData) - Test: TestWorkspaceHasCLISource* → TestWorkspaceHasAgentActivity* New test case in TestWorkspaceHasAgentActivity asserts that an item with source='mcp' also flips the signal on, exercising the broadened SQL clause. Comment updates explain today's "MCP attribution = source='cli'" reality so future readers don't search in vain for source='mcp' writers. The Svelte localStorage dismiss key (`pad-cli-banner-dismissed-`) is left unchanged in this PR — TASK-1114 will rename it with a soft-migration read of the old key for one release. This PR's goal is the rename + signal broadening, not the banner UX refactor. Unblocks TASK-1114 (banner two-mode refactor). Parent: PLAN-1111.	2026-05-03 10:50:54 -04:00
xarmian	92c05cb029	feat(auth): expose mcp_public_url on /auth/session (TASK-1113) (#393) ... Adds mcp_public_url to the /auth/session response (and the parallel setupStatePayload for the pre-bootstrap state). Sourced from the existing s.mcpPublicURL field that SetMCPTransport populates from PAD_MCP_PUBLIC_URL at startup. Empty string when unset — never null, never absent — so the web UI can branch on `mcp_public_url !== ''` as the gate for "this Pad instance exposes a Remote MCP server." Frontend gets a parallel `authStore.mcpPublicUrl` getter mirroring the existing `cloudMode` pattern. AuthSession.mcp_public_url is typed as required (string), since the server always emits it. Tests cover both shapes: empty string when PAD_MCP_PUBLIC_URL is unset (both pre-setup and post-bootstrap), and verbatim echo when configured. Unblocks TASK-1114 (banner two-mode refactor) which gates on this field. Parent: PLAN-1111. Note: AuthSession lives in web/src/lib/api/client.ts, not types/index.ts — the task description had the wrong file. Type was edited in client.ts.	2026-05-03 10:40:44 -04:00
xarmian	9b2234fce6	fix(workspaces): scope admin's personal workspace list to memberships (BUG-982) (#392) ... handleListWorkspaces special-cased server admins, routing them through an unfiltered store query that returned every non-deleted workspace regardless of membership. The admin's "switcher" therefore showed workspaces they had no member row in, labeled "shared with me" by the frontend even though they weren't actually shared. Filed in BUG-982 by the admin who saw the leak; the underlying mechanism would have leaked workspace metadata to any future server admin. The fix routes admins through the same GetUserWorkspaces path as every other authenticated user. Cross-tenant visibility for admins is still available via the admin-panel routes (/api/v1/admin/...), which call ListWorkspaces() directly with the appropriate auth gate — that's the correct surface for "see all workspaces on this server." Drive-by cleanups along the way: - Add ws.HydrateDerivedFields() to both branches of GetUserWorkspaces (member + guest) for parity with the admin path's previous behavior. Workspace context fields now hydrate consistently across all callers. - Delete the unused ListWorkspacesForUser store function. Its name implied per-user filtering, its body returned every workspace — pure footgun for any future code that grepped by name. Inline the no-userID branch into ListWorkspaces() (still used by the admin panel and pre-auth bootstrap). OUT OF SCOPE — handled by a follow-up Plan parented to PLAN-259 (Security Review): middleware_auth.go:449 still grants server admins implicit `owner` role on every workspace they navigate to. This PR closes the *listing* leak so admins no longer see workspaces in their switcher. It does NOT yet address the deeper concern in BUG-982's body — that on pad-cloud, admin access to other tenants should require an explicit auditable escalation flow (confirmation, audit log entry, owner notification, time-bound session, visible escalation banner). That's design-heavy and gets its own Plan. Tests: new internal/server/handlers_workspaces_test.go verifies that an admin who is NOT a member of a workspace does not see it in their listing, and that adding them as an explicit member restores visibility. Sister test confirms the existing non-admin behavior is unchanged. Both pass on SQLite and on Postgres via make test-pg. Full ./internal/server and ./internal/store suites stay green.	2026-05-03 00:35:31 -04:00
xarmian	6e4c7f617b	fix(timeline): drop \xff cursor sentinel that broke Postgres pagination (BUG-1086) (#391) ... The timeline handler defaulted the cursor's beforeID to the literal byte "\xff" as a sentinel intended to "sort after any UUID". SQLite tolerates that in TEXT columns, but Postgres rejects it as an invalid UTF-8 byte sequence (SQLSTATE 22021 — "invalid byte sequence for encoding 'UTF8': 0xff"), causing every timeline tab load on pad cloud to return 500. Reproduced empirically against the test Postgres with a one-line probe that issues a TEXT-typed bind of "\xff" — same error string the bug reported. The fix removes the sentinel and distinguishes three cursor cases in the handler: 1. Neither `before` nor `before_id` (true first page) → store gets beforeID = "" and drops the id tie-breaker from the WHERE clause entirely. Just `WHERE created_at < ?`. 2. Both supplied (normal cursor pagination) → unchanged. 3. `before` supplied without `before_id` (anomalous but possible for external clients) → use "g" as a UUID-safe upper-bound sentinel. Lowercase-hex UUIDs are bounded by "f", so "g" sorts above them in every reasonable collation while remaining valid UTF-8. This preserves the legacy semantics of including same-second rows that the naive `created_at < ?` would drop — a regression Codex caught on round 1 of review. The id-predicate branching is applied symmetrically across all three *BeforeTime store functions: ListCommentsBeforeTime, ListDocumentActivityBeforeTime, ListItemVersionsBeforeTime. New test file internal/store/timeline_pagination_test.go covers: - No-cursor first-page path (the broken one) - Real (timestamp, id) cursor pagination - Same-second cursor with sentinel id (regression guard) - Limit respected - Activity and version BeforeTime no-cursor paths All six pass on SQLite and on real Postgres via `make test-pg`. Full ./internal/store and ./internal/server suites stay green on Postgres.	2026-05-03 00:12:50 -04:00
xarmian	f9d3244660	feat(connected-apps): user-facing OAuth connection management page (TASK-954) (#390) ... * feat(connected-apps): user-facing OAuth connection management page (TASK-954) Adds /console/connected-apps where a logged-in user can see every OAuth grant chain they've authorized via the MCP consent flow (Claude Desktop, Cursor, …) and revoke any of them. Joins to the DCR client metadata for the display name + logo, and to the MCP audit log (TASK-960) for the "last used" + "30-day calls" columns. Pieces: - internal/store/connected_apps.go — ListUserOAuthConnections walks oauth_access_tokens + oauth_refresh_tokens, dedups by request_id, hydrates client metadata, parses session_data for the workspace allow-list, classifies granted_scopes into a coarse capability tier. RevokeUserOAuthConnection verifies ownership (ErrConnection NotFound for stranger's chains — anti-enumeration; same shape as for unknown chains) then calls the existing RevokeRefreshTokenFamily + RevokeAccessTokenFamily so the next /mcp call gets 401. - internal/models/connected_apps.go — OAuthConnection + CapabilityTier models. - internal/server/handlers_connected_apps.go — REST endpoints: GET /api/v1/connected-apps (list) + DELETE /api/v1/connected-apps/{id} (revoke, idempotent, 204). Wrapped in requireCloudMode group. List enriches with MCPConnectionStatsForUser (audit aggregates) — soft-fails on the audit lookup so a broken audit table degrades to "no last-used data" instead of a broken page. Revoke records an "oauth_connection_revoked" entry in audit_trail via the existing CreateActivity path. - web/src/routes/console/connected-apps/+page.svelte — list with per-app card (logo, name, capability badge, workspace chips with +N expander, connected/last-used relative times, 30-day count), Details expander showing scope_string + workspace list + redirect URIs, Revoke button → confirm modal → optimistic refresh, friendly empty state linking to /connect. - web/src/routes/console/+layout.svelte — Connected Apps nav link (cloud-mode-gated, between Settings and Billing). - web/src/lib/api/client.ts + types/index.ts — typed client + ConnectedApp interface. Tests cover: - Store: chain dedup across rotation siblings, subject filtering (Bob can't see Alice's), inactive chains excluded, ownership check on revoke, idempotent re-revoke, capability tier mapping, session-data allowed_workspaces parsing (both []string and JSON []interface{} round-trips). - Handler: cloud-mode gate (404 outside), owner-only filtering, DTO field shape + audit enrichment populating last_used_at + calls_30d, revoke ownership 404 (not 403 — anti-enumeration), idempotent 204, audit_trail row written. `make check` clean (lint + go test ./... + svelte-kit build). Parent: PLAN-943. * fix(connected-apps): point empty-state link at getpad.dev (Codex review round 1) Codex caught: the empty-state link to /connect 404s because /connect is a pad-web (marketing site) route, not a docapp route. From inside the authenticated console at app.getpad.dev, the right target is the absolute https://getpad.dev/connect URL — same pattern the +error.svelte page uses for its "Back to getpad.dev" + "/docs" links. * fix(console nav): exclude /console/connected-apps from Workspaces active match (Codex round 2) Codex caught: the Workspaces nav predicate `isActive('/console') && !isActive('/console/settings') && ...` was missing the new /console/connected-apps prefix, so both Workspaces AND Connected Apps lit up when viewing the connected-apps page. Same shape as the existing exclusions for settings / billing / admin.	2026-05-02 23:21:07 -04:00
xarmian	d8b1d98e08	feat(mcp): persistent audit log for /mcp tool calls (TASK-960) (#389) ... * feat(mcp): persistent audit log for /mcp tool calls (TASK-960) Adds a 90-day-retention audit log of every MCP request. Drives the "last used" + "30-day calls" columns the connected-apps page (TASK-954) will read, and gives ops + on-call a forensics surface via a new admin /console/admin/mcp-audit page. Schema deviation from the spec, documented in migration 049: the original task body called for `token_id REFERENCES oauth_tokens(id)` but pad has no `oauth_tokens` table — instead an OAuth grant chain is identified by `request_id` (preserved across refresh-token rotations, see migration 048), and PAT-authenticated MCP requests have no OAuth identity at all. The audit row therefore carries `(token_kind, token_ref)` — `oauth` + request_id for OAuth, or `pat` + api_tokens.id for PATs. The connected-apps page in TASK-954 will filter on token_kind='oauth' to surface third-party connections only. Pieces: - internal/store/migrations/049_mcp_audit.sql + pgmigrations/028 — table. - internal/models/mcp_audit.go — typed entry + 30-day stats DTO. - internal/store/mcp_audit.go — insert / list-by-user / list-by-connection / list-all / per-connection-stats aggregator / 90-day retention sweeper. - internal/server/middleware_mcp_audit.go — async writer + sweeper + middleware that wraps /mcp behind MCPBearerAuth. Hot path is non-blocking enqueue with drop-on-overflow + atomic drop counter. - internal/server/middleware_mcp_auth.go — both PAT + OAuth branches now stash WithMCPTokenIdentity so the audit row attributes correctly. - internal/server/handlers_mcp_audit.go — read endpoints: GET /api/v1/connected-apps/{id}/audit (owner-scoped) + GET /api/v1/admin/mcp-audit (admin-only). - web/src/routes/console/admin/mcp-audit/+page.svelte + tab in admin layout. - Tests cover required-field validation, round-trip, pagination, owner-only filtering, last-used + 30-day aggregates, retention sweep, body-sniff parser, canonical-JSON arg hashing, buffer-full drop path, status-to-result classification, admin gate, DTO field shape. `make check` clean (lint + go test ./... + svelte-kit build). Parent: PLAN-943. * fix(mcp-audit): emit denied row on rate-limit reject per Codex review (round 1) PR #389 round 1 caught: MCPAuditLog is mounted INSIDE MCPBearerAuth, so when bearer auth's per-token rate-limit fires (429) it returns before next.ServeHTTP — and the wrapping audit middleware never sees the response. classifyMCPResult mapped 401/403/429 with no path that could actually reach it. Fix: emitMCPAuditDenied helper called directly from the rate-limit deny branches of both PAT + OAuth paths. Resolved user + token identity are already in scope at that point, so the audit row gets attributed correctly. Pre-auth rejections (no/invalid bearer) stay un-audited because there's no user to attribute them to and the audit_trail table covers those auth-event signals already. Threading: handleMCPPATAuth + handleMCPOAuthAuth now take the entry timestamp so the denied row carries real latency. Test: TestMCPAudit_RateLimited_RecordsDeniedRow drives a real PAT through the rate limiter, drains to 429, and asserts the audit row lands with status="denied" + error_kind="rate_limited" + the right tool_name from the request body.	2026-05-02 22:56:10 -04:00
xarmian	22f6342794	fix(mcp): bundle BUG-1081 + BUG-1082 + TASK-1076 — three small MCP-UX fixes from dogfooding (#387) ... All three caught in Claude Desktop's second-round review against the deployed cloud build. Independent file surfaces, but they all polish the same MCP-tool-call user experience so they ride together. ## BUG-1081: star/unstar return structured JSON instead of 204 internal/server/handlers_stars.go — `handleStarItem` and `handleUnstarItem` previously returned 204 No Content. RESTfully fine, but the MCP HTTPHandlerDispatcher passes through whatever the handler wrote — empty body + 204 → empty MCP tool result. Agents had no signal whether the operation landed. BUG-989's earlier fix touched the CLI's text output via the JSON branch but missed the API endpoint itself. Fix: both endpoints now return 200 OK with `{ref, starred: bool}`. Mirrors the shape Claude's review requested + the broader "return enough info to be the next source of truth" pattern note/decide adopted. New test pins the wire shape including content-type. Negative control verified — reverting the handler fails the test with "expected 200, got 204" on the first assertion. ## BUG-1082: suggested_next surfaces orphans, not just plan-children internal/server/handlers_dashboard.go — the candidate loop only walked items that are children of an active plan. Workspaces without active plans (or with in-progress / high-priority items outside their active plans) got an empty suggested_next, even when the obvious answer was "continue your one in-progress task." BUG-990's earlier fix added in-progress to the active-plan scope but kept the orphan branch in scope-creep territory. Real dogfooding showed it's the common case for new workspaces. Fix: add a second pass that scans all items for in-progress (any priority — continuation always beats priority) and high/critical- priority open items not already in the active-plan candidates. Orphans rank lower than plan-children so existing plan-driven behavior is preserved when both are present. Reason text drops the plan-name reference for orphans. Two existing tests pinned the OLD "no suggestions when no active plans" behavior — that was pinning the bug. Updated to the new correct behavior. Added two new tests pinning the in-progress- beats-priority gating and the rank-below-active-plan ordering. ## TASK-1076: workspace auto-default from OAuth allow-list internal/mcp/dispatch_http.go + internal/mcp/dispatch_http_advanced.go — the dispatcher's preprocess flow now calls `maybeInjectWorkspace` after the existing --assign / --role resolution. When: - input["workspace"] is set → caller wins (no override) - d.Lister is nil → no-op (tests + non-OAuth paths) - lister returns 1 workspace → inject input["workspace"] = slug - lister returns 0 or N → no-op (caller must pass explicitly; route mapper's existing "missing required input" error surfaces if the route needs workspace) The lister already encodes the right policy (PAT auth → all the user's workspaces; wildcard token → same; specific allow-list → intersection with memberships) so we reuse it instead of building a parallel resolver. Auto-defaulting only when the resolved set collapses to one is the unambiguous case; multi-workspace tokens still require explicit choice (silently picking one would be a real audience-confusion hazard for write operations). Caller-passed workspace ALWAYS wins — agents that pass an explicit slug never see it silently overridden by the default. Lister error falls through to no-op (don't poison input on transient store hiccup). Tests pin all four matrix cases from the task spec plus three operational corner cases (nil lister, lister error, copy-on-write non-mutation). ## Combined CI surface `make check` clean across lint + tests + web. The test surface gained: - TestStarUnstar_ReturnsStructuredJSON (server) - TestDashboardSuggestedNextOrphan_InProgressBeatsPriority (server) - TestDashboardSuggestedNextOrphan_RanksBelowActivePlan (server) - TestMaybeInjectWorkspace_* (mcp, 7 cases) - TestDashboardSuggestedNextNoPlans + TestDashboardSuggestedNextFromPlannedPlan reframed from pin-the-old-bug to pin-the-new-correct-behavior	2026-05-02 21:31:48 -04:00
xarmian	7429de3933	fix(oauth): CSP nonce on consent screen so the inline UI-state script can run (#383) ... Pasting the bare https://mcp.getpad.dev URL into Claude Desktop now reaches pad's consent screen, but the Allow button stays disabled even when the user picks workspaces. Cause: the consent template ships UI-state JS in an inline <script> block (workspace selection flips disabled=false on the Allow button + handles the wildcard mutual-exclusion warning), but pad's strict response CSP is "script-src 'self'" with no 'unsafe-inline' and no nonce — so the browser silently blocks the inline script and the Allow button stays at its initial disabled=true. Adopts the same nonce + strict-dynamic CSP pattern pad already uses for the SvelteKit SPA bootstrap (see server.go's setupRouter SPA route): renderConsent generates a per-request nonce via generateCSPNonce, sets a CSP header that authorizes that nonce ("script-src 'self' 'nonce-X' 'strict-dynamic'") before writing the body, and threads the same nonce into the template's <script> tag's nonce attribute. This is per-handler (overrides SecurityHeaders middleware on the consent response only); every other endpoint keeps the strict no-nonce baseline. matches the existing SPA-bootstrap nonce path exactly so future security-hardening on either side stays self-consistent. Adds TestOAuth_ConsentScreen_NonceCSPLetsInlineScriptRun pinning two facts: 1. CSP on the consent response carries a 'nonce-...' token in script-src (proves the override fired and we didn't fall back to the strict baseline). 2. The body's <script> tag carries the SAME nonce value (proves the two are linked — drift would re-introduce the bug). Both are necessary; either failing causes the browser to block.	2026-05-02 19:21:25 -04:00
xarmian	229d47e189	fix(oauth): treat empty-path/root trailing slash as equivalent (RFC 3986 §6.2.3) (#382) ... Real OAuth clients reconstruct the resource indicator from the URL the user pasted. URL parsing canonicalizes empty path → "/", so a client given "https://mcp.getpad.dev" emits "resource=https://mcp.getpad.dev/" — with a trailing slash that pad's canonical "https://mcp.getpad.dev" doesn't have. The strict string compare in audienceMatchingStrategy (and the matching audienceContains check on the RS side at /mcp) rejected these as distinct audiences and the connector flow died on "Requested audience https://mcp.getpad.dev/ is not the canonical audience https://mcp.getpad.dev." Per RFC 3986 §6.2.3 (Scheme-Based Normalization) those forms ARE equivalent for the HTTP scheme. Adds NormalizeAudience(s) and applies it on both sides of every audience comparison: - internal/oauth/audience.go: audienceMatchingStrategy normalizes the canonical, then checks each needle and the haystack against it via audienceListContainsNormalized. - internal/server/middleware_mcp_auth.go: audienceContains (the RS-side gate at /mcp) normalizes both sides too. Mirroring the rule keeps AS and RS in lockstep — without it, tokens the AS minted for a slashed audience would fail validation at /mcp. Per Codex review #386 round 1, normalization is restricted to URIs whose path component is exactly the root ("/"). Earlier draft trimmed ANY trailing "/", which would have made "https://host/mcp" and "https://host/mcp/" compare equal — distinct HTTP resources collapsing to one audience is a real audience-confusion attack surface. The boundary is enforced via url.Parse: only normalize when u.Host is non-empty AND u.Path == "/" AND there's no query/fragment. Anything else returns byte-exact. TestNormalizeAudience pins both branches (root case trims; non-root paths, hostless strings, queries, fragments, and unparseable inputs all stay as-is). TestAudienceStrategy_PathSlashIsNotEquivalent guards the strategy layer directly: even with normalization active, "/mcp" and "/mcp/" are kept distinct.	2026-05-02 19:01:56 -04:00
xarmian	ba303e456f	fix(mcp): publish PAD_MCP_PUBLIC_URL verbatim as canonical resource (no /mcp suffix) (#381) ... Per the MCP authorization spec the client MUST verify the URL it was given matches the discovery doc's `resource` field exactly; auto- suffixing was forcing operators publishing the bare hostname (the industry convention — mcp.stripe.com, mcp.linear.app, mcp.atlassian.com) into a permanent client-side mismatch and Claude Desktop / Cursor reject pasting `https://mcp.getpad.dev` even though everything else works. Both production sites that previously appended "/mcp" to MCPPublicURL now use the value verbatim: - cmd/pad/main.go: AllowedAudience for the OAuth server constructor. Tokens are now audience-bound to MCPPublicURL exactly. - internal/server/handlers_well_known.go: the protected-resource discovery doc's `resource` field is the bare MCPPublicURL. The transport itself is unchanged — pad still mounts at /mcp on the chi router; pad-cloud's nginx router transparently rewrites mcp.* root → /mcp (TASK-997 PR #28) so external clients see a single canonical URL regardless of the internal HTTP path. The audience binding is just a string; it doesn't have to equal the internal mount path. config.go's MCPPublicURL doc updated to reflect the new semantic ("canonical URL clients paste") rather than the old "vhost URL we suffix-mangle". Operators who want the old shape just include the /mcp suffix in PAD_MCP_PUBLIC_URL — the operator owns the canonical. Test fixtures: testCanonicalAudience flipped from "https://mcp.test.example/mcp" to "https://mcp.test.example", and the two SetMCPTransport call sites that previously stripped /mcp now pass it directly. The TestMCP_DiscoveryDoc_PopulatedFromConfig assertion uses testCanonicalAudience so future renames stay consistent. All other test sites (audience= form fields, aud claim checks, mismatch fixtures) keep working unchanged because they reference testCanonicalAudience symbolically.	2026-05-02 18:36:55 -04:00
xarmian	69e471db8f	fix(oauth): default to canonical audience when client omits RFC 8707 resource= (TASK-951) (#380) ... * fix(oauth): default to canonical audience when client omits RFC 8707 resource= (TASK-951) Real MCP clients (Claude Desktop, Cursor as of 2026-05) don't send the RFC 8707 `resource` parameter on /oauth/authorize at all. Before this fix, translateResourceToAudience only translated resource→audience when resource= was present, so empty-resource requests reached fosite's audienceMatchingStrategy with an empty needle and got rejected with "resource parameter is required (RFC 8707)". fosite then redirected to the client's redirect_uri with ?error=invalid_request&error_description=..., and Claude's backend callback failed with the pydantic envelope "code: Field required" (because no `code` parameter was in the redirect query). RFC 8707 §2 marks the resource parameter OPTIONAL; servers with a single canonical audience are expected to default to it. pad's OAuth server has exactly one canonical audience by construction (cfg.MCPPublicURL + "/mcp"), so the right policy is to inject canonical when the client sends neither resource= nor audience=. Now translateResourceToAudience handles three cases in priority order: 1. audience= already set — leave both keys untouched. 2. resource= present — copy to audience= (existing path). 3. Neither present — inject canonical into both. The token gets bound to canonical exactly as if the client had sent it. audienceMatchingStrategy's strict empty-needle reject stays as defense in depth: case 3 only fires when canonical is configured (main.go won't construct the OAuth server otherwise), but if some future code path bypasses the translation helper, the matching strategy still fails loudly rather than minting an unbound token. Adds TestOAuth_Authorize_AcceptsNoResource_DefaultsToCanonical pinning Claude Desktop's exact request shape (no resource=, no audience=). Pairs with the existing AcceptsResourceOnly and audience-mismatch tests to lock in the full /authorize matrix. * docs(oauth): document RFC 8707 cross-server replay trade-off + audit log Per Codex review #383 round 1: defaulting to canonical when the client omits resource= weakens the cross-server replay defense RFC 8707 was designed to provide. Threat is the confused-deputy attack — malicious MCP server lies that pad's AS is its AS, client (which doesn't send resource=) drives a flow against pad's AS, pad mints a token bound to canonical, client returns it to the attacker, attacker replays at pad's /mcp. We're shipping with the default-to-canonical path because every real-world MCP client (Claude Desktop / Cursor / ChatGPT as of 2026-05) omits resource= and the alternative is "remote MCP doesn't work for any client until the entire ecosystem adopts RFC 8707." Mitigations now documented in the comment + active in the code: - Consent screen (TASK-952) is the trust anchor. Every grant requires a click-through that identifies the resource as "your Pad workspaces" and lists the user's actual workspace names. A user attempting to connect to a non-pad MCP server who lands on pad's consent screen sees the mismatch. - Matches industry practice (GitHub / Google / Atlassian all rely on consent-as-trust-anchor since RFC 8707 is barely deployed). - audienceMatchingStrategy's strict empty-needle reject stays as defense in depth — fires when canonical is unset and on any future code path that bypasses the helper. - Audit log (slog.Warn) on every default-fire gives ops a signal to detect anomalies — a spike of defaulted requests from a previously-unseen client_id is the earliest detectable shape of a confused-deputy attempt. Future task tracks restoring the strict reject once Claude / Cursor / ChatGPT all send resource=.	2026-05-02 16:50:34 -04:00
xarmian	3319ad5ea1	feat(mcp): per-token rate limit on /mcp (TASK-959) (#378) ... * feat(mcp): per-token rate limit on /mcp (TASK-959) Add a per-token rate limit to /mcp's auth middleware. Closes the "runaway agent burns through user quota" gap that PLAN-943 left as a follow-up to TASK-950. ## Policy - 60 requests / minute / token, burst 20. - Per-token (not per-IP): office-NAT-shared users don't share a bucket, and a runaway agent on one token can't burn another token's quota for the same user. - Limiter key: SHA-256(bearer) — the raw token never lives in the limiter map even though buckets persist for the 5-minute retention window. - Discovery docs (`/.well-known/oauth-*`) are NOT rate-limited. They're polled by MCP clients before any token exists; rate- limiting them per-IP would penalize office NATs and per-bearer doesn't apply (no bearer to hash). - No-bearer requests are 401'd before the limiter sees them, so a bare-bones DoS via empty Authorization headers gets the cheap rejection path without sharing a (necessarily-empty) bucket key. ## 429 response Per RFC 6585: `Retry-After: <seconds>` header (computed from the limiter's refill rate), plus `X-RateLimit-Limit`, `X-RateLimit-Remaining: 0`. Body is the MCP-shaped JSON envelope `{"error": {"code": "rate_limited", "message": "..."}}` so MCP clients (Claude Desktop, Cursor) can surface the error consistently. ## Implementation - `RateLimiters.MCPPerToken` — new `*ipRateLimiter` instance, drained in `Stop()` so cleanup goroutines don't leak (BUG-851 pattern). - `Server.checkMCPRateLimit` — called from `MCPBearerAuth` BEFORE auth validation. Returns false + writes 429 when bucket is exhausted; auth still 401s if the token is also invalid (the rate limit and validity checks are independent). - `hashTokenForLimiter` — SHA-256 hex digest helper. Uniform with the limiter's other (IP-string) keys. - `writeMCPRateLimit` — emits the 429 envelope. ## Tests - TestMCPRateLimit_PerToken_BucketEnforced — single token → 429 within 30 attempts (60/min, burst 20). - TestMCPRateLimit_PerToken_TwoTokensIndependent — drain token1 to 429, verify token2 still passes a full burst. - TestMCPRateLimit_DiscoveryDocsExempt — 50 hits to /.well-known/oauth-protected-resource, zero 429s. - TestMCPRateLimit_NoBearer_NotCounted — no-bearer requests 401 before the limiter, no 429s. - TestMCPRateLimit_429EnvelopeShape — Retry-After, X-RateLimit-* headers, MCP error envelope shape. - TestHashTokenForLimiter — hash determinism, length, no collision by prefix, empty input safety. * fix(mcp): move per-token rate limit AFTER auth validation (Codex round 1) Codex review #378 round 1 caught a memory-DoS risk: the pre-auth limiter created a new bucket entry for every distinct bearer string. An attacker rotating random bearer values would grow the limiter map unbounded until the 5-minute cleanup tick — millions of phantom entries before the goroutine catches up. Fix: relocate the checkMCPRateLimit call to AFTER auth validation in both PAT and OAuth paths. The limiter map now only fills with hashes of *valid* tokens, bounding map size by the active-token count rather than by the bearer-string space. Trade-off: invalid-bearer spam still hits the auth path's DB lookup (CPU cost, but a single indexed read per request) without any rate limiting. The CPU exposure is small enough to accept for v1; a follow-up could add a pre-auth per-IP cap for invalid-token flooding if real abuse appears. Tests: - TestMCPRateLimit_InvalidBearerNotRateLimited — 50 invalid bearers in a row, none get 429 (always 401). - TestMCPRateLimit_LimiterMapBoundedByValidTokensOnly — direct regression: 100 distinct invalid bearers, limiter map size must NOT grow. - Existing happy-path tests updated to use real PATs (via the new mustCreatePATForTest helper) so the post-auth-validation guard doesn't short-circuit them. * fix(mcp): move OAuth rate limit AFTER all validation gates (Codex round 2) Codex review #378 round 2 caught a P3 gap in round 1's fix. The OAuth path's rate-limit check ran AFTER IntrospectToken but BEFORE: - access-token-vs-refresh-token check - RFC 8707 audience match - session.GetSubject() presence - GetUser lookup So an active-but-not-authorized OAuth bearer (refresh token used as a bearer, wrong-audience token, deleted user) would create a limiter entry. After 30 such requests the response would flip from the intended 401 invalid_token to 429 — leaking limiter state to attackers and slightly defeating the bounded-map property. Fix: move the OAuth-path checkMCPRateLimit call to the very end of handleMCPOAuthAuth, just before context attachment + next.ServeHTTP. Now the limiter map only contains tokens that would have reached the dispatcher otherwise. Test: TestMCPRateLimit_OAuthRefreshTokenNotCounted — mints a real refresh token via the full OAuth flow, hammers /mcp with it 50 times, asserts every response is 401 AND the limiter map size is unchanged. * fix(mcp): move PAT rate limit AFTER all validation gates (Codex round 3) Codex review #378 round 3 caught the symmetric issue in the PAT path that round 2 fixed for OAuth. checkMCPRateLimit ran AFTER ValidateToken but BEFORE: - apiToken.UserID == "" check (legacy workspace-scoped tokens) - GetUser lookup (deleted-user case) Active-but-not-authorized PAT bearers (legacy tokens with no user_id, tokens whose user was deleted) would have created limiter entries and eventually 429'd instead of returning the intended 401 invalid_token. Fix: move the PAT-path checkMCPRateLimit call to the very end of handleMCPPATAuth, just before context attachment + next.ServeHTTP. Now mirrors the OAuth path's positioning — both run the rate limit exactly once, at the END of their happy path, so the limiter map only contains tokens that would otherwise reach the dispatcher.	2026-05-02 15:15:36 -04:00
xarmian	d01bbf6bf1	feat(oauth): live workspace allow-list + role enforcement (TASK-953) (#377) ... Closes the third leg of PLAN-943's OAuth permission model: (token capability tier) × (live workspace role) × (consent allow-list) The first two were already in place — TASK-1027 wired the tier scope check (pad:read / pad:write / pad:admin via tokenScopeAllows) and RequireWorkspaceAccess does the live role lookup. This PR adds the third gate: the workspace-allow-list set at consent time (TASK-952) actually denies workspaces NOT in the user's selection. ## What's new - `oauth.Session.AllowedWorkspaces()` / `SetAllowedWorkspaces()` — typed accessors on session.Extra. Handle BOTH the in-memory []string shape (consent-decide path) AND the JSON-decoded []interface{} shape (post-storage round-trip path). - `WithTokenAllowedWorkspaces` / `TokenAllowedWorkspacesFromContext` — context helpers in internal/server with defensive copies so callers can't corrupt the per-request token state. - `MCPBearerAuth` (OAuth path) reads the token's allow-list from session.Extra and stashes it in context. - `RequireWorkspaceAccess` checks the allow-list against the resolved workspace's slug. Three behaviours match TokenAllowedWorkspacesFromContext's return shapes: - nil → no token-level gate (PAT auth, pre-TASK-952 OAuth tokens). Standard membership applies. - ["*"] → wildcard. Every membership the user has passes. - [slug-a, slug-b, ...] → only listed slugs. Anything else gets 403 permission_denied BEFORE the membership check. ## Live role + revocation Membership revocation takes effect immediately. RequireWorkspaceAccess calls GetWorkspaceMember on every request — if the user lost membership in workspace X, the token's allow-list including X no longer helps; the request is rejected at the standard membership gate. Tested explicitly via TestWorkspaceAllowList_LiveMembershipRevocation. ## Tier × role The natural intersection of tokenScopeAllows (tier-based HTTP-method gate) and per-handler role checks (e.g. requireEditPermission) handles the tier × role table from the PLAN-943 spec: - pad:write tier passes tokenScopeAllows for POST. - But Viewer role fails requireEditPermission's role check. - Net: 403 — tested explicitly via TestWorkspaceAllowList_TierTimesRole_WriteByViewer. ## Tests Unit (no I/O): - TestTokenAllowedWorkspaceMatches — policy table for the helper. - TestWithTokenAllowedWorkspaces_DefensiveCopy + 1 reader counterpart. - TestSession_AllowedWorkspaces_*: setter/getter, nil-clear, defensive copy, JSON round-trip ([]string + []interface{} branches), wildcard JSON round-trip, not-set, nil-session. Integration (full chain, real OAuth flow): - TestWorkspaceAllowList_AllowsListedSlug — listed workspace passes. - TestWorkspaceAllowList_DeniesUnlistedSlug — unlisted gets 403 even though user is owner. - TestWorkspaceAllowList_WildcardAllowsAnyMembership — wildcard passes for every membership. - TestWorkspaceAllowList_LiveMembershipRevocation — token works, then membership revoked, then same token denied. - TestWorkspaceAllowList_PATPathUnaffected — PAT regression: PATs don't carry an allow-list, must NOT hit the gate. - TestWorkspaceAllowList_TierTimesRole_WriteByViewer — pad:write tier × Viewer role on POST item → 403.	2026-05-02 14:29:17 -04:00
xarmian	7d0de978f7	feat(oauth): consent UI with workspace allow-list + capability tier (TASK-952) (#376) ... * feat(oauth): consent UI with workspace allow-list + capability tier (TASK-952) Replace the inline-HTML stub from sub-PR C (TASK-1025) with the real consent page described in PLAN-943: server-rendered HTML with workspace multi-select, "any workspace" wildcard, and a capability- tier radio (read / write / admin). ## What the page does - Lists every workspace the user is a member of, with their role shown next to each row (informational — TASK-953 does live role resolution at MCP-call time). - Wildcard checkbox grants "any workspace I currently or later have access to," with a clear warning when checked. Mutually exclusive with per-workspace boxes (vanilla JS for UX, server-side rejection as the security gate). - Capability tier radio is constrained to the intersection of {pad:read, pad:write, pad:admin} and the client's requested scopes — fosite's grant-time subset check (RFC 6749 §3.3) rejects scopes outside the request, so the UI must never offer them. Default selects the highest tier the client requested. - Allow button stays disabled until ≥1 workspace (or wildcard) is selected. Server-side validation enforces the same rule regardless of JS state. ## Selective consent This is the central security property. The decide handler now grants *exactly* the chosen tier scope, NOT every requested scope. If the client requests `pad:read pad:write` and the user picks "read", the issued token has `scope=pad:read` only. Bonus fix: removed redundant scope re-grant loop in handleOAuthToken that would have expanded granted scopes back to the full requested set on every /token exchange — a real security bug that the auto-approve stub from sub-PR C masked because granted == requested for that flow. fosite's flow_authorize_code_token.go:134-138 + flow_refresh.go:91-103 copy GrantedScope/Audience automatically; our loop was undoing selective consent. ## Workspace allow-list storage The user's workspace selections live in `session.Extra["allowed_workspaces"]` (round-trips via storage.go's existing JSON marshal). Either `["*"]` for wildcard or a list of slugs. fosite's WriteIntrospectionResponse serializes Extra into the introspection response as top-level fields, so TASK-953's enforcement layer reads them off `/oauth/introspect` (or in-process via fosite.IntrospectToken). This sidesteps fosite's strict "granted ⊆ requested ⊆ client.Scopes" check — clients don't request `pad:workspaces:foo`, but the consent UI lets the user pick from their workspaces regardless. TASK-953 implements the live role resolution + workspace gate. ## Defense in depth - Server validates `capability_tier ∈ {read, write, admin}` AND that the chosen tier is among the client's requested scopes — fosite would reject otherwise with a less-readable error. - Server validates every non-wildcard slug is in the user's current membership table. A tampered form sending other slugs gets 400. - Wildcard wins: if a tampered POST sends both `*` and specific slugs, the result is `["*"]` only — never partial allow-list. ## Tests - TestConsent_RendersUserWorkspaces — multi-workspace list with role labels. - TestConsent_NoWorkspaces_ShowsEmptyState — clean empty state. - TestConsent_TierRadios_OnlyRequestedScopes — UI hides tiers the client didn't request. - TestConsent_ApproveWithSpecificWorkspaces — happy path, asserts introspection returns `allowed_workspaces=[alpha, beta]`. - TestConsent_ApproveWithWildcard — wildcard yields `["*"]`. - TestConsent_ApproveWithoutWorkspaceSelection_Rejected — 400 on empty allow-list. - TestConsent_ApproveWithUntrustedSlug_Rejected — defense in depth. - TestConsent_ApproveWithUnrequestedTier_Rejected — server tier validation matches UI's tier-radio constraint. - TestConsent_TokenScopeMatchesTierChoice_Read — selective consent: user picks read-only despite client requesting both, token has exactly `pad:read`. Existing tests + helpers updated to include the new consent fields (`capability_tier`, `allowed_workspaces`). * fix(oauth): prevent URL parameter pollution attack on consent UI (round 1) Codex review #376 round 1 caught a P1 security bug in the consent UI. The hidden-input round-trip used the full r.URL.Query() with only `csrf_token` stripped, so a malicious OAuth client could craft /oauth/authorize?...&capability_tier=admin&allowed_workspaces=* and the consent form would render those as hidden inputs BEFORE the user-controlled radios + checkboxes. On submit, the hidden values precede the user's selection in the form encoding, so: - r.FormValue("capability_tier") returns "admin" (first value matches the attacker's, not the user's) - r.PostForm["allowed_workspaces"] sees "*" first, the wildcard scan matches, the result is ["*"] regardless of which boxes the user actually checked Net effect: a user clicking through the consent UI for "read-only, just my docapp workspace" would silently authorize "admin, all workspaces" — without any visible cue that the values were wrong. Fix: build hidden inputs from an explicit allowlist of OAuth-standard authorize-request parameters (response_type, client_id, redirect_uri, scope, state, audience, resource, code_challenge, code_challenge_method, nonce). Anything outside the allowlist is silently dropped. This is strictly stronger than blocklisting consent-control names, because it also defends against future OAuth extensions adding new attacker- controllable params we haven't enumerated. Test: TestConsent_URLPollution_DoesNotOverrideUserSelection simulates the attack — GET /authorize with attacker params, asserts the rendered HTML contains zero `<input type="hidden" name="<attacker_name>">`, then completes the flow with the user's actual selection and confirms the issued token's scope matches the user's choice (pad:read), not the attacker's URL injection (pad:admin).	2026-05-02 14:08:11 -04:00
xarmian	924d82dae4	feat(oauth): MCPBearerAuth OAuth integration + public-info (TASK-1027) — closes TASK-951 (#375) ... * feat(oauth): MCPBearerAuth OAuth integration + public-info endpoint (TASK-1027, sub-PR E of TASK-951) Closes the OAuth server build-out by connecting sub-PRs A-D to the MCP transport from TASK-950 and shipping the consent-screen support endpoint. ## MCPBearerAuth OAuth path middleware_mcp_auth.go now branches on token shape: - pad_<60-hex> → existing PAT validation (TASK-950 path) - anything else → fosite.IntrospectToken via the new internal/oauth.Server.IntrospectToken wrapper (server-side, no HTTP roundtrip — pad-cloud is both auth server and resource server, so the public /oauth/introspect endpoint is for external clients only). OAuth path validation gates: - Token must be active (fosite returns ErrInactiveToken / ErrNotFound on revoked / unknown / expired tokens). - tokenUse must be access_token; refresh tokens explicitly rejected (RFC 6749 §1.5 — refresh tokens aren't bearers for resource calls). - Granted audience MUST contain the canonical MCP URL (RFC 8707 anti-replay; resource-server-side check defends against compromised or shared auth servers). - Subject must resolve to a real user row. Successful path stashes user + scopes via WithCurrentUser / WithTokenScopes. Scopes are translated from fosite's space-separated form to JSON-array form via oauthScopesToJSON. ## tokenScopeAllows pad:* extension Extended to recognize the OAuth scope vocabulary alongside PAT scopes: - pad:read ↔ read (GET/HEAD/OPTIONS only) - pad:write ↔ write (all methods) - pad:admin ↔ * (all methods) So MCP tool authorization stays uniform regardless of which transport issued the bearer. ## /api/v1/oauth/clients/{id}/public-info New read-only endpoint for the consent screen (TASK-952) and the OAuth-intent banner (TASK-1001, already shipped). Returns four non-sensitive fields: client_id, client_name, logo_uri, redirect_uris. - Auth-required (any logged-in user). - Cloud-mode-gated (404s outside cloud). - 404 for unknown clients. - Whitelisted leak surface — explicit fields, no embedded models.OAuthClient, so a future field addition (e.g. a confidential- client secret) doesn't accidentally appear here. ## Tests - TestMCP_OAuthAccessToken_Authenticates — happy path: full flow yields a token that authenticates against /mcp. - TestMCP_OAuthAccessToken_AudienceMismatch_Rejected — RFC 8707 resource-server check; mints a token, swaps the OAuth server for one with a different canonical, confirms 401. - TestMCP_OAuthRefreshToken_RejectedAtMCP — refresh tokens MUST NOT authenticate. - TestMCP_RevokedOAuthToken_Rejected — revocation takes effect at the resource server. - TestMCP_PATPath_StillWorks — regression for sub-PR D's coexistence with the OAuth path. - TestMCP_OAuthScopeReadOnly_StashesPadReadScope — scope round-trip. - TestOAuthClientPublicInfo_HappyPath / UnknownClient_404 / Unauthenticated_401 / NotMountedOutsideCloudMode — full coverage of the new endpoint. - TestE2E_ClaudeDesktopFlow — simulates the full sequence (discovery → DCR → authorize → token → /mcp call) Claude Desktop walks on first connect. - TestTokenScopeAllows extended with pad:* coverage. ## TASK-951 status Closes TASK-951 when this lands (5/5 sub-PRs done): - A: schema + storage layer (#370 / 2a00775) - B: fosite-backed authorization-server constructor (#371 / f6eeee4) - C: DCR + authorize + token endpoints + populated discovery (#372 / 48776a3) - D: revoke + introspect endpoints (#373 / 4250fb1) - E: MCPBearerAuth + public-info (this PR) * fix(oauth): fail-closed on empty OAuth scopes per Codex review (round 1) Codex caught a high-severity bug in oauthScopesToJSON: the helper mapped empty granted scopes to `[]`, which tokenScopeAllows interprets as the legacy "unrestricted" PAT shape (allow all methods). Combined with OAuth's RFC 6749 §3.3 rule that the `scope` parameter is OPTIONAL, this meant a client could: 1. Run the auth-code flow without requesting scopes. 2. Get back a token with empty granted_scopes. 3. Drive write MCP tools because MCPBearerAuth stashed `[]` and tokenScopeAllows fell through to the legacy unrestricted path. Fix: map empty OAuth scopes to JSON `null` instead. tokenScopeAllows denies on the "scopes == nil" branch (existing TASK-667 behavior), so the entire write surface is denied for empty-scope OAuth tokens. In production this path is hard to hit — sub-PR C's DCR handler defaults registered clients to `pad:read pad:write` when omitted, and audienceMatchingStrategy enforces canonical-audience matching at grant time. Defense-in-depth at the resource server is the right policy regardless. Test: TestOAuthScopesToJSON_FailClosedOnEmpty asserts both halves of the contract — the helper produces "null" for empty input, and tokenScopeAllows denies every method when fed that value.	2026-05-02 13:33:53 -04:00
xarmian	4250fb1976	feat(oauth): revoke + introspect endpoints (TASK-1026) (#373) ... * feat(oauth): revoke + introspect endpoints (TASK-1026, sub-PR D of TASK-951) Add the RFC 7009 revocation and RFC 7662 introspection endpoints, completing the spec'd surface that sub-PR C left as placeholders. - POST /oauth/revoke — fosite NewRevocationRequest delegates to our storage adapter's RevokeRefreshToken / RevokeAccessToken which walk the request_id (grant family) and mark every chain member inactive in one statement. Public clients authenticate by sending only client_id (token_endpoint_auth_method=none). - POST /oauth/introspect — fosite NewIntrospectionRequest with Bearer auth (a separate active access token). Returns {active:true, sub, scope, aud, client_id, exp, iat} for active tokens; bare {active:false} for unknown/revoked/expired (RFC 7662 §2.2 no-leak rule). Sub-PR E's MCPBearerAuth integration uses fosite.IntrospectToken directly server-side, but the public endpoint satisfies the discovery contract for clients that follow the chain. - Discovery doc populates revocation_endpoint + introspection_endpoint and their auth_methods_supported lists ("none" for both — public-clients-only model). Tests cover: - /revoke marks an access token inactive (verified via introspect). - /revoke on a refresh token revokes the entire grant family (paired access also goes inactive). - Refresh-token rotation: old pair becomes inactive, new pair active. - Refresh-token replay detection: replaying a rotated refresh kills the family (OAuth 2.1 §6.1, RFC 6819 §5.2.2.3). - Introspect happy path returns sub/scope/aud/client_id/exp. - Introspect on unknown token returns just {active:false} with no field leakage. - Introspect rejects requests with no Bearer Authorization header. - /revoke + /introspect 404 outside cloud mode. - Discovery doc advertises both endpoints + auth-methods lists. * fix(oauth): drop introspection_endpoint_auth_methods_supported per Codex review (round 1) Codex review of #373 caught a contradiction in the discovery doc: introspection_endpoint_auth_methods_supported: ["none"] advertised "no client authentication" for the introspection endpoint, but fosite's NewIntrospectionRequest rejects a request without Authorization: Bearer ... (and the test in this PR locks that in). A discovery-driven client would treat "none" as "post token+client_id unauthenticated" and get 401 — worse than no advertisement at all. Fix: omit introspection_endpoint_auth_methods_supported entirely. RFC 8414 §2 marks the field OPTIONAL; omission tells clients to negotiate auth out-of-band, which for our public-clients-only model means "send a separate active access token in the Authorization header." We document that in getpad.dev/mcp/local. revocation_endpoint_auth_methods_supported = ["none"] is kept and honest — fosite's NewRevocationRequest really does accept a public client posting only client_id (no Bearer required). * fix(oauth): RFC 7009 §2.2 idempotent revoke per Codex review (round 2) Codex caught that fosite v0.49 returns ErrInvalidRequest for the unknown-token path of NewRevocationRequest, which WriteRevocationResponse turns into 400. RFC 7009 §2.2 explicitly requires: "The authorization server responds with HTTP status code 200 if the token has been revoked successfully or if the client submitted an invalid token." The 400 break the entirely normal "client retried after a previous revoke succeeded" or "operator typo'd the token" cases. Fix: detect the bare ErrInvalidRequest from the !found branch via isRevocationUnknownToken (which inspects HintField — fosite sets the hint on every other ErrInvalidRequest path it returns from NewRevocationRequest) and write 200 directly. Genuine malformed requests (wrong method, unparseable body, empty form) still return 400 because their ErrInvalidRequest carries a hint. Tests: - TestOAuth_Revoke_UnknownToken_Returns200 — locks in 200 for the unknown-token path. - TestOAuth_Revoke_MalformedRequest_Returns400 — counterpart that ensures the 200 override doesn't accidentally swallow real malformed-request errors. * fix(oauth): require token param + remove dead-code revoke override (round 3) Codex round 3 noticed that POST /oauth/revoke with client_id but no token returned 200 OK — silently swallowing a missing-required- parameter error. RFC 7009 §2.1 marks `token` REQUIRED. Investigating the fix surfaced that round 2's isRevocationUnknownToken override was actually dead code: fosite v0.49's handler/oauth2/revocation.go's RevokeToken collapses ErrNotFound + ErrInactiveToken to nil via storeErrorsToRevocationError, so NewRevocationRequest returns nil and WriteRevocationResponse writes 200 natively for unknown tokens. The override never fired in any real path. Cleanup: - Replace the unused isRevocationUnknownToken + override with a pre-check that returns 400 invalid_request when `token` is missing. RFC 7009 §2.1 enforced; fosite's native idempotency handles unknown tokens. - Update TestOAuth_Revoke_UnknownToken_Returns200's comment to reflect that it pins fosite's native behavior (not our override). - Add TestOAuth_Revoke_MissingToken_Returns400 to lock in the pre-check. - Keep TestOAuth_Revoke_MalformedRequest_Returns400 — verifies fosite's own ErrInvalidRequest paths still surface as 400.	2026-05-02 12:47:54 -04:00