mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-22 10:33:27 +00:00
312cf06ce559fc4884100d1f0dcf1069ee79e910
350 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
7e37dfc34e |
refactor(store): collections.settings boundary normalization + scan revert (IDEA-1484 follow-up) (#563)
* refactor(store): drop defensive sql.NullString scans on collections.settings (IDEA-1484 follow-up) PR #562 (squash |
||
|
xarmian
|
0766d7ecf1 |
feat(store): enforce NOT NULL on collections.settings (IDEA-1484) (#562)
* feat(store): enforce NOT NULL on collections.settings (IDEA-1484)
Adds migration 055 (SQLite) and pg 034 (Postgres) to backfill any NULL
collections.settings rows to '{}' and then enforce NOT NULL DEFAULT '{}'
at the column level. Eliminates the bug class that BUG-1482 / PR #561
plugged defensively in the four reader sites.
SQLite uses the standard table-rebuild recipe (PRAGMA foreign_keys=OFF,
copy via COALESCE, RENAME, recreate the single dependent index from
032_permission_indexes.sql). Same PK values are preserved so FKs in
items, views, collection_access, and grants remain valid.
Postgres uses the simple in-place ALTER TABLE; SET DEFAULT is a no-op
belt-and-braces since 001_initial.sql:115 already had DEFAULT '{}'.
The defensive sql.NullString scans in collections.go / export.go and
the import-side ""→"{}" coercion in export.go remain in place — they
revert in a separate follow-up PR after this migration ships
everywhere.
Removes the four BUG-1482 NULL-only regression tests from
collections_test.go (their `UPDATE collections SET settings = NULL`
setup is now a hard write error against the new constraint and the
NULL-scan branch they guarded is no longer reachable). Reworks
TestExportImportRoundTripWithNullSettings into
TestExportImportRoundTripWithEmptyStringSettings — it now mutates
the exported bundle in-memory to carry the "" sentinel rather than
forcing a NULL row, still exercising the import-side coercion path
that survives this PR.
* test(store): cover collections.settings NOT NULL outcome (IDEA-1484)
Addresses Codex R1 P2: the migration test surface lacked direct
constraint-check coverage. Adds two focused outcome tests against the
post-migration schema:
- TestCollectionsSettingsNotNullEnforced — raw INSERT with settings=NULL
must fail. Error shape differs across SQLite (NOT NULL constraint
failed) and Postgres (SQLSTATE 23502); we only assert err != nil.
- TestCollectionsSettingsDefaultsToEmptyObject — raw INSERT omitting the
settings column entirely must materialize the column DEFAULT as the
Go string "{}" when read back via GetCollection. Same assertion on
both drivers; the defensive sql.NullString scan + Postgres JSONB
normalization both surface "{}".
Both tests reuse createTestWorkspace + the testStore harness, so they
run automatically on whichever driver the test invocation selects.
R1 P1 (migration runner atomicity) is out of scope per established
codebase precedent (022, 025 use the same pattern); will be filed as
a follow-up IDEA.
|
||
|
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
|
e621eacb9b |
feat(server): POST /api/v1/import/url endpoint + integration tests (TASK-1472) (#556)
Wires internal/urlimport into the API: Fetcher → Detect → converters.
Side-effect-free; the editor's "Insert from URL" modal owns any item
mutation (TASK-1474).
Endpoint:
- POST /api/v1/import/url, body {"url"}, response {markdown,
detected_type, title?, source_url, fetched_at, content_type}.
- Status mapping: 400 invalid URL / SSRF / malformed body; 502 upstream
failure (incl. size cap); 504 fetch timeout; 422 conversion failure.
- Swagger 2.0 fallback: detected as "openapi" by the sniffer but
rejected by ConvertOpenAPI → falls through to ConvertGeneric and
re-classifies the response as "generic" so the UI shows the right
affordance.
- 30s wall-clock budget on the whole pipeline via context.WithTimeout
(Fetcher's own timeout is the HTTP-level cap).
Package-level Fetcher is memoized via the existing sync.Once-cached
safe transport (shared keep-alive pool, no per-request leak). The
handler skips the pre-flight ValidateURL when the package fetcher
has AllowLocal=true so tests can swap in a loopback-friendly fetcher
without bypassing the production guard.
Integration tests (handlers_import_test.go):
- HTML happy path (httptest upstream, asserts detected_type/title/
source_url/fetched_at/content_type all wired up).
- OpenAPI 3.x happy path (inline YAML upstream, asserts ConvertOpenAPI
was invoked and detected_type=openapi).
- Swagger 2.0 fallback (asserts detected_type re-classified to
generic, markdown non-empty).
- SSRF rejection (default fetcher, loopback URL → 400 mentioning
"private"/"reserved").
- file:// scheme rejected (400).
- Missing/malformed body rejected (400).
- Upstream 5xx surfaces as 502.
- Size cap exceeded surfaces as 502.
- No-side-effects check: item count unchanged before/after import.
Parent: PLAN-1467.
|
||
|
xarmian
|
8771f95ab2 |
feat(urlimport): OpenAPI 3.x → Markdown converter (TASK-1471) (#555)
* feat(urlimport): OpenAPI 3.x → Markdown converter (TASK-1471)
Adds ConvertOpenAPI to internal/urlimport — the "openapi" branch of
the v1 importer. Built on pb33f/libopenapi.
Layout:
- H1 with the API title + version + description
- Contact + License lines
- Servers list
- Endpoints section grouped by primary tag (or "Other" for the
untagged). Per operation: `METHOD /path` heading, summary,
description, deprecation marker, operation ID, parameter table,
request-body summary (with media-type fences and YAML-rendered
example), and response code table.
- Schemas section with component schemas as Property/Type/Required/
Description tables, schema names sorted for stable output.
Scope:
- OpenAPI 3.x only. Swagger 2.0 detection returns an explicit
"only 3.x" error so the import endpoint (TASK-1472) can fall
through to the generic converter.
- Recoverable libopenapi build errors (unresolved refs, etc.) are
swallowed when the model is still produced — partial spec >
no output.
Tests:
- testdata/petstore-openapi.yaml — full v3 fixture: tags, params,
requestBody example, deprecated op, ref-typed schema array, two
component schemas with required-field markers.
- TestConvertOpenAPI_Petstore — 30+ markdown-substring assertions
on the rendered output.
- TestConvertOpenAPI_RejectsSwagger2 — explicit v2 error.
- TestConvertOpenAPI_RejectsGarbage — non-spec input.
- TestConvertOpenAPI_MinimalSpec — empty paths short-circuits.
- Helpers: schemaTypeBrief(nil), escapeTableCell, singleLine.
Dependency: github.com/pb33f/libopenapi v0.36.3 (MIT-licensed).
Parent: PLAN-1467.
* fix(urlimport): merge path-level + operation-level parameters per Codex review (round 1)
MEDIUM: OpenAPI path-item-level parameters apply to every operation
on the path. Previously only slot.op.Parameters was rendered, so
common specs that hoist a shared {id} parameter to the path-item
level emitted operations with the path parameter missing from the
docs.
Now opSlot carries item.Parameters as pathParams, and a new
mergeParameters helper produces the spec-conformant union:
- Path-level parameters first, in declared order.
- Operation-level parameters with matching (name, in) override the
path-level entry in place.
- Operation-only parameters appended after.
Tests:
- TestConvertOpenAPI_PathLevelParametersMerged — inline fixture
with a path-level widgetId + trace and an operation-level trace
override + fields op-only param. Asserts widgetId survives, trace
shows op-level (required=yes), no duplicate path-level trace row,
fields appears.
- TestMergeParameters_EmptyInputs — nil/nil short-circuit.
* fix(urlimport): no double-backticks on array-of-ref schema types per Codex review (round 2)
MEDIUM: schemaTypeBrief() previously wrapped refs in inline backticks
("`Pet`"). For array-of-ref schemas the brief became "array of `Pet`",
and the table-cell call site (codeOrBlank) then wrapped the entire
value in another pair, producing broken markdown like
"`array of `Pet``". Schema properties whose type is an array of a
component schema are a normal OpenAPI shape — `Litter.pets: array of
Pet` — so this would have hit real specs immediately.
Fixes:
- schemaTypeBrief now returns plain text — ref names without
surrounding backticks. Docstring updated to make the contract
explicit ("never contains backticks; caller wraps").
- codeOrBlank strips any stray backticks from input before wrapping
so the resulting cell always carries exactly one balanced pair.
Defensive: the contract from schemaTypeBrief is plain text now,
but stray backticks from any future caller can't corrupt the
table.
Tests added:
- TestConvertOpenAPI_ArrayOfRefTypeCell — inline spec with a
`Litter.pets: array of Pet` property. Asserts the type cell is
exactly `` `array of Pet` `` and no malformed variants leak.
- TestCodeOrBlank — 7-case table covering empty, plain, whitespace,
pre-backticked, embedded-backtick, and backtick-only inputs.
|
||
|
xarmian
|
d1560606cb |
feat(urlimport): generic HTML→Markdown converter (TASK-1470) (#553)
* feat(urlimport): generic HTML→Markdown converter (TASK-1470)
Adds ConvertGeneric to internal/urlimport — the v1 catch-all converter
for "non-OpenAPI" URLs. Pipeline:
1. go-shiori/go-readability strips chrome/nav/ads/scripts and returns
the page's primary article.
2. JohannesKaufmann/html-to-markdown/v2 converts the cleaned HTML to
markdown.
3. cleanupMarkdown normalizes line endings, trims trailing whitespace,
collapses blank-line runs, and ensures a single trailing newline.
Fallback path: when Readability cannot identify an article (directory
listings, single paragraphs, pages with no clear content container),
the converter falls back to a whole-body conversion so callers still
get usable markdown.
Dependencies (license-checked):
- github.com/JohannesKaufmann/html-to-markdown/v2 v2.5.1 (MIT)
- github.com/go-shiori/go-readability (Apache-2.0)
Fixtures + tests:
- testdata/availity-shape.html — Availity-style div soup with heavy
chrome (nav, ads, sidebar, footer, analytics script). Asserts the
article content survives and the chrome is stripped.
- testdata/mdn-shape.html — MDN-style semantic HTML (article/main +
proper heading levels + code fences). Asserts structure preserved.
- TestConvertGeneric_EmptyBody — empty-input rejection.
- TestConvertGeneric_PlainTextFallback — Readability-can't-find-article
fallback path.
- TestCleanupMarkdown — 5-case table-driven cleanup verification.
Parent: PLAN-1467.
* fix(urlimport): preserve hard-line-break markers + apply WithDomain on fallback per Codex review (round 1)
- MEDIUM: cleanupMarkdown was stripping the markdown two-trailing-
spaces hard-line-break idiom. html-to-markdown emits <br> as
" \n" — bulk-stripping trailing whitespace was demoting hard
breaks to soft wraps. Now the cleanup steps line-by-line, keeps
exactly-two trailing spaces (no tab), strips 1/3+/tab-mixed runs.
- MEDIUM: The raw-HTML fallback path (used when Readability cannot
identify an article) now passes converter.WithDomain(pageURL) so
relative links/images resolve against the source URL rather than
the Pad host where they'd 404.
Tests added:
- cleanupMarkdown: hard-line-break preserved, single/triple trailing
spaces stripped, tab-mixed spaces stripped, blank-line-with-spaces
collapsed (5 new cases).
- TestConvertGeneric_RelativeURLsResolvedOnFallback: relative href
in non-article HTML resolves to absolute URL via pageURL.
|
||
|
xarmian
|
0aa3988319 |
feat(urlimport): URL fetcher with SSRF guard + content-type detection (TASK-1469) (#552)
* feat(urlimport): URL fetcher with SSRF guard + content-type detection (TASK-1469) First slice of PLAN-1467's "Insert from URL" feature. Adds the internal/urlimport package with: - fetch.go: SSRF-guarded HTTP GET (10s timeout, 5 MB body cap, redirect re-validation, redacted-error formatting). Blocks loopback, RFC1918, CGNAT, IPv4/IPv6 link-local (incl. 169.254.169.254 cloud-metadata), IPv6 unique-local, and the unspecified address. Hostnames are resolved and every returned IP is checked. - detect.go: Content-type + body-prefix sniff returning "openapi" (JSON or YAML, OpenAPI 3.x or Swagger 2.0) or "generic". Inspects at most 64 KiB. - fetch_test.go: Table-driven SSRF tests covering 24 cases plus happy-path, size-cap, timeout, non-2xx, context-cancel, and a stubbed-transport redirect re-validation. - detect_test.go: 20 detection cases including OpenAPI JSON, Swagger YAML, vendor media types, leading comments, indented-key negatives, and charset-parameter normalization. Package name is urlimport (not "import" — reserved word). No callers yet; the endpoint that consumes Fetcher + Detect lands in TASK-1472. Parent: PLAN-1467. * fix(urlimport): close DNS-rebinding gap + handle >64 KiB OpenAPI JSON per Codex review (round 1) - HIGH: Add safe dialer transport (newSafeTransport). ValidateURL no longer does DNS — the dialer resolves once and validates the resolved IP at dial time, then dials that exact IP. DNS rebinding can no longer slip a public-IP validation past a loopback fetch. ValidateURL becomes a pre-flight (scheme/credentials/IP-literal only) with the canonical guarantee now at the transport layer. - MEDIUM: For JSON bodies over the 64 KiB sniff cap, switch from full Unmarshal (which fails on a truncated tail) to a streaming json.Decoder scan that walks top-level keys and short-circuits as soon as `openapi` or `swagger` is seen. Real-world specs over 64 KiB are now classified correctly, including the case where the `openapi` key is not the first top-level entry. Tests added: - TestFetch_DialerBlocksLoopbackHostname (dial-time rebinding guard) - TestDetect_LargeOpenAPIJSON (>200 KiB OpenAPI body, key first) - TestDetect_LargeOpenAPIJSON_KeyNotFirst (openapi key after huge info) - TestDetect_LargeJSONNotOpenAPI (huge non-OpenAPI stays generic) Removed the DNS-resolution case from TestValidateURL's notes and added a positive case proving hostnames pass the pre-flight (the dial-time check is now the canonical guard). * fix(urlimport): disable env proxy and reuse safe transport per Codex review (round 2) - HIGH: Set Proxy=nil on the safe transport. ProxyFromEnvironment would route via HTTP_PROXY/HTTPS_PROXY, where the dialer connects to the proxy host instead of the target — silently bypassing the hostname-resolution SSRF check inside DialContext. Operators who need an outbound proxy can wire their own trusted transport into Fetcher.Transport. - MEDIUM: Memoize the default safe transport per Fetcher via sync.Once. Previously each Fetch built a fresh *http.Transport whose keep-alive idle-pool stayed in scope until GC, leaking FDs under repeated imports. Now one transport is shared by all Fetch calls on a Fetcher; AllowLocal is captured at first use. |
||
|
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
|
438cb6180a |
fix(mcp): flexible JSON shapes on item create + clearer field surface (BUG-1431, BUG-1432) (#547)
* fix(mcp): flexible JSON shapes on item create + clearer field surface (BUG-1431, BUG-1432)
BUG-1432 root cause (real): models.ItemCreate.Tags is a Go string, so
the default unmarshaler rejected the natural JSON-array shape every
agent sends (`tags: ["foo","bar"]` → "cannot unmarshal array into Go
struct field ItemCreate.tags of type string", HTTP 400). On Postgres
the alternative — passing `tags: "foo,bar"` per the catalog's old
"Comma-separated tags" description — landed as a non-JSON value in
the JSONB column and surfaced as a generic HTTP 500. SQLite's TEXT
column silently accepted the corrupt value, which is why local repros
didn't show it.
Codex's independent investigation called out the asymmetry: ItemUpdate
already had a flexible UnmarshalJSON for `fields`/`tags` per BUG-1144,
but ItemCreate didn't. This PR mirrors that flexibility on the create
path and aligns the MCP surface description with reality.
BUG-1431 root cause (real, not the misdiagnosis the agent reported):
the dispatcher's `parseFieldKVP` only accepted the CLI-style array-of-
"key=value" shape, rejecting the JSON-native `field: {key: value}` map
shape with "expected array or string, got map[string]interface {}".
Agents naturally try the map shape and got a non-actionable error;
that drove the BUG-1409 agent to mis-blame status placement. Empirical
repro confirmed that `status` actually works in both top-level AND
inside-fields positions today (Tests 1, 4 in the investigation); the
real surface problem was the missing map shape on `field`.
Changes:
- internal/models/item.go: add UnmarshalJSON to ItemCreate mirroring
ItemUpdate's BUG-1144 pattern. Accepts `fields` as object or
JSON-encoded string; `tags` as array or JSON-encoded string; either
field absent / null leaves Go zero value. Wrong shapes surface
ErrInvalidFieldsType / ErrInvalidTagsType (existing sentinels) so
agents see clean domain errors instead of "Go struct field" leaks.
- internal/mcp/dispatch_http.go: parseFieldKVP now accepts
map[string]any in addition to the existing array/string shapes. Map
shape preserves non-string values verbatim (e.g. number from a typed
flag), matching the array path's existing pass-through for non-string
entries.
- internal/mcp/catalog_item.go: update `tags` description from
"Comma-separated tags" (wrong on both SQLite and Postgres) to
"Tags as a JSON array of strings, e.g. [\"v1\",\"frontend\"]". Update
`field` description to clarify it's the escape hatch for
SCHEMA-DECLARED custom fields, name the dedicated top-level params
agents should reach for instead (status/priority/category/parent/
role/assign/tags), and note the new map-shape acceptance. Tool-level
prose updated to match.
Tests:
- TestItemCreateUnmarshalFlexFields (mirror of
TestItemUpdateUnmarshalFlexFields): 9 cases covering array/string/
null/absent/wrong-shape tags + object/string/array fields, plus a
smoke test that other fields decode normally alongside the new
flex paths.
- TestParseFieldKVP_Variants: extended with 3 new map-shape cases
(basic map, empty-key-skipped, non-string-value preserved).
End-to-end verification: 5 input shapes via curl against the live
handler. Pre-fix `tags: ["foo","bar"]` returned HTTP 400; post-fix
returns HTTP 201 with `tags="[\"foo\",\"bar\"]"` in the column.
`tags: {x:1}` (wrong shape) now returns a clean
domain-level 400 instead of leaked Go internals. Existing back-compat
paths (JSON-encoded string forms) preserved.
Related: PR #546 (BUG-1430 rate limit) addressed the original 500
cascade that drove the agent's specific misdiagnoses in BUG-1409.
* fix(mcp): forward tags array on update + drop unsupported map-shape doc per Codex review (round 1)
Codex round 1 caught two issues:
[P1] dispatch_http_advanced.go's PATCH builder filtered on `string`
only when forwarding `tags`, so a schema-conforming
`pad_item.update tags: ["a","b"]` was silently dropped. Now forwards
verbatim like mapItemCreate does — the handler's ItemUpdate
flex-unmarshaler (BUG-1144) normalizes any shape downstream.
Regression test added.
[P2] The `field` description claimed `{key: value}` map shape was
accepted, but the schema Type stays `array<string>` so schema-following
clients won't send the map shape. parseFieldKVP's map-shape handling
(added in the previous commit) stays as defensive parsing for clients
that ignore the schema, but the description no longer promises a shape
the published schema doesn't advertise. Tool-level prose updated to
match.
* fix(mcp): revert speculative parseFieldKVP map-shape support per Codex review (round 2)
Codex round 2 [P2] pointed out the map-shape parseFieldKVP support
added in the first commit is dead code in practice:
1. The advertised schema for `field` is `array<string>` — no
schema-conforming client sends a map.
2. `BuildCLIArgs` rejects map-shaped repeatable flags before they
reach the HTTP dispatcher.
3. Even if a map did reach the dispatcher, `hasFieldChanges`
doesn't recognize map shapes as field changes — `pad_item.update
field: {effort: "l"}` would skip the merge and PATCH without
`fields`.
Either completing the support (fix hasFieldChanges + BuildCLIArgs +
ItemUpdate Unmarshal) OR reverting was the right call. Reverting
keeps the surface consistent with the schema and removes the
unreachable code; future agents who want to override fields can use
the documented `["key=value"]` array shape.
BUG-1431's functional fix lands as the catalog description tightening
(the empirical repro confirmed `status` placement already works in
both forms; the agent's misdiagnosis was rooted in unclear docs, not
broken code). BUG-1432's flexible JSON unmarshal on ItemCreate stays
— that's the real fix verified by the live-handler repro.
* fix(mcp): preserve empty-string tags no-op + table-driven test per Codex review (round 3)
Codex round 3 [P2] caught a regression introduced in round 1's fix: by
switching the tags forwarding guard from \`v.(string) && v != ""\` to
\`v != nil\` to support array shapes, the empty-string filter for tags
on update was lost. \`pad_item.update tags: ""\` would now forward an
empty string to ItemUpdate, which treats it as an explicit
empty-string write — corrupting the JSON/JSONB tags column (500 on
Postgres).
Fix: type-switch on tags. Empty string skips (matches pre-fix
behaviour); arrays (including empty array \`[]\`, the legitimate
"clear tags" case) and non-empty strings forward.
Tests: the single-shape array test is replaced with a table-driven
TestDispatchItemUpdate_TagsForwarding covering array, empty array,
empty string (no-op), and comma-separated back-compat. Each case
asserts the tags key's presence/absence and shape in the PATCH body.
|
||
|
xarmian
|
088ba2f839 |
fix(mcp): raise MCP per-token burst + classify 429 as ErrRateLimited (BUG-1430) (#546)
* fix(mcp): raise MCP per-token burst + classify 429 as ErrRateLimited (BUG-1430)
BUG-1409 reported an agent hitting "Pad backend 500s on parallel writes"
during workspace onboarding via remote MCP on Pad Cloud. Triage split
that umbrella into three children; this PR addresses BUG-1430 (the
parallel-writes symptom).
Root cause investigation showed the underlying write path is fine —
local SQLite handled 24 parallel item-create POSTs cleanly (busy_timeout
+ BEGIN IMMEDIATE + WAL serialize writers without errors). The most
plausible cause of the agent's "500 on parallel writes" report is the
MCP per-token rate limiter (burst 20, 60/min) rejecting requests 21-24
of an onboarding burst with HTTP 429, which the dispatcher's classifier
then collapsed into a generic ErrServerError envelope.
Changes:
- middleware_ratelimit.go: MCPPerToken burst 20 → 60. Sustained rate
unchanged at 60/min/token. Matches the general API limiter's burst-60
per-user cap so the MCP path no longer imposes a tighter ceiling than
the equivalent /api/v1 path. Comment expanded to record the rationale.
- internal/mcp/errors.go: add ErrRateLimited error code and an explicit
case http.StatusTooManyRequests in classifyHTTPStatusKind. 429s now
surface as a first-class rate-limited envelope with an actionable hint
pointing at Retry-After and the per-token cap, instead of landing in
the generic ErrServerError "other 4xx" bucket. Agents implementing
exponential backoff can switch on code without parsing free-form text.
- handlers_cloud.go: add slog.Error instrumentation to enforcePlanLimit
and enforceUserPlanLimit error paths. These are cloud-mode-only 500
candidates we couldn't exercise locally (local dev runs cloudMode=false);
the structured logs give operators a grep-able tag the next time the
symptom surfaces on real Pad Cloud, so we can rule the path in or out
empirically without another investigation pass.
- tests: bump iteration counts past the new burst (20 → 60), add 429
case to classifyHTTPStatus code-mapping table + envelope hint-shape
table.
Investigation context (full triage in BUG-1430):
- ../pad-cloud sidecar is NOT in the /api/v1 or /mcp request path
(nginx-router proxies those directly to pad backend).
- featureCount + advisory-lock contention on Postgres remain plausible
500 candidates under heavy bursts; the new logging is intended to
catch those if they fire.
Siblings BUG-1431 (status field placement) and BUG-1432 (tags field)
are tracked separately and not addressed here.
* fix(mcp): drop hardcoded cap from rate-limit hint per Codex review (round 1)
Codex round 1 [P2] caught that rateLimitHintFor's "the per-token cap is
60 req/min with a burst of 60" text was misleading: classifyHTTPStatusKind
handles 429s from the dispatcher's SYNTHESIZED /api/v1/... requests, which
come from the general API limiter (600/min, burst 60), the Search limiter
(30/min, burst 10), and potentially others — NOT the MCP per-token
limiter (which fires before the dispatcher runs and so never lands in
this classifier path).
Generalize the hint: point at Retry-After (which carries the correct
limiter-specific wait) and drop the cap from prose. Update the matching
test assertion to assert the generic shape ("burst-heavy" instead of
"60 req/min").
|
||
|
xarmian
|
de8679f535 |
chore(mcp): bump ToolSurfaceVersion 0.3 → 0.4; document v0.4 envelope (TASK-1418) (#544)
* chore(mcp): bump ToolSurfaceVersion 0.3 → 0.4; document v0.4 envelope (TASK-1418)
Final PR of PLAN-1410. The contractual announcement that the v0.4
bootstrap shape is stable.
## What
1. internal/mcp/version.go — ToolSurfaceVersion: "0.3" → "0.4".
The godoc on the constant gains a full v0.4 changelog entry
enumerating each shape change shipped by PLAN-1410's six
bootstrap PRs:
- BootstrapCollection projection (TASK-1412): drops id,
workspace_id, created_at, updated_at, settings; schema as
a nested JSON object.
- BootstrapRole projection (TASK-1423): drops id,
workspace_id, tools, created_at, updated_at.
- Convention slug dropped (TASK-1413).
- Top-level recent_activity duplicate removed (TASK-1413).
- BootstrapDashboard wrapper caps five sub-arrays (TASK-1413
+ TASK-1422): attention, recent_activity, active_items,
active_plans, by_role at 5 entries each, parallel
*_overflow_count fields. suggested_next deliberately
excluded — already capped to 3 upstream.
- Schema label omitted when label == TitleCase(key) (TASK-1424).
Plus an explicit compatibility note: all v0.4 changes are
additive or subtractive (no field renames); clients that read
the preserved field names keep working unchanged.
2. CLAUDE.md updates:
- "## MCP server" header: v0.3 catalog → v0.4 catalog, with a
one-paragraph summary of what v0.4 shipped.
- "Surface:" Tools bullet: v0.3 → v0.4, with a note that the
tool/action surface is unchanged — only the bootstrap JSON
these tools return has been trimmed.
- "Stability contract": ToolSurfaceVersion (currently "0.4"),
comprehensive single-paragraph description of the v0.4
envelope, cumulative size reduction (40% live / 54% fixture),
and explicit additive/subtractive note.
## Why the strategy worked
PLAN-1410's "version bump last" strategy paid off:
- Each individual shape PR (TASK-1412/1413/1422/1423/1424) was
reviewable in isolation against a stable v0.3 contract.
- The six skill-side PRs (TASK-1414/1415/1416) had no MCP-shape
impact and didn't need any version bump consideration.
- v0.4 is now announced as a single comprehensive contract change,
not five separate version bumps — easier for downstream MCP
consumers (Claude Desktop, Cursor, future Pad Cloud remote MCP)
to reason about.
## Verification
- `make check` — golangci-lint 0 issues, all Go tests pass
(including the version-tracking tests in catalog_meta_test.go
that auto-pin to whatever ToolSurfaceVersion is set to),
govulncheck clean, web build clean.
- MCP handshake (verified via `pad mcp serve` + an initialize
JSON-RPC request) advertises
capabilities.experimental.padToolSurface.version = "0.4".
padCmdhelp.version stays at "0.1" as expected.
## Post-merge follow-ups
After this lands:
- Update PLAN-1410's Result section with a "v0.4 announced" line
and the final post-everything measurement (taken against
docapp after `make install`).
- Flip PLAN-1410 status from `active` → `completed`.
These are pad-item operations, not git changes.
Parent: PLAN-1410. Closes the plan.
* fix(mcp): update stale v0.3 references after ToolSurfaceVersion bump (TASK-1418 follow-up)
Address Codex P2 + P3 findings on PR #544: bumping
ToolSurfaceVersion in version.go left four runtime/user-facing
docs still claiming v0.3:
P2 — runtime MCP docs:
- internal/mcp/instructions.md "## Tool surface (v0.3)" → v0.4
- internal/mcp/catalog_meta.go "v0.3 server-introspection tool" → "(v0.4 catalog)"
- internal/mcp/catalog_meta.go padMetaToolDescription twice:
* "the v0.3 tool catalog" → "the v0.4 tool catalog"
* "v0.3 catalog dump" → "v0.4 catalog dump"
- internal/mcp/catalog_meta.go actionMetaToolSurface godoc:
"v0.3 catalog" → "catalog" (de-versioned; the comment is
about scope, not version)
P3 — public README:
- README.md "Tool catalog (v0.3)" → "Tool catalog (v0.4)"
- README.md "tool_surface_version: '0.3'" → "'0.4'" with a
pointer to PLAN-1410's bootstrap-trim summary and
version.go's full v0.4 changelog.
Without these, agents reading the initialize-instructions blob or
pad_meta's tool description (both of which are part of the
runtime MCP surface, not just internal docs) would see v0.3 while
the handshake / pad_meta.action: version returned v0.4 — the
exact "contradictory metadata depending on what you read" failure
mode Codex flagged.
Same skill-↔-code sync pattern that has been a running theme
through PLAN-1410's review loops. The cluster of stale references
is a classic side effect of a version bump landing late in a
plan — the version constant is one string, but downstream prose
that names it lives in multiple places.
Verified no remaining "v0.3" claims that imply currency — `grep -rn
"v0\.3\|tool_surface_version" --include="*.{go,md}"` returns only
historical-context mentions in changelog godocs (correct) and the
runtime constant readback (correctly returns "0.4" now).
Parent: PLAN-1410 / TASK-1418.
* fix(mcp): correct schema-type-change disclosure + stale cmdhelp-walker description (TASK-1418 follow-up)
Address Codex round 2 P3 findings on PR #544:
## P3 — `cmd/pad/mcp.go` still described the retired leaf walker
The `pad mcp serve` command's Long description said "every leaf
command becomes an MCP tool, except the curated allow-list
exclusions" — that was true under v0.1 but the cmdhelp leaf
walker was retired in TASK-981 (PLAN-969's v0.2 rollout). The
v0.2/v0.3/v0.4 surface has always been the hand-curated catalog
of eight resource × action tools + pad_set_workspace.
Updated the Long description to:
- Name the v0.4 catalog explicitly.
- List the eight resource × action tools.
- Note that cmdhelp v0.1 still drives per-command arg schemas
at dispatch time (so it's not gone, just no longer drives
tool naming/count).
- Reference TASK-981 for the cutover.
## P3 — "additive/subtractive only" was misleading
The compatibility note in `version.go` and `CLAUDE.md` claimed
all v0.4 changes were additive or subtractive. That glossed over
one breaking change in TASK-1412: `collections[].schema` went
from a JSON-encoded string ("schema":"{\"fields\":...}") to a
nested JSON object ("schema":{"fields":...}). For any v0.3
consumer that read schema as a string and JSON.parse()'d it
themselves, that's a TYPE change, not a no-op.
Updated both godoc and CLAUDE.md to explicitly call this out
as the one breaking change, separately from the additive/
subtractive bucket. Better for downstream MCP consumers to see
the truth than to discover it via runtime failure.
The remaining v0.4 changes ARE additive (overflow counts on
BootstrapDashboard) or subtractive (dropped fields with named
canonical alternatives) — those parts of the original note
are accurate and kept.
Honesty about compatibility is more valuable than a tidy
narrative. Surfaced explicitly in the godoc + the public
contract doc; PLAN-1410's Result section was already honest
about the field-level deltas.
Parent: PLAN-1410 / TASK-1418.
|
||
|
xarmian
|
d18a5d8140 |
refactor(bootstrap): omit redundant schema labels + omitempty sort_order (TASK-1424) (#543)
Two small additive trims to the BootstrapCollection projection
introduced in TASK-1412.
## 1. Omit redundant schema `label` when label == TitleCase(key)
A schema field's `label` is auto-fillable from its `key` (the CLI
and the MCP-side CreateCollection helper both apply
TitleCase(key) when label is empty — see titleCaseLabel in
internal/mcp/dispatch_http_routes.go). When the persisted label
matches that rule, it's redundant — the agent can reconstruct it
from the key. Examples on docapp:
- {"key":"status", "label":"Status"} ← redundant
- {"key":"due_date", "label":"Due Date"} ← redundant
- {"key":"trigger", "label":"When"} ← CUSTOM, preserved
Implementation: projectBootstrapCollection now passes the schema
bytes through trimRedundantSchemaLabels, which:
1. Unmarshals into a parallel bootstrapSchema/bootstrapFieldDef
struct purpose-built for the bootstrap shape.
2. Walks fields, clears any Label that equals TitleCase(Key).
3. Re-marshals — `omitempty` on bootstrapFieldDef.Label drops
the empty-string labels from the output.
Field ordering is preserved by struct-based marshalling.
## 2. Omitempty on BootstrapCollection.SortOrder
`sort_order` defaults to 0. Most collections never get an explicit
non-zero sort_order, so the field carried "sort_order":0 per entry
needlessly. Added ,omitempty to the struct tag.
## Drift detection
bootstrapFieldDef mirrors models.FieldDef field-for-field with two
deliberate differences: `Label` is omitempty, and `Default` is
json.RawMessage (so any default value round-trips verbatim
without re-parsing). The risk: if models.FieldDef gains a new
field, bootstrapFieldDef silently loses it from the bootstrap
schema response.
TestBootstrapFieldDefMirrorsModelsFieldDef catches this via
reflection — compares NumField + per-field JSON tags + has an
explicit allow-list for the Label tag delta. A new field added to
models.FieldDef without mirroring here fails the test with a
field-name-pointed error message.
## Test coverage
- TestBootstrapFieldDefMirrorsModelsFieldDef — drift detector.
- TestTrimRedundantSchemaLabels (4 subtests):
* drops-redundant-labels
* preserves-custom-labels (key="trigger", label="When" stays)
* multi-word-keys-titlecase-correctly (due_date → Due Date)
* malformed-schema-returns-raw (defensive: never block on parse error)
- Existing TestBootstrapSizeBudget shows fixture collections
section drop: 3,979 → 3,532 bytes (-11.2%).
## Measurements
Fixture (TestBootstrapSizeBudget): 7,823 → 7,376 bytes (-447 b / -5.7%).
Collections section alone: 3,979 → 3,532 bytes (-447 b / -11.2%) —
all of the win is concentrated in schema bytes via the label trim.
Live docapp expected savings: 9,384 → ~8,400 bytes (~10% drop on
collections), totaling roughly 600-900 b additional reduction on
the bootstrap response.
## Out of scope
- ToolSurfaceVersion 0.3 → 0.4 bump — TASK-1418 (the FINAL PR in
PLAN-1410). This is the last shape PR; TASK-1418 is now unblocked.
Parent: PLAN-1410. With this merged, PLAN-1410's bootstrap-shape
work is complete; only the contractual v0.4 announcement remains.
|
||
|
xarmian
|
ad87b960d7 |
refactor(bootstrap): slim Roles projection (BootstrapRole struct) (TASK-1423) (#542)
Same drop-pattern as TASK-1412's BootstrapCollection applied to the
bootstrap response's `roles` section.
## Struct + helper
New BootstrapRole struct purpose-built for the bootstrap response:
- Keeps: slug, name, description, icon, sort_order, item_count
- Drops: id, workspace_id, tools, created_at, updated_at
The `tools` field has no consumer outside the store CRUD
(grep confirms — only referenced in internal/store/agent_roles.go);
docapp has it set to "" on all three roles in practice. If it ever
becomes load-bearing for the agent, it can be added back.
New projectBootstrapRole(models.AgentRole) helper mirrors
projectBootstrapCollection.
## BuildAgentBootstrap reorder
Roles projection now happens AFTER the role-count recompute for
restricted callers (which keys lookups by AgentRole.ID, which the
projection drops). Same reorder pattern as TASK-1412's collections
refactor — the local `roles` slice stays in models.AgentRole shape
through the count recompute, then projects in a final pass alongside
the collections projection.
## Tests
New TestBootstrapRoleProjection seeds one role via the agent-roles
endpoint and verifies the wire shape:
- Positive: slug/name/description/item_count are present and correct.
- Negative: id/workspace_id/tools/created_at/updated_at are NOT
present in the marshalled JSON.
The negative check is the load-bearing part — without it, a future
refactor that "fixes" the projection by re-adding a UUID would pass
the positive assertions silently. Mirrors
TestBootstrapEmptyArraysNotNull's pattern for the recent_activity dedup.
Existing TestBootstrapEmptyWorkspace and TestBootstrapEmptyArraysNotNull
still pass — their `b.Roles != nil` and `roles != null` checks work
at slice-level regardless of element type.
## Measurements
Fixture (TestBootstrapSizeBudget) unchanged at 7,823 bytes — the
fixture seeds 0 roles so the projection change doesn't affect the
size budget (roles section was already at 2 bytes "[]").
Live docapp measurement deferred until make install — expected
savings: roles section was 1,066 bytes for 3 roles; drop is
~30-40% (~350 b) of that section.
## Out of scope
- Schema label + sort_order trim — TASK-1424.
- ToolSurfaceVersion 0.3 → 0.4 — TASK-1418 (final PR).
Parent: PLAN-1410.
|
||
|
xarmian
|
aaa581b105 |
refactor(bootstrap): extend dashboard caps to active_items/active_plans/by_role (TASK-1422) (#541)
* refactor(bootstrap): extend dashboard caps to active_items/active_plans/by_role/suggested_next (TASK-1422) Implements IDEA-1421 (absorbed into PLAN-1410's v0.4 envelope). Extends the BootstrapDashboard wrapper from TASK-1413 to cap four more dashboard sub-arrays with parallel overflow counts, same shape and semantics as the existing attention/recent_activity caps. ## Struct + caps Four new int fields on BootstrapDashboard (all `,omitempty`): - active_items_overflow_count - active_plans_overflow_count - by_role_overflow_count - suggested_next_overflow_count Four new cap constants alongside the existing two: - bootstrapActiveItemsCap = 5 - bootstrapActivePlansCap = 5 - bootstrapByRoleCap = 5 - bootstrapSuggestedNextCap = 5 capBootstrapDashboard extended with four parallel truncate-and-count blocks — same shallow-copy mutation pattern, source pointer untouched (the dashboard endpoint still returns its full-length arrays per its own contract). ## Tests TestCapBootstrapDashboard rewritten to cover all six caps under one contract. `mk` now takes a `dashCounts` struct (Att/Rec/Items/Plans/ Role/Sugg) so each subtest exercises specific caps without populating the others. Each of the four existing subtests (under-cap-no-overflow, over-cap-truncates-and-counts-overflow, source-pointer-unchanged, exact-cap-no-overflow) now asserts the new caps too. Added tiny assertLen/assertOverflow helpers to keep the per-array assertion noise from drowning the contract being tested. bootstrapSectionBytes extended to surface the four new cap-effect lines when triggered, table-driven so future caps drop in cleanly. seedBootstrapSizeFixture updated to seed 6 in_progress tasks (was 5 open) so the new active_items cap fires visibly in the per-section breakdown: "active_items capped: 5 shown, 1 overflow". The status flip is deliberate — dashboard.active_items filters on isActiveStatus(), which excludes initial/terminal statuses; open tasks never appeared in the section. ## Budget bootstrapSizeBudget 7 KiB → 9 KiB. Note that this is FIXTURE-side growth, not shape-side regression: the fixture now seeds enough active items to exercise the new cap (active_items section was 0 bytes when tasks were status=open). The cap itself is purely a SAVINGS — on docapp it drops active_items from 7 → 5 entries with overflow_count=2. Budget history note in handlers_bootstrap_test.go updated with the TASK-1422 line and an explicit "fixture-side, not shape-side" explanation so future readers understand why the budget moved up. ## Out of scope - Slim BootstrapRole projection — TASK-1423. - Schema label + sort_order trim — TASK-1424. - ToolSurfaceVersion 0.3 → 0.4 bump — TASK-1418 (final PR). Parent: PLAN-1410. Resolves IDEA-1421 once merged. * fix(bootstrap): drop unreachable suggested_next cap (TASK-1422 follow-up) Address Codex P1 finding on PR #541: `suggested_next_overflow_count` was unreachable in production responses because `buildDashboardResponse` already truncates `SuggestedNext` to 3 upstream (see "Take top 3" comment in handlers_dashboard.go:854-858), while my bootstrap cap was 5. The cap-and-overflow logic could only have fired against synthetic test state, never against the real dashboard pipeline. Two responses to consider: 1. Lower bootstrap's cap to a number smaller than 3 — defeats the upstream design choice (3 IS the intentional limit). 2. Drop the bootstrap-side cap — clean, no dead surface. Going with (2). If the upstream cap is ever raised or removed, that's the moment to add a suggested_next_overflow_count back. Removed: - SuggestedNextOverflowCount field on BootstrapDashboard - bootstrapSuggestedNextCap constant - The truncate-and-count block in capBootstrapDashboard - The suggested_next row in TestCapBootstrapDashboard's dashCounts helper and all five subtest assertions - The suggested_next entry in bootstrapSectionBytes's cap-line loop The fixture still seeds tasks and a plan, so the no-cap path on SuggestedNext is naturally exercised through TestBootstrapSizeBudget. The godoc on BootstrapDashboard now explicitly calls out the exclusion + the upstream-cap rationale so a future reader knows why suggested_next is missing from the otherwise-uniform cap set. Parent: PLAN-1410 / TASK-1422. * docs(skill): align SKILL.md dashboard cap description with TASK-1422 Address Codex P3 finding on PR #541: the SKILL.md `Context Loading` section described only the two original cap fields (attention_overflow_count, recent_activity_overflow_count). After TASK-1422 the bootstrap response carries three more (active_items_overflow_count, active_plans_overflow_count, by_role_overflow_count), and the agent needs to know to pull the full set via `pad project dashboard` when any of them are > 0. Updated the bullet to enumerate all five capped sub-arrays and state the overflow-field pattern generically rather than per-field. Same in-PR-sync pattern used for TASK-1413, TASK-1415, TASK-1416. Parent: PLAN-1410 / TASK-1422. * docs(bootstrap): fix three stale comments after dropping suggested_next cap (TASK-1422 follow-up) Address Codex P3 finding on PR #541 round 3: three stale doc strings referenced the old shape (with suggested_next) after the cap was dropped in the prior commit. Updated: 1. handlers_bootstrap_test.go budget-history line for TASK-1422 — removed `suggested_next` from the cap list and added the "deliberately excluded — already capped to 3 upstream" rationale so future readers know why the otherwise-uniform cap set is missing one. 2. handlers_bootstrap.go BootstrapDashboard godoc — changed "two overflow counts" to "five overflow counts (one per capped sub-array)". 3. handlers_bootstrap.go capBootstrapDashboard godoc — changed "both caps are untriggered" to "all caps are untriggered". Tidy-up only, no behavior change. Same skill-↔-code sync hygiene that has been the running theme across PLAN-1410's review loops. Parent: PLAN-1410 / TASK-1422. * docs(bootstrap): update remaining stale call-site comment for capBootstrapDashboard (TASK-1422 follow-up) Final stale-doc cleanup per Codex P3 on PR #541 round 4: the BuildAgentBootstrap dashboard-wrapping call-site comment still listed only attention + recent_activity. Updated to enumerate all five capped sub-arrays for parity with the godoc on BootstrapDashboard / capBootstrapDashboard. Same hygiene as the previous commit; no behavior change. Parent: PLAN-1410 / TASK-1422. |
||
|
xarmian
|
9c9aac3ea6 |
chore(bootstrap): tighten size budget 8 KiB → 7 KiB to lock in PLAN-1410's win (TASK-1417) (#540)
PLAN-1410's bootstrap shape work is complete on main (TASK-1411..1416). The fixture still measures 6,355 bytes against the 8 KiB budget — too much headroom for a ratchet whose purpose is to detect shape regressions. Tightened to 7 KiB: - Fixture: 6,355 bytes - Budget: 7,168 bytes (7 KiB) - Headroom: 813 bytes (~12.8%) Tight enough to catch any meaningful shape regression (reintroducing a duplicated field, un-capping a dashboard array, re-stringifying schema), loose enough to absorb routine schema reordering or single-field additions without false alarms. Budget-history note updated with the TASK-1417 line and a forward-looking pointer at IDEA-1421 (next-round dashboard sub-array caps) which will land its own win under its own ratchet. The companion measurement (per-section bytes for fixture + live docapp + pre/post per-invocation totals) was recorded directly into PLAN-1410's body via `pad item update PLAN-1410 --stdin`, which is the canonical home for plan results. Cumulative win: fixture: 13,861 → 6,355 (-54.2%) live docapp bootstrap: 52,033 → 33,375 (-35.9%) live SKILL.md: 40,193 → 29,840 (-25.8%) live combined per /pad: 92,226 → 63,215 (-31.5%) The original ~43% target was set against a static-workspace assumption; the live shortfall is workspace-scale-dependent (conventions on docapp are 11.2 KB vs 0.5 KB on the fixture). IDEA-1421 captures the next round (dashboard active_items / active_plans / by_role / suggested_next caps, estimated ~3 KB additional live win, backwards-compatible). Parent: PLAN-1410. Final remaining task: TASK-1418 (ToolSurfaceVersion 0.3 → 0.4 contractual announcement). |
||
|
xarmian
|
638a456f98 |
refactor(bootstrap): dedup recent_activity, drop convention slug, cap dashboard arrays (TASK-1413) (#536)
* refactor(bootstrap): dedup recent_activity, drop convention slug, cap dashboard arrays (TASK-1413)
Three bundled handler-level cleanups against PLAN-1410's bootstrap
shape. Total fixture savings: 8,992 → 6,355 bytes (-2,637 b / -29%).
1. Drop duplicate top-level `recent_activity`
AgentBootstrap.RecentActivity was bit-for-bit identical to
AgentBootstrap.Dashboard.RecentActivity. Removed:
- AgentBootstrap.RecentActivity field
- capRecentActivity() helper
- recentActivityWindow constant
- the time import (no longer used)
Fixture savings: -1,751 bytes.
2. Drop `slug` from AgentBootstrapConvention
Agents address convention items by ref (CONVE-N); slug was dead
weight. Removed the field + the population line in
collectAlwaysOnConventions.
Fixture savings: -78 bytes.
3. Cap dashboard.attention + dashboard.recent_activity to 5 in bootstrap
New BootstrapDashboard wrapper embeds *DashboardResponse (so the
wire shape stays compatible — same field names, same nesting) and
adds two overflow counts:
- attention_overflow_count (omitempty when zero)
- recent_activity_overflow_count (omitempty when zero)
The cap is applied via capBootstrapDashboard which shallow-copies
the DashboardResponse before truncating the slices, so callers
downstream of buildDashboardResponse (the dashboard endpoint
itself, the web UI) see their original full-length arrays
unchanged. `pad project dashboard` contract is preserved verbatim.
Fixture savings: -789 bytes (recent_activity capped 9 → 5;
attention untouched, fixture has 0 attention items).
Coverage:
- TestCapBootstrapDashboard (4 subtests): under-cap-no-overflow,
over-cap-truncates-and-counts-overflow, source-pointer-unchanged,
exact-cap-no-overflow. Locks in the cap contract independent of
the full bootstrap pipeline.
- TestBootstrapEmptyArraysNotNull updated: the top-level
recent_activity key was removed from the required-keys list,
with a separate assertion that guards against it reappearing.
- TestBootstrapEmptyWorkspace updated: removed the b.RecentActivity
nil-check; added a (defensive) check that dashboard's nested
recent_activity is non-nil when dashboard is present.
- bootstrapSectionBytes now surfaces the cap effect ("attention
capped: 5 shown, 4 overflow") when triggered, so the trim's
value is legible from CI output.
bootstrapSizeBudget tightened 11 KiB → 8 KiB to lock in the win.
Budget-history comment updated.
Out of scope (handled by later PLAN-1410 PRs):
- Skill-file trim (TASK-1414/1415/1416)
- Final measurement (TASK-1417)
- ToolSurfaceVersion 0.3 → 0.4 (TASK-1418, after all shape
changes land)
Parent: PLAN-1410.
* docs(skill): align SKILL.md bootstrap shape with PLAN-1410 / TASK-1413
The skill's `Context Loading` section described the old wire shape:
- `dashboard {...}` — active items, attention, suggested next, recent activity
- `recent_activity [...]` — capped to the last 24h
After TASK-1413 the top-level `recent_activity` field is gone (it was
a bit-for-bit duplicate of `dashboard.recent_activity`), and the
remaining `dashboard.recent_activity` is capped by COUNT (top 5) not
by TIME (24h window). The two cap fields (attention_overflow_count
and recent_activity_overflow_count) tell the agent how much was
trimmed so it can decide whether to follow up with a full
`pad project dashboard` query.
Per the Codex P2 finding on PR #536: documenting the new contract
in this PR keeps skill ↔ wire-shape strictly synchronized (no
window where the docs are wrong about the shape this PR ships).
Parent: PLAN-1410 / TASK-1413.
|
||
|
xarmian
|
8da0473e4e |
refactor(bootstrap): slim Collections projection — drop id/timestamps/settings, parse schema inline (TASK-1412) (#535)
Introduces BootstrapCollection — a purpose-built projection for the
bootstrap response that replaces []models.Collection on
AgentBootstrap.Collections. Drops fields the /pad skill never reads:
- id, workspace_id — agent addresses collections by slug
- created_at, updated_at, — irrelevant at context-load time
deleted_at
- settings — quick_actions + view defaults are
web-UI chat-prompt config
The remaining schema string is delivered as a nested JSON object
(json.RawMessage) rather than a JSON-encoded string, killing the
backslash-escape overhead so the agent sees real {}/[] structure
instead of double-encoded quotes. json.Valid() gates the emission
so a future migration leaving non-JSON in the column can't break
agent-side json.Unmarshal — invalid/empty schemas are simply
omitted (omitempty).
Measured against the bootstrapSizeBudget fixture (TASK-1411):
before after delta
collections 8,848 b 3,979 b -4,869 b (-55%)
total bootstrap 13,861 b 8,992 b -4,869 b (-35%)
Budget tightened from 16 KiB to 11 KiB to lock in the win. Later
PLAN-1410 PRs (TASK-1413's dedup + dashboard caps, TASK-1417's
final measurement) tighten further.
Wire-shape change details:
- BuildAgentBootstrap holds collections as []models.Collection
through the visibility-restricted role+count recompute (which
keys lookups by Collection.ID), then projects to []BootstrapCollection
at the end of that section. ID-keyed recompute logic is preserved
verbatim — only the final wire shape changes.
- printBootstrapMarkdown was already reading {slug, name, prefix}
via its own anonymous struct; those three are preserved.
- No web-UI consumers exist for /agent/bootstrap (grep confirms),
so no client-side churn.
Out of scope (handled by later PLAN-1410 PRs):
- Dedup'ing top-level recent_activity, dropping convention slug,
capping dashboard.attention/recent_activity (TASK-1413).
- ToolSurfaceVersion bump 0.3 → 0.4 (TASK-1418, after all shape
changes land).
Parent: PLAN-1410.
|
||
|
xarmian
|
91f1c0a017 |
test(bootstrap): add size-budget benchmark for the agent bootstrap response (TASK-1411) (#534)
Adds TestBootstrapSizeBudget which builds an AgentBootstrap blob against
a seeded representative fixture (default template seeds + 2 always-on
conventions with bodies + 1 slug-invocable playbook + 5 tasks + 1 plan)
and asserts the marshalled JSON byte count stays at or below
bootstrapSizeBudget (initially 16 KiB, against a current actual of
~13.8 KiB on the seeded fixture).
On every run — pass or fail — the test logs a per-section breakdown
(workspace / user / collections / conventions / roles / playbooks /
dashboard / recent_activity) so size regressions are diagnosable from
CI output alone, and so the cumulative trim across PLAN-1410 is visible
as the budget tightens.
This is the baseline ratchet for PLAN-1410's bootstrap-shape PRs:
- TASK-1412 (slim Collections projection) tightens the budget down
once schema-as-string and the redundant ids/timestamps come out.
- TASK-1413 (dedup top-level recent_activity, drop convention slug,
cap dashboard arrays) tightens further.
- TASK-1418 records the final v0.4 actual.
The docapp workspace currently measures ~52 KB / ~13K tokens on the
real bootstrap payload; the fixture is intentionally small but
exercises the same shape contributors so a regression in the projected
shape (per-collection settings, schema-as-string, duplicate
recent_activity, etc.) trips the budget at fixture scale.
Parent: PLAN-1410.
|
||
|
xarmian
|
1db0e6a505 |
docs+test: closes the PLAN-1397 library overhaul loop (TASK-1404) (#533)
* docs+test: closes the PLAN-1397 library overhaul loop (TASK-1404)
## Tests
New `internal/collections/playbook_library_test.go` with 4 regression
guards for the invokable-first library:
- TestPlaybookLibrary_InvokableEntriesPresent — asserts ship + plan +
decompose are all present by invocation_slug, that each has at
least one Argument declared, and that at least 3 invokable entries
exist. Catches T1 widening getting half-reverted or T6's library
rebuild dropping an entry.
- TestPlaybookLibrary_AllTriggersKnown — asserts every library
entry's Trigger is one of the canonical values (manual,
on-implement, on-review, on-plan, on-triage, on-release,
on-deploy, on-pr-create, on-task-complete, always). Catches typos
that would seed an invalid trigger.
- TestPlaybookLibrary_ShipBodyShared — confirms the library `ship`
entry and ShipPlaybook() seed share the same body constant. The
whole point of T3 was to avoid duplication; this prevents drift.
- TestPlaybookLibraryArchive_BodiesCompiled — keeps archivedPlaybooks()
greppable and verifies the canonical retired title ("Implementation
Workflow") is still in the archive.
## CLAUDE.md
- Added a "Library — discovery surface" subsection to the Playbooks
section explaining the invokable-first lineup, the archive, and
how `softwareStarterPlaybookTitles` seeds plan + decompose into
software workspaces.
- Expanded "Code map" with the four new/refactored library files
(playbook_library.go, playbook_library_plan.go,
playbook_library_decompose.go, playbook_library_archive.go).
- Cross-referenced PLAN-1397 alongside PLAN-1377 for design history.
## skills/pad/SKILL.md
- "Planning: 'Let's create a plan'" and "Decomposition: 'Break plan
X into tasks'" now name `/pad plan` and `/pad decompose` as the
canonical entry points, with the inline workflow as the fallback
when those playbooks aren't activated in the workspace.
- The high-level "Planning:" intent list at the top of the routing
section updated to match.
- Note: SKILL.md is embedded into the Go binary at build time. The
next `make install` distributes the skill update; the .agents/
copy regenerates automatically.
Parent: PLAN-1397. Closes the loop — all 7 tasks of the playbook
library overhaul shipped.
* fix(test): source knownPlaybookTriggers from SoftwarePlaybookTriggers per Codex review (round 1)
Round 1: the hand-maintained `knownPlaybookTriggers` map admitted
`on-pr-create`, `on-task-complete`, and `always` — all valid in
the schema for *conventions*, but NOT in `SoftwarePlaybookTriggers`
(templates.go:266). A library entry could ship one of those and
the regression test would pass, but the same trigger would be
rejected when seeded into a software workspace at activation.
Fix: derive the test's known-set from `SoftwarePlaybookTriggers`
directly. If the schema widens or narrows, the test follows
automatically — no drift between the assertion baseline and the
actual schema. Also added a note clarifying that templates with
domain-specific trigger vocabularies (e.g. hiring's
on-candidate-advance) would need their own library scoped to
that template.
The set is now exactly: manual, on-implement, on-triage,
on-release, on-plan, on-review, on-deploy.
|
||
|
xarmian
|
8fa1dd36f9 |
refactor(library): archive 9 pre-PLAN-1377 playbook bodies, rebuild library as invokable-first (TASK-1403) (#532)
Retires the legacy trigger-only library entries from the public
surface and replaces the 4-category structure with a single
`agent-workflows` category housing the three invokable workflow
playbooks (ship, plan, decompose).
## Changes
- New file `internal/collections/playbook_library_archive.go` —
holds all 9 retired bodies in package-private `archivedPlaybooks()`.
Bodies stay compiled so they're greppable and refactor-safe;
per-entry "convert to invokable" / "promote to convention" /
"retire" decisions are tracked in IDEA-1396. `var _ = archivedPlaybooks`
keeps the symbol referenced for unused-symbol linters.
- `internal/collections/playbook_library.go::PlaybookLibrary()` —
removed the 4 categories (workflow, planning, quality, operations)
and the 9 bodies inline. Replaced with a single `agent-workflows`
category containing ship + plan + decompose (the invokable trio
landed in T3/T4/T5). All three carry InvocationSlug and Arguments
so the library teaches the PLAN-1377 invocation model from the
first card.
- `internal/collections/playbook_library_plan.go` /
`playbook_library_decompose.go` — bump each helper's `Category`
field from `workflow` to `agent-workflows` to match the new
registry grouping.
- `internal/collections/templates.go::softwareStarterPlaybookTitles` —
updated from the retired pair ("Implementation Workflow", "Code
Review Process") to the new invokable pair ("Plan a new
initiative", "Decompose a plan into tasks"). `startup` template
separately prepends `ship` (templates.go:~441), so every software
workspace now seeds the full invokable trio from day one.
- `internal/mcp/dispatch_http_slice4_test.go` — the activate-by-title
fixture used "Implementation Workflow"; switched to "Ship tasks"
(still a real library entry with trigger+scope in its activation
payload). T6's verify section flagged this fixture; addressing it
here keeps the build green on this branch rather than deferring
the breakage to T7.
- `cmd/pad/main.go` — `pad library activate --help` example used
"Implementation Workflow" as a sample title; switched to "Ship
tasks" so the example still resolves.
## Verify
- `go build ./...` clean (no dangling references to the 9 titles in
production code paths).
- `go vet ./...` clean.
- `go test ./...` — all packages green.
- `grep -r "Implementation Workflow" --include="*.go" --include="*.ts"
--include="*.svelte"` returns only:
- `playbook_library_archive.go` (expected — the archive)
- `playbook_library_plan.go` (historical comment, intentional)
- `templates.go:868` (demo-workspace seed item content — unrelated
to library lookup; literal item title in a template, would not
benefit from being retitled in this PR's scope)
Pre-existing workspaces' already-activated copies of the 9 entries
keep working — they live in workspace data, not library code. Only
future activations are affected (the legacy titles no longer resolve
via `pad library activate` or the web Library UI).
Parent: PLAN-1397. Depends on T3/T4/T5 — the library is never empty
because ship, plan, and decompose are already in place.
|
||
|
xarmian
|
8eb927cbbc |
feat(library): author the decompose invokable playbook (TASK-1402) (#531)
New library entry: `/pad decompose <PLAN-ref>` — takes a plan and creates the child task items its body implies, with the user's approval at every step. The natural follow-up to `/pad plan`: that playbook creates the plan; this one turns the breakdown into actionable items linked back via --parent. Code shape follows the templates_startup_ship.go pattern: - internal/collections/playbook_library_decompose.go (new) — holds decomposePlaybookBody + decomposePlaybookArguments + a DecomposePlaybook() helper. - internal/collections/playbook_library.go — registers DecomposePlaybook() in the workflow category right after PlanPlaybook(). Arguments: - target (required, ref) — the plan to decompose - dry-run (flag, default=false) — propose without creating - collection (optional, string, default=tasks) — for non-default workspaces (bugs, work-items, etc.) Body walks: load plan + existing children → analyze the body for actionable work → reconcile against existing children → propose task list with priorities and dependency hints → confirm in bulk → create approved tasks → wire dependencies via `pad item block` → report with suggested next moves. Mirrors skills/pad/SKILL.md's "Decomposition: 'Break plan X into tasks'" workflow so the conversational and invokable forms stay in lockstep (T7 covers SKILL.md sync). Closes the loop opened by T4: `/pad plan` step 7 now has a real `decompose` playbook to delegate to when it's activated in the workspace. Parent: PLAN-1397. |
||
|
xarmian
|
ad3926f643 |
feat(library): author the plan invokable playbook (TASK-1401) (#530)
* feat(library): author the plan invokable playbook (TASK-1401)
New library entry: `/pad plan <topic>` — a conversation-first
playbook for co-designing a new plan with the user. Subsumes the
pre-PLAN-1377 "Plan Creation" library entry, reframed as a
structured invokable procedure with explicit arguments.
Code shape follows the templates_startup_ship.go pattern:
- internal/collections/playbook_library_plan.go (new) — holds the
body + argument spec as package-level constants and exports
PlanPlaybook() LibraryPlaybook.
- internal/collections/playbook_library.go — registers
PlanPlaybook() in the workflow category right after ship.
Arguments (mirroring the body's `## Arguments` section):
- topic (required, string)
- parent (optional, ref)
- collection (optional, string, default=plans)
Tone is generic across project types — no software-only vocabulary,
no Pad-specific assumptions. A research workspace can plan an
experiment; a hiring workspace can plan a recruiting push; the
structure adapts via the conversation. Mirrors the "Planning: 'Let's
create a plan'" workflow in skills/pad/SKILL.md so the conversational
and invokable forms stay in lockstep (T7 docs pass calls this out).
The body's step 7 invites the user to follow up with
`/pad decompose <new-plan-ref>` — T5 (TASK-1402) authors that
playbook next.
Parent: PLAN-1397.
* fix(library): plan playbook handles ref-form quick-action invocation, softens /pad decompose forward-reference per Codex review (round 1)
Round 1 P2: /pad decompose forward-reference in step 7 would resolve to
an unknown playbook until T5 (TASK-1402) merges. Reworded step 7 to
check whether a `decompose` playbook is activated and fall back to
inline `pad item create task` calls otherwise. Body now works
regardless of whether T5 has landed.
Round 1 P3: existing UI quick-action prompt (defaults.go:157) emits
`/pad plan {ref} "{title}" — outline goals, deliverables, and
timeline` on plan-collection cards. With my original topic-first arg
shape, the agent's NL dispatcher would bind the ref as `topic` and
treat the title as an extra positional, getting the wrong invocation
mode.
Fix: documented the dual-purpose `topic` argument explicitly. New
"## Dispatch" section in the body tells the agent to detect:
- First positional matches `^[A-Z]+-\d+$` AND resolves to a real
item → elaborate mode (load item, work with user to expand it,
update via `pad item update <ref> --stdin`)
- Otherwise → create-new mode (full conversation flow below)
planPlaybookArguments updated to mirror: topic now described as
"string OR ref", parent/collection noted as create-new-mode only.
Strict CLI parsing remains string-typed; the dual semantics are
agent-interpreted per PLAN-1377's design.
The existing "Plan this" quick-action prompt now routes correctly
through the elaborate path without needing changes to defaults.go.
* fix(library): align plan playbook philosophy section with step 7 fallback per Codex review (round 2)
Round 2 P3: Philosophy said "never create tasks here" which
contradicted step 7's new inline-task-creation fallback (added in
round 1 to handle the case where no decompose playbook is active).
Reworded to permit the fallback explicitly while keeping the
"don't create before user approval" rule intact.
|
||
|
xarmian
|
8efcc21ef5 |
feat(library): add ship playbook to library, shared body with startup seed (TASK-1400) (#529)
The headline invokable example from PLAN-1377 (`/pad ship <args>`) was previously only seeded into `startup`-template workspaces. Library discovery is the canonical place users learn what invokable playbooks are, so `ship` now lives there too — available to any workspace template (`scrum`, `product`, `hiring`, `interviewing`, future ones). Single source of truth: the library entry references the same package-level `shipPlaybookBody` + `shipPlaybookArguments` constants that ShipPlaybook() uses. Updates to the playbook body live in one place; both surfaces (library activation + startup seed) consume it. The title "Ship tasks" matches ShipPlaybook()'s SeedPlaybook so the library UI's title-keyed `activePlaybookTitles` check renders the entry as "Active" in startup workspaces where it's already seeded — no duplicate activation, no behavior change. Placement: top of the existing `workflow` category. T6 will rearrange the category structure when retiring the legacy entries; for now adding to workflow is the least-intrusive prominent placement. Smoke-tested locally that ship resolves through PlaybookLibrary() with the expected title, trigger, args count, and shared content; the permanent regression test ships in T7. Parent: PLAN-1397. |
||
|
xarmian
|
4bbd0a210d |
feat(collections): widen LibraryPlaybook with InvocationSlug + Arguments (TASK-1398) (#527)
* feat(collections): widen LibraryPlaybook with InvocationSlug + Arguments (TASK-1398) Adds two optional fields to LibraryPlaybook: - InvocationSlug — kebab-case slug for `/pad <slug>` routing (PLAN-1377) - Arguments — argument spec mirroring the body's `## Arguments` section Both fields are tagged with `omitempty` so existing library entries (none of which set them) serialize unchanged. seedPlaybookFromLibrary() now forwards both into the seeded item's Fields JSON only when set, matching the shape ShipPlaybook() already writes. This is the foundational task that unblocks T2 through T6 of the playbook library overhaul. Parent: PLAN-1397. * fix(library): propagate invocation_slug + arguments through activation paths per Codex review (round 1) Codex round 1 caught that the activation paths for library playbooks rebuild the fields map and drop the new fields, so any library entry declaring invocation_slug/arguments would lose `/pad <slug>` routing after activation. Fixed in three places: - internal/cli/client.go LibraryPlaybook (the client-side mirror used by the CLI `pad library activate` command) - cmd/pad/main.go libraryActivate (CLI subprocess activation) - internal/mcp/dispatch_http_slice4.go dispatchLibraryActivate (MCP pad_project action=library-activate) All three now forward invocation_slug and arguments only when set, matching ShipPlaybook()'s shape exactly. Web client (web/src/lib/api/client.ts) activation payload is T2's explicit scope — left for that PR. |
||
|
xarmian
|
9764b2fe92 |
docs: document playbook invocation surface (TASK-1387) (#525)
* docs: document playbook invocation surface (TASK-1387)
Closes out PLAN-1377 — Make Playbooks first-class invokable procedures —
by bringing the four user-facing docs surfaces up to date with the
shipped invocation model. The pad-web docs ship in a separate commit
(../pad-web@main: docs: document playbook invocation surface).
- CLAUDE.md — new Playbooks section after Data Model covering the
three invocation surfaces, the invocation_slug/arguments schema
fields, bootstrap-returns-metadata, the seeded ship playbook, the
web UI editor, and a code map. MCP section grows pad_playbook,
pad://workspace/{ws}/bootstrap, and the pad_set_workspace embedded
response note.
- skills/pad/SKILL.md — adds a "Creating a playbook" subsection under
natural-language routing with CLI examples for trigger-only and
slug-invocable playbooks, plus a Playbooks block in the CLI
reference (pad playbook list/show/run with parsing rules).
- README.md — one-line bump in the feature list mentioning the new
/pad <slug> invocation form and the seeded ship playbook.
Parent: PLAN-1377.
* fix(docs): correct bootstrap route + CLI arguments authoring per Codex review (round 1)
Codex round 1 findings:
[P2] CLAUDE.md cited GET /api/v1/workspaces/{ws}/bootstrap but the
implemented route is /api/v1/workspaces/{ws}/agent/bootstrap (server.go
line 1182). Documented endpoint would 404 for HTTP integrators.
[P2] SKILL.md '--field arguments=[...]' example would fail validation
— pad item create stores all --field values as strings, while
arguments is a json field type. Rewrote the slug-invocable-playbook
authoring guidance to direct agents at the web UI editor for
structured argument authoring (the canonical path the editor was
built for) with the CLI handling everything else. Same fix applied
to the pad-web /docs/agent-integration page in a separate
../pad-web@main commit.
Parent: TASK-1387 / PLAN-1377.
* fix(docs): use full /{username}/{workspace}/playbooks route in SKILL.md per Codex review (round 2)
Codex round 2 finding:
[P3] SKILL.md's recommended web editor path was '/{workspace}/playbooks',
but the SvelteKit route is '/{username}/{workspace}/playbooks'. The
prior path would 404 or land on the wrong workspace. Fixed.
The pad-web docs ship the matching fix in a separate commit at
../pad-web@main: docs(playbooks): use full /{username}/{workspace}
route path per review.
Parent: TASK-1387 / PLAN-1377.
* fix(docs): bump CLAUDE.md MCP tool surface to v0.3 + close SKILL.md backtick per Codex review (round 3)
Codex round 3 findings:
[P2] CLAUDE.md still labelled the tool surface as v0.2; internal/mcp/
version.go advertises ToolSurfaceVersion = '0.3' (since PLAN-1377 /
TASK-1380). Updated to v0.3 and added a note about what v0.3 introduced
(pad_meta.action: bootstrap, pad_set_workspace embedded-bootstrap
response, pad://workspace/{ws}/bootstrap resource).
[P3] SKILL.md's web-editor route had the parenthetical inside the
code span: '`/{username}/{workspace}/playbooks (click "+ New
Playbook")`' — closed the backtick after '/playbooks' so the
rendered code span is the literal path.
The pad-web docs ship the matching v0.3 bump in a separate commit at
../pad-web@main: docs(mcp/tools): bump tool surface to v0.3.
Parent: TASK-1387 / PLAN-1377.
* fix(docs): bump stale MCP catalog references to v0.3 per Codex review (round 4)
Codex round 4 finding [P2]:
Three places still described the MCP catalog as v0.2, contradicting the
v0.3 surface block that landed in this PR:
- CLAUDE.md 'MCP server' lede paragraph — bumped to v0.3, added
pad_playbook to the listed tools, and noted what v0.3 introduced.
- skills/pad/SKILL.md MCP note for MCP-using agents — bumped to v0.3,
added pad_playbook to the listed tools and called out the playbook
invocation surface + bootstrap action.
- README.md 'Tool catalog (v0.2)' block — bumped to v0.3, added the
pad_playbook row, the pad_meta.action: bootstrap row, the bootstrap
resource, and bumped tool_surface_version.
Parent: TASK-1387 / PLAN-1377.
* fix(docs): bump MCP server-side self-description to v0.3 per Codex review (round 5)
Codex round 5 finding [P2]:
Two MCP-server-internal documentation surfaces still advertised v0.2:
- internal/mcp/instructions.md — the markdown blob the server returns
to MCP clients as initialization instructions. Updated 'Tool surface
(v0.2) / Eight tools' to 'Tool surface (v0.3) / Nine tools', added
pad_playbook with list/get/run, added bootstrap to pad_meta's actions,
noted pad_set_workspace's embedded-bootstrap response, and added
pad://workspace/{ws}/bootstrap to the resource list.
- internal/mcp/catalog_meta.go — the pad_meta tool's Description string
said 'v0.2 tool catalog' twice. Bumped both to v0.3.
These ship inside the binary; MCP clients read them directly so v0.2
mentions there contradict the v0.3 catalog the handshake actually
advertises (ToolSurfaceVersion in version.go).
Parent: TASK-1387 / PLAN-1377.
* fix(docs): finish MCP self-description v0.3 cleanup per Codex review (round 6)
Codex round 6 findings [P3]:
[1] catalog_meta.go's padMetaTool block-comment said 'Three actions,
all handled inline' even though bootstrap (the v0.3 fourth action)
dispatches through env.Dispatch. Fixed both the count and the
dispatch description, added the bootstrap row to the action list.
Also corrected the v0.2 mentions in actionMetaToolSurface's comment
and removed the rollout-era language now that the cmdhelp walker is
retired.
[2] instructions.md said 'Nine tools, each with an action enum' but
pad_set_workspace doesn't take an action. Clarified the count as
'eight resource × action tools, plus pad_set_workspace (which takes
a workspace slug only)' and scoped the 'Always pass action' rule to
the eight resource × action tools.
Parent: TASK-1387 / PLAN-1377.
* fix(docs): finish MCP self-description nine-tool wording per Codex review (round 7)
Codex round 7 findings:
[1] catalog_meta.go's padMetaToolDescription still mentioned the
PLAN-969-rollout cmdhelp walker contributing to tools/list. The walker
was retired in TASK-981. Rewrote the tool-surface action description
to match current behavior and explicitly note pad_set_workspace is
registered separately (not enumerated by tool-surface).
[2] actionMetaToolSurface's comment claimed scope includes
pad_set_workspace; the impl only loops env.Catalog. Updated the
comment to be accurate — tool-surface enumerates the eight catalog
tools only, callers should account for pad_set_workspace as a known
extra.
[3] CLAUDE.md and README.md described the MCP surface as if every
listed tool was resource × action. pad_set_workspace takes
'workspace' only. Reworded both to match instructions.md's
'eight resource × action tools plus pad_set_workspace' framing.
Parent: TASK-1387 / PLAN-1377.
|
||
|
xarmian
|
936145c524 |
feat(collections): seed generic ship playbook in startup template (TASK-1386) (#523)
* feat(collections): seed generic ship playbook in startup template (TASK-1386)
Adds the de-personalized `ship` playbook to the startup template's seeded
playbooks slice, the headline example of PLAN-1377's playbook invocation
model. A fresh `pad workspace init --template startup` workspace now ships
PLAYB-N with invocation_slug=ship, ready to drive via `/pad ship <args>`
or `pad playbook run ship`.
Body is derived from the personal /ship-tasks slash command, with
project-specific bits (Codex CLI, branch naming, commit format, build
commands) marked customizable so the playbook reads cleanly on day one
without forcing the seed user to wire two playbooks together to ship
anything. ## Arguments section in the body mirrors the structured
arguments JSON field so the web UI editor (TASK-1384) can keep them
in sync.
Parent: PLAN-1377.
* fix(collections): align ship playbook target/limit with strict parser per review (round 1)
Codex round 1 findings:
1. target docstring said "space or comma separated" task refs, but the
structured spec only has one positional 'target'. The strict CLI/MCP
parser would bind TASK-10 then reject TASK-11. Updated body to:
- declare 'comma-separated list' in the ## Arguments line
- note that the agent's NL parser collapses space-separated refs to
comma form before binding (so '/pad ship TASK-10 TASK-11' still works)
2. limit said 'default=∞' in markdown but had no default in the structured
arguments JSON. Removed the bogus default — limit is now genuinely
optional with 'unset = no limit', so the markdown and the spec agree.
Parent: TASK-1386 / PLAN-1377.
|
||
|
xarmian
|
9607139340 |
feat(mcp): add pad_playbook tool (list/get/run) (TASK-1381) (#521)
* feat(mcp): add pad_playbook tool (list/get/run) (TASK-1381)
PLAN-1377 T4 — exposes the playbook surface from TASK-1382 via MCP.
Three passThrough actions match the CLI:
- pad_playbook.list → pad playbook list (metadata catalog)
- pad_playbook.get → pad playbook show <ref> (full body + fields)
- pad_playbook.run → pad playbook run <ref> (parse + bind args,
return body. Side-effect-free; the agent
executes the steps, not the server)
Params advertised in the tool schema: ref (required for get/run),
args (pre-parsed map — MCP / programmatic callers), and raw_args
(CLI-style tokens — strict parsing rules applied server-side via
ParsePlaybookCLIArgs from TASK-1382).
HTTP dispatcher route entries (dispatch_http_routes.go) wire the same
three actions into pad-cloud's in-process path:
GET /workspaces/{ws}/playbooks
GET /workspaces/{ws}/playbooks/{ref}
POST /workspaces/{ws}/playbooks/{ref}/run
The run mapper (mapPlaybookRun) JSON-encodes args + raw_args into the
POST body so the server-side parser fires with the same shape it gets
from the CLI.
ToolSurfaceVersion already 0.3 (TASK-1380). This adds a tool but
existing actions are unchanged, so no further bump is needed.
Tests: catalog_readonly_test.go's bijection and dispatch tests
extended with pad_playbook entries (both `expected` maps + the
liveCmdhelpDoc stub). The actions-match-cmdhelp + dispatch-cmdpath
checks pass.
Parent: PLAN-1377.
* fix(mcp): align pad_playbook MCP shape with CLI cmdhelp (TASK-1381)
Codex round 1:
P1 — Renamed CLI Use strings from `show <slug|ref>` / `run <slug|ref> ...`
to plain `show <ref>` / `run <ref> [args...]`. The pipe-alternation
form makes cmdhelp synthesize the arg name as "value"; local stdio MCP
calls were failing with missing "value" because the tool param is
"ref". The Long descriptions still explain the resolver accepts
invocation_slug / item slug / issue ref.
P1 — pad_playbook.action=run is now a custom action handler that
flattens the structured `args` map + `raw_args` slice into the CLI's
positional/flag/kv token sequence before dispatching. Without this the
passThrough path dropped args/raw_args (they aren't cmdhelp args/flags),
making the local-stdio invocation a no-op from the agent's POV. Sort
order is deterministic for test replay stability.
P2 — raw_args type changed from "array" to "array<string>" so the
catalog builder's paramDefToToolOption recognizes it as a string array
instead of falling through to the WithString default. Without this MCP
advertised a string but the mapper expected a slice.
Test cmdhelp stub updated: playbook run now declares "ref" + variadic
"args" positionals, matching the new Use string.
Parent: PLAN-1377.
* fix(mcp): mapPlaybookRun accepts both flattened + map args (TASK-1381)
Codex round 2 HIGH: actionPlaybookRun's flattened input shape
({args: []string}) confused mapPlaybookRun, which expected args as a
map. Cloud/HTTP MCP calls were posting {"args":["PLAN-7"]} and the
server tried to decode that as map[string]any.
mapPlaybookRun now coerces all the shapes both dispatch paths produce:
- args as map → forwarded verbatim as the pre-parsed dictionary.
- args as []string / []any → treated as raw_args (CLI tokens).
- raw_args (any case form) → appended to the raw_args list.
This keeps env.Dispatch the single dispatch entry point on the action
side while letting the HTTP mapper translate freely.
Parent: PLAN-1377.
* fix(cli): use [args]... ellipsis-outside-brackets so cmdhelp parses arg name (TASK-1381)
Codex round 3 HIGH: cmdhelp's argRE bakes the ellipsis into the arg
NAME when it appears inside the brackets — `[args...]` parses as
Arg{Name: "args...", Repeatable: false}, not Arg{Name: "args",
Repeatable: true}. That made BuildCLIArgs (used by ExecDispatcher
for local stdio MCP) drop the playbook argument tokens because the
input map key "args" didn't match the cmdhelp positional name
"args...".
The fix is to move the ellipsis OUTSIDE the brackets per the cmdhelp
spec: `[args]...`. Code comment cross-references cmdhelp/json.go::argRE
so future Use-string editors don't regress.
Parent: PLAN-1377.
* fix(mcp): preserve structured args on HTTP dispatch path (TASK-1381)
Codex round 4 MEDIUM: actionPlaybookRun's flatten step dropped explicit
`false` values on flag-typed args, so an MCP call like
{args: {stop-after-each: false}} couldn't override a flag default of
true.
Fix: dispatcher-type-aware branching.
- HTTPHandlerDispatcher path: forward input as-is. mapPlaybookRun
preserves the structured args map (including explicit false values),
and the server's bindPlaybookArgs sees the override correctly.
- ExecDispatcher path: flatten args + raw_args into CLI tokens as
before. The CLI's strict parser only supports bareword flag
PRESENCE, so the flag=false override is a documented local-stdio
limitation; route through HTTP/in-process MCP for that rare case.
Function docstring now spells out the two paths and the CLI
limitation so the next reader doesn't have to reverse-engineer it.
Parent: PLAN-1377.
* fix(mcp): bypass BuildCLIArgs on HTTP playbook-run dispatch (TASK-1381)
Codex round 5: even with the dispatcher-type branch from round 4,
env.Dispatch still ran BuildCLIArgs FIRST and only forwarded to the
chosen dispatcher AFTER. BuildCLIArgs choked on args:map (the cmdhelp
positional 'args' wants strings) and returned a validation_failed
result before mapPlaybookRun ever saw the structured input.
Fix: when dispatching to HTTPHandlerDispatcher, attach the input map
to context manually via WithDispatchInput and call the dispatcher
directly, skipping BuildCLIArgs. ExecDispatcher path unchanged — it
still uses env.Dispatch with the flattened CLI tokens because the
local CLI needs them.
Parent: PLAN-1377.
* fix(mcp): use structured validation envelope for missing ref (TASK-1381)
Codex round 6 P3: actionPlaybookRun's missing-ref error returned a
plain text result, breaking the structured-envelope contract that
every other validation error in the catalog follows. Switch to
NewErrorResult/ErrorPayload so agents can branch on error.code.
Codex's round-6 P2 (read-scope tokens blocked from POST /playbooks/{ref}/run
because the middleware requires GET/HEAD/OPTIONS) is real but
out-of-scope for this PR — it touches the auth scope model and
deserves a dedicated HT item rather than a snap fix here. Filed as
follow-up. The action is still functional for any token with 'write'
scope, which is the default for local-stdio MCP and pad-cloud
deployments.
Parent: PLAN-1377.
|
||
|
xarmian
|
fed4b60e65 |
feat(playbook): add pad playbook CLI (list/show/run) + endpoints (TASK-1382) (#520)
* feat(playbook): add pad playbook CLI (list/show/run) + endpoints (TASK-1382)
PLAN-1377 T5 — first-class invokable-procedure surface. Three HTTP
endpoints + three CLI subcommands, with a strict CLI arg parser and a
side-effect-free run path.
Endpoints
GET /workspaces/{ws}/playbooks — metadata array, same
projection as bootstrap
GET /workspaces/{ws}/playbooks/{ref} — full item, resolved by
invocation_slug | ref | slug
POST /workspaces/{ws}/playbooks/{ref}/run — bind args, return body +
bound + unbound. No
execution; the agent
owns step playback.
Resolution
resolvePlaybook walks invocation_slug first (the /pad <slug>
user-facing identifier), then falls back to ResolveItem (UUID / ref /
slug). A stray TASK-5 hitting /playbooks/TASK-5 returns 404 instead
of leaking a non-playbook into the surface.
Arg parsing
ParsePlaybookCLIArgs implements the strict rules from PLAN-1377:
- Required positional args first, in declared order.
- Flag types: bareword presence sets true.
- Other types: key=value form (number is parsed as float64,
enum is validated against declared options).
The server takes either pre-parsed args (MCP / programmatic callers)
OR raw CLI tokens (CLI caller); merge logic prefers explicit args
over raw_args. The CLI sends raw_args, so there is one parser
implementation and no drift risk.
CLI
pad playbook list — table or json
pad playbook show <slug|ref> [--format] — markdown / json
pad playbook run <slug|ref> [args...] — body + bound args
Client method
cli.Client.{ListPlaybooks, ShowPlaybook, RunPlaybook(args,
rawArgs)} — runtime callers pass args; CLI passes rawArgs.
Tests
TestPlaybookList, TestPlaybookShowByInvocationSlug,
TestPlaybookShowByRef, TestPlaybookShowRejectsNonPlaybook,
TestPlaybookRunBindsArgs (with-args, missing-required, with-raw-args),
TestParsePlaybookCLIArgsErrors, TestPlaybookListEmptyShape.
Parent: PLAN-1377.
* fix(playbook): tighten arg parsing per Codex round 1 (TASK-1382)
P2.1 — Positional binding now skips optional and flag-typed slots.
The PLAN-1377 contract says ONLY required args fill positional slots;
other typed args must be key=value. Without this, a spec with an
optional arg before a required one (e.g.
[merge-strategy?, target!]) bound the caller's bareword TASK-7 to
merge-strategy instead of target.
P2.2 — number coercion now uses strconv.ParseFloat with NaN/Inf
rejection. Sscanf(%g) was sloppy: it accepted '1abc' as 1 (partial
match) and accepted NaN/Inf, which json.Marshal then refused after
the handler had already written a 200 header.
P3 — empty-body run requests now decode cleanly. The decodeJSON
wrapper folds io.EOF into 'invalid JSON: EOF' so the previous
err.Error() != "EOF" check never fired. errors.Is(err, io.EOF) on
the unwrapped chain handles the wrapping correctly.
Tests: TestPlaybookRunAcceptsEmptyBody,
TestParsePlaybookCLIArgsOptionalNotPositional,
TestCoercePlaybookValueNumberRejectsBadInput.
Parent: PLAN-1377.
|
||
|
xarmian
|
73208bf9d3 |
feat(mcp): expose AgentBootstrap via three MCP surfaces (TASK-1380) (#519)
* feat(mcp): expose AgentBootstrap via three MCP surfaces (TASK-1380)
PLAN-1377 T3: Expose the bootstrap blob (from TASK-1379) via the three
MCP surfaces the agent specs name. One canonical builder
(Server.BuildAgentBootstrap), three discovery paths.
Surfaces
1. Resource: pad://workspace/{ws}/bootstrap. Hosts that prefetch
resources at session start (Claude Desktop, Cursor) get full
context cheap. readBootstrap shells out to `pad bootstrap`.
2. Tool action: pad_meta.action=bootstrap. Mid-session refresh for
agents that didn't get the resource prefetch or want a fresh
snapshot after lots of mutations. Pass-through to `pad bootstrap`
via env.Dispatch — same source of truth.
3. pad_set_workspace response embed: when a BootstrapFetcher is wired
in (production has one via ExecBootstrapFetcher), the response
payload extends from {workspace, status} to {workspace, status,
bootstrap}. One call hands the agent full session context the
moment they switch workspaces. Purely additive — older clients
that ignore unknown keys keep working.
Plumbing
- New BootstrapFetcher interface + ExecBootstrapFetcher impl that
shells out to `pad bootstrap --workspace <ws> --format json` with
RootArgs (e.g. --url) preserved.
- RegistryOptions.BootstrapFetcher (optional) — cmd/pad/mcp.go wires
ExecBootstrapFetcher; tests pass nil and get legacy shape.
- pad_meta.Schema.Workspace flipped to true so the workspace param is
available to the bootstrap action. server-info / version /
tool-surface ignore it as before.
- ToolSurfaceVersion bumped 0.2 → 0.3 with detailed changelog in the
const doc-comment. Bumps are additive but rename pad_set_workspace's
response shape, which is a breaking contract change for any client
that asserts the exact key set.
Tests
- TestSetWorkspaceTool_EmbedsBootstrap, _BootstrapErrorFallsThrough,
_EmptyWorkspaceSkipsBootstrap.
- TestPadMetaTool_NoWorkspaceInSchema rewritten as
TestPadMetaTool_WorkspaceInSchema with reasoning.
- TestCatalogWorkspaceParamAdvertisedOnAllWorkspaceTools updated:
pad_meta is no longer on the intentionallyServerWide list.
Parent: PLAN-1377.
* fix(mcp): wire bootstrap route into HTTP dispatcher (TASK-1380)
Codex round 1: pad_meta.action=bootstrap dispatched cmdPath
['bootstrap'] but the HTTP MCP dispatcher's routeTable had no entry,
so pad-cloud and other HTTP-transport clients hit the 'not yet
implemented over HTTP transport' fallback. Add a route mapping that
GETs /api/v1/workspaces/{workspace}/agent/bootstrap — the canonical
endpoint Server.handleGetBootstrap exposes. Local stdio MCP is
unaffected (it dispatches via ExecDispatcher, which shells out to
`pad bootstrap`).
Parent: PLAN-1377.
|
||
|
xarmian
|
24f0445efa |
feat(bootstrap): single-roundtrip /pad context-load endpoint (TASK-1379) (#518)
* feat(bootstrap): single-roundtrip /pad context-load endpoint (TASK-1379)
Implements the agent bootstrap surface for PLAN-1377. One HTTP call
replaces the four separate /pad context-loading invocations (workspace
+ collections + conventions + roles + playbooks) the skill used to
make, cutting ~200-400ms of startup latency on every /pad command.
Wire shape:
- GET /api/v1/workspaces/{ws}/agent/bootstrap returns AgentBootstrap:
workspace { slug, name, id }, user { name, email, id },
collections [...], conventions [...always-on, status=active],
roles [...], playbooks [metadata-only — no bodies], dashboard {...},
recent_activity [... 24h].
- pad bootstrap [--format json|markdown] CLI wrapper. JSON is the
canonical wire format that the /pad skill consumes; markdown is a
human-readable summary for quick terminal inspection.
Implementation notes:
- Single source of truth: Server.BuildAgentBootstrap. The HTTP handler
is a thin wrapper, and TASK-1380 will reuse it from three MCP
surfaces (resource + set_workspace embed + pad_meta tool action).
- Dashboard reuse: handleGetDashboard's body extracted into
buildDashboardResponse(workspaceID, r) returning (*DashboardResponse,
error). The HTTP handler is now a thin wrapper; bootstrap calls the
builder directly to embed dashboard data without a second roundtrip.
- Playbook bodies are deliberately NOT included — metadata only
(~80 bytes/entry vs 5-10KB) so the bootstrap stays small for
workspaces with many playbooks. Full bodies load on invocation.
- Convention bodies ARE included for the always-on/active subset (must
be agent-known up front); trigger-specific conventions stay
load-on-demand.
- Empty slices serialize as [] not null so the agent doesn't need
defensive nil checks.
Tests:
- TestBootstrapEmptyWorkspace, TestBootstrapEmptyArraysNotNull,
TestBootstrapIncludesPlaybookMetadata,
TestPlaybookSummaryPrefersFirstParagraph.
Parent: PLAN-1377.
* fix(bootstrap): respect collection visibility + guest grants (TASK-1379)
Codex round 1: BuildAgentBootstrap was bypassing visibility filters,
so a guest admitted by RequireWorkspaceAccess could read collections,
conventions, and playbook metadata they don't have access to.
Now mirrors handleListCollections + handleGetDashboard:
1. Resolve visibleCollectionIDs(r, workspaceID) once.
2. Filter the Collections array through isCollectionVisible.
3. Gate the conventions + playbooks sub-queries on whether the caller
can see those collections at all (presence in the filtered slice
implies visibility).
4. Empty slices for inaccessible sub-resources, so the wire shape
stays consistent — no missing keys to confuse the agent skill.
5. Pass-r=nil callers (future MCP in-process dispatchers) keep the
'full visibility' shortcut, but the doc comment now spells out that
those callers MUST verify access out-of-band.
Parent: PLAN-1377.
* fix(bootstrap): apply guest item-level grants + recompute role counts (TASK-1379)
Codex round 2:
P1 — Item-level guest grants now flow into the convention + playbook
sub-queries. visibleCollectionIDs alone admits the conventions /
playbooks collection if a guest has ANY item grant inside it; without
ItemIDs filtering, those queries then return the whole always-on
convention body set or every playbook's metadata. Now mirrors the
handleListItems shape: resolve (fullCollIDs, grantedItemIDs) via
guestResourceFilter, and pass the (collIDs, itemIDs) tuple through to
collectAlwaysOnConventions + collectPlaybookMetadata so a guest with a
single grant only sees that one item.
P2 — AgentRole.ItemCount is now recomputed from the visible item set
for restricted callers, matching handleListAgentRoles. Without this,
a guest could read role counts computed across all workspace items and
infer hidden activity per role.
Helper signatures updated to accept (collIDs, itemIDs); the doc
comments explain the nil/non-nil semantics so future callers can't
silently regress this.
Parent: PLAN-1377.
* fix(bootstrap): rewrite collection counts from visible set for guests (TASK-1379)
Codex round 3: Collection.item_count + active_item_count are computed
across the whole collection by ListCollections, so a guest with one
item grant in a collection still received the collection in the
filtered list but with hidden counts. Reuse the visible item set
(already computed for role counts) to recompute collection item_count
for restricted callers. active_item_count is set equal to item_count
to avoid a separate done-rules buildup the bootstrap consumers don't
depend on — better a self-consistent number than a leaked one.
Parent: PLAN-1377.
|
||
|
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
|
bd8667eadf |
feat: seq-stamped SSE events + stale-event short-circuit (TASK-1358) (#505)
* feat: seq-stamped SSE events + stale-event short-circuit (TASK-1358)
## Server
- events.Event gains a `Seq int64` field (omitempty). Server populates
it on item lifecycle events so SSE consumers can reason about
ordering and contiguity against their /items-changes cursor.
- publishItemEventWithName takes seq as a parameter; all call sites
in handlers_items.go pass the item's current seq:
- item_created → item.Seq
- item_updated → updated.Seq
- item_archived → re-fetched via GetItemIncludeDeleted (DeleteItem
bumps seq but doesn't return the updated row)
- item_restored → restored.Seq
- move target → moved.Seq
## Web
- ItemEvent gains optional `seq` (matches the server's omitempty).
- localIndex.classifySSEEvent(ws, event) returns
'no-seq' | 'stale' | 'contiguous' | 'gap'. The collection page
uses this to short-circuit duplicate / replayed events the
server's replay buffer re-delivers after tab-resume. Non-stale
events still call deltaSync because the SSE wire payload only
carries metadata, not the row data; classify does NOT advance
the cursor (applyDelta with real row data is the only path that
does — preserving the IDB invariant from TASK-1356).
Parent: PLAN-1343.
* fix(events): version-restore item_updated event carries seq (Codex round 1)
Version-restore in handlers_item_versions.go was publishing the
item_updated event directly via events.Publish without Seq, bypassing
the new seq-stamped SSE contract from TASK-1358. localIndex's
classifySSEEvent would always return 'no-seq' for those events,
forcing a generic /items-changes refetch instead of allowing the
stale/gap fast paths.
Now passes updated.Seq from the store response.
Parent: PLAN-1343.
|
||
|
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
|
bdcb62e902 |
fix(api): accept nested object/array for PATCH items fields/tags (BUG-1144) (#485)
The PATCH /api/v1/workspaces/{ws}/items/{ref} endpoint previously
demanded `fields` and `tags` arrive as JSON-encoded strings, because
models.ItemUpdate declares them as *string to mirror the storage shape.
Sending the natural nested-object shape any reasonable HTTP client
would produce returned HTTP 400 with a leaked Go unmarshal error
naming the internal struct field — confusing for anyone integrating
against Pad over HTTP (webhook reactors, custom dashboards, non-CLI
agents, third-party MCP bridges).
This is the symmetric input-side counterpart to BUG-991, which was
fixed at the MCP boundary in PR #364 with dual-emit normalization
rather than the full Plan-sized models.Item migration.
Fix: add a custom ItemUpdate.UnmarshalJSON that accepts either shape
on the wire and normalizes to the canonical string internally. The
struct field type stays *string, so the validation/storage/web/CLI
pipeline is untouched. All in-process Go callers construct ItemUpdate
literals (15 grepped call sites) and never hit UnmarshalJSON, so the
change is invisible to them.
Wrong shapes (e.g. `{"fields":42}`, `{"tags":{"x":1}}`) now return a
domain-level 400 — `"fields" must be a JSON object or a JSON-encoded
string` — surfaced via sentinel errors (ErrInvalidFieldsType /
ErrInvalidTagsType) that the handler unwraps from decodeJSON's
"invalid JSON: %w" wrapper.
Coverage:
- models/item_test.go: 10 sub-tests covering object, array, string,
null, absent, and wrong-type cases for both fields and tags.
- server/handlers_items_test.go: 6 PATCH integration sub-tests
asserting back-compat, the BUG-1144 repro now returns 200, and
that error responses no longer leak Go struct field names.
Smoke-tested against the live server with the exact repro curl from
BUG-1144 (HTTP 200), plus malformed (HTTP 400 with clean message)
and stringified-string back-compat (HTTP 200).
|
||
|
xarmian
|
a28927c51e |
feat(mcp): add typed schema param to pad_collection.create (TASK-1335) (#483)
* feat(mcp): add typed schema param to pad_collection.create (TASK-1335)
TASK-1334 shipped the CLI --schema flag for BUG-1284. This wires the
same capability through the MCP surface so agents calling pad_collection
get a structured object parameter rather than a stringified DSL —
matching how every other Pad-native data structure (items, comments,
etc.) flows through MCP.
Three coordinated changes:
1. internal/mcp/catalog.go — add "object" Type to ParamDef, mapped to
mcp-go's WithObject. Generic addition; the schema param is the first
consumer.
2. internal/mcp/catalog_collection.go — declare schema (object) param
on pad_collection.create alongside the existing fields (string)
param. Tool description now points agents at schema for
terminal_options, defaults, computed fields, suffixes, and relation
collections (everything the DSL cannot express), and notes the
fields/schema mutual exclusion.
3a. internal/mcp/dispatch.go — BuildCLIArgs now JSON-encodes
non-string structured values via a new encodeFlagValue helper. The
ExecDispatcher path (`pad mcp serve`) hands MCP input through to
the CLI's --schema flag as inline JSON; previously fmt.Sprint(map)
produced garbage Go-syntax. Generic — applies to any future flag
that accepts JSON.
3b. internal/mcp/dispatch_http_routes.go — mapCollectionCreate accepts
`schema` (object or stringified JSON) alongside `fields`, errors
if both are set, and backfills missing field labels using
titleCaseLabel so HTTPHandlerDispatcher MCP clients see the same
rendering as CLI users.
Tests cover: object→JSON encoding in BuildCLIArgs, array→JSON encoding,
structured schema input round-trips through mapCollectionCreate with
terminal_options preserved, missing-label backfill, stringified-JSON
fallback shape, both-flags rejection, malformed-JSON rejection.
Parent: PLAN-1333.
* fix(mcp): normalize empty/null schema input to absent per Codex round 1
Codex flagged a transport mismatch: the CLI treats `--schema ""` as
absent (falls through to --fields), but the HTTP dispatcher treated
schema=null or schema="" as set, triggering the mutually-exclusive
guard against fields or an invalid-JSON error from unmarshal.
Fix: in mapCollectionCreate, normalize nil and empty/whitespace strings
to "schema absent" before the mutex check. Keeps both transports
symmetric so MCP clients sending optional-arg defaults (null/empty) get
identical behavior to CLI users passing --schema "".
Test: TestMapCollectionCreate_EmptySchemaFallsThroughToFields covers
nil, "", and whitespace-only inputs in subtests, asserting the request
body uses the --fields-parsed schema.
Parent: PLAN-1333 / TASK-1335.
|
||
|
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
|
fdc6b221d0 |
test(collab): regression guard for share-page collab isolation (TASK-1266) (#464)
The public /s/{token} share page must keep rendering markdown via
marked + DOMPurify and never open a WebSocket to /api/v1/collab —
anonymous viewers can't authenticate, and exposing per-item Y.Doc
traffic to the public internet would be a security regression.
This is a verification task: zero production code changes. Adds
TestSharePageDoesNotImportCollab in internal/server which:
- Walks the import closure rooted at the share-page +page.svelte
(resolves $lib/ aliases, relative paths, conventional extensions,
static AND dynamic `import(...)` forms; skips external packages)
- Asserts no file in the closure contains forbidden tokens:
wsProvider, CollabProvider, @tiptap/extension-collaboration,
@tiptap/y-tiptap, 'yjs' / "yjs", y-protocols, Editor.svelte
(suffix), WebSocket, /api/v1/collab
- Asserts the route file still imports + invokes marked and
DOMPurify.sanitize (catches a renderer swap)
- Strips comments before all checks so leftover commented-out
imports can't bypass
Verified by injection: direct AND transitive AND dynamic-import
regressions all fail the test.
Four rounds of Codex review.
|
||
|
xarmian
|
9b1a91ab00 |
feat(collab): 5s-idle + on-disconnect markdown flush (TASK-1260) (#458)
* feat(collab): 5s-idle + on-disconnect markdown flush (TASK-1260) Replaces the temporary handleContentUpdate suppression introduced in TASK-1259 (PR #457) with a proper flush mechanism. Under collab, the Y.Doc + op-log are canonical for live state but items.content needs to stay reasonably fresh for downstream consumers (search index, share-page, exports, plain API readers). ## Mechanism 1. **5s idle timer.** Every editor onUpdate (local OR remote) resets a 5s timer. On fire, PATCHes items.content via the new `?source=collab-snapshot` query param. 2. **Server-side bypass.** handleUpdateItem inspects the source query param. When set, skips the applyContentViaCollab routing entirely and writes directly. Without the bypass, the PATCH would loop back through the applier protocol (the same tab gets asked to apply, acks, server strips input.Content) and leave items.content unchanged forever. The flag is trustworthy because the caller already has edit access. 3. **Dedupe across peers.** Track lastFlushedContent. If our last successful flush already landed this exact markdown, skip the PATCH. Multiple connected tabs would otherwise each fire a redundant flush after every shared edit converges, multiplying server load by the peer count. 4. **On-disconnect flush.** $effect cleanup (item swap or page unmount) calls flushCollabNow(true) BEFORE provider.destroy(). A separate beforeunload listener catches close-tab / reload / external-nav. Both use fetch keepalive: true so the request outlives the page lifecycle. 5. **Item-id race guards.** runCollabFlush captures reqItemId before await; ignores response if item swapped. loadData() clears collabFlushTimer + lastFlushedContent on navigation. ## Files - internal/server/handlers_items.go — accept `?source=collab-snapshot` - web/src/lib/api/client.ts — add api.items.flushCollabContent - web/src/routes/.../[slug]/+page.svelte — handleContentUpdate gains scheduleCollabFlush + runCollabFlush + flushCollabNow; wired to $effect cleanup + beforeunload + loadData reset. Parent: PLAN-1248 * fix(collab): capture ws+itemId at provider mount + apply unescapeDocLinks per Codex review (round 1) Two findings from round 1: 1) [P1] runCollabFlush resolved item.id and wsSlug at execution time, not at schedule time. During item navigation the timer could fire (or $effect cleanup could run) AFTER `item` was already updated to the new item, causing the OLD editor's markdown to be PATCHed against the NEW item's URL — cross-item content corruption. Fix: introduce activeCollabContext = { wsSlug, itemId }, captured at $effect-body time (when the provider is minted). scheduleCollabFlush, runCollabFlush, and flushCollabNow all take their target identity from this captured context, never from live reactive state. Cleared in the $effect's own cleanup (defensive `=== ctx` slot guard so a fast-navigation churn doesn't clobber a successor context). 2) [P2] The disconnect flush read raw editor.storage.markdown .getMarkdown() without unescapeDocLinks, unlike the regular onUpdate path. Closing/navigating before the idle flush could persist escaped wiki links like \[\[TASK-1\]\] which then wouldn't be converted by markdownToWikiLinks. Fix: apply unescapeDocLinks() at the start of runCollabFlush (covers both the timer-driven idle path and the unmount path). * fix(collab): gate UI mutations on foreground+current-item per Codex review (round 2) [P2] runCollabFlush mutated page-scoped state (saveStatus, editorStore.lastSaveTime, lastFlushedContent) before checking whether the captured itemId still matches the foreground item. On item navigation, the cleanup-driven keepalive flush could stamp 'saving' onto the NEW page's saveStatus, leaving it pinned indefinitely (and pollute lastFlushedContent for the new item's dedupe state). Fix: introduce isForegroundCurrent() = !keepalive && item.id === itemId. Gate saveStatus / setLastSaveTime / showSaved on it so background cleanup flushes never touch UI state. Gate lastFlushedContent on item.id === itemId regardless of keepalive so a stale flush can't seed the wrong item's dedupe. * fix(collab): skip cleanup flush on rich→raw transition per Codex review (round 3) [P1] $effect cleanup fires the keepalive flushCollabNow on every provider teardown, including rawMode toggles. The raw-button onclick already pre-populated rawPendingMarkdown with the live editor markdown (which the 1.2s raw debounce will land), so the keepalive PATCH from cleanup is redundant — and worse, can land AFTER the raw save and clobber newer raw edits with the older Y.Doc snapshot. Fix: gate the cleanup flush on `!rawMode`. If rawMode is true at cleanup time, the user just toggled to raw and the raw-mode codepath owns items.content from here. The other cleanup triggers (item nav, canEdit flip, page unmount) all keep firing the flush as before. Note: rawMode === true at cleanup time unambiguously means "transitioning into raw" — the inverse case (already in raw and the cleanup fires for some other reason) is impossible because collabKey gates on !rawMode, so the provider $effect never runs while rawMode is true. * fix(collab): synchronously flush Y.Doc state on rich→raw toggle per Codex review (round 4) [P1] Rich → raw → navigate-without-typing-or-toggling-back never PATCHed the live Y.Doc state to items.content. The previous seed mechanism only set rawPendingMarkdown, which only fires the 1.2s debounce on a subsequent handleRawContentUpdate call — which never happens if the user doesn't type. Fix: await runCollabFlush(ws, itemId, md, true) inside the raw button's async onclick BEFORE flipping rawMode = true. This: - Lands items.content with the live Y.Doc state synchronously (one PATCH, awaited, with keepalive: true so it survives a fast post-toggle navigation). - Seeds lastFlushedContent so any cleanup-driven re-flush is deduped. - Avoids populating rawPendingMarkdown — the raw debounce now only fires for actual user edits in raw mode, eliminating the race where a stale debounce fired after navigation could clobber state. The Round 3 cleanup-skip on rawMode is kept as defense-in-depth (also makes the no-op-when-already-flushed semantics explicit). * fix(collab): loop-flush until stable + cancel timer on rich→raw toggle per Codex review (round 5) Two HIGH findings from round 5: 1) Round 4's single-flush captured md BEFORE the await; concurrent peer edits (e.g. same user's other tab) during the await were lost from the seed and could be overwritten by subsequent raw-mode saves. Fix: loop-flush until stable. Re-read editor markdown after each PATCH; if it changed, flush again. Capped at 3 iterations to bound the transition under aggressive concurrent typing. 2) An onUpdate during the await could schedule a 5s collab flush timer that survived the rawMode flip. The cleanup skipped flushCollabNow on rawMode, but the timer fired its own runCollabFlush — which then PATCHed stale rich markdown on top of subsequent raw saves. Fix: explicitly clearTimeout(collabFlushTimer) at the end of the rich→raw onclick (after the loop-flush, before flipping rawMode). Belt-and-braces with the Round 3 cleanup skip. * fix(collab): seed raw mode from lastFlushed (not unflushed Y.Doc) per Codex review (round 6) [HIGH] Round 5's loop-flush could exit at the 3-iteration cap with md still differing from the last-PATCHed value, then seed rawSeedMarkdown with that unflushed md. An immediate navigation without typing would lose the unpersisted state. Fix: track lastFlushed inside the loop. After the loop, seed rawSeedMarkdown = lastFlushed (the markdown we actually PATCHed), NOT md (potentially a never-flushed in-memory value). If peer edits keep arriving past our cap, items.content lags Y.Doc briefly — but the peer's own 5s flush will catch up shortly, and at least raw mode shows state consistent with items.content rather than holding a value the server never received. * fix(collab): three corner-case fixes per Codex review (round 7) 1) [HIGH] lastFlushed = md was set unconditionally inside the loop-flush, even when runCollabFlush returned false (PATCH failed). rawSeedMarkdown could then be seeded with markdown the server never received. Fix: gate `lastFlushed = md` on runCollabFlush returning true. Failed PATCHes leave lastFlushed at its prior value. 2) [HIGH] lastFlushedContent (the collab-flush dedupe key) was never invalidated by raw-mode direct saves. Scenario: collab flushes A. Raw saves B. User returns to rich + edits back to A. Next collab flush dedupes (lastFlushedContent === A) and skips, leaving items.content stuck on B. Fix: reset lastFlushedContent = null after every successful raw save (both the regular handleRawContentUpdate path and the flushRawIfPending drain loop) so subsequent collab flushes always re-PATCH. 3) [MEDIUM] The async rich→raw onclick applied rawSeedMarkdown + rawMode = true after multiple awaits without verifying the user was still on the same item. A navigation during the loop-flush could let item A's handler resume and seed raw mode on item B. Fix: before mutating component state (rawSeedMarkdown, rawMode), check `item?.id === itemId` (the captured target). Bail with `return` if mismatched. * fix(collab): differentiate flush outcomes + foreground keepalive=false per Codex review (round 8) Two findings from round 8: 1) [P1] runCollabFlush returned `false` for both PATCH failure AND dedupe-skip. The rich→raw toggle treated `false` as "didn't flush" and didn't seed rawSeedMarkdown — but a dedupe means items.content already matches our markdown (the prior successful flush put it there). Raw mode then seeded from the page's stale `item.content` field, and a subsequent raw save could overwrite the current server content with the pre-collab snapshot. Fix: change runCollabFlush's return type to a discriminated string: 'flushed' | 'deduped' | 'failed'. The toggle treats 'flushed' and 'deduped' equivalently for seeding (both mean "server has this markdown") and only bails on 'failed'. 2) [P2] The toggle path used keepalive=true for the awaited flush. Browser keepalive requests can reject for bodies larger than the per-origin keepalive quota (~64KB). On reject, the catch silently fell through and raw mode activated with rawSeedMarkdown null. Fix: switch the toggle path to keepalive=false. The await is synchronous and user-initiated; navigation isn't imminent, so the keepalive escape hatch isn't needed (and risks losing the explicit save). Also added an `aborted` short-circuit so a 'failed' result returns early WITHOUT entering raw mode — user can retry. Cleanup-driven flushes (which DO need to survive page lifecycle) still use keepalive=true. |
||
|
xarmian
|
5dc42b60df |
feat(collab): wire Yjs WebSocket provider + Y.Doc lifecycle (TASK-1259) (#457)
* feat(collab): wire Yjs WebSocket provider + Y.Doc lifecycle (TASK-1259)
Adds a thin y-websocket-style provider speaking the binary protocol
already implemented server-side in internal/collab/room.go. The
provider lives in a Svelte 5 .svelte.ts module so connection state
(`connected`, `synced`) can be consumed reactively by upcoming UX
tasks (TASK-1264 pending-sync indicator, TASK-1265 mobile reconnect).
Wire format mirrors the server's first-byte discriminator:
0x00 → y-protocols/sync (persisted to op-log + broadcast)
0x01 → y-protocols/awareness (broadcast only, ephemeral)
Lifecycle is bound to the item-detail page via $effect keyed on
`${item.id}:${canEdit}` — same key the <Editor> already re-mounts on,
so the Y.Doc and provider tear down in lockstep with the editor.
View-only viewers (canEdit === false) keep the legacy non-collab
editor; their read-only y-binding is deferred to TASK-1266.
Reconnect uses 1s/2s/4s/...30s exponential backoff. Sophisticated
mobile reconnect (visibility, network state) is TASK-1265.
KNOWN TEMPORARY REGRESSION: existing items with non-empty
items.content render an empty editor on first open under collab,
because the Y.Doc starts empty and TASK-1259 doesn't seed from
markdown. TASK-1261 (next in Phase 2) adds the lazy seed-after-
initial-sync path. New items + items already round-tripped through
collab are unaffected.
Drive-by lint cleanup of dead code that escaped Phase 1's
make-install-skips-lint loophole:
- gofmt -w on internal/collab/{applier,bus,manager}.go
- removed unused test/debug helpers Room.peerCount and
Room.applierConnCount (re-add with real callers when needed)
Parent: PLAN-1248
* fix(collab): gate Editor mount on ydoc + handle applier_request + catch-up state per Codex review (round 1)
Three findings from round 1:
1) [P1] $effect constructs ydoc AFTER Editor's onMount runs, so the
first mount on an editable item registered StarterKit history
instead of the Collaboration extension. The {#key} excluded ydoc,
so the editor never re-mounted when ydoc later became truthy →
editable users got a non-collab editor while the provider connected
to an unused Y.Doc.
Fix: gate the editable Editor mount on `ydoc` being ready
(`{#if !canEdit} ... {:else if ydoc} ...`). Adds at most one
reactive tick of delay; guarantees the first mount has the binding
registered.
2) [P1] Provider dropped non-binary WebSocket frames, but the server
sends `applier_request` as TextMessage. With TASK-1259 minting
active rooms, every concurrent CLI/MCP/API content PATCH would
sit blocked for 30s waiting for an ack, then fall back to a
direct write — and the in-memory Y.Doc would still hold stale
state and clobber it on the next 5s flush. Silent data loss.
Fix: parse TextMessage frames as JSON ControlMessage. On
`applier_request`, invoke an `onApplierRequest` callback (the
page passes `editor.commands.setContent(markdown)`) and send
`applier_ack` on success. The ExpiresAtMillis-driven late-apply
guard remains TASK-1262's full scope.
3) [P2] Local Y.Doc updates were silently dropped if the socket was
closed when handleDocUpdate fired. On reconnect the dumb-relay
server can't reconstruct missing updates from a state vector, so
any edits made before the first open or during a disconnect
could be lost.
Fix: after sending syncStep1 in onOpen, also send the current
doc state as a single update via `Y.encodeStateAsUpdate(ydoc)`.
CRDT idempotency makes this safe on initial open (server already
has these ops via op-log replay → sees a no-op update). Larger
docs incur a one-time cost on each connection; TASK-1265's
mobile-reconnect work can replace this with a buffered queue.
* fix(collab): destroy provider during rawMode + enforce ExpiresAtMillis on applier requests per Codex review (round 2)
Two findings from round 2:
1) [P1] collabKey ignored rawMode, leaving the WS provider connected
while the user edited via RawMarkdownEditor. Raw saves bypass the
y-binding (PATCH writes items.content directly), but the server
sees an active room → routes the PATCH through the applier flow
→ no editor mounted → 30s timeout fallback → direct write. The
stale Y.Doc still in memory then overwrote the raw save on the
next 5s flush after toggling back.
Fix: include rawMode in the collabKey derivation so toggling raw
destroys the provider (and the in-memory Y.Doc), and toggling back
mints a fresh pair that re-seeds from the op-log + TASK-1261's
lazy markdown seed.
2) [P1] Provider passed expires_at_millis to the handler but never
gated on it. A backgrounded tab that wakes after the server
retried or fell back could still apply setContent and overwrite
newer peer edits.
Fix: enforce the expiry in CollabProvider — check before
invoking the handler AND re-check before acking (handlers are
awaited and could span the deadline). Suppress the ack if either
gate trips; the server interprets "no ack" as "applier
unavailable" and falls back cleanly.
* fix(collab): prune op-log on direct-write fallback + pre-mutation expiry check per Codex review (round 3)
Two findings from round 3:
1) [P1] rawMode toggle to/from rich left a stale op-log: raw saves
wrote items.content directly while the destroyed provider's old
op-log persisted. Toggling back minted a fresh Y.Doc that
replayed the old log → showed pre-raw content → silently
overwrote the raw save on the next 5s flush.
Fix server-side: when ApplyExternalContent returns ErrNoActiveRoom
(no peers in memory, no in-flight Y.Doc state to corrupt), prune
the op-log alongside the direct items.content write so future
collab sessions start from a clean slate seeded by items.content
(TASK-1261's lazy seed). Pruning is intentionally NOT applied to
ErrNoApplierAvailable / ErrAllAppliersTimedOut — those paths
may have live peers whose Y.Doc state would diverge.
2) [P2] Provider's post-handler expiry check only suppressed the
ack, not the actual setContent mutation owned by the page
handler. An async handler that crossed the deadline could still
write stale markdown into the Y.Doc.
Fix: page handler now does its own pre-mutation expiry check
inside onApplierRequest before calling setContent. Documented
the contract on ApplierRequestHandler — handlers MUST honour
expiresAtMillis BEFORE mutating state.
* fix(collab): prune op-log on grace-TTL applier-unavailable + suppress autosave when collab active per Codex review (round 4)
Two findings from round 4:
1) [HIGH] op-log pruning still skipped ErrNoApplierAvailable. When
raw-mode destroys the in-tab provider, the room remains in its
60s grace TTL with zero conns, so the next direct-write PATCH
returns ErrNoApplierAvailable (not ErrNoActiveRoom). Stale op-log
rows persisted; toggling back within the grace window resurrected
pre-raw-save Y.Doc state.
Fix: prune op-log on ErrNoApplierAvailable too — the "no live
conns" condition makes pruning safe (no peers to corrupt).
ErrAllAppliersTimedOut still preserves op-log because peers may
still be alive there.
2) [HIGH] Once the WS provider is active the legacy 1.2s content
autosave PATCH gets intercepted by the applier path
(handleUpdateItem branch added in TASK-1252). On applier success
input.Content is nil'd out, so UpdateItem never writes the
markdown snapshot. The page's autosave was the only canonical
items.content flush in this diff — search / share-page / API
consumers would see stale content forever.
Fix: short-circuit handleContentUpdate when collabProvider is
set. The Y.Doc + op-log are canonical; items.content stays at
its pre-collab snapshot until TASK-1260 introduces the proper
5s idle flush with applier-bypass semantics. This is a known
Phase-2-internal regression closed by the very next task in
this run.
* fix(collab): tighten error classification + per-item lock + raw-mode flush per Codex review (round 5)
Three findings from round 5:
1) [HIGH] applier.go could return ErrAllAppliersTimedOut even when
no applier_request was ever successfully written (a row of write
failures followed by no remaining candidates). The handler-side
prune skipped that case, leaving stale op-log rows even though
no peer received the request.
Fix: track `anyWriteSucceeded` across the attempts and return
ErrNoApplierAvailable (which prunes) when the loop exits without
ever putting bytes on the wire.
2) [HIGH] Race between ApplyExternalContent's no-room classification
and the subsequent Prune/UpdateItem: a fresh Join could mint a
room and replay the soon-to-be-pruned op-log into a new client,
leaving it with stale Y.Doc state that overwrites the
freshly-written items.content on the next idle flush.
Fix: introduce per-item setup mutex on RoomManager. Join holds
the lock across addConn + replayTo and releases it before the
long-lived readLoop. New PruneAndApply method wraps the
prune+direct-write in the same per-item lock and re-verifies
"no live peers" under it (returns ErrRoomActiveDuringPrune if a
peer slipped in, in which case the caller falls through to a
plain direct write without pruning). Lock order: per-item lock
> m.mu > r.mu — Join and PruneAndApply both follow it.
3) [MEDIUM] Raw-mode 1.2s debounce timer could outlive the toggle
to rich mode: the deferred PATCH fired post-collab-mint and got
routed through the applier path (potentially overwriting newer
peer state).
Fix: track the latest pending raw markdown in
`rawPendingMarkdown`. The Rich-mode button is now an async
onclick that awaits a `flushRawIfPending()` synchronous PATCH
before flipping `rawMode = false` (which is what activates the
collab provider via the collabKey derivation).
* fix(collab): evict broken applier conn + retry on prune-race + retain raw pending on PATCH failure per Codex review (round 6)
Three findings from round 6:
1) [HIGH] When applier_request write failed, the broken roomConn
stayed in r.conns, defeating PruneAndApply's "no live peers"
check (which then returned ErrRoomActiveDuringPrune and the
handler skipped pruning). Net effect: the prune-safety
classification reverted to the round-5 hazard.
Fix: in the applier write-failure branch, force-close the conn
and call removeConn before continuing to the next applier. Both
are idempotent with the readLoop's natural cleanup path
(bus.Unsubscribe, conn map delete, conn.Close all tolerate
double-invocation).
2) [HIGH] On ErrRoomActiveDuringPrune the handler fell through to a
plain direct-write to items.content, bypassing the now-active
peer's applier. The peer's stale Y.Doc could still overwrite
items.content on the next idle flush.
Fix: surface ErrRoomActiveDuringPrune from
applyContentViaCollabOnce so the new applyContentViaCollab
wrapper can retry the full ApplyExternalContent flow against
the freshly-active room. Capped at applyContentMaxRetries=3 to
prevent runaway loops if joins keep landing during prune
attempts. After exhaustion, returns the same sentinel — the
handler's existing `if err == nil { input.Content = nil }`
gate falls through to direct write, which is the correct
degraded-mode behavior.
3) [MEDIUM] flushRawIfPending cleared rawPendingMarkdown before
the PATCH succeeded and the Rich-mode toggle always set
rawMode = false regardless of flush outcome. A failed flush
could activate collab with unsaved raw edits.
Fix: rework flushRawIfPending to return success bool, retain
rawPendingMarkdown on PATCH failure, and gate the Rich-button
transition on `ok`. Added a re-entrancy guard
(rawFlushInFlight) so a rapid double-click waits for the
in-flight flush to settle instead of issuing a duplicate PATCH.
* fix(collab): drain-loop flushRawIfPending to handle fast-typist edge per Codex review (round 7)
[P1] flushRawIfPending snapshotted rawPendingMarkdown then awaited
the PATCH; if the user typed during the await, the equality check
preserved the newer edit but the function still returned `true` and
the Rich-mode handler flipped collab on. The newly-active provider
then raced the un-flushed pending raw save — exactly the hazard
the guard is meant to close.
Fix: rework flushRawIfPending into a bounded drain loop. Each
iteration snapshots-PATCHes-clears (with the equality check). The
loop runs up to RAW_FLUSH_DRAIN_CAP=5 iterations, returning `true`
ONLY when rawPendingMarkdown is null on exit AND no PATCH failed.
A fast typist who keeps the queue non-null across the cap returns
`false`, leaving the user in raw mode (next click retries).
PATCH failure short-circuits with `false` so the toggle stays in
raw mode and the unsaved markdown is preserved for retry.
* fix(collab): atomic prune+content-write + preserve newer raw edit on stale PATCH response per Codex review (round 8)
Two findings from round 8:
1) [P1] PruneAndApply ran the op-log prune under the per-item lock
but the items.content write happened later in the post-loop
UpdateItem call, OUTSIDE the lock. A fresh Join landing in that
gap could replay the now-empty op-log, mint a peer with stale
Y.Doc state, and then overwrite the freshly-written
items.content on the next idle flush.
Fix: applyContentViaCollab now takes a `directWrite` callback
that the caller (handleUpdateItem) implements as a content-only
UpdateItem. PruneAndApply's applyFn invokes it AFTER the prune
so both run inside the same per-item critical section. The
trade-off is two DB round-trips when a PATCH carries content +
other fields together (rare): the content-only update happens
inside the lock; the rest (title, fields, status) flows through
the post-loop UpdateItem with input.Content nil'd to suppress
the duplicate write.
2) [P1] In flushRawIfPending's drain loop, `item = updated`
assigned the server-side snapshot from the just-PATCHed
markdown even when a newer raw edit had landed in the meantime.
RawMarkdownEditor mirrors `item.content` into its textarea
unconditionally (line 16), so the stale assignment would reset
the textarea mid-keystroke and lose the queued edit.
Fix: only swap in the full updated snapshot when
`rawPendingMarkdown === markdown` (no newer edit). Otherwise
keep our local content and adopt only the server-side metadata
(timestamps, version, modified_by) via spread.
* fix(collab): atomic mixed PATCH + raw autosave stale guard + rich→raw seeding per Codex review (round 9)
Three findings from round 9:
1) [P1] Toggling FROM rich+collab TO raw mode seeded
RawMarkdownEditor from items.content, which is intentionally
stale under collab (handleContentUpdate is suppressed while the
provider is connected; TASK-1260 closes that gap with a 5s
flush). Saving from raw mode would overwrite the live Y.Doc
state with a pre-collab snapshot.
Fix: when toggling to raw with a connected provider, capture
the editor's current Y.Doc-derived markdown via
`editor.storage.markdown.getMarkdown()` into a one-shot
`rawSeedMarkdown` slot and pre-populate `rawPendingMarkdown` so
the first auto-save persists it. RawMarkdownEditor seeds from
`rawSeedMarkdown ?? item.content`. Cleared on rich-mode toggle.
2) [P1] The regular debounced raw autosave still assigned
`item = updated` from a stale PATCH response. Same
stale-snapshot hazard the Round 8 fix closed in
flushRawIfPending.
Fix: equality-check `rawPendingMarkdown === toSave` before
swapping in the server snapshot. On stale, keep local content
and adopt only the server-side metadata via spread.
3) [P2] Round 8 split the items.content write (under per-item
lock) from the rest of UpdateItem (post-loop), losing
atomicity for mixed PATCHes (content + title) and breaking
Store.UpdateItem's content-versioning peek at Title.
Fix: directWrite callback now invokes the FULL UpdateItem
inside the per-item lock. A `fullWriteHandled` flag tells the
handler to skip the post-loop UpdateItem entirely (otherwise
we'd duplicate the write and create two version-history rows).
Mixed PATCHes are atomic again under the lock.
* fix(collab): clear raw seed/pending on item navigation per Codex review (round 10)
[P1] Navigating between items left rawSeedMarkdown,
rawPendingMarkdown, and the contentDebounceTimer set from the
previous item. This caused two concrete hazards:
(a) Item B's raw editor mounted with item A's live markdown via
`rawSeedMarkdown ?? item.content`.
(b) Clicking Rich on item B fired flushRawIfPending which
PATCHed A's queued markdown INTO item B (cross-item data
bleed).
Fix: at the top of loadData(), clear contentDebounceTimer,
rawSeedMarkdown, and rawPendingMarkdown so each navigation starts
from a clean slate. The collab provider's own lifecycle is
already keyed on item.id via $effect cleanup, so it doesn't need
the same explicit reset.
* fix(collab): item-id race guard on raw PATCH responses per Codex review (round 11)
[P1] In-flight raw PATCH responses (debounced autosave AND drain
loop) could clobber a newly navigated item. Clearing
contentDebounceTimer in loadData only cancels timers that have
not fired; an awaiting fetch keeps running and its `.then` /
`.catch` would assign back to the new page's `item` state.
Fix: mirror the existing TASK-754-style race guard pattern
(already used in the SSE / sync handlers above). Capture
`reqItemId = item.id` BEFORE the PATCH, then in the response
handler bail if `!item || item.id !== reqItemId`. Applied to
both handleRawContentUpdate's setTimeout body and
flushRawIfPending's drain loop.
* fix(collab): reset saveStatus on item navigation per Codex review (round 12)
[P2] After Round 11's race guard, a stale raw PATCH response that
matched a now-different item.id was correctly discarded — but
saveStatus had already been set to 'saving' before the await. With
loadData not resetting it, the next item could mount with
saveStatus pinned at 'saving' indefinitely, which then suppressed
all SSE/sync refreshes via the `if (saveStatus === 'saving')`
guards above.
Fix: in loadData's per-item state reset, clear saveStatusTimer
and reset saveStatus to 'idle' alongside the other transient
state. Cheap, scoped, no impact on the in-flight save's eventual
discard path.
|