mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-24 19:32:10 +00:00
714da48442d68e29c1a730be39b43601bf69964a
159 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
714da48442 |
fix(store): handle nullable collections.settings end-to-end (BUG-1482) (#561)
* fix(store): make ListCollectionsMinimal Postgres-safe (BUG-1482)
`COALESCE(settings, '')` failed at planner time on Postgres because
`collections.settings` is JSONB and `''` is not valid JSON
(SQLSTATE 22P02). The query failed regardless of row contents; SQLite
is type-loose and accepted it, leaving the bug latent in the two
production callers (`handlers_dashboard.go`, `handlers_items.go`).
Switch the query to a plain `SELECT ... settings ...` and scan into
`sql.NullString`, materializing NULL as the empty-string sentinel.
This preserves the existing contract that downstream consumers
(`buildDoneContextMap`, `ListCollections`'s own scan loop) gate on
via `if c.Settings != ""`, so no caller-side changes are needed.
Adds two regression tests in `collections_test.go` that exercise the
NULL-settings case (the planner-time failure mode) and the happy-path
JSON round-trip. Both run against SQLite and Postgres via the existing
PAD_TEST_POSTGRES_URL switch in `testStore`.
* test(store): tighten ListCollectionsMinimal happy-path assertion (BUG-1482)
Codex review round 1 flagged TestListCollectionsMinimalReturnsSettingsJSON
as too permissive: `Settings != ""` would pass for `{}` or any wrong JSON
payload. Postgres JSONB also normalizes formatting/key order, so a string
compare against the input literal would be brittle across drivers.
Switch to a semantic compare: unmarshal both sides into map[string]any
and reflect.DeepEqual. This actually verifies the JSON round-trips
through the (now fixed) ListCollectionsMinimal path on both drivers.
* fix(store): NULL-safe settings scan in GetCollection / ListCollections / ExportWorkspace (BUG-1482)
Round-2 extension of the same fix shape. Direct `Scan(... &c.Settings ...)`
into a Go string fails on Postgres for any row holding a real NULL with
"Scan error: converting NULL to string is unsupported". The column is
nullable on both drivers (TEXT DEFAULT '{}' / JSONB DEFAULT '{}'), so
legacy or manually-poisoned rows can 500 every handler that goes through
these readers — `GetCollection` is the hot reader on every item handler,
`ListCollections` powers dashboard + sidebar, `ExportWorkspace` crashes
the export pipeline before any data is emitted.
Same fix as ListCollectionsMinimal: scan into sql.NullString, materialize
NULL as "" to preserve the existing sentinel contract that downstream
consumers gate on via `if c.Settings != ""` (handlers_dashboard.go:247,
handlers_items.go:1626, collections.go:196 in ListCollections's own
post-scan loop). Audited; no caller depends on a non-empty default.
Adds TestGetCollectionHandlesNullSettings, TestListCollectionsHandlesNullSettings,
and TestExportWorkspaceHandlesNullSettings — each forces a NULL via direct
UPDATE (bypassing CreateCollection's empty→`{}` coercion) and asserts the
function returns without error and surfaces "" downstream. All pass on
SQLite and Postgres.
* fix(store): coerce empty-string settings to {} on workspace import (BUG-1482)
The earlier commits in this PR made ExportWorkspace, GetCollection, and
ListCollections all return `""` for a NULL `collections.settings` row,
preserving the in-process sentinel contract that downstream consumers
(buildDoneContextMap and friends) already gate on via `c.Settings != ""`.
That fix surfaced a paired contract gap: ImportWorkspace previously
inserted `c.Settings` verbatim into the collections table. After the
reader fixes, an exported NULL-settings row materializes as `""` in the
bundle, which Postgres's JSONB column rejects at INSERT time. Without
this commit, exporting a workspace with any NULL-settings row and
re-importing it would have crashed on Postgres — turning one half of a
symmetric contract green while leaving the other half broken.
Mirror the same coercion CreateCollection applies on the normal create
path: when the bundle's settings field is the empty-string sentinel,
write `"{}"` instead. Add a round-trip regression test
(TestExportImportRoundTripWithNullSettings) that NULL-poisons a workspace's
settings, exports, re-imports, and asserts the re-imported collections
hold valid JSON. Verified on both drivers.
* style(store): rewrite doc comment to avoid gofmt apostrophe-pair rewrite
Go 1.19+ gofmt's doc-comment formatter collapses `''` (two ASCII
apostrophes) inside backtick code spans into a single `”` (U+201D right
double quotation mark) — a typographic-pair heuristic that doesn't quite
fit when the literal pair is the load-bearing thing being described
(here: SQL's empty-string literal in COALESCE).
CI's golangci-lint flagged the file as gofmt-dirty for this reason.
Rewrite the prose to describe the bug without using `''` literally:
"coalesced settings against an empty SQL string literal" reads more
clearly than `COALESCE(settings, '')` becoming `COALESCE(settings, ”)`
after gofmt normalization. Functionally identical comment; lint-clean.
|
||
|
xarmian
|
7c663a3d3f |
feat(collections): add blank workspace template + retire auto-upgrade hook (IDEA-1479) (#560)
* feat(collections): add blank workspace template (IDEA-1479)
Introduces a `blank` workspace template that seeds only the two system
collections (Conventions, Playbooks) — no Tasks/Ideas/Plans/Docs, no
seeded items, no starter conventions or playbooks. Solves the
agent-self / non-template-fit use case where the existing software
templates leave undeletable ghost collections in the workspace.
Adds a new `CategoryCustom` ("Custom") top-level category so the blank
template doesn't mis-group with `startup` / `scrum` / `product`.
Category is appended last in `CategoryOrder` so it doesn't displace
recommended-path templates in the picker.
Tests:
- TestBlankTemplateShape — exactly 2 system collections, no seeds.
- TestBlankTemplateExcludesSoftwareCollections — no tasks/ideas/plans/docs.
- TestBlankTemplateAppearsInPicker — surfaces under a Custom group.
- TestSeedFromBlankTemplate — bootstrapping produces 2 collections, 0 items.
* fix: address codex review for blank template (IDEA-1479)
- CreateWorkspaceModal: remove hard-coded 'blank' picker entry that
silently fell through to collections.Defaults(). The API-driven blank
template (under the Custom category) is now the canonical surface.
- Dashboard: gate '+ New Task' button on tasks collection existence so
blank workspaces don't render a button that targets a missing
collection.
- OnboardingChecklist: accept collectionSlugs prop and filter steps
whose target collection (plans/tasks/docs) is absent. Conventions
step remains unconditional since the conventions collection ships
with every template, including blank. Empty-steps guard added to
progressPct to avoid NaN.
- web/src/lib/utils/templates.ts: add 'custom' -> 'Custom' to mirror
the Go CategoryOrder + categoryLabels updates.
- cmd/pad/templates_picker_test.go: extend the visible-template
assertion list to include 'blank' and assert the Custom category
header renders.
* fix(store): gate SeedDefaultCollections on zero-collection workspaces (IDEA-1479)
The server's startup auto-upgrade hook (cmd/pad/main.go) called
SeedDefaultCollections against every workspace at boot. That hook
dates to the initial release — long before workspace templates
existed — and was written as a backfill for workspaces created
before tasks/ideas/plans/docs landed in Defaults().
Post-templates, the hook unconditionally re-materialized the
Software-template collections into any workspace missing them —
including blank-template workspaces (IDEA-1479), which ship only
Conventions + Playbooks by design. Result: every restart silently
regrew the ghost user-facing collections the blank template was
explicitly built to avoid.
Fix: SeedDefaultCollections now returns nil immediately when the
workspace has any existing collection (system or user-facing). The
rescue path still triggers for genuinely-empty workspaces, preserving
the original backfill intent.
Tests:
- TestBlankWorkspaceSurvivesSeedDefaultCollections — blank workspace
remains 2 collections after auto-upgrade (and after a second pass).
- TestEmptyWorkspaceStillGetsDefaults — zero-collection workspace
still gets the full Software default set.
* refactor(server): remove SeedDefaultCollections auto-upgrade at startup (IDEA-1479)
The startup auto-upgrade hook in cmd/pad/main.go dated to the initial
release, predating workspace templates entirely. Its original intent
was per-collection backfill — workspaces created before a new entry
landed in Defaults() would acquire it on next boot. Post-templates,
that semantic is incompatible with templates that legitimately
diverge from Defaults() (e.g. `blank`, which ships only Conventions
+ Playbooks by design).
Round-2 of the IDEA-1479 review attempted to keep the hook by adding
a "zero collections" guard, but Dave (after codex round 3) decided
the cleanest fix is removing the hook entirely. The codebase has
proper migration infrastructure now; any future "add a default
collection" work should land as an explicit migration where the
author chooses which workspaces to backfill.
SeedDefaultCollections itself is preserved (with the round-2 guard)
as a building block for any future explicit rescue command or
migration. Its doc comment is updated to note it's no longer
auto-invoked at startup. The round-2 regression tests
(TestBlankWorkspaceSurvivesSeedDefaultCollections,
TestEmptyWorkspaceStillGetsDefaults) still apply and pass unchanged.
* fix(store): rescue gate uses COUNT(*), not ListCollectionsMinimal (IDEA-1479)
Postgres CI on PR #560 caught a regression introduced in commit
|
||
|
xarmian
|
be68292e03 |
fix(store): workspace list freshness reflects item activity (BUG-1481) (#559)
`pad workspace list` was showing `workspaces.updated_at`, which only moves on workspace-row mutations (rename, settings, members). After this fix the effective UpdatedAt surfaces item activity inside the workspace — answering "where is work happening?" instead of "when was this row last UPDATEd?". Implementation: scalar `MAX(items.updated_at)` subquery in `ListWorkspaces` and `GetUserWorkspaces`, then `effectiveWorkspaceUpdatedAt` picks the later of the two timestamps (portable across SQLite + Postgres, no GREATEST). Read-time approach per the bug's design notes. Visibility-aware: codex review surfaced that a naive MAX leaks activity timing for items the caller can't see. The member subquery mirrors `VisibleCollectionIDs` (`collection_access='all'` short-circuits; for `'specific'` members, system collections + `member_collection_access` + `collection_grants` + `item_grants` gate visibility). The guest subquery limits MAX to items reachable via `collection_grants` / `item_grants`. All grant lookups are workspace-scoped for defense-in-depth. Four regression tests cover: admin `ListWorkspaces`, member all-access, member specific-access leak guard, and guest leak guard. |
||
|
xarmian
|
38aa872864 |
fix(fields,activity): debounce typed-input field saves + collapse same-field activity runs (BUG-1466) (#549)
* fix(web): debounce typed-input field saves to stop per-keystroke activity rows (BUG-1466)
Text / number / URL fields in FieldEditor wired oninput directly to
onchange, so every keystroke became an item PATCH and an activity row.
Typing `ui/editor/tiptap` into a `component` field produced a 30-step
keystroke chain in the audit metadata (visible on BUG-1419's timeline).
Wrap the typed-input branches in a 500ms idle debounce, flush on blur
so tabbing away commits immediately, and flush on unmount so navigation
never drops a pending value. Discrete inputs (select / date / checkbox,
number ±1 buttons) keep firing on the user action — they aren't typing.
Mirrors the markdown content debounce pattern in the detail page.
* fix(activity): collapse same-field runs in merged changes metadata + unify diff separator (BUG-1466)
Follow-up to the web-side typing debounce. Two related changes:
1) collapseChanges() walks the merged "; "-delimited changes string and
collapses runs of consecutive same-field entries into a single
"field: first-old → last-new". Drops net no-ops (typed then backspaced).
When the web-side debounce in FieldEditor still produces multiple
PATCHes within the 5-minute coalesce window — or for older rows that
pre-date the debounce — the timeline now reads as one transition
instead of a chain. Run-based (not global) collapse so interleaved
edits on different fields keep their chronology.
2) diffFields now joins entries with "; " instead of ", " so the joiner
is consistent with mergeActivityMeta and TimelineActivityCard.svelte's
split delimiter. Multi-field PATCHes previously rendered as a single
unparseable blob in the web timeline because the parser only split on
";" — fixed as a side-effect.
Adds TestCollapseChanges (10 cases including the BUG-1419 repro) and
TestMergeActivityMeta_CollapsesSameFieldRun. Updates the existing
TestDiffFieldsPrimitives expectation to match the new joiner.
* fix(fields,activity): two follow-ups per Codex review (round 1)
[P1] FieldEditor.svelte::handleNumberStep
The ±1 buttons computed `(Number(value) || 0) + delta` AFTER calling
flushPendingSave(). But `value` is the parent prop — flushing fires
onchange asynchronously, so at the moment of the step computation
the prop still holds the pre-typed value. Typing 10 over 5 and
clicking + would flush 10 then send 6, overwriting the typed value.
Compute `base` from `pendingValue` (if hasPending) BEFORE clearing
the timer state, then send `base + delta` in one onchange call.
[P2] activities.go::collapseChanges
The drop-net-no-op step removed entries where `from == to`. But
diffFields intentionally emits same-display entries for
same-cardinality structured-field replacements like
`implementation_notes: (1 note) → (1 note)` (see
TestDiffFieldsSameCardinalityArrayChangeStillReported) — the labels
match because formatChangeValue summarizes by count, not content,
but the underlying data did change. Dropping them silently hides
real updates from the activity feed.
Track `mergedCount` per entry: increment when collapsing a run,
initialize to 1 on parse. Only drop when `mergedCount > 1 && from == to`
— i.e. only when the no-op resulted from collapsing multiple input
segments (the typed-then-backspaced case).
Tests: 2 new TestCollapseChanges cases (single structured-field
preserved, interleaved structured-field around a typed run).
* fix(fields,activity): two follow-ups per Codex review (round 2)
[P1] FieldEditor.svelte number stepper focus race
The number ±1 buttons race with the input's onblur handler: blur
fires before click in the natural focus-transfer flow, so
flushPendingSave clears hasPending → handleNumberStep reads stale
`value` from the parent prop → typing 25 over 10 and clicking +
sends 25 then 11, losing the typed value.
Add onmousedown={preventDefault} on both ±1 buttons. Mousedown
precedes blur, and preventDefault on mousedown suppresses the
natural focus transfer — the input keeps focus through the click,
so hasPending survives until handleNumberStep reads it.
[P2] collapseChanges still drops repeated same-display structured runs
Round 1's mergedCount>1 rule still dropped runs like
`implementation_notes: (1 note) → (1 note); implementation_notes: (1 note) → (1 note)`
— two real updates whose display strings happen to match because
formatChangeValue summarises array-valued fields by count. Each
PATCH represented a different underlying note (diffFields uses
reflect.DeepEqual to detect that), but the merged display showed
no transition.
Track `hadTransition` per entry: true iff the run had a display-
level transition (initial from != to, or a subsequent entry's `to`
differed from the anchored from). Drop only when
mergedCount > 1 && from == to && hadTransition — i.e. only true
net-cancellations (typed-then-backspaced). Same-display structured
repeats stay; real `foo → bar → foo` swings still drop.
Tests: 2 new TestCollapseChanges cases (repeated same-display preserved,
real foo→bar→foo swing still dropped).
* fix(fields,activity): two follow-ups per Codex review (round 3)
[P1] FieldEditor cross-item leak via debounce timer
When the parent reuses a FieldEditor instance across an item swap
(same schema, same field.key, different item — common when
navigating between items in the same collection), the parent's
`updateField` closure reads `item.id` at CALL time. A pending
timer set while item A was active would fire after item B mounted,
patching B with A's typed value.
Two-pronged fix in FieldEditor:
- Add a $effect that tracks the `value` prop and drops any pending
save the moment the parent re-props us. The user actively typed
for a now-stale context; aborting is safer than silently writing
to the new context. Also covers external collab/SSE rebases of
the same field on the same item.
- Switch the unmount cleanup from flush → drop. When the parent
navigates to an item whose schema lacks this field, the
FieldEditor unmounts AFTER the parent's `item` has already been
replaced, so a final onchange call would route through
updateField → wrong item. Blur is the supported commit gesture
(clicking elsewhere within the page, ±1 buttons, tab-out — all
flush eagerly); unmount-without-blur is treated as "user
abandoned the edit."
[P2] collapseChanges still dropped structured count-return swings
Round 2's hadTransition rule still dropped a run like
`implementation_notes: (1 note) → (2 notes); implementation_notes: (2 notes) → (1 note)`
— the user added a note then removed the original, ending with a
different single note. The merged display reads `(1 note) → (1 note)`
with hadTransition=true, indistinguishable from a typed-then-
backspaced cancellation. But formatChangeValue summaries are
LOSSY: same display label can wrap entirely different raw values.
Add a `hasLossySummary` flag per entry — true iff either `from`
or `to` matches the `(text)` format formatChangeValue emits for
structured fields. Propagate stickily across the run (any lossy
entry pins the whole run). Skip the drop step when the run
carries lossy summaries — we can't recover the raw delta from the
merged string, so we preserve the entry rather than risk hiding
a real change.
Tests: 2 new TestCollapseChanges cases (count-return swing
preserved, lossy-on-one-side still pins the run).
* fix(fields): demote hasPending from \$state to plain let — \$effect was cancelling every keystroke (round 4 [P1])
Critical regression in round 3: the value-track \$effect read
\`hasPending\` inside its body, which Svelte 5 promotes to a reactive
dependency. scheduleSave() setting \`hasPending = true\` retriggered
the same \$effect, whose body cleared the typing timer + pendingValue
+ hasPending before the debounce could fire. Net effect: typing into
any text / number / URL field was silently dropped — onchange never
ran, the field never saved.
hasPending is only read from imperative handlers (scheduleSave,
flushPendingSave, handleNumberStep, the value-track \$effect, the
unmount cleanup) — never from a template or other reactive context.
Demoting it to a plain \`let\` removes the unwanted subscription while
preserving the round-3 behaviour: external value-prop changes still
trigger the \$effect (it tracks \`value\`), the body reads hasPending
imperatively to decide whether to clear pending state.
No tests added — this is a Svelte reactivity edge case that can't
be unit-tested without a DOM. svelte-autofixer's pre-existing
"variable assigned inside \$effect" suggestion previously flagged the
hasPending mutation; that signal is gone now.
Per Codex review round 4.
|
||
|
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.
|
||
|
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. |
||
|
xarmian
|
979537e4bb |
feat(web): localIndex in-RAM canonical store (TASK-1355) (#495)
* feat(web): localIndex in-RAM canonical store (TASK-1355) New Svelte 5 module at web/src/lib/stores/localIndex.svelte.ts that owns the in-RAM truth for the local-first read model. Per DOC-1342 decision #4: the store is canonical; IndexedDB persistence (next task) is hydration + write-behind only. Per-workspace state in a Map keyed by workspace slug: - items: SvelteMap<itemId, ItemIndexRow> — keyed by item.id - cursor: monotonic seq cursor as opaque decimal string - bootstrapState: 'cold' | 'loading' | 'ready' | 'error' Public API: - bootstrap(ws): idempotent /items-index hydration (in-flight coalescing) - getByCollection(ws, collSlug): synchronous filtered read - applyDelta(ws, changes, cursor): batch upsert/remove + cursor advance - upsert(ws, row): single-item write for SSE/optimistic paths - remove(ws, id): single-item delete for SSE archive + 403 purge - cursorFor(ws) / bootstrapStateFor(ws): reactive getters - reset(ws): drop all state for a workspace Defensively strips Item.content on every ingest so a caller passing a full Item (e.g. from api.items.update) cannot leak the rich body into the local index — matches the destructure-by-rest pattern in api.items.listIndex / changes. Parent: PLAN-1343. * fix(web): localIndex reactivity + stale-batch guards per Codex review (round 1) - [P1] WorkspaceState is now a class with `$state` class fields for `cursor` and `bootstrapState`. Svelte 5 only permits `$state()` at variable-initializer / class-field / constructor-first-assign sites — the previous `state = $state({...})` inside `ensureState` silently produced a non-reactive object on first hydration, so `bootstrapStateFor` getters could stay 'cold' through 'loading' / 'ready' transitions. Class-field runes give us the same shape with reactivity intact. - [P1] Documented that the store intentionally holds both live and archived rows. `applyDelta` only removes on the soft-delete `deleted: true` tombstone — status='archived' rows stay so a later `showArchived` toggle on the consumer doesn't need a refetch. Intended consumer pattern (TASK-1357) filters on `fields.status` at render time. - [P2] `applyDelta` now drops the whole batch when `newCursor` does not strictly advance, AND skips individual rows whose `seq` is not greater than the cursor at the start of the call. In normal /items-changes flow the server filters to `seq > since`, but the per-row guard prevents test or future replay callers from overwriting newer state with older rows. Parent: PLAN-1343. * fix(web): localIndex archive filter + per-row seq guard (Codex round 2) - [P1] `Item.deleted_at` was missing from the TS interface even though the server populates it (`Item.DeletedAt *time.Time` with omitempty). Added the field to `Item`, which flows into `ItemIndexRow` via the existing `Omit<Item, 'content'>` mapping. - [P1] `getByCollection(ws, collSlug)` now filters soft-deleted rows out by default. The store still holds them (so a `showArchived` toggle doesn't need a refetch) but the default view is live-only, matching every other collection consumer in the codebase. Callers that want archived rows pass `{ includeArchived: true }`. Updated the module-level docstring: archived = `deleted_at` set (not `fields.status`), and clarified the upsert-vs-delete split on the change wire format (`deleted: true` = hard tombstone; soft deletes arrive as upserts with `deleted_at` populated). - [P2] `applyDelta` per-row check now compares against BOTH the cursor floor at start AND the existing row's `seq`. Without the second check, a delta that legitimately advances the cursor could still carry a row whose `seq` is older than what we already hold for that id (since `upsert` / SSE paths can store newer rows without touching the cursor). Parent: PLAN-1343. * fix(web): preserve soft-deleted rows + upsert seq guard (Codex round 3) - [P1] /items-changes sets `deleted: true` for soft-deleted rows (the server's derived view of `deleted_at != nil`), not for hard tombstones. The previous applyDelta removed those rows, defeating the store's stated invariant that archived items remain queryable via `getByCollection(..., { includeArchived: true })`. Now applyDelta always upserts on a change — the soft-deleted row keeps its skinny payload (with `deleted_at`) and falls out of the default filter but stays in the index. Hard deletes still flow through `remove()`. - [P2] `upsert` now mirrors `applyDelta`'s per-row seq guard: skip the write if the incoming row's `seq` is not strictly greater than the existing row's `seq`. Without this, a late SSE / out-of-order optimistic response could regress a row after a fresher version had already landed. Parent: PLAN-1343. * fix: items-index returns deleted_at + bootstrap merges instead of clears (round 4) - [P1] `/items-index` projection now selects `i.deleted_at` and `scanItemsIndex` populates `Item.DeletedAt`. Without this the local-first client could not distinguish archived rows from live ones, so `localIndex.getByCollection`'s default live-only filter would surface archived rows as live. Mirrors the projection of ListItemsChangesSince which has always carried this column. - [P2] `localIndex.bootstrap` now merges into the existing state using the same per-row `seq` guard as `upsert`/`applyDelta`, and the cursor only advances forward. The previous clear-and-replace could regress rows that an in-flight `upsert()` or SSE-driven write had landed during the bootstrap request, and could reset the cursor below an SSE delta that advanced it concurrently. Explicit "drop everything" still flows through `reset()`. Parent: PLAN-1343. * fix(web): localIndex.getByCollection sorts updated_at DESC, id ASC (round 5) [P2] SvelteMap iterates in insertion order; live upserts and applyDelta writes appended rows / kept stale positions, so consumers reading from getByCollection saw an order that drifted away from the server's /items-index documented `updated_at DESC, id ASC`. Sort on read so the collection page sees a stable, server-aligned order regardless of how recently a row arrived through the in-RAM index. The cost is O(n log n) per read; the consumer side is expected to memoize via $derived. Parent: PLAN-1343. |
||
|
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. |
||
|
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.
|
||
|
xarmian
|
7456b5aed6 |
feat(store): add workspace-scoped monotonic seq column to items (TASK-1352) (#492)
* feat(store): add workspace-scoped monotonic seq column to items (TASK-1352) Adds an `items.seq` column that bumps on every mutation (create/update/soft-delete/restore) as the cursor mechanic for the local-first read model's delta sync (PLAN-1343, DOC-1342 design decision #1). Each mutation stamps `MAX(seq) + 1 WHERE workspace_id = ?` inside the same transaction that performs the write, with a Postgres advisory lock keyed on the workspace serializing concurrent seq-bumping mutations. SQLite's single-writer rule covers the same guarantee there. Migration backfills existing rows with sequential per-workspace seqs in (updated_at, id) order so every workspace has a non-zero MAX(seq) floor immediately. Adds an idx_items_workspace_seq index supporting both the `/items-index` cursor read and the future `/items-changes` range scan. The Seq field is now populated through every items SELECT helper (GetItem, GetItemIncludeDeleted, ListItems, ListItemsIndex, listItemsFTS, SearchItems, ItemsModifiedSince, GetChildItems, ListStarredItems, ResolveItemIncludeDeleted, GetItemBySlugIncludeDeleted) and the workspace import path stamps it via the same MAX+1 subquery so imported rows don't all collapse to seq=0. Parent: PLAN-1343. Foundation for TASK-1353 (wire seq into /items-index cursor) and TASK-1354 (/items-changes delta endpoint). * fix(store): bump items.seq on role reorder, MoveItem, and field migrations per Codex review (round 1) Codex round 1 flagged that UpdateRoleSortOrder was rewriting items.role_sort_order without bumping the new workspace-scoped seq column — delta-sync clients would miss role-board reorders until a full refresh. The same gap applied to MoveItem (collection change) and MigrateItemFieldValues (bulk select-option rename), which are also user-visible mutations the cursor must surface. Each path now: - acquires the workspace seq advisory lock (no-op on SQLite) - stamps seq = MAX(seq)+1 inside the same transaction The bulk rename gives all rows affected by a single statement the same seq value (MAX+1 at statement start). That preserves the "no overlap, no gap" cursor contract — a client at cursor < MAX sees them all in one batch, at cursor >= MAX sees none. |
||
|
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.
|
||
|
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.
|
||
|
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.
|
||
|
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 |
||
|
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). |
||
|
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.
|
||
|
xarmian
|
04514817ae |
feat(store): add Yjs op-log table + store methods (TASK-1252) (#450)
* feat(store): add Yjs op-log table + store methods (TASK-1252)
Persistence groundwork for the dumb-relay WebSocket server in PLAN-1248.
The item_yjs_updates table records every Yjs binary update (browser
edits, future designated-applier conversions of CLI/API content
changes) so reconnecting peers can replay updates since their last
known cursor and cold rooms can rebuild their in-memory Y.Doc.
Schema (mirrored across SQLite + Postgres):
- id monotonic — INTEGER PRIMARY KEY AUTOINCREMENT (SQLite)
/ BIGSERIAL (Postgres). Never reused, even after
deletes; serves as the cursor every reconnecting
client compares against.
- item_id FK with ON DELETE CASCADE so item deletion reclaims
op-log space automatically.
- update_data raw Yjs binary update — BLOB / BYTEA. Opaque to the
server.
- schema_version stamped per row. Mismatch on connect drives
TASK-1268's snapshot-and-rebuild flow.
- created_at ISO8601 UTC TEXT, matching pad's cross-dialect
timestamp convention (see migrations/047_attachments).
Drives PruneYjsUpdatesBefore.
Store API (internal/store/yjs_updates.go):
- AppendYjsUpdate — validates non-empty itemID/data/schemaVersion,
inserts and returns the new monotonic id (RETURNING on Postgres,
LastInsertId on SQLite). Empty-zero-byte updates are rejected at
the Go layer rather than relying on NOT NULL — they're a no-op
that would only pollute the log.
- LoadYjsUpdatesSince — strict id > sinceID filter, ordered by id
ascending. sinceID=0 returns everything (cold-room rebuild path).
Tolerates either RFC3339 or "YYYY-MM-DD HH:MM:SS" timestamp formats
on read so any future operator-written / CURRENT_TIMESTAMP-style row
doesn't blow up the load path.
- PruneYjsUpdatesBefore — created_at < cutoff, scoped to itemID.
Returns rows-affected count. Used by the eventual GC sweeper
(out of scope for this task).
Tests cover: append + monotonic ids, load-since-cursor filtering,
input validation, prune scoped to itemID, and ON DELETE CASCADE on
parent item removal. Pass on SQLite locally; Postgres mirror migration
+ store methods are dialect-agnostic.
Parent: PLAN-1248. First task of Phase 1 — Backend foundation.
* docs(store): document AppendYjsUpdate per-item serialization contract per Codex review (round 1)
P1: Postgres BIGSERIAL ids are allocation-ordered, not commit-order.
Concurrent appends to the same item could in theory produce a cursor
gap — a slower transaction can hold a smaller id while a faster one
commits a larger id first, and a reader that advances past the visible
larger id would later miss the smaller id when it commits.
The dumb-relay room manager (TASK-1255) is the sole writer per item by
design — there's exactly one goroutine appending per Y.Doc — so the
hazard does not manifest in practice. The fix is at the API contract
level: the doc comment now spells out the serialization requirement,
why the room manager satisfies it, and the multi-replica re-enforcement
note for the future Redis-fanout IDEA. We do not take an internal
advisory lock because that would be paid by every append even though
the caller already holds the per-room mutex.
No code change — contract is at the doc comment.
|
||
|
xarmian
|
89e9551ae3 |
test(store): end-to-end onboarding walkthroughs for scrum + product (TASK-1151) (#410)
Mirrors TestOnboardingFlow_FullWalkthrough_Startup (PR #405) for the two newly-seeded software-category templates. Two new test fns, same three-phase shape: Phase 1 — Fresh seed: - Four onboarding seeds land at the right item_numbers + statuses. - Conventions + playbooks present (after the user-facing seeds). - Primary entry starts in its initial status (BACK-1=new for scrum; FEAT-1=proposed for product) — the gate the dashboard banner relies on. Phase 2 — Agent walks user through populating real items: - Primary's status flips out of initial (signaling engagement). - Real workspace activity gets captured in the template's verbs: scrum: a sprint, three real backlog items linked to it, one bug product: a roadmap commitment, three features under it, one user-feedback item from a sales call - Primary flips to terminal (BACK-1 → done; FEAT-1 → shipped) — banner hides on next dashboard refresh. Phase 3 — Idempotency on re-trigger: - User's items remain untouched. - Primary stays at terminal status — re-seed must NOT reset to initial, which would silently re-show the banner. - No duplicate seed items. - Conventions + playbooks counts unchanged. Reuses the helpers added in PR #405 (findItemByTitle, extractStatus, setItemStatus, countItemsInCollection) — no new helpers needed. This is the gate task for PLAN-1146. With this merged, scrum + product now have the same coverage as startup did after PLAN-1131. Parent: PLAN-1146. |
||
|
xarmian
|
8fc0cb3b8b |
feat(collections): seed onboarding items + explicit prefixes for scrum + product templates (TASK-1149) (#408)
* feat(collections): seed onboarding items + add explicit prefixes for scrum + product templates (TASK-1149) Mirrors TASK-1133's pattern (PR #402) for the remaining software-category templates. After this lands: - fresh `pad workspace init --template scrum` → BACK-1 / SPRINT-2 / BUG-3 / DOC-4 - fresh `pad workspace init --template product` → FEAT-1 / FB-2 / ROAD-3 / DOC-4 Each is a first-person note from the workspace owner's future self — agent-invocable via `/pad let's discuss <REF>`, schema-aware terminal verbs ("mark me done" / "completed" / "shipped" / "archived"), no "tutorial" / "lesson" language. Bodies pulled verbatim from DOC-1152 (scrum) and DOC-1153 (product). Precondition fix: explicit Prefix set on five collections so DerivePrefix doesn't yield awkward refs: Backlog BACKL → BACK Sprints SPRIN → SPRINT Features FEATU → FEAT Feedback FEEDB → FB Roadmap Items RI → ROAD Mirrors hiring template's pattern of explicit prefixes on its custom collections. Existing scrum/product workspaces (forward-only fix) keep their derived prefixes — the seeder doesn't migrate. New tests: internal/collections/templates_test.go - TestScrumOnboardingItemsOrderAndShape - TestProductOnboardingItemsOrderAndShape - TestScrumProductTemplatesShipOnboardingSeedItems - TestScrumProductTemplatesUseExplicitFriendlyPrefixes (locks the prefix-fix precondition) internal/store/items_test.go - TestSeedCollectionsFromTemplateScrumRefSequence - TestSeedCollectionsFromTemplateProductRefSequence (Both also assert the prefix lands on each seeded item — drift in templates.go would surface here as a test failure pointing at the PLAN-1146 prefix precondition.) Existing onboarding test (TestSeedCollectionsFromTemplateStartupRefSequence) still passes — startup template untouched. Parent: PLAN-1146. Source content: DOC-1152, DOC-1153. * docs(comments): clarify the post-signup hint is wired in TASK-1150, not this PR (Codex review round 1) Codex flagged that the helper-file + templates.go comments said things like "the post-signup hint will name BACK-1" — which read as "it does today" but actually means "it will once TASK-1150 lands." Until that ships, the dashboard banner and CLI hint still hardcode IDEA-1 from PR #403, so a fresh scrum/product workspace gets the seeded items but no UI prompt that names them. Comments now explicitly call out the in-flight state so readers between this PR and TASK-1150 know what's wired and what isn't. No behavior change. |
||
|
xarmian
|
de9c87622a |
test(store): end-to-end onboarding walkthrough — fresh seed → user activity → idempotent re-trigger (TASK-1137) (#405)
Validates the entire arc PLAN-1131 promises, at the store level (the
API surface real workspace creation and real agent activity ultimately
call through). The "agent" steps are stubbed via direct CRUD — the
agent's reasoning is independently locked down by TASK-1136's resource
test, so this layer focuses on the workspace state machine.
Three phases mirror the user's experience:
Phase 1 — Fresh-workspace seed:
- The four onboarding seeds land at IDEA-1 / PLAN-2 / TASK-3 / DOC-4
in the right order with the right titles + statuses.
- Conventions + playbooks land too (after the user-facing seeds).
- IDEA-1 starts in status=new — the gate the post-signup hint relies
on for "should I show the dashboard banner?".
Phase 2 — Agent walks user through populating real items:
- IDEA-1 status flips new → exploring (signaling engagement).
- One real plan gets created, three tasks under it, one user-supplied
idea — using the actual user-facing collections.
- IDEA-1 status flips exploring → implemented (closes the loop;
dashboard banner hides on next refresh).
Phase 3 — Idempotency on re-trigger (server-startup auto-upgrade or
explicit re-init):
- User's plan / tasks / idea remain untouched.
- IDEA-1 status STAYS at `implemented` — re-seeding must NOT reset
it to `new`, which would silently re-show the banner and confuse
the user.
- No duplicate seed items.
- Conventions + playbooks counts unchanged.
Failure of any of these signals a regression in PLAN-1131's success
criteria. Walk back through the design doc before "fixing" the test.
Three small test helpers also added (findItemByTitle, extractStatus,
setItemStatus, countItemsInCollection) — kept private to the package
and used only by this test, but factored out so the assertions read
cleanly.
Parent: PLAN-1131. Origin: IDEA-1128.
|
||
|
xarmian
|
96253f18a2 |
feat(collections): seed IDEA-1/PLAN-2/TASK-3/DOC-4 in startup workspaces (TASK-1133) (#402)
* feat(collections): seed IDEA-1/PLAN-2/TASK-3/DOC-4 in startup workspaces (TASK-1133) A fresh `pad workspace init --template startup` now seeds four onboarding items — one per user-facing collection — that any agent can fetch and meaningfully converse around. The post-signup hint will name IDEA-1 specifically, but PLAN-2 / TASK-3 / DOC-4 are all viable entry points for `/pad let's discuss <REF>`. The bodies are first-person notes from the workspace owner's future self that introduce each collection's purpose by inviting a real conversation about the user's project — no marker, no skill detection, no schema fields. Word-audit clean: no "tutorial / lesson / step / walkthrough". Bodies pulled verbatim from DOC-1139. Sequence-stability: the existing seeder loop in store.SeedCollectionsFromTemplate already runs SeedItems before conventions/playbooks, so the workspace-scoped item_number sequence naturally lands at IDEA-1 / PLAN-2 / TASK-3 / DOC-4. A dedicated test (TestSeedCollectionsFromTemplateStartupRefSequence) locks the invariant down — drift means the post-signup hint silently misfires. Scope: startup template only. Scrum and product templates have different collection sets (Backlog/Sprints/Bugs and Features/Feedback/Roadmap respectively) and need their own bodies — tracked as follow-up under PLAN-1131. People-category templates (hiring, interviewing) are PLAN-1140. Parent: PLAN-1131. Source content: DOC-1139. * fix(collections): use schema-valid terminal statuses in onboarding bodies per Codex review (round 1) The seed bodies told agents to "mark me done" but ideas/plans/docs don't have a `done` terminal status — the HTTP/MCP update path validates select options, so an agent following the seeded copy would hit a validation error instead of completing the seed item. - IDEA-1: "mark this idea done" → "mark this idea implemented" (Ideas terminal: implemented|rejected) - PLAN-2: "mark me done" → "mark me completed" (Plans terminal: completed) - TASK-3: unchanged — "done" is the canonical terminal for Tasks - DOC-4: "mark me done" → "archive me" (Docs terminal: archived) Caught by Codex on PR #402. Same hard validation path the rest of the app honors — the seed copy needs to be schema-aware. |
||
|
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).
|
||
|
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.
|
||
|
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. |
||
|
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.
|
||
|
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.
|
||
|
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.
|
||
|
xarmian
|
2a00775481 |
feat(oauth): schema + storage layer (TASK-1023, sub-PR A of TASK-951) (#370)
* feat(oauth): schema + storage layer for OAuth 2.1 server (TASK-1023, sub-PR A of TASK-951)
First of 5 sub-PRs landing the OAuth 2.1 authorization server in
PLAN-943. This one is foundation only — no HTTP exposure, no fosite
import, no public surface change.
Schema (5 tables, parallel SQLite + Postgres migrations):
- oauth_clients — RFC 7591 Dynamic Client Registration; public clients only for v1
- oauth_authorization_codes — short-lived codes for the auth-code grant
- oauth_access_tokens — opaque HMAC; subject denormalized for fast user-bound queries
- oauth_refresh_tokens — same shape; access_token_signature link + request_id chain
- oauth_pkce_requests — PKCE session keyed by auth-code signature
Storage layer (internal/store/oauth.go):
- 12 public methods covering fosite's ClientManager + CoreStorage +
PKCERequestStorage + TokenRevocationStorage interface shapes,
using pad-internal types so the package stays fosite-free.
- Three sentinel errors (ErrOAuthNotFound, ErrOAuthInvalidatedCode,
ErrOAuthInactiveToken) that sub-PR B's adapter maps to the
matching fosite errors.
- request_id IS the chain identifier (fosite preserves it across
rotations — handler/oauth2/flow_refresh.go:86), so family
revocation is a single indexed UPDATE rather than a separate
chain_id column.
14 tests covering: client CRUD + idempotent delete + empty-slice
normalization, auth-code create/get/invalidate (including the
"return payload alongside ErrInvalidatedCode" contract fosite
relies on for family revocation), access-token CRUD + delete,
refresh CRUD + RotateRefreshToken (single-row flip), refresh-token
family revocation (entire chain via request_id, leaves other
chains untouched), access-token family revocation, PKCE CRUD, and
required-field validation.
Both backends share test bodies via testStore(t); set
PAD_TEST_POSTGRES_URL=... to run the same suite against Postgres.
Out of scope for this PR (subsequent sub-PRs):
- fosite import + adapter (sub-PR B / TASK-1024)
- DCR + authorize + token endpoints (sub-PR C / TASK-1025)
- revoke + introspect endpoints (sub-PR D / TASK-1026)
- MCPBearerAuth OAuth integration (sub-PR E / TASK-1027)
* fix(oauth): always insert active=true; drop broken zero-value Active override per Codex review (round 1)
Codex round 1 caught a P1 in insertOAuthRequestRow:
active := defaultActive
if req.Active != defaultActive {
active = req.Active // <- zero-value collides
}
When defaultActive=true and req.Active=false (the zero value), this
branch fires and the row is stored with active=FALSE — silently
producing immediately-revoked tokens. Any sub-PR B adapter that
built an OAuthRequest without explicitly setting Active=true would
ship broken.
Fix: hardcode active=TRUE on insert. Drop the defaultActive
parameter (it's always true for the three flagged tables; PKCE
has no active column). Pre-seeding inactive isn't a supported flow
— fosite never does it, and tests that need a revoked row do
Create + Invalidate / Rotate / RevokeFamily as a two-step.
Regression test TestOAuth_Insert_AlwaysActive constructs an
OAuthRequest with zero-value Active and asserts the row is
readable as active for all three table types (codes, access,
refresh). Without the fix the test fails on the first GetAccessToken
call with ErrOAuthInactiveToken.
* fix(oauth): RotateRefreshToken revokes both refresh + access families per Codex review (round 2)
Codex round 2 caught: my RotateRefreshToken only marked the named
refresh row inactive, but fosite's reference MemoryStore.RotateRefreshToken
(storage/memory.go:497-504) revokes BOTH the refresh family AND the
access family for the grant's request_id. Without this, every access
token issued before a refresh remained active until TTL — defeating
the rotation's invalidation contract.
Fix: RotateRefreshToken now delegates to RevokeRefreshTokenFamily +
RevokeAccessTokenFamily (both already existed). The signatureToRotate
parameter becomes vestigial — fosite passes it but the family revoke
catches every chain member regardless of which row triggered the
rotation. The new pair fosite immediately issues via
CreateAccessTokenSession + CreateRefreshTokenSession inherits the
same request_id (flow_refresh.go:86) and lands active=TRUE per the
round-1 hardcode, so the net post-rotation state is "all old rows
in this grant inactive, the new pair active."
Test rewrite: TestOAuth_RotateRefreshToken_FlipsActiveOnSingleRow
asserted the OPPOSITE behavior (only one row touched) — that was
the original bug. Replaced with TestOAuth_RotateRefreshToken_RevokesEntireGrant
which seeds a refresh + access pair in the same chain, plus a
distinct unrelated grant, then asserts after rotation:
- old refresh + old access both inactive
- unrelated grant untouched (request_id-scoped)
* fix(oauth): DeleteOAuthClient cascades dependent rows in a tx per Codex review (round 3)
Round 3 finding: DeleteOAuthClient errored with FK constraint
violation for any client that had ever issued a grant. The
migrations declare client_id FKs without ON DELETE CASCADE — by
design, so a stray DELETE FROM oauth_clients elsewhere fails
loudly rather than silently nuking grants — but that meant the
"officially supported" delete path was unusable.
Fix: DeleteOAuthClient now runs five sequential DELETEs inside a
single transaction:
1. oauth_pkce_requests
2. oauth_refresh_tokens
3. oauth_access_tokens
4. oauth_authorization_codes
5. oauth_clients
Order matters (children before parent) because the FKs aren't
cascading. The tx makes it atomic — if any step fails, nothing's
deleted, so we never leave a half-deleted client. Idempotent
because every WHERE matches nothing on a non-existent client.
Test TestOAuth_DeleteOAuthClient_CascadesDependentRows seeds a row
in each of the four dependent tables, deletes the client, and
asserts ErrOAuthNotFound on every dependent row + the client itself.
Without the fix this fails on the first DELETE FROM oauth_clients
with an FK constraint violation.
* fix(oauth): SELECT FOR UPDATE row lock in DeleteOAuthClient on Postgres per Codex review (round 4)
Codex round 4 caught a Postgres race in DeleteOAuthClient: the
five-DELETE cascade is atomic, but between the child-row deletes
and the parent delete, a concurrent fosite handler can insert a
fresh grant/token referencing the same client_id. The parent
DELETE then fails with an FK violation and the whole tx rolls
back — the cascade is correct, but unreliable under concurrent
OAuth issuance.
Fix: take SELECT id FROM oauth_clients WHERE id = ? FOR UPDATE
as the very first statement in the tx (Postgres only). The
exclusive row-level lock blocks any concurrent statement that
tries to read the client row — which fosite does on FK resolution
during grant/token inserts — until our tx commits.
Skipped on SQLite because:
(a) BEGIN IMMEDIATE serializes the entire write workload globally
(DSN configures _txlock=immediate per store.go), so the race
doesn't exist.
(b) FOR UPDATE syntax isn't reliably accepted across SQLite
drivers.
ErrNoRows on the lock query is treated as "client doesn't exist
yet" — the subsequent DELETEs match nothing and the call remains
idempotent. Tests still pass on the SQLite path; the Postgres
path's race fix will be exercised by CI's PAD_TEST_POSTGRES_URL
runs and any future concurrency test we add.
|
||
|
xarmian
|
0f05012169 |
fix(mcp): six surgical fixes for BUG-987 (bugs 6, 8, 11, 12, 13, 14) (#361)
* fix(mcp): six surgical fixes for BUG-987 (bugs 6, 8, 11, 12, 13, 14)
Hotfix follow-up to v0.1.0-rc.3's Claude Desktop dogfood. Six
surgical fixes; bigger items (5, 7, 9, 10) deferred to separate
tasks.
- Bug 6: `pad project next --format json` was emitting the entire
dashboard, indistinguishable from `pad project dashboard --format
json`. Now slices to suggested_next only. cmd/pad/main.go.
- Bug 8: standup blockers carried empty `ref` strings, blocking
agent linkback to the actually-blocked items. dashboard's
attention[].item_ref is canonical; the standup composer in
internal/mcp/dispatch_http_slice4.go just wasn't propagating it.
Same fix applied to suggested_next entries.
- Bug 11: cobra's auto-emitted "Usage: pad item block ..." help
block leaked into MCP error envelopes via classifyExecError. The
Usage text references OLD CLI verb names (pre-v0.2 catalog) that
agents using the new surface have no business seeing, and bloats
every error response. New stripCobraUsageBlock helper truncates
stderr at the first line-anchored "Usage:" marker before
classification + envelope construction.
- Bug 12: BuildCLIArgs validation errors (missing required arg, type
mismatch) came out of env.Dispatch as bare-text NewToolResultErrorf
results, breaking the structured envelope contract. New helper
validationFailedFromBuildErr wraps them as ErrValidationFailed
envelopes with the field name extracted via regex from the
underlying message.
- Bug 13: every Task / Idea / Plan with a `priority` field got a
phantom `convention: { enforcement: "<priority>" }` surfaced on
its response, because ExtractItemConventionMetadata's legacy
fallback treated `priority` as the Convention enforcement tier
unconditionally. Restructured to track hasConventionShape
separately from hasMetadata — only Convention-specific markers
(structured convention field, trigger, scope, surfaces, commands,
direct enforcement) flip the shape flag. category alone is
insufficient (Ideas / Bugs / Roadmap items legitimately use it).
Final guard returns nil when only category was matched.
- Bug 14: GetRoleBreakdown's unassigned row was emitted with empty
role_name + role_slug, presenting as a "phantom" entry in the
dashboard. Now explicitly labelled "Unassigned" / "unassigned"
while keeping role_id null so it's still distinguishable from a
real role.
Tests:
- internal/mcp/bug987_test.go (new) — stripCobraUsageBlock + classify
+ validation envelope wrapping + env.Dispatch integration.
- internal/models/item_test.go — three cases covering non-Convention
items (Task, Idea, Plan with priority) returning nil metadata, and
one preservation test for legacy Conventions with priority field.
- internal/store/agent_roles_test.go (new) — confirms unassigned row
carries explicit "Unassigned" / "unassigned" labels.
Live verified: pad_project action=next returns just the suggestions
array; pad_item action=create with no fields returns validation_failed
with field=collection; pad_item action=link with self-target returns
without Usage-block leakage.
Deferred to separate items (per BUG-987 triage):
- Bug 5: text vs JSON returns across note, decide, star, unstar,
delete, bulk-update — needs CLI-side handler updates per command.
- Bug 7: suggested_next algorithm — needs to consider in-progress
items, not just open ones; behavior change needs design.
- Bug 9: fields/tags double-stringified — potentially breaking for
web UI/CLI consumers.
- Bug 10: decision_log/notes embedded in fields blob duplicating
top-level arrays — might require data migration.
Parent: BUG-987.
* fix(mcp): HTTP transport equivalence + ordering for BUG-987 per Codex review (round 1)
Two findings from Codex review of PR #361:
1. project.next on HTTP transport still returned the full dashboard.
The route table mapped "project next" directly to /dashboard, so
the CLI fix (slice to suggested_next) didn't reach OAuth-authed
agents going through HTTPHandlerDispatcher. Catalog actions must
produce equivalent shapes on stdio and HTTP — that's the contract
that lets agents be transport-agnostic.
Fix: new dispatchProjectNext method on HTTPHandlerDispatcher that
fetches the dashboard via the existing fetchDashboardJSON helper,
slices to suggested_next[], re-encodes, and runs through
packageJSONResult so it gets the same {items: [...]} wrap as
other list responses.
Also retires the broken route-table entry — replaced with a
comment pointing at the new method so future contributors don't
re-add a passthrough.
Test: TestDispatch_ProjectNext_SlicesToSuggestedNext + the empty-
array case. Asserts dashboard-only top-level fields (summary,
active_items) don't leak into the response — that's the whole
point of project.next being distinct from project.dashboard.
2. ExtractItemConventionMetadata's priority→enforcement legacy
fallback ran BEFORE surfaces/scope/commands had a chance to flip
hasConventionShape, so a Convention with only `{scope, priority}`
would silently drop enforcement.
Fix: move the priority fallback to AFTER all marker checks. Direct
`enforcement` still resolves first; the legacy priority fallback
runs at the bottom once shape detection is complete.
Tests: two new cases covering scope-only and commands-only legacy
Conventions — both must resolve enforcement via the priority
fallback.
Parent: BUG-987.
|
||
|
xarmian
|
5e27989ab8 |
feat(attachment): add pad attachment view|show|list CLI surfaces (IDEA-898) (#321)
* feat(attachment): add `pad attachment view|show|list` CLI surfaces (IDEA-898) Agents and CLI users had no first-class way to fetch attachment bytes through the API: the only path to read an `` reference was to read the raw blob out of `~/.pad/attachments/<storage_key>`, which bypasses workspace ACLs, doesn't work on Pad Cloud / remote / Postgres deployments, skips the variant pipeline (TASK-872 / TASK-879 / TASK-880), and breaks when storage moves to S3. Three new subcommands wrap the existing REST endpoints: - `pad attachment view <id> [-o path]` — agent-friendly: with no `-o`, fetches to a fresh OS temp directory using the stored filename and prints just the absolute path on stdout (so `$(pad attachment view <id>)` composes cleanly into shell pipelines). Reuses `download`'s atomic temp-then-rename pattern via a shared helper. - `pad attachment show <id>` — HEAD-based metadata only; surfaces MIME, size, filename, ETag, Last-Modified. - `pad attachment list [--item REF] [--category X] [--attached|--unattached] [--collection ID] [--sort ...] [--limit N] [--offset N]` — workspace list. The `--item REF` flag resolves a TASK-5-style ref to a UUID client-side and passes it to a new `item_id` query param on the list endpoint (server side: AttachmentListFilters.ItemID, ~6 lines in the store + 1 in the handler). Skill update: `skills/pad/SKILL.md` gains a "Working with attachments" subsection plus a CLI Reference entry, both ending in the hard rule that agents must NEVER read directly from `~/.pad/attachments/`. * style(cli): gofmt AttachmentListParams field alignment CI's golangci-lint v2 flagged this with the gofmt formatter (configured with simplify: true in .golangci.yml). The contiguous Sort/Limit/Offset block at the end of the struct needs uniform column alignment — gofmt considers the doc comment above Sort attached to that field rather than a block separator, so the three int/string fields get aligned together. Verified locally with `golangci-lint run --timeout=5m ./...` (v2.11.4 to match CI) — 0 issues. Local make lint only runs `go vet ./...` and `golangci-lint` wasn't installed, which is why this slipped through; filing a separate follow-up to mirror the CI checks in the local workflow. |
||
|
xarmian
|
f8ed3e10a7 |
fix(search): explicit selection on Enter + numeric go-to (BUG-864, BUG-910) (#320)
* fix(search): require explicit selection on Enter; add bare-number go-to (BUG-864, BUG-910) The command palette had two related issues: - BUG-864: Pressing Enter armed the first search result automatically — the user could close the modal and navigate without ever pressing an arrow key. selectedIdx now starts at -1 and only advances on ArrowDown/ArrowUp. - BUG-910: Typing a bare number (e.g. "843") returned no results because parseItemRef requires PREFIX-NUMBER and FTS doesn't index item_number. Backend (internal/store): - Add parseItemNumber() helper alongside parseItemRef. - In Search(), add a bare-numeric direct-lookup path that mirrors the existing ref-lookup block but without a collection prefix filter. item_number is unique per workspace (idx_items_workspace_number) so this resolves to at most one direct hit, prepended with rank=-1000. Frontend (CommandPalette.svelte): - selectedIdx defaults to -1; reset to -1 (not 0) on modal open and after every search. - Enter on a non-numeric query is a no-op unless the user has arrow-selected. - Numeric queries are a deliberate exception: Enter on a bare-number query flushes the debounce, navigates directly to the matching item, and lets the search palette double as a quick "go to item N" jump. Tests: - TestSearch_BareNumericQueryFindsItemByNumber covers the new path. - TestParseItemNumber covers helper edge cases. * fix(search): exclude direct hits from FTS WHERE to keep pagination correct Codex review (round 1) on PR #320: > Numeric direct hits are appended before the FTS query, but the later > pagination only removes duplicates after SQL LIMIT/OFFSET. If item #2 > also matches FTS for query "2" through its title/content, that > duplicate consumes an FTS slot, so page 1 can return fewer than `limit` > results and later pages can repeat/skip rows. Hoist the direct-hit (ref + numeric) snapshot to before the FTS query is built, then append `AND i.id NOT IN (...)` to both the SELECT and COUNT FTS queries. After a successful count, add refCount back so SearchResponse.Total still reflects the full result set (since FTS itself no longer counts those rows). The flaw also applied to the pre-existing parseItemRef path; this fix covers both. The post-LIMIT dedup loop is now defense-in-depth. New test TestSearch_BareNumericQueryDedupsAgainstFTS guards the case: an item whose title/content literally contains its own item_number (so it matches both the direct lookup and FTS) appears exactly once in Results and Total counts it exactly once. * fix(search): paginate direct hits properly across workspaces Codex review (round 2) on PR #320: > P1: Bare numeric direct hits break pagination in global search. > item_number is only unique per workspace, so q=1 with WorkspaceIDs > spanning N workspaces returns N direct hits — all appended without > being sliced to Limit. limit=1 with three workspaces each having #1 > returns three results on page 0, and offset=1 drops all direct hits > then returns FTS rows instead of the second direct hit. The same flaw applied to the pre-existing parseItemRef path: the global search "TASK-5" can match TASK-5 in multiple workspaces. Fix: - Add deterministic ORDER BY i.workspace_id, i.id to both ref and bare- numeric direct-hit lookups so pagination is stable across pages. - Replace the offset==0/offset>0 branching pagination with a uniform slice: directStart = min(Offset, refCount); directEnd = min(Offset+Limit, refCount); results = results[directStart:directEnd]; ftsLimit = Limit - directConsumed; ftsOffset = max(Offset - refCount, 0). This honours (offset, limit) whether direct hits, FTS, or both fill the page. Total stays correct because the FTS count was already excluding direct hits (round-1 fix) and we add refCount back unconditionally. New test TestSearch_BareNumericQueryPaginatesAcrossWorkspaces creates three workspaces each with item #1 and verifies that limit=1 with offsets 0/1/2 returns three different direct hits in stable order, and limit=10 returns all three. * chore: gofmt — column alignment in struct field declarations CI Go (SQLite) lint failed on two files: - internal/store/store_test.go (TestParseItemNumber, this PR's new test) — unaligned column widths and inconsistent comment spacing. - internal/config/config.go (drive-by) — pre-existing alignment regression in the Config struct that snuck in via an earlier landed PR; included here because it blocks merge. No semantic changes — `gofmt -w` only. |
||
|
xarmian
|
47a4448afc |
chore(import-bundle): audit + harden bundle import validation (TASK-891) (#308)
* chore(import-bundle): audit + harden bundle import validation (TASK-891) Re-reviewed handlers_import_bundle.go before exposing the bundle import flow through the web UI under PLAN-890. The audit doc lives at DOC-895; this commit lands the small inline fixes. Findings + actions: - Duplicate pad-export.json now rejected (was: silently ran ImportWorkspace twice, stranding the first workspace as an orphan with no attachments). - Duplicate attachments/manifest.json now rejected (was: silently overwrote manifestByPath, dropping prior entries). - Defense-in-depth path-traversal guard added via isSafeBundleEntryName — rejects entries with `..` segments, absolute paths, or NUL bytes BEFORE the switch. Storage was already hash-keyed and safe, but the silent-skip behavior on malicious tar names was a poor audit story. - Auth/permissions now documented on the handler — RequireAuth middleware gates the endpoint; no per-workspace role check applies because the request creates a new workspace (mirrors handleCreateWorkspace). Tests added: TestImportBundle_RejectsDuplicateExport, TestImportBundle_RejectsDuplicateManifest, TestImportBundle_RejectsPathTraversal, TestIsSafeBundleEntryName. Two larger gaps deferred as their own tasks: - TASK-896 (partial-import orphan workspace on mid-stream failure — needs design discussion). - TASK-897 (per-user storage quota enforcement on import — gated on Phase 2 quota work; matches upload handler's warn-only Phase 1 policy today). Parent: PLAN-890. * fix(import-bundle): roll back partial workspace on validation reject per Codex review (round 1) Codex P1 on PR #308: when the duplicate-pad-export.json or duplicate-manifest.json guards fire, the workspace from the first occurrence has already been inserted by ImportWorkspace. The handler returned 400 but the orphan workspace stayed in the destination DB. A malformed/malicious bundle could repeatedly POST and pile up half-imported workspaces. Fix: when importBundle returns an importStatusError after creating a workspace, the handler now soft-deletes that workspace via DeleteWorkspace before returning the 400. Mid-stream errors that are NOT importStatusError (e.g. manifest decode after items inserted) intentionally keep the partial workspace — that's the existing design tracked under TASK-896 (partial-import design discussion). Tests extended: TestImportBundle_RejectsDuplicateExport and TestImportBundle_RejectsDuplicateManifest now also list the destination workspaces after the rejected import and assert the partial workspace does NOT appear. Parent: PLAN-890. * fix(import-bundle): cascade attachment tombstone on rollback per Codex review (round 2) Codex P1 round 2 on PR #308: when the duplicate-manifest guard fires AFTER blobs have already been rehydrated (e.g. bundle layout [pad-export, manifest, blob1, blob2, duplicate-manifest]), the previous fix soft-deleted the workspace but left the attachment rows live. Live rows pin blobs from orphan-GC and continue counting toward per-user storage usage even though the workspace is gone. Fix: added Store.SoftDeleteWorkspaceAttachments(workspaceID), a single bulk UPDATE that tombstones every live attachment row (originals AND thumbnails — both carry the same workspace_id) under a workspace. The handler's rollback path now calls this BEFORE DeleteWorkspace so orphan-GC reclaims the blobs after the grace window. Best-effort: any error in either op is logged with workspace context but the original 400 still flows. New test: TestImportBundle_RollbackTombstonesAttachments builds a real export bundle from a source workspace with one attachment, surgically appends a duplicate manifest.json AFTER the real entries, posts it, and asserts (a) 400, (b) workspace gone from listings, (c) zero live attachment rows on the destination. The pre-fix code left rows=1 live; the new path tombstones them. Parent: PLAN-890. * fix(import-bundle): return ws on path-traversal reject so rollback fires (Codex round 3) Codex P1 round 3 on PR #308: the path-traversal early-return at the top of the import loop returned (nil, importStatusError) instead of (ws, importStatusError). When a malicious path-traversal entry follows a valid pad-export.json, the workspace was already created — but because the handler saw ws == nil, it skipped the rollback cascade, leaving the workspace and any rehydrated attachments behind. Fix: return ws (which is nil before pad-export.json is processed, so the no-workspace cleanup path still works for first-entry-bad bundles, and non-nil after, so the cascade runs). One-line change keyed off the existing rollback flow. New test: TestImportBundle_PathTraversalAfterExportRollsBack hand-builds a tar with a valid pad-export.json followed by a "attachments/../../etc/passwd" entry, posts it, asserts 400, and asserts the partial workspace is GONE from listings. Pre-fix this test would have shown the workspace leaking through. Parent: PLAN-890. |
||
|
xarmian
|
2bb7ac35e4 |
feat(attachments): orphan GC sweep with periodic scheduler (TASK-886) (#307)
* feat(attachments): orphan GC sweep with periodic scheduler (TASK-886)
Background job that reclaims attachments past the grace period. Two
qualification criteria, both with a 30-day default grace:
- item_id IS NULL AND deleted_at IS NULL AND created_at < cutoff
(never-attached uploads — editor uploaded then tab-closed before
attaching to an item)
- deleted_at IS NOT NULL AND deleted_at < cutoff
(soft-deleted via the Settings → Storage delete button or the
DELETE /attachments/{id} endpoint)
Reclamation is dedupe-aware: content-addressed storage means the same
hash can be referenced by multiple rows, so the on-disk blob is only
removed when the GC'd row is the LAST live reference to its
content_hash. Otherwise the row drops and the blob stays for the
remaining references. CountLiveAttachmentsForHash is the predicate.
Per-row failures (resolve backend, blob delete, hard-delete) are
logged and skipped; the sweep keeps making progress. Catastrophic
errors (DB failure) return up to the loop, which logs and waits for
the next tick rather than crashing the server.
Lifecycle:
- SetOrphanGCConfig overrides the default 24h interval / 30-day
grace. cmd/pad reads PAD_ORPHAN_GC_INTERVAL / PAD_ORPHAN_GC_GRACE
(Go duration syntax — 1m, 24h, 720h) so operators can tune
without recompiling and tests can crank the interval down to 1ms
to see sweeps land in CI.
- StartOrphanGC kicks the loop. Idempotent — second call is a
no-op so a misconfigured caller can't double-spawn.
- Server.Stop() now signals the loop via stopOrphanGC() before
s.bg.Wait(), so process shutdown drains the goroutine cleanly
(BUG-842 invariant).
- Each tick wraps the sweep in a 30m context timeout so a slow
scan can't pin the goroutine across multiple intervals.
Tests:
- TestOrphanGC_ReclaimsSoftDeleted: upload → soft-delete → sweep
with future cutoff → DB row gone + blob gone from FSStore.
- TestOrphanGC_ReclaimsLongOrphans: upload → backdate created_at
31d → sweep with 30d grace cutoff → row reclaimed.
- TestOrphanGC_KeepsRecentRows: upload → soft-delete → sweep with
past cutoff → row stays. Catches a typo in the WHERE clause that
would silently destroy live attachments.
- TestOrphanGC_PreservesSharedBlob: two uploads with identical
bytes (same hash, same blob), soft-delete only one → sweep →
one row reclaimed BUT BlobsReclaimed=0 because the other row
still references the blob. Pin for content-addressed dedupe.
- TestOrphanGC_StartStop: loop spins up at 1ms interval, second
StartOrphanGC is a no-op, Stop drains via testServer's cleanup.
Parent: PLAN-866. Closes the phase 1 plan with full export →
import → orphan-cleanup round-trip.
* fix(attachments): protect referenced/in-flight blobs from orphan GC per Codex (round 1)
Two real correctness issues Codex caught on PR #307:
P1. The editor's normal upload flow leaves attachments.item_id NULL.
The canonical association lives in markdown content (the editor
PATCHes "pad-attachment:UUID" into the item) — but the GC's
"never-attached past 30d" predicate only checked item_id. So a
legitimate inline image could be hard-deleted 30 days after upload
even though item content still references it.
Added store.AttachmentReferencedInItems(workspaceID, attachmentID)
that scans items.content + items.fields for "pad-attachment:UUID".
The GC sweep now runs this check before reclaiming any
never-attached row; if any live item references the attachment,
the row is left alone (and re-checked next sweep).
P2. Race between concurrent upload and GC. Upload calls
AttachmentStore.Put (blob lands on disk) → THEN inserts the DB row.
Between those two steps an orphan-GC sweep could count zero live
refs for the hash, delete the blob, and the upload's row insert
would then point at a missing blob.
Added Server.inFlightUploadHashes (sync.Map of *atomic.Int64
counters) with markUploadInFlight / uploadInFlight helpers. Every
Put + CreateAttachment site fences itself via markUploadInFlight:
the upload handler, the transform handler, the thumbnail
derivation pipeline, and the bundle-import rehydrate path. The GC
sweep treats an in-flight hash as "another live ref" so it leaves
the blob alone.
Tests:
- TestOrphanGC_KeepsReferencedNeverAttachedRows: upload (item_id
NULL) → create item with pad-attachment: ref → backdate 31d →
sweep with 30d cutoff → row stays.
- TestOrphanGC_RespectsInFlightUploads: upload → soft-delete →
register an in-flight upload at the same hash → sweep → DB row
goes (it's tombstoned past grace) but blob stays so the
in-flight upload can complete cleanly.
The DB row still gets reclaimed in the in-flight case because the
soft-deleted row is independently past grace; only the blob delete
is fenced. That's correct: the blob remains usable for the
incoming upload and the new upload will register its own
attachments row.
* fix(attachments): mutex-protect in-flight tracker + portable JSONB scan per Codex (round 2)
Two fixes for the round-2 findings on PR #307:
P1. Same-hash race in the in-flight upload tracker. The sync.Map +
*atomic.Int64 design split increment from LoadOrStore-then-add and
release-decrement from delete, so a release could see "0" and start
deleting while another upload concurrently reloaded the same map
entry and incremented to "1" — the second upload's signal then
lived in a doomed map slot, invisible to subsequent uploadInFlight
calls.
Replaced with a plain map[string]int64 + sync.Mutex. Inc, dec,
delete-when-zero all run under one critical section, so any
inspection sees a consistent snapshot. Net cost is one mutex per
mark/release; uncontended this is ~10ns and the upload path is
already doing far more expensive work (Put + DB insert).
Stress test: 20 goroutines × 500 iterations of mark→check→release
on a shared hash. Every check must observe in-flight=true while
the calling goroutine holds the mark. Final state must be empty.
Runs cleanly under -race -count=3.
P2. Postgres JSONB compatibility. items.fields is TEXT on SQLite
but JSONB on PostgreSQL (per pgmigrations/001_initial.sql). LIKE
on JSONB fails with a type error, so the orphan GC's reference
scan would error on Postgres and skip every never-attached row —
breaking orphan reclamation for those rows entirely.
Cast fields::text in the Postgres dialect path:
fieldsExpr := "fields"
if s.dialect.Driver() == DriverPostgres {
fieldsExpr = "fields::text"
}
Same approach used elsewhere in the store for dialect-sensitive
text searches.
* fix(attachments): close GC/upload TOCTOU + protect in-grace peers per Codex (round 3)
P1 round 3: TOCTOU race between uploadInFlight check and store.Delete.
The mutex protected the in-flight counter but not the GC's
check-and-delete sequence. A new upload could call markUploadInFlight
between our check and our blob delete, then run Put after the blob
was gone — its CreateAttachment would insert a live row pointing at
the missing hash.
Fixed by holding inFlightHashesMu across the check + FS Delete:
s.inFlightHashesMu.Lock()
inFlight := s.inFlightHashes[hash] > 0
if !inFlight && others == 0 {
store.Delete(ctx, key)
}
s.inFlightHashesMu.Unlock()
A concurrent markUploadInFlight blocks until either we skip (because
we observed in-flight) or finish deleting. Lock window is ms-class
on FSStore; a per-hash lock can replace this server-wide mutex when
S3 lands in Phase 2.
P2 round 3: CountLiveAttachmentsForHash counted only live rows, so
GC could reclaim the blob from row A (soft-deleted 31d ago) even
when row B was also soft-deleted but only 1 day old — within
grace, so its blob must stay reachable until its own grace lapses.
Replaced with CountProtectingAttachmentsForHash which counts rows
where deleted_at IS NULL OR deleted_at >= graceCutoff. The blob is
preserved until every soft-deleted peer has aged past its own
grace window.
Tests:
- TestOrphanGC_RespectsSoftDeletedInGracePeer: two rows sharing a
hash, soft-delete both, backdate only one past 30d → sweep with
30d cutoff → older row reclaimed but blob stays for the still-in-
grace peer.
- existing TestOrphanGC_RespectsInFlightUploads still passes
(still uses the in-flight signal correctly).
* fix(attachments): dedupe blob-reclaim metric across same-hash peers per Codex (round 4)
Codex round 4 noted that when multiple soft-deleted peers share a
content_hash and all are past grace, the GC sweep would inflate
BlobsReclaimed and BytesReclaimed: AttachmentStore.Delete treats a
missing key as success, so the second peer's idempotent no-op
delete still bumped the counter.
Functional cleanup was correct (the blob really was gone after the
first peer); only the metric / log line was wrong, which makes
operator dashboards report fictitious bytes-reclaimed values.
Track per-sweep reclaimed hashes in a map and skip the Delete call
+ counter increment for repeats. The DB row still gets hard-deleted
on each peer.
Test: TestOrphanGC_DedupesBlobReclaimMetric uploads twice with
identical bytes (single shared blob), soft-deletes both, backdates
deleted_at past grace → sweep deletes 2 rows and reports
BlobsReclaimed=1 / BytesReclaimed=blobLen rather than 2 / 2*blobLen.
|
||
|
xarmian
|
134f55045d |
feat(attachments): import workspace bundle with rehydrate + UUID remap (TASK-885) (#306)
* feat(attachments): import workspace bundle with attachment rehydrate + UUID remap (TASK-885) POST /workspaces/import now accepts a tar.gz bundle (Content-Type: application/gzip) and rebuilds the workspace + attachments + items in one round trip. JSON imports still work — content-type dispatch in handleImportWorkspace routes the request. Three-phase flow: 1. Walk the tar, capture pad-export.json + manifest.json + every attachment blob into memory. 2. Run the existing ImportWorkspace path to create the workspace + collections + items + comments + links + versions. New IDs are generated; item.slug is preserved (the existing remap path doesn't re-slugify). 3. For each manifest entry, rehydrate the blob through the storage backend (re-validate MIME + re-hash defensively, don't trust the manifest), insert a fresh attachments row. Build an oldID→newID map keyed on attachment uuid. 4. Walk every imported item's content + fields, replace "pad-attachment:OLD" with "pad-attachment:NEW" in one transactional pass. Refresh FTS afterward (direct UPDATE bypasses triggers). Phase 2 errors per-attachment are logged and skipped — the workspace keeps importing rather than rolling back. The import handler returns the new workspace and the operator can inspect logs for any attachment that didn't make it. CLI: - pad workspace export now defaults to --bundle (.tar.gz) since pad import handles bundles. --json reverts to legacy items-only. - pad import auto-detects format by file extension (.tar.gz / .tgz → application/gzip). Other extensions go through the legacy JSON path. - New Client.PostRawWithContentType for explicit-content-type POSTs. Tests: - TestImportBundle_RoundTrip: upload → embed in markdown → export source → import to FRESH server → verify attachment list has 1 row with new UUID → item content rewritten to new UUID and old UUID is gone → download new blob matches original bytes. - TestImportBundle_LegacyJSONStillWorks: JSON content-type still hits the legacy path. - TestImportBundle_RejectsBadGzip: garbage gzip body returns 400. Parent: PLAN-866. With TASK-884 + TASK-885 merged, the round-trip acceptance criterion (export → import → images intact) is met. * fix(attachments): stream import end-to-end per Codex (round 1) Two memory regressions Codex caught on PR #306: P1 (server). importBundle was buffering every blob into a map[string][]byte during a first pass, then iterating the manifest on a second pass. A 2 GiB bundle full of 25 MiB attachments would pin ~2 GiB of heap. Reworked to single-pass streaming: pad-export.json → import workspace + build slug→id map attachments/manifest.json → index entries by tar path attachments/<uuid>.<ext> → look up entry, rehydrate now The export bundler always writes pad-export.json + manifest.json BEFORE any blob (deterministic order from handlers_export_bundle.go), so this works without buffering. Bundles that violate the ordering — a third-party tool that writes blobs first — return 400 with a clear error. Memory footprint now bounded by the largest single blob (≤25 MiB) regardless of bundle size. Stale blobs without a manifest entry are skipped (their bytes io.Copy'd to io.Discard so the tar reader stays in sync). Unknown top-level entries (forward-compat for future bundle additions) are also consumed and ignored rather than left dangling. P2 (CLI). pad import used os.ReadFile, buffering the entire bundle client-side before posting. Switched to os.Open + a new Client.PostStreamWithContentType helper that streams the body directly into the request — together with the server-side fix, import is end-to-end streaming. Tests: - TestImportBundle_RejectsOutOfOrderTar: hand-crafted bundle with a blob before pad-export.json returns 400 with "ordering" in the message. - existing TestImportBundle_RoundTrip / LegacyJSONStillWorks / RejectsBadGzip continue to pass under the new streaming flow. * fix(cli): give streaming endpoints a 1h timeout per Codex (round 2) Codex P1 round 2: PostStreamWithContentType + RawStream were both using the shared 10s-timeout httpClient. The default works fine for normal API calls but kills a multi-GiB bundle import or export over anything slower than a local network — Client.Timeout fires mid-stream with "Client.Timeout exceeded". Added a dedicated streamClient on Client with a 1h timeout, used by both RawStream (export bundle download) and PostStreamWithContentType (import bundle upload). 1h is generous enough for ~100 MB/s uplinks shipping a 350 GiB bundle and still caps a hung connection eventually. The 10s default stays in place for every other call — short timeouts are the right SLA for normal API requests and protect the CLI from hanging on a wedged server. * fix(attachments): make import bundle cap configurable per Codex (round 3) Codex P1: the 2 GiB import cap was hard-coded with a comment promising operator override "later" — but no setter existed, so workspaces over 2 GiB stream out fine on export and fail on re-import. Added Server.SetImportBundleMaxBytes wired from the PAD_IMPORT_BUNDLE_MAX_BYTES env var in cmd/pad/main.go. Mirrors the existing PAD_ATTACHMENT_MAX_BYTES pattern. Default stays at 2 GiB so the typical workspace works without configuration; operators with larger exports can raise it without recompiling. The per-blob cap (importBlobMaxBytes = 25 MiB) is intentionally kept constant — it bounds in-flight memory regardless of total bundle size, and a 25 MiB-per-blob ceiling matches the upload handler's default, so a bundle can never smuggle larger blobs than the upload endpoint accepts. * fix(attachments): scale per-blob import cap with PAD_ATTACHMENT_MAX_BYTES per Codex (round 4) Codex P1 round 4: importBlobMaxBytes was hard-coded at 25 MiB but the upload handler's per-file cap is configurable via PAD_ATTACHMENT_MAX_BYTES. An operator who raised the upload cap to allow 50 MiB attachments could export a workspace successfully (WorkspaceAttachmentsForExport doesn't gate on size) but the re-import would reject every blob over 25 MiB. Replaced the const with effectiveBlobMaxBytes() which reads s.attachmentMaxBytes (or falls back to defaultAttachmentMaxBytes). The pad-export.json cap also scales with this value (4×) so a content-heavy workspace doesn't trip its own JSON ceiling on a server with raised attachment limits. Error message on a too-large blob now points the operator at PAD_ATTACHMENT_MAX_BYTES so they know which knob to turn rather than digging through code to find the cap. * fix(attachments): independent metadata cap for bundle import per Codex (round 5) Codex P2 round 5: tying pad-export.json + manifest.json caps to PAD_ATTACHMENT_MAX_BYTES regressed deployments that LOWER the attachment cap. A 1 MiB attachment cap would force metadata to fit in 4 MiB / 1 MiB respectively — but metadata size scales with workspace item count, not attachment blob sizes, so a tight upload limit shouldn't gate it. Added importMetadataMaxBytes = 100 MiB constant for both metadata files. effectiveBlobMaxBytes() still drives the per-blob cap which genuinely tracks attachment-upload policy. |
||
|
xarmian
|
a0336e0248 |
feat(attachments): bundle attachments + manifest in workspace export (TASK-884) (#305)
* feat(attachments): bundle attachments + manifest in workspace export (TASK-884)
GET /workspaces/{ws}/export?format=tar streams a gzip'd tar bundle:
pad-export.json # the existing WorkspaceExport JSON
attachments/manifest.json # uuid → {filename, mime, size, hash, ...}
attachments/<uuid>.<ext> # original blobs only — no thumbnails
Default (no ?format) keeps returning JSON so existing automation
hitting the endpoint without a query param continues to work
unchanged. The CLI's pad workspace export now opts into the bundle
by default; pass --json for the legacy items-only output.
Implementation:
- store.WorkspaceAttachmentsForExport returns originals only
(parent_id IS NULL); thumbnails are re-derived on import via the
existing pipeline so shipping them would double the bundle size.
- handleExportWorkspaceBundle streams chunks straight into the
response writer rather than buffering — a workspace with multi-
GB of attachments would otherwise pin that much memory.
- AttachmentManifest is versioned (separate from WorkspaceExport
version) so the bundle layout can evolve independently.
- bundleAttachmentPath is exported (lowercase package fn) so the
import path in TASK-885 can resolve manifest entries to tar
entries without duplicating the filename logic.
- CLI gates against writing binary tar.gz to a TTY and appends the
conventional extension when -o is passed without one.
Tests:
- TestExportBundle_RoundTrip: two uploads → bundle contains
pad-export.json + manifest + 2 blobs whose bytes match the
uploads + manifest decodes cleanly + WorkspaceExport decodes.
- TestExportBundle_HidesThumbnails: synthetic thumbnail row, the
manifest excludes it.
- TestExportBundle_LegacyJSONStillWorks: no ?format param returns
application/json with a decodable WorkspaceExport (backward
compat regression guard).
Parent: PLAN-866. TASK-885 (import path + UUID remap) consumes the
manifest produced here.
* fix(attachments): stream export bundle + revert default to JSON per Codex (round 1)
Two findings from Codex on PR #305:
1. CLI buffered the entire response in memory via RawGet → io.ReadAll,
defeating the server-side streaming design and risking OOM on a
multi-GB bundle. Added Client.RawStream which copies the response
body straight into an io.Writer; export now opens the target file
and streams directly into it.
2. Default tar.gz output broke `pad export → pad import` round trip
because the import handler still only accepts JSON. Reverted the
CLI default to JSON; bundle is now opt-in via --bundle. The flag
docstring notes that TASK-885 will flip the default once import
handles bundles.
* fix(attachments): surface tar/gzip close errors and truncation per Codex (round 2)
Codex round 2 finding: deferred tw.Close() / gzw.Close() ignored
errors. If a backend returned fewer bytes than size_bytes claimed,
io.Copy returned nil, the tar writer's "missed N bytes" trip fired
at Close, and the handler still completed a 200 OK with a corrupt
bundle that gunzip would later refuse to decompress — silently from
the operator's perspective.
Two changes:
1. The deferred close now logs both tw.Close() and gzw.Close()
errors with structured context, so a corruption-on-finalize
trip shows up in the operator log.
2. streamAttachmentToTar checks the bytes-copied count against
a.SizeBytes after io.Copy and returns a per-attachment error
when they disagree. The error is logged with attachment_id +
storage_key so an operator can correlate the corruption with
the row to investigate.
Regression test: TestExportBundle_TruncatedBlobLogsError forces a
size_bytes/blob desync via direct UPDATE and asserts the resulting
bundle bytes don't decode cleanly. (HTTP status stays 200 because
headers are already on the wire by the time we detect the desync;
that's an inherent limitation of mid-stream errors, but the new
logs + close-error surfacing make the failure observable.)
* fix(attachments): X-Bundle-Status trailer for export-stream success per Codex (round 3)
Codex P1 round 3: even with the per-blob truncation log + tar/gzip
close-error logs, mid-stream failures looked successful to clients.
The CLI's RawStream finished without a transport error, the file
landed on disk, and "Exported workspace" printed regardless of
whether the bundle was actually complete.
Two complementary signals now mark a clean stream:
1. HTTP trailer X-Bundle-Status. The handler declares the trailer
in the initial Trailer header and sets it to "ok" only after
tw.Close() and gzw.Close() both return without error. CLI checks
the trailer after streaming and discards the file + returns
error if it's absent or non-"ok".
2. The handler skips the deferred clean close on the error path,
leaving the gzip footer unwritten. A client that ignores the
trailer (curl, third-party tooling) still sees a corrupt gzip
stream that gunzip refuses to decompress.
CLI: pad workspace export --bundle now removes any partial output
file on failure rather than leaving a corrupt one behind.
Client.RawStream signature changed to return (bytes, *http.Response,
error) so callers can inspect resp.Trailer; the only caller is the
export command.
Tests: TestExportBundle_TruncatedBlobAbortsStream now asserts both
signals (trailer absent + gzip/tar can't fully decode), and
TestExportBundle_SuccessTrailer pins the happy-path trailer.
|
||
|
xarmian
|
504d348917 |
feat(attachments): Settings → Storage tab with attachment list (TASK-882) (#303)
* feat(attachments): Settings → Storage tab with attachment list (TASK-882)
Adds the Settings → Storage tab and the underlying list/delete API
endpoints so workspace owners can audit and reclaim attachment bytes.
Backend (TASK-882 needs this — there was no list/delete API yet):
- store.WorkspaceAttachments: paginated list with filter (category,
attached/unattached, collection_id) + sort allowlist (size, filename,
created_at — each with desc variant). LEFT JOIN to items + collections
enriches each row with item_title/slug + collection_slug for the
"in [[Item]]" link. Hides derived (thumbnail) rows by default — they
count toward quota but are managed automatically and would clutter
the user-facing list.
- store.SoftDeleteAttachment: tombstones the row + every variant. Blob
on disk stays put; orphan GC reclaims past the grace period (TASK-886).
- GET /workspaces/{ws}/attachments — viewer+, returns
{attachments, total, limit, offset}.
- DELETE /workspaces/{ws}/attachments/{id} — editor+. Refuses to delete
derived rows directly (returns 400 with derived_attachment code) and
invalidates the storage-usage cache.
Frontend:
- StorageTab.svelte component (lib/components/settings) with usage bar
(color thresholds at 80%/100%, override badge), 5-select filter row
(category, item, collection, sort, page size), attachment list with
thumbnails (image variants via thumb-sm, emoji icon otherwise), item
link, MIME, size, date, and per-row delete with confirm() dialog.
Pagination footer with Prev/Next + "showing X–Y of Z".
- TS api.attachments.list() / delete() + types.
- Wired as a new "Storage" tab on the workspace settings page.
Tests:
- TestListAttachments_Pagination: 3 uploads, default + size-asc sort,
limit/offset paging.
- TestListAttachments_HidesDerived: synthetic thumbnail row, asserts
the list excludes parent_id != NULL rows.
- TestDeleteAttachment_HappyPath: upload → delete → list empty →
storage usage drops to 0 (cache invalidation hook fires) → second
delete returns 404.
- TestDeleteAttachment_DerivedRefused: thumbnail rows can't be deleted
directly via the API.
Parent: PLAN-866.
* fix(attachments): collection visibility + category gaps + item ref shape per Codex (round 1)
Three findings from Codex on PR #303 round 1:
P1 — Collection visibility leak. The storage list returned all
workspace attachments without applying per-user collection access,
so a member with collection_access=specific would receive hidden
collections' attachment IDs/filenames/item titles and could then
pull the bytes via the existing download endpoint.
Fixed by threading visibleCollectionIDs(r, workspaceID) through to
the store filter. nil = admin/no restriction; empty slice = zero
visible collections (zero rows by design); explicit set = restrict
i.collection_id IN (...). Orphans (item_id IS NULL) are excluded
for restricted users since their filenames would still leak.
P2 — item_ref shape didn't match the route. The store synthesized
"<collection_slug>/<item_number>" and the UI inserted it verbatim
into the URL, producing /user/ws/tasks/tasks/5. Dropped item_ref
entirely; UI now builds URLs from item_slug + collection_slug
which is the actual route shape.
P2 — Category filter coverage. mimePrefixForCategory only handled
image/video/audio. Selecting Documents/Text/Archive/Other in the
UI silently passed through with no MIME predicate so the list
showed everything. Replaced with mimePredicateForCategory which
emits the right SQL fragment per bucket: prefix LIKE for the type/
buckets, explicit IN list for document/text/archive (mirroring the
allowlist in internal/attachments/mime.go), and a NOT-IN composite
for "other".
Tests:
- TestWorkspaceAttachments_VisibilityFilter: admin sees all 3 rows;
restricted to one collection sees only that collection's row +
orphan suppressed; empty visibility yields zero rows.
- TestWorkspaceAttachments_CategoryFilters: image/document/text/
archive/other each return exactly the matching MIME types.
* fix(attachments): item-level visibility on list + delete per Codex (round 2)
Two more findings from Codex on PR #303 round 2:
1. The list filter used VisibleCollectionIDs alone — but that set
includes collections containing any item-level grant for the user.
A guest with one item granted in collection B would still receive
attachment metadata for every item in collection B. Replaced with
the (fullCollIDs, grantedItemIDs) tuple from guestResourceFilter so
the SQL ORs collection-level full access against per-item grants,
matching how handlers_search / handlers_activity narrow lists.
2. The delete endpoint validated workspace membership but never
checked the attachment's parent item is visible to the caller.
An editor with restricted collection access could delete
attachments in hidden collections by guessing/obtaining the
attachment ID. Added requireItemVisible after fetching the parent
item, plus a fallback gate for orphan attachments (item_id IS
NULL) so restricted users get 404 there as well.
Store-level filter renamed: VisibleCollectionIDs → Restricted +
FullCollectionIDs + GrantedItemIDs. Tests cover the collection-only,
item-grant-only, and zero-visibility paths.
* fix(attachments): allow deleting attachments when parent item is soft-deleted (round 3)
Codex P2 from PR #303 round 3: the storage list intentionally surfaces
attachments whose parent item has been soft-deleted (so the user sees
what's still consuming quota), but the delete handler used GetItem,
which filters soft-deleted out and returned 404 before
SoftDeleteAttachment could run — turning every Delete button on those
rows into a no-op.
Fixed by adding store.GetItemIncludeDeleted (mirroring the existing
GetItemBySlugIncludeDeleted) and switching the delete path to use it.
The visibility check still keys off the (still-set) collection_id, so
soft-deleting an item doesn't escalate access — restricted users still
hit requireItemVisible's 404 if they couldn't see the parent.
Regression test: create item → attach → soft-delete item → list still
returns the row → delete returns 204.
* fix(attachments): list surfaces attachments under soft-deleted parents (round 4)
Codex round-4 finding: WorkspaceAttachments still LEFT JOIN'd items
with AND i.deleted_at IS NULL, so attachments whose parent item was
soft-deleted disappeared from the list — even though the previous
round wired GetItemIncludeDeleted on the delete path. Net effect:
restricted editors with access to that collection couldn't discover
the row in the UI; only full-access users saw it as an orphan-looking
entry.
Fix: drop the deleted_at filter from the JOIN. The collection ACL
predicate (i.collection_id IN ...) now sees the (still-set)
collection_id from the soft-deleted item, so visibility behaves
consistently for live and tombstoned parents. Soft-deleted items
don't escalate access — the collection_id stays put.
UX: response now carries item_deleted=true when the parent is
soft-deleted; the StorageTab renders the title with strike-through
+ a small "deleted" badge instead of a clickable link (which would
404).
Tests:
- store-level: admin/full-access sees the row + ItemDeleted flag,
restricted-to-correct-collection sees it, restricted-to-other-
collection does not.
- (existing TestDeleteAttachment_AfterParentSoftDeleted continues
to pass on the handler side.)
|
||
|
xarmian
|
335762c2bf |
feat(attachments): storage usage API + effective-limit computation (TASK-881) (#302)
* feat(attachments): storage usage API + effective-limit computation (TASK-881)
Adds GET /api/v1/workspaces/{ws}/storage/usage returning
{used_bytes, limit_bytes, plan, override_active}. Resolves the effective
limit through the existing three-tier chain (per-user override → platform
setting → hardcoded plan default) and surfaces the override flag for the
upcoming Settings → Storage and admin user-detail UIs.
Implementation:
- store.WorkspaceStorageInfo consolidates SUM(size_bytes) + owner-plan
resolution in one call; WorkspaceStorageLimit is now a thin wrapper so
the upload-time quota check and the API path stay consistent.
- Server.storageInfoCache is a 30s TTL memoizer to absorb repeated
Settings → Storage page loads. Invalidation hooks fire on upload,
thumbnail derivation, and transform — the ~30s eventual-consistency
window is bounded by TTL only when invalidation isn't reachable.
- Defensive copy on cache read so a caller mutating the returned struct
can't poison subsequent reads.
- New CLI command `pad workspace storage` prints "X used of Y (Z%)" with
IEC units (humanBytes helper) and surfaces the override flag.
- TS api.attachments.storageUsage() + WorkspaceStorageInfo type ready
for TASK-882's Settings → Storage page consumer.
Tests:
- Store-level: no-owner fallback, free-plan resolution chain, override
flip, pro-plan override-active visibility, soft-delete exclusion.
- Server-level: empty-workspace happy path, two uploads with cache
invalidation between, dedicated cache TTL/invalidate/copy-safety test.
Parent: PLAN-866.
* fix(attachments): gate storage usage on viewer+ per Codex review (round 1)
Codex correctly flagged that the storage/usage handler relied solely on
RequireWorkspaceAccess, which admits item-grant guests with
workspaceRole=="guest". Workspace-wide quota numbers (used_bytes, plan,
override status) shouldn't surface to guests — every other workspace-
level read handler uses requireMinRole("viewer") for exactly this case.
Adds the explicit gate + a regression test that calls the handler with
a guest-role context and asserts 403.
|
||
|
xarmian
|
00baf75576 |
feat(attachments): download/serve API with auth + Range support (TASK-872) (#289)
Adds the GET endpoint that pairs with TASK-871's upload. Streams the
blob from the resolved storage backend with proper headers, Range
support, and cross-workspace defense.
GET /api/v1/workspaces/{slug}/attachments/{attachmentID}
Optional ?variant=thumb-sm|thumb-md
- 200 inline render for images / video / audio / PDF / etc.
- 200 attachment download for HTML / JS / forced-download MIMEs
- 206 Partial Content on Range requests (video/audio seek)
- 304 Not Modified on conditional GETs (If-Modified-Since etc.)
- 400 unknown variant
- 404 missing attachment OR cross-workspace probe (not 403, to avoid
leaking existence of attachments in other workspaces)
- 404 blob_missing if DB row exists but on-disk blob is gone (logs a
warning since this is a "shouldn't happen" state)
- 503 if attachments registry not configured
internal/server/handlers_attachments.go
handleGetAttachment looks up the row, gates cross-workspace via 404,
optionally swaps to a derived variant via GetAttachmentVariant
(silent fallback to original when the variant row doesn't exist
yet — TASK-878 will populate them; this handler shipping today
doesn't have to wait), resolves the storage backend via Registry,
and hands off to http.ServeContent when the body satisfies
io.ReadSeeker. FSStore returns *os.File so that's the common path
and gets us Range / 206 / conditional GETs for free. Backends
without Seek (a future S3 streaming reader) fall through to a
plain io.Copy with no Range support — the contract is "Range works
when the backend supports it, never breaks correctness".
Headers:
Content-Type from att.MimeType (already canonical post-allowlist)
Content-Disposition: inline | attachment, filename sanitized to
strip quotes/backslashes/control bytes (header-injection defense
on top of the upload-time basenaming)
Cache-Control: private, max-age=3600 (Phase 3 revisits for CDN)
X-Content-Type-Options: nosniff (browser should never re-sniff;
we already validated MIME at upload)
Upload response now includes "url" again — TASK-871 had dropped it
because the GET handler didn't exist yet. Slug-form path matches
every other API endpoint.
internal/store/attachments.go
GetAttachmentVariant(parentID, variant) for the ?variant lookup.
internal/server/server.go
GET /workspaces/{slug}/attachments/{attachmentID} wired alongside
the existing POST.
Tests
Happy-path PNG, HTML force-download, 404 missing, cross-workspace
404 (NOT 403), Range 206 with bytes 10-29 of an MP4 payload,
variant fallback to original, unknown variant rejected, derived
thumb-sm row honored when present, blob-missing 404, and the
filename sanitizer table.
Verification
go build ./... — clean
go vet ./... — clean
go test ./... — all packages pass
make install — server restarts on the new binary
Parent: PLAN-866.
|
||
|
xarmian
|
48b9e18d34 |
feat(attachments): upload API with MIME sniff + dedupe + quota tracking (TASK-871) (#288)
* feat(attachments): upload API with MIME sniff + dedupe + quota tracking (TASK-871)
Wires the upload endpoint that turns a multipart POST into an
attachments row plus a stored blob. Auth-gated (editor+), per-file
size cap, hash-streaming, MIME allowlist with extension blocklist,
fire-and-forget quota warning.
POST /api/v1/workspaces/{slug}/attachments
Multipart "file" field. Optional ?item_id=… or form item_id to
associate at upload time. Returns
{id, url, mime, size, width?, height?, filename, category, render_mode}.
Errors: 400 bad multipart, 400 empty file, 401 unauthorized, 403
insufficient role, 413 over per-file cap, 415 MIME or extension
rejection, 503 attachments not configured.
internal/attachments/mime.go
MIMEEntry + RenderMode + Category typed allowlist mirroring DOC-865.
Default-deny. SniffMIME wraps http.DetectContentType. ValidateUpload
cross-checks the sniff result against the filename extension and:
(a) rejects when the extension maps to a *blocked* MIME — covers
.svg (sniffs as text/xml; .svg ext makes the browser run embedded
<script>) and .exe family (sniffs vary; extension is unambiguous);
(b) rejects when the extension maps to an allowed MIME but the
sniff's category disagrees — the "exe pretending to be png" case.
Tests cover normalize/lookup/sniff plus happy path, exe-as-png,
extension mismatch, SVG, .exe-by-extension-alone, text/plain accept,
HTML force-download.
internal/store/attachments.go
CreateAttachment / GetAttachment / WorkspaceStorageUsage. Pointer
scan for nullables; SUM(size_bytes) excludes soft-deleted rows but
includes derived blobs (thumbnails are real bytes on disk).
internal/server/handlers_attachments.go
Body capped via http.MaxBytesReader BEFORE ParseMultipartForm spools
any of it. Streams "file" part into an os.CreateTemp file, sha256ing
in one io.MultiWriter pass — multi-GB POST never reaches RAM. Sniff
on first 512 bytes; image dimension probe via stdlib image.DecodeConfig
(PNG/JPEG/GIF). WebP/AVIF/HEIC accepted but width/height nil — matches
the "pure-Go gracefully degrades" decision in DOC-865. Calls
AttachmentStore.Put (which hash-verifies via the dedup fast path) and
inserts the row. Quota check (CheckLimit + WorkspaceStorageUsage) runs
in a goroutine — Phase 1 logs only; Phase 2 will enforce.
Anonymous uploads on a fresh install (RequireWorkspaceAccess grants
implicit owner without a current user) get uploaded_by="system".
internal/server/server.go
Server.attachments + attachmentMaxBytes fields and SetAttachments
setter. Route POST /workspaces/{slug}/attachments wired inside the
authenticated workspace block.
cmd/pad/main.go
Boot wiring: NewFSStore(<DataDir>/attachments) → Registry registered
under "fs" → SetAttachments. PAD_ATTACHMENT_MAX_BYTES env override
for the per-file cap.
Tests
internal/server/handlers_attachments_test.go covers:
happy path PNG (1x1, dimensions resolve to 1×1)
exe bytes with .png filename → 415
PNG bytes with .pdf filename → 415 (extension mismatch)
empty body → 400
missing file part → 400
over the size cap → 413
same content uploaded twice → two rows, same content_hash + storage_key,
WorkspaceStorageUsage = 2 × bytes (dedupe is at the blob layer,
not the row layer)
8 concurrent uploads of identical bytes → all 201, no corruption
no registry wired → 503
Verification
go build ./... — clean
go vet ./... — clean
go test ./... — all packages pass
make install — server restarts on the new binary
Parent: PLAN-866.
* fix(attachments): three Codex round-1 findings — drop premature url, accept Office docs, real quota probe
1. Upload response no longer returns "url". TASK-872 wires GET so any
URL we return today is a 404 — pulling it out keeps clients from
baking in the broken endpoint.
2. Office Open XML docs (.docx/.xlsx/.pptx) and OpenDocument formats
(.odt/.ods/.odp) are zipped XML — http.DetectContentType correctly
sniffs them as application/zip. Previously the validator's
extension-vs-sniff category check rejected them as
"mime_extension_mismatch" (archive vs document). Now: when the
sniffed type is exactly application/zip and the extension maps to
a document MIME, trust the extension and route to the document
entry. Plain .zip with the same bytes still routes to archive.
Test covers all six office/odf extensions plus the plain-zip case.
3. CheckLimit("storage_bytes") returned "unknown workspace feature"
because featureCount only knows row-counted features (items,
members, webhooks). The warning path silently dropped every probe.
Added Store.WorkspaceStorageLimit which does the same three-tier
resolution (user override → platform setting → hardcoded fallback)
but returns the limit only — usage is computed separately via the
existing WorkspaceStorageUsage. Self-hosted/pro plans return -1
(unlimited). Workspaces without an owner_id (fresh installs and
legacy rows) also return -1, so a fresh-install upload no longer
logs "owner not found". Switched maybeWarnStorageQuota to use
WorkspaceStorageLimit + WorkspaceStorageUsage directly. Now also
spawned via Server.goAsync so Stop() drains it (BUG-842 hygiene).
Tests
- TestValidateUpload_AcceptsOfficeOpenXMLAsZipBytes covers all six
extensions + plain .zip
- TestUpload_QuotaCheckResolves regression-tests finding 3: both
storage helpers return non-error after a real upload
- TestUpload_HappyPathPNG asserts the response no longer carries url
Verification
go build ./... — clean
go vet ./... — clean
go test ./... — all packages pass
* fix(attachments): trim trailing blank line at EOF in mime.go per Codex review (round 2)
Round 2 LOW: git diff --check flagged a "new blank line at EOF" on
internal/attachments/mime.go. Cosmetic but addressed because the
ship-tasks workflow requires zero findings (HIGH/MEDIUM/LOW alike) —
leaving LOWs unfixed compounds across PRs and prevents the loop from
ever converging clean on later work.
* fix(attachments): alias stdlib MIME-sniff quirks per Codex review (round 3)
http.DetectContentType returns names that don't match modern IANA
conventions for two formats on the allowlist:
audio/wave → audio/wav (.wav uploads)
application/x-gzip → application/gzip (.gz uploads)
Without aliasing, valid uploads of either format hit "mime_not_allowed"
because the allowlist uses canonical names. Added a sniffAliases map
applied inside SniffMIME so allowlist lookups always see the canonical
form. Allowlist stays single-sourced; the fix is one map entry per
quirk we discover.
Tests:
- TestSniffMIME_AliasesStdlibQuirks pins both aliases at the sniff layer
- TestValidateUpload_AcceptsWAV / TestValidateUpload_AcceptsGzip verify
the end-to-end accept path with real WAV (RIFF/WAVE) and gzip headers
|
||
|
xarmian
|
6461aafd16 |
feat(store): attachments table + Attachment model (TASK-869) (#286)
Adds the schema groundwork for inline images and file uploads — see DOC-865 (Attachments — architecture & migration design). - migrations/047_attachments.sql — SQLite migration. Table + 4 indexes (workspace, item, hash, parent). Partial indexes on workspace/item/parent match the items table convention. The hash index is full (not partial) so dedupe can resurrect a soft-deleted blob if the same bytes are re-uploaded without writing a duplicate. - pgmigrations/026_attachments.sql — Postgres mirror with BIGINT for size_bytes; same partial-index pattern. - internal/models/attachment.go — Go model with all columns. Uses pointer types for nullable columns (item_id, width, height, parent_id, variant, deleted_at) so JSON omitempty works correctly. No call sites yet — purely schema groundwork. Verified the migration runs cleanly on a fresh install and on the live dev DB. Parent: PLAN-866. |
||
|
xarmian
|
e5eae5e94e |
feat(web): persistent dismissible CLI-connect banner on workspace pages (TASK-862) (#284)
* feat(web): persistent dismissible CLI-connect banner on workspace pages (TASK-862)
Final web piece of the web-first onboarding on-ramp from PLAN-859 / IDEA-750.
A slim banner now nudges users to connect their workspace to the CLI on
every workspace page, until they either dismiss it or actually do it.
Server:
- New store method WorkspaceHasCLISource(workspaceID) — backed by
EXISTS(... WHERE source='cli' AND deleted_at IS NULL), so it's a
cheap O(1) check that short-circuits on the first match.
- Dashboard payload (GET /workspaces/{ws}/dashboard) gains
HasCLISource bool (json: has_cli_source).
- Unit tests cover empty workspace, web/skill items don't trip it,
one cli item flips it on, soft-delete flips it back off, and
cross-workspace isolation.
Web:
- New <ConnectBanner> Svelte 5 component
(web/src/lib/components/ConnectBanner.svelte). Self-contained:
reads dismissed state from localStorage, fetches has_cli_source
itself, mounts <ConnectWorkspaceModal> internally. Two split
$effect blocks per CONVE-606 — one for the localStorage sync, one
for the dashboard fetch — so a workspace change doesn't entangle
the two reactive lifecycles.
- Banner is hidden while loading (hasCliSource === null) to avoid a
flash-then-auto-hide on workspaces that already have CLI items.
- Storage key pad-cli-banner-dismissed-${wsSlug} matches the existing
onboarding-dismissed pattern. Per-browser only; TODO comment in
source about backing it with a workspace_user_state row if cross-
device persistence is wanted later.
- Mounted in web/src/routes/[username]/[workspace]/+layout.svelte
above {@render children()} so it appears on every workspace page
(dashboard, collection lists, item detail, search, activity, etc.)
and NOT on console/auth pages (the layout is workspace-scoped).
- DashboardResponse type in web/src/lib/types/index.ts gains
has_cli_source: boolean.
Smoke-tested against the running server: the field is live in the
dashboard payload and reflects reality (this workspace returns
has_cli_source: true since it has many CLI-sourced items, so the
banner is correctly auto-hidden here).
Test plan:
- go build ./... && go test ./... — all green (incl. new
TestWorkspaceHasCLISource with 5 sub-cases).
- cd web && npm run build — clean.
- make install — clean, server restarted.
- Svelte MCP autofixer ran on ConnectBanner.svelte — no issues.
Parent: PLAN-859. Driving idea: IDEA-750.
* fix(web/connect-banner): stale-response guard + refetch on modal close (Codex round 1)
Two findings from Codex review on PR #284:
1. Stale-response race: rapid workspace switches could let a slow
dashboard fetch from workspace A overwrite hasCliSource for
workspace B after the user navigated. Capture the requested slug
at fetch time, ignore the response if wsSlug has changed since.
2. Auto-hide didn't work in-session: if a user opened the banner
modal, copied the command, ran it elsewhere, and closed the modal,
the banner stayed visible because hasCliSource was stale. Refetch
when the modal transitions from open → closed (the natural moment
the user has just connected). Uses $effect.pre with a tracked
previous value, matching the transition pattern in ShareDialog.
The 'someone ran the CLI from another terminal without ever opening
the modal' edge case is left for a follow-up — would require SSE
item-created subscription, which is heavier than this PR's scope.
* fix(server/items): persist source from auth context on create (Codex round 2)
Codex caught an architectural bug while reviewing the TASK-862 banner
work: items created via the CLI were persisting with source='web'
(the column default) instead of 'cli', because handleCreateItem decoded
ItemCreate from the body — which the CLI doesn't set Source on — and
only consulted actorFromRequest AFTER persisting (for SSE / activity
log emission). Result: TASK-862's has_cli_source dashboard signal
would never flip on for normal CLI usage, so the connect-CLI banner
would never auto-hide for users who actually wired up the CLI.
Fix: in handleCreateItem, backfill input.Source from actorFromRequest
before calling store.CreateItem, but only when the client didn't
explicitly set it (so agents marking themselves as 'skill' still
pass through unchanged).
Test: TestCreateItemSourcePersistedFromAuth covers all three branches
- bearer Authorization header → source=cli (uses bootstrap + a real
session token in the header since the auth middleware validates
token format and rejects fake values with 401 before the handler
runs)
- cookie session, no Authorization → source=web
- explicit source in body wins over auth-derived (e.g. 'skill')
* fix(web/connect-banner): seq counter for same-workspace race (Codex round 3)
Round 3 caught a same-workspace race the slug guard didn't cover: an
in-flight workspace-change fetch that resolves AFTER the modal-close
refetch could overwrite the newer 'true' with the older 'false',
making the banner reappear after the user actually wired up the CLI.
Add a monotonic fetchSeq counter — captured at call time, rechecked
before applying the response. Only the LATEST request's result wins,
regardless of arrival order. The slug guard stays as a second-layer
defense for cross-workspace races.
* fix(web/connect-banner): guard banner keydown to currentTarget (Codex round 4)
Round 4 caught a keyboard-event bubble: pressing Enter or Space on
the dismiss X button also fired the banner-level keydown handler,
so the user would dismiss AND open the modal in one stroke.
Guard the parent handler with `e.target !== e.currentTarget` so it
only reacts to keydown that originated on the banner itself. Tabbing
to the dismiss button + Enter now ONLY dismisses.
* fix(store): visibility-filter has_cli_source query (Codex round 5)
Round 5 caught a P2 information leak: WorkspaceHasCLISource scanned
the entire workspace regardless of caller visibility, so a guest
with grants only on web-sourced items could still see has_cli_source
return true (revealing that CLI items exist somewhere they can't see).
That also produced wrong UX — the banner could auto-hide for guests
who couldn't actually use the CLI.
Extend the query to take optional collectionIDs/itemIDs filters
matching the dashboard's existing visibility model: an item counts
when its collection is in collectionIDs OR its id is in itemIDs
(union — guest item-level grants can expose items in otherwise-
hidden collections). Mirrors ListItems' filtering pattern incl. the
"non-nil empty CollectionIDs = no visibility = short-circuit false"
semantics.
Handler now passes dashCollIDs and dashItemIDs to match the rest of
the dashboard payload's filtering. New TestWorkspaceHasCLISourceVisibility
covers the four cases: unfiltered sees all, visible-coll-only hides
CLI items in hidden collections, item-level grant surfaces a hidden
CLI item, and empty visibility short-circuits to false.
|
||
|
xarmian
|
53b5add4e9 |
fix(store): bump SQLite busy_timeout from 5s to 30s (BUG-853) (#277)
TestSQLiteConcurrentWritersNoBusy intermittently fails on the GitHub- hosted Go (SQLite) CI job with `database is locked (5) (SQLITE_BUSY)` under 25 concurrent writers × 5 ops. The test asserts ZERO errors so that BUG-748's `_txlock=immediate` regression stays pinned — but with the DSN's busy_timeout at 5s, the unluckiest writer on a slow shared runner can exceed the timeout: 125 serialized inserts under heavy contention from sibling test packages add up. Bumping busy_timeout to 30s gives a 6× margin over the worst observed CI run and ~50× the normal local p95. Genuine deadlocks don't happen with WAL + BEGIN IMMEDIATE, so the only thing the higher value costs is "how long we wait before declaring lock contention pathological". For Pad's workload, 30s is fine. Surfaced once BUG-851 (PR #276) cleared the rate-limiter goroutine leak that had been masking everything else on main. Verified locally: $ go test -count=20 -run TestSQLiteConcurrentWritersNoBusy \ ./internal/store/ ok github.com/PerpetualSoftware/pad/internal/store 6.102s Note: this is a single-character DSN change plus a doc-comment update; no code path or contract is altered. Read concurrency (WAL) is unchanged — we're not touching SetMaxOpenConns. |
||
|
xarmian
|
0fd5d0cdfb |
fix: green up Go (PostgreSQL) CI (BUG-842) (#275)
* fix(store): swap plainto_tsquery → websearch_to_tsquery for PG FTS (BUG-842)
`TestListItems_FTS_HyphenatedSearchTerm/task-five` has been failing on
every Go (PostgreSQL) CI run because `plainto_tsquery('english',
'task-five')` doesn't match the asciihword lexeme(s) the english parser
produces for an indexed `task-five-distinctive`. The result is that
every PG full-text search for hyphenated terms returns zero rows.
`websearch_to_tsquery` (Postgres 11+) is purpose-built for arbitrary
user input and tokenizes hyphenated terms the same way `to_tsvector`
does for the indexed document, so the query intersects the index
correctly. Swapped in three spots in the postgres dialect — FTSMatch,
FTSSnippet, FTSRank — and updated the caller-side comments that
referenced plainto_tsquery. SQLite path is unchanged: it goes through
items_fts MATCH with sanitizeFTSQuery, never through these methods.
* fix(server): drain background goroutines on Stop() (BUG-842)
`TestAdminBillingStats_SidecarSidecarError_DegradesToLocalOnly` (and
other server tests) have been flaking on the Go (PostgreSQL) CI runner
with `TempDir RemoveAll cleanup: directory not empty`. Root cause:
several request handlers spawned bare `go func() { ... }()` goroutines
that touched the SQLite WAL DB after the test function returned.
testServer's t.Cleanup closed the store but had no way to drain those
goroutines first, so a fire-and-forget WAL write could re-create the
`-wal`/`-shm` files between Close() and t.TempDir's RemoveAll.
Add a Server.bg sync.WaitGroup, a Server.goAsync helper that wraps a
WaitGroup-tracked goroutine, and a Server.Stop() that blocks until
every goAsync closure has finished. Convert the four known
fire-and-forget sites to goAsync:
- middleware_auth.go (TouchUserActivity)
- handlers_auth.go (password reset email)
- handlers_cloud.go (stripe_processed_events pruning)
- handlers_members.go (workspace invitation email)
Wire `srv.Stop()` into both testServer (server_test.go) and
newMetricsTestServer (metrics_auth_test.go) so cleanup order is
Stop → Close → TempDir RemoveAll. Add
TestServer_Stop_DrainsBackgroundGoroutines to pin the contract: a
goAsync goroutine must block Stop until it returns.
* fix(store): correct PG FTS hyphenation via OR-combined plainto_tsquery (BUG-842)
The previous attempt swapped plainto_tsquery → websearch_to_tsquery,
which was wrong: websearch_to_tsquery treats `-` as a NEGATION operator
(Google-style), so `task-five` becomes `task & !five` and the search
returns 0 rows for the same reason as before. This commit reverts the
swap and applies the actual fix.
PG's english parser indexes `task-five-distinctive` as
`{task-five-distinct, task, five, distinct}` — the asciihword AND its
parts. plainto_tsquery applied to the partial query `task-five`
produces `task-fiv & task & five`: the stemmed asciihword for the
PARTIAL query (`task-fiv`) is NOT in the vector, so the AND fails.
Replacing the hyphen with a space makes plainto emit `task & five`,
which DOES match — but doing that unconditionally breaks `BUG-842`-
style queries: PG indexes the `-842` suffix as a negative-number
lexeme, so `plainto_tsquery('BUG-842')` matches via `-842`, while
`plainto_tsquery('BUG 842')` searches for `842` and misses.
The fix ORs the two query variants together so the search vector is
matched against either the raw user query OR its hyphen-as-space form.
Both `task-five` (against `task-five-distinctive`) and `BUG-842`
(against `BUG-842 fix the cleanup race`) hit. Verified locally against
postgres:17-alpine via PAD_TEST_POSTGRES_URL — both 10x stress and
race-detector runs are green.
Surfaces:
- dialect.go: FTSMatch / FTSSnippet / FTSRank now consume TWO
placeholders each in the PG dialect.
- items.go: listItemsFTS PG branch + SearchItems PG branch update
args to pass (raw, sanitized) for every PG `?` placeholder.
- search.go: SearchItems main / count / facets PG branches updated
likewise. New sanitizePGFTSQuery helper alongside sanitizeFTSQuery.
- documents.go: ListDocuments PG branch updated.
Tests:
- TestListItems_FTS_HyphenatedSearchTerm extended with a `BUG-842`
case to pin the OR-combined logic — naive hyphen-stripping would
silently regress this.
- New TestSanitizePGFTSQuery unit test.
* chore: gofmt 11 files with import-order issues (BUG-842 PR cleanup)
The Go (SQLite) CI job has been failing on `main` (and every PR built
against it) because golangci-lint flags 11 files whose third-party
imports are intermixed with internal imports — the import-grouping
rule that gofmt enforces. None of these were introduced by the
BUG-842 PR; they're pre-existing on main. The PR can't go green
without this cleanup, though, so it's bundled here.
Pure mechanical change — `gofmt -w <files>` only re-orders import
groups; no logic changes. Files touched:
cmd/pad/configure.go
cmd/pad/main.go
internal/cli/format.go
internal/server/handlers_admin_invitations.go
internal/server/handlers_admin_users.go
internal/server/handlers_grants.go
internal/server/handlers_share_links.go
internal/server/handlers_stars.go
internal/server/middleware_auth.go
internal/store/store.go
internal/store/store_test.go
After this commit `gofmt -l ./cmd ./internal` returns clean.
|
||
|
xarmian
|
7cda0d7896 |
feat: rebrand to Perpetual Software + new tagline (IDEA-832) (#273)
Migrates from xarmian/pad to PerpetualSoftware/pad across the entire
repo and updates the product subtitle to "Collaborate with your AI
agents".
Go module rename
- go.mod: github.com/xarmian/pad → github.com/PerpetualSoftware/pad
- All Go imports updated across cmd/pad, internal/{cli,server,store,
models,collections,items,events,metrics,webhooks} (~130 files)
- Test fixtures with the literal repo slug ("xarmian/pad" in JSON
shapes, SSH/HTTPS git URL strings, workspace_context fixtures)
also updated, including the secondary repo entry
(xarmian/pad-web → PerpetualSoftware/pad-web — pad-web was also
moved to the org per branch context)
Docs / config
- README badges, install instructions, brew tap, Docker image, source
build path, sponsor link (sponsor link kept as personal @xarmian)
- Subtitle: "Project management for developers and AI agents." →
"Collaborate with your AI agents." (README, manifests, web layout
meta, .goreleaser homebrew description)
- CONTRIBUTING.md, SECURITY.md, skills/INSTALL.md
- .goreleaser.yaml: homebrew_casks owner, GHCR image, release github
owner, cosign cert-identity regex, comments
- .github/workflows/release.yml: tap/release comments
- deploy/k8s/deployment.yaml: container image
- docs/deployment.md: clone URL
- web/static/{site.webmanifest,manifest.json}: description
- web/src/routes/+layout.svelte: meta description + og:description
Brew tap path is PerpetualSoftware/tap/pad (CamelCase, matches
GitHub user case). GHCR image is ghcr.io/perpetualsoftware/pad
(lowercased per GHCR's URL normalization). CODEOWNERS @xarmian and
FUNDING.yml github: xarmian intentionally retained — those are the
personal maintainer / sponsor account, separate from the org repo.
Verification: go build ./..., go test ./... (all pkgs pass), web
build, and make install all clean (TASK-844, TASK-845).
|
||
|
xarmian
|
afe721d202 |
feat(cli): add Cloud mode to pad init, drop Docker option (TASK-837, TASK-838) (#272)
Merging despite Go (PostgreSQL) red — those failures (TestListItems_FTS_HyphenatedSearchTerm/task-five + TestAdminBillingStats_SidecarSidecarError_DegradesToLocalOnly TempDir cleanup race) are pre-existing on main and tracked in BUG-842. Codex reviewed in 3 rounds (round 1 clean → round 2 found a real semantic bug → fix → round 3 clean). Tests, vet, and lint all green; remaining check failures are documented pre-existing. |
||
|
xarmian
|
8e067c19db |
feat(admin): add /api/v1/admin/billing-stats proxy + cloud client (TASK-827) (#266)
* feat(admin): add /api/v1/admin/billing-stats proxy + cloud client (TASK-827)
New admin endpoint that powers the upcoming Pad Cloud Billing dashboard:
GET /api/v1/admin/billing-stats merges Stripe-derived metrics from pad-cloud
(active subs, MRR, ARR, churn, 30-day cancellations) with locally-computed
aggregates from the users table (customers_by_plan, new_signups_30d in the
last 30 days for plan='pro').
Architecture (PLAN-825 Option B):
- pad-cloud (TASK-826, already merged) hosts the Stripe API access in one
place; this PR adds the reverse pad → pad-cloud client method.
- Existing internal/billing.CloudClient gains GetBillingMetrics(): GET on
/admin/metrics/billing with the X-Cloud-Secret header (the same secret
pad-cloud already validates inbound calls with).
- New CloudSidecar.GetBillingMetrics() interface method keeps the server
package free of HTTP/Stripe dependencies and lets tests inject fakes.
- Existing fakeSidecar in handlers_account_test.go grows a no-op stub so
the account-delete tests still satisfy the extended interface.
Degradation contract:
- The endpoint always returns 200. Two booleans tell the UI which fallback
to render: cloud_unreachable=true (sidecar errored or unwired) and
stripe_configured=false (sidecar reachable but no STRIPE_SECRET_KEY yet).
- requireCloudMode + requireAdmin gate the route. Self-host gets 404,
non-admin gets 403.
Web glue:
- Added AdminBillingStats type to web/src/lib/types/index.ts.
- Added api.admin.getBillingStats() to web/src/lib/api/client.ts.
The Billing tab and metric cards land in TASK-828.
Tests:
- Billing package: GetBillingMetrics happy path (verifies method, path,
X-Cloud-Secret header, Accept header), Stripe-not-configured pass-through,
non-200 → SidecarError, transport error stays bare, malformed JSON,
nil/unconfigured client guards.
- Server package: self-host 404, non-admin 403, admin happy path
(merges local + remote correctly, handles plan="" → "free", filters
new_signups_30d to plan='pro' AND created_at >30d ago), no-sidecar
degrades to local-only, transport error degrades, sidecar 5xx degrades,
stripe_configured=false propagates verbatim with cloud_unreachable=false.
Part of PLAN-825 (Pad Cloud Admin Billing Dashboard).
* fix(admin): address Codex review (round 1) on billing-stats proxy
- Replace handler-side ListUsers walk with store.CountBillingAggregates
(two scalar SQL queries: COUNT(*) GROUP BY plan + a single COUNT(*)
for new pro signups). Removes the per-row TOTP decrypt overhead that
ListUsers performs and bounds CPU/bandwidth as the user table grows.
- Fix misleading TS comment on AdminBillingStats: clarify that "fully
healthy" requires cloud_unreachable=false AND stripe_configured=true,
not "both flags false" as previously stated.
Adds TestCountBillingAggregates exercising empty store, mixed plans,
empty-plan → "free" bucketing, and the 30-day cutoff filter for new
pro signups.
* fix(store): GROUP BY normalised plan expression in CountBillingAggregates
Codex round 2 caught a real bug: SELECT projected the COALESCE'd plan but
GROUP BY operated on the raw `plan` column, so users with plan='' and
plan='free' produced two distinct result rows that both scanned as "free"
in Go — the second iteration overwrote the first in CustomersByPlan,
silently underreporting the free-tier count.
Fix: GROUP BY COALESCE(NULLIF(plan, ''), 'free') so the grouping matches
the projection. Test updated: insertWithPlanAndDate now seeds an explicit
'' plan alongside two explicit 'free' rows and asserts the aggregate
rolls them up to 3 — the previous test only used CreateUser which always
inserts the column default ('free') and never exercised the empty-string
path.
|
||
|
xarmian
|
c4b5a36330 |
feat: warn at startup when shipped FTS triggers are missing (TASK-824) (#265)
* feat(store): warn at startup when shipped FTS triggers are missing (TASK-824) Defensive follow-up to BUG-822, where the documents_* triggers had silently drifted off some production DBs and search was broken until a user noticed. The migration runner had no notion that the triggers should exist — the only invariant was "this migration ran without erroring," which is too weak when SQLite's table-rebuild path can leave auxiliary objects in a different state than the migration intended. Add a hardcoded list of expected FTS5 triggers (one row per trigger, naming the table it's attached to) and a one-shot validateFTSInvariants step at the end of Store.migrate(). Each missing trigger emits a structured slog.Warn that points the operator at the recovery migration (046). Choices: - SQLite-only. Postgres uses tsvector update functions in pgmigrations with a different invariant model. - Logging-only, no auto-repair. Auto-creating triggers here would mask legitimate future removals and obscure the source of truth (the migrations directory). The recovery path is a targeted migration like 046_restore_documents_fts_triggers.sql. - Non-fatal. A missing trigger doesn't block startup; the operator may have intentionally removed one and just not updated the list yet, and we'd rather warn loudly than refuse to boot. Tests: - TestStartupInvariants_AllFTSTriggersExist — fresh DB has all 9 expected triggers (forward-looking guard against future migrations that break one). - TestStartupInvariants_LogsOnMissingTrigger — drop a trigger, run validator, capture slog records, assert a warning naming the missing trigger was emitted. Manual verification on the production DB: - Clean DB (after migration 046): no warnings on startup. - After manually `DROP TRIGGER documents_ai`: server logs `level=WARN msg="FTS trigger missing — ..." trigger=documents_ai table=documents` immediately on startup. * test(store): address Codex review on TASK-824 — bidirectional drift + Record.Clone Two LOW findings from Codex's first pass: 1. recordCapturingHandler.Handle stored slog.Record values without cloning. Records have internal shared state; the documented pattern for retaining them is r.Clone() first. Test passed today only because nothing mutated the record after Handle, but the helper was relying on slog internals. 2. TestStartupInvariants_AllFTSTriggersExist only proved every entry in expectedFTSTriggers exists. It didn't catch the inverse: a future migration adding a new FTS-style trigger on items/comments/documents without also adding it to expectedFTSTriggers, leaving the new trigger off the invariant check forever. Add TestExpectedFTSTriggers_MatchesActual which queries sqlite_master for every trigger on items/comments/documents and asserts each is in the expected list. A new trigger that isn't tracked fails this test with a clear "update the list in store.go" message. If a future trigger on these tables is legitimately not FTS-related, the test failure points the developer at this guard and they can either add it to expectedFTSTriggers or extend the exclusion. |
||
|
xarmian
|
4608108acf |
fix: restore documents_fts triggers + rebuild index (BUG-822) (#264)
* fix(store): restore documents_fts triggers + rebuild index (BUG-822)
Some production DBs ended up missing the documents_ai/au/ad triggers,
even though migration 025 (which rebuilt the documents table for the
doc_type CHECK constraint change) was recorded as applied. Items_fts
and comments_fts triggers were unaffected — issue is isolated to the
documents table-rebuild path.
Without these triggers, INSERT INTO documents never propagates rows
into documents_fts, so newly-created documents are silently invisible
to search. Plain list views still surface them, masking the regression.
Migration 046 is idempotent and safe to apply on any DB:
1. DROP TRIGGER IF EXISTS for the three documents_* triggers — round-
trips for DBs that ran 025 cleanly, recovers DBs missing the
triggers.
2. CREATE TRIGGER for all three (matching the bodies in 001/025).
3. INSERT INTO documents_fts(documents_fts) VALUES ('rebuild') to
repopulate the FTS5 internal index from the current documents
table — recovers searchability for documents created while the
triggers were missing.
Postgres path uses a separate tsvector trigger function and is not
affected (only pgmigrations are applied there; this migration lives
in the SQLite migrations directory).
Tests:
- TestMigration046_DocumentsFTSTriggersExist — assert all three
documents_* triggers exist after migrations run.
- TestCreateDocument_IsSearchableImmediately — regression test for
the failure mode: create a doc, immediately search by a unique
title-keyword, assert it's findable.
Manual verification on the production DB:
- Triggers re-appeared after `make install` (migration 046 applied).
- POST /documents with title "BUG822verify distinctive" → immediately
findable via ?q=BUG822verify (returned 1 result, the new doc).
* test(store): pin the BUG-822 recovery path with a rebuild test
Codex review on the BUG-822 fix flagged that neither existing test would
fail if the `INSERT INTO documents_fts(documents_fts) VALUES('rebuild')`
step were removed from migration 046. The trigger-existence and
post-fix-search-works tests both pass on a clean migration run, but
they don't exercise the historical-recovery half of the migration —
the part that rescues already-broken DBs whose documents were inserted
while the triggers were missing.
Add TestMigration046_RebuildRecoversUnindexedDocs which:
1. Drops the documents_* triggers to simulate the broken state.
2. Inserts a document via the store path — won't reach FTS without
triggers.
3. Asserts the doc is invisible to ListDocuments (sanity-pinning the
broken state).
4. Runs just the rebuild step from migration 046.
5. Asserts the previously-unindexed doc is now searchable.
This locks in the recovery contract: removing the rebuild step from
046 will now make this test fail.
|
||
|
xarmian
|
dcf7c1d58e |
fix(store): apply Tag and Pinned filters in ListDocuments FTS branch (BUG-820) (#263)
The non-FTS path in ListDocuments applies Tag and Pinned filters (lines 31-42), but when params.Query is non-empty the FTS branch rebuilds query and args from scratch and only re-applies Type and Status — Tag and Pinned were silently dropped. Result: `/documents?q=foo&tag=urgent` returned all docs matching foo regardless of tag, similarly for pinned. Documents-side analog of BUG-812 (which fixed the equivalent issue on the items FTS path). Fix: mirror the Tag (s.dialect.JSONArrayContains on d.tags) and Pinned (d.pinned = TRUE/FALSE) filter blocks into the FTS branch after the existing Type/Status blocks. Backend-only — handlers and DocumentListParams already plumb both params through. Tests: - TestListDocuments_FTS_TagFilter — two docs match the search; only one has the tag; assert exactly the tagged one returned. - TestListDocuments_FTS_PinnedFilter — covers both pinned=true and pinned=false branches, asserting each narrows correctly. Manual verification: with two docs `BUG820scratch alpha` (tagged "urgent", pinned) and `BUG820scratch beta` (untagged, unpinned): - ?q=BUG820scratch → 2 docs - ?q=BUG820scratch&tag=urgent → 1 doc (alpha) - ?q=BUG820scratch&pinned=true → 1 doc (alpha) - ?q=BUG820scratch&pinned=false → 1 doc (beta) |
||
|
xarmian
|
068c208824 |
fix: sanitize SQLite FTS5 queries + whitespace guards (BUG-818) (#261)
* fix(store): sanitize FTS5 queries in listItemsFTS and SearchItems (BUG-818)
The sanitizeFTSQuery helper in internal/store/search.go wraps each
whitespace-delimited token in double quotes so SQLite FTS5 treats
specials (hyphens, AND/OR/NOT, parens) as literal characters rather
than boolean operators. Store.Search already used it; Store.listItemsFTS
and Store.SearchItems didn't, so any hyphen in `?search=` returned
HTTP 500 with "no such column: <suffix>" — including issue refs like
TASK-5, kebab-case slugs, dates, etc.
Apply sanitizeFTSQuery at the SQLite arg-binding sites in both unfixed
functions. Postgres branches stay unsanitized: plainto_tsquery accepts
arbitrary input safely (matches the existing pattern in Store.Search).
Tests:
- TestListItems_FTS_HyphenatedSearchTerm — exercises the listItems path
on multiple hyphenated queries via a table-driven sub-test.
- TestSearchItems_HyphenatedQuery — same regression on the SearchItems
path used by /api/v1/search.
- TestSanitizeFTSQuery — direct unit test covering empty, whitespace-
only, plain word, hyphenated phrase, multi-token, FTS5 boolean
operators (AND/OR/NOT), parens, embedded quotes (stripped),
surrounding whitespace, and unicode.
Manual verification: previously-500 queries now return 200 with results:
/items?search=match-me → HTTP 200, 2 items
/items?search=TASK-5 → HTTP 200, 8 items
/items?search=pad-cloud → HTTP 200, 103 items
* fix(store): address Codex review on PR for BUG-818
Codex review caught two extensions to the original BUG-818 fix:
1. MEDIUM — Store.ListDocuments (internal/store/documents.go) had the
same FTS5 boolean-parser vulnerability as Store.listItemsFTS and
Store.SearchItems before the original commit. Hyphenated /documents?q=
queries (e.g. ?q=release-notes-q2) returned HTTP 500 with "no such
column" the same way. Apply sanitizeFTSQuery in the SQLite branch;
leave Postgres unchanged.
2. LOW — Whitespace-only queries collapse to empty after FTS sanitization,
and SQLite FTS5 errors on `MATCH ''` with "syntax error near \"\"".
Add TrimSpace guards at the routing/entry points:
- listItems: route to FTS only if TrimSpace(Search) != ""
- SearchItems: short-circuit to empty results
- ListDocuments: same routing guard
- Store.Search: short-circuit to empty results
Tests:
- TestListDocuments_HyphenatedQuery — regression on the documents FTS path
- TestFTS_WhitespaceOnlyQuery_DoesNotCrash — covers all 3 entry points
(ListItems, SearchItems, ListDocuments) for spaces, tabs, mixed
whitespace
Manual verification (all 6 endpoints now HTTP 200):
- /workspaces/{ws}/items?search=task-five
- /workspaces/{ws}/items?search=<3 spaces>
- /workspaces/{ws}/documents?q=release-notes
- /workspaces/{ws}/documents?q=<3 spaces>
- /search?q=task-5
- /search?q=<3 spaces>
|